Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
ericpaulsen merged 5 commits intomainfromdefault-reqs-limits
Apr 22, 2025

Conversation

ericpaulsen
Copy link
Member

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

@ericpaulsenericpaulsen added the helmArea: helm chart labelMar 7, 2025
@ericpaulsenericpaulsen self-assigned thisMar 7, 2025
@ericpaulsenericpaulsen changed the titlehelm: set default requests/limitschore: set default requests/limits in helm chartMar 7, 2025
Copy link
Member

@johnstcnjohnstcn 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
MemberAuthor

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

@johnstcn
Copy link
Member

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

  1. If I define no default requests/limits in myvalues.yaml, I get default requests/limits in the resulting deployment.
  2. 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.
ericpaulsen reacted with thumbs up emoji

@ericpaulsen
Copy link
MemberAuthor

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

Copy link
Member

@johnstcnjohnstcn 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-actionsGitHub Actions
Copy link

github-actionsbot commentedMar 17, 2025
edited
Loading

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

ericpaulsen and johnstcn reacted with hooray emoji

@ericpaulsen
Copy link
MemberAuthor

@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>
@ericpaulsen
Copy link
MemberAuthor

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

johnstcn reacted with eyes emoji

@johnstcn
Copy link
Member

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

ericpaulsen reacted with thumbs up emoji

@ericpaulsen
Copy link
MemberAuthor

sorted.

Copy link
Member

@johnstcnjohnstcn 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-actionsgithub-actionsbot added the staleThis issue is like stale bread. labelApr 2, 2025
@ericpaulsenericpaulsen merged commitcbc699b intomainApr 22, 2025
30 checks passed
@ericpaulsenericpaulsen deleted the default-reqs-limits branchApril 22, 2025 10:05
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsApr 22, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@johnstcnjohnstcnjohnstcn approved these changes

@spikecurtisspikecurtisAwaiting requested review from spikecurtis

@bpmctbpmctAwaiting requested review from bpmct

Assignees

@ericpaulsenericpaulsen

Labels
helmArea: helm chartstaleThis 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

[8]ページ先頭

©2009-2025 Movatter.jp