-
Notifications
You must be signed in to change notification settings - Fork 24
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(helm): Introduce priorityClassName system-node-critical #220
Conversation
…as an optional boolean value Signed-off-by: Florian Fürstenberg <[email protected]>
50a3f22
to
e678245
Compare
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.
Thanks for the patch @ffuerste. This totally makes sense.
I'm just wondering should be the desired default be true
, for at least some of the plugins 🧐 If a resource policy plugin (like balloons or topology-aware) is deployed I think it is kinda critical and should not be evicted (thus default to true
). WDYT? @klihub @fmuyassarov @askervin @kad
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.
I think this is a very nice improvement.
I'm fine both merging this PR as it is, or with some/all existing plugins set as system-node-critical
by default.
Maybe I'd even prefer taking this PR is as it is, as it only enables configuring the priorityClassName but does not change the default behavior. And then have a separate commit that changes the default behavior for chosen plugins.
I don't have a strong opinion about that to be frank. I'm fine with the current state as well. |
I think it makes sense. |
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.
I'm fine with the default either way for the current plugins. |
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.
Hold until comments are resolved
…ass for node-critical pods Signed-off-by: Florian Fürstenberg <[email protected]>
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.
Thanks @ffuerste. LGTM on my part
What does this PR do?
Introducing in the helm charts of all NRI plugins an optional boolean field for setting
priorityClassName: system-node-critical
.As suggest here the
priorityClassName
is introduced as an optional helm flag analog topatchRuntimeConfig
. However, in this way the feature can only be enabled with no option to define a custompriorityClassName
.Related issues
Resolves #194