Merge pull request kubevirt#2420 from rmohr/pdb-fix

Fix expectations in disruptionbudget controller

pkg/virt-controller/watch/drain/disruptionbudget/disruptionbudget.go

@@ -235,15 +235,14 @@ func (c *DisruptionBudgetController) resolveControllerRef(namespace string, cont
 	if controllerRef == nil || controllerRef.Kind != virtv1.VirtualMachineInstanceGroupVersionKind.Kind {
 		return nil
 	}
-	vmi, exists, err := c.vmiInformer.GetStore().GetByKey(namespace + "/" + controllerRef.Name)
-	if err != nil {
-		return nil
-	}
-	if !exists {
-		return nil
-	}
 
-	return vmi.(*virtv1.VirtualMachineInstance)
+	return &virtv1.VirtualMachineInstance{
+		ObjectMeta: v1.ObjectMeta{
+			Name:      controllerRef.Name,
+			Namespace: namespace,
+			UID:       controllerRef.UID,
+		},
+	}
 }
 
 // Run runs the passed in NodeController.
@@ -339,7 +338,7 @@ func (c *DisruptionBudgetController) sync(key string, vmi *virtv1.VirtualMachine
 		} else if !wantsToMigrate && pdb != nil {
 			// We don't want migrations on evictions, if there is a pdb, remove it
 			delete = true
-		} else if wantsToMigrate && pdb == nil {
+		} else if wantsToMigrate && vmi.DeletionTimestamp == nil && pdb == nil {
 			// No pdb and we want migrations on evictions
 			create = true
 		} else if wantsToMigrate && pdb != nil {

pkg/virt-controller/watch/drain/disruptionbudget/disruptionbudget_test.go

@@ -156,6 +156,56 @@ var _ = Describe("Disruptionbudget", func() {
 			testutils.ExpectEvent(recorder, disruptionbudget.SuccessfulDeletePodDisruptionBudgetReason)
 		})
 
+		It("should recreate the PDB if the VMI is recreated", func() {
+			vmi := newVirtualMachine("testvm")
+			vmi.Spec.EvictionStrategy = newEvictionStrategy()
+			addVirtualMachine(vmi)
+			pdb := newPodDisruptionBudget(vmi)
+			pdbFeeder.Add(pdb)
+
+			controller.Execute()
+
+			vmiFeeder.Delete(vmi)
+			shouldExpectPDBDeletion(pdb)
+			controller.Execute()
+			testutils.ExpectEvent(recorder, disruptionbudget.SuccessfulDeletePodDisruptionBudgetReason)
+
+			pdbFeeder.Delete(pdb)
+			vmi.UID = "45356"
+			vmiFeeder.Add(vmi)
+			shouldExpectPDBCreation(vmi.UID)
+			controller.Execute()
+
+			testutils.ExpectEvent(recorder, disruptionbudget.SuccessfulCreatePodDisruptionBudgetReason)
+		})
+
+		It("should delete a PDB which belongs to an old VMI", func() {
+			vmi := newVirtualMachine("testvm")
+			vmi.Spec.EvictionStrategy = newEvictionStrategy()
+			pdb := newPodDisruptionBudget(vmi)
+			pdbFeeder.Add(pdb)
+			// new UID means new VMI
+			vmi.UID = "changed"
+			addVirtualMachine(vmi)
+
+			shouldExpectPDBDeletion(pdb)
+			controller.Execute()
+			testutils.ExpectEvent(recorder, disruptionbudget.SuccessfulDeletePodDisruptionBudgetReason)
+		})
+
+		It("should not create a PDB for VMIs which are already marked for deletion", func() {
+			vmi := newVirtualMachine("testvm")
+			vmi.Spec.EvictionStrategy = newEvictionStrategy()
+			now := v13.Now()
+			vmi.DeletionTimestamp = &now
+			addVirtualMachine(vmi)
+
+			controller.Execute()
+
+			vmiFeeder.Delete(vmi)
+			controller.Execute()
+		})
+
 		It("should remove the pdb if the VMI does not want to be migrated anymore", func() {
 			vmi := newVirtualMachine("testvm")
 			vmi.Spec.EvictionStrategy = newEvictionStrategy()

pkg/virt-controller/watch/vm.go

@@ -754,7 +754,8 @@ func (c *VMController) addVirtualMachine(obj interface{}) {
 		log.Log.Object(vmi).V(4).Info("Looking for VirtualMachineInstance Ref")
 		vm := c.resolveControllerRef(vmi.Namespace, controllerRef)
 		if vm == nil {
-			log.Log.Object(vmi).Errorf("Cant find the matching VM for VirtualMachineInstance: %s", vmi.Name)
+			// not managed by us
+			log.Log.Object(vmi).V(4).Infof("Cant find the matching VM for VirtualMachineInstance: %s", vmi.Name)
 			return
 		}
 		vmKey, err := controller.KeyFunc(vm)

tests/migration_test.go

@@ -963,6 +963,34 @@ var _ = Describe("[rfe_id:393][crit:high[vendor:[email protected]][level:system]
 				Expect(errors.IsTooManyRequests(err)).To(BeTrue())
 			})
 
+			It("should recreate the PDB if VMIs with similar names are recreated", func() {
+				for x := 0; x < 3; x++ {
+					By("creating the VMI")
+					_, err := virtClient.VirtualMachineInstance(tests.NamespaceTestDefault).Create(vmi)
+					Expect(err).ToNot(HaveOccurred())
+
+					By("checking that the PDB appeared")
+					Eventually(func() []v1beta1.PodDisruptionBudget {
+						pdbs, err := virtClient.PolicyV1beta1().PodDisruptionBudgets(tests.NamespaceTestDefault).List(metav1.ListOptions{})
+						Expect(err).ToNot(HaveOccurred())
+						return pdbs.Items
+					}, 3*time.Second, 500*time.Millisecond).Should(HaveLen(1))
+					By("deleting the VMI")
+					err = virtClient.VirtualMachineInstance(tests.NamespaceTestDefault).Delete(vmi.Name, &metav1.DeleteOptions{})
+					Expect(err).ToNot(HaveOccurred())
+					By("checking that the PDB disappeared")
+					Eventually(func() []v1beta1.PodDisruptionBudget {
+						pdbs, err := virtClient.PolicyV1beta1().PodDisruptionBudgets(tests.NamespaceTestDefault).List(metav1.ListOptions{})
+						Expect(err).ToNot(HaveOccurred())
+						return pdbs.Items
+					}, 3*time.Second, 500*time.Millisecond).Should(HaveLen(0))
+					Eventually(func() bool {
+						_, err := virtClient.VirtualMachineInstance(tests.NamespaceTestDefault).Get(vmi.Name, &metav1.GetOptions{})
+						return errors.IsNotFound(err)
+					}, 30*time.Second, 500*time.Millisecond).Should(BeTrue())
+				}
+			})
+
 			It("should block the eviction api while a slow migration is in progress", func() {
 				vmi = fedoraVMIWithEvictionStrategy()