Skip to content

Commit f297ea9

Browse files
author
Kubernetes Submit Queue
authored
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.
2 parents d6fb8b0 + 67b7c72 commit f297ea9

File tree

7 files changed

+230
-39
lines changed

7 files changed

+230
-39
lines changed

‎pkg/api/rest/delete.go‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,20 @@ type RESTDeleteStrategy interface {
3232
runtime.ObjectTyper
3333
}
3434

35+
type GarbageCollectionPolicy string
36+
37+
const (
38+
DeleteDependents GarbageCollectionPolicy = "DeleteDependents"
39+
OrphanDependents GarbageCollectionPolicy = "OrphanDependents"
40+
)
41+
42+
// GarbageCollectionDeleteStrategy must be implemented by the registry that wants to
43+
// orphan dependents by default.
44+
type GarbageCollectionDeleteStrategy interface {
45+
// DefaultGarbageCollectionPolicy returns the default garbage collection behavior.
46+
DefaultGarbageCollectionPolicy() GarbageCollectionPolicy
47+
}
48+
3549
// RESTGracefulDeleteStrategy must be implemented by the registry that supports
3650
// graceful deletion.
3751
type RESTGracefulDeleteStrategy interface {

‎pkg/registry/controller/strategy.go‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import (
2424
"strconv"
2525

2626
"k8s.io/kubernetes/pkg/api"
27+
"k8s.io/kubernetes/pkg/api/rest"
2728
"k8s.io/kubernetes/pkg/api/validation"
2829
"k8s.io/kubernetes/pkg/fields"
2930
"k8s.io/kubernetes/pkg/labels"
@@ -41,6 +42,12 @@ type rcStrategy struct {
4142
// Strategy is the default logic that applies when creating and updating Replication Controller objects.
4243
var Strategy = rcStrategy{api.Scheme, api.SimpleNameGenerator}
4344

45+
// DefaultGarbageCollectionPolicy returns Orphan because that was the default
46+
// behavior before the server-side garbage collection was implemented.
47+
func (rcStrategy) DefaultGarbageCollectionPolicy() rest.GarbageCollectionPolicy {
48+
return rest.OrphanDependents
49+
}
50+
4451
// NamespaceScoped returns true because all Replication Controllers need to be within a namespace.
4552
func (rcStrategy) NamespaceScoped() bool {
4653
return true

‎pkg/registry/generic/registry/store.go‎

Lines changed: 38 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -445,29 +445,49 @@ var (
445445
errEmptiedFinalizers = fmt.Errorf("emptied finalizers")
446446
)
447447

448-
// return if we need to update the finalizers of the object, and the desired list of finalizers
449-
func shouldUpdateFinalizers(accessor meta.Object, options *api.DeleteOptions) (shouldUpdate bool, newFinalizers []string) {
450-
if options == nil || options.OrphanDependents == nil {
451-
return false, accessor.GetFinalizers()
452-
}
453-
shouldOrphan := *options.OrphanDependents
454-
alreadyOrphan := false
448+
// shouldUpdateFinalizers returns if we need to update the finalizers of the
449+
// object, and the desired list of finalizers.
450+
// When deciding whether to add the OrphanDependent finalizer, factors in the
451+
// order of highest to lowest priority are: options.OrphanDependents, existing
452+
// finalizers of the object, e.DeleteStrategy.DefaultGarbageCollectionPolicy.
453+
func shouldUpdateFinalizers(e *Store, accessor meta.Object, options *api.DeleteOptions) (shouldUpdate bool, newFinalizers []string) {
454+
shouldOrphan := false
455+
// Get default orphan policy from this REST object type
456+
if gcStrategy, ok := e.DeleteStrategy.(rest.GarbageCollectionDeleteStrategy); ok {
457+
if gcStrategy.DefaultGarbageCollectionPolicy() == rest.OrphanDependents {
458+
shouldOrphan = true
459+
}
460+
}
461+
// If a finalizer is set in the object, it overrides the default
462+
hasOrphanFinalizer := false
455463
finalizers := accessor.GetFinalizers()
456-
newFinalizers = make([]string, 0, len(finalizers))
457464
for _, f := range finalizers {
458465
if f == api.FinalizerOrphan {
459-
alreadyOrphan = true
460-
if !shouldOrphan {
466+
shouldOrphan = true
467+
hasOrphanFinalizer = true
468+
break
469+
}
470+
// TODO: update this when we add a finalizer indicating a preference for the other behavior
471+
}
472+
// If an explicit policy was set at deletion time, that overrides both
473+
if options != nil && options.OrphanDependents != nil {
474+
shouldOrphan = *options.OrphanDependents
475+
}
476+
if shouldOrphan && !hasOrphanFinalizer {
477+
finalizers = append(finalizers, api.FinalizerOrphan)
478+
return true, finalizers
479+
}
480+
if !shouldOrphan && hasOrphanFinalizer {
481+
var newFinalizers []string
482+
for _, f := range finalizers {
483+
if f == api.FinalizerOrphan {
461484
continue
462485
}
486+
newFinalizers = append(newFinalizers, f)
463487
}
464-
newFinalizers = append(newFinalizers, f)
465-
}
466-
if shouldOrphan && !alreadyOrphan {
467-
newFinalizers = append(newFinalizers, api.FinalizerOrphan)
488+
return true, newFinalizers
468489
}
469-
shouldUpdate = shouldOrphan != alreadyOrphan
470-
return shouldUpdate, newFinalizers
490+
return false, finalizers
471491
}
472492

473493
// markAsDeleting sets the obj's DeletionGracePeriodSeconds to 0, and sets the
@@ -560,7 +580,7 @@ func (e *Store) updateForGracefulDeletionAndFinalizers(ctx api.Context, name, ke
560580
if err != nil {
561581
return nil, err
562582
}
563-
shouldUpdate, newFinalizers := shouldUpdateFinalizers(existingAccessor, options)
583+
shouldUpdate, newFinalizers := shouldUpdateFinalizers(e, existingAccessor, options)
564584
if shouldUpdate {
565585
existingAccessor.SetFinalizers(newFinalizers)
566586
}
@@ -654,7 +674,7 @@ func (e *Store) Delete(ctx api.Context, name string, options *api.DeleteOptions)
654674
err, ignoreNotFound, deleteImmediately, out, lastExisting = e.updateForGracefulDeletion(ctx, name, key, options, preconditions, obj)
655675
}
656676
} else {
657-
shouldUpdateFinalizers, _ := shouldUpdateFinalizers(accessor, options)
677+
shouldUpdateFinalizers, _ := shouldUpdateFinalizers(e, accessor, options)
658678
// TODO: remove the check, because we support no-op updates now.
659679
if graceful || pendingFinalizers || shouldUpdateFinalizers {
660680
err, ignoreNotFound, deleteImmediately, out, lastExisting = e.updateForGracefulDeletionAndFinalizers(ctx, name, key, options, preconditions, obj)

‎pkg/registry/generic/registry/store_test.go‎

Lines changed: 112 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,14 @@ import (
4646
"k8s.io/kubernetes/pkg/util/wait"
4747
)
4848

49+
type testOrphanDeleteStrategy struct {
50+
*testRESTStrategy
51+
}
52+
53+
func (t *testOrphanDeleteStrategy) DefaultGarbageCollectionPolicy() rest.GarbageCollectionPolicy {
54+
return rest.OrphanDependents
55+
}
56+
4957
type testRESTStrategy struct {
5058
runtime.ObjectTyper
5159
api.NameGenerator
@@ -680,109 +688,196 @@ func TestStoreDeleteWithOrphanDependents(t *testing.T) {
680688
nonOrphanOptions := &api.DeleteOptions{OrphanDependents: &falseVar}
681689
nilOrphanOptions := &api.DeleteOptions{}
682690

691+
// defaultDeleteStrategy doesn't implement rest.GarbageCollectionDeleteStrategy.
692+
defaultDeleteStrategy := &testRESTStrategy{api.Scheme, api.SimpleNameGenerator, true, false, true}
693+
// orphanDeleteStrategy indicates the default garbage collection policy is
694+
// to orphan dependentes.
695+
orphanDeleteStrategy := &testOrphanDeleteStrategy{defaultDeleteStrategy}
696+
683697
testcases := []struct {
684698
pod *api.Pod
685699
options *api.DeleteOptions
700+
strategy rest.RESTDeleteStrategy
686701
expectNotFound bool
687702
updatedFinalizers []string
688703
}{
689704
// cases run with DeleteOptions.OrphanDedependents=true
690705
{
691706
podWithOrphanFinalizer("pod1"),
692707
orphanOptions,
708+
defaultDeleteStrategy,
693709
false,
694710
[]string{"foo.com/x", api.FinalizerOrphan, "bar.com/y"},
695711
},
696712
{
697713
podWithOtherFinalizers("pod2"),
698714
orphanOptions,
715+
defaultDeleteStrategy,
699716
false,
700717
[]string{"foo.com/x", "bar.com/y", api.FinalizerOrphan},
701718
},
702719
{
703720
podWithNoFinalizer("pod3"),
704721
orphanOptions,
722+
defaultDeleteStrategy,
705723
false,
706724
[]string{api.FinalizerOrphan},
707725
},
708726
{
709727
podWithOnlyOrphanFinalizer("pod4"),
710728
orphanOptions,
729+
defaultDeleteStrategy,
711730
false,
712731
[]string{api.FinalizerOrphan},
713732
},
714733
// cases run with DeleteOptions.OrphanDedependents=false
734+
// these cases all have oprhanDeleteStrategy, which should be ignored
735+
// because DeleteOptions has the highest priority.
715736
{
716737
podWithOrphanFinalizer("pod5"),
717738
nonOrphanOptions,
739+
orphanDeleteStrategy,
718740
false,
719741
[]string{"foo.com/x", "bar.com/y"},
720742
},
721743
{
722744
podWithOtherFinalizers("pod6"),
723745
nonOrphanOptions,
746+
orphanDeleteStrategy,
724747
false,
725748
[]string{"foo.com/x", "bar.com/y"},
726749
},
727750
{
728751
podWithNoFinalizer("pod7"),
729752
nonOrphanOptions,
753+
orphanDeleteStrategy,
730754
true,
731755
[]string{},
732756
},
733757
{
734758
podWithOnlyOrphanFinalizer("pod8"),
735759
nonOrphanOptions,
760+
orphanDeleteStrategy,
736761
true,
737762
[]string{},
738763
},
739-
// cases run with nil DeleteOptions, the finalizers are not updated.
764+
// cases run with nil DeleteOptions.OrphanDependents. If the object
765+
// already has the orphan finalizer, then the DeleteStrategy should be
766+
// ignored. Otherwise the DeleteStrategy decides whether to add the
767+
// orphan finalizer.
740768
{
741769
podWithOrphanFinalizer("pod9"),
742-
nil,
770+
nilOrphanOptions,
771+
defaultDeleteStrategy,
743772
false,
744773
[]string{"foo.com/x", api.FinalizerOrphan, "bar.com/y"},
745774
},
746775
{
747-
podWithOtherFinalizers("pod10"),
748-
nil,
776+
podWithOrphanFinalizer("pod10"),
777+
nilOrphanOptions,
778+
orphanDeleteStrategy,
779+
false,
780+
[]string{"foo.com/x", api.FinalizerOrphan, "bar.com/y"},
781+
},
782+
{
783+
podWithOtherFinalizers("pod11"),
784+
nilOrphanOptions,
785+
defaultDeleteStrategy,
749786
false,
750787
[]string{"foo.com/x", "bar.com/y"},
751788
},
752789
{
753-
podWithNoFinalizer("pod11"),
754-
nil,
790+
podWithOtherFinalizers("pod12"),
791+
nilOrphanOptions,
792+
orphanDeleteStrategy,
793+
false,
794+
[]string{"foo.com/x", "bar.com/y", api.FinalizerOrphan},
795+
},
796+
{
797+
podWithNoFinalizer("pod13"),
798+
nilOrphanOptions,
799+
defaultDeleteStrategy,
755800
true,
756801
[]string{},
757802
},
758803
{
759-
podWithOnlyOrphanFinalizer("pod12"),
760-
nil,
804+
podWithNoFinalizer("pod14"),
805+
nilOrphanOptions,
806+
orphanDeleteStrategy,
761807
false,
762808
[]string{api.FinalizerOrphan},
763809
},
764-
// cases run with non-nil DeleteOptions, but nil OrphanDependents, it's treated as OrphanDependents=true
765810
{
766-
podWithOrphanFinalizer("pod13"),
811+
podWithOnlyOrphanFinalizer("pod15"),
767812
nilOrphanOptions,
813+
defaultDeleteStrategy,
768814
false,
769-
[]string{"foo.com/x", api.FinalizerOrphan, "bar.com/y"},
815+
[]string{api.FinalizerOrphan},
770816
},
771817
{
772-
podWithOtherFinalizers("pod14"),
818+
podWithOnlyOrphanFinalizer("pod16"),
773819
nilOrphanOptions,
820+
orphanDeleteStrategy,
821+
false,
822+
[]string{api.FinalizerOrphan},
823+
},
824+
825+
// cases run with nil DeleteOptions should have exact same behavior.
826+
// They should be exactly the same as above cases where
827+
// DeleteOptions.OrphanDependents is nil.
828+
{
829+
podWithOrphanFinalizer("pod17"),
830+
nil,
831+
defaultDeleteStrategy,
832+
false,
833+
[]string{"foo.com/x", api.FinalizerOrphan, "bar.com/y"},
834+
},
835+
{
836+
podWithOrphanFinalizer("pod18"),
837+
nil,
838+
orphanDeleteStrategy,
839+
false,
840+
[]string{"foo.com/x", api.FinalizerOrphan, "bar.com/y"},
841+
},
842+
{
843+
podWithOtherFinalizers("pod19"),
844+
nil,
845+
defaultDeleteStrategy,
774846
false,
775847
[]string{"foo.com/x", "bar.com/y"},
776848
},
777849
{
778-
podWithNoFinalizer("pod15"),
779-
nilOrphanOptions,
850+
podWithOtherFinalizers("pod20"),
851+
nil,
852+
orphanDeleteStrategy,
853+
false,
854+
[]string{"foo.com/x", "bar.com/y", api.FinalizerOrphan},
855+
},
856+
{
857+
podWithNoFinalizer("pod21"),
858+
nil,
859+
defaultDeleteStrategy,
780860
true,
781861
[]string{},
782862
},
783863
{
784-
podWithOnlyOrphanFinalizer("pod16"),
785-
nilOrphanOptions,
864+
podWithNoFinalizer("pod22"),
865+
nil,
866+
orphanDeleteStrategy,
867+
false,
868+
[]string{api.FinalizerOrphan},
869+
},
870+
{
871+
podWithOnlyOrphanFinalizer("pod23"),
872+
nil,
873+
defaultDeleteStrategy,
874+
false,
875+
[]string{api.FinalizerOrphan},
876+
},
877+
{
878+
podWithOnlyOrphanFinalizer("pod24"),
879+
nil,
880+
orphanDeleteStrategy,
786881
false,
787882
[]string{api.FinalizerOrphan},
788883
},
@@ -793,6 +888,7 @@ func TestStoreDeleteWithOrphanDependents(t *testing.T) {
793888
defer server.Terminate(t)
794889

795890
for _, tc := range testcases {
891+
registry.DeleteStrategy = tc.strategy
796892
// create pod
797893
_, err := registry.Create(testContext, tc.pod)
798894
if err != nil {

‎pkg/registry/replicaset/strategy.go‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import (
2424
"strconv"
2525

2626
"k8s.io/kubernetes/pkg/api"
27+
"k8s.io/kubernetes/pkg/api/rest"
2728
"k8s.io/kubernetes/pkg/apis/extensions"
2829
"k8s.io/kubernetes/pkg/apis/extensions/validation"
2930
"k8s.io/kubernetes/pkg/fields"
@@ -42,6 +43,12 @@ type rsStrategy struct {
4243
// Strategy is the default logic that applies when creating and updating ReplicaSet objects.
4344
var Strategy = rsStrategy{api.Scheme, api.SimpleNameGenerator}
4445

46+
// DefaultGarbageCollectionPolicy returns Orphan because that's the default
47+
// behavior before the server-side garbage collection is implemented.
48+
func (rsStrategy) DefaultGarbageCollectionPolicy() rest.GarbageCollectionPolicy {
49+
return rest.OrphanDependents
50+
}
51+
4552
// NamespaceScoped returns true because all ReplicaSets need to be within a namespace.
4653
func (rsStrategy) NamespaceScoped() bool {
4754
return true

0 commit comments

Comments
 (0)