-
Notifications
You must be signed in to change notification settings - Fork 759
feat: Add per-plugin nodeSelector support #1766
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
base: master
Are you sure you want to change the base?
feat: Add per-plugin nodeSelector support #1766
Conversation
|
Welcome @SarCode! |
|
Hi @SarCode. Thanks for your PR. I'm waiting for a github.com 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Adds nodeSelector field to HighNodeUtilizationArgs and LowNodeUtilizationArgs to allow filtering nodes per plugin. This enables different plugins in the same profile to target different sets of nodes based on labels, solving the limitation where only global nodeSelector was available. Use case: Run HighNodeUtilization only on nodes blocked by cluster-autoscaler while other plugins run on all nodes. Changes: - Add NodeSelector field to both plugin Args structs - Implement node filtering logic in Balance() methods - Add nodeMatchesSelector helper function - Generate deepcopy code for new fields - Add comprehensive tests for nodeSelector logic Addresses: kubernetes-sigs#1486
65eef0b to
5fb5911
Compare
|
/ok-to-test |
|
@SarCode: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
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. |
|
Apologies if I am being too eager but is there a way I could get a review on this, it's my first open source contribution so really excited for this 😁 @googs1025 or @ingvagabund maybe? |
|
/ok-to-test |
|
/retest |
|
/retest |
|
Formatting issues fixed 😃 |
|
@googs1025, the tests went through 👌 |
|
@SarCode thank you for your first contribution here and welcome to the open source community. The current approach is less practical since only two plugin will benefit from it. Also, each plugin will need to specify the node selector which is more suitable for a profile level field. Saying that I recommend to take the more generic approach and define a node selector field on a profile level. |
|
Thank you for the kind words @ingvagabund ! I was thinking of the same thing, but I thought it would be easier if I contained it to just Utilization use-case, I will make some changes and make it more generic 👌 |
|
What if we add optional profile-level nodeSelector while keeping existing plugin-level support: Will look something like this |
|
NodeSelector needs to be a string to keep with the current globally scoped node selector. |
Would there be any use for the plugin-level node selector? |
|
Thank you for your contribution since I barely have time to fix it... I will review it recently as soon as possible, too. |
Agree. Just profile-level node selector seems to be ok. Because from my perspective, if someone wants to configure LowNodeUtilization with "no" nodeSelector while configure another plugin with "some" nodeSelector, it's better devided into 2 seperate profiles. |
|
If I understand this correctly, the descheduler should run with separate profiles if they want different nodeSelector? Does descheduler supports this right now? cause if I remember correctly nodeSelector is global for all profiles |
|
Looking at the docs it seems like profile right now does have a nodeSelector config
|
Yes, your example is just what I want to implement and proposed inside #1486 . |
If I understand it right, this is a plugin-level nodeSelector only for DefaultEvictor. |
I tried the two profile config with different nodeSelector and it worked The logs confirmed: Profile 1 correctly processed only worker nodes for HighNodeUtilization |
@SarCode Thanks for your verification! But just as I described, this use-case is trying to configure DefaultEvictor in 2 profiles. The nodeSelector is DefaultEvictor's argument. It's a tricky way to meet the initial requirement because now only DefaultEvictor implements EvictorPlugin interface. If someday we have a "FooEvictor" that implements EvictorPlugin, then the "FooEvictor" must supports nodeSelector as well. So what I hope to do is add nodeSelector in DeschedulerProfile maybe like this: This can prevent that every plugin need to support nodeSelector one by one. |
|
My intention behind this all is to run HighNodeUtilization only on nodes which can't be removed by cluster-autoscaler maybe due to local-storage or whatever. Meanwhile all other plugins can run on all nodes. |
|
NodeSelector on the profile level can skip many nodes. Whereas, if it is used through the DefaultEvictor all nodes still gets processed. Which will not be that effective in big clusters. |
Adds nodeSelector field to HighNodeUtilizationArgs and LowNodeUtilizationArgs to allow filtering nodes per plugin.
This enables different plugins in the same profile to target different sets of nodes based on labels, solving the limitation where only global nodeSelector was available.
Use case: Run HighNodeUtilization only on nodes blocked by cluster-autoscaler while other plugins run on all nodes.
Changes:
Addresses: #1486
Description
Checklist
Please ensure your pull request meets the following criteria before submitting
for review, these items will be used by reviewers to assess the quality and
completeness of your changes: