Allow to configure workspaces persistent storage from Eclipse Che CR

ibuziuk commented 2 years ago

Is your task related to a problem? Please describe

CheCluster API v2 [1] is going to have the dedicated properties for workspace storage configuration:

devEnvironments:
  storage:
      pvcStrategy: <strategy> // 'per-user' or 'per-workspace'
      perUserStrategyPvcConfig:
        claimSize: <size> // will only be applied for 'per-user'
        storageClass: <classname> // will only be applied for 'per-user'

once specified those properties should be used as defaults for DevWorkspaces creation

[1] https://github.com/eclipse-che/che-operator/pull/1324

Describe the solution you'd like

Configure storage related properties from the Eclipse Che CR

Release Notes Text

Workspaces persistent storage options like the strategy and the size are now configurable using CheCluster CR. It's also possible to set the storage strategy to per-workspace now so that developers will be able to run multiple workspaces concurrently.

amisevsk commented 2 years ago

A few additional notes:

claimSize is configured in the DevWorkspaceOperatorConfig. However, the DWOC has separate configuration based on workspace storage-type (you can set a different default for per-workspace vs common). It's not clear which default the CheCluster config refers to
storageClass is configured in the DWOC as well.
pvcStrategy would need to be consumed by whichever component creates DevWorkspaces (e.g. the Dashboard) as DWO does not provide a way to configure the default storage type used for workspaces. Also in terms of naming, pvcStrategy may be somewhat confusing as the DevWorkspace attribute is controller.devfile.io/storage-type

AObuchow commented 2 years ago

claimSize is configured in the DevWorkspaceOperatorConfig. However, the DWOC has separate configuration based on workspace storage-type (you can set a different default for per-workspace vs common). It's not clear which default the CheCluster config refers to

This ambiguity has been confusing me a bit, too. Something that came to mind, however, is perhaps we need to use the pvcStrategy and claimSize in combination to determine which default PVC size should be set on DWO? E.g. if pvcStrategy is set to per-workspace and claimSize is set to 7Gi then the per-workspace PVC size will be set to 7Gi on the DWOC.

pvcStrategy would need to be consumed by whichever component creates DevWorkspaces (e.g. the Dashboard) as DWO does not provide a way to configure the default storage type used for workspaces. Also in terms of naming, pvcStrategy may be somewhat confusing as the DevWorkspace attribute is controller.devfile.io/storage-type

This question still holds - from what I understand, pvcStrategy should be consumed by the Dashboard to set the storage-type attribute in the workspace.

ibuziuk commented 2 years ago

@amisevsk @AObuchow folks, wdyt about adding default strategy to the DWOC as part of this issue e.g.

  workspace:
    defaultStorageStrategy: 'common| per-workspace | ephemeral'

This would allow configuring storage from the Che Cluster CR without a need of adding extra attributes on the dashboard level

amisevsk commented 2 years ago

On the DevWorkspace Operator level, we can't change the default. Currently, DWO treats an empty/blank storage type as common -- if it was possible to configure this default then we would risk orphaning workspace data on the cluster whenever it was changed. For example, if the default is changed from common to per-workspace, all existing workspaces would need to be re-provisioned with per-workspace storage, and all storage kept in the common PVC would be untracked.

In fact, the DevWorkspace Operator does not allow changing the storage type of a DevWorkspace once it has been used for this reason. Otherwise, flows like

Create common storage DevWorkspace, clone a large repository
Switch storage-type to ephemeral
Delete the DevWorkspace

would result in all the data saved in the common PVC to be left in-tact with no clear way to clean it up short of doing it manually or deleting the DevWorkspace. Eventually, this would result in the common PVC being full of old data, preventing new workspaces from functioning correctly.

ibuziuk commented 2 years ago

After some discussion, it was decided to change the CR properties to make it more straightforward and do NOT use DWOC for configuring any of the storage properties from the Eclipse Che CR, since it complicates the flow and would create a global DevWorkspace Operator configuration which in some cases might not be desired. The current plan is:

Need to make the CR more straightforward. Based on the feedback common is sometimes wrongly associated with the common PVC provisioned for all users, which is not the case. The suggested CR structure that should make the configuration less ambiguous:

devEnvironments:
  storage:
    pvcStrategy: per-user | per-workspace // `per-user` - single PVC per-user, per-workspace - sinle PVC per-workspace
    claimSize: <size> // applied only for the `per-user`
    storageClass: <classname> // applied only `per-user`

claimSize and storageClass will be relevant only for per-user strategy. (per-workspace properties like size and class can be manually configured by the cluster-admin by creating a dedicated DWOC manually)

In the case of the per-user strategy, PVC with the given storage class and size will be created by the che-server during the namespace configuration process [1]

Caveats:

for per-user strategy PVC will not be deleted once all user workspaces are deleted since it is not going to be managed by the DWO directly.
if the admin wants to configure per-workspace properties (size / class) they could achieve it by manually creating the DWOC

[1] https://github.com/eclipse-che/che-server/blob/main/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceFactory.java#L566-L572

tolusha commented 2 years ago

I don't understand how it correlates with the current CheCluster CR configuration

workspaces:
  storage:
    pvc:
      claimSize: <size>
      storageClass: <classname>
    pvcStrategy: <strategy>

l0rd commented 2 years ago

I don't understand how it correlates with the current CheCluster CR configuration
workspaces:
  storage:
    pvc:
      claimSize: <size>
      storageClass: <classname>
    pvcStrategy: <strategy>

The idea is to:

rename the PVC strategy common as per-user
apply claimSize and storageClass only if the strategy is per-user

That's why I would rather add a new field:

devEnvironments:
  storage:
    pvcStrategy: <strategy>
-    pvc:
-      claimSize: <size>
-      storageClass: <classname>
+    perUserStrategyPvcConfig:
+      claimSize: <size>
+      storageClass: <classname>

amisevsk commented 2 years ago

The issue is that since checluster v2 has released, the pvc field cannot be removed safely.

tolusha commented 2 years ago

Catalog source to test: docker.io/abazko/catalog:21405 Started subscription:

apiVersion: operators.coreos.com/v1alpha1
kind: Subscription
metadata:
  name: eclipse-che-subscription
  namespace: openshift-operators
spec:
  channel: next
  installPlanApproval: Manual
  name: eclipse-che-preview-openshift
  source: eclipse-che-custom-catalog-source
  sourceNamespace: openshift-operators
  startingCSV: eclipse-che-preview-openshift.v7.50.0-621.next

CheCluster CR:

...
  devEnvironments:
    defaultNamespace:
      template: <username>-che
    storage:
      pvc:
        claimSize: 10Gi
        storageClass: default
      pvcStrategy: common
...

After upgrading Eclipse Che to eclipse-che-preview-openshift.v7.51.0-622.next where devEnvironments.storage.pvc field is removed from a CRD, CheCluster CR looks like:

...
  devEnvironments:
    defaultNamespace:
      template: <username>-che
    storage:
      pvcStrategy: common
...

So, there are no issues with upgrading.

ibuziuk commented 2 years ago

On my end, I have verified on the dogfooding instance that once namespace is removed it will be recreated by che-server on the next login, which means that namespace configuration [1] routine will be triggered

[1] https://github.com/eclipse-che/che-server/blob/main/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceFactory.java#L566-L572

amisevsk commented 2 years ago

I've verified @tolusha's sample -- seems to work. Don't know what problem I ran into in the past :shrug:

ibuziuk commented 2 years ago

@l0rd @AObuchow @amisevsk @tolusha based on the customer requests I believe we need to make this story a blocker for 3.2 release. All the changes need to be backported to 7.52.x branch

tolusha commented 2 years ago

@AObuchow Let me know if you need any help with che-operator part.

l0rd commented 2 years ago

I think that's important to mention that there is an alternative: pre-create the PVC in the developers namespaces. This is a powerful alternative as it let the admin to carefully specify any properties of the developers' PVs.

ibuziuk commented 2 years ago

@l0rd right, but I think you can only pre-create it for per-user (common) strategy e.g. it is not possible to pre-create it for per-workspace

AObuchow commented 2 years ago

@AObuchow Let me know if you need any help with che-operator part.

@tolusha I'm not sure if this addition to the Che-Operator CRD is required for the 3.2 release, but if it is, then it might be best if you make this change as I am currently still working on the required DWO changes for PVC-configuration-through-Che, and afterwards I need to finish the che-operator patch that will make use of these changes to DWO.

I apologize for asking so late. However, if you are busy with other tasks just let me know, and I'll find some time to get it done :)

nickboldt commented 2 years ago

There is definitely still time for 3.2 respins, so if you push changes to 7.52.x branch, we'll get 'er built.

AObuchow commented 2 years ago

@olexii4 Part of this issue requires a change in the Che-Dashboard. The requirement (from my understanding) is to read the Che-Operator CR's pvcStrategy field and inject this value into the controller.devfile.io/storage-type attribute of a user's devfile when starting a workspace.

Something like:

   schemaVersion: 2.1.0
   metadata:
     name: some-devfile
+  attributes:
+    controller.devfile.io/storage-type: per-workspace
   projects:
 ...

The valid values for this attribute should currently be common, per-user and per-workspace, though I'm not sure if validation of these values is required on the dashboard end, as they are validated by the Che-Operator CRD.

tolusha commented 2 years ago

The valid values for this attribute should currently be common, per-user and per-workspace, though I'm not sure if validation of these values is required on the dashboard end, as they are validated by the Che-Operator CRD.

We don't have to validate those values on Dashboard side.

AObuchow commented 2 years ago

@olexii4 I apologize for the late notice, I realized another attribute needs to be added to devfiles for this feature to work.

The attribute is controller.devfile.io/devworkspace-config, which has the following structure:

attributes:
  controller.devfile.io/devworkspace-config:
    name: <string>
    namespace: <string>

It specifies the name and namespace of the external, che-operator-provided devworkspace-operator configuration that should be used for the workspace.

My task today will be to work on the che-operator patch that creates this devworkspace-operator configuration that the attribute references.

There are 2 ambiguities, however, @ibuziuk @l0rd:

What should be the name of the che-provided DWOC? che-DWOC?
In which namespace should this DWOC exist in? I assume the same namespace as che is deployed in. @tolusha Is there a way for the dashboard to find out which namespace che is deployed in?

ibuziuk commented 2 years ago

What should be the name of the che-provided DWOC? che-DWOC?

I would not prefix it with che- since it will be also used by Dev Spaces. Could be just devworkspace-config.yaml I think

in which namespace should this DWOC exist in?

We are going to have a single DWOC used by all users, right? in this case it makes sense to have it in the namespace where Eclipse Che CR is created.

l0rd commented 2 years ago

👍 I agree with @ibuziuk answers

tolusha commented 2 years ago

7.52.x: https://github.com/eclipse-che/che-operator/pull/1490

AObuchow commented 2 years ago

We are going to have a single DWOC used by all users, right?

Correct

in this case it makes sense to have it in the namespace where Eclipse Che CR is created.

Sounds good :)

ibuziuk commented 2 years ago

Today, after merging all the relevant PRs and testing on dogfooding 2 more issues have been created that need to be resolved before closing this story:

l0rd commented 2 years ago

Closing as completed

max-cx commented 1 year ago

Sync'd with Red Hat JIRA https://issues.redhat.com/browse/CRW-3630

devstudio-release commented 1 year ago

sync'd to Red Hat JIRA https://issues.redhat.com/browse/CRW-3854

eclipse-che / che