Add a cleanup loop for failed vm

pkg/controller/virtualmachine/virtualmachine_controller.go

@@ -96,5 +96,6 @@ func (r *ReconcilePolicy) Reconcile(request reconcile.Request) (reconcile.Result
 		return reconcile.Result{}, err
 	}
 
+	r.poolManager.MarkVMAsReady(fmt.Sprintf("%s/%s", request.Namespace, request.Name))
 	return reconcile.Result{}, nil
 }

pkg/pool-manager/pool.go

@@ -35,16 +35,18 @@ const (
 var log = logf.Log.WithName("PoolManager")
 
 type PoolManager struct {
-	kubeClient      kubernetes.Interface         // kubernetes client
-	rangeStart      net.HardwareAddr             // fist mac in range
-	rangeEnd        net.HardwareAddr             // last mac in range
-	currentMac      net.HardwareAddr             // last given mac
-	macPoolMap      map[string]AllocationStatus  // allocated mac map and status
-	podToMacPoolMap map[string]map[string]string // map allocated mac address by networkname and namespace/podname: {"namespace/podname: {"network name": "mac address"}}
-	vmToMacPoolMap  map[string]map[string]string // map for namespace/vmname and a map of interface name with allocated mac address
-	poolMutex       sync.Mutex                   // mutex for allocation an release
-	isLeader        bool                         // leader boolean
-	isKubevirt      bool                         // bool if kubevirt virtualmachine crd exist in the cluster
+	kubeClient        kubernetes.Interface         // kubernetes client
+	rangeStart        net.HardwareAddr             // fist mac in range
+	rangeEnd          net.HardwareAddr             // last mac in range
+	currentMac        net.HardwareAddr             // last given mac
+	macPoolMap        map[string]AllocationStatus  // allocated mac map and status
+	podToMacPoolMap   map[string]map[string]string // map allocated mac address by networkname and namespace/podname: {"namespace/podname: {"network name": "mac address"}}
+	vmToMacPoolMap    map[string]map[string]string // map for namespace/vmname and a map of interface name with allocated mac address
+	vmCreationWaiting map[string]int               // map for namespace/vmname and count to wait for vm creation event
+	vmMutex           sync.Mutex                   // mutex for creation waiting cleanup loop
+	poolMutex         sync.Mutex                   // mutex for allocation an release
+	isLeader          bool                         // leader boolean
+	isKubevirt        bool                         // bool if kubevirt virtualmachine crd exist in the cluster
 }
 
 type AllocationStatus string
@@ -64,21 +66,25 @@ func NewPoolManager(kubeClient kubernetes.Interface, rangeStart, rangeEnd net.Ha
 	copy(currentMac, rangeStart)
 
 	poolManger := &PoolManager{kubeClient: kubeClient,
-		isLeader:        false,
-		isKubevirt:      kubevirtExist,
-		rangeEnd:        rangeEnd,
-		rangeStart:      rangeStart,
-		currentMac:      currentMac,
-		podToMacPoolMap: map[string]map[string]string{},
-		vmToMacPoolMap:  map[string]map[string]string{},
-		macPoolMap:      map[string]AllocationStatus{},
-		poolMutex:       sync.Mutex{}}
+		isLeader:          false,
+		isKubevirt:        kubevirtExist,
+		rangeEnd:          rangeEnd,
+		rangeStart:        rangeStart,
+		currentMac:        currentMac,
+		podToMacPoolMap:   map[string]map[string]string{},
+		vmToMacPoolMap:    map[string]map[string]string{},
+		macPoolMap:        map[string]AllocationStatus{},
+		vmCreationWaiting: map[string]int{},
+		vmMutex:           sync.Mutex{},
+		poolMutex:         sync.Mutex{}}
 
 	err = poolManger.InitMaps()
 	if err != nil {
 		return nil, err
 	}
 
+	go poolManger.vmWaitingCleanupLook()
+
 	return poolManger, nil
 }
 

pkg/pool-manager/virtualmachine_pool.go

@@ -18,10 +18,11 @@ package pool_manager
 
 import (
 	"fmt"
-	"k8s.io/apimachinery/pkg/api/errors"
 	"net"
+	"time"
 
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/errors"
 	kubevirt "kubevirt.io/kubevirt/pkg/api/v1"
 )
 
@@ -82,6 +83,7 @@ func (p *PoolManager) AllocateVirtualMachineMac(virtualMachine *kubevirt.Virtual
 	}
 
 	virtualMachine.Spec.Template.Spec.Domain.Devices.Interfaces = copyVM.Spec.Template.Spec.Domain.Devices.Interfaces
+	p.vmCreationWaiting[vmNamespaced(virtualMachine)] = 0
 	return nil
 }
 
@@ -113,6 +115,7 @@ func (p *PoolManager) ReleaseVirtualMachineMac(virtualMachineName string) error
 	}
 
 	delete(p.vmToMacPoolMap, virtualMachineName)
+	delete(p.vmCreationWaiting, virtualMachineName)
 	log.V(1).Info("removed virtual machine from vmToMacPoolMap", "virtualMachineName", virtualMachineName)
 	return nil
 }
@@ -196,6 +199,7 @@ func (p *PoolManager) UpdateMacAddressesForVirtualMachine(virtualMachine *kubevi
 	p.releaseMacAddressesFromInterfaceMap(releaseOldAllocations)
 
 	virtualMachine.Spec.Template.Spec.Domain.Devices.Interfaces = copyVM.Spec.Template.Spec.Domain.Devices.Interfaces
+	p.MarkVMAsReady(vmNamespaced(virtualMachine))
 	return nil
 }
 
@@ -343,11 +347,53 @@ func (p *PoolManager) releaseMacAddressesFromInterfaceMap(allocations map[string
 
 // Revert allocation if one of the requested mac addresses fails to be allocated
 func (p *PoolManager) revertAllocationOnVm(vmName string, allocations map[string]string) {
+	// If the vm is in the vm creation waiting skip the revert
+	// this vm will be clean in the waiting clean loop
+	if _, isExist := p.vmCreationWaiting[vmName]; isExist {
+		return
+	}
+
 	log.V(1).Info("Revert vm allocation", "vmName", vmName, "allocations", allocations)
 	p.releaseMacAddressesFromInterfaceMap(allocations)
 	delete(p.vmToMacPoolMap, vmName)
 }
 
+// This function remove the virtual machine from the waiting list
+// this mean that we got an event about a virtual machine in vm controller loop
+func (p *PoolManager) MarkVMAsReady(vmName string) {
+	p.vmMutex.Lock()
+	defer p.vmMutex.Unlock()
+
+	delete(p.vmCreationWaiting, vmName)
+}
+
+// This function check if there are virtual machines that hits the create
+// mutating webhook but we didn't get the creation event in the controller loop
+// this mean the create was failed by some other mutating or validating webhook
+// so we release the virtual machine
+func (p *PoolManager) vmWaitingCleanupLook() {
+	c := time.Tick(3 * time.Second)
+	for _ = range c {
+		p.vmMutex.Lock()
+
+		for vmName, waitingLookCount := range p.vmCreationWaiting {
+			if waitingLookCount < 2 {
+				p.vmCreationWaiting[vmName] += 1
+				continue
+			}
+
+			log.V(1).Info("found a non existing vm, start releasing", "vmName", vmName)
+			err := p.ReleaseVirtualMachineMac(vmName)
+			if err != nil {
+				log.Error(err, "failed to release virtual machine mac addresses in cleanup look")
+			}
+			delete(p.vmCreationWaiting, vmName)
+		}
+
+		p.vmMutex.Unlock()
+	}
+}
+
 func vmNamespaced(machine *kubevirt.VirtualMachine) string {
 	return fmt.Sprintf("%s/%s", machine.Namespace, machine.Name)
 }

tests/virtual_machines_test.go

@@ -364,6 +364,61 @@ var _ = Describe("Virtual Machines", func() {
 				}
 			})
 		})
+		Context("When we re-apply a failed VM yaml", func() {
+			It("should allow to assign to the VM the same MAC addresses, name as requested before and do not return an error", func() {
+				err := setRange(rangeStart, rangeEnd)
+				Expect(err).ToNot(HaveOccurred())
+
+				vm1 := CreateVmObject(TestNamespace, false,
+					[]kubevirtv1.Interface{newInterface("br1", "02:00:ff:ff:ff:ff")},
+					[]kubevirtv1.Network{newNetwork("br1"), newNetwork("br2")})
+
+				baseVM := vm1.DeepCopy()
+
+				Eventually(func() bool {
+					err := testClient.VirtClient.Create(context.TODO(), vm1)
+					if err != nil && strings.Contains(err.Error(), "every network must be mapped to an interface") {
+						return true
+					}
+					return false
+
+				}, 40*time.Second, 5*time.Second).Should(BeTrue(), "failed to apply the new vm object")
+
+				baseVM.Spec.Template.Spec.Domain.Devices.Interfaces = append(baseVM.Spec.Template.Spec.Domain.Devices.Interfaces, newInterface("br2", ""))
+
+				Eventually(func() error {
+					return testClient.VirtClient.Create(context.TODO(), baseVM)
+
+				}, 50*time.Second, 5*time.Second).Should(Not(HaveOccurred()), "failed to apply the new vm object error")
+			})
+			It("should allow to assign to the VM the same MAC addresses, different name as requested before and do not return an error", func() {
+				err := setRange(rangeStart, rangeEnd)
+				Expect(err).ToNot(HaveOccurred())
+
+				vm1 := CreateVmObject(TestNamespace, false,
+					[]kubevirtv1.Interface{newInterface("br1", "02:00:ff:ff:ff:ff")},
+					[]kubevirtv1.Network{newNetwork("br1"), newNetwork("br2")})
+
+				baseVM := vm1.DeepCopy()
+				baseVM.Name = "new-vm"
+
+				Eventually(func() bool {
+					err := testClient.VirtClient.Create(context.TODO(), vm1)
+					if err != nil && strings.Contains(err.Error(), "every network must be mapped to an interface") {
+						return true
+					}
+					return false
+
+				}, 40*time.Second, 5*time.Second).Should(BeTrue(), "failed to apply the new vm object")
+
+				baseVM.Spec.Template.Spec.Domain.Devices.Interfaces = append(baseVM.Spec.Template.Spec.Domain.Devices.Interfaces, newInterface("br2", ""))
+
+				Eventually(func() error {
+					return testClient.VirtClient.Create(context.TODO(), baseVM)
+
+				}, 50*time.Second, 5*time.Second).Should(Not(HaveOccurred()), "failed to apply the new vm object error")
+			})
+		})
 	})
 })