Ensure plugin broker works with ephemeral workspaces #11786

amisevsk · 2018-10-31T03:33:02Z

What does this PR do?

Changes necessary to make plugin broker work correctly when a workspace is marked as ephemeral. This is done by making the broker run as an init container on ephemeral workspaces, so that any volume-related actions are reflected in the workspace's emptyDir volumes.

Detailed changes:

Slight changes to EphemeralWorkspaceAdapter: 1. Make ephemeral workspace check methods static to allow them to be used elsewhere, and 2. add method to "make" a workspace ephemeral, so that all attribute management happens in one place.
Modify PVC strategies to account for init containers. For EphemeralWorkspaceAdapter this is necessary, as otherwise the /plugins directory cannot be mounted. I've also included this change for the common and unique strategies, although it doesn't do anything at the moment (since no init containers are added in the non-ephemeral case).
Apply plugin broker as init container if workspace is ephemeral

What issues does this PR fix or reference?

#11554

Release Notes

Allow Che 7 workspaces to run as ephermal (emptyDir) containers on Kubernetes/OpenShift.

l0rd · 2018-10-31T17:29:33Z

I have a couple of question with respect to the init container approach:

Will that work when plugins will run in distinct pods (c.f. Support running Che 7 plugins and runtime in separate pods #11476 and Che 7: Need to support running non-volume dependent sidecars as separate pods redhat-developer/rh-che#1005)? I mean ephemeral mode for containers running in distinct pods.
Will that work if we want some workspace containers to be loaded first than others (i.e. the editor container is deployed immediately to shorten the start up time for users and other containers are loaded in a lazy manner)?

amisevsk · 2018-11-01T00:01:53Z

@l0rd I think ephemeral workspaces will make distinct pods a difficult issue -- an emptyDir volume can only be attached to one pod. It might be complicated but with some architecture work it might be possible.

As it stands, the diagram above shows what a Che 7 workspace looks like in terms of volumes. From this diagram, to make sure access to projects is shared, theia-ide, che-dev, and any plugins (e.g. che-hello) that needs access to /projects would need to be in the same pod. Further, for the broker to work, it needs to be an init container on the theia pod, so the only plugin that could be separated out into its own pod is the che-machine-exec container.

On the plugin broker side, however, since we only need to touch the plugins volume, the broker container needs only to run as a init container on the theia (or editor) container. If e.g. /projects was RWX, then the theia container could be its own pod that starts separately, with the broker as its init container.

One thing Alex and I talked about briefly is potentially figuring out requirements for multiple brokers (e.g. a theia plugin-only broker would not need an initial run, and could run exclusively as an init container. We didn't really go far down this route though.

garagatyi · 2018-11-01T14:29:35Z

@amisevsk FYI we were also considering an ability to create plugins not only for editors but for other items, such as plugins (yeah, plugins of plugins). For example JDT.LS can have its own plugins. But we haven't finished discussions regarding that.
And I would love to remove workspace logs volume in Che 7 since it doesn't make much sense now, here is the issue.

garagatyi

Overall looks good. Would be cool to have unit tests, though.
I was thinking about the overall design and maybe we could have a code that would add broker as regular or init container depending on something. In this case, depending on the ephemeral property.
Or later, depending on some config of a broker we would add. With such an approach the code might seem cleaner. But not sure whether we need to do this in this particular PR.

garagatyi · 2018-11-02T11:19:49Z

...ava/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/CommonPVCStrategy.java

@@ -108,6 +110,9 @@ public void provision(KubernetesEnvironment k8sEnv, RuntimeIdentity identity)
    k8sEnv.getPersistentVolumeClaims().put(pvcName, pvc);
    for (Pod pod : k8sEnv.getPods().values()) {
      PodSpec podSpec = pod.getSpec();
+      List<Container> containers = new ArrayList<>();


I don't see where it is used

Good catch -- thank you. I meant to loop over this list.

garagatyi · 2018-11-02T13:22:13Z

...eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/EphemeralWorkspaceAdapter.java

@@ -61,11 +63,21 @@ public boolean isEphemeral(Workspace workspace) {
   *     of the PVC strategy, workspace volumes would be created as `emptyDir`. When a workspace Pod
   *     is removed for any reason, the data in the `emptyDir` volume is deleted forever
   */
-  public boolean isEphemeral(Map<String, String> workspaceAttributes) {
+  public static boolean isEphemeral(Map<String, String> workspaceAttributes) {


Would be better to move these static methods to a static utility class

garagatyi · 2018-11-02T13:35:50Z

@l0rd after discussing the brokering process with Angel and Serhii I'm thinking that we can do improvements described below to tackle your points.
We can set whether a broker changes workspace configuration or copies files. If it just changes configuration we don't need to add volumes to it (actually it can specify that it doesn't need any volume) and it can play with ephemeral workspaces nicely. If it just copies files it would go to the init containers and would probably be able to copy plugins without the usage of non-ephemeral volumes. And only in a case, it needs to do both steps it would be running 2 times in a case of the ephemeral workspace.
Maybe someone would also come up with other improvements for this flow.

l0rd · 2018-11-05T07:43:09Z

@garagatyi @amisevsk @skabashnyuk I will try to provide a better description of the requirements. And I think we should have a call today or tomorrow to discuss it.

amisevsk · 2018-11-06T02:07:47Z

I've addressed the review comments and added tests. I also had to rebase the PR to fix a merge conflict.

amisevsk · 2018-11-06T02:07:56Z

ci-test

riuvshin · 2018-11-06T05:46:20Z

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:11786
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

garagatyi · 2018-11-06T06:35:18Z

@eclipse/eclipse-che-qa Please, take a look at the report

artaleks9 · 2018-11-06T08:01:19Z

Selenium tests execution on Eclipse Che Multiuser on OCP (https://ci.codenvycorp.com/job/che-pullrequests-test-ocp/1139/) doesn't show any regression against this Pull Request.

Changes necessary to ensure that plugin broker can work with ephemeral workspaces, which, since they use emptyDir volumes, cannot share storage between pods (as in the separate pod that runs plugin broker). This is done by adding the plugin broker as an init container on ephemeral workspaces. Details: - Change EphemeralWorkspaceAdapter to make check methods static, add method to "make" a workspace ephemeral (to ensure how we check for ephemeral workspaces is done in one place) - Make PVC strategies include init containers in assigning volumes, to ensure broker's volumes are correctly set when running as an init container - Add KubernetesBrokerInitContainerApplier, which modifies the workspace env to include the broker as an init container. Signed-off-by: Angel Misevski <amisevsk@redhat.com>

- Add tests for SideCarToolingProvisioner, KubernetesBrokerInitContainerApplier, and EphemeralWorkspaceAdapter - Refactor static methods in EphemeralWorkspaceAdapter to their own utility class - Minor bugfixes Signed-off-by: Angel Misevski <amisevsk@redhat.com>

amisevsk · 2018-11-08T18:01:29Z

Fixed codacy's complaining and rebased onto current master.

@garagatyi @ibuziuk @sleshchenko I'd like to merge tomorrow unless there's something else that needs to be improved.

sleshchenko

LGTM
The only one bad side of current implementation - all plugin brokers are run twice and as result Plugins Binaries are downloaded twice as well, but I'm OK to merge it since I can't propose an alternative implementation with current Che 7 Model

Please take a look my minor comments

.../che/workspace/infrastructure/kubernetes/wsplugins/KubernetesBrokerInitContainerApplier.java

Signed-off-by: Angel Misevski <amisevsk@redhat.com>

amisevsk · 2018-11-09T15:42:01Z

@sleshchenko Thanks for your review. I agree that running plugin broker twice is not ideal, but also don't know if it will be possible to avoid (at least, for the plugins that require e.g. services on the workspace Pod. One way the init container may be more convenient is for plugins (e.g. theia) that don't require modifying the OpenShift object, where the init container could be sufficient.

amisevsk added kind/enhancement A feature request - must adhere to the feature request template. status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. labels Oct 31, 2018

amisevsk requested review from ibuziuk and garagatyi October 31, 2018 03:33

amisevsk requested a review from sleshchenko as a code owner October 31, 2018 03:33

amisevsk mentioned this pull request Oct 31, 2018

Che plugin broker should support ephemeral mode redhat-developer/rh-che#987

Closed

garagatyi mentioned this pull request Nov 1, 2018

Do not add workspace logs volume in Che 7 workspaces #11806

Closed

garagatyi reviewed Nov 2, 2018

View reviewed changes

amisevsk force-pushed the issue-11554 branch from 1d318b8 to e86aeb9 Compare November 6, 2018 02:07

garagatyi approved these changes Nov 6, 2018

View reviewed changes

amisevsk mentioned this pull request Nov 8, 2018

Need to compare Che 7 workspace startup time when using images from docker registry deployed in the user namespace in comparison with dockerhub / quay redhat-developer/rh-che#1014

Closed

amisevsk added 2 commits November 8, 2018 12:32

amisevsk force-pushed the issue-11554 branch from e86aeb9 to f0b8215 Compare November 8, 2018 18:00

sleshchenko approved these changes Nov 9, 2018

View reviewed changes

fixup: improve KubernetesBrokerInitContainerApplier

0c647ec

Signed-off-by: Angel Misevski <amisevsk@redhat.com>

ibuziuk approved these changes Nov 11, 2018

View reviewed changes

amisevsk merged commit 58c83a0 into eclipse-che:master Nov 12, 2018

amisevsk deleted the issue-11554 branch November 12, 2018 21:22

benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Nov 12, 2018

benoitf added this to the 6.14.0 milestone Nov 12, 2018

This was referenced Nov 13, 2018

Image pulling for predefined images should always be fast on che.openshift.io redhat-developer/rh-che#1056

Closed

Che 7 startup should be faster on che.openshift.io redhat-developer/rh-che#1011

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure plugin broker works with ephemeral workspaces #11786

Ensure plugin broker works with ephemeral workspaces #11786

amisevsk commented Oct 31, 2018

l0rd commented Oct 31, 2018

amisevsk commented Nov 1, 2018

garagatyi commented Nov 1, 2018

garagatyi left a comment

garagatyi Nov 2, 2018

amisevsk Nov 6, 2018

garagatyi Nov 2, 2018

amisevsk Nov 6, 2018

garagatyi commented Nov 2, 2018

l0rd commented Nov 5, 2018

amisevsk commented Nov 6, 2018

amisevsk commented Nov 6, 2018

riuvshin commented Nov 6, 2018

garagatyi commented Nov 6, 2018

artaleks9 commented Nov 6, 2018

amisevsk commented Nov 8, 2018

sleshchenko left a comment

amisevsk commented Nov 9, 2018

Ensure plugin broker works with ephemeral workspaces #11786

Ensure plugin broker works with ephemeral workspaces #11786

Conversation

amisevsk commented Oct 31, 2018

What does this PR do?

What issues does this PR fix or reference?

Release Notes

l0rd commented Oct 31, 2018

amisevsk commented Nov 1, 2018

garagatyi commented Nov 1, 2018

garagatyi left a comment

Choose a reason for hiding this comment

garagatyi Nov 2, 2018

Choose a reason for hiding this comment

amisevsk Nov 6, 2018

Choose a reason for hiding this comment

garagatyi Nov 2, 2018

Choose a reason for hiding this comment

amisevsk Nov 6, 2018

Choose a reason for hiding this comment

garagatyi commented Nov 2, 2018

l0rd commented Nov 5, 2018

amisevsk commented Nov 6, 2018

amisevsk commented Nov 6, 2018

riuvshin commented Nov 6, 2018

garagatyi commented Nov 6, 2018

artaleks9 commented Nov 6, 2018

amisevsk commented Nov 8, 2018

sleshchenko left a comment

Choose a reason for hiding this comment

amisevsk commented Nov 9, 2018