Skip to content

Commit

Permalink
dryRun, Adapt VM Mac Address allocation to support dryRun (#327)
Browse files Browse the repository at this point in the history 
* dryRun, Adapt VM Mac Address allocation to support dryRun

In case of "server" dryRun ("All" if used by controller runtime)
we do want to return an updated server response,
which allocates Mac Address, but without doing side effects.
Fix the flow to support it.

Note that the Mac pool head is changed, so calling twice dryRun
won't yield the same Mac Address in the response.
It is needed because in case there are multiple Mac Addresses in
the same request, we should not give the same Mac even on dryRun,
hence the mac pool head should be updated.

Signed-off-by: Or Shoval <[email protected]>

* tests, Add vm dry run tests

Signed-off-by: Or Shoval <[email protected]>

* dryRun, Adapt pod allocation to dryRun

Signed-off-by: Or Shoval <[email protected]>
  • Loading branch information
@oshoval
oshoval authored Aug 23, 2021
1 parent 54a6a16 commit 4cdd268
Show file tree
Hide file tree
Showing 7 changed files with 155 additions and 88 deletions.
2 changes: 1 addition & 1 deletion pkg/controller/pod/pod_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ func (r *ReconcilePolicy) Reconcile(ctx context.Context, request reconcile.Reque
return reconcile.Result{}, nil
}

err = r.poolManager.AllocatePodMac(instance)
err = r.poolManager.AllocatePodMac(instance, true)
if err != nil {
logger.Error(err, "failed to allocate mac for pod")
}
Expand Down
40 changes: 25 additions & 15 deletions pkg/pool-manager/pod_pool.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import (

const tempPodName = "tempPodName"

func (p *PoolManager) AllocatePodMac(pod *corev1.Pod) error {
func (p *PoolManager) AllocatePodMac(pod *corev1.Pod, isNotDryRun bool) error {
p.poolMutex.Lock()
defer p.poolMutex.Unlock()

Expand Down Expand Up @@ -74,15 +74,15 @@ func (p *PoolManager) AllocatePodMac(pod *corev1.Pod) error {
networkList := []*multus.NetworkSelectionElement{}
for _, network := range networks {
if network.MacRequest != "" {
if err := p.allocatePodRequestedMac(network, pod); err != nil {
p.revertAllocationOnPod(podFullName, newAllocations)
if err := p.allocatePodRequestedMac(network, pod, isNotDryRun); err != nil {
p.revertAllocationOnPod(podFullName, newAllocations, isNotDryRun)
return err
}
newAllocations[network.Name] = network.MacRequest
} else {
macAddr, err := p.allocatePodFromPool(network, pod)
macAddr, err := p.allocatePodFromPool(network, pod, isNotDryRun)
if err != nil {
p.revertAllocationOnPod(podFullName, newAllocations)
p.revertAllocationOnPod(podFullName, newAllocations, isNotDryRun)
return err
}

Expand Down Expand Up @@ -129,7 +129,7 @@ func (p *PoolManager) ReleaseAllPodMacs(podFullName string) error {
return nil
}

func (p *PoolManager) allocatePodRequestedMac(network *multus.NetworkSelectionElement, pod *corev1.Pod) error {
func (p *PoolManager) allocatePodRequestedMac(network *multus.NetworkSelectionElement, pod *corev1.Pod, isNotDryRun bool) error {
requestedMac := network.MacRequest

if _, err := net.ParseMAC(requestedMac); err != nil {
Expand All @@ -141,7 +141,9 @@ func (p *PoolManager) allocatePodRequestedMac(network *multus.NetworkSelectionEl
// the pod name may have not been updated in the webhook context yet,
// so we use a temp pod name and update it the the controller
podFullName = tempPodName
p.macPoolMap.createOrUpdateEntry(requestedMac, podFullName, network.Name)
if isNotDryRun {
p.macPoolMap.createOrUpdateEntry(requestedMac, podFullName, network.Name)
}
return nil
}
if macEntry, exist := p.macPoolMap[requestedMac]; exist {
Expand All @@ -153,16 +155,17 @@ func (p *PoolManager) allocatePodRequestedMac(network *multus.NetworkSelectionEl
}
}

p.macPoolMap.createOrUpdateEntry(requestedMac, podFullName, network.Name)

if isNotDryRun {
p.macPoolMap.createOrUpdateEntry(requestedMac, podFullName, network.Name)
}
log.Info("requested mac was allocated for pod",
"requestedMap", requestedMac,
"podFullName", podFullName)

return nil
}

func (p *PoolManager) allocatePodFromPool(network *multus.NetworkSelectionElement, pod *corev1.Pod) (string, error) {
func (p *PoolManager) allocatePodFromPool(network *multus.NetworkSelectionElement, pod *corev1.Pod, isNotDryRun bool) (string, error) {
macAddr, err := p.getFreeMac()
if err != nil {
return "", err
Expand All @@ -173,12 +176,15 @@ func (p *PoolManager) allocatePodFromPool(network *multus.NetworkSelectionElemen
// the pod name may have not been updated in the webhook context yet,
// so we use a temp pod name and update it the the controller
podFullName = tempPodName
p.macPoolMap.createOrUpdateEntry(macAddr.String(), podFullName, network.Name)
if isNotDryRun {
p.macPoolMap.createOrUpdateEntry(macAddr.String(), podFullName, network.Name)
}
return macAddr.String(), nil
}

p.macPoolMap.createOrUpdateEntry(macAddr.String(), podFullName, network.Name)

if isNotDryRun {
p.macPoolMap.createOrUpdateEntry(macAddr.String(), podFullName, network.Name)
}
log.Info("mac from pool was allocated to the pod",
"allocatedMac", macAddr.String(),
"podFullName", podFullName)
Expand Down Expand Up @@ -253,7 +259,7 @@ func (p *PoolManager) initPodMap() error {
continue
}

if err := p.allocatePodRequestedMac(network, &pod); err != nil {
if err := p.allocatePodRequestedMac(network, &pod, true); err != nil {
// Dont return an error here if we can't allocate a mac for a running pod
log.Error(fmt.Errorf("failed to parse mac address for pod"),
"Invalid mac address for pod",
Expand Down Expand Up @@ -285,7 +291,11 @@ func macAlreadyBelongsToPodAndNetwork(podFullName, networkName string, macEntry
}

// Revert allocation if one of the requested mac addresses fails to be allocated
func (p *PoolManager) revertAllocationOnPod(podFullName string, allocations map[string]string) {
func (p *PoolManager) revertAllocationOnPod(podFullName string, allocations map[string]string, isNotDryRun bool) {
if isNotDryRun == false {
return
}

log.V(1).Info("Revert vm allocation", "podFullName", podFullName, "allocations", allocations)
for _, macAddress := range allocations {
delete(p.macPoolMap, macAddress)
Expand Down
66 changes: 43 additions & 23 deletions pkg/pool-manager/pool_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -355,7 +355,7 @@ var _ = Describe("Pool", func() {
newVM.Name = "newVM"

transactionTimestamp := updateTransactionTimestamp(0)
err := poolManager.AllocateVirtualMachineMac(&newVM, &transactionTimestamp, logger)
err := poolManager.AllocateVirtualMachineMac(&newVM, &transactionTimestamp, true, logger)
Expect(err).ToNot(HaveOccurred())
Expect(poolManager.macPoolMap).To(BeEmpty(), "Should not allocate mac for unsupported bridge binding")
})
Expand All @@ -368,7 +368,7 @@ var _ = Describe("Pool", func() {
newVM := masqueradeVM
newVM.Name = "newVM"
transactionTimestamp := updateTransactionTimestamp(0)
err := poolManager.AllocateVirtualMachineMac(&newVM, &transactionTimestamp, logger)
err := poolManager.AllocateVirtualMachineMac(&newVM, &transactionTimestamp, true, logger)
Expect(err).ToNot(HaveOccurred())

Expect(poolManager.macPoolMap).To(HaveLen(2))
Expand All @@ -385,7 +385,7 @@ var _ = Describe("Pool", func() {
newVM.Name = "newVM"

transactionTimestamp := updateTransactionTimestamp(0)
err := poolManager.AllocateVirtualMachineMac(newVM, &transactionTimestamp, logger)
err := poolManager.AllocateVirtualMachineMac(newVM, &transactionTimestamp, true, logger)
Expect(err).ToNot(HaveOccurred())

Expect(poolManager.macPoolMap).To(HaveLen(3))
Expand All @@ -412,14 +412,14 @@ var _ = Describe("Pool", func() {

addDiskIO(newVM, "native-new")
transactionTimestamp := updateTransactionTimestamp(0)
err := poolManager.AllocateVirtualMachineMac(newVM, &transactionTimestamp, logger)
err := poolManager.AllocateVirtualMachineMac(newVM, &transactionTimestamp, true, logger)
Expect(err).ToNot(HaveOccurred())
Expect(newVM.Spec.Template.Spec.Domain.Devices.Disks[0].IO).To(Equal(kubevirt.DriverIO("native-new")), "disk.io configuration must be preserved after mac allocation")

updateVm := multipleInterfacesVM.DeepCopy()
updateVm.Name = "newVM"
addDiskIO(updateVm, "native-update")
err = poolManager.UpdateMacAddressesForVirtualMachine(newVM, updateVm, &transactionTimestamp, logger)
err = poolManager.UpdateMacAddressesForVirtualMachine(newVM, updateVm, &transactionTimestamp, true, logger)
Expect(err).ToNot(HaveOccurred())
Expect(updateVm.Spec.Template.Spec.Domain.Devices.Disks[0].IO).To(Equal(kubevirt.DriverIO("native-update")), "disk.io configuration must be preserved after mac allocation update")
})
Expand All @@ -428,7 +428,7 @@ var _ = Describe("Pool", func() {
newVM := multipleInterfacesVM.DeepCopy()
newVM.Name = "newVM"
transactionTimestamp := updateTransactionTimestamp(0)
err := poolManager.AllocateVirtualMachineMac(newVM, &transactionTimestamp, logger)
err := poolManager.AllocateVirtualMachineMac(newVM, &transactionTimestamp, true, logger)
Expect(err).ToNot(HaveOccurred())
Expect(newVM.Spec.Template.Spec.Domain.Devices.Interfaces[0].MacAddress).To(Equal("02:00:00:00:00:00"))
Expect(newVM.Spec.Template.Spec.Domain.Devices.Interfaces[1].MacAddress).To(Equal("02:00:00:00:00:01"))
Expand All @@ -438,7 +438,7 @@ var _ = Describe("Pool", func() {
updateVm := multipleInterfacesVM.DeepCopy()
updateVm.Name = "newVM"
newTransactionTimestamp := updateTransactionTimestamp(1)
err = poolManager.UpdateMacAddressesForVirtualMachine(newVM, updateVm, &newTransactionTimestamp, logger)
err = poolManager.UpdateMacAddressesForVirtualMachine(newVM, updateVm, &newTransactionTimestamp, true, logger)
Expect(err).ToNot(HaveOccurred())
Expect(updateVm.Spec.Template.Spec.Domain.Devices.Interfaces[0].MacAddress).To(Equal("02:00:00:00:00:00"))
Expect(updateVm.Spec.Template.Spec.Domain.Devices.Interfaces[1].MacAddress).To(Equal("02:00:00:00:00:01"))
Expand All @@ -450,7 +450,7 @@ var _ = Describe("Pool", func() {
newVM.Name = "newVM"

transactionTimestamp := updateTransactionTimestamp(0)
err := poolManager.AllocateVirtualMachineMac(newVM, &transactionTimestamp, logger)
err := poolManager.AllocateVirtualMachineMac(newVM, &transactionTimestamp, true, logger)
Expect(err).ToNot(HaveOccurred())
Expect(newVM.Spec.Template.Spec.Domain.Devices.Interfaces[0].MacAddress).To(Equal("02:00:00:00:00:00"))
Expect(newVM.Spec.Template.Spec.Domain.Devices.Interfaces[1].MacAddress).To(Equal("02:00:00:00:00:01"))
Expand All @@ -462,7 +462,7 @@ var _ = Describe("Pool", func() {
By("changing one of the macs")
updateVm.Spec.Template.Spec.Domain.Devices.Interfaces[1].MacAddress = "01:00:00:00:00:02"
newTransactionTimestamp := updateTransactionTimestamp(1)
err = poolManager.UpdateMacAddressesForVirtualMachine(newVM, updateVm, &newTransactionTimestamp, logger)
err = poolManager.UpdateMacAddressesForVirtualMachine(newVM, updateVm, &newTransactionTimestamp, true, logger)
Expect(err).ToNot(HaveOccurred())
Expect(updateVm.Spec.Template.Spec.Domain.Devices.Interfaces[0].MacAddress).To(Equal("02:00:00:00:00:00"))
Expect(updateVm.Spec.Template.Spec.Domain.Devices.Interfaces[1].MacAddress).To(Equal("01:00:00:00:00:02"))
Expand All @@ -474,7 +474,7 @@ var _ = Describe("Pool", func() {
newVM.Name = "newVM"

transactionTimestamp := updateTransactionTimestamp(0)
err := poolManager.AllocateVirtualMachineMac(newVM, &transactionTimestamp, logger)
err := poolManager.AllocateVirtualMachineMac(newVM, &transactionTimestamp, true, logger)
Expect(err).ToNot(HaveOccurred())
Expect(newVM.Spec.Template.Spec.Domain.Devices.Interfaces[0].MacAddress).To(Equal("02:00:00:00:00:00"))
Expect(newVM.Spec.Template.Spec.Domain.Devices.Interfaces[1].MacAddress).To(Equal("02:00:00:00:00:01"))
Expand All @@ -486,7 +486,7 @@ var _ = Describe("Pool", func() {
updatedVM.Spec.Template.Spec.Domain.Devices.Interfaces = append(updatedVM.Spec.Template.Spec.Domain.Devices.Interfaces, anotherMultusBridgeInterface)
updatedVM.Spec.Template.Spec.Networks = append(updatedVM.Spec.Template.Spec.Networks, anotherMultusNetwork)
NewTransactionTimestamp := updateTransactionTimestamp(1)
err = poolManager.UpdateMacAddressesForVirtualMachine(newVM, updatedVM, &NewTransactionTimestamp, logger)
err = poolManager.UpdateMacAddressesForVirtualMachine(newVM, updatedVM, &NewTransactionTimestamp, true, logger)
Expect(err).ToNot(HaveOccurred())
Expect(updatedVM.Spec.Template.Spec.Domain.Devices.Interfaces[0].MacAddress).To(Equal("02:00:00:00:00:00"))
Expect(updatedVM.Spec.Template.Spec.Domain.Devices.Interfaces[1].MacAddress).To(Equal("02:00:00:00:00:01"))
Expand All @@ -501,7 +501,7 @@ var _ = Describe("Pool", func() {
newVM.Spec.Template.Spec.Networks = append(newVM.Spec.Template.Spec.Networks, anotherMultusNetwork)

transactionTimestamp := updateTransactionTimestamp(0)
err := poolManager.AllocateVirtualMachineMac(newVM, &transactionTimestamp, logger)
err := poolManager.AllocateVirtualMachineMac(newVM, &transactionTimestamp, true, logger)
Expect(err).ToNot(HaveOccurred())
Expect(newVM.Spec.Template.Spec.Domain.Devices.Interfaces[0].MacAddress).To(Equal("02:00:00:00:00:00"))
Expect(newVM.Spec.Template.Spec.Domain.Devices.Interfaces[1].MacAddress).To(Equal("02:00:00:00:00:01"))
Expand All @@ -513,7 +513,7 @@ var _ = Describe("Pool", func() {
updatedVM.Spec.Template.Spec.Domain.Devices.Interfaces[0].MacAddress = newVM.Spec.Template.Spec.Domain.Devices.Interfaces[0].MacAddress
updatedVM.Spec.Template.Spec.Domain.Devices.Interfaces[1].MacAddress = newVM.Spec.Template.Spec.Domain.Devices.Interfaces[1].MacAddress
NewTransactionTimestamp := updateTransactionTimestamp(1)
err = poolManager.UpdateMacAddressesForVirtualMachine(newVM, updatedVM, &NewTransactionTimestamp, logger)
err = poolManager.UpdateMacAddressesForVirtualMachine(newVM, updatedVM, &NewTransactionTimestamp, true, logger)
Expect(err).ToNot(HaveOccurred())
Expect(updatedVM.Spec.Template.Spec.Domain.Devices.Interfaces[0].MacAddress).To(Equal("02:00:00:00:00:00"))
Expect(updatedVM.Spec.Template.Spec.Domain.Devices.Interfaces[1].MacAddress).To(Equal("02:00:00:00:00:01"))
Expand All @@ -525,7 +525,7 @@ var _ = Describe("Pool", func() {
newVM.Name = "newVM"

transactionTimestamp := updateTransactionTimestamp(0)
err := poolManager.AllocateVirtualMachineMac(newVM, &transactionTimestamp, logger)
err := poolManager.AllocateVirtualMachineMac(newVM, &transactionTimestamp, true, logger)
Expect(err).ToNot(HaveOccurred())
Expect(newVM.Spec.Template.Spec.Domain.Devices.Interfaces[0].MacAddress).To(Equal("02:00:00:00:00:00"))
Expect(newVM.Spec.Template.Spec.Domain.Devices.Interfaces[1].MacAddress).To(Equal("02:00:00:00:00:01"))
Expand All @@ -538,7 +538,7 @@ var _ = Describe("Pool", func() {
updatedVM.Spec.Template.Spec.Domain.Devices.Interfaces = append(updatedVM.Spec.Template.Spec.Domain.Devices.Interfaces, anotherMultusBridgeInterface)
updatedVM.Spec.Template.Spec.Networks = append(updatedVM.Spec.Template.Spec.Networks, anotherMultusNetwork)
NewTransactionTimestamp := updateTransactionTimestamp(1)
err = poolManager.UpdateMacAddressesForVirtualMachine(newVM, updatedVM, &NewTransactionTimestamp, logger)
err = poolManager.UpdateMacAddressesForVirtualMachine(newVM, updatedVM, &NewTransactionTimestamp, true, logger)
Expect(err).ToNot(HaveOccurred())
Expect(updatedVM.Spec.Template.Spec.Domain.Devices.Interfaces[0].MacAddress).To(Equal("02:00:00:00:00:00"))
Expect(updatedVM.Spec.Template.Spec.Domain.Devices.Interfaces[1].MacAddress).To(Equal("02:00:00:00:00:02"))
Expand Down Expand Up @@ -568,7 +568,7 @@ var _ = Describe("Pool", func() {
vm = masqueradeVM.DeepCopy()
vm.Name = "testVm"

err := poolManager.AllocateVirtualMachineMac(vm, &vmCreationTimestamp, logger)
err := poolManager.AllocateVirtualMachineMac(vm, &vmCreationTimestamp, true, logger)
Expect(err).ToNot(HaveOccurred())
allocatedMac = vm.Spec.Template.Spec.Domain.Devices.Interfaces[0].MacAddress
macEntry, exist := poolManager.macPoolMap[allocatedMac]
Expand All @@ -592,7 +592,7 @@ var _ = Describe("Pool", func() {
vmFirstUpdate.Spec.Template.Spec.Domain.Devices.Interfaces = vmFirstUpdate.Spec.Template.Spec.Domain.Devices.Interfaces[:0]

By("updating the vm, removing the interface")
err := poolManager.UpdateMacAddressesForVirtualMachine(vm, vmFirstUpdate, &vmFirstUpdateTimestamp, logger)
err := poolManager.UpdateMacAddressesForVirtualMachine(vm, vmFirstUpdate, &vmFirstUpdateTimestamp, true, logger)
Expect(err).ToNot(HaveOccurred(), "should update vm with no error")
macEntry, exist := poolManager.macPoolMap[allocatedMac]
Expect(exist).To(BeTrue(), "mac should be updated in the macPoolMap after first update")
Expand All @@ -615,7 +615,7 @@ var _ = Describe("Pool", func() {
By("updating the vm, re-adding the interface")
vmSecondUpdate = vm.DeepCopy()
By("updating the vm, removing the interface")
err := poolManager.UpdateMacAddressesForVirtualMachine(vmFirstUpdate, vmSecondUpdate, &vmSecondUpdateTimestamp, logger)
err := poolManager.UpdateMacAddressesForVirtualMachine(vmFirstUpdate, vmSecondUpdate, &vmSecondUpdateTimestamp, true, logger)
Expect(err).ToNot(HaveOccurred(), "should update vm with no error")
macEntry, exist := poolManager.macPoolMap[allocatedMac]
Expect(exist).To(BeTrue(), "mac should be updated in the macPoolMap after first update")
Expand Down Expand Up @@ -684,7 +684,7 @@ var _ = Describe("Pool", func() {

By("Create a VM")
transactionTimestamp = updateTransactionTimestamp(0)
err := poolManager.AllocateVirtualMachineMac(newVM, &transactionTimestamp, logger)
err := poolManager.AllocateVirtualMachineMac(newVM, &transactionTimestamp, true, logger)
Expect(err).ToNot(HaveOccurred(), "should successfully allocated macs")

allocatedMac = newVM.Spec.Template.Spec.Domain.Devices.Interfaces[0].MacAddress
Expand All @@ -706,6 +706,26 @@ var _ = Describe("Pool", func() {
Expect(poolManager.macPoolMap).To(HaveLen(1), "macPoolMap should hold the mac address waiting for approval")
Expect(poolManager.macPoolMap[allocatedMac]).To(Equal(expectedMacEntry), "macPoolMap's mac's entry should be as expected")
})
Context("and creating a dry run VM", func() {
var macPoolMapCopy macMap
BeforeEach(func() {
macPoolMapCopy = macMap{}
for key, value := range poolManager.macPoolMap {
macPoolMapCopy[key] = value
}

newVMDryRun := masqueradeVM.DeepCopy()
newVMDryRun.Name = "newVMDryRun"

By("Create a dry run VM")
transactionTimestamp = updateTransactionTimestamp(0)
err := poolManager.AllocateVirtualMachineMac(newVM, &transactionTimestamp, false, logger)
Expect(err).ToNot(HaveOccurred(), "should successfully create dry run VM")
})
It("should not storage the dry run mac in the mac pool", func() {
Expect(poolManager.macPoolMap).To(Equal(macPoolMapCopy), "macPoolMap should left unchanged")
})
})
Context("and VM is marked as ready by controller reconcile", func() {
var lastPersistedtransactionTimstamp *time.Time
BeforeEach(func() {
Expand Down Expand Up @@ -758,7 +778,7 @@ var _ = Describe("Pool", func() {
newPod.Name = "newPod"
newPod.Annotations = beforeAllocationAnnotation

err := poolManager.AllocatePodMac(&newPod)
err := poolManager.AllocatePodMac(&newPod, true)
Expect(err).ToNot(HaveOccurred())
preAllocatedPodMac := "02:00:00:00:00:00"
expectedAllocatedMac := "02:00:00:00:00:01"
Expand All @@ -785,7 +805,7 @@ var _ = Describe("Pool", func() {
newPod := managedPodWithMacAllocated
newPod.Name = "newPod"

err := poolManager.AllocatePodMac(&newPod)
err := poolManager.AllocatePodMac(&newPod, true)
Expect(err).ToNot(HaveOccurred())
Expect(newPod.Annotations[NetworksAnnotation]).To(Equal(afterAllocationAnnotation(managedNamespaceName, "02:00:00:00:00:00")[NetworksAnnotation]))
})
Expand All @@ -806,7 +826,7 @@ var _ = Describe("Pool", func() {
pod.Annotations = map[string]string{NetworksAnnotation: fmt.Sprintf("%s", networkRequestAnnotation)}

By("Request specific mac-address by adding the address to the networks pod annotation")
err := poolManager.AllocatePodMac(&pod)
err := poolManager.AllocatePodMac(&pod, true)
Expect(err).ToNot(HaveOccurred(), "should allocate mac address and ip address correspond to networks annotation")

By("Convert obtained networks annotation JSON to multus.NetworkSelectionElement array")
Expand Down Expand Up @@ -840,7 +860,7 @@ var _ = Describe("Pool", func() {
NetworksAnnotation: `[{"name":"ovs-conf","namespace":"default","ips":"10.10.0.1","mac":"02:00:00:00:00:00"}]`}

By("Request specific mac-address by adding the address to the networks pod annotation")
err := poolManager.AllocatePodMac(&pod)
err := poolManager.AllocatePodMac(&pod, true)
Expect(err).To(HaveOccurred(), "should fail to allocate mac address due to bad annotation format")
})
})
Expand Down
Loading

0 comments on commit 4cdd268

Please sign in to comment.