Allow per-resource default garbage collection behavior

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

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 {

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