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

Default grace period to 0 when --force is used to delete an object #87776

Merged
merged 1 commit into from Feb 27, 2020

Conversation

@brianpursley
Copy link
Contributor

brianpursley commented Feb 3, 2020

What type of PR is this?
/kind feature

What this PR does / why we need it:
When deleting objects, kubectl currently requires you to specify --grace-period=0 when you use --force. There are two usability problems with this:

  1. This is redundant because --grace-period=0 is only allowed to be specified if you also pass --force, enforced by a validation check.

  2. If you forget to provide --grace-period=0 when using --force, you get a warning telling you that you should have used --grace-period=0 and then the delete operation still proceeds proceeds, but without force. This can be sort of frustrating since you clearly specified --force so your intention to force was already indicated, yet kubectl proceeded in a non-force way.

To fix these two problems, this PR eliminates the need to specify --grace-period=0 when using --force by defaulting grace period to 0 when force is used.

Because I discovered there are several different validation checks performed on various combinations of --force, --grace-period and --now, I also added unit test cases for each of these scenarios to help make sure the expected behavior is clear and tested for during unit testing in the future.

Which issue(s) this PR fixes:

kubernetes/kubectl#813

Special notes for your reviewer:
None

Does this PR introduce a user-facing change?:

When deleting objects using kubectl with the --force flag, you are no longer required to also specify --grace-period=0.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
None

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 3, 2020

Hi @brianpursley. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@brianpursley

This comment has been minimized.

Copy link
Contributor Author

brianpursley commented Feb 3, 2020

/assign @mengqiy

@brianpursley

This comment has been minimized.

Copy link
Contributor Author

brianpursley commented Feb 7, 2020

/cc @mengqiy
Looking for ok to test...

@k8s-ci-robot k8s-ci-robot requested a review from mengqiy Feb 7, 2020
@brianpursley brianpursley force-pushed the brianpursley:kubectl-813 branch from 454f550 to dbf2ac4 Feb 17, 2020
@brianpursley

This comment has been minimized.

Copy link
Contributor Author

brianpursley commented Feb 17, 2020

Squashed commits

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Feb 18, 2020

Copy link
Contributor

soltysh left a comment

Please ping me on slack when you get this updated (same nick 😉 )

@soltysh

This comment has been minimized.

Copy link
Contributor

soltysh commented Feb 19, 2020

/assign
/priority backlog

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 19, 2020

@soltysh: The label(s) priority/ cannot be applied, because the repository doesn't have them

In response to this:

/assign
/priority backlog

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.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 20, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brianpursley, 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

@brianpursley brianpursley force-pushed the brianpursley:kubectl-813 branch from af6e040 to 16c4052 Feb 20, 2020
@brianpursley

This comment has been minimized.

Copy link
Contributor Author

brianpursley commented Feb 21, 2020

@soltysh ptal, needs lgtm for tide

Copy link
Contributor

soltysh left a comment

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 21, 2020
@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Feb 22, 2020

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

3 similar comments
@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Feb 22, 2020

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Feb 22, 2020

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Feb 22, 2020

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@brianpursley

This comment has been minimized.

Copy link
Contributor Author

brianpursley commented Feb 22, 2020

/hold

OK it looks like there is some kind of problem blocking the rebuild from succeeding:

ERROR(all) vendor/k8s.io/kubectl/pkg/cmd/delete/delete_test.go:38:2: cmdutil redeclared in this block
ERROR(all) vendor/k8s.io/kubectl/pkg/cmd/delete/delete_test.go:23:2: 	other declaration of cmdutil
exit status 1
!!! Type Check has failed. This may cause cross platform build failures.
!!! Please see https://git.k8s.io/kubernetes/test/typecheck for more information.

I'll investigate later today or tomorrow.

@brianpursley brianpursley force-pushed the brianpursley:kubectl-813 branch from 16c4052 to 8777951 Feb 24, 2020
@k8s-ci-robot k8s-ci-robot removed the lgtm label Feb 24, 2020
@brianpursley

This comment has been minimized.

Copy link
Contributor Author

brianpursley commented Feb 24, 2020

Tried to reproduce locally by running hack/verify-typecheck-providerless.sh but it succeeded.
I rebased and will see if the test passes on the build server now.

@brianpursley

This comment has been minimized.

Copy link
Contributor Author

brianpursley commented Feb 24, 2020

/unhold

@brianpursley brianpursley force-pushed the brianpursley:kubectl-813 branch from 8777951 to 0f31bef Feb 24, 2020
@brianpursley

This comment has been minimized.

Copy link
Contributor Author

brianpursley commented Feb 24, 2020

/retest

@brianpursley

This comment has been minimized.

Copy link
Contributor Author

brianpursley commented Feb 25, 2020

/cc @soltysh

@k8s-ci-robot k8s-ci-robot requested a review from soltysh Feb 25, 2020
Copy link
Contributor

soltysh left a comment

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 27, 2020
@k8s-ci-robot k8s-ci-robot merged commit 882b6f8 into kubernetes:master Feb 27, 2020
16 checks passed
16 checks passed
cla/linuxfoundation brianpursley authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-dependencies Job succeeded.
Details
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-kind Job succeeded.
Details
pull-kubernetes-e2e-kind-ipv6 Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Feb 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.