che #18369 Deleting the common PVC if there are no workspaces left for a given user #18713

ibuziuk · 2020-12-30T11:50:10Z

What does this PR do?

Deleting the common PVC if there are no workspaces left for a given user

What issues does this PR fix or reference?

#18369

How to test this PR?

image - quay.io/ibuziuk/che-server:che-18369

When all workspaces are removed the common PVC is also deleted
PVC is re-created once a new non-ephemeral workspace is started

PR Checklist

As the author of this Pull Request I made sure that:

Reviewers

Reviewers, please comment how you tested the PR when approving it.

sleshchenko

In general looks good but there are corner cases:

We have different namespace strategy, one of them is common predefined, like che, then multiple users use the same PVC.
Do we have a guarantee that the same PVC was not used for another purpose apart from Che?

The safe solution I see here: PVC-clean-up job apart from removing the files is able to remove PVC if there are no files left.

sleshchenko · 2020-12-30T12:01:07Z

...ipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesPersistentVolumeClaims.java

+   * Removes PVC by name.
+   *
+   * @param name the name of the PVC
+   * @throws InfrastructureException


please add a few words

there are a few words already ;-)

oh, you probably mean smth. like when any error occurs while removing for exeption

sleshchenko · 2020-12-30T12:03:38Z

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

+        Page<WorkspaceImpl> workspaces = workspaceManager.getWorkspaces(userId, false, 30, 0);
+        if (workspaces.isEmpty()) {
+          log.debug("User '{}' has no more workspaces left", userId);
+          userHasNoWorkspaces = true;


Personally I prefer early return

private boolean userHasNoWorkspaces() { String userId = getSessionUserIdOrUndefined(); if (!"undefined".equals(userId)) { try { Page<WorkspaceImpl> workspaces = workspaceManager.getWorkspaces(userId, false, 1, 0); if (workspaces.isEmpty()) { log.debug("User '{}' has no more workspaces left", userId); return true; } } catch (ServerException e) { log.error( "Unable to get the number of workspaces for user '{}'. Cause: {}", userId, e.getMessage(), e); } } return false; }

Also, I don't think you need to fetch 30 workspaces, even one should be enough.

good point regarding 30, but I preferred to use the default configuration.

good point regarding 30, but I preferred to use the default configuration.

Since it's not a constant but just a hard-coded value, there is no guarantee that it will continue to be a default.

right, but currently it is default on the service level, anyway changed to 1

I think that is unnecessary logic. userid can be che in single-user mode or real in multi-user mode. It can't be "undefined"

I suppose it may work in the wrong way when admin removes user and their workspaces.

@sleshchenko @sparkoo @metlos wdyt. Maybe we should iterate over all workspace and check if that is the last workspace in the concrete k8s namespace. Of cause, if there are no workspaces at all it's safe to delete pvc.

it should fix the issue above

Anonymus user will be "undefined" based on implementation.
Handling the case when the common PVC is used across multiple users ('workspaceNamespaceDefault' without placeholders is defined) - 8e02f78

ibuziuk · 2020-12-30T12:16:36Z

We have different namespace strategy, one of them is common predefined, like che, then multiple users use the same PVC.

I'm not aware of this strategy, could you clarify how is it possible to configure a common PVC for multiple users?

Do we have a guarantee that the same PVC was not used for another purpose apart from Che?

Well, I would expect that che.infra.kubernetes.pvc.name is used explicitly for workspaces and nothing more - docs ref Defines the settings of PVC name for che workspaces

sleshchenko · 2020-12-30T13:40:46Z

I'm not aware of this strategy, could you clarify how is it possible to configure a common PVC for multiple users?

Just deploy Che with Che Operator where workspaceNamespaceDefault has a fixed name, like workspaceNamespaceDefault(probably you also need to disable openshift oauth). It's not a production configuration but still, it's possible.

spec:
  server:
    workspaceNamespaceDefault: che

skabashnyuk · 2020-12-31T07:22:05Z

@sleshchenko @sparkoo @metlos wdyt. Maybe we should iterate over all workspace and check if that is the last workspace in the concrete k8s namespace. Of cause, if there are no workspaces at all - it's safe to delete pvc.

ibuziuk · 2020-12-31T09:38:37Z

Just deploy Che with Che Operator where workspaceNamespaceDefault has a fixed name

@sleshchenko could you clarify what is the expected flow when the property is enabled. Tested with latest Che against OCP 4.6 and this setup fails with permissions issue:

Error: Failed to start the workspace: "Failure executing: POST at: https://172.30.0.1/apis/project.openshift.io/v1/projectrequests. Message: Forbidden!Configured service account doesn't have access. Service account may have been revoked. projectrequests.project.openshift.io is forbidden: User "system:serviceaccount:eclipse-che:che" cannot create resource "projectrequests" in API group "project.openshift.io" at the cluster scope. The error may be caused by an expired token or changed password. Update Che server deployment with a new token or password."

Is it expected that cluster-admin grants manually the required permissions for che SA for a namespace specified in workspaceNamespaceDefault ?

sleshchenko · 2020-12-31T11:16:20Z

Is it expected that cluster-admin grants manually the required permissions for che SA for a namespace specified in workspaceNamespaceDefault ?

It's not expected and it used to work. @tolusha Do you remember the changes in that area?

BTW It works fine on minikube

ibuziuk · 2020-12-31T12:56:15Z

Handling the case when the common PVC is used across multiple users ('workspaceNamespaceDefault' without placeholders is defined) - 8e02f78

ibuziuk · 2020-12-31T15:26:02Z

Sonar check is failing:

Error:  Failed to execute goal org.sonarsource.scanner.maven:sonar-maven-plugin:3.7.0.1746:sonar (default-cli) on project che-server: You're not authorized to run analysis. Please contact the project administrator.

any ideas on how the permissions are expected to be granted?

skabashnyuk · 2020-12-31T21:58:29Z

Sonar check is failing:

That is ok for the fork.

tolusha · 2021-01-04T07:18:31Z

@ibuziuk @sleshchenko
It seems the case when OpenShift OAuth is not enabled and related to that issue #15493

skabashnyuk · 2021-01-04T10:00:41Z

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

+  }
+
+  private String getSessionUserIdOrUndefined() {
+    final Subject subject = EnvironmentContext.getCurrent().getSubject();


this code is not needed. You always should have a valid subject.

not following, why there is an isAnonymous() method in this case?

removed this method and improved the case when the subject is anonymous

amisevsk

I haven't had a chance to test, but I did have a question: are there any issues around timing when removing the common PVC? i.e. if I delete my last workspace and then immediately try to create a new one, will a terminating claim-che-workspace PVC block workspace creation for some length of time?

I seem to recall running into issues waiting for garbage collection on PVCs in e.g. minikube, but don't know the details further than that.

ibuziuk · 2021-01-07T10:23:13Z

That is ok for the fork.

So this is considered to be an expected setup and all the PRs would be marked as red?

skabashnyuk · 2021-01-07T13:40:23Z

So this is considered to be an expected setup and all the PRs would be marked as red?

Red, but it's not mandatory.

ibuziuk · 2021-01-07T13:44:03Z

Red, but it's not mandatory.

pretty ugly IMO ;-) Wondering how the plugin can be configured to be green for PRs (currently it is false positive)

ibuziuk · 2021-01-07T13:59:48Z

I haven't had a chance to test, but I did have a question: are there any issues around timing when removing the common PVC? i.e. if I delete my last workspace and then immediately try to create a new one, will a terminating claim-che-workspace PVC block workspace creation for some length of time?

@amisevsk yeah, this looks like a very good corner case to handle

…ces left for a given user Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>

…oss multiple users ('workspaceNamespaceDefault' without placeholders is defined) Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>

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

che-bot · 2021-01-08T10:58:19Z

❌ E2E Happy path tests failed ❗

See Details

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

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

Use comment "[crw-ci-test]" to rerun happy path E2E test.
Use comment "[crw-ci-test --rebuild]" to re-build the images and rerun happy path E2E test.

skabashnyuk · 2021-05-21T09:03:00Z

code moved to https://github.com/eclipse-che/che-server/

che-bot · 2021-05-21T09:20:25Z

❌ E2E Happy path tests failed ❗

See Details

Jenkins job
test report
logs and configs
Happy path tests DevFile
image: quay.io/crw_pr/che-server:18713

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

Use comment "[crw-ci-test]" to rerun happy path E2E test.
Use comment "[crw-ci-test --rebuild]" to re-build the images and rerun happy path E2E test.

Eclipse Che QE channel: https://mattermost.eclipse.org/eclipse/channels/eclipse-che-qe

ibuziuk · 2021-06-08T06:32:45Z

Closing in favour of eclipse-che/che-server#16

ibuziuk requested review from amisevsk, metlos, nickboldt, skabashnyuk and sleshchenko as code owners December 30, 2020 11:50

sleshchenko reviewed Dec 30, 2020

View reviewed changes

ibuziuk force-pushed the che-18369 branch from 701ffc4 to 8e594b5 Compare December 30, 2020 12:34

ibuziuk mentioned this pull request Dec 30, 2020

When all workspaces of a user are deleted, delete the PVC to free storage (common storage strategy) #18369

Closed

ibuziuk force-pushed the che-18369 branch from 8e594b5 to 8e02f78 Compare December 31, 2020 12:46

skabashnyuk reviewed Jan 4, 2021

View reviewed changes

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/enhancement A feature request - must adhere to the feature request template. labels Jan 4, 2021

amisevsk reviewed Jan 5, 2021

View reviewed changes

ibuziuk added 3 commits January 8, 2021 11:37

che eclipse-che#18369 Deleting the common PVC if there are no workspa…

e41b8f2

…ces left for a given user Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>

che eclipse-che#18369 Handling a case when the common PVC is used acr…

398dd35

…oss multiple users ('workspaceNamespaceDefault' without placeholders is defined) Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>

che eclipse-che#18369 Improving the case when the subject is anonymous

d64ef2c

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

ibuziuk force-pushed the che-18369 branch from 8e02f78 to d64ef2c Compare January 8, 2021 10:50

che-bot added the target/branch Indicates that a PR will be merged into a branch other than master. label May 21, 2021

This was referenced May 25, 2021

Che service account cannot delete PVC when Che is deployed with the operator #19867

Closed

feat: Deleting the common PVC if there are no workspaces left for a given user eclipse-che/che-server#16

Merged

dkwon17 mentioned this pull request Jun 2, 2021

docs: Add note about common PVC deletion eclipse-che/che-docs#2015

Merged

6 tasks

ibuziuk closed this Jun 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

che #18369 Deleting the common PVC if there are no workspaces left for a given user #18713

che #18369 Deleting the common PVC if there are no workspaces left for a given user #18713

ibuziuk commented Dec 30, 2020 •

edited

Loading

sleshchenko left a comment

sleshchenko Dec 30, 2020

ibuziuk Dec 30, 2020

ibuziuk Dec 30, 2020

ibuziuk Dec 30, 2020

sleshchenko Dec 30, 2020

ibuziuk Dec 30, 2020

ibuziuk Dec 30, 2020

sleshchenko Dec 30, 2020

ibuziuk Dec 30, 2020

skabashnyuk Dec 31, 2020

sleshchenko Dec 31, 2020

ibuziuk Dec 31, 2020

ibuziuk commented Dec 30, 2020 •

edited

Loading

sleshchenko commented Dec 30, 2020

skabashnyuk commented Dec 31, 2020 •

edited

Loading

ibuziuk commented Dec 31, 2020 •

edited

Loading

sleshchenko commented Dec 31, 2020 •

edited

Loading

ibuziuk commented Dec 31, 2020 •

edited

Loading

ibuziuk commented Dec 31, 2020 •

edited

Loading

skabashnyuk commented Dec 31, 2020

tolusha commented Jan 4, 2021

skabashnyuk Jan 4, 2021

ibuziuk Jan 7, 2021

ibuziuk Jan 8, 2021

amisevsk left a comment

ibuziuk commented Jan 7, 2021

skabashnyuk commented Jan 7, 2021

ibuziuk commented Jan 7, 2021

ibuziuk commented Jan 7, 2021

che-bot commented Jan 8, 2021

skabashnyuk commented May 21, 2021

che-bot commented May 21, 2021

ibuziuk commented Jun 8, 2021

che #18369 Deleting the common PVC if there are no workspaces left for a given user #18713

che #18369 Deleting the common PVC if there are no workspaces left for a given user #18713

Conversation

ibuziuk commented Dec 30, 2020 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

How to test this PR?

PR Checklist

Reviewers

sleshchenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibuziuk commented Dec 30, 2020 • edited Loading

sleshchenko commented Dec 30, 2020

skabashnyuk commented Dec 31, 2020 • edited Loading

ibuziuk commented Dec 31, 2020 • edited Loading

sleshchenko commented Dec 31, 2020 • edited Loading

ibuziuk commented Dec 31, 2020 • edited Loading

ibuziuk commented Dec 31, 2020 • edited Loading

skabashnyuk commented Dec 31, 2020

tolusha commented Jan 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amisevsk left a comment

Choose a reason for hiding this comment

ibuziuk commented Jan 7, 2021

skabashnyuk commented Jan 7, 2021

ibuziuk commented Jan 7, 2021

ibuziuk commented Jan 7, 2021

che-bot commented Jan 8, 2021

skabashnyuk commented May 21, 2021

che-bot commented May 21, 2021

ibuziuk commented Jun 8, 2021

ibuziuk commented Dec 30, 2020 •

edited

Loading

ibuziuk commented Dec 30, 2020 •

edited

Loading

skabashnyuk commented Dec 31, 2020 •

edited

Loading

ibuziuk commented Dec 31, 2020 •

edited

Loading

sleshchenko commented Dec 31, 2020 •

edited

Loading

ibuziuk commented Dec 31, 2020 •

edited

Loading

ibuziuk commented Dec 31, 2020 •

edited

Loading