Merge pull request #48354 from deads2k/gc-01-deletenever

Automatic merge from submit-queue (batch tested with PRs 47784, 47793, 48334, 48435, 48354)

allow a deletestrategy to opt-out of GC

Not all resources should be GC-able and we implemented an ignore list to handle this, but at the storage layer they could still set finalizers, they just hung in a stuck state forever.  This updates the strategy to allow a resource to indicate that they shouldn't be GCed.

@kubernetes/sig-api-machinery-misc

pkg/registry/core/event/BUILD

@@ -24,6 +24,7 @@ go_library(
         "//vendor/k8s.io/apimachinery/pkg/util/validation/field:go_default_library",
         "//vendor/k8s.io/apiserver/pkg/endpoints/request:go_default_library",
         "//vendor/k8s.io/apiserver/pkg/registry/generic:go_default_library",
+        "//vendor/k8s.io/apiserver/pkg/registry/rest:go_default_library",
         "//vendor/k8s.io/apiserver/pkg/storage:go_default_library",
         "//vendor/k8s.io/apiserver/pkg/storage/names:go_default_library",
     ],

pkg/registry/core/event/strategy.go

@@ -25,6 +25,7 @@ import (
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
 	"k8s.io/apiserver/pkg/registry/generic"
+	"k8s.io/apiserver/pkg/registry/rest"
 	"k8s.io/apiserver/pkg/storage"
 	"k8s.io/apiserver/pkg/storage/names"
 	"k8s.io/kubernetes/pkg/api"
@@ -40,6 +41,10 @@ type eventStrategy struct {
 // Event objects via the REST API.
 var Strategy = eventStrategy{api.Scheme, names.SimpleNameGenerator}
 
+func (eventStrategy) DefaultGarbageCollectionPolicy() rest.GarbageCollectionPolicy {
+	return rest.Unsupported
+}
+
 func (eventStrategy) NamespaceScoped() bool {
 	return true
 }

staging/src/k8s.io/apiserver/pkg/registry/generic/registry/BUILD

@@ -68,6 +68,7 @@ go_library(
         "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/runtime:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/util/sets:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/validation/field:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/watch:go_default_library",
         "//vendor/k8s.io/apiserver/pkg/endpoints/request:go_default_library",

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

@@ -34,6 +34,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/apimachinery/pkg/watch"
 	genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
@@ -628,120 +629,120 @@ var (
 	errEmptiedFinalizers = fmt.Errorf("emptied finalizers")
 )
 
-// shouldUpdateFinalizerOrphanDependents returns if the finalizers need to be
+// shouldOrphanDependents returns true if the finalizer for orphaning should be set
 // updated for FinalizerOrphanDependents. In the order of highest to lowest
 // priority, there are three factors affect whether to add/remove the
 // FinalizerOrphanDependents: options, existing finalizers of the object,
 // and e.DeleteStrategy.DefaultGarbageCollectionPolicy.
-func shouldUpdateFinalizerOrphanDependents(e *Store, accessor metav1.Object, options *metav1.DeleteOptions) (shouldUpdate bool, shouldOrphan bool) {
+func shouldOrphanDependents(e *Store, accessor metav1.Object, options *metav1.DeleteOptions) bool {
-	shouldOrphan = false
-	// Get default orphan policy from this REST object type
 	if gcStrategy, ok := e.DeleteStrategy.(rest.GarbageCollectionDeleteStrategy); ok {
-		if gcStrategy.DefaultGarbageCollectionPolicy() == rest.OrphanDependents {
+		if gcStrategy.DefaultGarbageCollectionPolicy() == rest.Unsupported {
-			shouldOrphan = true
+			// return  false to indicate that we should NOT orphan
+			return false
 		}
 	}
 
-	// If a finalizer is set in the object, it overrides the default
+	// An explicit policy was set at deletion time, that overrides everything
-	hasOrphanFinalizer := false
-	finalizers := accessor.GetFinalizers()
-	for _, f := range finalizers {
-		// validation should make sure the two cases won't be true at the same
-		// time.
-		switch f {
-		case metav1.FinalizerOrphanDependents:
-			shouldOrphan = true
-			hasOrphanFinalizer = true
-			break
-		case metav1.FinalizerDeleteDependents:
-			shouldOrphan = false
-			break
-		}
-	}
-
-	// If an explicit policy was set at deletion time, that overrides both
 	if options != nil && options.OrphanDependents != nil {
-		shouldOrphan = *options.OrphanDependents
+		return *options.OrphanDependents
 	}
 	if options != nil && options.PropagationPolicy != nil {
 		switch *options.PropagationPolicy {
 		case metav1.DeletePropagationOrphan:
-			shouldOrphan = true
+			return true
 		case metav1.DeletePropagationBackground, metav1.DeletePropagationForeground:
-			shouldOrphan = false
+			return false
 		}
 	}
 
-	shouldUpdate = shouldOrphan != hasOrphanFinalizer
+	// If a finalizer is set in the object, it overrides the default
-	return shouldUpdate, shouldOrphan
+	// validation should make sure the two cases won't be true at the same time.
+	finalizers := accessor.GetFinalizers()
+	for _, f := range finalizers {
+		switch f {
+		case metav1.FinalizerOrphanDependents:
+			return true
+		case metav1.FinalizerDeleteDependents:
+			return false
+		}
+	}
+
+	// Get default orphan policy from this REST object type if it exists
+	if gcStrategy, ok := e.DeleteStrategy.(rest.GarbageCollectionDeleteStrategy); ok {
+		if gcStrategy.DefaultGarbageCollectionPolicy() == rest.OrphanDependents {
+			return true
+		}
+	}
+	return false
 }
 
-// shouldUpdateFinalizerDeleteDependents returns if the finalizers need to be
+// shouldDeleteDependents returns true if the finalizer for foreground deletion should be set
 // updated for FinalizerDeleteDependents. In the order of highest to lowest
 // priority, there are three factors affect whether to add/remove the
 // FinalizerDeleteDependents: options, existing finalizers of the object, and
 // e.DeleteStrategy.DefaultGarbageCollectionPolicy.
-func shouldUpdateFinalizerDeleteDependents(e *Store, accessor metav1.Object, options *metav1.DeleteOptions) (shouldUpdate bool, shouldDeleteDependentInForeground bool) {
+func shouldDeleteDependents(e *Store, accessor metav1.Object, options *metav1.DeleteOptions) bool {
-	// default to false
+	// Get default orphan policy from this REST object type
-	shouldDeleteDependentInForeground = false
+	if gcStrategy, ok := e.DeleteStrategy.(rest.GarbageCollectionDeleteStrategy); ok && gcStrategy.DefaultGarbageCollectionPolicy() == rest.Unsupported {
-
+		// return false to indicate that we should NOT delete in foreground
-	// If a finalizer is set in the object, it overrides the default
+		return false
-	hasFinalizerDeleteDependents := false
-	finalizers := accessor.GetFinalizers()
-	for _, f := range finalizers {
-		// validation has made sure the two cases won't be true at the same
-		// time.
-		switch f {
-		case metav1.FinalizerDeleteDependents:
-			shouldDeleteDependentInForeground = true
-			hasFinalizerDeleteDependents = true
-			break
-		case metav1.FinalizerOrphanDependents:
-			shouldDeleteDependentInForeground = false
-			break
-		}
 	}
 
 	// If an explicit policy was set at deletion time, that overrides both
 	if options != nil && options.OrphanDependents != nil {
-		shouldDeleteDependentInForeground = false
+		return false
 	}
 	if options != nil && options.PropagationPolicy != nil {
 		switch *options.PropagationPolicy {
 		case metav1.DeletePropagationForeground:
-			shouldDeleteDependentInForeground = true
+			return true
 		case metav1.DeletePropagationBackground, metav1.DeletePropagationOrphan:
-			shouldDeleteDependentInForeground = false
+			return false
 		}
 	}
 
-	shouldUpdate = shouldDeleteDependentInForeground != hasFinalizerDeleteDependents
+	// If a finalizer is set in the object, it overrides the default
-	return shouldUpdate, shouldDeleteDependentInForeground
+	// validation has made sure the two cases won't be true at the same time.
-}
+	finalizers := accessor.GetFinalizers()
-
+	for _, f := range finalizers {
-// shouldUpdateFinalizers returns if we need to update the finalizers of the
+		switch f {
-// object, and the desired list of finalizers.
+		case metav1.FinalizerDeleteDependents:
-func shouldUpdateFinalizers(e *Store, accessor metav1.Object, options *metav1.DeleteOptions) (shouldUpdate bool, newFinalizers []string) {
+			return true
-	shouldUpdate1, shouldOrphan := shouldUpdateFinalizerOrphanDependents(e, accessor, options)
+		case metav1.FinalizerOrphanDependents:
-	shouldUpdate2, shouldDeleteDependentInForeground := shouldUpdateFinalizerDeleteDependents(e, accessor, options)
+			return false
-	oldFinalizers := accessor.GetFinalizers()
+		}
-	if !shouldUpdate1 && !shouldUpdate2 {
-		return false, oldFinalizers
 	}
 
+	return false
+}
+
+// deletionFinalizers returns the deletion finalizers we should set on the object and a bool indicate they did or
+// did not change.
+func deletionFinalizers(e *Store, accessor metav1.Object, options *metav1.DeleteOptions) (bool, []string) {
+	shouldOrphan := shouldOrphanDependents(e, accessor, options)
+	shouldDeleteDependentInForeground := shouldDeleteDependents(e, accessor, options)
+	newFinalizers := []string{}
+
 	// first remove both finalizers, add them back if needed.
-	for _, f := range oldFinalizers {
+	for _, f := range accessor.GetFinalizers() {
 		if f == metav1.FinalizerOrphanDependents || f == metav1.FinalizerDeleteDependents {
 			continue
 		}
 		newFinalizers = append(newFinalizers, f)
 	}
+
 	if shouldOrphan {
 		newFinalizers = append(newFinalizers, metav1.FinalizerOrphanDependents)
 	}
 	if shouldDeleteDependentInForeground {
 		newFinalizers = append(newFinalizers, metav1.FinalizerDeleteDependents)
 	}
+
+	oldFinalizerSet := sets.NewString(accessor.GetFinalizers()...)
+	newFinalizersSet := sets.NewString(newFinalizers...)
+	if oldFinalizerSet.Equal(newFinalizersSet) {
+		return false, accessor.GetFinalizers()
+	}
 	return true, newFinalizers
 }
 
@@ -871,8 +872,8 @@ func (e *Store) updateForGracefulDeletionAndFinalizers(ctx genericapirequest.Con
 			if err != nil {
 				return nil, err
 			}
-			shouldUpdate, newFinalizers := shouldUpdateFinalizers(e, existingAccessor, options)
+			needsUpdate, newFinalizers := deletionFinalizers(e, existingAccessor, options)
-			if shouldUpdate {
+			if needsUpdate {
 				existingAccessor.SetFinalizers(newFinalizers)
 			}
 
@@ -964,7 +965,7 @@ 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, _ := shouldUpdateFinalizers(e, accessor, options)
+		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)

staging/src/k8s.io/apiserver/pkg/registry/rest/delete.go

@@ -39,6 +39,9 @@ type GarbageCollectionPolicy string
 const (
 	DeleteDependents GarbageCollectionPolicy = "DeleteDependents"
 	OrphanDependents GarbageCollectionPolicy = "OrphanDependents"
+	// Unsupported means that the resource knows that it cannot be GC'd, so the finalizers
+	// should never be set in storage.
+	Unsupported GarbageCollectionPolicy = "Unsupported"
 )
 
 // GarbageCollectionDeleteStrategy must be implemented by the registry that wants to