Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Apr 22, 2025
Merged

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

merged 5 commits into from
Apr 22, 2025

Conversation

ericpaulsen
Copy link
Member

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

@ericpaulsen ericpaulsen added the helm Area: helm chart label Mar 7, 2025
@ericpaulsen ericpaulsen self-assigned this Mar 7, 2025
@ericpaulsen ericpaulsen changed the title helm: set default requests/limits chore: set default requests/limits in helm chart Mar 7, 2025
johnstcn
johnstcn reviewed Mar 7, 2025
View reviewed changes
Copy link
Member

@johnstcn 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

@ericpaulsen
Copy link
Member Author

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

@johnstcn
Copy link
Member

@johnstcn what do you recommend setting in this case? if we set null 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:

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

@ericpaulsen ericpaulsen requested a review from johnstcn March 11, 2025 17:36
@ericpaulsen
Copy link
Member Author

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

johnstcn
johnstcn reviewed Mar 11, 2025
View reviewed changes
Copy link
Member

@johnstcn 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.

@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 17, 2025 •
edited
Loading

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@ericpaulsen
Copy link
Member Author

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

@ericpaulsen
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 requests
2. 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 ericpaulsen force-pushed the default-reqs-limits branch from cf981ed to c3327d3 Compare March 17, 2025 15:03
@ericpaulsen ericpaulsen requested a review from johnstcn March 18, 2025 09:19
@ericpaulsen
Copy link
Member Author

@johnstcn any idea why gen is failing? running make gen isn't cutting it

@johnstcn
Copy link
Member

@johnstcn any idea why gen is failing? running make gen isn't cutting it

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

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

@ericpaulsen ericpaulsen force-pushed the default-reqs-limits branch from 559bd10 to 2376d54 Compare March 18, 2025 13:41
@ericpaulsen
Copy link
Member Author

sorted.

johnstcn
johnstcn approved these changes Mar 25, 2025
View reviewed changes
Copy link
Member

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

@github-actions github-actions bot added the stale This issue is like stale bread. label Apr 2, 2025
@github-actions github-actions bot closed this Apr 5, 2025
@ericpaulsen ericpaulsen reopened this Apr 22, 2025
@ericpaulsen ericpaulsen merged commit cbc699b into main Apr 22, 2025
35 checks passed
@ericpaulsen ericpaulsen deleted the default-reqs-limits branch April 22, 2025 10:05
@github-actions github-actions bot locked and limited conversation to collaborators Apr 22, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@johnstcn johnstcn johnstcn approved these changes

@spikecurtis spikecurtis Awaiting requested review from spikecurtis

@bpmct bpmct Awaiting requested review from bpmct

Assignees

@ericpaulsen ericpaulsen

Labels
helm Area: helm chart stale This issue is like stale bread.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add default requests and limits to our Helm chart
2 participants
@ericpaulsen @johnstcn