Implement support for Volume components #237

amisevsk · 2021-01-07T22:14:23Z

What does this PR do?

This PR is the first part of a (probably 3-part) series in removing remaining devfile 1.0 functionality from the controller.

It's a fairly significant departure in flow: the Component reconciler is disabled (and will be removed in the next PR). Instead we use libraries to resolve container and volume components directly. This changes how storage is provisioned for a workspace and makes it clearer where this happens (now: happens right after components are processed, as opposed to before, where it would happen during Deployment provisioning).

Note: Only flattened (i.e. no plugins, no parents) DevWorkspaces are supported as of this PR. This functionality will be re-added soon in a separate PR.

There are some TODOs left in the code, largely for tasks which are out of scope for now (they are labelled TODO#185 to reference the issue this PR closes), and TODOs on package docs for ambiguities in the devfile API spec; these reference issues in devfile/api where appropriate.

Short summary of changes

Add library functions for converting container components to k8s components (based largely off of the current adaptor code)
Add library functions for provisioning common PVC storage to a set of podAdditions based off volume components in a devfile
Add a shim library for re-generating ComponentDescriptions as required for che-rest-apis
Remove usage of our Component reconciler and disable support for plugins; use libraries instead. This means only flattened devfiles are supported for now
Add flattened samples to allow for testing (prefixed by flattened_*)
Add tests for library code (:tada: :tada: :tada:)
Fix TODO to only add storage + the storage finalizer when a workspace requires storage (we can now tell if persistent storage is needed)

The commit history should be fairly linear with self-explanatory commit messages; except for commit c6cbe19 which does some refactoring to better support kind of subtle mountSources functionality.

Next PRs will feature:

Flattening functionality via a subcontroller, to re-enable support for plugins.
Remove ComponentDescriptions from codebase, as they exist mostly to support Theia.
A "storage provisioner" abstraction to allow for other potential storage setups (unique PVC, ephemeral only, etc.)

What issues does this PR fix or reference?

Closes #185

Is it tested? How?

Tested on minikube. Since this PR disables support for unflattened devfiles, new files are provided in the samples directory for testing (they are prefixed with flattened_).

controllers/workspace/finalize.go

amisevsk · 2021-01-08T19:11:48Z

/retest

sleshchenko · 2021-01-11T10:12:19Z

locally tests fail with error Error processing devfile: devfile is not flattened

amisevsk · 2021-01-11T17:25:21Z

locally tests fail with error Error processing devfile: devfile is not flattened

Fixed in fd0b0f1 -- forgot to update e2e tests to accomodate the changes :)

Signed-off-by: Angel Misevski <[email protected]>

JPinkney

LGTM, I also tried it out and everything seemed to be working as expected for me

Add tests for library code (:tada: :tada: :tada:)

🎉 🎉 🎉 🎉 🎉 🎉

openshift-ci-robot · 2021-01-14T12:53:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amisevsk, JPinkney

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~~ [JPinkney,amisevsk]

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

sleshchenko

Nothing major is found in the code. Proceeding to testing.

pkg/library/container/mountSources.go

controllers/workspace/devworkspace_controller.go

pkg/library/storage/commonStorage.go

controllers/workspace/devworkspace_controller.go

sleshchenko · 2021-01-15T11:11:07Z

That's not really related to the current PR but I got devworkspace hanging in removing state, but if I get it, I see it's reported as Running. So, we should update state to probably something liek Terminating, before we run finalizers.
I got ^ due RBAC roles caused local changes which appeared after I executed make test 🤯

diff --git a/config/components/rbac/role.yaml b/config/components/rbac/role.yaml
@@ -22,8 +22,6 @@ rules:
   - namespaces
   verbs:
   - get
-  - list
-  - watch

amisevsk · 2021-01-15T16:29:40Z

I got ^ due RBAC roles caused local changes which appeared after I executed make test exploding_head

I ran make manifests yesterday and noticed it removed a couple of roles -- I'll look into it, that shouldn't be happening.

amisevsk · 2021-01-15T17:01:43Z

@sleshchenko It looks like the RBAC issue came from commit a7db052 -- we add a couple of lines to role.yaml but not the annotations needed to generate them. I'll fixup.

openshift-ci-robot · 2021-01-15T17:09:12Z

New changes are detected. LGTM label has been removed.

amisevsk · 2021-01-15T17:10:45Z

Commit c757a56 fixes the RBAC issue. I've also broken it out as a separate PR here: #246 ~~in case we want to merge it separately.~~ PR merged; I'll rebase this one before merging

Signed-off-by: Angel Misevski <[email protected]>

amisevsk · 2021-01-15T22:08:29Z

/test v5-devworkspaces-operator-e2e

amisevsk · 2021-01-18T23:06:13Z

/test v5-devworkspaces-operator-e2e

amisevsk · 2021-01-19T15:48:34Z

/test v5-devworkspaces-operator-e2e

amisevsk · 2021-01-19T16:26:54Z

Fixed tests and also cherry-picked commit 846689a from #240 to fix machine-exec (and web terminal)

amisevsk · 2021-01-19T16:31:38Z

/test v5-devworkspaces-operator-e2e

Move files in pkg/library to pkg/library/lifecycle to make adding more library functions cleaner. Signed-off-by: Angel Misevski <[email protected]>

Add library function GetKubeContainersFromDevfile to convert container components in a flattened devfile into k8s Container objects. Signed-off-by: Angel Misevski <[email protected]>

Add library function RewriteContainerVolumeMounts that converts containers and initContainers VolumeMounts to use a common PVC. Signed-off-by: Angel Misevski <[email protected]>

Add package pkg/library/shim which is used to reconstruct ComponentMetadata as required by che-rest-apis (which is in turn required by Che-Theia). This package is necessary as a shim between purely devfile 2.0 devworkspaces and current devworkspaces, which use devfile 1.0 concepts (plugin meta.yaml, Theia's dependence on Che server, etc.). Eventually this library should be deprecated. Signed-off-by: Angel Misevski <[email protected]>

Replace usage of Component subresource with straight conversion of devfile components to k8s objects. This commit disables support for un-flattened devfiles (i.e. devfiles that contain parents or plugins). For flattened devfiles, functionality is identical. Signed-off-by: Angel Misevski <[email protected]>

Signed-off-by: Angel Misevski <[email protected]>

- Add documentation to various methods as library functions deal with more edge cases - Split package pkg/library/container into multiple files for readability - Improve handling of mountSources to respect sourceMapping, correctly define projects volume - Improve common PVC provisioning to not add PVC if not needed Signed-off-by: Angel Misevski <[email protected]>

- Only set storage finalizer on workspace if workspace needs storage as defined by storage library - Only generate and sync PVC if workspace needs storage Signed-off-by: Angel Misevski <[email protected]>

Signed-off-by: Angel Misevski <[email protected]>

Need to make sure we only add each exposed port once on a container Signed-off-by: Angel Misevski <[email protected]>

Signed-off-by: Angel Misevski <[email protected]>

This avoids an issue where a container can mount the projects volume directly and have `mountSources: true`, which results in a duplicate mount and invalid deployment. Related to issue devfile/api#290 and how projects volume should be handled with `mountSources`. Signed-off-by: Angel Misevski <[email protected]>

Improve check for whether the volumeMount subpaths workaround is required for a particular deployment. Previously, we only checked whether any volumes were defined, which failed on OpenShift due to a secret volume being in the list. Now we check to make sure the common PVC volume is present in the deployment before applying the workaround. Signed-off-by: Angel Misevski <[email protected]>

Machine exec depends on the CHE_MACHINE_NAME plugin to find the right container to exec into. Add a function for populating env vars to the shim library. Signed-off-by: Angel Misevski <[email protected]>

amisevsk · 2021-01-20T23:21:13Z

/test v5-devworkspaces-operator-e2e

amisevsk · 2021-01-21T00:56:47Z

/retest

amisevsk · 2021-01-21T21:59:53Z

I've tested our usual flows on minikube and crc and haven't found any major defects. I'm merging this PR to unblock other issues; any problems that arise can be fixed in follow-up PRs.

amisevsk requested review from davidfestal, sleshchenko and JPinkney January 7, 2021 22:14

openshift-ci-robot added the approved label Jan 7, 2021

sleshchenko reviewed Jan 8, 2021

View reviewed changes

controllers/workspace/finalize.go Show resolved Hide resolved

amisevsk force-pushed the devfile-volumes branch from 313fa71 to 0db22b0 Compare January 11, 2021 18:00

amisevsk added a commit to amisevsk/devworkspace-operator that referenced this pull request Jan 12, 2021

TEMP: squashed commits from PR devfile#237

a3e8859

Signed-off-by: Angel Misevski <[email protected]>

amisevsk mentioned this pull request Jan 12, 2021

Plugin flattening #240

Merged

3 tasks

JPinkney approved these changes Jan 14, 2021

View reviewed changes

openshift-ci-robot assigned JPinkney Jan 14, 2021

openshift-ci-robot added the lgtm label Jan 14, 2021

sleshchenko reviewed Jan 15, 2021

View reviewed changes

openshift-ci-robot removed the lgtm label Jan 15, 2021

amisevsk force-pushed the devfile-volumes branch 2 times, most recently from 5c6e94d to 993c1bf Compare January 15, 2021 19:54

amisevsk added a commit to amisevsk/devworkspace-operator that referenced this pull request Jan 15, 2021

TEMP: squashed commits from PR devfile#237

c736d00

Signed-off-by: Angel Misevski <[email protected]>

amisevsk added a commit to amisevsk/devworkspace-operator that referenced this pull request Jan 15, 2021

TEMP: squashed commits from PR devfile#237

72ead32

Signed-off-by: Angel Misevski <[email protected]>

amisevsk added 14 commits January 20, 2021 17:43

Move lifecycle functions to separate package

553294f

Move files in pkg/library to pkg/library/lifecycle to make adding more library functions cleaner. Signed-off-by: Angel Misevski <[email protected]>

Add library functions for converting Containers

f76d79b

Add library function GetKubeContainersFromDevfile to convert container components in a flattened devfile into k8s Container objects. Signed-off-by: Angel Misevski <[email protected]>

Add library function for provisioning 'common' PVC storage

21778b9

Add library function RewriteContainerVolumeMounts that converts containers and initContainers VolumeMounts to use a common PVC. Signed-off-by: Angel Misevski <[email protected]>

Add flattened devfile samples for testing

c8e9579

Signed-off-by: Angel Misevski <[email protected]>

Update reconciler to use library to decide if storage is needed

cdec8d5

- Only set storage finalizer on workspace if workspace needs storage as defined by storage library - Only generate and sync PVC if workspace needs storage Signed-off-by: Angel Misevski <[email protected]>

Add tests for storage and container libraries

c5c0fd9

Signed-off-by: Angel Misevski <[email protected]>

Fix issue with duplicate containerPort due to webviews endpoint

e2ed92e

Need to make sure we only add each exposed port once on a container Signed-off-by: Angel Misevski <[email protected]>

Use flattened web-terminal devworkspace in e2e tests

6ed526e

Signed-off-by: Angel Misevski <[email protected]>

Add function to populate env vars required by machine-exec

ce39b26

Machine exec depends on the CHE_MACHINE_NAME plugin to find the right container to exec into. Add a function for populating env vars to the shim library. Signed-off-by: Angel Misevski <[email protected]>

amisevsk force-pushed the devfile-volumes branch from 568da95 to ce39b26 Compare January 20, 2021 23:20

amisevsk merged commit ea28204 into devfile:master Jan 21, 2021

amisevsk deleted the devfile-volumes branch January 21, 2021 22:00

amisevsk mentioned this pull request Jan 27, 2021

Remove plugin registry #252

Merged

amisevsk mentioned this pull request Mar 18, 2021

Remove Component CRD and related code #336

Merged

l0rd mentioned this pull request Jul 27, 2022

Full support for Devfile 2.x spec eclipse-che/che#17883

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement support for Volume components #237

Implement support for Volume components #237

amisevsk commented Jan 7, 2021 •

edited

Loading

amisevsk commented Jan 8, 2021

sleshchenko commented Jan 11, 2021

amisevsk commented Jan 11, 2021

JPinkney left a comment

openshift-ci-robot commented Jan 14, 2021

sleshchenko left a comment

sleshchenko commented Jan 15, 2021 •

edited

Loading

amisevsk commented Jan 15, 2021

amisevsk commented Jan 15, 2021

openshift-ci-robot commented Jan 15, 2021

amisevsk commented Jan 15, 2021 •

edited

Loading

amisevsk commented Jan 15, 2021

amisevsk commented Jan 18, 2021

amisevsk commented Jan 19, 2021

amisevsk commented Jan 19, 2021

amisevsk commented Jan 19, 2021

amisevsk commented Jan 20, 2021

amisevsk commented Jan 21, 2021

amisevsk commented Jan 21, 2021

Implement support for Volume components #237

Implement support for Volume components #237

Conversation

amisevsk commented Jan 7, 2021 • edited Loading

What does this PR do?

Short summary of changes

What issues does this PR fix or reference?

Is it tested? How?

amisevsk commented Jan 8, 2021

sleshchenko commented Jan 11, 2021

amisevsk commented Jan 11, 2021

JPinkney left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Jan 14, 2021

sleshchenko left a comment

Choose a reason for hiding this comment

sleshchenko commented Jan 15, 2021 • edited Loading

amisevsk commented Jan 15, 2021

amisevsk commented Jan 15, 2021

openshift-ci-robot commented Jan 15, 2021

amisevsk commented Jan 15, 2021 • edited Loading

amisevsk commented Jan 15, 2021

amisevsk commented Jan 18, 2021

amisevsk commented Jan 19, 2021

amisevsk commented Jan 19, 2021

amisevsk commented Jan 19, 2021

amisevsk commented Jan 20, 2021

amisevsk commented Jan 21, 2021

amisevsk commented Jan 21, 2021

amisevsk commented Jan 7, 2021 •

edited

Loading

sleshchenko commented Jan 15, 2021 •

edited

Loading

amisevsk commented Jan 15, 2021 •

edited

Loading