Update plugin brokers to v3.4.0 and enable plugin merging #17785

amisevsk · 2020-09-03T22:56:09Z

What does this PR do?

Update plugin brokers to v3.4.0 to pick up changes from eclipse-che/che-plugin-broker#111 and enable merging plugins.

Note: This PR depends on eclipse-che/che-operator#429. If a Che server with these changes is deployed, workspace start will fail unless the v3.4.0 brokers are used (as the commandline argument is not recognized)

What issues does this PR fix or reference?

Required for #15373

Release Notes

The plugin broker, which downloads Theia plugins while workspaces are starting up, now has the capability to merge plugins that run in the same sidecar container.

Docs PR

eclipse-che/che-docs#1576

amisevsk · 2020-09-03T22:56:44Z

To test the changes, the following workspaces will result in plugins being merged:

devfiles/apache-camel-k/devfile.yaml
devfiles/apache-camel-springboot/devfile.yaml
devfiles/dotnet-asp.net/devfile.yaml
devfiles/dotnet/devfile.yaml
devfiles/nodejs/devfile.yaml
devfiles/nodejs-mongo/devfile.yaml
devfiles/php-laravel/devfile.yaml
devfiles/php-mysql/devfile.yaml
devfiles/php-symfony/devfile.yaml
devfiles/php-web-simple/devfile.yaml

che-bot · 2020-09-03T23:18:06Z

❌ E2E Happy path tests failed ❗

See Details

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

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

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

sleshchenko · 2020-09-07T06:42:29Z

Just rebased against master to fix happy path test.

che-bot · 2020-09-07T06:59:47Z

❌ E2E Happy path tests failed ❗

See Details

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

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

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

sleshchenko · 2020-09-07T07:09:47Z

There is some issue with used plugin broker:

Failed to start Kubernetes runtime of workspace workspacexba1p6wioqv734up. Cause: Plugins installation process timed out

Manual testing is needed to retrieve plugin broker logs

amisevsk · 2020-09-08T19:11:41Z

I suspect the happy path tests are failing because the Che instance is installing with brokers v3.3.0 set in the configmap. They are always overridden in the operator; this should be addressed by eclipse-che/che-operator#429

che-bot · 2020-09-08T19:37:36Z

❌ E2E Happy path tests failed ❗

See Details

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

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

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

sleshchenko · 2020-09-09T09:40:40Z

[crw-ci-test]

sleshchenko

I think che-operator changes should be pulled into happy path test and hope the latest run will pass

...che/workspace/infrastructure/kubernetes/wsplugins/brokerphases/BrokerEnvironmentFactory.java

che-bot · 2020-09-09T10:00:18Z

❌ E2E Happy path tests failed ❗

See Details

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

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

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

amisevsk · 2020-09-11T00:05:40Z

@sleshchenko I've added a property + devfile attribute to control merging, PTAL. I set the default behavior for merging plugins to disabled for now, since the next release is coming so soon. We should enable it by default in the next snapshot.

che-bot · 2020-09-11T00:39:10Z

✅ E2E Happy path tests succeed 🎉

See Details

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

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

amisevsk · 2020-09-11T01:33:11Z

Added docs PR eclipse-che/che-docs#1576

sleshchenko

👍

...org/eclipse/che/workspace/infrastructure/kubernetes/wsplugins/SidecarToolingProvisioner.java

...-core-api-workspace-shared/src/main/java/org/eclipse/che/api/workspace/shared/Constants.java

...org/eclipse/che/workspace/infrastructure/kubernetes/wsplugins/SidecarToolingProvisioner.java

sleshchenko · 2020-09-14T07:00:24Z

...-core-api-workspace-shared/src/main/java/org/eclipse/che/api/workspace/shared/Constants.java

+   * This attribute enables or disables merging plugins while provisioning a workspace. When
+   * enabled, the plugin broker will be configured to attempt to merge plugins where possible (when
+   * two or more plugins share the same image and do not have conflicting settings). Should be
+   * set/read from {@link WorkspaceConfig#getAttributes}.


Suggested change

* set/read from {@link WorkspaceConfig#getAttributes}.

* set/read from {@link Devfile#getAttributes}.

?

P.S. I'm Ok with leaving as is as well just to be consistent in that file... But eventually, all these WorkspaceConfig#getAttributes should be changed to Devfile#getAttributes

Done, except for WORKSPACE_TOOLING_EDITOR_ATTRIBUTE which seems to be a legacy attribute.

I also noticed the editorFree attribute is in org.eclipse.che.api.workspace.server.devfile.Constants instead of this file, which I think is maybe a mistake?

I may be wrong but there was some time when Devfile lived in a separate maven module and workspace was not aware of that. So, I assume that now editorFree can be moved here but it's not the purpose of this PR...

metlos

LGTM apart from 1 comment.

metlos · 2020-09-14T07:29:02Z

assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/che/che.properties

+# If set to true, the plugin brokers will attempt to merge plugins when possible (i.e. they run in
+# the same sidecar image and do not have conflicting settings). This value is the default setting
+# used when the devfile does not specify otherwise, via the "mergePlugins" attribute.
+che.workspace.plugin_broker.default_merge_plugins=false


Is this set to false by default because of minimizing the possible changes in the behavior? If I understood the changes in the plugin broker, it should be fairly conservative about when it actually does merge the plugins or not.

I set it to false because the intention was to merge this for the coming release -- I didn't want to risk introducing a regression with no time to fix it. My basic plan was

7.19.0: merging available, can be enabled by setting attribute in devfile

7.20.0: merging available and default, can be disabled by setting attribute in devfile

I generally try to get changes that can impact the default flow + workspace startup in earlier in a sprint so we can catch any unforeseen issues before releasing, so this is kind of a workaround.

che-bot · 2020-09-14T18:58:34Z

✅ E2E Happy path tests succeed 🎉

See Details

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

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

Add attribute 'mergePlugins' to devfile to manually enable/disable plugin merging in the plugin broker. Default value when no property is present is controlled by property che.workspace.plugin_broker.default_merge_plugins Add a small note to the plugin broker images config lines in che.properties to make it clear that the Che Operator overrides the properties by default. Signed-off-by: Angel Misevski <amisevsk@redhat.com>

amisevsk · 2020-09-15T13:55:31Z

Merged #17870 and squashed + rebased this PR on master.

che-bot · 2020-09-15T14:04:14Z

❌ E2E Happy path tests failed ❗

See Details

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

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

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

amisevsk · 2020-09-15T18:12:04Z

[crw-ci-test]

che-bot · 2020-09-15T18:15:57Z

❌ E2E Happy path tests failed ❗

See Details

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

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

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

amisevsk · 2020-09-15T21:34:30Z

[crw-ci-test]

che-bot · 2020-09-15T21:48:45Z

✅ E2E Happy path tests succeed 🎉

See Details

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

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

amisevsk requested a review from sleshchenko September 3, 2020 22:56

amisevsk requested review from ibuziuk, l0rd, metlos and nickboldt as code owners September 3, 2020 22:56

che-bot added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/enhancement A feature request - must adhere to the feature request template. labels Sep 3, 2020

sleshchenko approved these changes Sep 7, 2020

View reviewed changes

sleshchenko force-pushed the update-broker-v3.4.0 branch from 08ccea5 to 5d7a2a5 Compare September 7, 2020 06:42

sleshchenko self-requested a review September 7, 2020 07:09

sleshchenko approved these changes Sep 9, 2020

View reviewed changes

sleshchenko reviewed Sep 9, 2020

View reviewed changes

...che/workspace/infrastructure/kubernetes/wsplugins/brokerphases/BrokerEnvironmentFactory.java Outdated Show resolved Hide resolved

amisevsk force-pushed the update-broker-v3.4.0 branch from 2e47e0c to 9bf5f6f Compare September 11, 2020 00:04

amisevsk requested a review from skabashnyuk as a code owner September 11, 2020 00:04

amisevsk mentioned this pull request Sep 11, 2020

Add mergePlugins to attributes in devfile reference and fix minor issues in doc eclipse-che/che-docs#1576

Merged

6 tasks

sleshchenko approved these changes Sep 11, 2020

View reviewed changes

...org/eclipse/che/workspace/infrastructure/kubernetes/wsplugins/SidecarToolingProvisioner.java Outdated Show resolved Hide resolved

...-core-api-workspace-shared/src/main/java/org/eclipse/che/api/workspace/shared/Constants.java Outdated Show resolved Hide resolved

skabashnyuk reviewed Sep 11, 2020

View reviewed changes

...-core-api-workspace-shared/src/main/java/org/eclipse/che/api/workspace/shared/Constants.java Outdated Show resolved Hide resolved

sleshchenko mentioned this pull request Sep 11, 2020

FR: add podTemplate + runtimeClassName to Workspace in devfile #17761

Closed

skabashnyuk reviewed Sep 14, 2020

View reviewed changes

...org/eclipse/che/workspace/infrastructure/kubernetes/wsplugins/SidecarToolingProvisioner.java Outdated Show resolved Hide resolved

sleshchenko reviewed Sep 14, 2020

View reviewed changes

metlos reviewed Sep 14, 2020

View reviewed changes

This was referenced Sep 14, 2020

Update plugin brokers to v3.4.0 eclipse-che/che-operator#429

Merged

Moves images updating to update-nightly-bundle.sh. script eclipse-che/che-operator#442

Merged

amisevsk mentioned this pull request Sep 15, 2020

Update plugin brokers to v3.4.0 #17870

Merged

amisevsk force-pushed the update-broker-v3.4.0 branch from ba5c321 to 0d0bd0b Compare September 15, 2020 13:55

amisevsk mentioned this pull request Sep 15, 2020

Happy-path tests should use values in che.properties when running tests #17871

Closed

amisevsk merged commit cf7e314 into eclipse-che:master Sep 16, 2020

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 Sep 16, 2020

che-bot added this to the 7.19 milestone Sep 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update plugin brokers to v3.4.0 and enable plugin merging #17785

Update plugin brokers to v3.4.0 and enable plugin merging #17785

amisevsk commented Sep 3, 2020 •

edited

Loading

amisevsk commented Sep 3, 2020

che-bot commented Sep 3, 2020

sleshchenko commented Sep 7, 2020

che-bot commented Sep 7, 2020

sleshchenko commented Sep 7, 2020

amisevsk commented Sep 8, 2020

che-bot commented Sep 8, 2020

sleshchenko commented Sep 9, 2020

sleshchenko left a comment

che-bot commented Sep 9, 2020

amisevsk commented Sep 11, 2020

che-bot commented Sep 11, 2020

amisevsk commented Sep 11, 2020

sleshchenko left a comment

sleshchenko Sep 14, 2020

sleshchenko Sep 14, 2020

amisevsk Sep 14, 2020

amisevsk Sep 14, 2020

sleshchenko Sep 15, 2020

metlos left a comment

metlos Sep 14, 2020

amisevsk Sep 14, 2020

che-bot commented Sep 14, 2020

amisevsk commented Sep 15, 2020

che-bot commented Sep 15, 2020

amisevsk commented Sep 15, 2020

che-bot commented Sep 15, 2020

amisevsk commented Sep 15, 2020

che-bot commented Sep 15, 2020

	* set/read from {@link WorkspaceConfig#getAttributes}.
	* set/read from {@link Devfile#getAttributes}.

Update plugin brokers to v3.4.0 and enable plugin merging #17785

Update plugin brokers to v3.4.0 and enable plugin merging #17785

Conversation

amisevsk commented Sep 3, 2020 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

Release Notes

Docs PR

amisevsk commented Sep 3, 2020

che-bot commented Sep 3, 2020

sleshchenko commented Sep 7, 2020

che-bot commented Sep 7, 2020

sleshchenko commented Sep 7, 2020

amisevsk commented Sep 8, 2020

che-bot commented Sep 8, 2020

sleshchenko commented Sep 9, 2020

sleshchenko left a comment

Choose a reason for hiding this comment

che-bot commented Sep 9, 2020

amisevsk commented Sep 11, 2020

che-bot commented Sep 11, 2020

amisevsk commented Sep 11, 2020

sleshchenko left a comment

Choose a reason for hiding this comment

sleshchenko Sep 14, 2020

Choose a reason for hiding this comment

sleshchenko Sep 14, 2020

Choose a reason for hiding this comment

amisevsk Sep 14, 2020

Choose a reason for hiding this comment

amisevsk Sep 14, 2020

Choose a reason for hiding this comment

sleshchenko Sep 15, 2020

Choose a reason for hiding this comment

metlos left a comment

Choose a reason for hiding this comment

metlos Sep 14, 2020

Choose a reason for hiding this comment

amisevsk Sep 14, 2020

Choose a reason for hiding this comment

che-bot commented Sep 14, 2020

amisevsk commented Sep 15, 2020

che-bot commented Sep 15, 2020

amisevsk commented Sep 15, 2020

che-bot commented Sep 15, 2020

amisevsk commented Sep 15, 2020

che-bot commented Sep 15, 2020

amisevsk commented Sep 3, 2020 •

edited

Loading