Enable finalizers independent of GC enablement

Decouple finalizer processing from garbage collection configuration.
Finalizers should be effective even when garbage collection is disabled
for a given store.

Fixes https://github.com/kubernetes/kubernetes/issues/50528.

staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go

@@ -442,12 +442,8 @@ func (e *Store) WaitForInitialized(ctx genericapirequest.Context, obj runtime.Ob
 
 // shouldDeleteDuringUpdate checks if a Update is removing all the object's
 // finalizers. If so, it further checks if the object's
-// DeletionGracePeriodSeconds is 0. If so, it returns true. If garbage collection
-// is disabled it always returns false.
+// DeletionGracePeriodSeconds is 0.
 func (e *Store) shouldDeleteDuringUpdate(ctx genericapirequest.Context, key string, obj, existing runtime.Object) bool {
-	if !e.EnableGarbageCollection {
-		return false
-	}
 	newMeta, err := meta.Accessor(obj)
 	if err != nil {
 		utilruntime.HandleError(err)
@@ -765,9 +761,16 @@ func shouldDeleteDependents(e *Store, accessor metav1.Object, options *metav1.De
 	return false
 }
 
-// deletionFinalizers returns the deletion finalizers we should set on the object and a bool indicate they did or
-// did not change.
+// deletionFinalizers returns the deletion finalizers we should set on the
+// object and a bool indicate they did or did not change.
+//
+// Because the finalizers created here are only cleared by the garbage
+// collector, deletionFinalizers always returns false when garbage collection is
+// disabled for the Store.
 func deletionFinalizers(e *Store, accessor metav1.Object, options *metav1.DeleteOptions) (bool, []string) {
+	if !e.EnableGarbageCollection {
+		return false, []string{}
+	}
 	shouldOrphan := shouldOrphanDependents(e, accessor, options)
 	shouldDeleteDependentInForeground := shouldDeleteDependents(e, accessor, options)
 	newFinalizers := []string{}
@@ -816,72 +819,6 @@ func markAsDeleting(obj runtime.Object) (err error) {
 	return nil
 }
 
-// updateForGracefulDeletion and updateForGracefulDeletionAndFinalizers both
-// implement deletion flows for graceful deletion.  Graceful deletion is
-// implemented as setting the deletion timestamp in an update.  If the
-// implementation of graceful deletion is changed, both of these methods
-// should be changed together.
-
-// updateForGracefulDeletion updates the given object for graceful deletion by
-// setting the deletion timestamp and grace period seconds and returns:
-//
-// 1. an error
-// 2. a boolean indicating that the object was not found, but it should be
-//    ignored
-// 3. a boolean indicating that the object's grace period is exhausted and it
-//    should be deleted immediately
-// 4. a new output object with the state that was updated
-// 5. a copy of the last existing state of the object
-func (e *Store) updateForGracefulDeletion(ctx genericapirequest.Context, name, key string, options *metav1.DeleteOptions, preconditions storage.Preconditions, in runtime.Object) (err error, ignoreNotFound, deleteImmediately bool, out, lastExisting runtime.Object) {
-	lastGraceful := int64(0)
-	out = e.NewFunc()
-	err = e.Storage.GuaranteedUpdate(
-		ctx,
-		key,
-		out,
-		false, /* ignoreNotFound */
-		&preconditions,
-		storage.SimpleUpdate(func(existing runtime.Object) (runtime.Object, error) {
-			graceful, pendingGraceful, err := rest.BeforeDelete(e.DeleteStrategy, ctx, existing, options)
-			if err != nil {
-				return nil, err
-			}
-			if pendingGraceful {
-				return nil, errAlreadyDeleting
-			}
-			if !graceful {
-				return nil, errDeleteNow
-			}
-			lastGraceful = *options.GracePeriodSeconds
-			lastExisting = existing
-			return existing, nil
-		}),
-	)
-	switch err {
-	case nil:
-		if lastGraceful > 0 {
-			return nil, false, false, out, lastExisting
-		}
-		// If we are here, the registry supports grace period mechanism and
-		// we are intentionally delete gracelessly. In this case, we may
-		// enter a race with other k8s components. If other component wins
-		// the race, the object will not be found, and we should tolerate
-		// the NotFound error. See
-		// https://github.com/kubernetes/kubernetes/issues/19403 for
-		// details.
-		return nil, true, true, out, lastExisting
-	case errDeleteNow:
-		// we've updated the object to have a zero grace period, or it's already at 0, so
-		// we should fall through and truly delete the object.
-		return nil, false, true, out, lastExisting
-	case errAlreadyDeleting:
-		out, err = e.finalizeDelete(ctx, in, true)
-		return err, false, false, out, lastExisting
-	default:
-		return storeerr.InterpretUpdateError(err, e.qualifiedResourceFromContext(ctx), name), false, false, out, lastExisting
-	}
-}
-
 // updateForGracefulDeletionAndFinalizers updates the given object for
 // graceful deletion and finalization by setting the deletion timestamp and
 // grace period seconds (graceful deletion) and updating the list of
@@ -1013,18 +950,12 @@ func (e *Store) Delete(ctx genericapirequest.Context, name string, options *meta
 
 	// Handle combinations of graceful deletion and finalization by issuing
 	// the correct updates.
-	if e.EnableGarbageCollection {
-		shouldUpdateFinalizers, _ := deletionFinalizers(e, accessor, options)
-		// TODO: remove the check, because we support no-op updates now.
-		if graceful || pendingFinalizers || shouldUpdateFinalizers {
-			err, ignoreNotFound, deleteImmediately, out, lastExisting = e.updateForGracefulDeletionAndFinalizers(ctx, name, key, options, preconditions, obj)
-		}
-	} else {
-		// TODO: remove the check on graceful, because we support no-op updates now.
-		if graceful {
-			err, ignoreNotFound, deleteImmediately, out, lastExisting = e.updateForGracefulDeletion(ctx, name, key, options, preconditions, obj)
-		}
+	shouldUpdateFinalizers, _ := deletionFinalizers(e, accessor, options)
+	// TODO: remove the check, because we support no-op updates now.
+	if graceful || pendingFinalizers || shouldUpdateFinalizers {
+		err, ignoreNotFound, deleteImmediately, out, lastExisting = e.updateForGracefulDeletionAndFinalizers(ctx, name, key, options, preconditions, obj)
 	}
+
 	// !deleteImmediately covers all cases where err != nil. We keep both to be future-proof.
 	if !deleteImmediately || err != nil {
 		return out, false, err

staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store_test.go

@@ -931,56 +931,62 @@ func TestGracefulStoreHandleFinalizers(t *testing.T) {
 
 	testContext := genericapirequest.WithNamespace(genericapirequest.NewContext(), "test")
 	destroyFunc, registry := NewTestGenericStoreRegistry(t)
-	registry.EnableGarbageCollection = true
 	defaultDeleteStrategy := testRESTStrategy{scheme, names.SimpleNameGenerator, true, false, true}
 	registry.DeleteStrategy = testGracefulStrategy{defaultDeleteStrategy}
 	defer destroyFunc()
-	// create pod
-	_, err := registry.Create(testContext, podWithFinalizer, false)
-	if err != nil {
-		t.Errorf("Unexpected error: %v", err)
-	}
 
-	// delete the pod with grace period=0, the pod should still exist because it has a finalizer
-	_, wasDeleted, err := registry.Delete(testContext, podWithFinalizer.Name, metav1.NewDeleteOptions(0))
-	if err != nil {
-		t.Fatalf("Unexpected error: %v", err)
-	}
-	if wasDeleted {
-		t.Errorf("unexpected, pod %s should not have been deleted immediately", podWithFinalizer.Name)
-	}
-	_, err = registry.Get(testContext, podWithFinalizer.Name, &metav1.GetOptions{})
-	if err != nil {
-		t.Fatalf("Unexpected error: %v", err)
-	}
+	gcStates := []bool{true, false}
+	for _, gcEnabled := range gcStates {
+		t.Logf("garbage collection enabled: %t", gcEnabled)
+		registry.EnableGarbageCollection = gcEnabled
 
-	updatedPodWithFinalizer := &example.Pod{
-		ObjectMeta: metav1.ObjectMeta{Name: "foo", Finalizers: []string{"foo.com/x"}, ResourceVersion: podWithFinalizer.ObjectMeta.ResourceVersion},
-		Spec:       example.PodSpec{NodeName: "machine"},
-	}
-	_, _, err = registry.Update(testContext, updatedPodWithFinalizer.ObjectMeta.Name, rest.DefaultUpdatedObjectInfo(updatedPodWithFinalizer, scheme))
-	if err != nil {
-		t.Fatalf("Unexpected error: %v", err)
-	}
+		// create pod
+		_, err := registry.Create(testContext, podWithFinalizer, false)
+		if err != nil {
+			t.Errorf("Unexpected error: %v", err)
+		}
 
-	// the object should still exist, because it still has a finalizer
-	_, err = registry.Get(testContext, podWithFinalizer.Name, &metav1.GetOptions{})
-	if err != nil {
-		t.Fatalf("Unexpected error: %v", err)
-	}
+		// delete the pod with grace period=0, the pod should still exist because it has a finalizer
+		_, wasDeleted, err := registry.Delete(testContext, podWithFinalizer.Name, metav1.NewDeleteOptions(0))
+		if err != nil {
+			t.Fatalf("Unexpected error: %v", err)
+		}
+		if wasDeleted {
+			t.Errorf("unexpected, pod %s should not have been deleted immediately", podWithFinalizer.Name)
+		}
+		_, err = registry.Get(testContext, podWithFinalizer.Name, &metav1.GetOptions{})
+		if err != nil {
+			t.Fatalf("Unexpected error: %v", err)
+		}
 
-	podWithNoFinalizer := &example.Pod{
-		ObjectMeta: metav1.ObjectMeta{Name: "foo", ResourceVersion: podWithFinalizer.ObjectMeta.ResourceVersion},
-		Spec:       example.PodSpec{NodeName: "anothermachine"},
-	}
-	_, _, err = registry.Update(testContext, podWithFinalizer.ObjectMeta.Name, rest.DefaultUpdatedObjectInfo(podWithNoFinalizer, scheme))
-	if err != nil {
-		t.Fatalf("Unexpected error: %v", err)
-	}
-	// the pod should be removed, because its finalizer is removed
-	_, err = registry.Get(testContext, podWithFinalizer.Name, &metav1.GetOptions{})
-	if !errors.IsNotFound(err) {
-		t.Fatalf("Unexpected error: %v", err)
+		updatedPodWithFinalizer := &example.Pod{
+			ObjectMeta: metav1.ObjectMeta{Name: "foo", Finalizers: []string{"foo.com/x"}, ResourceVersion: podWithFinalizer.ObjectMeta.ResourceVersion},
+			Spec:       example.PodSpec{NodeName: "machine"},
+		}
+		_, _, err = registry.Update(testContext, updatedPodWithFinalizer.ObjectMeta.Name, rest.DefaultUpdatedObjectInfo(updatedPodWithFinalizer, scheme))
+		if err != nil {
+			t.Fatalf("Unexpected error: %v", err)
+		}
+
+		// the object should still exist, because it still has a finalizer
+		_, err = registry.Get(testContext, podWithFinalizer.Name, &metav1.GetOptions{})
+		if err != nil {
+			t.Fatalf("Unexpected error: %v", err)
+		}
+
+		podWithNoFinalizer := &example.Pod{
+			ObjectMeta: metav1.ObjectMeta{Name: "foo", ResourceVersion: podWithFinalizer.ObjectMeta.ResourceVersion},
+			Spec:       example.PodSpec{NodeName: "anothermachine"},
+		}
+		_, _, err = registry.Update(testContext, podWithFinalizer.ObjectMeta.Name, rest.DefaultUpdatedObjectInfo(podWithNoFinalizer, scheme))
+		if err != nil {
+			t.Fatalf("Unexpected error: %v", err)
+		}
+		// the pod should be removed, because its finalizer is removed
+		_, err = registry.Get(testContext, podWithFinalizer.Name, &metav1.GetOptions{})
+		if !errors.IsNotFound(err) {
+			t.Fatalf("Unexpected error: %v", err)
+		}
 	}
 }
 
@@ -1030,73 +1036,79 @@ func TestNonGracefulStoreHandleFinalizers(t *testing.T) {
 
 	testContext := genericapirequest.WithNamespace(genericapirequest.NewContext(), "test")
 	destroyFunc, registry := NewTestGenericStoreRegistry(t)
-	registry.EnableGarbageCollection = true
+
 	defer destroyFunc()
-	// create pod
-	_, err := registry.Create(testContext, podWithFinalizer, false)
-	if err != nil {
-		t.Errorf("Unexpected error: %v", err)
-	}
+	gcStates := []bool{true, false}
+	for _, gcEnabled := range gcStates {
+		t.Logf("garbage collection enabled: %t", gcEnabled)
+		registry.EnableGarbageCollection = gcEnabled
 
-	// delete object with nil delete options doesn't delete the object
-	_, wasDeleted, err := registry.Delete(testContext, podWithFinalizer.Name, nil)
-	if err != nil {
-		t.Errorf("Unexpected error: %v", err)
-	}
-	if wasDeleted {
-		t.Errorf("unexpected, pod %s should not have been deleted immediately", podWithFinalizer.Name)
-	}
+		// create pod
+		_, err := registry.Create(testContext, podWithFinalizer, false)
+		if err != nil {
+			t.Errorf("Unexpected error: %v", err)
+		}
 
-	// the object should still exist
-	obj, err := registry.Get(testContext, podWithFinalizer.Name, &metav1.GetOptions{})
-	if err != nil {
-		t.Errorf("Unexpected error: %v", err)
-	}
-	podWithFinalizer, ok := obj.(*example.Pod)
-	if !ok {
-		t.Errorf("Unexpected object: %#v", obj)
-	}
-	if podWithFinalizer.ObjectMeta.DeletionTimestamp == nil {
-		t.Errorf("Expect the object to have DeletionTimestamp set, but got %#v", podWithFinalizer.ObjectMeta)
-	}
-	if podWithFinalizer.ObjectMeta.DeletionGracePeriodSeconds == nil || *podWithFinalizer.ObjectMeta.DeletionGracePeriodSeconds != 0 {
-		t.Errorf("Expect the object to have 0 DeletionGracePeriodSecond, but got %#v", podWithFinalizer.ObjectMeta)
-	}
-	if podWithFinalizer.Generation <= initialGeneration {
-		t.Errorf("Deletion didn't increase Generation.")
-	}
+		// delete object with nil delete options doesn't delete the object
+		_, wasDeleted, err := registry.Delete(testContext, podWithFinalizer.Name, nil)
+		if err != nil {
+			t.Errorf("Unexpected error: %v", err)
+		}
+		if wasDeleted {
+			t.Errorf("unexpected, pod %s should not have been deleted immediately", podWithFinalizer.Name)
+		}
 
-	updatedPodWithFinalizer := &example.Pod{
-		ObjectMeta: metav1.ObjectMeta{Name: "foo", Finalizers: []string{"foo.com/x"}, ResourceVersion: podWithFinalizer.ObjectMeta.ResourceVersion},
-		Spec:       example.PodSpec{NodeName: "machine"},
-	}
-	_, _, err = registry.Update(testContext, updatedPodWithFinalizer.ObjectMeta.Name, rest.DefaultUpdatedObjectInfo(updatedPodWithFinalizer, scheme))
-	if err != nil {
-		t.Errorf("Unexpected error: %v", err)
-	}
+		// the object should still exist
+		obj, err := registry.Get(testContext, podWithFinalizer.Name, &metav1.GetOptions{})
+		if err != nil {
+			t.Errorf("Unexpected error: %v", err)
+		}
+		podWithFinalizer, ok := obj.(*example.Pod)
+		if !ok {
+			t.Errorf("Unexpected object: %#v", obj)
+		}
+		if podWithFinalizer.ObjectMeta.DeletionTimestamp == nil {
+			t.Errorf("Expect the object to have DeletionTimestamp set, but got %#v", podWithFinalizer.ObjectMeta)
+		}
+		if podWithFinalizer.ObjectMeta.DeletionGracePeriodSeconds == nil || *podWithFinalizer.ObjectMeta.DeletionGracePeriodSeconds != 0 {
+			t.Errorf("Expect the object to have 0 DeletionGracePeriodSecond, but got %#v", podWithFinalizer.ObjectMeta)
+		}
+		if podWithFinalizer.Generation <= initialGeneration {
+			t.Errorf("Deletion didn't increase Generation.")
+		}
 
-	// the object should still exist, because it still has a finalizer
-	obj, err = registry.Get(testContext, podWithFinalizer.Name, &metav1.GetOptions{})
-	if err != nil {
-		t.Errorf("Unexpected error: %v", err)
-	}
-	podWithFinalizer, ok = obj.(*example.Pod)
-	if !ok {
-		t.Errorf("Unexpected object: %#v", obj)
-	}
+		updatedPodWithFinalizer := &example.Pod{
+			ObjectMeta: metav1.ObjectMeta{Name: "foo", Finalizers: []string{"foo.com/x"}, ResourceVersion: podWithFinalizer.ObjectMeta.ResourceVersion},
+			Spec:       example.PodSpec{NodeName: "machine"},
+		}
+		_, _, err = registry.Update(testContext, updatedPodWithFinalizer.ObjectMeta.Name, rest.DefaultUpdatedObjectInfo(updatedPodWithFinalizer, scheme))
+		if err != nil {
+			t.Errorf("Unexpected error: %v", err)
+		}
 
-	podWithNoFinalizer := &example.Pod{
-		ObjectMeta: metav1.ObjectMeta{Name: "foo", ResourceVersion: podWithFinalizer.ObjectMeta.ResourceVersion},
-		Spec:       example.PodSpec{NodeName: "anothermachine"},
-	}
-	_, _, err = registry.Update(testContext, podWithFinalizer.ObjectMeta.Name, rest.DefaultUpdatedObjectInfo(podWithNoFinalizer, scheme))
-	if err != nil {
-		t.Errorf("Unexpected error: %v", err)
-	}
-	// the pod should be removed, because its finalizer is removed
-	_, err = registry.Get(testContext, podWithFinalizer.Name, &metav1.GetOptions{})
-	if !errors.IsNotFound(err) {
-		t.Errorf("Unexpected error: %v", err)
+		// the object should still exist, because it still has a finalizer
+		obj, err = registry.Get(testContext, podWithFinalizer.Name, &metav1.GetOptions{})
+		if err != nil {
+			t.Errorf("Unexpected error: %v", err)
+		}
+		podWithFinalizer, ok = obj.(*example.Pod)
+		if !ok {
+			t.Errorf("Unexpected object: %#v", obj)
+		}
+
+		podWithNoFinalizer := &example.Pod{
+			ObjectMeta: metav1.ObjectMeta{Name: "foo", ResourceVersion: podWithFinalizer.ObjectMeta.ResourceVersion},
+			Spec:       example.PodSpec{NodeName: "anothermachine"},
+		}
+		_, _, err = registry.Update(testContext, podWithFinalizer.ObjectMeta.Name, rest.DefaultUpdatedObjectInfo(podWithNoFinalizer, scheme))
+		if err != nil {
+			t.Errorf("Unexpected error: %v", err)
+		}
+		// the pod should be removed, because its finalizer is removed
+		_, err = registry.Get(testContext, podWithFinalizer.Name, &metav1.GetOptions{})
+		if !errors.IsNotFound(err) {
+			t.Errorf("Unexpected error: %v", err)
+		}
 	}
 }