Skip to content
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

fix(helm): set CP memory limits, by default equal to memory request, set CP CPU requests #6127

Merged
merged 6 commits into from
Mar 10, 2023
Merged

fix(helm): set CP memory limits, by default equal to memory request, set CP CPU requests #6127

merged 6 commits into from
Mar 10, 2023

Conversation

michaelbeaumont
Copy link
Contributor

@michaelbeaumont michaelbeaumont commented Feb 24, 2023 •
edited
Loading

See https://web.archive.org/web/20220805232857/https://home.robusta.dev/blog/stop-using-cpu-limits/ and https://web.archive.org/web/20220720151847/https://github.com/robusta-dev/alert-explanations/wiki/CPUThrottlingHigh-(Prometheus-Alert)
Generally we shouldn't be setting CPU limits on Pods.

Best practice on memory is request=limit

Blocked on #6121

Reference stackrox/kube-linter#459

Checklist prior to review

  • Link to relevant issue as well as docs and UI issues -- Part of reintroduce kube-linter checks #6049
  • This will not break child repos: it doesn't hardcode values (.e.g "kumahq" as a image registry) and it will work on Windows, system specific functions like syscall.Mkfifo have equivalent implementation on the other OS --
  • Tests (Unit test, E2E tests, manual test on universal and k8s) --
  • Do you need to update UPGRADE.md? --
  • Does it need to be backported according to the backporting policy? --
  • Do you need to explicitly set a > Changelog: entry here or add a ci/ label to run fewer/more tests?

@michaelbeaumont michaelbeaumont changed the title fix(helm): set CP memory limits, by default equal to memory request fix(helm): set CP memory limits, by default equal to memory request, set CP CPU requests Feb 24, 2023
lahabana
lahabana reviewed Feb 28, 2023
View reviewed changes
mk/check.mk Show resolved Hide resolved
@michaelbeaumont michaelbeaumont marked this pull request as ready for review February 28, 2023 17:11
@michaelbeaumont michaelbeaumont requested review from a team, lobkovilya, lukidzi and lahabana and removed request for a team February 28, 2023 17:11
@michaelbeaumont michaelbeaumont enabled auto-merge (squash) February 28, 2023 17:20
@michaelbeaumont michaelbeaumont added the ci/run-full-matrix PR: Runs all possible e2e test combination (expensive use carefully) label Mar 1, 2023
lahabana
lahabana previously approved these changes Mar 1, 2023
View reviewed changes
Copy link
Contributor

@lahabana lahabana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like we need to do something for limits to make it simpler and for keeping our helm version in sync with our dependencies

@michaelbeaumont
Copy link
Contributor Author

michaelbeaumont commented Mar 1, 2023 •
edited
Loading

Feels like we need to do something for limits to make it simpler

Make what exactly simpler?

keeping our helm version in sync with our dependencies

I'll create an issue: #6165

@lahabana
Copy link
Contributor

lahabana commented Mar 1, 2023 •
edited
Loading

Make what exactly simpler?

The complex ifs. Should we just have a sensible default in values.yaml and avoid the ifs? Just have it be:

resources:
  requests:
    cpu: 500m
    memory: 256Mi
  limits:
     memory: 256Mi

We're trying to be clever with global and zone difference which actually doesn't make a lot of sense anyway.
Feels good defaults in values.yaml for all our component should work well

EDIT: this can be a separate issue btw :)

@lahabana
Copy link
Contributor

lahabana commented Mar 1, 2023

/golden_files

@michaelbeaumont
ci: don't require CPU limits and enable kube-linter checks
ab73ae4 
See stackrox/kube-linter#459
and https://web.archive.org/web/20220805232857/https://home.robusta.dev/blog/stop-using-cpu-limits/
Generally we shouldn't be setting CPU limits on Pods.

Signed-off-by: Mike Beaumont <mjboamail@gmail.com>
@michaelbeaumont
ci(make): only kube-lint deployments/charts/kuma
cc5a795 
Signed-off-by: Mike Beaumont <mjboamail@gmail.com>
@michaelbeaumont
fix(helm): set CP memory limits, by default equal to memory request
bc9d236 
Signed-off-by: Mike Beaumont <mjboamail@gmail.com>
@michaelbeaumont
chore: regenerate
7685ce6 
Signed-off-by: Mike Beaumont <mjboamail@gmail.com>
@michaelbeaumont michaelbeaumont requested review from lahabana and removed request for lahabana March 9, 2023 16:13
lahabana
lahabana approved these changes Mar 10, 2023
View reviewed changes
Copy link
Contributor

@lahabana lahabana left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM that makes things a lot clearer I find! We should also make these configurable for egress. They are currently hardcoded. Opened: #6252

@lahabana lahabana mentioned this pull request Mar 10, 2023
Closed
@michaelbeaumont michaelbeaumont enabled auto-merge (squash) March 10, 2023 13:01
@michaelbeaumont michaelbeaumont merged commit cc367db into master Mar 10, 2023
@michaelbeaumont michaelbeaumont deleted the fix/helm_limits branch March 10, 2023 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lahabana lahabana lahabana approved these changes

@lobkovilya lobkovilya Awaiting requested review from lobkovilya

@lukidzi lukidzi Awaiting requested review from lukidzi

Assignees
No one assigned
Labels
ci/run-full-matrix PR: Runs all possible e2e test combination (expensive use carefully)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@michaelbeaumont @lahabana