From ee48704a73233dce92c7effeb36dcf76a663cc37 Mon Sep 17 00:00:00 2001 From: Di Xu Date: Mon, 6 Nov 2017 17:12:57 +0800 Subject: [PATCH] change DefaultGarbageCollectionPolicy to DeleteDependents for workload controllers Kubernetes-commit: 344fe56ed30c0b83ab0a01e3b1344ecea3925863 --- pkg/registry/generic/registry/BUILD | 1 + pkg/registry/generic/registry/store.go | 39 +++++---- pkg/registry/generic/registry/store_test.go | 93 ++++++++++++++++++++- pkg/registry/rest/delete.go | 2 +- 4 files changed, 115 insertions(+), 20 deletions(-) diff --git a/pkg/registry/generic/registry/BUILD b/pkg/registry/generic/registry/BUILD index c6eb9a421..02c6becc7 100644 --- a/pkg/registry/generic/registry/BUILD +++ b/pkg/registry/generic/registry/BUILD @@ -24,6 +24,7 @@ go_test( "//vendor/k8s.io/apimachinery/pkg/fields:go_default_library", "//vendor/k8s.io/apimachinery/pkg/labels:go_default_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/runtime/serializer:go_default_library", "//vendor/k8s.io/apimachinery/pkg/selection:go_default_library", "//vendor/k8s.io/apimachinery/pkg/util/validation/field:go_default_library", diff --git a/pkg/registry/generic/registry/store.go b/pkg/registry/generic/registry/store.go index 8536d0de7..db77f4efe 100644 --- a/pkg/registry/generic/registry/store.go +++ b/pkg/registry/generic/registry/store.go @@ -699,12 +699,17 @@ var ( // priority, there are three factors affect whether to add/remove the // FinalizerOrphanDependents: options, existing finalizers of the object, // and e.DeleteStrategy.DefaultGarbageCollectionPolicy. -func shouldOrphanDependents(e *Store, accessor metav1.Object, options *metav1.DeleteOptions) bool { - if gcStrategy, ok := e.DeleteStrategy.(rest.GarbageCollectionDeleteStrategy); ok { - if gcStrategy.DefaultGarbageCollectionPolicy() == rest.Unsupported { - // return false to indicate that we should NOT orphan - return false - } +func shouldOrphanDependents(ctx genericapirequest.Context, e *Store, accessor metav1.Object, options *metav1.DeleteOptions) bool { + // Get default GC policy from this REST object type + gcStrategy, ok := e.DeleteStrategy.(rest.GarbageCollectionDeleteStrategy) + var defaultGCPolicy rest.GarbageCollectionPolicy + if ok { + defaultGCPolicy = gcStrategy.DefaultGarbageCollectionPolicy(ctx) + } + + if defaultGCPolicy == rest.Unsupported { + // return false to indicate that we should NOT orphan + return false } // An explicit policy was set at deletion time, that overrides everything @@ -733,10 +738,8 @@ func shouldOrphanDependents(e *Store, accessor metav1.Object, options *metav1.De } // 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 - } + if defaultGCPolicy == rest.OrphanDependents { + return true } return false } @@ -746,9 +749,9 @@ func shouldOrphanDependents(e *Store, accessor metav1.Object, options *metav1.De // priority, there are three factors affect whether to add/remove the // FinalizerDeleteDependents: options, existing finalizers of the object, and // e.DeleteStrategy.DefaultGarbageCollectionPolicy. -func shouldDeleteDependents(e *Store, accessor metav1.Object, options *metav1.DeleteOptions) bool { - // Get default orphan policy from this REST object type - if gcStrategy, ok := e.DeleteStrategy.(rest.GarbageCollectionDeleteStrategy); ok && gcStrategy.DefaultGarbageCollectionPolicy() == rest.Unsupported { +func shouldDeleteDependents(ctx genericapirequest.Context, e *Store, accessor metav1.Object, options *metav1.DeleteOptions) bool { + // Get default GC policy from this REST object type + if gcStrategy, ok := e.DeleteStrategy.(rest.GarbageCollectionDeleteStrategy); ok && gcStrategy.DefaultGarbageCollectionPolicy(ctx) == rest.Unsupported { // return false to indicate that we should NOT delete in foreground return false } @@ -789,12 +792,12 @@ func shouldDeleteDependents(e *Store, accessor metav1.Object, options *metav1.De // The finalizers returned are intended to be handled by the garbage collector. // If garbage collection is disabled for the store, this function returns false // to ensure finalizers aren't set which will never be cleared. -func deletionFinalizersForGarbageCollection(e *Store, accessor metav1.Object, options *metav1.DeleteOptions) (bool, []string) { +func deletionFinalizersForGarbageCollection(ctx genericapirequest.Context, 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) + shouldOrphan := shouldOrphanDependents(ctx, e, accessor, options) + shouldDeleteDependentInForeground := shouldDeleteDependents(ctx, e, accessor, options) newFinalizers := []string{} // first remove both finalizers, add them back if needed. @@ -880,7 +883,7 @@ func (e *Store) updateForGracefulDeletionAndFinalizers(ctx genericapirequest.Con if err != nil { return nil, err } - needsUpdate, newFinalizers := deletionFinalizersForGarbageCollection(e, existingAccessor, options) + needsUpdate, newFinalizers := deletionFinalizersForGarbageCollection(ctx, e, existingAccessor, options) if needsUpdate { existingAccessor.SetFinalizers(newFinalizers) } @@ -972,7 +975,7 @@ func (e *Store) Delete(ctx genericapirequest.Context, name string, options *meta // Handle combinations of graceful deletion and finalization by issuing // the correct updates. - shouldUpdateFinalizers, _ := deletionFinalizersForGarbageCollection(e, accessor, options) + shouldUpdateFinalizers, _ := deletionFinalizersForGarbageCollection(ctx, 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) diff --git a/pkg/registry/generic/registry/store_test.go b/pkg/registry/generic/registry/store_test.go index 9e632113c..14d8fa6a3 100644 --- a/pkg/registry/generic/registry/store_test.go +++ b/pkg/registry/generic/registry/store_test.go @@ -35,6 +35,7 @@ import ( "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/runtime/serializer" "k8s.io/apimachinery/pkg/selection" "k8s.io/apimachinery/pkg/util/validation/field" @@ -82,7 +83,7 @@ type testOrphanDeleteStrategy struct { *testRESTStrategy } -func (t *testOrphanDeleteStrategy) DefaultGarbageCollectionPolicy() rest.GarbageCollectionPolicy { +func (t *testOrphanDeleteStrategy) DefaultGarbageCollectionPolicy(ctx genericapirequest.Context) rest.GarbageCollectionPolicy { return rest.OrphanDependents } @@ -2005,3 +2006,93 @@ func denyCreateValidation(obj runtime.Object) error { func denyUpdateValidation(obj, old runtime.Object) error { return fmt.Errorf("admission denied") } + +type fakeStrategy struct { + runtime.ObjectTyper + names.NameGenerator +} + +func (fakeStrategy) DefaultGarbageCollectionPolicy(ctx genericapirequest.Context) rest.GarbageCollectionPolicy { + appsv1beta1 := schema.GroupVersion{Group: "apps", Version: "v1beta1"} + appsv1beta2 := schema.GroupVersion{Group: "apps", Version: "v1beta2"} + extensionsv1beta1 := schema.GroupVersion{Group: "extensions", Version: "v1beta1"} + if requestInfo, found := genericapirequest.RequestInfoFrom(ctx); found { + groupVersion := schema.GroupVersion{Group: requestInfo.APIGroup, Version: requestInfo.APIVersion} + switch groupVersion { + case appsv1beta1, appsv1beta2, extensionsv1beta1: + // for back compatibility + return rest.OrphanDependents + default: + return rest.DeleteDependents + } + } + return rest.OrphanDependents +} + +func TestDeletionFinalizersForGarbageCollection(t *testing.T) { + destroyFunc, registry := NewTestGenericStoreRegistry(t) + defer destroyFunc() + + registry.DeleteStrategy = fakeStrategy{} + registry.EnableGarbageCollection = true + + tests := []struct { + requestInfo genericapirequest.RequestInfo + desiredFinalizers []string + isNilRequestInfo bool + changed bool + }{ + { + genericapirequest.RequestInfo{ + APIGroup: "extensions", + APIVersion: "v1beta1", + }, + []string{metav1.FinalizerOrphanDependents}, + false, + true, + }, + { + genericapirequest.RequestInfo{ + APIGroup: "apps", + APIVersion: "v1beta1", + }, + []string{metav1.FinalizerOrphanDependents}, + false, + true, + }, + { + genericapirequest.RequestInfo{ + APIGroup: "apps", + APIVersion: "v1beta2", + }, + []string{metav1.FinalizerOrphanDependents}, + false, + true, + }, + { + genericapirequest.RequestInfo{ + APIGroup: "apps", + APIVersion: "v1", + }, + []string{}, + false, + false, + }, + } + + for _, test := range tests { + context := genericapirequest.NewContext() + if !test.isNilRequestInfo { + context = genericapirequest.WithRequestInfo(context, &test.requestInfo) + } + changed, finalizers := deletionFinalizersForGarbageCollection(context, registry, &example.ReplicaSet{}, &metav1.DeleteOptions{}) + if !changed { + if test.changed { + t.Errorf("%s/%s: no new finalizers are added", test.requestInfo.APIGroup, test.requestInfo.APIVersion) + } + } else if !reflect.DeepEqual(finalizers, test.desiredFinalizers) { + t.Errorf("%s/%s: want %#v, got %#v", test.requestInfo.APIGroup, test.requestInfo.APIVersion, + test.desiredFinalizers, finalizers) + } + } +} diff --git a/pkg/registry/rest/delete.go b/pkg/registry/rest/delete.go index d7782a279..11a1474c8 100644 --- a/pkg/registry/rest/delete.go +++ b/pkg/registry/rest/delete.go @@ -48,7 +48,7 @@ const ( // orphan dependents by default. type GarbageCollectionDeleteStrategy interface { // DefaultGarbageCollectionPolicy returns the default garbage collection behavior. - DefaultGarbageCollectionPolicy() GarbageCollectionPolicy + DefaultGarbageCollectionPolicy(ctx genericapirequest.Context) GarbageCollectionPolicy } // RESTGracefulDeleteStrategy must be implemented by the registry that supports