CPU limitations by vitaliy-guliy · Pull Request #774 · eclipse-che/che-plugin-registry

vitaliy-guliy · 2020-12-17T17:05:22Z

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

What does this PR do?

Adds CPU requests/limits to all the plugin sidecars.

What issues does this PR fix or reference?

eclipse-che/che#16685

How to test this PR?

Option 1

Build plugin registry by hands and deploy local Che.

Steps to do:

build the plugin registry from this branch

$ ./build.sh

upload the plugin registry image to your account at quay.io

$ docker tag quay.io/eclipse/che-plugin-registry:nightly quay.io/vgulyy/che-plugin-registry:resource-limit
$ docker push quay.io/vgulyy/che-plugin-registry:resource-limit

deploy local Che with applying following patch.yaml to replace the default plugin registry

$ chectl server:start --platform=minikube --che-operator-cr-patch-yaml patch.yaml

path.yaml content

spec:
  server:
    pluginRegistryImage: quay.io/vgulyy/che-plugin-registry:resource-limit

create any workspace and test how plugins work

Plugins in local plugin registry must have resource limitations provided by this PR.

Option 2

Use devfiles to test some stacks.

All the devfiles are using pre-built plugin registry from this PR.

Option 3

Use factories to test changes on che.openshift.io using devfiles from Option 2.

Test summary

https://docs.google.com/spreadsheets/d/1aktG-vJM1Mxqb77pkWQpiiBbXudzdByMwKG1toION8Q/edit#gid=0

PR Checklist

As the author of this Pull Request I made sure that:

Reviewers

Reviewers, please comment how you tested the PR when approving it.

sunix · 2020-12-28T09:35:53Z

@vitaliy-guliy we have gh action that automatically build and push the content to surge,sh. https://pr-check-774-alpine-che-plugin-registry.surge.sh/
Alternatively if the PR is not created yet, you can use the devfile to push on surge.sh

vitaliy-guliy · 2020-12-28T09:42:01Z

@vitaliy-guliy we have gh action that automatically build and push the content to surge,sh. https://pr-check-774-alpine-che-plugin-registry.surge.sh/
Alternatively if the PR is not created yet, you can use the devfile to push on surge.sh

I haven't noticed, that we have this action. Thanks.
Will rework the links for workspace creation.

svor · 2020-12-28T10:20:43Z

che-theia-plugins.yaml

@@ -1,22 +1,30 @@
 version: 1.0.0
 plugins:
+


@vitaliy-guliy maybe it makes sense to apply some formatter for this file, for example https://jsonformatter.org/yaml-formatter, like it was done for https://github.com/eclipse/che-plugin-registry/blob/master/che-plugins.yaml

I intentionally added empty lines to separate the plugins each other. Also the formatter removes comments with plugin ID.

The links to the extensions become written in two lines, instead of one line.

extensions: - >- https://github.com/microsoft/vscode-pull-request-github/releases/download/0.20.0/vscode-pull-request-github-0.20.0.vsix

Anything else stay unchanged. Let's leave it as it is at the moment. WDYT?

I would prefer it if the comments with the plugin ID are removed, and the che-theia-plugins.yaml file stays in the same format as the others che-*.yaml files. The ID fields are based on the publisher field from the upstream package.json files, which can change at any time. Writing the IDs here means that they could be out of date.

@ericwill the file is formatted by https://jsonformatter.org/yaml-formatter as @svor mentioed

che-theia-plugins.yaml

svor · 2020-12-28T13:59:42Z

I don't see how it is related to this PR but nodejs-mongo stack from your list of factories doesn't work, the command to run the application is failed:

I tried to create a workspace from existing devfile registry on che.openshift.io and the command works for that ws.

vitaliy-guliy · 2020-12-28T14:13:12Z

@svor I think it's related to permissions issue on PVC. You probably got an ephemeral workspace when you tried to created workspace from same devfile on che.openshift.io. That's why it's working.

svor · 2020-12-28T14:50:37Z

yes, it was the reason. Tested nodejs-mongo devfile and everything worked without problems

ibuziuk

Is it expected to define such a big CPU request? I though the idea would be defining CPU request fairly low, and currently, it is not clear why 0.2 is considered to be the fair request

vitaliy-guliy · 2020-12-30T11:35:04Z

Is it expected to define such a big CPU request? I though the idea would be defining CPU request fairly low, and currently, it is not clear why 0.2 is considered to be the fair request

There was no special idea to set 0.2. Taking a look at cpu limitation for jwtproxy, I see that 20m would be enough. WDYT?

ibuziuk · 2020-12-30T12:50:47Z

@vitaliy-guliy jwt proxy request is 0.03 not 0.2 - https://github.com/eclipse/che/pull/18551/files#diff-deca31b9cca47d5b0356dd75fc0fd0c8cb980f3aa029da27b475bdbb37353eb0R611

you can follow up on the verification discussion in the PR e.g. - eclipse-che/che#18551 (comment)

ibuziuk

CPU requests are very high

vitaliy-guliy · 2020-12-30T13:18:45Z

CPU requests are very high

updated

svor

OK for me

vitaliy-guliy · 2021-01-05T13:45:21Z

@ibuziuk it seems both CPU request and limitation were not applied to the plugin container eclipse-che/che#18730

ibuziuk

This looks like a good start, but I'm personally missing the justification / metrics for the following request & limits which is set identical to every plugin:

      cpuLimit: 500m
      cpuRequest: 30m

ericwill · 2021-01-12T15:52:22Z

This looks like a good start, but I'm personally missing the justification / metrics for the following request & limits which is set identical to every plugin:
      cpuLimit: 500m
      cpuRequest: 30m

Let's start with a higher bound and we can tighten it with each iteration. From my side I'm fine with the PR, if you don't have any other concerns then let's merge it.

ibuziuk

Let's start with a higher bound and we can tighten it with each iteration. From my side I'm fine with the PR, if you don't have any other concerns then let's merge it.

agree, @vitaliy-guliy could you please also update the eclipse-che/che#16685 with details

@l0rd WDYT about backporing this also to 7.24.x?

l0rd · 2021-01-13T11:55:30Z

Have those changes been tested on openshift dev cluster (or cluster bot) and RHPDS?

vitaliy-guliy · 2021-01-13T12:35:22Z

Have those changes been tested on openshift dev cluster (or cluster bot) and RHPDS?

No, I will test them today on both instances you described.
On hosted Che everything is working fine.

vitaliy-guliy · 2021-01-25T11:00:30Z

@l0rd all the devfiles have been tested on RHPDS and openshift dev cluster. Everything is working as expected.
I attached a spreadsheet to the PR description with testing results.

l0rd · 2021-01-27T00:24:50Z

Thanks @vitaliy-guliy. You have tested on minikube too right?

vitaliy-guliy · 2021-01-27T13:27:56Z

Thanks @vitaliy-guliy. You have tested on minikube too right?

It's the first what I did. On minikube I've tested not all the devfiles, but node, java, dotnet worked well. One thing is java language server works a bit slowly even locally. So, I will add it a bit more CPU.

vitaliy-guliy · 2021-01-27T13:29:10Z

@benoitf it seems happy path tests don't work for all the PRs. WDYT if we merge this PR as it is?

ericwill · 2021-02-01T18:12:12Z

@vitaliy-guliy it's fine from my POV, please resolve conflicts and merge when you are ready

codecov · 2021-02-02T15:47:00Z

Codecov Report

Merging #774 (0cdaa89) into master (a87ebf3) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #774   +/-   ##
=======================================
  Coverage   91.44%   91.44%           
=======================================
  Files          38       38           
  Lines         923      923           
  Branches      105      105           
=======================================
  Hits          844      844           
  Misses         79       79

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ba6127...07ca325. Read the comment docs.

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

vitaliy-guliy marked this pull request as ready for review December 24, 2020 23:42

vitaliy-guliy requested review from benoitf, ericwill, sunix, svor, tsmaeder and vinokurig as code owners December 24, 2020 23:42

svor reviewed Dec 28, 2020

View reviewed changes

che-theia-plugins.yaml Outdated Show resolved Hide resolved

svor reviewed Dec 28, 2020

View reviewed changes

che-theia-plugins.yaml Outdated Show resolved Hide resolved

ibuziuk reviewed Dec 30, 2020

View reviewed changes

ibuziuk requested changes Dec 30, 2020

View reviewed changes

svor approved these changes Jan 5, 2021

View reviewed changes

vitaliy-guliy mentioned this pull request Jan 5, 2021

[hosted Che] CPU limitations for plugin sidecar is not applied eclipse-che/che#18730

Closed

1 task

vitaliy-guliy requested a review from ibuziuk January 11, 2021 11:03

ibuziuk reviewed Jan 11, 2021

View reviewed changes

ibuziuk approved these changes Jan 13, 2021

View reviewed changes

vitaliy-guliy mentioned this pull request Jan 20, 2021

Remove Memory limitations for plugins eclipse-che/che-devfile-registry#326

Merged

9 tasks

ericwill approved these changes Jan 25, 2021

View reviewed changes

ericwill approved these changes Feb 1, 2021

View reviewed changes

Limit CPU usage

07ca325

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

vitaliy-guliy force-pushed the resource-limit branch from 0cdaa89 to 07ca325 Compare February 2, 2021 15:55

vitaliy-guliy merged commit 58d0493 into master Feb 2, 2021

vitaliy-guliy deleted the resource-limit branch February 2, 2021 16:33

ericwill mentioned this pull request Feb 2, 2021

Add CPU limitation #471

Closed

azatsarynnyy mentioned this pull request Feb 3, 2021

Java Maven sample on minikube is not starting eclipse-che/che#18955

Closed

l0rd mentioned this pull request May 3, 2021

Add resource limits/requests to the devfiles and plugins in the registries eclipse-che/che#16685

Closed

7 tasks

Conversation

vitaliy-guliy commented Dec 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

What issues does this PR fix or reference?

How to test this PR?

Option 1

Option 2

Option 3

Test summary

PR Checklist

Reviewers

Uh oh!

sunix commented Dec 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vitaliy-guliy commented Dec 28, 2020

Uh oh!

svor Dec 28, 2020

Choose a reason for hiding this comment

Uh oh!

vitaliy-guliy Dec 30, 2020

Choose a reason for hiding this comment

Uh oh!

ericwill Jan 4, 2021

Choose a reason for hiding this comment

Uh oh!

vitaliy-guliy Jan 12, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

svor commented Dec 28, 2020

Uh oh!

vitaliy-guliy commented Dec 28, 2020

Uh oh!

svor commented Dec 28, 2020

Uh oh!

ibuziuk left a comment

Choose a reason for hiding this comment

Uh oh!

vitaliy-guliy commented Dec 30, 2020

Uh oh!

ibuziuk commented Dec 30, 2020

Uh oh!

ibuziuk left a comment

Choose a reason for hiding this comment

Uh oh!

vitaliy-guliy commented Dec 30, 2020

Uh oh!

svor left a comment

Choose a reason for hiding this comment

Uh oh!

vitaliy-guliy commented Jan 5, 2021

Uh oh!

ibuziuk left a comment

Choose a reason for hiding this comment

Uh oh!

ericwill commented Jan 12, 2021

Uh oh!

ibuziuk left a comment

Choose a reason for hiding this comment

Uh oh!

l0rd commented Jan 13, 2021

Uh oh!

vitaliy-guliy commented Jan 13, 2021

Uh oh!

vitaliy-guliy commented Jan 25, 2021

Uh oh!

l0rd commented Jan 27, 2021

Uh oh!

vitaliy-guliy commented Jan 27, 2021

Uh oh!

vitaliy-guliy commented Jan 27, 2021

Uh oh!

ericwill commented Feb 1, 2021

Uh oh!

codecov bot commented Feb 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

vitaliy-guliy commented Dec 17, 2020 •

edited

Loading

sunix commented Dec 28, 2020 •

edited

Loading

codecov bot commented Feb 2, 2021 •

edited

Loading