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: PreferNoSchedule taint on candidates for disruption #629

Closed
wants to merge 5 commits into from

Conversation

sadath-12
Copy link
Contributor

Fixes #624 Part 2 (Candidate (PreferNoSchedule Taint) )

Description

This pr will add preferNoSchedule taints to nodes that are selected as candidates for disruption and any fails on that operation will remove this taint .

Also this pr is a follow up of pr #626

How was this change tested?

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 23, 2023 08:09
@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

There's some design here that is in-flux before we can continue with this work. Generally, I'd like to see the design tackle some of the high level concerns that come with this, and some considerations on how this may evolve in the future.

This is a pretty important design space concerned with schedulability and the wide range of use-cases that come with this. Some thoughts on what I'd love to see explored in the design:

  1. What use-cases are we trying to solve here?
  2. How does a PreferNoSchedule taint affect scheduling in most Karpenter clusters?
  3. When is the right time to add this Taint during disruption?

I can imagine most clusters are tightly packed due to the way the Consolidation algorithm converges to an optimal state. In that case, I don't think that a PreferNoSchedule taint would affect and solve all use-cases. As a follow-up to this taint addition, we might need to explore allowing the Effect here be configurable. It would be nice if the design mentioned this as well.

Can you add your design doc here in the PR so that we can iterate through the design? Since there's a bit of work needed on the maintainer side, I can imagine that there will be a lot of collaboration here to reach the desired state of the design doc, where we should eventually review this design at the working group.

@sadath-12
Copy link
Contributor Author

sure @njtran , ill try if I can get things done within this pr itself or if necessary will close this and raise new pr

@sadath-12
Copy link
Contributor Author

Linking the design doc here #649

@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.

@k8s-triage-robot
Copy link

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-contributor-experience at kubernetes/community.

/check-cla
/easycla

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 19, 2024
@unfor19
Copy link

unfor19 commented Feb 10, 2024

@sadath-12 @njtran

Is any help needed here to merge this? This feature is awesome

@jmdeal
Copy link
Member

jmdeal commented Apr 2, 2024

Since the design has been closed, closing out the corresponding implementation PR. Work is still being done to address this issue, I'm hoping to have a design out in the next week or two.

/close

@k8s-ci-robot
Copy link
Contributor

@jmdeal: Closed this PR.

In response to this:

Since the design has been closed, closing out the corresponding implementation PR. Work is still being done to address this issue, I'm hoping to have a design out in the next week or two.

/close

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 cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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
6 participants