Configurable default PVC size for common and per-workspace storage classes #827

AObuchow · 2022-04-26T14:16:29Z

What does this PR do?

Allows for (optionally) configuring the default PVC size for common and per-workspace storage classes using DevWorkspaceOperatorConfig CRD
Sets default PVC sizes for common/async and per-workspace storage classes

In order to configure the default PVC size for a storage class, a DevWorkspaceOperatorConfig CRD must be applied to the cluster with the DefaultStorageSize workspace property set. This property takes a struct where the resource.Quantity fields define the PVC sizes for common/async and per-workspace storage classes.

What issues does this PR fix or reference?

Fix #792.

Is it tested? How?

Manual testing of PVC size defaults has been done by starting up a cluster, applying a devfile that uses the controller.devfile.io/storage-type attribute set to per-workspace and verifying the newly created PVC size is set to the default size for per-workspace storage.
- Manually testing the default PVC size for common storage still needs to be done, however the "new" default is currently the same as it used to be (10Gi)
The commonStorage test has been modified to support this new change. Both the common and per-workspace storage class tests are passing with this new change.
The sync tests have been modified to support this new change.

Additional WIP notes

Setting the storage class sizes with a CRD still needs to be manually tested

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-26T14:16:40Z

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.

Details

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.

apis/controller/v1alpha1/devworkspaceoperatorconfig_types.go

pkg/config/defaults.go

amisevsk · 2022-04-26T19:58:30Z

You'll need to rebase onto main (dropping the first commit)

AObuchow · 2022-04-26T22:05:43Z

deploy/deployment/kubernetes/combined.yaml

+                      x-kubernetes-int-or-string: true
+                    description: 'StorageClassSize defines an optional map of sizes
+                      of persistent volume claims for each storage class used by DevWorkspaces.
+                      The available keys to the map are: "common", "per-workspace".


I'm not sure why these files don't have the mention of async using the common storage class PVC size that I added?

The CI is also complaining about this :P

You have to run make generate to regenerate CRDs after changing the go definition. I use

make generate manifests fmt generate_default_deployment generate_olm_bundle_yaml

to make sure all autogenerated files are in sync.

pkg/config/sync.go

pkg/provision/storage/commonStorage_test.go

pkg/config/sync.go

apis/controller/v1alpha1/devworkspaceoperatorconfig_types.go

pkg/config/sync.go

AObuchow · 2022-04-28T14:49:42Z

I opened che-server and built it on the dog fooding cluster, then checked the workspace's associated PVC usage from the OpenShift dashboard.

I also made sure that any other workspace I had (other than che-server) was deleted.

It says 545.4 MiB were used:

AObuchow · 2022-04-28T17:48:54Z

I opened che-code and built it on the dog fooding cluster.

After initial workspace setup (git clone and plugins, which were automatically setup), disk usage was 981.3MiB.
After running yarn prepare, disk usage was just under 3GiB (2.7GiB IIRC)
After running yarn build, disk usage is 3.39GiB.
Finally, I ran yarn watch and in another terminal, yarn server which brought up the disk usage to 3.49GiB

amisevsk

After some discussion with Andrew, we've decided that it makes more sense to define the default storage size configuration fields as a struct rather than map[string]string. This will allow for better documentation and easier validation (since we can rely on the k8s API).

amisevsk · 2022-04-28T20:34:21Z

I'm 👍 on setting the default per-workspace storage size to 5Gi to be on the safe side (4Gi isn't much wiggle room for a project like che-code). One of the benefits of this PR is that these defaults become configurable, so we should aim for something that always works as a default, even if it overprovisions storage in most cases.

pkg/config/sync.go

apis/controller/v1alpha1/devworkspaceoperatorconfig_types.go

pkg/config/sync.go

apis/controller/v1alpha1/devworkspaceoperatorconfig_types.go

pkg/config/sync.go

AObuchow · 2022-05-02T19:18:50Z

Not sure yet why the check sources CI is suddenly failing:

go: downloading golang.org/x/sys v0.0.0-20220502124256-b6088ccd6cba
Error: ../../../go/pkg/mod/golang.org/x/tools@v0.1.10/cmd/goimports/goimports.go:14:2: //go:build comment without // +build comment
Error: Process completed with exit code 1.

amisevsk · 2022-05-03T17:25:32Z

#831 should fix PR check

amisevsk · 2022-05-11T19:06:09Z

pkg/config/sync.go

+				config = append(config, fmt.Sprintf("workspace.defaultStorageSize.%s=%s", constants.CommonStorageClassType, Workspace.DefaultStorageSize.Common.String()))
+			}
+			if Workspace.DefaultStorageSize.PerWorkspace != nil {
+				config = append(config, fmt.Sprintf("workspace.defaultStorageSize.%s=%s", constants.PerWorkspaceStorageClassType, Workspace.DefaultStorageSize.PerWorkspace.String()))


We should hard-code the config names, this will result in logging e.g.

workspace.defaultStorageSize.per-workspace=5Gi

even though the actual config field is

workspace.defaultStorageSize.perWorkspace

Suggested change

config = append(config, fmt.Sprintf("workspace.defaultStorageSize.%s=%s", constants.CommonStorageClassType, Workspace.DefaultStorageSize.Common.String()))

}

if Workspace.DefaultStorageSize.PerWorkspace != nil {

config = append(config, fmt.Sprintf("workspace.defaultStorageSize.%s=%s", constants.PerWorkspaceStorageClassType, Workspace.DefaultStorageSize.PerWorkspace.String()))

config = append(config, fmt.Sprintf("workspace.defaultStorageSize.common=%s", Workspace.DefaultStorageSize.Common.String()))

}

if Workspace.DefaultStorageSize.PerWorkspace != nil {

config = append(config, fmt.Sprintf("workspace.defaultStorageSize.perWorkspace=%s", Workspace.DefaultStorageSize.PerWorkspace.String()))

Sounds good, I went with per-workspace instead of perWorkspace. Let me know if you rather it be perWorkspace

It should match the actual field in the config though -- the dot delimited format is meaningful here. If you try to set workspace.defaultStorageSize.per-workspace in the DevWorkspaceOperatorConfig, it will be invalid.

ibuziuk

@AObuchow could you please resolve the conficts?

AObuchow · 2022-05-18T14:59:55Z

@AObuchow could you please resolve the conficts?

Sorry about that - will be done in a moment.

ibuziuk

LGTM 👍

amisevsk · 2022-05-24T21:30:33Z

pkg/config/sync.go

+				config = append(config, fmt.Sprintf("workspace.defaultStorageSize.common=%s", Workspace.DefaultStorageSize.Common.String()))
+			}
+			if Workspace.DefaultStorageSize.PerWorkspace != nil {
+				config = append(config, fmt.Sprintf("workspace.defaultStorageSize.per-workspace=%s", Workspace.DefaultStorageSize.PerWorkspace.String()))


Suggested change

config = append(config, fmt.Sprintf("workspace.defaultStorageSize.per-workspace=%s", Workspace.DefaultStorageSize.PerWorkspace.String()))

config = append(config, fmt.Sprintf("workspace.defaultStorageSize.perWorkspace=%s", Workspace.DefaultStorageSize.PerWorkspace.String()))

Done :) Will autosquash when ready for merge

Should be good for merge assuming nothing else needs to be changed 😎

Both common and per-workspace storage class PVC sizes can be configured through CRD's Signed-off-by: Andrew Obuchowicz <aobuchow@redhat.com>

Part of devfile#740 Signed-off-by: Andrew Obuchowicz <aobuchow@redhat.com>

Fix devfile#836 Signed-off-by: Andrew Obuchowicz <aobuchow@redhat.com>

openshift-ci · 2022-05-25T17:07:28Z

[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

Details

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-05-25T18:38:04Z

/retest

openshift-ci bot added the do-not-merge/work-in-progress label Apr 26, 2022

openshift-ci bot requested a review from amisevsk April 26, 2022 14:16

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

AObuchow force-pushed the configurable_default_pvc_size branch from 00bb54c to 30d5d73 Compare April 26, 2022 14:25

amisevsk reviewed Apr 26, 2022

View reviewed changes

apis/controller/v1alpha1/devworkspaceoperatorconfig_types.go Outdated Show resolved Hide resolved

pkg/config/defaults.go Outdated Show resolved Hide resolved

AObuchow force-pushed the configurable_default_pvc_size branch from 30d5d73 to 7266ce6 Compare April 26, 2022 21:56

AObuchow commented Apr 26, 2022

View reviewed changes

pkg/config/sync.go Outdated Show resolved Hide resolved

amisevsk reviewed Apr 27, 2022

View reviewed changes

AObuchow commented Apr 27, 2022

View reviewed changes

pkg/config/sync.go Outdated Show resolved Hide resolved

amisevsk reviewed Apr 28, 2022

View reviewed changes