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

kubeadm: implement deletion of multiple tokens #75646

Merged

Conversation

@bart0sh
Copy link
Contributor

commented Mar 24, 2019

What type of PR is this?

/kind feature

What this PR does / why we need it:

Currently kubeadm doesn't allow to delete more than one bootstrap token at once. This PR implements this feature.

Does this PR introduce a user-facing change?:

kubeadm: implement deletion of multiple bootstrap tokens at once
@bart0sh

This comment has been minimized.

Copy link
Contributor Author

commented Mar 24, 2019

@k8s-ci-robot k8s-ci-robot requested a review from neolit123 Mar 24, 2019

@bart0sh

This comment has been minimized.

Copy link
Contributor Author

commented Mar 24, 2019

/test pull-kubernetes-integration

@yagonobre
Copy link
Member

left a comment

Thanks @bart0sh
/priority important-longterm

Show resolved Hide resolved cmd/kubeadm/app/cmd/token.go Outdated
Show resolved Hide resolved cmd/kubeadm/app/cmd/token.go Outdated
Show resolved Hide resolved cmd/kubeadm/app/cmd/token.go Outdated
@neolit123
Copy link
Member

left a comment

@kubernetes/sig-cluster-lifecycle-pr-reviews

@bart0sh thanks for the PR. please ping the above team ^ on kubeadm PRs too.

@rosti
Copy link
Member

left a comment

Thanks for the contribution @bart0sh !
I am, however, a bit reserved about this. Having multiple active tokens is not a very common case and deleting more than one at a time is even less common operation.
What if deleting one of the tokens fails for some reason? Do we delete all tokens that we can, do we stop now (potentially deleting only some) or we don't delete anything (if possible)?
Having the user decide on this is way better than us imposing a policy on it.

@bart0sh @fabriziopandini @timothysc @neolit123 WDYT?

Show resolved Hide resolved cmd/kubeadm/app/cmd/token.go Outdated
Show resolved Hide resolved cmd/kubeadm/app/cmd/token.go Outdated
Show resolved Hide resolved cmd/kubeadm/app/cmd/token.go Outdated

@bart0sh bart0sh force-pushed the bart0sh:PR0066-kubeadm-token-delete-multiple branch from 9ba2426 to 3707995 Mar 25, 2019

@bart0sh

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

@rosti thank you for the review. Having a possibility to delete more than one token at a time doesn't make it mandatory. People can continue deleting one token if they wish so. Why not to give them such a possibility?

P.S. I've updated the PR addressing the issues you and @yagonobre pointed to. Please review again.

@dixudx
Copy link
Member

left a comment

Others lgtm.

@@ -158,11 +158,11 @@ func NewCmdToken(out io.Writer, errW io.Writer) *cobra.Command {
tokenCmd.AddCommand(listCmd)

deleteCmd := &cobra.Command{
Use: "delete [token-value]",
Use: "delete token-value ...",

This comment has been minimized.

Copy link
@dixudx

dixudx Mar 26, 2019

Member

I'd prefer to the old way. Better not changing the style we're using in current kube command lines.

This comment has been minimized.

Copy link
@dixudx

dixudx Mar 26, 2019

Member

And this usage is misleading. Users may think token-value is another new subcommand. But actually it is not.

This comment has been minimized.

Copy link
@bart0sh

bart0sh Mar 26, 2019

Author Contributor

Well, I'd say that current syntax is misleading. It suggests that token-value is optional as it's in square brackets.

Anyway, how do you want it to be? delete [token-value] ... ?

This comment has been minimized.

Copy link
@rosti

rosti Mar 26, 2019

Member

+1 for @dixudx 's comment. For better or worse, that's our current way of doing things in help. So we have to stick to it for now and, possibly, change things in another PR.

This comment has been minimized.

Copy link
@bart0sh

bart0sh Mar 26, 2019

Author Contributor

ok, makes sense to me. will chang it to delete [token-value] ...

@rosti

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

@bart0sh I am not all negative about this feature. Sorry if my comment looked like that.
We just need to choose a policy of "What happens when one token cannot be deleted?". One such policy is to delete everything possible (rather than stop with an error at the first failure).

@bart0sh

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

@rosti you didn't sound negative. Sorry if my answer made you to think so.
Current code exits on any errors, so if even one token can't be deleted kubeadm throws an error and exists. I can change it if reviewers prefer "try to delete all" approach.

@bart0sh bart0sh force-pushed the bart0sh:PR0066-kubeadm-token-delete-multiple branch from 3707995 to 63c090b Mar 26, 2019

@bart0sh

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

@kubernetes/sig-cluster-lifecycle-pr-reviews updated, please review again. Thanks.

Show resolved Hide resolved cmd/kubeadm/app/cmd/token.go Outdated
@neolit123

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

@rosti @bart0sh

We just need to choose a policy of "What happens when one token cannot be deleted?"

seems to me that we are stopping on the first encountered error with the current code.
i'm OK either way.

@bart0sh bart0sh force-pushed the bart0sh:PR0066-kubeadm-token-delete-multiple branch from 63c090b to 405a971 Mar 27, 2019

@bart0sh

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2019

/test pull-kubernetes-integration

1 similar comment
@bart0sh

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2019

/test pull-kubernetes-integration

@bart0sh

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

@neolit123 @rosti any more objections? if not, please lgtm&approve.

@neolit123
Copy link
Member

left a comment

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bart0sh, neolit123

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

@rosti

rosti approved these changes Mar 29, 2019

Copy link
Member

left a comment

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 29, 2019

@k8s-ci-robot k8s-ci-robot merged commit e6d2742 into kubernetes:master Mar 29, 2019

17 checks passed

cla/linuxfoundation bart0sh authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
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-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
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.