Adding support of the ephemeral workspaces (using `emptyDir` volume instead of PVC) via 'persistVolumes' attribute by ibuziuk · Pull Request #11343 · eclipse-che/che

ibuziuk · 2018-09-25T09:32:20Z

What does this PR do?

Adding support of the ephemeral workspaces (using emptyDir volume instead of PVC) via mountSources attribute. Once persistVolumes attribute is set to false all workspace volumes would be created not according to the defined as emptyDirs :

    Mounts:
      /another/volume from ephemeral-z8cst7yy (rw)
      /projects from ephemeral-3euiezrk (rw)
      /test/my/volume from ephemeral-gqhpebls (rw)
      /var/run/secrets/kubernetes.io/serviceaccount from default-token-jhsmm (ro)
      /workspace_logs from ephemeral-wjc5yu37 (rw)
Conditions:
  Type           Status
  Initialized    True 
  Ready          True 
  PodScheduled   True 
Volumes:
  ephemeral-gqhpebls:
    Type:    EmptyDir (a temporary directory that shares a pod's lifetime)
    Medium:  
  ephemeral-3euiezrk:
    Type:    EmptyDir (a temporary directory that shares a pod's lifetime)
    Medium:  
  ephemeral-z8cst7yy:
    Type:    EmptyDir (a temporary directory that shares a pod's lifetime)
    Medium:  
  ephemeral-wjc5yu37:
    Type:    EmptyDir (a temporary directory that shares a pod's lifetime)
    Medium:

What issues does this PR fix or reference?

#11350
redhat-developer/rh-che#860

WIP

Release Notes

WIP

Docs PR

WIP

…tead of PVC) via 'mountSources' attribute Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>

ibuziuk · 2018-09-25T09:34:46Z

@garagatyi @sleshchenko PR is still WIP and missing tests / docs, but I just want to make sure that you are ok with the overall approach.

…PVCCleaner Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>

garagatyi

@ibuziuk Overall looks good. Would be nice to clean it a bit to simplify PR review and code support.

garagatyi · 2018-09-26T11:03:10Z

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

-        String machineName = Names.machineName(pod, container);
-        InternalMachineConfig machineConfig = k8sEnv.getMachines().get(machineName);
-        addMachineVolumes(workspaceId, subPaths, pod, container, machineConfig.getVolumes());
+    if (ephemeralWorkspaceAdapter.isEphemeral(workspaceId)) {


Changing code like that shows a big diff I would say. Can you just:

if (ephemeralWorkspaceAdapter.isEphemeral(workspaceId)) { ephemeralWorkspaceAdapter.provision(k8sEnv, identity); return; }

?

garagatyi · 2018-09-26T11:03:32Z

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

-        }
-        if (preCreateDirs && subpaths != null) {
-          pvcSubPathHelper.createDirs(workspaceId, subpaths);
+    if (!ephemeralWorkspaceAdapter.isEphemeral(workspaceId)) {


See comment above

garagatyi · 2018-09-26T11:06:25Z

...a/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleaner.java

            final String workspaceId = event.getWorkspace().getId();
            try {
-              strategy.cleanup(workspaceId);
+              if (!isEphemeral(event.getWorkspace())) {


Would be better to move this logic to the strategy to avoid splitting it between components.

yeah, good point

@garagatyi do you think it would be acceptable to change strategy interface e.g. void cleanup(Workspace workspace) throws InfrastructureException; in order to get the whole workspace config directly in strategies ?

Yes, I do. @sleshchenko WDYT?

garagatyi · 2018-09-26T11:07:28Z

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

+  }
+
+  private String getSubPath(String workspaceId, String volumeName, String machineName) {
+    // logs must be located inside the folder related to the machine because few machines can


It seems that a lot of code is inspired by strategies code. Would be nice to reuse the code if possible.

you are right, but reusing strategies code might be not really straight-forward

Then let's go with a simpler approach. Time will show whether we would need to refactor that later

…ntSources' attribute Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>

ibuziuk · 2018-09-26T18:10:06Z

ci-test

riuvshin · 2018-09-26T18:18:35Z

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

Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>

ibuziuk · 2018-09-26T18:48:35Z

ci-test

riuvshin · 2018-09-26T19:22:00Z

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

garagatyi · 2018-09-27T07:45:48Z

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

+   */
+  public boolean isEphemeral(String workspaceId) throws InternalInfrastructureException {
+    try {
+      WorkspaceImpl workspace = workspaceManager.getWorkspace(workspaceId);


We can actually avoid using WorkspaceMnaager here since we can pass WorkspaceConfig attributes here. And we can get them from Workspace object or K8s environment object. I'm not sure we really need that but would want to hear about that from @sleshchenko @ibuziuk . This doesn't block the PR.

when I tested PR on minishift for some reason K8s environment does not seem to contain WorkspaceConfig attributes, so I had to use WorkspaceManager instead

@ibuziuk I think it should be considered it as a bug because it must be there https://github.com/eclipse/che/blob/27bed82d5c57dc0c95093a1cea6043cfa47877f1/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceRuntimes.java#L740
Otherwise, WorkspaceNext won't be working on Kubernetes infra. And three weeks ago I tested it and it worked.

@sleshchenko I will re-test and provide a comment about it. AFAIK k8sEnv.getAttributes() always returned emptyMap for me
`

ok, seems I was wrong and all the config attributes are actually located in k8sEnv.getAttributes()
I will remove the usage of WorkspaceMnaager. Thanks for noticing

done, WorkspaceMnaager usage has been removed from PR

sleshchenko

In general LGTM
Please take a look my inlined comments

sleshchenko · 2018-09-27T07:47:31Z

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

+ */
+@Singleton
+public class EphemeralWorkspaceAdapter {
+  private final String EPHEMERAL_VOLUME_NAME_PREFIX = "ephemeral-che-workspace-";


static modifier is missing

sleshchenko · 2018-09-27T07:54:03Z

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

+   *     PVC strategies</a>
+   * @see <a href="https://kubernetes.io/docs/concepts/storage/volumes/#emptydir">emptyDir</a>
+   */
+  public static final String MOUNT_SOURCES_ATTRIBUTE = "mountSources";


Just interesting to know, is there similar functionality for Docker infrastructure?

Does sources mean source code here? If yes, then actually it is related not to sources only but to all workspaces volumes, like maven repository may be declared by a user. Maybe it makes sense to choose another name for this attribute that won't be source related by that will indicate non-persistent volumes.

I was not able to find anything similar for docker infra. VolumesConverter just grabs all the volumes from workspace config and adds them to container config.

yeah, this is a good point. Maybe persistVolumes would be a better option ? @l0rd @garagatyi wdyt ?

I think we don't have to support this in Docker infra. It can be implemented later in case it would be needed.

persistVolumes looks good to me

@sleshchenko attribute has been renamed to persistVolumes

sleshchenko · 2018-09-27T08:01:09Z

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

+      return isEphemeral(workspace);
+    } catch (NotFoundException | ServerException e) {
+      throw new InternalInfrastructureException(
+          "Failed to load workspace info" + e.getMessage(), e);


Your message and message from exception will be merged. Consider adding space or another separator.

sleshchenko · 2018-09-27T08:01:33Z

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

+   *     this case regardless 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
+   * @throws InternalInfrastructureException


Actually this exception can not be thrown here

…ests Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>

…aceManager. Renaming atribute to 'persistVolumes' Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>

ibuziuk · 2018-09-27T12:18:37Z

ci-test

amisevsk

Great work! Looks good to me, but I have one question:

Since the persistVolumes attribute is mutable (i.e. I can start a workspace with bound volume and then later modify the config), how does this interact with workspace deletion? What's the UX if e.g. we create a workspace with persistVolumes true, do some work, set it to false, restart the workspace?

What will happen if I delete a workspace that is ephemeral? We currently don't do anything if the clean up job fails but ideally in the future we would work to ensure the PVC is cleaned up. I suppose this is more an issue with cleanup though.

ibuziuk · 2018-09-27T14:45:37Z

@amisevsk yeah, good point. Currently there are no checks for such scenarios during cleanup. Basically, if workspace is ephmeral no pvc cleanup is performed, even if it was originally non-ephemeral. However, I believe the main usage of the ephemeral workspaces would be when the persistVolumes property is set as a config value from the very beginning, so we could probably treat the fact that pvc is not cleaned when the property is changed as an edge case for now.

sleshchenko

LGTM

sleshchenko · 2018-09-27T14:58:08Z

@amisevsk Really good point 👍
@ibuziuk

we could probably treat the fact that pvc is not cleaned when the property is changed as an edge case for now.

it's OK for me to leave it as it is for now. But:
Does it lead to performance issues if we would try to clean up volumes (in both strategies, common and unique) without check if workspace is configured as ephemeral? I suppose there will be some issue with common strategy but not with unique
Should not we create an issue that such a situation is possible?

riuvshin · 2018-09-27T15:43:01Z

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

ibuziuk · 2018-09-27T15:55:20Z

@sleshchenko I'm mostly worried about the common strategy since rm pod which mounts PVC would be still created and since the main goal of the ephemeral workspaces is to avoid volume mount problems always cleaning PVC would potentially bring more problems than benefits. But let me create a separate issue for the PVC cleanup situation.

l0rd · 2018-09-27T16:08:35Z

We should document how to configure an ephemeral workspaces in https://www.eclipse.org/che/docs/

ibuziuk · 2018-09-27T16:44:39Z

@sleshchenko @amisevsk I have created a separate issue to start the cleanup discussion - #11391
@l0rd sure, will provide a doc PR in in the short term

…nstead of PVC) via 'persistVolumes' attribute (eclipse-che#11343) che eclipse-che#11350 Adding support of the ephemeral workspaces (using emptyDir volume instead of PVC) via 'persistVolumes' attribute Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>

Adding support of the ephemeral workspaces (using emptyDir volume ins…

b2beb51

…tead of PVC) via 'mountSources' attribute Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>

ibuziuk requested review from garagatyi and sleshchenko as code owners September 25, 2018 09:32

ibuziuk added the status/in-progress This issue has been taken by an engineer and is under active development. label Sep 25, 2018

Ephemeral support for the unique strategy. Adding tests for Workspace…

d24e731

…PVCCleaner Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>

ibuziuk force-pushed the ephemeral branch from 509ed65 to d24e731 Compare September 25, 2018 11:29

garagatyi reviewed Sep 26, 2018

View reviewed changes

che eclipse-che#11350 Adding support of ephemeral workspaces via 'mou…

274c73c

…ntSources' attribute Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>

ibuziuk changed the title ~~[WIP] Adding support of the ephemeral workspaces (using emptyDir volume instead of PVC) via 'mountSources' attribute~~ Adding support of the ephemeral workspaces (using emptyDir volume instead of PVC) via 'mountSources' attribute Sep 26, 2018

benoitf added the kind/enhancement A feature request - must adhere to the feature request template. label Sep 26, 2018

che eclipse-che#11350: Adding more java docs

877555d

Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>

garagatyi reviewed Sep 27, 2018

View reviewed changes

garagatyi approved these changes Sep 27, 2018

View reviewed changes

sleshchenko reviewed Sep 27, 2018

View reviewed changes

ibuziuk added 2 commits September 27, 2018 11:38

che eclipse-che#11350 Adressing comments after review, adding basic t…

0b2a3e2

…ests Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>

che eclipse-che#11350: Using attributes from k8sEnv instead of Worksp…

dca4c7b

…aceManager. Renaming atribute to 'persistVolumes' Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>

ibuziuk changed the title ~~Adding support of the ephemeral workspaces (using emptyDir volume instead of PVC) via 'mountSources' attribute~~ Adding support of the ephemeral workspaces (using emptyDir volume instead of PVC) via 'persistVolumes' attribute Sep 27, 2018

garagatyi approved these changes Sep 27, 2018

View reviewed changes

amisevsk reviewed Sep 27, 2018

View reviewed changes

sleshchenko approved these changes Sep 27, 2018

View reviewed changes

ibuziuk merged commit 174e1ae into eclipse-che:master Sep 27, 2018

benoitf added this to the 6.12.0 milestone Sep 27, 2018

ibuziuk mentioned this pull request Sep 27, 2018

Need to support ephemeral workspaces via persistVolumes attribute #11350

Closed

amisevsk mentioned this pull request Nov 1, 2018

PVC cleanup is never performed for the ephemeral workspaces #11391

Closed

Conversation

ibuziuk commented Sep 25, 2018 • 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?

Release Notes

Docs PR

Uh oh!

ibuziuk commented Sep 25, 2018

Uh oh!

garagatyi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibuziuk commented Sep 26, 2018

Uh oh!

riuvshin commented Sep 26, 2018

Uh oh!

ibuziuk commented Sep 26, 2018

Uh oh!

riuvshin commented Sep 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sleshchenko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibuziuk commented Sep 27, 2018

Uh oh!

ibuziuk commented Sep 25, 2018 •

edited

Loading