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

feat: added NoExecute taint to terminating Nodes #626

Closed
wants to merge 4 commits into from

Conversation

sadath-12
Copy link
Contributor

@sadath-12 sadath-12 commented Oct 21, 2023

Fixes #624 Part 4 (Terminating:NoExecute Taint)

Description

This pr attempts to solve issue of #621 as mentioned #624 . Tainting the nodes as NoExecute before attempting to terminate and also along side tries to make the taints operation code structure a bit more scalable and reusable

How was this change tested?

make presubmit

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@sadath-12 sadath-12 requested a review from a team as a code owner October 21, 2023 18:02
@sadath-12 sadath-12 requested a review from njtran October 21, 2023 18:02
Signed-off-by: sadath-12 <[email protected]>
Signed-off-by: sadath-12 <[email protected]>
@jonathan-innis
Copy link
Member

Hey @sadath-12. I'd discuss more on the issue with @njtran about this one. He is driving the design around this issue and is planning to put together an RFC around the implications of this design. This change has a number of implications, once of which is that adding a NoExecute taint necessarily means that pods that don't tolerate the taint will sit on the node up to their tolerationSeconds (default is 5m) up until they are removed. That means that nodes in general will sit on the cluster for 5m longer on average, which is untenable on its own. I think that @njtran has some thoughts around this, but it would be good to discuss this over the issue.

@sadath-12
Copy link
Contributor Author

Sure,Thanks @jonathan-innis

@njtran njtran added blocked Unable to make progress due to some dependency needs-design labels Oct 25, 2023
@njtran
Copy link
Contributor

njtran commented Oct 30, 2023

Added some information on the #629. I think this PR/Design should be explored and iterated on after that is completed, which I believe is the opposite as what you've described.

@sadath-12
Copy link
Contributor Author

Sure @njtran

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 22, 2023
@k8s-ci-robot
Copy link
Contributor

PR needs rebase.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Unable to make progress due to some dependency needs-design needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mega Issue: Node Disruption Lifecycle Taints
4 participants