Merge pull request #30838 from caesarxuchao/per-resource-orphan-behavior

Automatic merge from submit-queue

[GarbageCollector] Allow per-resource default garbage collection behavior

What's the bug:
When deleting an RC with `deleteOptions.OrphanDependents==nil`, garbage collector is supposed to treat it as `deleteOptions.OrphanDependents==true", and orphan the pods created by it. But the apiserver is not doing that.

What's in the pr:
Allow each resource to specify the default garbage collection behavior in the registry. For example, RC registry's default GC behavior is Orphan, and Pod registry's default GC behavior is CascadingDeletion.

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 {

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

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
+// shouldUpdateFinalizers returns if we need to update the finalizers of the
-func shouldUpdateFinalizers(accessor meta.Object, options *api.DeleteOptions) (shouldUpdate bool, newFinalizers []string) {
+// object, and the desired list of finalizers.
-	if options == nil || options.OrphanDependents == nil {
+// When deciding whether to add the OrphanDependent finalizer, factors in the
-		return false, accessor.GetFinalizers()
+// 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
+	// If a finalizer is set in the object, it overrides the default
-	alreadyOrphan := false
+	hasOrphanFinalizer := false
 	finalizers := accessor.GetFinalizers()
-	newFinalizers = make([]string, 0, len(finalizers))
 	for _, f := range finalizers {
 		if f == api.FinalizerOrphan {
-			alreadyOrphan = true
+			shouldOrphan = true
-			if !shouldOrphan {
+			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 {
+	return false, finalizers
-		newFinalizers = append(newFinalizers, api.FinalizerOrphan)
-	}
-	shouldUpdate = shouldOrphan != alreadyOrphan
-	return shouldUpdate, newFinalizers
 }
 
 // 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)

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"),
+			podWithOrphanFinalizer("pod10"),
-			nil,
+			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"),
+			podWithOtherFinalizers("pod12"),
-			nil,
+			nilOrphanOptions,
+			orphanDeleteStrategy,
+			false,
+			[]string{"foo.com/x", "bar.com/y", api.FinalizerOrphan},
+		},
+		{
+			podWithNoFinalizer("pod13"),
+			nilOrphanOptions,
+			defaultDeleteStrategy,
 			true,
 			[]string{},
 		},
 		{
-			podWithOnlyOrphanFinalizer("pod12"),
+			podWithNoFinalizer("pod14"),
-			nil,
+			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"},
+			[]string{api.FinalizerOrphan},
-		},
-		{
-			podWithOtherFinalizers("pod14"),
-			nilOrphanOptions,
-			false,
-			[]string{"foo.com/x", "bar.com/y"},
-		},
-		{
-			podWithNoFinalizer("pod15"),
-			nilOrphanOptions,
-			true,
-			[]string{},
 		},
 		{
 			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 {

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

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)
+	})
 })

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)
 		}
 	}