change DefaultGarbageCollectionPolicy to DeleteDependents for workloads controllers #55148

dixudx · 2017-11-06T09:18:06Z

What this PR does / why we need it:
As part of the apps/v1 GA effort (kubernetes/enhancements#353) for v1.9. For core controllers, like Deployment, DaemonSet, ReplicaSet, and StatefulSet, changing the DefaultGarbageCollectionPolicy from OrphanDependents to DeleteDependents will make these objects consistent with the default behavior for all new objects.

For legacy API versions, the DefaultGarbageCollectionPolicy remains OrphanDependents.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
ref #55027

Special notes for your reviewer:
/cc @enisoc @caesarxuchao @kow3ns
/assign @kubernetes/sig-apps-api-reviews

Release note:

The default garbage collection policy for Deployment, DaemonSet, StatefulSet, and ReplicaSet has changed from OrphanDependents to DeleteDependents when the deletion is requested through an `apps/v1` endpoint. Clients using older endpoints will be unaffected. This change is only at the REST API level and is independent of the default behavior of particular clients (e.g. this does not affect the default for the kubectl `--cascade` flag).

If you upgrade your client-go libs and use the `AppsV1()` interface, please note that the default garbage collection behavior is changed.

dixudx · 2017-11-06T09:19:52Z

/unassign @derekwaynecarr @eparis
/assign @smarterclayton @janetkuo

enisoc

Thanks for picking this up so quickly! It looks like what I had in mind. Can you just add strategy tests and integration tests like in #50719?

enisoc · 2017-11-06T18:41:34Z

pkg/registry/apps/statefulset/strategy.go

+	if requestInfo, found := genericapirequest.RequestInfoFrom(ctx); found {
+		groupVersion := schema.GroupVersion{Group: requestInfo.APIGroup, Version: requestInfo.APIVersion}
+		switch groupVersion {
+		case extensionsv1beta1.SchemeGroupVersion, appsv1beta1.SchemeGroupVersion, appsv1beta2.SchemeGroupVersion:


You can leave out extensions/v1beta1 for StatefulSet, since it never existed there.

enisoc · 2017-11-06T18:49:46Z

pkg/registry/extensions/daemonset/strategy.go

+	if requestInfo, found := genericapirequest.RequestInfoFrom(ctx); found {
+		groupVersion := schema.GroupVersion{Group: requestInfo.APIGroup, Version: requestInfo.APIVersion}
+		switch groupVersion {
+		case extensionsv1beta1.SchemeGroupVersion, appsv1beta1.SchemeGroupVersion, appsv1beta2.SchemeGroupVersion:


You can leave out apps/v1beta1 for DaemonSet, since it never existed there.

enisoc · 2017-11-06T18:51:45Z

pkg/registry/extensions/replicaset/strategy.go

+	if requestInfo, found := genericapirequest.RequestInfoFrom(ctx); found {
+		groupVersion := schema.GroupVersion{Group: requestInfo.APIGroup, Version: requestInfo.APIVersion}
+		switch groupVersion {
+		case extensionsv1beta1.SchemeGroupVersion, appsv1beta1.SchemeGroupVersion, appsv1beta2.SchemeGroupVersion:


You can leave out apps/v1beta1 for ReplicaSet, since it never existed there.

kow3ns

Generally LGTM but could use some unit tests.

mbohlool · 2017-11-06T21:10:55Z

/cc @mml

dixudx · 2017-11-07T05:53:35Z

@enisoc @kow3ns Updated. PTAL. Thanks.

caesarxuchao · 2017-11-07T06:38:40Z

@dixudx @enisoc @kow3ns this came up in our team meeting today. @lavalamp raised concerns for upgrading/downgrading the cluster. I think what he meant was that if the apiserver was downgraded to an old version, but the controller manager was not downgraded yet, was it possible that the controller manager got confused because the old apiserver didn't do cascading deletion for apps/v1/replicaset? I don't have any specific objection, but I agree that we need to think the implication through.

enisoc · 2017-11-07T17:25:26Z

if the apiserver was downgraded to an old version, but the controller manager was not downgraded yet, was it possible that the controller manager got confused because the old apiserver didn't do cascading deletion for apps/v1/replicaset?

If I'm understanding the scenario correctly, I think it should be ok. We aren't changing anything in the GC itself (the part that lives in controller manager). We're only changing the finalizers added by apiserver at the moment when a DELETE request comes in.

If a DELETE comes through apps/v1 after apiserver is upgraded, it sets the right finalizers for cascading. After that, the behavior of GC should depend only on the finalizers on the object (I don't see anywhere that GC fails to send an explicit DeletionPropagation policy), so downgrading apiserver during this process shouldn't have any effect.

If a DELETE comes through apps/v1 after apiserver is downgraded, it will fail because apps/v1 doesn't exist there. This is the same as any time we add a new version; it's not related to whether we change the default GC policy.

enisoc

The unit tests look good, but I'd like to have at least one integration test before we merge to prove this works.

Can you test that the expected finalizers get added when you delete via apps/v1beta2 vs apps/v1? To avoid racing with actual deletion, you can add your own made up finalizer that you don't remove until after you've verified the other expected values.

enisoc · 2017-11-07T20:53:24Z

pkg/registry/apps/statefulset/strategy_test.go

+			genericapirequest.RequestInfo{
+				APIGroup:   "apps",
+				APIVersion: "v1beta1",
+				Resource:   "statefulset",


nit: It doesn't affect the current code, but for the sake of realism I think the Resource names should be plural.

dixudx · 2017-11-08T07:44:53Z

Can you test that the expected finalizers get added when you delete via apps/v1beta2 vs apps/v1?

@enisoc Did find a bug. shouldDeleteDependents will not return true if we set default GC policy to DeleteDependents. Already fixed together with this PR.

janetkuo · 2017-11-20T21:17:25Z

Another comment on release note:

The default garbage collection policy for Deployment, DaemonSet, StatefulSet, and ReplicaSet has changed from OrphanDependents to DeleteDependents when the deletion is requested through an `apps/v1` endpoint.

apps/v1 is new in this release (1.9), so there's no need to mention previous behavior. Mentioning current behavior is enough.

Is OrphanDependents or DeleteDependents visible to users? Do we need to mention it at all, or should we instead say something like "dependent objects are deleted by default unless DeleteOptions is specified otherwise"?

We need to update documents (e.g. https://kubernetes.io/docs/concepts/workloads/controllers/garbage-collection/#setting-the-cascading-deletion-policy) once this is merged.

dixudx · 2017-11-21T05:07:25Z

ping @janetkuo @enisoc @caesarxuchao Updated. PTAL. Thanks.

dixudx · 2017-11-21T14:22:31Z

apps/v1 is new in this release (1.9), so there's no need to mention previous behavior. Mentioning current behavior is enough.

@janetkuo SGTM.
@enisoc Any comments?

enisoc · 2017-11-21T22:29:50Z

test/integration/replicaset/replicaset_test.go

+		t.Fatalf("Failed to verify the finalizer: %v", err)
+	}
+
+	// remove fakeFinalizer


Use updateRS just like above, so this gets retried on conflict. It's fine to do the update via extensions/v1beta1.

enisoc · 2017-11-21T22:33:43Z

test/integration/replicaset/replicaset_test.go

+			return false, err
+		}
+		t.Logf("getting the fianlizer: %s", newRS.Finalizers)
+		return newRS.DeletionTimestamp != nil && reflect.DeepEqual(newRS.Finalizers, []string{fakeFinalizer}), nil


It's better to fail if any new finalizers were added, rather than continue polling:

if newRS.DeletionTimestamp == nil { return false, nil } if got, want := newRS.Finalizers, []string{fakeFinalizer}; !reflect.DeepEqual(got, want) { return false, fmt.Errorf("Finalizers = %+v; want %+v", got, want) } return true, nil

enisoc · 2017-11-21T22:35:21Z

/assign @liggitt
for approval

…d controllers

dixudx · 2017-11-22T05:10:40Z

@enisoc @liggitt @janetkuo Updated. PTAL. Thanks

enisoc · 2017-11-22T17:53:22Z

/lgtm

liggitt · 2017-11-22T22:22:19Z

/approve

k8s-github-robot · 2017-11-22T22:22:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dixudx, enisoc, liggitt

Associated issue: 353

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~pkg/registry/OWNERS~~ [liggitt]
~~staging/src/k8s.io/apiserver/OWNERS~~ [liggitt]
~~test/OWNERS~~ [enisoc,liggitt]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

jberkus · 2017-11-22T23:57:14Z

/priority critical-urgent

/remove-priority important-soon

adjusting priorities for code freeze

k8s-github-robot · 2017-11-23T00:00:54Z

[MILESTONENOTIFIER] Milestone Pull Request Current

@dixudx @enisoc @janetkuo @liggitt @smarterclayton

Note: This pull request is marked as priority/critical-urgent, and must be updated every 1 day during code freeze.

Example update:

ACK.  In progress
ETA: DD/MM/YYYY
Risks: Complicated fix required

Pull Request Labels

sig/apps: Pull Request will be escalated to these SIGs if needed.
priority/critical-urgent: Never automatically move pull request out of a release milestone; continually escalate to contributor and SIG through all available channels.
kind/cleanup: Adding tests, refactoring, fixing old bugs.

Help

k8s-github-robot · 2017-11-23T03:49:35Z

Automatic merge from submit-queue (batch tested with PRs 52767, 55065, 55148, 56228, 56221). If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot requested review from caesarxuchao, enisoc and kow3ns November 6, 2017 09:18

k8s-github-robot assigned eparis and derekwaynecarr Nov 6, 2017

k8s-ci-robot assigned janetkuo and smarterclayton and unassigned derekwaynecarr and eparis Nov 6, 2017

enisoc reviewed Nov 6, 2017

View reviewed changes

kow3ns reviewed Nov 6, 2017

View reviewed changes

k8s-ci-robot requested a review from mml November 6, 2017 21:10

dixudx force-pushed the controller_defaultGC_DeleteDependents branch from fa8eac8 to 3ce2bac Compare November 7, 2017 05:52

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 7, 2017

dixudx force-pushed the controller_defaultGC_DeleteDependents branch from 3ce2bac to 6facef9 Compare November 7, 2017 06:06

enisoc reviewed Nov 7, 2017

View reviewed changes

dixudx force-pushed the controller_defaultGC_DeleteDependents branch from 6facef9 to f70da82 Compare November 8, 2017 07:42

dixudx force-pushed the controller_defaultGC_DeleteDependents branch from f70da82 to 178f0cd Compare November 8, 2017 08:41

k8s-github-robot added the milestone/needs-attention label Nov 21, 2017

enisoc added the status/in-progress label Nov 21, 2017

k8s-github-robot removed the milestone/needs-attention label Nov 21, 2017

dixudx force-pushed the controller_defaultGC_DeleteDependents branch 2 times, most recently from 68db654 to e389936 Compare November 21, 2017 03:34

enisoc mentioned this pull request Nov 21, 2017

[apps/v1] Change DefaultGarbageCollectionPolicy for workload controllers #55027

Closed

2 tasks

enisoc reviewed Nov 21, 2017

View reviewed changes

k8s-ci-robot assigned liggitt Nov 21, 2017

change DefaultGarbageCollectionPolicy to DeleteDependents for workloa…

344fe56

…d controllers

dixudx force-pushed the controller_defaultGC_DeleteDependents branch from e389936 to 344fe56 Compare November 22, 2017 02:10

k8s-ci-robot assigned enisoc Nov 22, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 22, 2017

liggitt mentioned this pull request Nov 22, 2017

3.9 rebase checklist openshift/origin#17428

Closed

5 tasks

janetkuo approved these changes Nov 22, 2017

View reviewed changes

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 22, 2017

k8s-ci-robot added priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. and removed priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Nov 22, 2017

k8s-github-robot merged commit 00b2d95 into kubernetes:master Nov 23, 2017

dixudx deleted the controller_defaultGC_DeleteDependents branch November 23, 2017 05:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change DefaultGarbageCollectionPolicy to DeleteDependents for workloads controllers #55148

change DefaultGarbageCollectionPolicy to DeleteDependents for workloads controllers #55148

dixudx commented Nov 6, 2017 •

edited by enisoc

Loading

dixudx commented Nov 6, 2017

enisoc left a comment

enisoc Nov 6, 2017

enisoc Nov 6, 2017

enisoc Nov 6, 2017

kow3ns left a comment

mbohlool commented Nov 6, 2017

dixudx commented Nov 7, 2017

caesarxuchao commented Nov 7, 2017

enisoc commented Nov 7, 2017

enisoc left a comment

enisoc Nov 7, 2017

dixudx commented Nov 8, 2017

janetkuo commented Nov 20, 2017

dixudx commented Nov 21, 2017

dixudx commented Nov 21, 2017

enisoc Nov 21, 2017

enisoc Nov 21, 2017

enisoc commented Nov 21, 2017

dixudx commented Nov 22, 2017

enisoc commented Nov 22, 2017

liggitt commented Nov 22, 2017

k8s-github-robot commented Nov 22, 2017

jberkus commented Nov 22, 2017

k8s-github-robot commented Nov 23, 2017

k8s-github-robot commented Nov 23, 2017

change DefaultGarbageCollectionPolicy to DeleteDependents for workloads controllers #55148

change DefaultGarbageCollectionPolicy to DeleteDependents for workloads controllers #55148

Conversation

dixudx commented Nov 6, 2017 • edited by enisoc Loading

dixudx commented Nov 6, 2017

enisoc left a comment

Choose a reason for hiding this comment

enisoc Nov 6, 2017

Choose a reason for hiding this comment

enisoc Nov 6, 2017

Choose a reason for hiding this comment

enisoc Nov 6, 2017

Choose a reason for hiding this comment

kow3ns left a comment

Choose a reason for hiding this comment

mbohlool commented Nov 6, 2017

dixudx commented Nov 7, 2017

caesarxuchao commented Nov 7, 2017

enisoc commented Nov 7, 2017

enisoc left a comment

Choose a reason for hiding this comment

enisoc Nov 7, 2017

Choose a reason for hiding this comment

dixudx commented Nov 8, 2017

janetkuo commented Nov 20, 2017

dixudx commented Nov 21, 2017

dixudx commented Nov 21, 2017

enisoc Nov 21, 2017

Choose a reason for hiding this comment

enisoc Nov 21, 2017

Choose a reason for hiding this comment

enisoc commented Nov 21, 2017

dixudx commented Nov 22, 2017

enisoc commented Nov 22, 2017

liggitt commented Nov 22, 2017

k8s-github-robot commented Nov 22, 2017

jberkus commented Nov 22, 2017

k8s-github-robot commented Nov 23, 2017

k8s-github-robot commented Nov 23, 2017

dixudx commented Nov 6, 2017 •

edited by enisoc

Loading