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.

Sign up for GitHub

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

Jump to bottom

kubeadm: Don't drain and remove the current node on kubeadm reset #42713

Merged
merged 1 commit into from
Mar 14, 2017
Merged

kubeadm: Don't drain and remove the current node on kubeadm reset #42713

merged 1 commit into from
Mar 14, 2017
+5 −57

Conversation

luxas
Copy link
Member

@luxas luxas commented Mar 8, 2017 •
edited
Loading

What this PR does / why we need it:

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

Special notes for your reviewer:

In v1.5, kubeadm reset would drain your node and remove it from your cluster if you specified, but now in v1.6 we can't do that due to the RBAC rules we have set up.

After conversations with @liggitt, I also agree this functionality was somehow a little mis-placed (though still very convenient to use), so we're removing it for v1.6.

It's the system administrator's duty to drain and remove nodes from the cluster, not the nodes' responsibility.

The current behavior is therefore a bug that needs to be fixed in v1.6

Release note:

kubeadm: `kubeadm reset` won't drain and remove the current node anymore

@liggitt @deads2k @jbeda @dmmcquay @pires @errordeveloper

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 8, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 8, 2017
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Mar 8, 2017
@luxas luxas assigned liggitt and unassigned pires Mar 8, 2017
@luxas luxas added the kind/bug Categorizes issue or PR as related to a bug. label Mar 8, 2017
@luxas luxas added this to the v1.6 milestone Mar 8, 2017
@luxas
Copy link
Member Author

luxas commented Mar 8, 2017

Sorry, my intention was not to unassign @pires, that was a race condition in the Github page

@luxas luxas force-pushed the kubeadm_fix_reset branch from b803069 to c7fc530 Compare March 8, 2017 07:31
@luxas
Copy link
Member Author

luxas commented Mar 8, 2017

@k8s-bot unit test this

pires
pires approved these changes Mar 8, 2017
View reviewed changes
Copy link
Contributor

@pires pires left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

liggitt
liggitt reviewed Mar 8, 2017
View reviewed changes
cmd/kubeadm/app/cmd/reset.go
}, nil
}

// Run reverts any changes made to this host by "kubeadm init" or "kubeadm join".
func (r *Reset) Run(out io.Writer) error {

// Try to drain and remove the node from the cluster
err := drainAndRemoveNode(r.removeNode)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would you want to mark it unschedulable or taint it instead? seems like it should do something to signal that no more pods should be sent

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could do it, but I think the node will be marked NotReady by controller manager by default, right?
If we have to set the NodeNotReady taint I'll do it, but I'd like to change the minimum amount of things in this PR

@jbeda
Copy link
Contributor

jbeda commented Mar 13, 2017

Haven't reviewed the code but...

Ideally we would do the following:

  • Detect if the node is drained. If it is not, then error out with instructions on what to run with admin privs.
  • Provide a --force flag for reseting the node without draining.

Not sure we can bring this together in time for 1.6.

@luxas
Copy link
Member Author

luxas commented Mar 13, 2017

@jbeda I think we should do that long-term, but it won't fit into this release.
I think this is the best we can do, just revert to the earlier behavior.

@liggitt
Copy link
Member

liggitt commented Mar 14, 2017

If this was previous behavior, I'm ok with reverting to it. admins should drain their node first, but if they don't, the nodecontroller will take care of the node when it stops updating status.

@jbeda jbeda self-assigned this Mar 14, 2017
@jbeda
Copy link
Contributor

jbeda commented Mar 14, 2017

Not excited about this as it is a regression in functionality but I guess it is the best we can do on short notice.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 14, 2017
@luxas
Copy link
Member Author

luxas commented Mar 14, 2017

Not excited about this as it is a regression in functionality but I guess it is the best we can do on short notice.

Feel exactly the same but this is better than erroring in 100 % of the cases.

Root cause it (as discussed) that with RBAC a node can't drain itself.
We should document the best practices around removing a node in the docs though. @lukemarsden

@luxas
Copy link
Member Author

luxas commented Mar 14, 2017

But thanks for the LGTM!

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jbeda, luxas

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

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

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Mar 14, 2017 •
edited
Loading

@luxas: The following test(s) failed:

Test name Commit Details Rerun command
Jenkins non-CRI GCE e2e c7fc530 link @k8s-bot non-cri e2e test this

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.

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 43018, 42713)

@k8s-github-robot k8s-github-robot merged commit 5826b09 into kubernetes:master Mar 14, 2017
@bboreham bboreham mentioned this pull request Apr 26, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

@pires pires pires approved these changes

Assignees

@jbeda jbeda

@liggitt liggitt

@pires pires

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. 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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.6
Development

Successfully merging this pull request may close these issues.
7 participants
@luxas @k8s-reviewable @jbeda @liggitt @k8s-github-robot @k8s-ci-robot @pires