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.

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

Add CPU limitation #471

Closed
wants to merge 2 commits into from
Closed

Conversation

vitaliy-guliy
Copy link
Contributor

@vitaliy-guliy vitaliy-guliy commented May 25, 2020

Signed-off-by: Vitaliy Gulyy vgulyy@redhat.com

What does this PR do?

Adds necessary CPU limitation for the plugin container configuration

Solves eclipse-che/che#16685

A sheet with a list of images used in plugins
https://docs.google.com/spreadsheets/d/1twoMLRC2A6YjTGiqXGvGvZP_M8GuKGydK75vhXQor2A/edit?usp=sharing

Signed-off-by: Vitaliy Gulyy <vgulyy@redhat.com>
Signed-off-by: Vitaliy Gulyy <vgulyy@redhat.com>
@che-bot
Copy link
Contributor

che-bot commented May 25, 2020

Happy path tests passed.

1 similar comment
@che-bot
Copy link
Contributor

che-bot commented May 25, 2020

Happy path tests passed.

Copy link
Contributor

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

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

Generally LGTM but should be tested. In many cases we're providing a lower cpuLimit than plugins had to begin with -- e.g. on che.openshift.io, cpuLimit was/is set to 2x the memoryLimit. E.g. for the java plugin, this means going from 3 CPUs allocated to 0.5.

@l0rd
Copy link
Contributor

l0rd commented May 25, 2020

A few comments:

  1. setting the same limit for every plugin is useless, we already have defaults for this (CHE_WORKSPACE_SIDECAR_DEFAULT__CPU__LIMIT__CORES)
  2. we should profile a plugins to see how much CPU it uses instead of randomly providing a value: workspaces get broken if we specify wrong values, it's better to set nothing instead.
  3. we should specify requests too (in the case of this PR cpuRequest but in general memoryRequest would be helpful as well)
  4. I would recommend to start with che-theia and che-machine-exec only and then, after we get some feedback and we see how it goes, iterate with other plugins as well

@@ -78,10 +78,11 @@ spec:
- exposedPort: 13131
- exposedPort: 13132
- exposedPort: 13133
memoryLimit: "512M"
memoryLimit: 512M
cpuLimit: 500m
Copy link
Member

Choose a reason for hiding this comment

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

Does it really work well?
The benchmarks @vzhukovs did in eclipse-che/che#18565 (comment) say the opposite - with 500m Che-Theia slows down significantly. See the Measurement 3.

@ericwill
Copy link
Contributor

ericwill commented Feb 2, 2021

Obsoleted by #774

@ericwill ericwill closed this Feb 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants