- Notifications
You must be signed in to change notification settings - Fork907
chore: set default requests/limits in helm chart#16844
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
This will cause unexpected changes to existing deployments.
Currently if you set the following:
resources: requests: memory: 128Mi
you get the following result:
resources: requests: memory: 128Mi
After this change, the limits you set are merged with the default limits:
resources: limits: cpu: 2000m memory: 4096Mi requests: cpu: 2000m memory: 128Mi
In order to avoid these 'defaults' from creeping in unexpectedly, you need to do something like:
resources: requests: cpu: null memory: 128Mi limits: null
@johnstcn what do you recommend setting in this case? if we set |
We need to conditionally set these default values if the deployment administrator does not specify values for resource requests/limits. This produces the following test cases at least:
|
thanks@johnstcn - have a look, i've added the conditional logic. |
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.
We don't have any test cases that override requests/limits currently. We should add them as part of this change.
github-actionsbot commentedMar 17, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
All contributors have signed the CLA ✍️ ✅ |
@johnstcn FYI - the most recent commit used ClaudeCode to create the test cases. |
This commit adds two test cases to the Helm chart tests:1. custom_resources - Tests overriding both resource limits and requests2. partial_resources - Tests specifying only resource requests without limits🤖 Generated with [Claude Code](https://claude.ai/code)Co-Authored-By: Claude <noreply@anthropic.com>
cf981ed
toc3327d3
Compare@johnstcn any idea why |
The template defined in It may be possible to scope this only to the |
559bd10
to2376d54
Comparesorted. |
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.
Approving; I don't have any further blocking comments.
I do note that the 'default' resources for a provisioner are a bit high, but they match what we specify in the chart defaults.
I think this is worth calling out in the release notes though.
cbc699b
intomainUh oh!
There was an error while loading.Please reload this page.
closes#16825 - my first commit from across the pond 😄