[GarbageCollector] Allow per-resource default garbage collection behavior #30838

caesarxuchao · 2016-08-18T00:39:05Z

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.

This change is

caesarxuchao · 2016-08-18T01:00:16Z

cluster/common.sh

 NUM_NODES: $(yaml-quote ${NUM_NODES})
 STORAGE_BACKEND: $(yaml-quote ${STORAGE_BACKEND:-})
-ENABLE_GARBAGE_COLLECTOR: $(yaml-quote ${ENABLE_GARBAGE_COLLECTOR:-false})
+ENABLE_GARBAGE_COLLECTOR: $(yaml-quote ${ENABLE_GARBAGE_COLLECTOR:-})


These will be removed before merge. Just here to run the e2e tests with GC on.

caesarxuchao · 2016-08-18T03:47:18Z

@lavalamp, I only make the RC and RS registry to set OrphanDependents as the default behavior and all tests have passed.
I think we can let the Deployment, Job controller to apply the same default when we convert them to use controllerRef. They are not needed now.

caesarxuchao · 2016-08-18T18:06:07Z

@lavalamp it's ready for review. Thanks.

lavalamp · 2016-08-18T23:40:41Z

Review status: 0 of 10 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

cluster/common.sh, line 649 [r1] (raw file):

Previously, caesarxuchao (Chao Xu) wrote…

These will be removed before merge. Just here to run the e2e tests with GC on.

I don't understand why it's needed-- I thought it was on by default? Did it get turned off?

pkg/registry/controller/strategy.go, line 46 [r3] (raw file):

// DefaultGarbageCollectionPolicy returns Orphan because that's the default
// behavior before the server-side garbage collection is implemented.

nits:
s/that's/that was/
s/is/was/

pkg/registry/generic/registry/store.go, line 455 [r3] (raw file):

func shouldUpdateFinalizers(e *Store, accessor meta.Object, options *api.DeleteOptions) (shouldUpdate bool, newFinalizers []string) {
  // check if the object already has the orphan finalizer set.
  alreadyOrphan := false

For clarity, please change to something like the below:

shouldOrphan := false
// Get default orphan policy from this REST object type
if gcStrategy, ok := e.DeleteStrategy.(rest.GarbageCollectionDeleteStrategy); ok {
    if gcStrategy.DefaultGarbageCollectionPolicy() == rest.OrphanDependents {
        shouldOrhpan = true
    }
}
// If a finalizer is set in the object, it overrides the default
hasOrphanFinalizer := false
finalizers := accessor.GetFinalizers()
for _, f := range finalizers {
    if f == api.FinalizerOrphan {
        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 { /* add the finalizer */ }
if !shouldOrphan && hasOrphanFinalizer { /* remove the finalizer */ }

Comments from Reviewable

caesarxuchao · 2016-08-18T23:46:25Z

Review status: 0 of 10 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

cluster/common.sh, line 649 [r1] (raw file):

Previously, lavalamp (Daniel Smith) wrote…

I don't understand why it's needed-- I thought it was on by default? Did it get turned off?

The PR is not merged. wojtek remove the label because of the scalalibity failure.

pkg/registry/generic/registry/store.go, line 455 [r3] (raw file):

Previously, lavalamp (Daniel Smith) wrote…

For clarity, please change to something like the below:

shouldOrphan := false
// Get default orphan policy from this REST object type
if gcStrategy, ok := e.DeleteStrategy.(rest.GarbageCollectionDeleteStrategy); ok {
  if gcStrategy.DefaultGarbageCollectionPolicy() == rest.OrphanDependents {
      shouldOrhpan = true
  }
}
// If a finalizer is set in the object, it overrides the default
hasOrphanFinalizer := false
finalizers := accessor.GetFinalizers()
for _, f := range finalizers {
  if f == api.FinalizerOrphan {
      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 { /* add the finalizer */ }
if !shouldOrphan && hasOrphanFinalizer { /* remove the finalizer */ }

Thanks! Will do.

Comments from Reviewable

caesarxuchao · 2016-08-20T03:27:06Z

@lavalamp this is needed for 1.4.

k8s-bot · 2016-08-22T20:04:06Z

GCE e2e build/test passed for commit 67b7c72.

lavalamp · 2016-08-22T20:23:49Z

Review status: 0 of 7 files reviewed at latest revision, 2 unresolved discussions.

Comments from Reviewable

caesarxuchao · 2016-08-22T21:11:07Z

Adding the label per previous comment.

k8s-github-robot · 2016-08-23T15:04:16Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-08-23T15:46:28Z

GCE e2e build/test passed for commit 67b7c72.

k8s-github-robot · 2016-08-23T15:46:31Z

Automatic merge from submit-queue

0xmichalis · 2016-10-14T10:23:04Z

pkg/registry/controller/strategy.go

+// 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


Will the policy change if I specify OrphanDependents=false in DeleteOptions now?

Yes. DeleteOptions have the highest priority. The existing finalizers on the object has the second highest priority. This default policy has the lowest priority.

caesarxuchao added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Aug 18, 2016

caesarxuchao self-assigned this Aug 18, 2016

googlebot added the cla: yes label Aug 18, 2016

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Aug 18, 2016

caesarxuchao reviewed Aug 18, 2016
View reviewed changes

caesarxuchao changed the title ~~[WIP][GarbageCollector] Allow per-resource default garbage collection behavior~~ [GarbageCollector] Allow per-resource default garbage collection behavior Aug 18, 2016

caesarxuchao assigned lavalamp and unassigned caesarxuchao Aug 18, 2016

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 19, 2016

caesarxuchao force-pushed the per-resource-orphan-behavior branch from aedf6a8 to c87e640 Compare August 19, 2016 01:09

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 19, 2016

caesarxuchao force-pushed the per-resource-orphan-behavior branch 2 times, most recently from c8bcbad to 9c1ffd2 Compare August 19, 2016 04:26

caesarxuchao added this to the v1.4 milestone Aug 20, 2016

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 20, 2016

Allow per-resource default garbage collection behavior

67b7c72

caesarxuchao force-pushed the per-resource-orphan-behavior branch from 9c1ffd2 to 67b7c72 Compare August 22, 2016 18:37

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 22, 2016

caesarxuchao added release-note Denotes a PR that will be considered when it comes time to generate release notes. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed release-note-label-needed labels Aug 22, 2016

k8s-github-robot merged commit f297ea9 into kubernetes:master Aug 23, 2016

0xmichalis reviewed Oct 14, 2016

View reviewed changes

enisoc mentioned this pull request Feb 1, 2017

Update ControllerRef proposal kubernetes/community#298

Merged

[GarbageCollector] Allow per-resource default garbage collection behavior #30838

[GarbageCollector] Allow per-resource default garbage collection behavior #30838

Uh oh!

Conversation

caesarxuchao commented Aug 18, 2016 • edited by k8s-oncall Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

caesarxuchao Aug 18, 2016

Choose a reason for hiding this comment

Uh oh!

caesarxuchao commented Aug 18, 2016

Uh oh!

caesarxuchao commented Aug 18, 2016

Uh oh!

lavalamp commented Aug 18, 2016

Uh oh!

caesarxuchao commented Aug 18, 2016

Uh oh!

caesarxuchao commented Aug 20, 2016

Uh oh!

k8s-bot commented Aug 22, 2016

Uh oh!

lavalamp commented Aug 22, 2016

Uh oh!

caesarxuchao commented Aug 22, 2016

Uh oh!

k8s-github-robot commented Aug 23, 2016

Uh oh!

k8s-bot commented Aug 23, 2016

Uh oh!

k8s-github-robot commented Aug 23, 2016

Uh oh!

0xmichalis Oct 14, 2016

Choose a reason for hiding this comment

Uh oh!

caesarxuchao Oct 14, 2016

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

caesarxuchao commented Aug 18, 2016 •

edited by k8s-oncall

Loading