[per-workspace PVC strategy] - When deleting workspace, PVC is not deleted, nor cleaned up.

[rhopp]

Description

When CRW is deployed with "per-workspace" PVC strategy and workspace is deleted, I guess the corresponding PVC should be deleted also, but this isn't happening.

Downstream issue: https://issues.jboss.org/browse/CRW-83

Reproduction Steps

1. Have CRW with "per-workspace" PVC strategy.
2. Create workspace
3. Delete workspace
   EXPECTED: PVC of this workspace is deleted.
   ACTUAL: It is not.

OS and version:

Diagnostics:
Nothing in the logs.

[sleshchenko]

I think it should be already fixed after next 6.18.0 release. The corresponding commit dda3bf2

@rhopp Do you think manually checking of Che Server built from the master branch is enough or it is needed to check it on CRW assembly?

[rhopp]

@sleshchenko Great to hear the fix is already in place. In case of CRW respin, we will probably have to cherry-pick that commit to 6.17.x branch.

[sleshchenko]

Checked locally on Multi-User Che deployed on minishift. PVC created for workspace is removed. Files on minishift host are not removed

[root@minishift openshift.local.pv]# tree pv0090
pv0090
`-- workspacej64bcwxvlx1xnqkj
    |-- che-logs
    |   |-- che-plugin-broker
    |   |   |-- eclipse-che-init-plugin-broker-v0-7-1
    |   |   `-- eclipse-che-plugin-broker-v0-7-0
    |   `-- ws
    |       |-- che-machine-exec
    |       |-- dev
    |       |-- dev2
    |       `-- theia-ide
    |-- plugins
    |-- projects
    `-- userdata
        `-- user
            `-- data
                `-- hello.txt

15 directories, 1 file

But Che Server should remove PVC but not files in such configuration (per-workspace PVC). And minishift failed to recycle PVC

[rhopp]

@sleshchenko You tried that with 6.18.0, right? We need to check that cherrypicked to 6.17.x branch.

We pretty much don't care if minishift is capable of recycling PVCs ;-)

[sleshchenko]

You tried that with 6.18.0, right?

@rhopp Yeah, 6.18.0-SNAPSHOT where the mentioned commit is present
So, please close this issue after check. Note that fix will work only for new workspaces but not existing ones. Do you think it is an issue and the corresponding fix for existing workspaces should be prepared?

[sleshchenko]

@rhopp It won't help for existing workspaces because Che Server doesn't update existing PVCs where workspace id label is missing.
One more issue that may be caused by it (fact that Che Server doesn't update existing PVCs): changes of che.infra.kubernetes.pvc.quantity or che.infra.kubernetes.pvc.access_mode will affect only new workspaces. But I'm not sure if K8s/OpenShift allows updating these fields in bound PVCs at all. Need to check.

[riuvshin]

@rhopp I will start bugfix release once this issue will be closed.

[rhopp]

Closing after discussion with @sleshchenko. The fix (for new workspaces) is already in master. We don't need this one fixed for an "upgrade" path (for existing workspaces)

[sleshchenko]

Well, it is not possible to update AccessMode of existing PVCs.
But it is possible to update metadata (like fill in missing workspace id label) and requested resources configured with https://github.com/eclipse/che/blob/076b06ab174eaeffb08c11f711bf0c6c42255d5d/assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/che/che.properties#L429

So, not sure if we need create an issue for that but PVC strategies may be improved to:

• make sure that PVCs#metadata is up to date with values that Che Server initially adds;
• make sure that PVC#spec#resources#requests#storage is up to date according to value in che.properties
• propagate/log warning if existing PVC access mode is different from configured one
  https://github.com/eclipse/che/blob/076b06ab174eaeffb08c11f711bf0c6c42255d5d/assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/che/che.properties#L443