Fix templateless vm clean (#436)

* Revert "pool-manager: Prevent panic if VM template is nil (#431)" This reverts commit 1478ec9. Signed-off-by: Ram Lavi <[email protected]> * vm, webhook: Fix panic on templateless VM former fix [0] was not sufficient. Moving the check earlier in the webhoook handling. Signed-off-by: Ram Lavi <[email protected]> --------- Signed-off-by: Ram Lavi <[email protected]>
k8snetworkplumbingwg · Sep 9, 2024 · c1e70b8 · c1e70b8
1 parent 36eed74
commit c1e70b8
Show file tree

Hide file tree

Showing 3 changed files with 6 additions and 43 deletions.
diff --git a/pkg/pool-manager/pool_test.go b/pkg/pool-manager/pool_test.go
@@ -357,16 +357,6 @@ var _ = Describe("Pool", func() {
 		updateTransactionTimestamp := func(secondsPassed time.Duration) time.Time {
 			return time.Now().Add(secondsPassed * time.Second)
 		}
-		It("should not allocate if VM template is nil", func() {
-			poolManager := createPoolManager("02:00:00:00:00:00", "02:00:00:00:00:02")
-			newInvalidVM := multipleInterfacesVM.DeepCopy()
-			newInvalidVM.Name = "newVM"
-			newInvalidVM.Spec.Template = nil
-
-			transactionTimestamp := updateTransactionTimestamp(0)
-			err := poolManager.AllocateVirtualMachineMac(newInvalidVM, &transactionTimestamp, true, logger)
-			Expect(err).ToNot(HaveOccurred())
-		})
 		It("should reject allocation if there are interfaces with the same name", func() {
 			poolManager := createPoolManager("02:00:00:00:00:00", "02:00:00:00:00:02")
 			newVM := duplicateInterfacesVM.DeepCopy()
@@ -429,26 +419,6 @@ var _ = Describe("Pool", func() {
 			})
 		})
 		Describe("Update vm object", func() {
-			It("should not allocate if VM template is nil", func() {
-				poolManager := createPoolManager("02:00:00:00:00:00", "02:00:00:00:00:02")
-				newVM := multipleInterfacesVM.DeepCopy()
-				newVM.Name = "newVM"
-
-				transactionTimestamp := updateTransactionTimestamp(0)
-				err := poolManager.AllocateVirtualMachineMac(newVM, &transactionTimestamp, true, logger)
-				Expect(err).ToNot(HaveOccurred())
-
-				updateVm := multipleInterfacesVM.DeepCopy()
-				newInvalidVM := newVM.DeepCopy()
-				newInvalidVM.Spec.Template = nil
-				err = poolManager.UpdateMacAddressesForVirtualMachine(newInvalidVM, updateVm, &transactionTimestamp, true, logger)
-				Expect(err).ToNot(HaveOccurred())
-
-				updateInvalidVm := multipleInterfacesVM.DeepCopy()
-				updateInvalidVm.Spec.Template = nil
-				err = poolManager.UpdateMacAddressesForVirtualMachine(newVM, updateInvalidVm, &transactionTimestamp, true, logger)
-				Expect(err).ToNot(HaveOccurred())
-			})
 			It("should preserve disk.io configuration on update", func() {
 				addDiskIO := func(vm *kubevirt.VirtualMachine, ioName kubevirt.DriverIO) {
 					vm.Spec.Template.Spec.Domain.Devices.Disks = make([]kubevirt.Disk, 1)

diff --git a/pkg/pool-manager/virtualmachine_pool.go b/pkg/pool-manager/virtualmachine_pool.go
@@ -42,10 +42,6 @@ func (p *PoolManager) AllocateVirtualMachineMac(virtualMachine *kubevirt.Virtual
 	defer p.poolMutex.Unlock()
 	logger := parentLogger.WithName("AllocateVirtualMachineMac")
 
-	if virtualMachine.Spec.Template == nil {
-		logger.Info("virtual machine template is nil, skipping mac allocation", "virtualMachine", virtualMachine)
-		return nil
-	}
 	if len(virtualMachine.Spec.Template.Spec.Domain.Devices.Interfaces) == 0 {
 		logger.Info("no interfaces found for virtual machine, skipping mac allocation", "virtualMachine", virtualMachine)
 		return nil
@@ -137,15 +133,6 @@ func (p *PoolManager) UpdateMacAddressesForVirtualMachine(previousVirtualMachine
 	}
 	defer p.poolMutex.Unlock()
 
-	if previousVirtualMachine.Spec.Template == nil {
-		logger.Info("virtual machine template is nil, skipping mac allocation", "virtualMachine", virtualMachine)
-		return nil
-	}
-	if virtualMachine.Spec.Template == nil {
-		logger.Info("virtual machine template is nil, skipping mac allocation", "virtualMachine", virtualMachine)
-		return nil
-	}
-
 	// We can't allow for duplicate interfaces names, as interface.Name is macMap's key.
 	if isNotDryRun {
 		if err := checkVmForInterfaceDuplication(virtualMachine); err != nil {

diff --git a/pkg/webhook/virtualmachine/virtualmachine.go b/pkg/webhook/virtualmachine/virtualmachine.go
@@ -76,6 +76,12 @@ func (a *virtualMachineAnnotator) Handle(ctx context.Context, req admission.Requ
 
 	logger.V(1).Info("got a virtual machine event")
 
+	if virtualMachine.Spec.Template == nil {
+		const errMsg = "virtual machine template is nil, ignoring virtual machine"
+		logger.Info(errMsg)
+		return admission.Allowed(errMsg)
+	}
+
 	isNotDryRun := (req.DryRun == nil || *req.DryRun == false)
 	if req.AdmissionRequest.Operation == admissionv1.Create {
 		err = a.mutateCreateVirtualMachinesFn(virtualMachine, isNotDryRun, logger)