feat: 'one PVC per-workspace' storage class #818

AObuchow · 2022-04-13T15:00:54Z

What does this PR do?

This PR adds a new storage class called 'perWorkspace'. When in use in the devfile, every workspace gets its own PVC.

What issues does this PR fix or reference?

Is it tested? How?

In order to enable the 'perWorkspace' storage class, the controller.devfile.io/storage-type devfile attribute must be set to per-workspace.

Eg.

spec:
  template:
    attributes:
        controller.devfile.io/storage-type: per-workspace

Manual testing:

Start up DWO (i.e. make run)
Apply the sample devfile that uses the perWorkspace storage class: kubectl apply -f ./samples/theia-next_per-workspaceStorage.yaml -n $NAMESPACE
Check K8s status (through terminal or dashboard) and ensure a PVC has been created for the workspace. The naming of the PVC should be of the form storage-[workspaceID]
Delete the workspace: kubectl delete dw theia-next -n $NAMESPACE
Ensure the PVC is also deleted shortly after (check K8s dashboard for PVC or use terminal commands)

Automated testing:
There is also a new test file, pkg/provision/storage/perWorkspaceStorage_test.go.
It is based off of pkg/provision/storage/CommonStorage_test.go with slight adaptions. It also ensures that the owner reference is set for the perWorkspace PVC, which is required for automatic PVC deletion with cluster garbage collection.

PR Checklist

E2E tests pass (when PR is ready, comment /test v8-devworkspace-operator-e2e, v8-che-happy-path to trigger)
- v8-devworkspace-operator-e2e: DevWorkspace e2e test
- v8-che-happy-path: Happy path for verification integration with Che

openshift-ci · 2022-04-13T15:01:05Z

Hi @AObuchow. Thanks for your PR.

I'm waiting for a devfile member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pkg/provision/storage/perWorkspaceStorage.go

amisevsk

First pass at review -- generally looks good!

I'll get around to deeper review and testing later.

docs/additional-configuration.adoc

pkg/common/naming.go

pkg/constants/constants.go

samples/flattened_theia-next_perWorkspaceStorage.yaml

pkg/provision/storage/shared.go

pkg/provision/storage/perWorkspaceStorage.go

pkg/provision/storage/perWorkspaceStorage_test.go

pkg/constants/constants.go

pkg/provision/storage/perWorkspaceStorage_test.go

pkg/provision/storage/perWorkspaceStorage.go

amisevsk

Mostly just minor nitpicks and polish at this point. Well done!

Tested on OpenShift 4.10 and works as expected 👍

pkg/provision/storage/perWorkspaceStorage.go

pkg/provision/storage/perWorkspaceStorage_test.go

pkg/provision/storage/shared.go

AObuchow · 2022-04-20T15:40:42Z

Mostly just minor nitpicks and polish at this point. Well done!

Tested on OpenShift 4.10 and works as expected +1

Awesome, thank you so much for the help & reviews @amisevsk 😁
I made the requested changes.

amisevsk · 2022-04-20T17:07:07Z

Well done 👍

pkg/common/naming.go

ibuziuk · 2022-04-21T10:38:21Z

pkg/common/naming.go

@@ -90,6 +90,10 @@ func PVCCleanupJobName(workspaceId string) string {
 	return fmt.Sprintf("cleanup-%s", workspaceId)
 }

+func PerWorkspacePVCName(workspaceId string) string {
+	return fmt.Sprintf("storage-%s", workspaceId)


Suggested change

return fmt.Sprintf("storage-%s", workspaceId)

return fmt.Sprintf("claim-%s", workspaceId)

ibuziuk · 2022-04-21T10:41:34Z

pkg/provision/storage/perWorkspaceStorage.go

+	// TODO: Determine the storage size that is needed by iterating through workspace volumes,
+	// adding the sizes specified and figuring out overrides/defaults


This TODO is planned to be addressed as part of #740, right?

Correct, or at the least, it might end up being a separate but related issue

ibuziuk

Great job 👍

Fix devfile#792. Signed-off-by: Andrew Obuchowicz <[email protected]>

amisevsk · 2022-04-25T13:42:45Z

/test v8-devworkspace-operator-e2e, v8-che-happy-path

AObuchow · 2022-04-25T14:47:21Z

/retest

openshift-ci · 2022-04-25T14:47:36Z

@AObuchow: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ibuziuk · 2022-04-26T09:02:00Z

/retest

ibuziuk · 2022-04-26T11:31:12Z

I believe we can merge with the failed test
@musienko-maxim do you happen to know the state of the happy path?

amisevsk

We generally treat happy path as optional; the QE folks have had their plate full with other projects and its been unhealthy for a while now.

👍 to merge

openshift-ci · 2022-04-26T14:34:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amisevsk, AObuchow, ibuziuk

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [amisevsk]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

amisevsk · 2022-04-26T17:09:09Z

/retest

openshift-ci · 2022-04-26T18:05:57Z

@AObuchow: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/v8-che-happy-path	`7b89ee5`	link	true	`/test v8-che-happy-path`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

AObuchow requested review from amisevsk and ibuziuk as code owners April 13, 2022 15:00

openshift-ci bot added the needs-ok-to-test label Apr 13, 2022

AObuchow commented Apr 13, 2022

View reviewed changes

pkg/provision/storage/perWorkspaceStorage.go Outdated Show resolved Hide resolved

amisevsk reviewed Apr 13, 2022

View reviewed changes

AObuchow force-pushed the one_pvc_per_workspace branch from 8810b3f to 54e9f04 Compare April 13, 2022 21:51

AObuchow commented Apr 13, 2022

View reviewed changes

pkg/constants/constants.go Outdated Show resolved Hide resolved

AObuchow force-pushed the one_pvc_per_workspace branch 2 times, most recently from 53c8e9b to 51e660b Compare April 14, 2022 03:54

amisevsk reviewed Apr 14, 2022

View reviewed changes

pkg/provision/storage/perWorkspaceStorage_test.go Outdated Show resolved Hide resolved

pkg/provision/storage/perWorkspaceStorage_test.go Outdated Show resolved Hide resolved

AObuchow force-pushed the one_pvc_per_workspace branch from 51e660b to ba4c27b Compare April 14, 2022 15:44

AObuchow commented Apr 14, 2022

View reviewed changes

pkg/provision/storage/perWorkspaceStorage_test.go Outdated Show resolved Hide resolved

AObuchow force-pushed the one_pvc_per_workspace branch from ba4c27b to b2ee4e4 Compare April 19, 2022 15:46

AObuchow commented Apr 19, 2022

View reviewed changes

pkg/provision/storage/perWorkspaceStorage.go Outdated Show resolved Hide resolved

AObuchow force-pushed the one_pvc_per_workspace branch from b2ee4e4 to 8d65602 Compare April 19, 2022 15:50

AObuchow requested a review from amisevsk April 19, 2022 15:55

amisevsk approved these changes Apr 19, 2022

View reviewed changes

openshift-ci bot assigned amisevsk Apr 19, 2022

openshift-ci bot added lgtm approved labels Apr 19, 2022

AObuchow force-pushed the one_pvc_per_workspace branch from 8d65602 to dd4baca Compare April 20, 2022 15:39

openshift-ci bot removed the lgtm label Apr 20, 2022

AObuchow requested a review from amisevsk April 20, 2022 15:42

amisevsk approved these changes Apr 20, 2022

View reviewed changes

openshift-ci bot added the lgtm label Apr 20, 2022

ibuziuk reviewed Apr 21, 2022

View reviewed changes

pkg/common/naming.go Show resolved Hide resolved

ibuziuk reviewed Apr 21, 2022

View reviewed changes

AObuchow force-pushed the one_pvc_per_workspace branch from dd4baca to 15b6705 Compare April 21, 2022 14:30

openshift-ci bot removed the lgtm label Apr 21, 2022

AObuchow requested a review from ibuziuk April 21, 2022 14:53

ibuziuk approved these changes Apr 22, 2022

View reviewed changes

openshift-ci bot assigned ibuziuk Apr 22, 2022

openshift-ci bot added the lgtm label Apr 22, 2022

feat: 'one PVC per-workspace' storage class

7b89ee5

Fix devfile#792. Signed-off-by: Andrew Obuchowicz <[email protected]>

AObuchow force-pushed the one_pvc_per_workspace branch from 15b6705 to 7b89ee5 Compare April 22, 2022 16:21

openshift-ci bot removed the lgtm label Apr 22, 2022

ibuziuk requested a review from musienko-maxim April 26, 2022 11:31

amisevsk approved these changes Apr 26, 2022

View reviewed changes

openshift-ci bot added the lgtm label Apr 26, 2022

amisevsk merged commit 1afbe20 into devfile:main Apr 26, 2022

tolusha mentioned this pull request Aug 19, 2022

0/4 nodes are available: 4 pod has unbound immediate PersistentVolumeClaims eclipse-che/che#21613

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: 'one PVC per-workspace' storage class #818

feat: 'one PVC per-workspace' storage class #818

AObuchow commented Apr 13, 2022 •

edited

Loading

openshift-ci bot commented Apr 13, 2022

amisevsk left a comment

amisevsk left a comment •

edited

Loading

AObuchow commented Apr 20, 2022

amisevsk commented Apr 20, 2022

ibuziuk Apr 21, 2022

ibuziuk Apr 21, 2022

AObuchow Apr 21, 2022

ibuziuk left a comment

amisevsk commented Apr 25, 2022

AObuchow commented Apr 25, 2022

openshift-ci bot commented Apr 25, 2022

ibuziuk commented Apr 26, 2022

ibuziuk commented Apr 26, 2022

amisevsk left a comment

openshift-ci bot commented Apr 26, 2022

amisevsk commented Apr 26, 2022

openshift-ci bot commented Apr 26, 2022

	return fmt.Sprintf("storage-%s", workspaceId)
	return fmt.Sprintf("claim-%s", workspaceId)

		// TODO: Determine the storage size that is needed by iterating through workspace volumes,
		// adding the sizes specified and figuring out overrides/defaults

feat: 'one PVC per-workspace' storage class #818

feat: 'one PVC per-workspace' storage class #818

Conversation

AObuchow commented Apr 13, 2022 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

Is it tested? How?

PR Checklist

openshift-ci bot commented Apr 13, 2022

amisevsk left a comment

Choose a reason for hiding this comment

amisevsk left a comment • edited Loading

Choose a reason for hiding this comment

AObuchow commented Apr 20, 2022

amisevsk commented Apr 20, 2022

ibuziuk Apr 21, 2022

Choose a reason for hiding this comment

ibuziuk Apr 21, 2022

Choose a reason for hiding this comment

AObuchow Apr 21, 2022

Choose a reason for hiding this comment

ibuziuk left a comment

Choose a reason for hiding this comment

amisevsk commented Apr 25, 2022

AObuchow commented Apr 25, 2022

openshift-ci bot commented Apr 25, 2022

ibuziuk commented Apr 26, 2022

ibuziuk commented Apr 26, 2022

amisevsk left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Apr 26, 2022

amisevsk commented Apr 26, 2022

openshift-ci bot commented Apr 26, 2022

AObuchow commented Apr 13, 2022 •

edited

Loading

amisevsk left a comment •

edited

Loading