-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Modify scale down set processor to add reasons to unremovable nodes #7307
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
Conversation
|
Hi @abdelrahman882. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
35f6b05 to
92f97a5
Compare
bskiba
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the change! Especially for adding unit tests where they were missing.
I've got some suggestions but the comments are all relatively minor.
cluster-autoscaler/processors/nodes/scale_down_set_processor.go
Outdated
Show resolved
Hide resolved
cluster-autoscaler/processors/nodes/scale_down_set_processor.go
Outdated
Show resolved
Hide resolved
cluster-autoscaler/processors/nodes/scale_down_set_processor_test.go
Outdated
Show resolved
Hide resolved
cluster-autoscaler/processors/nodes/scale_down_set_processor_test.go
Outdated
Show resolved
Hide resolved
cluster-autoscaler/processors/nodes/scale_down_set_processor_test.go
Outdated
Show resolved
Hide resolved
9053d15 to
35e5e2e
Compare
cluster-autoscaler/processors/nodes/scale_down_set_processor.go
Outdated
Show resolved
Hide resolved
cluster-autoscaler/processors/nodes/scale_down_set_processor.go
Outdated
Show resolved
Hide resolved
cluster-autoscaler/processors/nodes/scale_down_set_processor_test.go
Outdated
Show resolved
Hide resolved
bskiba
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
35e5e2e to
203732f
Compare
203732f to
7386a22
Compare
|
I think you can remove the isValidCandidatesFiltering function now. Otherwise |
7386a22 to
98396b4
Compare
|
/lgtm |
x13n
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, just minor comments.
/approve
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abdelrahman882, bskiba, x13n 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 |
98396b4 to
2754f1f
Compare
2754f1f to
f945fc4
Compare
|
/lgtm |
|
@abdelrahman882 this is such an amazing change, and I was about to put this out as a feature request 🙌🏻 Unfortunately I don't see the annotation part (where it is supposed to mention on the node about blockers) present in any of the releases done after this merge, is there any blocker that I can help with? @x13n @omerap12 Edit: Seems like I misunderstood the code. It seems to be only giving this info internally and not storing it in CM or annotating the node |
What type of PR is this?
/kind bug
/kind cleanup
What this PR does / why we need it:
Modify scale down set processor to add reasons to unremovable nodes so that planner can have these nodes excluded as unremovable nodes with reasons
Special notes for your reviewer:
Does this PR introduce a user-facing change?
NONE