Add support for containerContributions

[amisevsk]

This PR is an early draft. Before merging:

• Decide on format for specifying container contributions cc: @l0rd, @benoitf
  • Is the current design sufficient? Should we define contributions in another way?
  • Does the mechanism need a way to specify which container to merge into, or is "merge into the first available target" sufficient? (This can be added later if necessary)
  • Does this approach cover all use cases for Code-OSS and Theia?
• Write tests to cover feature once design is settled
• Clean up and polish

What does this PR do?

Adds support for container contributions as described in #656 without needing to add new component types.

This PR adds two new attributes to the operator:

• controller.devfile.io/container-contribution: true defines a container component as a "container contribution"
• controller.devfile.io/merge-contribution: true defines a container as a target for merging a "container contribution"

If a flattened DevWorkspace has a container component with the merge-contribution attribute, then any container contributions are merged into that container component according to

• Resource requirements (memory/cpu limits/requests) are summed
• The image from the contribution component is ignored
• Remaining fields are merged according to a Kubernetes strategic merge patch (e.g. env vars and endpoints are appended)

This approach has the additional benefit of failing safely if it's not obvious how to merge containers, defaulting to using the unmerged image to host the container components that cannot be merged.

If multiple components have the merge-contribution attribute, contributions are merged into the first one encountered.

Background

Eclipse Che uses DWO to create workspaces from a basic devfile, injecting an editor into the Devfile before converting it to a DevWorkspace. However, since Che intends to run editors inside a devfile container component and not as sidecars, it's necessary to modify existing container components to add endpoints, environment variables, and override the args. For example, to update a container component to run Eclipse Theia plugins, Che has to update the component to add the following fields:

 components:
   - name: tools
+    attributes:
+      app.kubernetes.io/name: tools
+      che-theia.eclipse.org/vscode-extensions:
+        - https://github.com/golang/vscode-go/releases/download/v0.23.0/go-0.23.0.vsix
     container:
+      args:
+        - ${PLUGIN_REMOTE_ENDPOINT_EXECUTABLE}
+        - -c
+        - sh
       env:
         - name: GOCACHE
           value: /tmp/.cache
         - name: GOPATH
           value: /projects:/home/user/go
+        - name: PLUGIN_REMOTE_ENDPOINT_EXECUTABLE
+          value: /remote-endpoint/plugin-remote-endpoint
+        - name: THEIA_PLUGINS
+          value: local-dir:///plugins/sidecars/tools
       image: quay.io/devfile/universal-developer-image:ubi8-0e189d9
       memoryLimit: 2Gi
       mountSources: true
+      sourceMapping: /projects
+      volumeMounts:
+        - name: plugins
+          path: /plugins
+        - name: remote-endpoint
+          path: /remote-endpoint
   # Theia is added as a plugin here
+  - name: theia-ide-golang-example
+    plugin:
+      kubernetes:
+        name: theia-ide-golang-example

The added fields are defined by the plugin (theia-ide-golang-example in this case), so it would be useful if plugins had a mechanism for automatically defining these additions. This would also mean that devfiles could be converted into DevWorkspaces by only adding plugin components, leaving original devfile components untouched (as the merging happens within the flatten process, leaving the DevWorkspace as-is).

What issues does this PR fix or reference?

Closes #656

Is it tested? How?

There are two DevWorkspaces for testing this feature:

1. Che-Theia + Golang [link]

   kubectl apply -f https://gist.githubusercontent.com/amisevsk/46da73704a126d2324e5c633ff077b8c/raw/41dee8cbe9531363ffd8975982bdc1113d7b58c4/theia-golang.devworkspace.yaml
   

   Creates a DevWorkspace that references plugin devfile theia-next.yaml. The contributions in this plugin add fields as described above to allow Theia plugins to run inside the tools container component without having to update the tools component in the DevWorkspace itself (duplicating the process that Che does when creating a workspace from a devfile). The diff between the referenced plugin and the default plugin at https://che-plugin-registry-main.surge.sh/v3/plugins/eclipse/che-theia/next/devfile.yaml is

   @@ -9,6 +9,26 @@
      preStart:
        - init-container-command
    components:
   +  - name: theia-ide-contributions
   +    attributes:
   +      controller.devfile.io/container-contribution: true
   +    container:
   +      args:
   +        - sh
   +        - '-c'
   +        - '${PLUGIN_REMOTE_ENDPOINT_EXECUTABLE}'
   +      env:
   +        - name: PLUGIN_REMOTE_ENDPOINT_EXECUTABLE
   +          value: /remote-endpoint/plugin-remote-endpoint
   +        - name: THEIA_PLUGINS
   +          value: local-dir:///plugins/sidecars/tools
   +      memoryLimit: 512Mi
   +      volumeMounts:
   +        - name: plugins
   +          path: /plugins
   +        - name: remote-endpoint
   +          path: /remote-endpoint
   +      image: quay.io/devfile/universal-developer-image@sha256:53cec58dd190dd1e06100478ae879d7c28abd8fc883d5fdf5be3eb6e943fe5e7
      - name: theia-ide
        container:
          image: quay.io/eclipse/che-theia:next

2. Che-Code + Golang [link]

   kubectl apply -f https://gist.githubusercontent.com/amisevsk/46da73704a126d2324e5c633ff077b8c/raw/9af5c2d5ffa2012e760acdcd6fa4427fb611af1c/code-golang.devfile.yaml
   

   Creates a DevWorkspace that references the plugin devfile che-code.yaml. This plugin is a modified version of the current plugin hosted here, which converts the che-code-runtime-description into a container contribution. This workspace results in a deployment with two init containers (che-code-injector and project-clone) and only one running container (tools). The changes to the plugin used here compared to the one in the registry are:

   @@ -13,8 +13,9 @@
        attributes:
          app.kubernetes.io/component: che-code-runtime
          app.kubernetes.io/part-of: che-code.eclipse.org
   +      controller.devfile.io/container-contribution: true
        container:
   -      image: quay.io/devfile/universal-developer-image@sha256:53cec58dd190dd1e06100478ae879d7c28abd8fc883d5fdf5be3eb6e943fe5e7
   +      image: quay.io/devfile/universal-developer-image:ubi8-0e189d9
          command:
            - /checode/entrypoint-volume.sh
          volumeMounts:

   (I updated the image to match the one from the devfile to avoid having to pull two separate UDI images)

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

[amisevsk]

@dkwon17 @AObuchow I'm not able to request review from you yet, but please take a look as well :)

[amisevsk]

To add, this PR enables more easily switching editors in the Che case, as only the plugin needs to be swapped out (and potentially the annotation saying which vsix plugins need to be installed). With the existing approach in Che, the changes to the devfile have to be undone before another editor can be applied

[l0rd]

@amisevsk thanks I will test that later. To help me understanding the approach can you please confirm that it requires that:

1. The DevWorkspaceTemplate that defines the IDE include the attribute controller.devfile.io/container-contribution: true
2. The DevWorkspace that references the IDE DWT (as a plugin) includes a component with the attribute controller.devfile.io/merge-contribution: true

Or are the attributes inferred automatically (for instance if the component is a plugin then, when flattened, it will have container-contribution: true)?

[amisevsk]

At the moment, attributes are explicit and have to be defined in the DevWorkspace/plugins. I took this approach to avoid breaking devfiles, as e.g. the che-docs devfile does not use an UDI image and injecting the editor stuff there might break things. The merge-contribution attribute could be added when plugins are added (e.g. in the Che dashboard), and the container-contribution attribute would be included in the Theia/che-code plugin devfile.

Part of my hope with how this works is that plugins with container contributions can use a fallback image in case injecting is not possible. For example, che-code could specify an image that will mostly work to host the editor in the case that a container that can run it is not found -- I believe this is the case with the current plugin definition.

[amisevsk]

I've tested the container contribution functionality with the existing Che dashboard:

• For Theia, the dashboard will add env vars, args, and volumeMounts to the devfile container to inject Theia
• These values will be overwritten by the containerContributions from the theia plugin, but this is a no-op since they're the same values (the dashboard gets these values from the plugin devfile anyways)
• Removing the dashboard-added values does not change functionality as the contribution adds them back into the "flattened" devworkspace.

However, in the vscode case, it appears that the dashboard drops the che-code-runtime-description component before creating the DevWorkspaceTemplate, breaking injection (since DWO never sees the contribution component). Testing directly by changing the plugin to point at the plugin devfile (instead of the DWT) seems to work fine though.

A note on testing: to test this flow, it's not possible to use standalone devfiles due to eclipse-che/che#21433. The correct flow appears to be

1. Create a workspace using a factory URL referencing a github repo (I use https://github.com/che-samples/golang-example/tree/devfilev2) and the editor to be tested
2. Update the DevWorkspace in-cluster to add the merge-contribution attribute and replace the plugin with the intended one.

[benoitf]

Hello,

is that controller.devfile.io/merge-contribution: true is optional and then it takes the first container of the Devfile ?

because I don't expect to have users having to change their devfile.

For controller.devfile.io/container-contribution: true yes we can add that into the editors definitions in the plugin registry.

[amisevsk]

is that controller.devfile.io/merge-contribution: true is optional and then it takes the first container of the Devfile ? Because I don't expect to have users having to change their devfile.

Currently it's not optional, and merging does not happen if it is not present. The reason for this is that it could be a breaking change -- for example, I don't know if the che-docs repository would work if the injection happened automatically, since it does not use the common UDI image. If automatically merging contributions breaks your devfile, then you'd have to change your devfile to make it work again, as opposed to changing it to make it work in a different way.

The upshot is that it should be possible to write the editor plugins so that the workspace starts and shows an editor even if merging does not happen, as the contribution component could bring its own default image.

If we want the merge-contribution feature to be inferred, this could also be done in the Che Dashboard, where we currently manually do the injection. For example, in the case of che-code, instead of taking the fields from the che-code-runtime-description component and applying them to the main developer container, the dashboard could just add the merge-contribution attribute when it adds a the plugin.

[dkwon17]

@amisevsk I've followed the testing steps for the two test DevWorkspaces (Che-Code + Golang, Che-Theia + Golang) and the flattened devfile has the container contributions merged as expected 👍 .

I wasn't able to open the Che-Code + Golang editor via the devworkspace's status.mainUrl however. Was this expected?

[benoitf]

@dkwon17 you may have to add CODE_HOST env variable set to 0.0.0.0
che-incubator/che-code#41

[amisevsk]

@dkwon17 Shouldn't be happening 🤔. Florent is right (in case your DevWorkspace doesn't have the CODE_HOST override, though that is in the sample devfile so if you used that it might be something else.

Without the CODE_HOST override, I believe the DevWorkspace won't reach a running state since it tries to check the mainUrl.

[dkwon17]

I see, I'm still not able to open the editor even though I'm using the sample devfile that @amisevsk provided. Here is the flattened devfile I retrieved by running cat $DEVWORKSPACE_FLATTENED_DEVFILE from the tools container:
https://gist.githubusercontent.com/dkwon17/a2c6cccbeacdf984eb9ab7230d507117/raw/1ec8dbe13df965aa241be0168c42226b5c9843a9/flattened-code-golang.yaml

I also confirmed that the CODE_HOST environment variable is also set in the tools container:

sh-4.4$ echo $CODE_HOST
0.0.0.0

Also, the devworkspace seems to enter the running state without any issues:

Here is the yaml for the test-contrib-code devworkspace in the screenshot ^ https://gist.githubusercontent.com/dkwon17/4b1e768f9873d262219e9f85ed940fdf/raw/fcc074c03962355bc03f68f7c516d40ba8cbfab5/golangcode.yaml

[ibuziuk]

hit the same problem as David during the vs code verification.
from my end, I think it is ok to make the attributes required in the initial PR to make sure that no existing devfiles are broken and promote it to the dogfooding.

[l0rd]

After discussion yesterday with @amisevsk +1 to require the explicit attributes: adding the merge-contribution: true will be Che dashboard responsibility. It makes sense that who adds the contribution (e.g. Che dashboard) selects the contribution recipient too.

On the other hand, to keep the DevWorkspace.spec.template as close to a devfile as possible we have discussed to introduce a new contributions field at the DevWorkspace.spec level. That would allow to avoid using plugins for contributions. Plugins would continue to be supported with their existing behavior (no impact on existing Che/OCP) and we will consider to deprecate them after Che and the OpenShift Console has been updated to use contributions instead of plugins.

The theia-golang example above would change like this:

kind: DevWorkspace
apiVersion: workspace.devfile.io/v1alpha2
metadata:
  name: test-contrib-theia
spec:
  started: true
+ contributions:
+  - name: che-theia-ide
+    uri: https://gist.githubusercontent.com/amisevsk/b342b9ae064b5a79e08945d04d02c208/raw/84cd2df9266b4a608fbc0fd0cc18816bca03e344/theia-contrib.devfile.yaml:
  template:
    attributes:
      controller.devfile.io/storage-type: ephemeral
    components:
    (...)      
-      - name: che-theia-ide
-        plugin:
-          uri: https://gist.githubusercontent.com/amisevsk/b342b9ae064b5a79e08945d04d02c208/raw/84cd2df9266b4a608fbc0fd0cc18816bca03e344/theia-contrib.devfile.yaml

[AObuchow]

I was able to test out both the che-theia and che-code gists provided with this PR.
The code looks good to me so far as well.

I am also +1 for making the controller.devfile.io/merge-contribution attribute required to prevent breaking any existing devfile.

[svor]

@amisevsk a small remark: looking at the Che-Theia + Golang DevWorkspace, it's better to rename the editor component, because che-code is confusing a bit

[amisevsk]

PR devfile/api#879 would add the proposed containerContributions field if merged. However, in the interest of sequencing PRs/issues, I'm marking this PR as ready to review -- if there's no issues with this approach this PR can be merged and the functionality can be extended/applied to the contributions field as well.

[amisevsk]

@svor good catch -- updated.

[amisevsk]

/retest

[amisevsk]

To test these changes via OLM, I've build an index image quay.io/amisevsk/devworkspace-operator-index:container-contributions this can be used to create a catalogsource on the cluster and deploy the current changes via OLM.

[l0rd]

I don't recall why we need to add the suffix ?tkn=eclipse-che to VS Code URL. @azatsarynnyy may know that?

[azatsarynnyy]

@l0rd @amisevsk
The connection token (passed through the tkn query parameter) used to be required by VS Code some time ago.
Currently, it's optional and we don't use it in Che-Code.
Che-Code is launched with --without-connection-token startup parameter:
https://github.com/che-incubator/che-code/blob/d6291d3ede49eabd27e331ded40d465b7b5f0385/build/scripts/entrypoint-volume.sh#L76

[l0rd]

Thanks @azatsarynnyy for the clarification. I think that then we should replace that path with ?folder=${PROJECT_SOURCE} in the DevWorkspaceTemplate to fix #21400. And maybe we can also get rid of our built in extension che-on-start?

[azatsarynnyy]

Thanks @azatsarynnyy for the clarification. I think that then we should replace that path with ?folder=${PROJECT_SOURCE} in the DevWorkspaceTemplate to fix #21400. And maybe we can also get rid of our built in extension che-on-start?

@l0rd please see my comment on eclipse-che/che#21400 (comment)
Getting rid of che-on-start extension means losing such important VS Code functionality as the multi-root workspaces support.

upd: I mean not losing that functionality completely, but it will require the user to do some manual steps for opening a multi-root VS Code workspace. The mentioned VS Code extension does that automatically. We just need to reworking that in a more efficient way, w/o reloading the page.

[amisevsk]

In that case, it appears that this issue isn't a real problem for merging the current PR -- we can figure out the parameters later and it works as intended in the Che dashboard in my testing. I'm still interested in why providing the ?tkn=eclipse-che parameter causes a wrong redirect, but if there's an issue in DWO there it's likely present in main as well.

[amisevsk]

/retest

[AObuchow]

Though this line (and other similar ones) could be just if component.Attributes.GetBoolean(constants.ContainerContributionAttribute, nil) {... I think it's okay to be explicit here about wanting the value of the ContainerContributionAttribute to be true.

[amisevsk]

No, this is just me being a dummy :D

I'll remove it.

[amisevsk]

Fixed now, thanks :)

[AObuchow]

This is nitpicking, but maybe an error holder should be added here and at line 134. It'll make the code that calls this function in merge.go a bit uglier though.

Future calls of component.Attributes.GetBoolean(constants.ContainerContributionAttribute, nil) in mergeContainerContributions wouldn't need this error holder since needsContainerContributionMerge is called first.

[amisevsk]

This is a good point -- previously I was just interpreting wrong values as false but it makes sense to highlight the case where the attribute is present but cannot be read correctly, e.g. if the value is ture or something.

I'll update the PR with that change.

[AObuchow]

Left some minor comments that aren't really necessary for this to be merged.
Code looks good to me, as do the automated test cases.

Tested (again) the provided samples on Minikube, and they work as expected. Awesome work @amisevsk 😁

[l0rd]

@amisevsk I have tested the PR updating the devworkspace-demo and it worked perfectly. I have also tried to reproduce the scenario of this downstream issue and the CPU limits are summed up correctly. Just one note: I have also tired to use the spec.contributions in the devworkspace-demo but it's not working yet: I think that's supposed to be implemented in a separate PR.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

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

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]

I have also tired to use the spec.contributions in the devworkspace-demo but it's not working yet: I think that's supposed to be implemented in a separate PR.

This is correct. This PR does not update the devfile/api dependency to include containerContributions in the DevWorkspace CR. The dependency bump is part of #868, which is currently on hold to ensure we don't cause issues applying the CRDs in other tools. Once that PR is merged, I'll open another PR to also process containerContributions.

[openshift-ci]

New changes are detected. LGTM label has been removed.