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

Implement cordon/uncordon for Node #762

Merged
merged 4 commits into from
Dec 22, 2021
Merged

Conversation

ChinYing-Li
Copy link
Contributor

Signed-off-by: ChinYing-Li chinying.li@mail.utoronto.ca

Motivation

This PR aims to solve #706.

Solution

Currently only cordon/uncordon is implemented, as I would like to have some feedback before moving on to drain etc.
One "test" is added as example/node_cordon.rs, but I wonder if it makes sense to have a conditional compilation feature test-integration.

Also, I suppose this change needs an entry in the CHANGELOG, but please let me know in case I am wrong.
Any suggestion is appreciated!

Signed-off-by: ChinYing-Li <chinying.li@mail.utoronto.ca>
@codecov-commenter

This comment has been minimized.

@clux clux linked an issue Dec 20, 2021 that may be closed by this pull request
@clux clux added the changelog-add changelog added category for prs label Dec 20, 2021
Copy link
Member

@clux clux left a comment

Choose a reason for hiding this comment

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

initial comments. looks good, just minor points here and there.

examples/node_cordon.rs Outdated Show resolved Hide resolved
examples/node_cordon.rs Outdated Show resolved Hide resolved
examples/node_cordon.rs Outdated Show resolved Hide resolved
examples/node_cordon.rs Outdated Show resolved Hide resolved
kube-core/src/util.rs Outdated Show resolved Hide resolved
kube-core/src/util.rs Outdated Show resolved Hide resolved
@clux
Copy link
Member

clux commented Dec 20, 2021

Currently only cordon/uncordon is implemented, as I would like to have some feedback before moving on to drain etc. One "test" is added as example/node_cordon.rs, but I wonder if it makes sense to have a conditional compilation feature test-integration.

I think having the bulk of node_cordon be an integration test inside kube-client instead makes sense here because it is so light weight.

If that works out we can maybe follow a similar approach for drain, but it might be a bit different because I don't see how we can schedule pods on a fake node to verify eviction 🤔

Also, I suppose this change needs an entry in the CHANGELOG, but please let me know in case I am wrong. Any suggestion is appreciated!

Don't worry about it! We are going to try a system with auto-generated changelogs from labels and pr title for next release.

Thanks for looking these help-wanted issues! Very much appreciated.

Signed-off-by: ChinYing-Li <chinying.li@mail.utoronto.ca>
@ChinYing-Li
Copy link
Contributor Author

Haven't push the "code formatting" commit yet; will do that once I wrapped up my attempt to implement drain. Thanks!

@ChinYing-Li
Copy link
Contributor Author

The draft of node drain example is here, though I still haven't figured out how to make the created "fake" node become Ready. Testing with kubectl, the created fake node remains NotReady.

Signed-off-by: ChinYing-Li <chinying.li@mail.utoronto.ca>
@ChinYing-Li
Copy link
Contributor Author

I think it's best to open a separate PR for drain, which is a more complicated utility function, so this PR is ready for review! Thanks.

@clux
Copy link
Member

clux commented Dec 22, 2021

Ok, will have a go over. I think it's possible that drain might be best served as an example, because it is non-trivial to startup a real node, get pods on it, then evict those, and drain it. If we can verify it as a one-off thing in an example, that might be good enough.

Copy link
Member

@clux clux left a comment

Choose a reason for hiding this comment

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

minor nit on a comment, but otherwise happy with this 👍

kube-client/src/api/util.rs Outdated Show resolved Hide resolved
Co-authored-by: Eirik A <sszynrae@gmail.com>
Signed-off-by: Chin-Ying Li <chinying.li@mail.utoronto.ca>
@clux
Copy link
Member

clux commented Dec 22, 2021

Looking good. I'll merge this in. I'll leave the original issue open (since we still want something for drain), but will add a comment therein.

@clux clux merged commit 52f69b9 into kube-rs:master Dec 22, 2021
@clux clux added this to the 0.66.0 milestone Jan 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-add changelog added category for prs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants