From 67b7c7290ac76e0ae619263ffc116d9b1e596181 Mon Sep 17 00:00:00 2001 From: Chao Xu Date: Wed, 17 Aug 2016 14:23:09 -0700 Subject: [PATCH] Allow per-resource default garbage collection behavior --- pkg/api/rest/delete.go | 14 ++ pkg/registry/controller/strategy.go | 7 + pkg/registry/generic/registry/store.go | 54 ++++--- pkg/registry/generic/registry/store_test.go | 142 +++++++++++++++--- pkg/registry/replicaset/strategy.go | 7 + test/e2e/garbage_collector.go | 49 +++++- .../garbage_collector_test.go | 8 +- 7 files changed, 236 insertions(+), 45 deletions(-) diff --git a/pkg/api/rest/delete.go b/pkg/api/rest/delete.go index 0fe7af4340..5a76f214a6 100644 --- a/pkg/api/rest/delete.go +++ b/pkg/api/rest/delete.go @@ -32,6 +32,20 @@ type RESTDeleteStrategy interface { runtime.ObjectTyper } +type GarbageCollectionPolicy string + +const ( + DeleteDependents GarbageCollectionPolicy = "DeleteDependents" + OrphanDependents GarbageCollectionPolicy = "OrphanDependents" +) + +// GarbageCollectionDeleteStrategy must be implemented by the registry that wants to +// orphan dependents by default. +type GarbageCollectionDeleteStrategy interface { + // DefaultGarbageCollectionPolicy returns the default garbage collection behavior. + DefaultGarbageCollectionPolicy() GarbageCollectionPolicy +} + // RESTGracefulDeleteStrategy must be implemented by the registry that supports // graceful deletion. type RESTGracefulDeleteStrategy interface { diff --git a/pkg/registry/controller/strategy.go b/pkg/registry/controller/strategy.go index 6b96706b60..f206512ec9 100644 --- a/pkg/registry/controller/strategy.go +++ b/pkg/registry/controller/strategy.go @@ -24,6 +24,7 @@ import ( "strconv" "k8s.io/kubernetes/pkg/api" + "k8s.io/kubernetes/pkg/api/rest" "k8s.io/kubernetes/pkg/api/validation" "k8s.io/kubernetes/pkg/fields" "k8s.io/kubernetes/pkg/labels" @@ -41,6 +42,12 @@ type rcStrategy struct { // Strategy is the default logic that applies when creating and updating Replication Controller objects. var Strategy = rcStrategy{api.Scheme, api.SimpleNameGenerator} +// DefaultGarbageCollectionPolicy returns Orphan because that was the default +// behavior before the server-side garbage collection was implemented. +func (rcStrategy) DefaultGarbageCollectionPolicy() rest.GarbageCollectionPolicy { + return rest.OrphanDependents +} + // NamespaceScoped returns true because all Replication Controllers need to be within a namespace. func (rcStrategy) NamespaceScoped() bool { return true diff --git a/pkg/registry/generic/registry/store.go b/pkg/registry/generic/registry/store.go index 99ee27691b..9cf3872d8c 100644 --- a/pkg/registry/generic/registry/store.go +++ b/pkg/registry/generic/registry/store.go @@ -445,29 +445,49 @@ var ( errEmptiedFinalizers = fmt.Errorf("emptied finalizers") ) -// return if we need to update the finalizers of the object, and the desired list of finalizers -func shouldUpdateFinalizers(accessor meta.Object, options *api.DeleteOptions) (shouldUpdate bool, newFinalizers []string) { - if options == nil || options.OrphanDependents == nil { - return false, accessor.GetFinalizers() +// shouldUpdateFinalizers returns if we need to update the finalizers of the +// object, and the desired list of finalizers. +// When deciding whether to add the OrphanDependent finalizer, factors in the +// order of highest to lowest priority are: options.OrphanDependents, existing +// finalizers of the object, e.DeleteStrategy.DefaultGarbageCollectionPolicy. +func shouldUpdateFinalizers(e *Store, accessor meta.Object, options *api.DeleteOptions) (shouldUpdate bool, newFinalizers []string) { + shouldOrphan := false + // Get default orphan policy from this REST object type + if gcStrategy, ok := e.DeleteStrategy.(rest.GarbageCollectionDeleteStrategy); ok { + if gcStrategy.DefaultGarbageCollectionPolicy() == rest.OrphanDependents { + shouldOrphan = true + } } - shouldOrphan := *options.OrphanDependents - alreadyOrphan := false + // If a finalizer is set in the object, it overrides the default + hasOrphanFinalizer := false finalizers := accessor.GetFinalizers() - newFinalizers = make([]string, 0, len(finalizers)) for _, f := range finalizers { if f == api.FinalizerOrphan { - alreadyOrphan = true - if !shouldOrphan { + shouldOrphan = true + hasOrphanFinalizer = true + break + } + // TODO: update this when we add a finalizer indicating a preference for the other behavior + } + // If an explicit policy was set at deletion time, that overrides both + if options != nil && options.OrphanDependents != nil { + shouldOrphan = *options.OrphanDependents + } + if shouldOrphan && !hasOrphanFinalizer { + finalizers = append(finalizers, api.FinalizerOrphan) + return true, finalizers + } + if !shouldOrphan && hasOrphanFinalizer { + var newFinalizers []string + for _, f := range finalizers { + if f == api.FinalizerOrphan { continue } + newFinalizers = append(newFinalizers, f) } - newFinalizers = append(newFinalizers, f) + return true, newFinalizers } - if shouldOrphan && !alreadyOrphan { - newFinalizers = append(newFinalizers, api.FinalizerOrphan) - } - shouldUpdate = shouldOrphan != alreadyOrphan - return shouldUpdate, newFinalizers + return false, finalizers } // markAsDeleting sets the obj's DeletionGracePeriodSeconds to 0, and sets the @@ -560,7 +580,7 @@ func (e *Store) updateForGracefulDeletionAndFinalizers(ctx api.Context, name, ke if err != nil { return nil, err } - shouldUpdate, newFinalizers := shouldUpdateFinalizers(existingAccessor, options) + shouldUpdate, newFinalizers := shouldUpdateFinalizers(e, existingAccessor, options) if shouldUpdate { existingAccessor.SetFinalizers(newFinalizers) } @@ -654,7 +674,7 @@ func (e *Store) Delete(ctx api.Context, name string, options *api.DeleteOptions) err, ignoreNotFound, deleteImmediately, out, lastExisting = e.updateForGracefulDeletion(ctx, name, key, options, preconditions, obj) } } else { - shouldUpdateFinalizers, _ := shouldUpdateFinalizers(accessor, options) + shouldUpdateFinalizers, _ := shouldUpdateFinalizers(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 17c4e4491b..65bf2b2e7d 100644 --- a/pkg/registry/generic/registry/store_test.go +++ b/pkg/registry/generic/registry/store_test.go @@ -46,6 +46,14 @@ import ( "k8s.io/kubernetes/pkg/util/wait" ) +type testOrphanDeleteStrategy struct { + *testRESTStrategy +} + +func (t *testOrphanDeleteStrategy) DefaultGarbageCollectionPolicy() rest.GarbageCollectionPolicy { + return rest.OrphanDependents +} + type testRESTStrategy struct { runtime.ObjectTyper api.NameGenerator @@ -680,9 +688,16 @@ func TestStoreDeleteWithOrphanDependents(t *testing.T) { nonOrphanOptions := &api.DeleteOptions{OrphanDependents: &falseVar} nilOrphanOptions := &api.DeleteOptions{} + // defaultDeleteStrategy doesn't implement rest.GarbageCollectionDeleteStrategy. + defaultDeleteStrategy := &testRESTStrategy{api.Scheme, api.SimpleNameGenerator, true, false, true} + // orphanDeleteStrategy indicates the default garbage collection policy is + // to orphan dependentes. + orphanDeleteStrategy := &testOrphanDeleteStrategy{defaultDeleteStrategy} + testcases := []struct { pod *api.Pod options *api.DeleteOptions + strategy rest.RESTDeleteStrategy expectNotFound bool updatedFinalizers []string }{ @@ -690,99 +705,179 @@ func TestStoreDeleteWithOrphanDependents(t *testing.T) { { podWithOrphanFinalizer("pod1"), orphanOptions, + defaultDeleteStrategy, false, []string{"foo.com/x", api.FinalizerOrphan, "bar.com/y"}, }, { podWithOtherFinalizers("pod2"), orphanOptions, + defaultDeleteStrategy, false, []string{"foo.com/x", "bar.com/y", api.FinalizerOrphan}, }, { podWithNoFinalizer("pod3"), orphanOptions, + defaultDeleteStrategy, false, []string{api.FinalizerOrphan}, }, { podWithOnlyOrphanFinalizer("pod4"), orphanOptions, + defaultDeleteStrategy, false, []string{api.FinalizerOrphan}, }, // cases run with DeleteOptions.OrphanDedependents=false + // these cases all have oprhanDeleteStrategy, which should be ignored + // because DeleteOptions has the highest priority. { podWithOrphanFinalizer("pod5"), nonOrphanOptions, + orphanDeleteStrategy, false, []string{"foo.com/x", "bar.com/y"}, }, { podWithOtherFinalizers("pod6"), nonOrphanOptions, + orphanDeleteStrategy, false, []string{"foo.com/x", "bar.com/y"}, }, { podWithNoFinalizer("pod7"), nonOrphanOptions, + orphanDeleteStrategy, true, []string{}, }, { podWithOnlyOrphanFinalizer("pod8"), nonOrphanOptions, + orphanDeleteStrategy, true, []string{}, }, - // cases run with nil DeleteOptions, the finalizers are not updated. + // cases run with nil DeleteOptions.OrphanDependents. If the object + // already has the orphan finalizer, then the DeleteStrategy should be + // ignored. Otherwise the DeleteStrategy decides whether to add the + // orphan finalizer. { podWithOrphanFinalizer("pod9"), - nil, + nilOrphanOptions, + defaultDeleteStrategy, false, []string{"foo.com/x", api.FinalizerOrphan, "bar.com/y"}, }, { - podWithOtherFinalizers("pod10"), - nil, + podWithOrphanFinalizer("pod10"), + nilOrphanOptions, + orphanDeleteStrategy, + false, + []string{"foo.com/x", api.FinalizerOrphan, "bar.com/y"}, + }, + { + podWithOtherFinalizers("pod11"), + nilOrphanOptions, + defaultDeleteStrategy, false, []string{"foo.com/x", "bar.com/y"}, }, { - podWithNoFinalizer("pod11"), - nil, + podWithOtherFinalizers("pod12"), + nilOrphanOptions, + orphanDeleteStrategy, + false, + []string{"foo.com/x", "bar.com/y", api.FinalizerOrphan}, + }, + { + podWithNoFinalizer("pod13"), + nilOrphanOptions, + defaultDeleteStrategy, true, []string{}, }, { - podWithOnlyOrphanFinalizer("pod12"), - nil, + podWithNoFinalizer("pod14"), + nilOrphanOptions, + orphanDeleteStrategy, false, []string{api.FinalizerOrphan}, }, - // cases run with non-nil DeleteOptions, but nil OrphanDependents, it's treated as OrphanDependents=true { - podWithOrphanFinalizer("pod13"), + podWithOnlyOrphanFinalizer("pod15"), nilOrphanOptions, + defaultDeleteStrategy, false, - []string{"foo.com/x", api.FinalizerOrphan, "bar.com/y"}, - }, - { - podWithOtherFinalizers("pod14"), - nilOrphanOptions, - false, - []string{"foo.com/x", "bar.com/y"}, - }, - { - podWithNoFinalizer("pod15"), - nilOrphanOptions, - true, - []string{}, + []string{api.FinalizerOrphan}, }, { podWithOnlyOrphanFinalizer("pod16"), nilOrphanOptions, + orphanDeleteStrategy, + false, + []string{api.FinalizerOrphan}, + }, + + // cases run with nil DeleteOptions should have exact same behavior. + // They should be exactly the same as above cases where + // DeleteOptions.OrphanDependents is nil. + { + podWithOrphanFinalizer("pod17"), + nil, + defaultDeleteStrategy, + false, + []string{"foo.com/x", api.FinalizerOrphan, "bar.com/y"}, + }, + { + podWithOrphanFinalizer("pod18"), + nil, + orphanDeleteStrategy, + false, + []string{"foo.com/x", api.FinalizerOrphan, "bar.com/y"}, + }, + { + podWithOtherFinalizers("pod19"), + nil, + defaultDeleteStrategy, + false, + []string{"foo.com/x", "bar.com/y"}, + }, + { + podWithOtherFinalizers("pod20"), + nil, + orphanDeleteStrategy, + false, + []string{"foo.com/x", "bar.com/y", api.FinalizerOrphan}, + }, + { + podWithNoFinalizer("pod21"), + nil, + defaultDeleteStrategy, + true, + []string{}, + }, + { + podWithNoFinalizer("pod22"), + nil, + orphanDeleteStrategy, + false, + []string{api.FinalizerOrphan}, + }, + { + podWithOnlyOrphanFinalizer("pod23"), + nil, + defaultDeleteStrategy, + false, + []string{api.FinalizerOrphan}, + }, + { + podWithOnlyOrphanFinalizer("pod24"), + nil, + orphanDeleteStrategy, false, []string{api.FinalizerOrphan}, }, @@ -793,6 +888,7 @@ func TestStoreDeleteWithOrphanDependents(t *testing.T) { defer server.Terminate(t) for _, tc := range testcases { + registry.DeleteStrategy = tc.strategy // create pod _, err := registry.Create(testContext, tc.pod) if err != nil { diff --git a/pkg/registry/replicaset/strategy.go b/pkg/registry/replicaset/strategy.go index a8adc95cdf..b7c78d2c9f 100644 --- a/pkg/registry/replicaset/strategy.go +++ b/pkg/registry/replicaset/strategy.go @@ -24,6 +24,7 @@ import ( "strconv" "k8s.io/kubernetes/pkg/api" + "k8s.io/kubernetes/pkg/api/rest" "k8s.io/kubernetes/pkg/apis/extensions" "k8s.io/kubernetes/pkg/apis/extensions/validation" "k8s.io/kubernetes/pkg/fields" @@ -42,6 +43,12 @@ type rsStrategy struct { // Strategy is the default logic that applies when creating and updating ReplicaSet objects. var Strategy = rsStrategy{api.Scheme, api.SimpleNameGenerator} +// DefaultGarbageCollectionPolicy returns Orphan because that's the default +// behavior before the server-side garbage collection is implemented. +func (rsStrategy) DefaultGarbageCollectionPolicy() rest.GarbageCollectionPolicy { + return rest.OrphanDependents +} + // NamespaceScoped returns true because all ReplicaSets need to be within a namespace. func (rsStrategy) NamespaceScoped() bool { return true diff --git a/test/e2e/garbage_collector.go b/test/e2e/garbage_collector.go index de8e9eba7d..467785c883 100644 --- a/test/e2e/garbage_collector.go +++ b/test/e2e/garbage_collector.go @@ -168,7 +168,7 @@ var _ = framework.KubeDescribe("Garbage collector", func() { gatherMetrics(f) }) - It("[Feature:GarbageCollector] should orphan pods created by rc", func() { + It("[Feature:GarbageCollector] should orphan pods created by rc if delete options say so", func() { clientSet := f.Clientset_1_3 rcClient := clientSet.Core().ReplicationControllers(f.Namespace.Name) podClient := clientSet.Core().Pods(f.Namespace.Name) @@ -214,4 +214,51 @@ var _ = framework.KubeDescribe("Garbage collector", func() { } gatherMetrics(f) }) + + It("[Feature:GarbageCollector] should orphan pods created by rc if deleteOptions.OrphanDependents is nil", func() { + clientSet := f.Clientset_1_3 + rcClient := clientSet.Core().ReplicationControllers(f.Namespace.Name) + podClient := clientSet.Core().Pods(f.Namespace.Name) + rcName := "simpletest.rc" + rc := newOwnerRC(f, rcName) + By("create the rc") + rc, err := rcClient.Create(rc) + if err != nil { + framework.Failf("Failed to create replication controller: %v", err) + } + // wait for rc to create some pods + if err := wait.Poll(5*time.Second, 30*time.Second, func() (bool, error) { + rc, err := rcClient.Get(rc.Name) + if err != nil { + return false, fmt.Errorf("Failed to get rc: %v", err) + } + if rc.Status.Replicas == *rc.Spec.Replicas { + return true, nil + } else { + return false, nil + } + }); err != nil { + framework.Failf("failed to wait for the rc.Status.Replicas to reach rc.Spec.Replicas: %v", err) + } + By("delete the rc") + deleteOptions := &api.DeleteOptions{} + deleteOptions.Preconditions = api.NewUIDPreconditions(string(rc.UID)) + if err := rcClient.Delete(rc.ObjectMeta.Name, deleteOptions); err != nil { + framework.Failf("failed to delete the rc: %v", err) + } + By("wait for 30 seconds to see if the garbage collector mistakenly deletes the pods") + if err := wait.Poll(5*time.Second, 30*time.Second, func() (bool, error) { + pods, err := podClient.List(api.ListOptions{}) + if err != nil { + return false, fmt.Errorf("Failed to list pods: %v", err) + } + if e, a := int(*(rc.Spec.Replicas)), len(pods.Items); e != a { + return false, fmt.Errorf("expect %d pods, got %d pods", e, a) + } + return false, nil + }); err != nil && err != wait.ErrWaitTimeout { + framework.Failf("%v", err) + } + gatherMetrics(f) + }) }) diff --git a/test/integration/garbagecollector/garbage_collector_test.go b/test/integration/garbagecollector/garbage_collector_test.go index 226d3482ad..91514294c1 100644 --- a/test/integration/garbagecollector/garbage_collector_test.go +++ b/test/integration/garbagecollector/garbage_collector_test.go @@ -209,7 +209,7 @@ func TestCascadingDeletion(t *testing.T) { go gc.Run(5, stopCh) defer close(stopCh) // delete one of the replication controller - if err := rcClient.Delete(toBeDeletedRCName, nil); err != nil { + if err := rcClient.Delete(toBeDeletedRCName, getNonOrphanOptions()); err != nil { t.Fatalf("failed to delete replication controller: %v", err) } @@ -374,7 +374,7 @@ func TestStressingCascadingDeletion(t *testing.T) { wg.Add(collections * 4) rcUIDs := make(chan types.UID, collections*4) for i := 0; i < collections; i++ { - // rc is created with empty finalizers, deleted with nil delete options, pods will be deleted + // rc is created with empty finalizers, deleted with nil delete options, pods will remain. go setupRCsPods(t, gc, clientSet, "collection1-"+strconv.Itoa(i), ns.Name, []string{}, nil, &wg, rcUIDs) // rc is created with the orphan finalizer, deleted with nil options, pods will remain. go setupRCsPods(t, gc, clientSet, "collection2-"+strconv.Itoa(i), ns.Name, []string{api.FinalizerOrphan}, nil, &wg, rcUIDs) @@ -397,7 +397,7 @@ func TestStressingCascadingDeletion(t *testing.T) { if err := wait.Poll(5*time.Second, 30*time.Second, func() (bool, error) { podsInEachCollection := 3 // see the comments on the calls to setupRCsPods for details - remainingGroups := 2 + remainingGroups := 3 return verifyRemainingObjects(t, clientSet, ns.Name, 0, collections*podsInEachCollection*remainingGroups) }); err != nil { t.Fatal(err) @@ -411,7 +411,7 @@ func TestStressingCascadingDeletion(t *testing.T) { t.Fatal(err) } for _, pod := range pods.Items { - if !strings.Contains(pod.ObjectMeta.Name, "collection2-") && !strings.Contains(pod.ObjectMeta.Name, "collection4-") { + if !strings.Contains(pod.ObjectMeta.Name, "collection1-") && !strings.Contains(pod.ObjectMeta.Name, "collection2-") && !strings.Contains(pod.ObjectMeta.Name, "collection4-") { t.Errorf("got unexpected remaining pod: %#v", pod) } }