Use DeleteOptions.PropagationPolicy instead of OrphanDependents (deprecated) in kubectl #59851

nilebox · 2018-02-14T05:00:03Z

What this PR does / why we need it:
Replaces the deprecated DeleteOptions.OrphanDependents deletion option with DeleteOptions.PropagationPolicy. It will improve the cascade deletion by using Foreground GC deletion instead of the old OrphanDependents: false which has a confusing behavior (leaves orphans if children has finalizers set).

Which issue(s) this PR fixes:
Fixes #59850

Special notes for your reviewer:

Release note:

Use DeleteOptions.PropagationPolicy instead of OrphanDependents in kubectl

nilebox · 2018-02-14T10:17:21Z

/assign @liggitt

liggitt · 2018-02-14T13:25:26Z

pkg/kubectl/cmd/delete.go

-	falseVar := false
-	deleteOptions := &metav1.DeleteOptions{OrphanDependents: &falseVar}
+	policy := metav1.DeletePropagationForeground
+	deleteOptions := &metav1.DeleteOptions{PropagationPolicy: &policy}


Why not background deletion?

@liggitt as I noted in #59850, I would like blockOwnerDeletion: true flag to be respected by default deletion with kubectl delete command. As far as I understand, with background deletion the owner will be deleted immediately regardless of whether there are dependents with blockOwnerDeletion: true flag or not.
Also, I think that with kubectl --cascade=true seeing the owner gone only after all dependents deleted is a more straightforward UX.

liggitt · 2018-02-14T13:25:56Z

pkg/kubectl/delete.go

-	falseVar := false
-	deleteOptions := &metav1.DeleteOptions{OrphanDependents: &falseVar}
+	policy := metav1.DeletePropagationForeground
+	deleteOptions := &metav1.DeleteOptions{PropagationPolicy: &policy}


Same question, why not background?

The reasoning is the same as for generic deletion. Is there a specific reason for deleting ReplicationController immediately?

At this point, the reaper has called scaler.Scale to stop all the dependent pods, so you can set the policy to "background" to delete the owner immediately. Moving away from OrphanDependents is good, as the field is marked deprecated.

Or, you can remove the reapers, and rely on the garbage collector (GC) to do the foreground deletion. The behavior will be different in that:

the GC deletion waits for all the dependent objects to be deleted before deleting the replicaset, not limiting to pods;

the GC deletion waits for the dependents to be deleted from etcd; while the scalers only wait for the pods to be in the NotReady state. GC's behavior is better, because for objects like pods, only the deletion from etcd indicates the occupied resources are released.

GC cascadingly deletes dependents' dependents; reapers don't.

Moving away from reapers requires more discussion in the sig-cli, and can be done in another PR.

Thanks for the context, will change it to "background" there.

ash2k · 2018-02-14T23:15:30Z

pkg/kubectl/cmd/delete_test.go

-		return body == nil && expectedOrphanDependents == nil
+func hasExpectedPropagationPolicy(body io.ReadCloser, policy *metav1.DeletionPropagation) bool {
+	if body == nil || policy == nil {
+		return body == nil && policy == nil
 	}
 	var parsedBody metav1.DeleteOptions
 	rawBody, _ := ioutil.ReadAll(body)
 	json.Unmarshal(rawBody, &parsedBody)


No error handling on these two lines. Maybe fix it as a separate commit while at it?

Or maybe its ok...

I think it's fine - it's part of a unit test which will break if anything is wrong with the body.

nilebox · 2018-02-21T00:44:36Z

/assign @caesarxuchao

caesarxuchao · 2018-02-21T07:08:22Z

/ok-to-test

deads2k · 2018-05-17T17:44:50Z

/retest

smarterclayton · 2018-05-17T18:15:57Z

/hold

Just looking at this, it looks like this makes kubectl delete for the majority of resources change from "completes and the object is deleted" to "you will now race with the GC". That means kubectl delete && kubectl create no longer works correctly for anything that isn't 'pod' or 'namespace'. Is that correct?

I'm in favor of this change, but to do it we need to make kubectl delete not break, which implies wait as discussed above. wait should be the default - if we need to gather consensus to also make pod and namespace wait I'd be happy to arrange for that.

nilebox · 2018-05-17T21:45:38Z

/test pull-kubernetes-e2e-gce
/test pull-kubernetes-unit

nilebox · 2018-05-17T21:49:22Z

@smarterclayton ok, I'll change default for -wait to be set by default in #63695

soltysh · 2018-05-18T10:44:51Z

@smarterclayton I agree with your approach to --wait but I don't want to do it here since first I want to see this code cleaned up. This includes this PR and #63979. Only then we can add nice and clean --wait functionality, which will happen in #63695 like Nail mentioned earlier. If we include it here, the code will be a mess, and I don't want to see that. I have a list of items that are required for 1.11 and --wait is already there.

soltysh · 2018-05-18T20:53:48Z

Let's wait for #64034 and then you'll have to rebase your PR on top of that. We'll get nice generic --wait mechanism which will help me with #63979.

…ecated) in kubectl

nilebox · 2018-05-23T07:02:55Z

@soltysh #64034 is merged, I rebased this PR. should not be marked with do-not-merge/hold anymore I believe? Also need to re-add lgtm after rebase.

nilebox · 2018-05-23T10:14:17Z

/retest

soltysh

/lgtm
/hold cancel

k8s-ci-robot · 2018-05-23T11:23:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nilebox, seans3, soltysh

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~hack/OWNERS~~ [soltysh]
~~pkg/kubectl/OWNERS~~ [seans3,soltysh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

nilebox · 2018-05-23T12:57:47Z

/retest

k8s-ci-robot · 2018-05-23T15:16:06Z

@nilebox: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-unit	`b92940f`	link	`/test pull-kubernetes-unit`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

soltysh · 2018-05-23T15:27:22Z

/retest

k8s-github-robot · 2018-05-23T15:53:22Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 14, 2018

k8s-ci-robot requested review from ericchiang and soltysh February 14, 2018 05:00

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 14, 2018

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

nilebox force-pushed the kubectl-foreground-deletion branch from 1f41571 to 18bd17b Compare February 14, 2018 05:05

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

nilebox force-pushed the kubectl-foreground-deletion branch 2 times, most recently from 03cedb4 to 84b24b9 Compare February 14, 2018 05:25

k8s-ci-robot assigned liggitt Feb 14, 2018

liggitt reviewed Feb 14, 2018

View reviewed changes

ash2k reviewed Feb 14, 2018

View reviewed changes

k8s-ci-robot assigned caesarxuchao Feb 21, 2018

nilebox force-pushed the kubectl-foreground-deletion branch from 33e74b9 to be952d6 Compare February 22, 2018 03:33

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

nilebox force-pushed the kubectl-foreground-deletion branch 2 times, most recently from f5fc75e to eb03996 Compare February 22, 2018 04:36

k8s-ci-robot assigned soltysh May 17, 2018

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels May 17, 2018

soltysh mentioned this pull request May 17, 2018

Drop reapers #63979

Merged

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 17, 2018

Use DeleteOptions.PropagationPolicy instead of OrphanDependents (depr…

3b5afd8

…ecated) in kubectl

nilebox force-pushed the kubectl-foreground-deletion branch from 44b3ed9 to 3b5afd8 Compare May 23, 2018 07:01

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 23, 2018

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels May 23, 2018

soltysh approved these changes May 23, 2018

View reviewed changes

k8s-github-robot merged commit 8f4674d into kubernetes:master May 23, 2018

nilebox mentioned this pull request May 23, 2018

Add imperative deprovision/unbind flag to ServiceInstance/ServiceBinding kubernetes-retired/service-catalog#1942

Closed

nilebox deleted the kubectl-foreground-deletion branch May 27, 2018 13:16

nilebox mentioned this pull request Jun 2, 2018

kubectl delete should wait for resource to be deleted before returning #42594

Closed

4 tasks

yasongxu mentioned this pull request Apr 16, 2020

delete policy not same as "kubectl delete" kubernetes/dashboard#5074

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use DeleteOptions.PropagationPolicy instead of OrphanDependents (deprecated) in kubectl #59851

Use DeleteOptions.PropagationPolicy instead of OrphanDependents (deprecated) in kubectl #59851

nilebox commented Feb 14, 2018 •

edited by soltysh

Loading

nilebox commented Feb 14, 2018

liggitt Feb 14, 2018

nilebox Feb 14, 2018 •

edited

Loading

liggitt Feb 14, 2018

nilebox Feb 14, 2018

caesarxuchao Feb 21, 2018

nilebox Feb 21, 2018

ash2k Feb 14, 2018

ash2k Feb 14, 2018

nilebox Feb 14, 2018

nilebox commented Feb 21, 2018

caesarxuchao commented Feb 21, 2018

deads2k commented May 17, 2018

smarterclayton commented May 17, 2018

nilebox commented May 17, 2018

nilebox commented May 17, 2018

soltysh commented May 18, 2018

soltysh commented May 18, 2018

nilebox commented May 23, 2018

nilebox commented May 23, 2018

soltysh left a comment

k8s-ci-robot commented May 23, 2018

nilebox commented May 23, 2018

k8s-ci-robot commented May 23, 2018 •

edited

Loading

soltysh commented May 23, 2018

k8s-github-robot commented May 23, 2018

Use DeleteOptions.PropagationPolicy instead of OrphanDependents (deprecated) in kubectl #59851

Use DeleteOptions.PropagationPolicy instead of OrphanDependents (deprecated) in kubectl #59851

Conversation

nilebox commented Feb 14, 2018 • edited by soltysh Loading

nilebox commented Feb 14, 2018

Choose a reason for hiding this comment

nilebox Feb 14, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilebox commented Feb 21, 2018

caesarxuchao commented Feb 21, 2018

deads2k commented May 17, 2018

smarterclayton commented May 17, 2018

nilebox commented May 17, 2018

nilebox commented May 17, 2018

soltysh commented May 18, 2018

soltysh commented May 18, 2018

nilebox commented May 23, 2018

nilebox commented May 23, 2018

soltysh left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 23, 2018

nilebox commented May 23, 2018

k8s-ci-robot commented May 23, 2018 • edited Loading

soltysh commented May 23, 2018

k8s-github-robot commented May 23, 2018

nilebox commented Feb 14, 2018 •

edited by soltysh

Loading

nilebox Feb 14, 2018 •

edited

Loading

k8s-ci-robot commented May 23, 2018 •

edited

Loading