diff --git a/pkg/controller/garbagecollector/graph_builder.go b/pkg/controller/garbagecollector/graph_builder.go index f1020047483..525e357c201 100644 --- a/pkg/controller/garbagecollector/graph_builder.go +++ b/pkg/controller/garbagecollector/graph_builder.go @@ -427,15 +427,14 @@ func referencesDiffs(old []metav1.OwnerReference, new []metav1.OwnerReference) ( return added, removed, changed } -// returns if the object in the event just transitions to "being deleted". -func deletionStarts(oldObj interface{}, newAccessor metav1.Object) bool { - // The delta_fifo may combine the creation and update of the object into one - // event, so if there is no oldObj, we just return if the newObj (via - // newAccessor) is being deleted. +func deletionStartsWithFinalizer(oldObj interface{}, newAccessor metav1.Object, matchingFinalizer string) bool { + // if the new object isn't being deleted, or doesn't have the finalizer we're interested in, return false + if !beingDeleted(newAccessor) || !hasFinalizer(newAccessor, matchingFinalizer) { + return false + } + + // if the old object is nil, or wasn't being deleted, or didn't have the finalizer, return true if oldObj == nil { - if newAccessor.GetDeletionTimestamp() == nil { - return false - } return true } oldAccessor, err := meta.Accessor(oldObj) @@ -443,7 +442,7 @@ func deletionStarts(oldObj interface{}, newAccessor metav1.Object) bool { utilruntime.HandleError(fmt.Errorf("cannot access oldObj: %v", err)) return false } - return beingDeleted(newAccessor) && !beingDeleted(oldAccessor) + return !beingDeleted(oldAccessor) || !hasFinalizer(oldAccessor, matchingFinalizer) } func beingDeleted(accessor metav1.Object) bool { @@ -471,13 +470,13 @@ func hasFinalizer(accessor metav1.Object, matchingFinalizer string) bool { // this function takes newAccessor directly because the caller already // instantiates an accessor for the newObj. func startsWaitingForDependentsDeleted(oldObj interface{}, newAccessor metav1.Object) bool { - return deletionStarts(oldObj, newAccessor) && hasDeleteDependentsFinalizer(newAccessor) + return deletionStartsWithFinalizer(oldObj, newAccessor, metav1.FinalizerDeleteDependents) } // this function takes newAccessor directly because the caller already // instantiates an accessor for the newObj. func startsWaitingForDependentsOrphaned(oldObj interface{}, newAccessor metav1.Object) bool { - return deletionStarts(oldObj, newAccessor) && hasOrphanFinalizer(newAccessor) + return deletionStartsWithFinalizer(oldObj, newAccessor, metav1.FinalizerOrphanDependents) } // if an blocking ownerReference points to an object gets removed, or gets set to diff --git a/test/integration/garbagecollector/garbage_collector_test.go b/test/integration/garbagecollector/garbage_collector_test.go index dee8be7478e..6f3e97f79df 100644 --- a/test/integration/garbagecollector/garbage_collector_test.go +++ b/test/integration/garbagecollector/garbage_collector_test.go @@ -786,6 +786,69 @@ func TestNonBlockingOwnerRefDoesNotBlock(t *testing.T) { } } +func TestDoubleDeletionWithFinalizer(t *testing.T) { + // test setup + ctx := setup(t, 5) + defer ctx.tearDown() + clientSet := ctx.clientSet + ns := createNamespaceOrDie("gc-double-foreground", clientSet, t) + defer deleteNamespaceOrDie(ns.Name, clientSet, t) + + // step 1: creates a pod with a custom finalizer and deletes it, then waits until gc removes its finalizer + podClient := clientSet.CoreV1().Pods(ns.Name) + pod := newPod("lucy", ns.Name, nil) + pod.ObjectMeta.Finalizers = []string{"x/y"} + if _, err := podClient.Create(pod); err != nil { + t.Fatalf("Failed to create pod: %v", err) + } + if err := podClient.Delete(pod.Name, getForegroundOptions()); err != nil { + t.Fatalf("Failed to delete pod: %v", err) + } + if err := wait.PollImmediate(1*time.Second, 10*time.Second, func() (bool, error) { + returnedPod, err := podClient.Get(pod.Name, metav1.GetOptions{}) + if err != nil { + return false, err + } + if len(returnedPod.Finalizers) != 1 || returnedPod.Finalizers[0] != "x/y" { + t.Logf("waiting for pod %q to have only one finalizer %q at step 1, got %v", returnedPod.Name, "x/y", returnedPod.Finalizers) + return false, nil + } + return true, nil + }); err != nil { + t.Fatalf("Failed waiting for pod to have only one filanizer at step 1, error: %v", err) + } + + // step 2: deletes the pod one more time and checks if there's only the custom finalizer left + if err := podClient.Delete(pod.Name, getForegroundOptions()); err != nil { + t.Fatalf("Failed to delete pod: %v", err) + } + if err := wait.PollImmediate(1*time.Second, 10*time.Second, func() (bool, error) { + returnedPod, err := podClient.Get(pod.Name, metav1.GetOptions{}) + if err != nil { + return false, err + } + if len(returnedPod.Finalizers) != 1 || returnedPod.Finalizers[0] != "x/y" { + t.Logf("waiting for pod %q to have only one finalizer %q at step 2, got %v", returnedPod.Name, "x/y", returnedPod.Finalizers) + return false, nil + } + return true, nil + }); err != nil { + t.Fatalf("Failed waiting for pod to have only one finalizer at step 2, gc hasn't removed its finalzier?, error: %v", err) + } + + // step 3: removes the custom finalizer and checks if the pod was removed + patch := []byte(`[{"op":"remove","path":"/metadata/finalizers"}]`) + if _, err := podClient.Patch(pod.Name, types.JSONPatchType, patch); err != nil { + t.Fatalf("Failed to update pod: %v", err) + } + if err := wait.Poll(1*time.Second, 10*time.Second, func() (bool, error) { + _, err := podClient.Get(pod.Name, metav1.GetOptions{}) + return errors.IsNotFound(err), nil + }); err != nil { + t.Fatalf("Failed waiting for pod %q to be deleted", pod.Name) + } +} + func TestBlockingOwnerRefDoesBlock(t *testing.T) { ctx := setup(t, 0) defer ctx.tearDown()