Ephemeral volume for che containers

[AndrienkoAleksandr]

What does this PR do?

Add ability mark volumes in the CheContainers like persisted or ephemeral. Set up volume like persisted by default(If user in the yaml doesn't applied persistVolume value)

What issues does this PR fix or reference?

eclipse-che/che#13387
Needed for eclipse-che/che#14539

[codecov-io]

Codecov Report

Merging #74 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #74   +/-   ##
=======================================
  Coverage   67.17%   67.17%           
=======================================
  Files           6        6           
  Lines         591      591           
=======================================
  Hits          397      397           
  Misses        168      168           
  Partials       26       26

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e46b1d...406c966. Read the comment docs.

[AndrienkoAleksandr]

@amisevsk @sleshchenko Why volume /plugins should be persisted? I mean it looks strange, /plugins is persisted volume, but we have init container eclipse/che-init-plugin-broker to clean up this folder after workspace restart. Maybe it should be ephemeral and than nothing to clean? And ephemeral volume should be faster...

[sleshchenko]

@AndrienkoAleksandr The main reason why it's persisted - because of fact that Plugin Broker as a separate Pod downloads plugins binaries and they should be delivered to Workspace Pod.
Two different workspaces are not able to share emptyDir volume.

I guess there are ways to avoid necessity of using different pods, but it's another store.

[sleshchenko]

Is it ready to review? Could you please link it with the related PRs, like plugin registry(I guess model description should be updated) and Che Server.

[AndrienkoAleksandr]

Is it ready to review?

Yes, it is.

[sleshchenko]

LGTM 👍

I've left just one comment but it's more for discussing than for requiring changes.

[sleshchenko]

It's not a real recommendation but just a statement...
Common practice - false is a default value.
Like from Java Boolean.parse(String)

The {@code boolean}
     * returned represents the value {@code true} if the string argument
     * is not {@code null} and is equal, ignoring case, to the string
     * {@code "true"}. <p>

or when you run mvn clean install tests are run by default, and if you want to skip them you should specify like -Dmaven.skip.tests or -DskipTests arguments...

It makes me think maybe it makes sense to rename persistVolume to ephemeralVolume or ephemeral(maybe without volume word since it's a property of volume) with default value - false.
At the same time, on Che Server side we already have persistVolumes wsConfig attribute with default value true.

Let me clarify that I'm not against the current approach but just shared my thoughts.
Let's wait others thoughts before changing it. @amisevsk WDYT?

[amisevsk]

I'm a big +1 for keeping our terms in line with the k8s API spec. I would suggest we use emptyDir here, to more closely signify to a user that this results in the volume being an emptyDir volume. persistVolume is a Che-specific concept, so while it makes sense to Che devs it doesn't make sense to users without documentation.

Also +1 on having false be a default, in which case the default initialization in go can be used here.

[amisevsk]

With regards to persistVolume in Che and something else here, I think the difference isn't a huge issue; calling the property emptyDir here makes sense since we're describing a Kubernetes volume. On the Che side, persistVolumes describes a Che-specific setting. Maybe a better name for the Che server property would be something like useEmptyDirVolumes, but that's a different discussion.

[sleshchenko]

Ephemeral - it's something general. In theory, we can imagine that we will move to another implementation like CSI ephemeral volumes
for some cases.
emptyDir is like one of k8s implementation, but it's just an assumption maybe it would be more straight forward to go with emptyDir since in K8s docs emptyDir is not directly described as ephemeral.

[davidfestal]

+1 for ephemeral and having false as the default.

I think we should:

• Be as much general as possible in the definition of the components of a cloud workspace,
• Choose default values to enable as small yaml definitions as possible and as simple implementations as possible: in Go APIs, false would be the default for a boolean value that is omitted in Json or Yaml afaik.

[amisevsk]

Taking a look in a moment; to add to the question of why /plugins is persisted -- the broker currently does wipe the directory, but I'd like to work on making it only make necessary changes (so starting the same workspace twice doesn't require downloading all plugins again) in the near future.

[davidfestal]

As suggested by @sleshchenko I'd prefer using the ephemeral field name in the model, with false as the default value.

[AndrienkoAleksandr]

As suggested by @sleshchenko I'd prefer using the ephemeral field name in the model, with false as > the default value.

Done

[amisevsk]

LGTM

[sleshchenko]

still LGTM