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 resource limits/requests to the plugins #461

Merged
merged 3 commits into from
Apr 1, 2021
Merged

Add resource limits/requests to the plugins #461

merged 3 commits into from
Apr 1, 2021

Conversation

svor
Copy link
Contributor

@svor svor commented Apr 1, 2021

What does this PR do?

Adds cpuLimit and cpuRequest into plugin yamls. All values were ported from the upstream.

What issues does this PR fix or reference?

https://issues.redhat.com/browse/CRW-1637

svor added 3 commits April 1, 2021 11:06
Signed-off-by: svor <vsvydenk@redhat.com>
Signed-off-by: svor <vsvydenk@redhat.com>
Signed-off-by: svor <vsvydenk@redhat.com>
@svor svor requested review from nickboldt and ghatwala April 1, 2021 08:33
@ghatwala
Copy link
Contributor

ghatwala commented Apr 1, 2021

@svor - generic query about this - how did you find/calculate/deduce on the values mentioned in all plugins here ? Am guessing unless we try these on all arches , we wouldn't know (All values were ported from the upstream, correct ? : )

cpuLimit: xxm
cpuRequest: xxm

@svor
Copy link
Contributor Author

svor commented Apr 1, 2021

@svor - generic query about this - how did you find/calculate/deduce on the values mentioned in all plugins here ? Am guessing unless we try these on all arches , we wouldn't know , correct ? : )

@ghatwala this investigation was done in the upstream eclipse-che/che-plugin-registry#774. And you are right we should test these changes on all arches to guarantee that it doesn't break anything.

Copy link
Contributor

@ghatwala ghatwala left a comment

Choose a reason for hiding this comment

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

+1 , for trying these on all arches, to check if it breaks anywhere .

@nickboldt
Copy link
Member

I'm +1 to merge this but was just wondering what the units are and what these actually do...

cpuLimit: 500m
cpuRequest: 30m

Does that mean this plugin will ask for a minimum of 0.030 and maximum of 0.500 virtual CPUs? Is the "m" for milli, as in 1/1000th of a virtual CPU?

@ericwill
Copy link
Contributor

ericwill commented Apr 1, 2021

I'm +1 to merge this but was just wondering what the units are and what these actually do...

cpuLimit: 500m
cpuRequest: 30m

Does that mean this plugin will ask for a minimum of 0.030 and maximum of 0.500 virtual CPUs? Is the "m" for milli, as in 1/1000th of a virtual CPU?

https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/#meaning-of-cpu

Basically the m stands for millicores, so 100m is 0.1 of a CPU core/hyperthread

@nickboldt nickboldt merged commit c481f3a into redhat-developer:crw-2-rhel-8 Apr 1, 2021
@svor
Copy link
Contributor Author

svor commented Apr 1, 2021

@nickboldt
cpuLimit -> to make sure that container never go above the value
cpuRequest -> is what guaranteed by k8s

nickboldt pushed a commit that referenced this pull request Apr 1, 2021
* Add resource limits/requests to the plugins

Signed-off-by: svor <vsvydenk@redhat.com>

* update cpuRequest for che-theia editor

Signed-off-by: svor <vsvydenk@redhat.com>

* code cleanup

Change-Id: If4f6b9a9e40d79b1ae1142228defba890865eaba
Signed-off-by: svor <vsvydenk@redhat.com>
@nickboldt
Copy link
Member

cherrypicked to 2.8 branch as 28fd926

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.

4 participants