-
Notifications
You must be signed in to change notification settings - Fork 899
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
fix(chart): explicitly set resourceFieldRef.divisor to avoid flapping #4538
Conversation
@@ -69,11 +69,13 @@ spec: | |||
resourceFieldRef: | |||
containerName: {{ .Chart.Name }}-init | |||
resource: limits.cpu | |||
divisor: "0" |
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.
Shouldn't we set to its default, which is documented as "1" here?
The divisor field is optional and has the default value of 1. A divisor of 1 means cores for cpu resources, or bytes for memory resources.
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.
Yeah, that was my understanding too, I found a lot of examples setting it to "0", but couldn't really get why's that. I'll dig into it a bit more.
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.
The current chart results in a default of "0", because the field is a quantity and is not a pointer, but it's not defaulted per se, so they end up handling it as "1" in code: https://github.com/kubernetes/kubernetes/blob/34b85c593d13168edee426c63e97dbfbc760395b/pkg/api/v1/resource/helpers.go#L345C1-L347C10
I'd say that although we are currently implicitly setting it to 0, we can set it to 1 to be more explicit.
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.
Updated accordingly
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 @phisco 🙌
Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
Successfully created backport PR for |
Successfully created backport PR for |
Successfully created backport PR for |
Description of your changes
Fixes #4509.
I have:
Added or updated unit and E2E tests for my change.Runmake reviewable
to ensure this PR is ready for review.backport release-x.y
labels to auto-backport this PR, if necessary.Opened a PR updating the docs, if necessary.