[GarbageCollector] Add orphaning finalizer logic to GC #25599

caesarxuchao · 2016-05-13T23:35:21Z

If the `--enable-garbage-collector` (alpha feature) is set when starting the apiserver and the controller manager, sending deletion request with `DeleteOptions.OrphanDependents=true` will instruct the garbage collector to orphan the object's dependents, that is, to not delete the dependents.

Added the orphaning finalizer logic to GC
Added the generic finalizer logic to the APIServer
Added the logic that handles DeleteOptions.OrphanDependents to the APIServer

Design doc is at https://github.com/kubernetes/kubernetes/blob/master/docs/proposals/garbage-collection.md

@lavalamp @gmarek @kubernetes/sig-api-machinery

This change is

caesarxuchao · 2016-05-13T23:46:33Z

pkg/api/validation/validation.go

@@ -395,7 +395,7 @@ func ValidateObjectMetaUpdate(newMeta, oldMeta *api.ObjectMeta, fldPath *field.P
 	allErrs = append(allErrs, ValidateImmutableField(newMeta.Namespace, oldMeta.Namespace, fldPath.Child("namespace"))...)
 	allErrs = append(allErrs, ValidateImmutableField(newMeta.UID, oldMeta.UID, fldPath.Child("uid"))...)
 	allErrs = append(allErrs, ValidateImmutableField(newMeta.CreationTimestamp, oldMeta.CreationTimestamp, fldPath.Child("creationTimestamp"))...)
-	allErrs = append(allErrs, ValidateImmutableField(newMeta.Finalizers, oldMeta.Finalizers, fldPath.Child("finalizers"))...)
+	// allErrs = append(allErrs, ValidateImmutableField(newMeta.Finalizers, oldMeta.Finalizers, fldPath.Child("finalizers"))...)


I'll remove the line.

caesarxuchao · 2016-05-16T04:52:04Z

pkg/controller/garbagecollector/garbagecollector.go

+		fmt.Println("CHAO: e.oldObj=e.oldObj")
+	}
+	if e.oldObj == nil {
+		if accessor.GetDeletionTimestamp() == nil {


This line handles the case where the DeletionTimestamp is set at the first time GC sees it

caesarxuchao · 2016-05-16T06:32:23Z

Still missing one piece of code: the API server's deletion handler needs to add the orphan finalizer if the deleteOption has Orphaning set.

caesarxuchao · 2016-05-17T08:23:38Z

One minor problem: the dynamic client cannot decode the v1.Status returned by the API server for Update call, so the gc always retries the maximum times to remove the orphan finalizer from owner's finalziers list. This is causes inefficiency, but doesn't affect the functionality.

caesarxuchao · 2016-05-17T08:37:41Z

All tests are passing locally. Some remaining work:

we need to defaults the DeleteOptions.OrphanDependents to true
we need to fix the dynamic client to handle the return of the Update call correctly.

I think 2 is not required for 1.3. 1 may be required, but given that we don't automatically set OwnerReferences, and we gc is default turning off, it might not be that critical. @lavalamp what do you think?

liggitt · 2016-05-24T17:48:22Z

pkg/registry/generic/registry/store.go

+	if delete {
+		out := e.NewFunc()
+		glog.V(6).Infof("going to delete %s from regitry, triggered by update", name)
+		if err := e.Storage.Delete(ctx, key, out, preconditions); err != nil {


how does finalized delete interact with graceful delete?

The object is deleted here only if the update request empties the finalizers and the DeletionGracePeriods is already set to 0.
So if the request just empties the finalizers, and the DeletionGracePeriods is not 0, then it will be handled as a regular update. The object will be deleted until someone sends a deletion request with DeletionGracePeriod=0.

liggitt · 2016-05-24T19:05:48Z

pkg/registry/generic/registry/store.go

@@ -255,10 +303,10 @@ func (e *Store) Update(ctx api.Context, name string, objInfo rest.UpdatedObjectI
 	}

 	out := e.NewFunc()
-
+	var obj runtime.Object


call this deletedObj and only set it in the delete case to make it clear what should be using it outside the func

caesarxuchao · 2016-05-24T19:50:46Z

e2e failure is a flake: #26131

liggitt · 2016-05-24T20:05:34Z

pkg/registry/generic/registry/store.go

+		utilruntime.HandleError(err)
+		return false
+	}
+	return len(newMeta.Finalizers) == 0 && oldMeta.DeletionGracePeriodSeconds != nil && *oldMeta.DeletionGracePeriodSeconds == 0


shouldn't we also check len(oldMeta.Finalizers) > 0?

No, that would make the apiserver non-idempotent. Here is the previous discussion: #25599 (comment)

hmm... then this isn't really checking that this update is removing finalizers, is it?

Your statement is correct. My reasoning is: If oldMeta.DeletionGracePeriodSeconds==0, and the object still exists, it could only be that the finalizers were not empty, or the apiserver had crashed, either way we should delete the object now.

…dling DeleteOptions.OrphanDependents in the API server

caesarxuchao · 2016-05-24T20:15:25Z

@k8s-oncall. It seems the merge-bot stops checking the labels after rebase, it doesn't remove the needs-rebase label nor the lgtm.

caesarxuchao · 2016-05-24T20:24:25Z

I added back the lgtm after rebase to make sure this PR get in v1.3. @liggitt @lavalamp feel free to remove it if you have more concerns.

nikhiljindal · 2016-05-24T20:34:16Z

docs/admin/kube-apiserver.md

@@ -72,6 +72,7 @@ kube-apiserver
      --cors-allowed-origins=[]: List of allowed origins for CORS, comma separated.  An allowed origin can be a regular expression to support subdomain matching.  If this list is empty CORS will not be enabled.
      --delete-collection-workers=1: Number of workers spawned for DeleteCollection call. These are used to speed up namespace cleanup.
      --deserialization-cache-size=50000: Number of deserialized json objects to cache in memory.
+      --enable-garbage-collector[=false]: Enables the generic garbage collector. MUST be synced with the corresponding flag of the kube-controller-manager.


Do you want to say something about this being an alpha feature and admins should be careful and understand the risks before setting this to true? :)

caesarxuchao · 2016-05-24T20:40:42Z

@k8s-bot unit test this, issue #25967

alex-mohr · 2016-05-26T17:48:44Z

Because submit queue is blocked, I'm going to kick off re-tests of the top 5 PRs in the queue, then merge them if they pass.
@k8s-bot test this please: issue #IGNORE

caesarxuchao · 2016-05-26T18:03:22Z

The node e2e error seems to be a flake:

package gopkg.in/yaml.v2: unrecognized import path "gopkg.in/yaml.v2" (https fetch: Get https://gopkg.in/yaml.v2?go-get=1: net/http: TLS handshake timeout)

caesarxuchao · 2016-05-26T18:03:55Z

@k8s-bot node e2e test this, issue #IGNORE

k8s-bot · 2016-05-26T18:56:30Z

GCE e2e build/test passed for commit 1665546.

caesarxuchao · 2016-05-26T20:07:26Z

@alex-mohr all tests passed. Could you merge? Thanks.

erictune · 2016-07-02T01:14:27Z

@caesarxuchao Does this PR require action by the user when upgrading from 1.2.x to 1.3.0? (Think about non-developer users.) If so, please edit your first comment to have a release-note block, like in #28132. If it is just an optional feature, please change the label to just release-note. If it is not a complete feature by itself, then apply "release-note-none" label instead.

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

googlebot added the cla: yes label May 13, 2016

caesarxuchao force-pushed the orphaning-finalizer branch from c6ca084 to d90165a Compare May 13, 2016 23:37

k8s-github-robot assigned thockin May 13, 2016

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels May 13, 2016

caesarxuchao reviewed May 13, 2016
View reviewed changes

thockin assigned lavalamp and unassigned thockin May 14, 2016

caesarxuchao force-pushed the orphaning-finalizer branch from d90165a to 592eb83 Compare May 15, 2016 23:53

caesarxuchao changed the title ~~Add orphaning finalizer logic to GC~~ [WIP] Add orphaning finalizer logic to GC May 15, 2016

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

caesarxuchao reviewed May 16, 2016
View reviewed changes

caesarxuchao force-pushed the orphaning-finalizer branch from 3e5a255 to cfb2bdd Compare May 16, 2016 05:12

caesarxuchao force-pushed the orphaning-finalizer branch 2 times, most recently from 6452d0f to 85c3284 Compare May 17, 2016 05:15

caesarxuchao force-pushed the orphaning-finalizer branch from 4bd021a to 97ad501 Compare May 17, 2016 08:34

caesarxuchao added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels May 24, 2016

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

liggitt reviewed May 24, 2016
View reviewed changes

caesarxuchao force-pushed the orphaning-finalizer branch from d45c8d8 to 4964dd3 Compare May 24, 2016 18:49

liggitt reviewed May 24, 2016
View reviewed changes

add finalizer logics to the API server and the garbage collector; han…

1665546

…dling DeleteOptions.OrphanDependents in the API server

caesarxuchao force-pushed the orphaning-finalizer branch from 4964dd3 to 1665546 Compare May 24, 2016 20:07

caesarxuchao removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 24, 2016

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 24, 2016

caesarxuchao added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 24, 2016

nikhiljindal reviewed May 24, 2016
View reviewed changes

alex-mohr merged commit edda837 into kubernetes:master May 26, 2016

caesarxuchao mentioned this pull request May 26, 2016

e2e test flake: package gopkg.in/yaml.v2: unrecognized import path "gopkg.in/yaml.v2" (https fetch: Get https://gopkg.in/yaml.v2?go-get=1: net/http: TLS handshake timeout) #26385

Closed

erictune mentioned this pull request Jul 2, 2016

Need "action required" notes in CHANGELOG.md #28405

Closed

caesarxuchao added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. labels Jul 2, 2016

caesarxuchao changed the title ~~Add orphaning finalizer logic to GC~~ [GarbageCollector] Add orphaning finalizer logic to GC Aug 15, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GarbageCollector] Add orphaning finalizer logic to GC #25599

[GarbageCollector] Add orphaning finalizer logic to GC #25599

caesarxuchao commented May 13, 2016 •

edited by k8s-oncall

Loading

caesarxuchao May 13, 2016

lavalamp May 13, 2016

caesarxuchao May 16, 2016

caesarxuchao commented May 16, 2016

caesarxuchao commented May 17, 2016

caesarxuchao commented May 17, 2016

liggitt May 24, 2016

caesarxuchao May 24, 2016

liggitt May 24, 2016

caesarxuchao May 24, 2016

caesarxuchao May 24, 2016

caesarxuchao commented May 24, 2016

liggitt May 24, 2016

caesarxuchao May 24, 2016

liggitt May 24, 2016

caesarxuchao May 24, 2016

caesarxuchao commented May 24, 2016

caesarxuchao commented May 24, 2016

nikhiljindal May 24, 2016

caesarxuchao commented May 24, 2016

alex-mohr commented May 26, 2016 •

edited

Loading

caesarxuchao commented May 26, 2016 •

edited

Loading

caesarxuchao commented May 26, 2016

k8s-bot commented May 26, 2016

caesarxuchao commented May 26, 2016

erictune commented Jul 2, 2016

[GarbageCollector] Add orphaning finalizer logic to GC #25599

[GarbageCollector] Add orphaning finalizer logic to GC #25599

Conversation

caesarxuchao commented May 13, 2016 • edited by k8s-oncall Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesarxuchao commented May 16, 2016

caesarxuchao commented May 17, 2016

caesarxuchao commented May 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesarxuchao commented May 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesarxuchao commented May 24, 2016

caesarxuchao commented May 24, 2016

Choose a reason for hiding this comment

caesarxuchao commented May 24, 2016

alex-mohr commented May 26, 2016 • edited Loading

caesarxuchao commented May 26, 2016 • edited Loading

caesarxuchao commented May 26, 2016

k8s-bot commented May 26, 2016

caesarxuchao commented May 26, 2016

erictune commented Jul 2, 2016

caesarxuchao commented May 13, 2016 •

edited by k8s-oncall

Loading

alex-mohr commented May 26, 2016 •

edited

Loading

caesarxuchao commented May 26, 2016 •

edited

Loading