feat: Deleting the common PVC if there are no workspaces left for a given user #16

dkwon17 · 2021-05-27T20:53:39Z

What does this PR do?

Deletes the common PVC if there are no workspaces left for a given user. This PR copies over the three commits from this PR: eclipse-che/che#18713.

Screenshot/screencast of this PR

After the three workspaces are deleted, the common PVC is deleted.

PVCs are also deleted when pvcStrategy is set to per-workspace:

If pvcStrategy is set to per-workspace, after a workspace is deleted, the corresponding PVC is deleted.

What issues does this PR fix or reference?

eclipse-che/che#18369

How to test this PR?

The image: quay.io/dkwon17/che-server:che-18369

Testing with helm installer:

minikube start --addons=ingress --vm=true --memory=8192
chectl server:deploy --installer helm -p minikube --cheimage=quay.io/dkwon17/che-server:che-18369

Testing with operator installer:
When Che is deployed with the operator, we need this PR to provide delete permissions for the Che service account: eclipse-che/che-operator#847.
My che-operator image for that PR is: quay.io/dkwon17/che-operator:19867.

minikube start --addons=ingress --vm=true --memory=8192
chectl server:deploy --installer operator -p minikube --cheimage=quay.io/dkwon17/che-server:che-18369 --che-operator-image=quay.io/dkwon17/che-operator:19867

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.

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

…e 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-05-27T20:53:42Z

Can one of the admins verify this patch?

dkwon17 · 2021-05-27T21:21:28Z

@amisevsk @ibuziuk I had trouble reproducing this possible corner case: eclipse-che/che#18713 (review).

For the common strategy, I delete the Go workspace, and immediately create a Java Maven workspace, I don't seem to experience any blocking due to PVC deletion:

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

...g/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/PerWorkspacePVCStrategy.java

...org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/CommonPVCStrategyTest.java

ibuziuk

@dkwon17 could you please follow the brand new semantic PR requirment https://github.com/zeke/semantic-pull-requests

Signed-off-by: David Kwon <dakwon@redhat.com>

...org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/CommonPVCStrategyTest.java

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

Signed-off-by: David Kwon <dakwon@redhat.com>

...org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/CommonPVCStrategyTest.java

skabashnyuk · 2021-06-02T07:05:27Z

[crw-ci-test --rebuild]

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

...org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/CommonPVCStrategyTest.java

ibuziuk

LGTM, assuming that formatting is fixed (mvn fmt:format should be run according to the PR check) + the non-relevant test is removed #16 (comment)

@dkwon17 please also provide a relevant PR to docs with a new feature description. Smth. like:

NOTE: the common PVC will be removed once all user's workspaces are deleted. PVC will be re-created once a new non-ephemeral workspace is started.

https://www.eclipse.org/che/docs/che-7/installation-guide/configuring-storage-strategies/

Signed-off-by: David Kwon <dakwon@redhat.com>

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

Signed-off-by: David Kwon <dakwon@redhat.com>

skabashnyuk · 2021-06-04T06:00:28Z

[crw-ci-test --rebuild]

che-bot · 2021-06-04T06:34:18Z

❌ 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:16

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

sparkoo

great contribution, thanks

skabashnyuk · 2021-06-04T08:19:34Z

[crw-ci-test --rebuild]

che-bot · 2021-06-04T08:40:54Z

❌ 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:16

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

metlos

Looks great, thank you!

skabashnyuk · 2021-06-04T10:43:22Z

[crw-ci-test --rebuild]

che-bot · 2021-06-04T11:21:29Z

✅ E2E Happy path tests succeed 🎉

See Details

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

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

dkwon17 · 2021-06-04T13:15:05Z

Thanks a lot, everyone. Is now a good time for me to squash all commits into one?

nickboldt · 2021-06-04T13:36:13Z

Is now a good time for me to squash all commits into one?

Unless there's a compelling reason to show the commit history, I always use 'squash and merge' to apply PRs.

Sometimes I'll manually rebase a bunch of commits from, say, 12 commits down to 2 or 3 (if the PR covers more than one interesting change) and then NOT squash-merge... but that's the exception.

TL;DR, I'd say yes, squash away!

ibuziuk and others added 4 commits May 27, 2021 13:28

che #18369 Deleting the common PVC if there are no workspaces left fo…

10957fb

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

che #18369 Handling a case when the common PVC is used across multipl…

3e3da65

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

che #18369 Improving the case when the subject is anonymous

d83e355

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

Use DeletionPropagation.BACKGROUND

962d4a5

dkwon17 requested review from amisevsk, metlos, nickboldt, skabashnyuk and sleshchenko as code owners May 27, 2021 20:53

skabashnyuk reviewed May 29, 2021

View reviewed changes

ibuziuk reviewed May 31, 2021

View reviewed changes

dkwon17 changed the title ~~che #18369 Deleting the common PVC if there are no workspaces left for a given user~~ Feat: deleting the common PVC if there are no workspaces left for a given user May 31, 2021

dkwon17 changed the title ~~Feat: deleting the common PVC if there are no workspaces left for a given user~~ feat: Deleting the common PVC if there are no workspaces left for a given user May 31, 2021

dkwon17 added 3 commits May 31, 2021 14:45

Remove @nullable

4fd7d3b

Signed-off-by: David Kwon <dakwon@redhat.com>

Throw RuntimeException

5512341

Remove 'che.infra.kubernetes.namespace.default' and placeholder check

74f2469

Signed-off-by: David Kwon <dakwon@redhat.com>

dkwon17 force-pushed the che-18369 branch from cbff2ab to 74f2469 Compare May 31, 2021 19:29

skabashnyuk reviewed Jun 1, 2021

View reviewed changes

...org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/CommonPVCStrategyTest.java Outdated Show resolved Hide resolved

...ava/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/CommonPVCStrategy.java Outdated Show resolved Hide resolved

skabashnyuk reviewed Jun 1, 2021

View reviewed changes

...ava/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/CommonPVCStrategy.java Outdated Show resolved Hide resolved

dkwon17 added 3 commits June 1, 2021 14:35

Remove lenient() from tests

9efd18c

Signed-off-by: David Kwon <dakwon@redhat.com>

Remove anonymous subject check

49ed330

Signed-off-by: David Kwon <dakwon@redhat.com>

Use InfrastructureException

f016079

Signed-off-by: David Kwon <dakwon@redhat.com>

dkwon17 force-pushed the che-18369 branch from f1945e4 to f016079 Compare June 1, 2021 19:00

dkwon17 commented Jun 1, 2021

View reviewed changes

...org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/CommonPVCStrategyTest.java Outdated Show resolved Hide resolved

metlos reviewed Jun 2, 2021

View reviewed changes

...ava/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/CommonPVCStrategy.java Outdated Show resolved Hide resolved

...org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/CommonPVCStrategyTest.java Outdated Show resolved Hide resolved

ibuziuk approved these changes Jun 2, 2021

View reviewed changes

Remove test

04d2cc1

Signed-off-by: David Kwon <dakwon@redhat.com>

dkwon17 mentioned this pull request Jun 2, 2021

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

Merged

6 tasks

sparkoo reviewed Jun 3, 2021

View reviewed changes

...ava/org/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/CommonPVCStrategy.java Outdated Show resolved Hide resolved

dkwon17 added 2 commits June 3, 2021 13:44

Remove unnecessary variable, get workspaces from AccountImpl

ec4275b

Signed-off-by: David Kwon <dakwon@redhat.com>

Create CommonPVCStrategy#isPersonalAccount()

63a04f2

Signed-off-by: David Kwon <dakwon@redhat.com>

sparkoo approved these changes Jun 4, 2021

View reviewed changes

skabashnyuk approved these changes Jun 4, 2021

View reviewed changes

metlos approved these changes Jun 4, 2021

View reviewed changes

skabashnyuk merged commit 30a8e48 into eclipse-che:main Jun 7, 2021

ibuziuk mentioned this pull request Jun 8, 2021

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

Closed

9 tasks

skabashnyuk added this to the 7.32 milestone Jun 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Deleting the common PVC if there are no workspaces left for a given user #16

feat: Deleting the common PVC if there are no workspaces left for a given user #16

dkwon17 commented May 27, 2021 •

edited

Loading

che-bot commented May 27, 2021

dkwon17 commented May 27, 2021

ibuziuk left a comment

skabashnyuk commented Jun 2, 2021

ibuziuk left a comment •

edited

Loading

skabashnyuk commented Jun 4, 2021

che-bot commented Jun 4, 2021

sparkoo left a comment

skabashnyuk commented Jun 4, 2021

che-bot commented Jun 4, 2021

metlos left a comment

skabashnyuk commented Jun 4, 2021

che-bot commented Jun 4, 2021

dkwon17 commented Jun 4, 2021 •

edited

Loading

nickboldt commented Jun 4, 2021

feat: Deleting the common PVC if there are no workspaces left for a given user #16

feat: Deleting the common PVC if there are no workspaces left for a given user #16

Conversation

dkwon17 commented May 27, 2021 • edited Loading

What does this PR do?

Screenshot/screencast of this PR

What issues does this PR fix or reference?

How to test this PR?

PR Checklist

Reviewers

che-bot commented May 27, 2021

dkwon17 commented May 27, 2021

ibuziuk left a comment

Choose a reason for hiding this comment

skabashnyuk commented Jun 2, 2021

ibuziuk left a comment • edited Loading

Choose a reason for hiding this comment

skabashnyuk commented Jun 4, 2021

che-bot commented Jun 4, 2021

sparkoo left a comment

Choose a reason for hiding this comment

skabashnyuk commented Jun 4, 2021

che-bot commented Jun 4, 2021

metlos left a comment

Choose a reason for hiding this comment

skabashnyuk commented Jun 4, 2021

che-bot commented Jun 4, 2021

dkwon17 commented Jun 4, 2021 • edited Loading

nickboldt commented Jun 4, 2021

dkwon17 commented May 27, 2021 •

edited

Loading

ibuziuk left a comment •

edited

Loading

dkwon17 commented Jun 4, 2021 •

edited

Loading