Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from May 23, 2018

Conversation

@nilebox
Copy link
Member

nilebox commented Feb 14, 2018

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 

@k8s-ci-robot k8s-ci-robot requested review from ericchiang and soltysh Feb 14, 2018

@nilebox nilebox force-pushed the nilebox:kubectl-foreground-deletion branch from 1f41571 to 18bd17b Feb 14, 2018

@nilebox nilebox force-pushed the nilebox:kubectl-foreground-deletion branch 2 times, most recently from 03cedb4 to 84b24b9 Feb 14, 2018

@nilebox

This comment has been minimized.

Copy link
Member Author

nilebox commented Feb 14, 2018

/assign @liggitt

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

This comment has been minimized.

@liggitt

liggitt Feb 14, 2018

Member

Why not background deletion?

This comment has been minimized.

@nilebox

nilebox Feb 14, 2018

Author Member

@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.

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

This comment has been minimized.

@liggitt

liggitt Feb 14, 2018

Member

Same question, why not background?

This comment has been minimized.

@nilebox

nilebox Feb 14, 2018

Author Member

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

This comment has been minimized.

@caesarxuchao

caesarxuchao Feb 21, 2018

Member

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:

  1. the GC deletion waits for all the dependent objects to be deleted before deleting the replicaset, not limiting to pods;
  2. 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.
  3. 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.

This comment has been minimized.

@nilebox

nilebox Feb 21, 2018

Author Member

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

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)

This comment has been minimized.

@ash2k

ash2k Feb 14, 2018

Member

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

This comment has been minimized.

@ash2k

ash2k Feb 14, 2018

Member

Or maybe its ok...

This comment has been minimized.

@nilebox

nilebox Feb 14, 2018

Author Member

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

@nilebox

This comment has been minimized.

Copy link
Member Author

nilebox commented Feb 21, 2018

/assign @caesarxuchao

@caesarxuchao

This comment has been minimized.

Copy link
Member

caesarxuchao commented Feb 21, 2018

/ok-to-test

@nilebox nilebox force-pushed the nilebox:kubectl-foreground-deletion branch from 33e74b9 to be952d6 Feb 22, 2018

@nilebox nilebox force-pushed the nilebox:kubectl-foreground-deletion branch 2 times, most recently from f5fc75e to eb03996 Feb 22, 2018

@k8s-ci-robot k8s-ci-robot added size/M size/L and removed size/L size/M labels Feb 22, 2018

@deads2k

This comment has been minimized.

Copy link
Contributor

deads2k commented May 17, 2018

/retest

@smarterclayton

This comment has been minimized.

Copy link
Contributor

smarterclayton commented May 17, 2018

/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

This comment has been minimized.

Copy link
Member Author

nilebox commented May 17, 2018

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

@nilebox

This comment has been minimized.

Copy link
Member Author

nilebox commented May 17, 2018

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

@soltysh

This comment has been minimized.

Copy link
Contributor

soltysh commented May 18, 2018

@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

This comment has been minimized.

Copy link
Contributor

soltysh commented May 18, 2018

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.

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

@k8s-ci-robot k8s-ci-robot removed the lgtm label May 23, 2018

@nilebox

This comment has been minimized.

Copy link
Member Author

nilebox commented May 23, 2018

@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

This comment has been minimized.

Copy link
Member Author

nilebox commented May 23, 2018

/retest

@k8s-ci-robot k8s-ci-robot added lgtm and removed do-not-merge/hold labels May 23, 2018

@soltysh
Copy link
Contributor

soltysh left a comment

/lgtm
/hold cancel

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented May 23, 2018

[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:

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

@nilebox

This comment has been minimized.

Copy link
Member Author

nilebox commented May 23, 2018

/retest

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented May 23, 2018

@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

This comment has been minimized.

Copy link
Contributor

soltysh commented May 23, 2018

/retest

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented May 23, 2018

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

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

16 of 18 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-integration
Details
pull-kubernetes-kubemark-e2e-gce-big Job triggered.
Details
cla/linuxfoundation nilebox authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.