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

Set memory limit for sidecar plugins on devfile #12981

Closed
gorkem opened this issue Mar 26, 2019 · 11 comments
Closed

Set memory limit for sidecar plugins on devfile #12981

gorkem opened this issue Mar 26, 2019 · 11 comments
Assignees
Labels
kind/enhancement A feature request - must adhere to the feature request template. severity/P1 Has a major impact to usage or development of the system.

Comments

@gorkem
Copy link
Contributor

gorkem commented Mar 26, 2019

We should be able to set the memory limits for a sidecar container for a plugin on a devfile. There is already a memoryLimit property associated with the tools on the defile that applies to docker image types. We should extend it to include plugins as well. I was thinking something like

tools:
  - name: theia-editor
    type: cheEditor
    id: org.eclipse.che.editor.theia:master
    memoryLimit: 256M
@gorkem gorkem added the kind/enhancement A feature request - must adhere to the feature request template. label Mar 26, 2019
@garagatyi
Copy link

What should be the behavior when several containers are injected with the plugin?

@garagatyi
Copy link

Another option could be allowing overriding plugin configuration in devfile tool in the same format as merged meta.yaml will be.
So devfile part where user overrides tool components can be:

tools:
  - name: theia-editor
    type: cheEditor
    id: org.eclipse.che.editor.theia:master
    spec:
# We override or complement endpoints here, aka exposed ports
        endpoints:
        -  name: "theia"
           public: true
           targetPort: 3100
           attributes:
             protocol: http 
             type: ide
             secure: true
             cookiesAuthEnabled: true
             discoverable: false
# We override parts of container(s) here or even add new ones
# if there is single container in the plugin no matching needed
# if there are several containers in the plugin container names are used for matching which container config gets overriden
        containers:
         - name: theia-ide
# We can rewrite image here if we want something truly customizable
           image: eclipse/che-theia:next
# We can add environment variables here
           env:
               - name: THEIA_PLUGINS
                 value: local-dir:///plugins
               - name: HOSTED_PLUGIN_HOSTNAME
                 value: 0.0.0.0
               - name: HOSTED_PLUGIN_PORT
                 value: 3130
# We can add volumes here
           volumes:
               - mountPath: "/plugins"
                 name: plugins
               - mountPath: "/projects"
                 name: projects
           ports:
               - exposedPort: 3100
           memoryLimit: "1536M"
     mountSources: true

or in the simplest case when only memory needs to be overriden:

tools:
  - name: theia-editor
    type: cheEditor
    id: org.eclipse.che.editor.theia:master
    spec:
        containers:
         - memoryLimit: "1536M"

It doesn't look that pretty, I agree. On the other hand it brings additional features like overriding volumes/ports/env vars.

@garagatyi
Copy link

Question that I'm thinking ATM is how UX for memory overriding should look like if we do automatic plugins collocation as Thomas suggested. In that case several plugins would go into the same sidecar, so should we also add overall mem limit of all the plugins involved?

@garagatyi
Copy link

@sleshchenko It is what we discussed earlier today.

@skabashnyuk skabashnyuk added team/platform severity/P1 Has a major impact to usage or development of the system. labels Mar 27, 2019
@gorkem
Copy link
Contributor Author

gorkem commented Mar 30, 2019

If several plugins are co-located on the same container are they considered one che plugin or multiple?

@garagatyi
Copy link

@gorkem sorry, missed your comment
At the moment, we have an ability to specify several VS Code plugins from a marketplace in a single Che plugin, but we do not have automatic co-location. I was referring to the discussion with Thomas on when we would have it.

@skabashnyuk
Copy link
Contributor

FYI we took this issue in the current sprint. The goal would be to set up the container memory limit or if a tool has more then one container we either split or set the same limit for all containers (it's up to a person who will implement it).

@garagatyi
Copy link

Not sure we should implement without deciding what behavior it should use. And this doesn't solve overriding env vars as requested here #13157.
Shouldn't it be better to provide a more holistic approach?

@garagatyi
Copy link

By aligning configuration in meta.yaml and devfile tool we could make it simpler because of less differences. We could even allow providing whole configuration of the plugin in devfile over meta.yaml without publishing it in pastebin or github which would simplify testing of new plugins. And all of that is achievable but just aligning formats.

@skabashnyuk
Copy link
Contributor

@garagatyi sorry for late informing. Described approach was agreed with @l0rd . I doubt that we can handle more complex logic until GA. Maybe we can do it as an improvement later.

@l0rd
Copy link
Contributor

l0rd commented Apr 16, 2019

@garagatyi we have chosen the more pragmatic approach. Rapidly get the 80% of use cases solved. We will iterate and improve that after GA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement A feature request - must adhere to the feature request template. severity/P1 Has a major impact to usage or development of the system.
Projects
None yet
Development

No branches or pull requests

5 participants