🚧 Propagate default ephemeral mode value #15713

sleshchenko · 2020-01-16T09:18:20Z

What does this PR do?

🚧 It's suspended in favor of #15775
In the scope of issue about configurable Dashboard #15462, the default value of ephemeral mode should be configurable as well.
But Che Server already contains hard-coded behavior for persistVolumes (it's true), that can be overridden with the corresponding attribute on workspace level.
It makes more sense to make Dashboard consume Che Server default instead of configuring a separate default for it.
So, this PR makes Che Server propagating default ephemeral mode value.

Open question: to avoid reconfiguring existing workspaces, Dashboard must set default value explicitly (even if the user does not click on the toggle at all)

What issues does this PR fix or reference?

#15462

Signed-off-by: Sergii Leshchenko <[email protected]>

centos-ci · 2020-01-16T09:18:22Z

Can one of the admins verify this patch?

...-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceService.java

sparkoo · 2020-01-16T09:38:04Z

To comment your question, I definitely vote for setting the value explicitly from the dashboard. If user does not click on the toggle, there is still some value set, so we can't get into some undefined state here. And as you said, we avoid some hidden unexpected behavior and possible inconsistencies between dashboard and server.

che-bot · 2020-01-16T09:43:24Z

✅ E2E Happy path tests succeed 🎉

See Details

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

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

che-bot · 2020-01-16T10:09:41Z

E2E tests of Eclipse Che Multiuser on OCP has failed:

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

skabashnyuk · 2020-01-16T10:46:34Z

vote for setting the value explicitly

...-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceService.java

Signed-off-by: Sergii Leshchenko <[email protected]>

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

Signed-off-by: Sergii Leshchenko <[email protected]>

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

che-bot · 2020-01-21T13:45:09Z

✅ E2E Happy path tests succeed 🎉

See Details

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

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

che-bot · 2020-01-21T13:46:30Z

✅ E2E Happy path tests succeed 🎉

See Details

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

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

che-bot · 2020-01-21T14:03:19Z

✅ E2E Happy path tests succeed 🎉

See Details

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

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

che-bot · 2020-01-21T14:38:05Z

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

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

amisevsk · 2020-01-21T15:07:23Z

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

+  public boolean isEphemeral(Map<String, String> workspaceAttributes) {
+    String persistVolumes = workspaceAttributes.get(PERSIST_VOLUMES_ATTRIBUTE);
+    if (persistVolumes == null) {
+      return !defaultPersistVolumes;


I'm not sure this is the logic we want to go with -- I'd propose

If persistVolumes is not present, assumed default value is true.

Only use ephemeral mode when persistVolumes is explicitly false.

This avoids the need to set it explicitly in all devfiles, and avoids breaking existing workspaces by disconnecting them from existing PVCs.

On the workspace creation side (e.g. in the dashboard), this would be handled by explicitly setting persistVolumes: false if necessary, and leaving it out of the devfile otherwise.

Put another way: if

che.workspace.persist_volumes.default=false, any workspaces created will additionally have persistVolumes: false in their devfiles upon creation.

che.workspace.persist_volumes.default=true, workspaces don't get an added persistVolumes field.

I wanted to give an ability for Che Admin to configure default behavior of persisting volumes...
AFAIU you propose to keep the current Che Server logic where explicit value is used or volumes are persisted by default and only propagate default value to be consumed by clients like Dashboard.
I agree that it's safer in the perspective of existing workspaces without explicit values, where users do not expect changing behavior.
On another hand, Che Admin configured value won't be set to all workspaces by default, even new if workspaces are created with another client but not Dashboard - like chectl, or factory(if the value is not set explicitly in devfile).
I'm OK with the proposed alternative way since it's a lighter way that does not change the current Che Server behavior and at the same time allows us to configure Dashboard behavior via Che Server configuration.

I believe we need to reevaluate this pr.

metlos · 2020-01-23T11:52:54Z

Being late to the discussion as usual - I think the che server admin should have the ability to force all the workspaces to be ephemeral. Consider a "playground" che installation where really nothing should ever be stored because it is just for "fooling around".

But maybe that would better be done differently than providing a default value for an ephemeral mode. Maybe we could have a permission to create (non-)ephemeral workspaces that admin could grant to users?

amisevsk · 2020-01-23T14:39:22Z

@metlos I think #15775 supersedes this PR. On the topic of allowing an admin to force ephemeral mode, I think it creates too many edge cases to be something we want to support:

If an admin turns the feature on, all data in existing workspaces is lost
If the admin turns the feature off, workspace configs change in an unpredictable way and may cause issues that are very hard to diagnose
If the default setting is decided by an property and not devfiles, we potentially leak data in persistent volumes if they aren't properly cleaned.
Forcing the option makes it impossible to debug some issues unless you have admin access, since you would only be able to create ephemeral workspaces.

The difference between ephemeral and non-ephemeral is significant enough that I think it should be stored explicitly in devfiles.

sleshchenko · 2020-01-24T10:15:27Z

The alternative solution is already merged in #15775

Propagate default ephemeral mode value

8b77f7f

Signed-off-by: Sergii Leshchenko <[email protected]>

sleshchenko requested review from metlos, sparkoo, skabashnyuk and mshaposhnik January 16, 2020 09:18

sleshchenko requested review from l0rd, nickboldt and rhopp as code owners January 16, 2020 09:18

sleshchenko self-assigned this Jan 16, 2020

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/task Internal things, technical debt, and to-do tasks to be performed. labels Jan 16, 2020

sparkoo reviewed Jan 16, 2020

View reviewed changes

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

skabashnyuk previously approved these changes Jan 16, 2020

View reviewed changes

amisevsk reviewed Jan 16, 2020

View reviewed changes

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

sleshchenko changed the title ~~Propagate default ephemeral mode value~~ WIP Propagate default ephemeral mode value Jan 16, 2020

sleshchenko changed the title ~~WIP Propagate default ephemeral mode value~~ [WIP] Propagate default ephemeral mode value Jan 16, 2020

akurinnoy mentioned this pull request Jan 17, 2020

User Dashboard should be configurable enough for reusing in Hosted Che #15462

Closed

6 tasks

sleshchenko requested a review from ibuziuk as a code owner January 21, 2020 13:18

fixup! Propagate default ephemeral mode value

03e6439

Signed-off-by: Sergii Leshchenko <[email protected]>

sleshchenko force-pushed the propagateDefaultPersistVolumeBehaviour branch from 956d17c to 03e6439 Compare January 21, 2020 13:19

sleshchenko commented Jan 21, 2020

View reviewed changes

assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/che/che.properties Outdated Show resolved Hide resolved

fixup! Propagate default ephemeral mode value

e9f4951

Signed-off-by: Sergii Leshchenko <[email protected]>

sleshchenko commented Jan 21, 2020

View reviewed changes

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

sleshchenko changed the title ~~[WIP] Propagate default ephemeral mode value~~ Propagate default ephemeral mode value Jan 21, 2020

amisevsk reviewed Jan 21, 2020

View reviewed changes

sleshchenko mentioned this pull request Jan 22, 2020

Propagate default value for persistVolumes for clients #15775

Merged

sleshchenko changed the title ~~Propagate default ephemeral mode value~~ 🚧 Propagate default ephemeral mode value Jan 22, 2020

sleshchenko closed this Jan 24, 2020

sleshchenko deleted the propagateDefaultPersistVolumeBehaviour branch January 24, 2020 10:15

sleshchenko mentioned this pull request Jun 17, 2020

Align Get Started and Create Custom Workspace pages in propagating persistVolumes attribute #17170

Closed

22 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🚧 Propagate default ephemeral mode value #15713

🚧 Propagate default ephemeral mode value #15713

sleshchenko commented Jan 16, 2020 •

edited

Loading

centos-ci commented Jan 16, 2020

sparkoo commented Jan 16, 2020

che-bot commented Jan 16, 2020

che-bot commented Jan 16, 2020

skabashnyuk commented Jan 16, 2020

che-bot commented Jan 21, 2020

che-bot commented Jan 21, 2020

che-bot commented Jan 21, 2020

che-bot commented Jan 21, 2020

amisevsk Jan 21, 2020

amisevsk Jan 21, 2020

sleshchenko Jan 22, 2020

metlos commented Jan 23, 2020

amisevsk commented Jan 23, 2020 •

edited

Loading

sleshchenko commented Jan 24, 2020

🚧 Propagate default ephemeral mode value #15713

🚧 Propagate default ephemeral mode value #15713

Conversation

sleshchenko commented Jan 16, 2020 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

centos-ci commented Jan 16, 2020

sparkoo commented Jan 16, 2020

che-bot commented Jan 16, 2020

che-bot commented Jan 16, 2020

skabashnyuk commented Jan 16, 2020

che-bot commented Jan 21, 2020

che-bot commented Jan 21, 2020

che-bot commented Jan 21, 2020

che-bot commented Jan 21, 2020

amisevsk Jan 21, 2020

Choose a reason for hiding this comment

amisevsk Jan 21, 2020

Choose a reason for hiding this comment

sleshchenko Jan 22, 2020

Choose a reason for hiding this comment

metlos commented Jan 23, 2020

amisevsk commented Jan 23, 2020 • edited Loading

sleshchenko commented Jan 24, 2020

sleshchenko commented Jan 16, 2020 •

edited

Loading

amisevsk commented Jan 23, 2020 •

edited

Loading