From 2b5d87f73ef24aa9caba4c356e4d682e15869e9c Mon Sep 17 00:00:00 2001 From: David Vossel Date: Wed, 16 Jun 2021 14:50:23 -0400 Subject: [PATCH 1/2] Fix segfault in pdb controller when vmi is deleted before pdb is deleted Signed-off-by: David Vossel --- .../disruptionbudget/disruptionbudget.go | 24 ++++++++++++------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/pkg/virt-controller/watch/drain/disruptionbudget/disruptionbudget.go b/pkg/virt-controller/watch/drain/disruptionbudget/disruptionbudget.go index 260b88352714..c10640e49d18 100644 --- a/pkg/virt-controller/watch/drain/disruptionbudget/disruptionbudget.go +++ b/pkg/virt-controller/watch/drain/disruptionbudget/disruptionbudget.go @@ -342,7 +342,7 @@ func (c *DisruptionBudgetController) execute(key string) error { } // Fetch the latest Vm state from cache - obj, exists, err := c.vmiInformer.GetStore().GetByKey(key) + obj, vmiExists, err := c.vmiInformer.GetStore().GetByKey(key) if err != nil { return err @@ -350,8 +350,14 @@ func (c *DisruptionBudgetController) execute(key string) error { var vmi *virtv1.VirtualMachineInstance // Once all finalizers are removed the vmi gets deleted and we can clean all expectations - if exists { + if vmiExists { vmi = obj.(*virtv1.VirtualMachineInstance) + } else { + namespace, name, err := cache.SplitMetaNamespaceKey(key) + if err != nil { + return err + } + vmi = virtv1.NewVMIReferenceFromNameWithNS(namespace, name) } namespace, name, err := cache.SplitMetaNamespaceKey(key) @@ -370,11 +376,11 @@ func (c *DisruptionBudgetController) execute(key string) error { } if len(pdbs) == 0 { - return c.sync(key, vmi, nil) + return c.sync(key, vmiExists, vmi, nil) } for i := range pdbs { - if syncErr := c.sync(key, vmi, pdbs[i]); syncErr != nil { + if syncErr := c.sync(key, vmiExists, vmi, pdbs[i]); syncErr != nil { err = syncErr } } @@ -473,12 +479,12 @@ func isPDBFromOldVMI(vmi *virtv1.VirtualMachineInstance, pdb *v1beta1.PodDisrupt return ownerRef != nil && ownerRef.UID != vmi.UID } -func (c *DisruptionBudgetController) sync(key string, vmi *virtv1.VirtualMachineInstance, pdb *v1beta1.PodDisruptionBudget) error { - migratableOnDrain := vmiMigratableOnDrain(vmi) +func (c *DisruptionBudgetController) sync(key string, vmiExists bool, vmi *virtv1.VirtualMachineInstance, pdb *v1beta1.PodDisruptionBudget) error { + migratableOnDrain := vmiMigratableOnDrain(vmiExists, vmi) // check for deletions if pod exists if pdb != nil { - if vmi == nil || vmi.DeletionTimestamp != nil { + if !vmiExists || vmi.DeletionTimestamp != nil { // being deleted log.Log.Infof("deleting pdb %s/%s due to VMI deletion", pdb.Namespace, pdb.Name) return c.deletePDB(key, pdb, vmi) @@ -527,8 +533,8 @@ func (c *DisruptionBudgetController) pdbsForVMI(namespace, name string) ([]*v1be return pdbs, nil } -func vmiMigratableOnDrain(vmi *virtv1.VirtualMachineInstance) bool { - if vmi == nil || vmi.DeletionTimestamp != nil || vmi.Spec.EvictionStrategy == nil { +func vmiMigratableOnDrain(vmiExists bool, vmi *virtv1.VirtualMachineInstance) bool { + if !vmiExists || vmi.DeletionTimestamp != nil || vmi.Spec.EvictionStrategy == nil { return false } return *vmi.Spec.EvictionStrategy == virtv1.EvictionStrategyLiveMigrate From b5d456e0820761e776225ae4c1e34cbfe582981b Mon Sep 17 00:00:00 2001 From: David Vossel Date: Wed, 16 Jun 2021 14:51:04 -0400 Subject: [PATCH 2/2] Fix pdb unit tests so recorder will validate Event object parsing Signed-off-by: David Vossel --- .../drain/disruptionbudget/disruptionbudget_test.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/pkg/virt-controller/watch/drain/disruptionbudget/disruptionbudget_test.go b/pkg/virt-controller/watch/drain/disruptionbudget/disruptionbudget_test.go index 878a68820e9c..f022200f1981 100644 --- a/pkg/virt-controller/watch/drain/disruptionbudget/disruptionbudget_test.go +++ b/pkg/virt-controller/watch/drain/disruptionbudget/disruptionbudget_test.go @@ -96,6 +96,7 @@ var _ = Describe("Disruptionbudget", func() { vmimInformer, _ = testutils.NewFakeInformerFor(&v1.VirtualMachineInstanceMigration{}) podInformer, _ = testutils.NewFakeInformerFor(&v12.Pod{}) recorder = record.NewFakeRecorder(100) + recorder.IncludeObject = true controller = disruptionbudget.NewDisruptionBudgetController(vmiInformer, pdbInformer, podInformer, vmimInformer, recorder, virtClient) mockQueue = testutils.NewMockWorkQueue(controller.Queue) @@ -164,6 +165,17 @@ var _ = Describe("Disruptionbudget", func() { testutils.ExpectEvent(recorder, disruptionbudget.SuccessfulDeletePodDisruptionBudgetReason) }) + It("should remove the pdb if VMI doesn't exist", func() { + vmi := newVirtualMachine() + vmi.Spec.EvictionStrategy = newEvictionStrategy() + pdb := newPodDisruptionBudget(vmi) + pdbFeeder.Add(pdb) + + shouldExpectPDBDeletion(pdb) + controller.Execute() + testutils.ExpectEvent(recorder, disruptionbudget.SuccessfulDeletePodDisruptionBudgetReason) + }) + It("should recreate the PDB if the VMI is recreated", func() { vmi := newVirtualMachine() vmi.Spec.EvictionStrategy = newEvictionStrategy()