update Che CRD for DWO per-user strategy storage configuration #1442

AObuchow · 2022-07-11T16:54:14Z

What does this PR do?

This PR updates the Eclipse Che CRD regarding the workspace storage to align it with the discussion held in eclipse-che/che#21405 (comment)

The previously named field pvc has been renamed to perUserStrategyPvcConfig to make it clear that these PVC settings only are in effect when using the "per-user" (or "common") PVC strategy.

Screenshot/screencast of this PR

n/a

What issues does this PR fix or reference?

eclipse-che/che#21405

How to test this PR?

There is not much to test for this PR as it is essentially just renaming the CRD field CheClusterDevEnvironments.Storage.Pvc -> CheClusterDevEnvironments.Storage.PerUserStrategyPvcConfig.

However, this field is being used in the automated tests, so running the tests is recommended, e.g. make test

PR Checklist

Reviewers

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

openshift-ci · 2022-07-11T16:54:27Z

Hi @AObuchow. Thanks for your PR.

I'm waiting for a eclipse-che 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.

tolusha · 2022-07-12T12:06:19Z

Sounds good. Could you also update [1]:

	// +kubebuilder:default:="per-user"
	// +kubebuilder:validation:Enum=common;per-workspace;per-user

[1]

che-operator/api/v2/checluster_types.go

Lines 363 to 364 in 885d623

    
           // +kubebuilder:default:="common" 
        
           // +kubebuilder:validation:Enum=common;per-workspace

api/v2/checluster_types.go

tolusha · 2022-07-12T17:50:34Z

Pls run make update-dev-resources after updating checluster_types.go

codecov · 2022-07-12T17:53:08Z

Codecov Report

Merging #1442 (8356598) into main (35bd691) will decrease coverage by 0.03%.
The diff coverage is 0.00%.

❗ Current head 8356598 differs from pull request most recent head 3650ba7. Consider uploading reports for the commit 3650ba7 to get more accurate results

@@            Coverage Diff             @@
##             main    #1442      +/-   ##
==========================================
- Coverage   60.41%   60.38%   -0.04%     
==========================================
  Files          74       74              
  Lines        6286     6247      -39     
==========================================
- Hits         3798     3772      -26     
+ Misses       2118     2107      -11     
+ Partials      370      368       -2

Impacted Files	Coverage Δ
api/v2/checluster_types.go	`35.00% <ø> (ø)`
api/v2/zz_generated.deepcopy.go	`0.00% <0.00%> (ø)`
pkg/deploy/server/server_configmap.go	`70.25% <0.00%> (ø)`
controllers/usernamespace/controller.go	`61.63% <0.00%> (-2.06%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 35bd691...3650ba7. Read the comment docs.

tolusha · 2022-07-13T07:20:45Z

Pls, sign ECA https://accounts.eclipse.org/user/login?destination=legal/eca/validation/112175

AObuchow · 2022-07-13T14:58:29Z

Pls, sign ECA accounts.eclipse.org/user/login?destination=legal/eca/validation/112175

I just updated my existing Eclipse.org account to use my Red Hat email again (it was previously set up to my personal email). The update will take a bit of time since I have changed employers and the legal team must reply. Will follow up once this update is completed.

tolusha · 2022-07-13T15:11:44Z

Hi.

Remove bin directory of the che-operator project. It seems contains incorrect controller-gen tools.

 - controller-gen.kubebuilder.io/version: v0.7.0
 + controller-gen.kubebuilder.io/version: v0.4.1

Pls update default values here as well

che-operator/api/v2/checluster_types.go

Line 40 in 885d623

// +kubebuilder:default:={storage: {pvcStrategy: common}, defaultNamespace: {template: <username>-che}}

che-operator/api/v2/checluster_types.go

Line 66 in 885d623

// +kubebuilder:default:={pvcStrategy: common}
Update dev resources

AObuchow · 2022-07-13T15:29:18Z

bundle/next/eclipse-che-preview-openshift/manifests/org.eclipse.che_checlusters.yaml

@@ -14,7 +14,7 @@ apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
  annotations:
-    controller-gen.kubebuilder.io/version: v0.4.1
+    controller-gen.kubebuilder.io/version: v0.7.0


Hm I deleted the bin directory and this still occurred after running make update-dev-resources

I may be wrong, but after a quick check, it seems the makefile is currently set up to download controller-gen@v0.7.0. I can locally update it to 0.4.1?

No, I've checked PR and it looks good now.

Sounds great :) Once the ECA situation is resolved, I'll squash my fix up commits for final PR approval

AObuchow · 2022-07-14T15:39:07Z

@tolusha Can you approve running the CI workflow please? I rebased my patch. Once the workflows succeed, I will squash the fixup commits. Thank you :)

tolusha

Let's wait for PR checks

tolusha · 2022-07-14T16:19:11Z

che-server fails with

Learn more:
  https://github.com/google/guice/wiki/ERROR_INJECTING_CONSTRUCTOR
Caused by: IllegalArgumentException: Unsupported PVC strategy 'per-user' configured
	at WorkspaceVolumeStrategyProvider.<init>(WorkspaceVolumeStrategyProvider.java:41)

PR against che-server must be merged first.

l0rd · 2022-08-19T11:17:05Z

api/v2/checluster_types.go

@@ -366,15 +366,16 @@ type TrustedCerts struct {

 // Configuration settings related to the workspaces persistent storage.
 type WorkspaceStorage struct {
-	// PVC settings.
+	// PVC settings when using the "per-user" PVC strategy.


I don't think that's only for per-user (i.e. the pvcStrategy is configured in this same struct).

And I would make it clear that those settings are about the Development environments PVCs.

Ok sorry my bad, the comment above is for the perUserStrategyPvcConfig. The diff confused me. Don't consider my comments above.

l0rd

LGTM

openshift-ci · 2022-08-19T11:24:51Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Needs approval from an approver in each of these files:

OWNERS

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

l0rd · 2022-08-19T11:32:32Z

LGTM 👍 @AObuchow my understanding that the next step would be adding perWorkspaceStrategyPvcConfig, right?

e.g. the final property set is expected to be the following
devEnvironments:
  storage:
    pvcStrategy: <strategy>
      perUserStrategyPvcConfig:
        claimSize: <size>
        storageClass: <classname>
      perWorkspaceStrategyPvcConfig:
        claimSize: <size>
        storageClass: <classname>
cc: @l0rd could you please review?

@ibuziuk that's fine to add the perWorkspaceStrategyPvcConfig (in a separate PR) because I am sure that it will be requested. But we should not invest on the per-workspace strategy beyond that. per-workspace is a workaround to let users run multiple workspaces at the same time until we have good support for async storage. My beleif is that with async storage we won't need per-workspace strategy anymore (and faster workspace startup too).

openshift-ci · 2022-08-19T15:22:14Z

New changes are detected. LGTM label has been removed.

AObuchow · 2022-08-19T15:23:33Z

LGTM +1 @AObuchow my understanding that the next step would be adding perWorkspaceStrategyPvcConfig, right?

e.g. the final property set is expected to be the following
devEnvironments:
  storage:
    pvcStrategy: <strategy>
      perUserStrategyPvcConfig:
        claimSize: <size>
        storageClass: <classname>
      perWorkspaceStrategyPvcConfig:
        claimSize: <size>
        storageClass: <classname>
cc: @l0rd could you please review?

Sounds good @ibuziuk, I will submit a followup PR to add the equivalent configuration for the perWorkspace PVC strategy

tolusha · 2022-08-26T14:30:15Z

/retest

Signed-off-by: Andrew Obuchowicz <aobuchow@redhat.com>

Signed-off-by: Anatolii Bazko <abazko@redhat.com>

tolusha · 2022-08-29T14:31:06Z

/test v11-devworkspace-happy-path

tolusha · 2022-08-29T15:11:36Z

/retest

openshift-ci · 2022-08-29T15:34:02Z

@AObuchow: The following tests 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/v9-upgrade-stable-to-next	`3650ba7`	link	true	`/test v9-upgrade-stable-to-next`
ci/prow/v9-devworkspace-happy-path	`3650ba7`	link	true	`/test v9-devworkspace-happy-path`
ci/prow/v10-che-behind-proxy	`3650ba7`	link	true	`/test v10-che-behind-proxy`
ci/prow/v9-operator-test	`3650ba7`	link	true	`/test v9-operator-test`
ci/prow/v11-che-behind-proxy	`caab811`	link	true	`/test v11-che-behind-proxy`
ci/prow/v11-devworkspace-happy-path	`caab811`	link	true	`/test v11-devworkspace-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.

tolusha · 2022-08-29T17:53:56Z

/test v10-upgrade-stable-to-next

AObuchow requested review from nickboldt, tolusha, AndrienkoAleksandr and flacatus as code owners July 11, 2022 16:54

openshift-ci bot added the needs-ok-to-test label Jul 11, 2022

tolusha reviewed Jul 12, 2022

View reviewed changes

api/v2/checluster_types.go Outdated Show resolved Hide resolved

api/v2/checluster_types.go Outdated Show resolved Hide resolved

AObuchow force-pushed the update_che_cr_storage_api_2 branch from 0617b6d to 1ed7526 Compare July 12, 2022 15:24

AObuchow commented Jul 13, 2022

View reviewed changes

AObuchow force-pushed the update_che_cr_storage_api_2 branch from 8356598 to 3650ba7 Compare July 14, 2022 15:36

tolusha added the ok-to-test label Jul 14, 2022

tolusha approved these changes Jul 14, 2022

View reviewed changes

openshift-ci bot assigned tolusha Jul 14, 2022

openshift-ci bot added the lgtm label Jul 14, 2022

ibuziuk mentioned this pull request Jul 18, 2022

Remove che server properties that are outdated eclipse-che/che#21411

Open

AObuchow mentioned this pull request Jul 19, 2022

chore: Remove some outdated PVC storage properties eclipse-che/che-server#330

Closed

9 tasks

openshift-merge-robot added the needs-rebase label Jul 25, 2022

AObuchow force-pushed the update_che_cr_storage_api_2 branch from 3650ba7 to 8319b0c Compare August 4, 2022 20:51

openshift-ci bot removed the lgtm label Aug 4, 2022

openshift-merge-robot removed the needs-rebase label Aug 4, 2022

openshift-ci bot removed the needs-ok-to-test label Aug 4, 2022

AObuchow mentioned this pull request Aug 5, 2022

chore: Remove some outdated PVC storage properties eclipse-che/che-server#337

Merged

9 tasks

openshift-ci bot assigned ibuziuk Aug 19, 2022

openshift-ci bot added the lgtm label Aug 19, 2022

l0rd reviewed Aug 19, 2022

View reviewed changes

l0rd approved these changes Aug 19, 2022

View reviewed changes

openshift-ci bot assigned l0rd Aug 19, 2022

AObuchow force-pushed the update_che_cr_storage_api_2 branch from f938363 to 8aa10b5 Compare August 19, 2022 15:22

openshift-ci bot removed the lgtm label Aug 19, 2022

AObuchow mentioned this pull request Aug 24, 2022

Allow to configure workspaces persistent storage from Eclipse Che CR eclipse-che/che#21405

Closed

openshift-merge-robot added the needs-rebase label Aug 25, 2022

tolusha force-pushed the update_che_cr_storage_api_2 branch from 9b1de93 to d3a5697 Compare August 25, 2022 16:54

openshift-merge-robot removed the needs-rebase label Aug 25, 2022

tolusha force-pushed the update_che_cr_storage_api_2 branch from d3a5697 to 43dc048 Compare August 26, 2022 12:12

AObuchow and others added 6 commits August 29, 2022 15:58

update Che CRD for DWO per-user strategy storage configuration

3ef099b

Signed-off-by: Andrew Obuchowicz <aobuchow@redhat.com>

Generate files for Che CRD update for DWOC storage config

13c772f

Signed-off-by: Andrew Obuchowicz <aobuchow@redhat.com>

Update OLM bundles for Che CR per-user PVC config naming modification

6f23a87

Signed-off-by: Andrew Obuchowicz <aobuchow@redhat.com>

Update dev resources

77a862e

Signed-off-by: Anatolii Bazko <abazko@redhat.com>

format

847350d

Signed-off-by: Anatolii Bazko <abazko@redhat.com>

Add PerWorkspaceStrategyPvcConfig

caab811

Signed-off-by: Anatolii Bazko <abazko@redhat.com>

tolusha force-pushed the update_che_cr_storage_api_2 branch from 43dc048 to caab811 Compare August 29, 2022 12:58

tolusha merged commit aa27398 into eclipse-che:main Aug 30, 2022

che-bot added this to the 7.53 milestone Aug 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update Che CRD for DWO per-user strategy storage configuration #1442

update Che CRD for DWO per-user strategy storage configuration #1442

AObuchow commented Jul 11, 2022 •

edited

Loading

openshift-ci bot commented Jul 11, 2022

tolusha commented Jul 12, 2022

tolusha commented Jul 12, 2022

codecov bot commented Jul 12, 2022 •

edited

Loading

tolusha commented Jul 13, 2022

AObuchow commented Jul 13, 2022

tolusha commented Jul 13, 2022 •

edited

Loading

AObuchow Jul 13, 2022 •

edited

Loading

AObuchow Jul 13, 2022

tolusha Jul 13, 2022

AObuchow Jul 13, 2022

AObuchow commented Jul 14, 2022

tolusha left a comment

tolusha commented Jul 14, 2022 •

edited

Loading

l0rd Aug 19, 2022

l0rd Aug 19, 2022 •

edited

Loading

l0rd Aug 19, 2022

l0rd left a comment

openshift-ci bot commented Aug 19, 2022

l0rd commented Aug 19, 2022 •

edited

Loading

openshift-ci bot commented Aug 19, 2022

AObuchow commented Aug 19, 2022

tolusha commented Aug 26, 2022

tolusha commented Aug 29, 2022

tolusha commented Aug 29, 2022

openshift-ci bot commented Aug 29, 2022 •

edited

Loading

tolusha commented Aug 29, 2022

update Che CRD for DWO per-user strategy storage configuration #1442

update Che CRD for DWO per-user strategy storage configuration #1442

Conversation

AObuchow commented Jul 11, 2022 • 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

openshift-ci bot commented Jul 11, 2022

tolusha commented Jul 12, 2022

tolusha commented Jul 12, 2022

codecov bot commented Jul 12, 2022 • edited Loading

Codecov Report

tolusha commented Jul 13, 2022

AObuchow commented Jul 13, 2022

tolusha commented Jul 13, 2022 • edited Loading

AObuchow Jul 13, 2022 • edited Loading

Choose a reason for hiding this comment

AObuchow Jul 13, 2022

Choose a reason for hiding this comment

tolusha Jul 13, 2022

Choose a reason for hiding this comment

AObuchow Jul 13, 2022

Choose a reason for hiding this comment

AObuchow commented Jul 14, 2022

tolusha left a comment

Choose a reason for hiding this comment

tolusha commented Jul 14, 2022 • edited Loading

l0rd Aug 19, 2022

Choose a reason for hiding this comment

l0rd Aug 19, 2022 • edited Loading

Choose a reason for hiding this comment

l0rd Aug 19, 2022

Choose a reason for hiding this comment

l0rd left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Aug 19, 2022

l0rd commented Aug 19, 2022 • edited Loading

openshift-ci bot commented Aug 19, 2022

AObuchow commented Aug 19, 2022

tolusha commented Aug 26, 2022

tolusha commented Aug 29, 2022

tolusha commented Aug 29, 2022

openshift-ci bot commented Aug 29, 2022 • edited Loading

tolusha commented Aug 29, 2022

AObuchow commented Jul 11, 2022 •

edited

Loading

codecov bot commented Jul 12, 2022 •

edited

Loading

tolusha commented Jul 13, 2022 •

edited

Loading

AObuchow Jul 13, 2022 •

edited

Loading

tolusha commented Jul 14, 2022 •

edited

Loading

l0rd Aug 19, 2022 •

edited

Loading

l0rd commented Aug 19, 2022 •

edited

Loading

openshift-ci bot commented Aug 29, 2022 •

edited

Loading