NotificationsYou must be signed in to change notification settings
Fork911
Star10k

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

Jump to bottom

Merged

ericpaulsen merged 5 commits intomainfromdefault-reqs-limits

Apr 22, 2025

Merged

chore: set default requests/limits in helm chart#16844

ericpaulsen merged 5 commits intomainfromdefault-reqs-limits

Apr 22, 2025

Conversation

Copy link

Member

ericpaulsen commentedMar 7, 2025

closes#16825 - my first commit from across the pond 😄

helm: set default requests/limits

dfbddd6

ericpaulsen added the helmArea: helm chart label

Mar 7, 2025

ericpaulsen requested review fromjohnstcn,spikecurtis andbpmct

March 7, 2025 13:58

ericpaulsen self-assigned this

Mar 7, 2025

ericpaulsen changed the title~~helm: set default requests/limits~~chore: set default requests/limits in helm chart

Mar 7, 2025

johnstcn reviewed

Mar 7, 2025

View reviewed changes

Copy link

Member

johnstcn left a 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

Copy link

MemberAuthor

ericpaulsen commentedMar 10, 2025

@johnstcn what do you recommend setting in this case? if we setnull in the values file, then it seems we are rendering this PR moot.

Copy link

Member

johnstcn commentedMar 10, 2025

@johnstcn what do you recommend setting in this case? if we setnull in the values file, then it seems we are rendering this PR moot.

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:

If I define no default requests/limits in myvalues.yaml, I get default requests/limits in the resulting deployment.
If I define any requests/limits in myvalues.yaml (either partial or full), I get those exactly in line with existing behaviour. Partial requests/limits in this case is, for example, settingresources.requests.memory and nothing else.

conditional logic for default resource values

323ebd4

ericpaulsen requested a review fromjohnstcn

March 11, 2025 17:36

Copy link

MemberAuthor

ericpaulsen commentedMar 11, 2025

thanks@johnstcn - have a look, i've added the conditional logic.

johnstcn reviewed

Mar 11, 2025

View reviewed changes

Copy link

Member

johnstcn left a 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.

Copy link

github-actionsbot commentedMar 17, 2025•
edited
Loading

All contributors have signed the CLA ✍️ ✅
_{Posted by theCLA Assistant Lite bot.}

Copy link

MemberAuthor

ericpaulsen commentedMar 17, 2025

@johnstcn FYI - the most recent commit used ClaudeCode to create the test cases.

helm: add test cases for custom resource limits and requests

c3327d3

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>

ericpaulsen force-pushed thedefault-reqs-limits branch fromcf981ed toc3327d3Compare

March 17, 2025 15:03

ericpaulsen requested a review fromjohnstcn

March 18, 2025 09:19

Copy link

MemberAuthor

ericpaulsen commentedMar 18, 2025

@johnstcn any idea whygen is failing? runningmake gen isn't cutting it

Copy link

Member

johnstcn commentedMar 18, 2025

@johnstcn any idea whygen is failing? runningmake gen isn't cutting it

The template defined inlibcoder/_coder.yaml is used for both thecoder andcoder-provisioner charts. The changes here reflect that this also causes default requests and limits to be set for thecoder-provisioner helm chart as well.

It may be possible to scope this only to thecoder Helm chart usingbuilt-in objects, otherwise we may have to also define similar defaults for thecoder-provisioner chart.