Propagate plugin/editor component environment variables #15435

sleshchenko · 2019-12-09T14:58:19Z

What does this PR do?

It introduces a new feature: propagating plugin/editor component environment variables.

This is archived by the following architecture change:
Now we propagate the whole Devfile along with WorkspaceConfig and InternalEnvironment and it makes it possible to access components configuration and apply it on plugin brokering phase.

What can be done better maybe in scope of separate PRs:

Now EditorComponentToWorkspaceConfigApplier/PluginComponentToWorkspaceConfigApplier modifies components that was originally passed. It's needed to make it possible to match component (configured with reference) with ChePlugin provided by plugin broker. As an improvement, we can get rid of it but Plugin Broker should provide additional information where it downloaded ChePlugin configuration. More see Propagate plugin/editor component environment variables #15435 (comment)
The whole Devfile is passed to WorkspaceConfig/InternalEnvironment and by the time being only Plugins/Editors components are used. As an alternative way we could propagate only used parts and there is no clear and easy way to do it smoothly. See more Propagate plugin/editor component environment variables #15435 (comment)

How to test this PR

Build this branch or use the following image: sleshchenko/che-server:env-vars
Create a workspace with the following Devfile

Devfile

metadata:
  name: env-override
components:
  - type: kubernetes
    reference: >-
      https://gist.githubusercontent.com/sleshchenko/86d28a2c4426bdd0cef57bdfe3c7a829/raw/555b20c10ff7d6519b80d0841a30735a2f4f2118/deployment.yaml
    alias: env-test
    env:
      - value: Serhii
        name: NAME
  - reference: https://gist.githubusercontent.com/sleshchenko/5e1118c93a17e6b7d4efc4cd6f88b164/raw/471553b59cf72510289b497e0f7a6f1c86e48450/meta.yaml
    memoryLimit: 1G
    type: cheEditor
    alias: editor
    env:
      - value: Hello My Dear Friend
        name: TEST
  - id: ms-vscode/go/latest
    type: chePlugin
    alias: plugin
    memoryLimit: 256M
    env:
      - value: Hello My Dear Friend
        name: TEST
apiVersion: 1.0.0
commands:
  - name: testk8s
    actions:
      - type: exec
        command: cd /hello && ls
        component: env-test
  - name: test-editor
    actions:
      - type: exec
        command: 'echo ${TEST}'
        component: editor
  - name: test-plugin
    actions:
      - type: exec
        command: 'echo ${TEST}'
        component: plugin

2. Run every command and make sure that output has values provided with env vars. 3. Check workspace pod deployment (kubectl or K8s/OS Dashboard) and make sure that memoryLimit is right.

What issues does this PR fix or reference?

#14343

Release Notes

N/A

Docs PR

eclipse-che/che-docs#966

che-bot · 2019-12-09T14:58:35Z

❌ E2E Happy path tests failed ❗

See Details

Jenkins job
test report
logs and configs
Happy path tests DevFile
image: maxura/che-server:15435

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

⚠️ https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

ℹ️ Use comment "crw-ci-test" to rerun happy path E2E test.

che-bot · 2019-12-09T15:02:48Z

E2E tests of Eclipse Che Multiuser on OCP has failed:

Build details
Test report
docker image: eclipseche/che-server:15435
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.
Use command "ci-test" to rerun the test.

amisevsk

I agree that having to set the plugin id on components is a difficult situation, I think it might be cleaner to update our model and the plugin broker, as I feel like we'll run into the situation of components without IDs more in the future.

amisevsk · 2019-12-09T19:48:32Z

...lipse/che/workspace/infrastructure/kubernetes/wsplugins/KubernetesPluginsToolingApplier.java

@@ -124,8 +128,16 @@ public void apply(

      Collection<CommandImpl> pluginRelatedCommands = commandsResolver.resolve(chePlugin);

+      DevfileImpl devfile = k8sEnv.getDevfile();
+      Optional<ComponentImpl> pluginRelatedComponentOpt = devfile.getComponents().stream()
+          .filter(c -> chePlugin.getId().equals(c.getId())).findAny();


What about updating the ChePlugin model to include a source field, where we could set either plugin ID or reference as a unique identifier in the plugin broker (I can implement this side if it helps). If the Che server is going to start downloading plugin metas it may as well do all the work in the broker.

it does not change a lot but makes a things a bit cleaner. Added the corresponding TODO

sleshchenko · 2019-12-10T10:01:56Z

.../src/main/java/org/eclipse/che/api/workspace/server/spi/environment/InternalEnvironment.java

@@ -151,6 +153,10 @@ public InternalEnvironment setWarnings(List<Warning> warnings) {
    return attributes;
  }

+  public DevfileImpl getDevfile() {


To solve the issue with env vars propagating we could instead of putting here Devfile introduce two fields: Component editor, List<Component> plugins, and then use this as one storage instead of putting workspace config attributes + whole devfile.

Sounds OK for me at this stage, since devfile contains a lot of info which is unused here

Maybe another solution to the problem solved by explicitly setting the id of editors and components would be if we stored Map<String, Component> for editors and plugins (or maybe separately, not sure) where the key would be the resolved FQN.

I tried and failed.
Editor/Plugin component now is in workspace module (not k8s specific).
Moving editor/plugin configuration into environment would mean that we need make editor/plugin components do k8s/os infra specific and I'm not sure that it's a good think to do.

We also would be able to introduce editor/plugins in workspace config level BUT now they are in workspace config attributes which are propagated like in many places, and it would mean that we should propagate editor/plugins as a separate arguments to all places where it's needed...
And I'm not sure that we should invest time into such evolution. The more meaningful change would be to get rid of WorkspaceConfig at all, but it requires quite a lot of changes.

It seems that putting while Devfile into WorkspaceConfig and InternalEnvironment is the quickest option.

che-bot · 2019-12-10T13:08:36Z

❌ E2E Happy path tests failed ❗

See Details

Jenkins job
test report
logs and configs
Happy path tests DevFile
image: maxura/che-server:15435

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

⚠️ https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

ℹ️ Use comment "crw-ci-test" to rerun happy path E2E test.

che-bot · 2019-12-10T13:27:50Z

❌ E2E Happy path tests failed ❗

See Details

Jenkins job
test report
logs and configs
Happy path tests DevFile
image: maxura/che-server:15435

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

⚠️ https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

ℹ️ Use comment "crw-ci-test" to rerun happy path E2E test.

che-bot · 2019-12-10T13:51:39Z

E2E tests of Eclipse Che Multiuser on OCP has failed:

Build details
Test report
docker image: eclipseche/che-server:15435
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.
Use command "ci-test" to rerun the test.

amisevsk · 2019-12-10T13:57:59Z

...api/workspace/server/devfile/convert/component/editor/EditorComponentToWorkspaceApplier.java

+    // TODO it's needed to introduce source field into ChePlugin object with the following structure
+    // String registryUrl
+    // String id
+    // String reference


My thought was to update ChePlugin to have a field source that is just a string with either

source: <reference>

or

source: id // or registryUrl/id if applicable

but I agree that this makes sense as a future consideration; it would be easier to implement after the brokers refactor is mergd.

source: id // or registryUrl/id if applicable

I believe having single string value would make a component to che-plugin matching harder...
Component has

id registryUrl //optional - default will be used if omitted reference

if plugin broker used default plugin registry - should it be included into source?
if yes - Che Server then should do the same defaultPluginRegistry+id, or cut id from source and then compare it.
But Che Server does not know for sure what is value of source field...
not sure if I clearly share my concerns BTW it's good that we agreed that it could be improved as a future consideration.

Hmm, I'm not sure on the best approach. I'll need to think about it more -- my initial idea was to have source match the attribute on the WorkspaceConfig, so that it could just be parsed, but maybe that complicates things.

If adding the whole devfile into workspace config is a working solution, that may just be easier.

...lipse/che/workspace/infrastructure/kubernetes/wsplugins/KubernetesPluginsToolingApplier.java

metlos · 2019-12-10T15:01:38Z

.../src/main/java/org/eclipse/che/api/workspace/server/spi/environment/InternalEnvironment.java

@@ -151,6 +153,10 @@ public InternalEnvironment setWarnings(List<Warning> warnings) {
    return attributes;
  }

+  public DevfileImpl getDevfile() {


Maybe another solution to the problem solved by explicitly setting the id of editors and components would be if we stored Map<String, Component> for editors and plugins (or maybe separately, not sure) where the key would be the resolved FQN.

che-bot · 2019-12-11T09:58:39Z

✅ E2E Happy path tests succeed 🎉

See Details

Jenkins job
logs and configs
Happy path tests DevFile
image: maxura/che-server:15435

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

che-bot · 2019-12-11T10:44:22Z

E2E tests of Eclipse Che Multiuser on OCP has been successful:

Build details
Test report
docker image: eclipseche/che-server:15435

slemeur · 2019-12-11T13:08:43Z

@sleshchenko : This work is also important for this issue eclipse-che/che-theia#429 . We need to test the pull request in the context of configuring a settings.xml for the project and language server.
@tolusha : Would you check that with @sleshchenko ?

amisevsk

LGTM. Tested using your image and devfile and it works as expected

metlos

LGTM. Just a small improvement in the error messages suggested.

...lipse/che/workspace/infrastructure/kubernetes/wsplugins/KubernetesPluginsToolingApplier.java

Signed-off-by: Sergii Leshchenko <sleshche@redhat.com>

che-bot · 2019-12-13T09:06:17Z

✅ E2E Happy path tests succeed 🎉

See Details

Jenkins job
logs and configs
Happy path tests DevFile
image: maxura/che-server:15435

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

che-bot · 2019-12-13T09:06:20Z

✅ E2E Happy path tests succeed 🎉

See Details

Jenkins job
logs and configs
Happy path tests DevFile
image: maxura/che-server:15435

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

che-bot · 2019-12-13T09:40:50Z

E2E tests of Eclipse Che Multiuser on OCP has been successful:

Build details
Test report
docker image: eclipseche/che-server:15435

che-bot · 2019-12-17T16:48:33Z

❌ E2E Happy path tests failed ❗

See Details

Jenkins job
test report
logs and configs
Happy path tests DevFile
image: maxura/che-server:15435

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

⚠️ https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

ℹ️ Use comment "crw-ci-test" to rerun happy path E2E test.

che-bot · 2019-12-17T17:41:15Z

E2E tests of Eclipse Che Multiuser on OCP has failed:

Build details
Test report
docker image: eclipseche/che-server:15435
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.
Use command "ci-test" to rerun the test.

che-bot added the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Dec 9, 2019

amisevsk reviewed Dec 9, 2019

View reviewed changes

sleshchenko commented Dec 10, 2019

View reviewed changes

sleshchenko force-pushed the editorEnv branch from ec46260 to 42a04ee Compare December 10, 2019 13:08

amisevsk reviewed Dec 10, 2019

View reviewed changes

metlos reviewed Dec 10, 2019

View reviewed changes

sleshchenko force-pushed the editorEnv branch from 80dd818 to 822a1b3 Compare December 11, 2019 08:48

sleshchenko mentioned this pull request Dec 11, 2019

Add info how to tune memory limits and env vars for components eclipse-che/che-docs#966

Merged

sleshchenko marked this pull request as ready for review December 11, 2019 09:19

sleshchenko requested review from l0rd, nickboldt, rhopp and skabashnyuk as code owners December 11, 2019 09:19

che-bot added the kind/enhancement A feature request - must adhere to the feature request template. label Dec 11, 2019

amisevsk approved these changes Dec 11, 2019

View reviewed changes

mshaposhnik mentioned this pull request Dec 12, 2019

Support ENV variables in cheEditor/chePlugin components of the devfile #15401

Closed

mshaposhnik approved these changes Dec 12, 2019

View reviewed changes

metlos approved these changes Dec 12, 2019

View reviewed changes

...lipse/che/workspace/infrastructure/kubernetes/wsplugins/KubernetesPluginsToolingApplier.java Outdated Show resolved Hide resolved

sleshchenko force-pushed the editorEnv branch from 822a1b3 to b30f16f Compare December 13, 2019 08:45

mshaposhnik and others added 3 commits December 13, 2019 10:45

Add env to editors

caa6187

Support ENV variables for cheEditor-chePlugin

2b82471

Propage env variables and memory limit on plugin brokering phase

294cf4e

Signed-off-by: Sergii Leshchenko <sleshche@redhat.com>

sleshchenko force-pushed the editorEnv branch from b30f16f to 294cf4e Compare December 13, 2019 08:45

sleshchenko merged commit c0039ec into eclipse-che:master Dec 13, 2019

sleshchenko deleted the editorEnv branch December 13, 2019 11:57

che-bot removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Dec 13, 2019

che-bot added this to the 7.6.0 milestone Dec 13, 2019

This was referenced Dec 13, 2019

Support for Environment Variables in Plugins and Editors #14343

Closed

Provision environment variables for initContainers of chePlugin/cheEditor #15508

Merged

slemeur mentioned this pull request Jan 7, 2020

Implement Airgapped Configuration Mechanism #15518

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Propagate plugin/editor component environment variables #15435

Propagate plugin/editor component environment variables #15435

sleshchenko commented Dec 9, 2019 •

edited

Loading

che-bot commented Dec 9, 2019

che-bot commented Dec 9, 2019

amisevsk left a comment

amisevsk Dec 9, 2019

sleshchenko Dec 10, 2019

sleshchenko Dec 10, 2019

mshaposhnik Dec 10, 2019

metlos Dec 10, 2019

sleshchenko Dec 10, 2019

che-bot commented Dec 10, 2019

che-bot commented Dec 10, 2019

che-bot commented Dec 10, 2019

amisevsk Dec 10, 2019

sleshchenko Dec 10, 2019

amisevsk Dec 10, 2019

metlos Dec 10, 2019

che-bot commented Dec 11, 2019

che-bot commented Dec 11, 2019

slemeur commented Dec 11, 2019

amisevsk left a comment

metlos left a comment

che-bot commented Dec 13, 2019

che-bot commented Dec 13, 2019

che-bot commented Dec 13, 2019

che-bot commented Dec 17, 2019

che-bot commented Dec 17, 2019

Propagate plugin/editor component environment variables #15435

Propagate plugin/editor component environment variables #15435

Conversation

sleshchenko commented Dec 9, 2019 • edited Loading

What does this PR do?

How to test this PR

What issues does this PR fix or reference?

Release Notes

Docs PR

che-bot commented Dec 9, 2019

che-bot commented Dec 9, 2019

amisevsk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

che-bot commented Dec 10, 2019

che-bot commented Dec 10, 2019

che-bot commented Dec 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

che-bot commented Dec 11, 2019

che-bot commented Dec 11, 2019

slemeur commented Dec 11, 2019

amisevsk left a comment

Choose a reason for hiding this comment

metlos left a comment

Choose a reason for hiding this comment

che-bot commented Dec 13, 2019

che-bot commented Dec 13, 2019

che-bot commented Dec 13, 2019

che-bot commented Dec 17, 2019

che-bot commented Dec 17, 2019

sleshchenko commented Dec 9, 2019 •

edited

Loading