Allow external DWOC to be merged with internal DWOC on a per-workspace basis #918

AObuchow · 2022-09-02T22:30:39Z

What does this PR do?

This PR makes 2 changes to DWO:

Refactor (almost) all uses of the global DWOC, so that, internally, each workspace instance may have its own configuration which it uses. Currently, this per-workspace DWOC is not visible to users, and was added only to support change n.2
Allow for specifying an external DWOC (that exists on the cluster), which gets merged with the workspace's internal DWOC.

The external DWOC's name and namespace are specified with the controller.devfile.io/devworkspace-config DevWorkspace attribute, which has the following structure:

attributes:
  controller.devfile.io/devworkspace-config:
    name: <string>
    namespace: <string>

What issues does this PR fix or reference?

Part of eclipse-che/che#21405

Is it tested? How?

2 automated tests were added.

For manual testing, perform the following steps:

Deploy DWO
Create a devfile with the controller.devfile.io/devworkspace-config attribute set, for reference, we'll call it external-dwoc-workspace.yaml, e.g.

kind: DevWorkspace
apiVersion: workspace.devfile.io/v1alpha2
metadata:
  name: theia-next-external-dwoc
spec:
  started: true
  template:
    attributes:
        controller.devfile.io/devworkspace-config:
          name: external-dwoc-test
          namespace: default
        controller.devfile.io/storage-type: per-workspace
    projects:
      - name: web-nodejs-sample
        git:
          remotes:
            origin: "https://github.com/che-samples/web-nodejs-sample.git"
    components:
      - name: theia
        plugin:
          uri: https://che-plugin-registry-main.surge.sh/v3/plugins/eclipse/che-theia/next/devfile.yaml
          components:
            - name: theia-ide
              container:
                env:
                  - name: THEIA_HOST
                    value: 0.0.0.0
    commands:
      - id: say-hello
        exec:
          component: theia-ide
          commandLine: echo "Hello from $(pwd)"
          workingDir: ${PROJECTS_ROOT}/project/app

Create a DWOC file, for reference, we'll call it test-external-dwoc.yaml. Also, ensure its name field is set to the same value as the controller.devfile.io/devworkspace-config-name attribute in the devfile from step 2:

apiVersion: controller.devfile.io/v1alpha1
config:
  enableExperimentalFeatures: true
  routing:
    clusterHostSuffix: 192.168.49.2.nip.io
    defaultRoutingClass: basic
  workspace:
    defaultStorageSize:
      common: 20Gi
      perWorkspace: 29Gi
    imagePullPolicy: Always
kind: DevWorkspaceOperatorConfig
metadata:
  name: external-dwoc-test
  namespace: default

Apply the (external) DWOC file you just created, ensuring it's in the namespace you specified in the controller.devfile.io/devworkspace-config-namespace (in my example, the default namespace is used): kubectl apply -f test-external-dwoc.yaml
Apply the devfile created in step 2. e.g. kubectl apply -f external-dwoc-workspace.yaml -n $NAMESPACE
Verify that the internal DWOC has been modified. I recommend checking the size of the per-workspace PVC that is created and ensuring it is 29Gi instead of the default 10Gi
Afterwards, you can ensure that workspace's that do not have the controller.devfile.io/devworkspace-config attribute set are not affected by the external DWOC, and instead, they use the global/internal DWOC. For instance, start up the per-workspace example and ensure the PVC created for it is 10Gi big: kubectl apply -f per-workspace-storage.yaml -n $NAMESPACE.

Some notes and limitations:

The HTTP client / Proxy settings cannot be affected by the externally applied DWOC, as the HTTP client is created and configured on DWO startup rather than workspace startup.
The basic DevWorkspace Router is still using the cluster host suffix from the global DWOC. If required in the future, it is possible to allow the cluster host suffix to be retrieved from the workspace's configuration. Here's an example of how this may be done.
There is a handler that checks if the common PVC was deleted. Currently, it filters for PVC's related to DWO by checking the PVC name against the global configs PVC name. In the future, this limitation can be worked around by applying a DWO-specific annotation to all PVC's created by DWO, and filtering by that annotation.
This change could have weird implications for anything configurable that is shared across workspaces. Most notably, the common PVC. For example, if the common PVC does not yet exist on the cluster, and an external DWOC that has the common PVC size set to 20Gi is applied to a specific workspace (resulting in creation of the common PVC), then other (future) workspaces that use the common storage strategy will also be using a PVC of 20Gi, even if they aren't using an external DWOC.

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

AObuchow · 2022-09-02T22:33:10Z

Something that may be currently missing from this PR is documenting the new controller.devfile.io/devworkspace-config attribute.

AObuchow · 2022-09-06T15:47:41Z

Something that may be currently missing from this PR is documenting the new controller.devfile.io/devworkspace-config attribute.

Additional documentation has now been updated.

AObuchow · 2022-09-06T15:56:45Z

docs/additional-configuration.adoc

+        namespace: <string>
+----
+
+*Note:* the alternate DevWorkspace-Operator configuration will be merged with the global DevWorkspace-Operator configuration; the configuration resulting from the merge will be used by the workspace that has the `controller.devfile.io/devworkspace-config` attribute set.


It might be worth mentioning the limitations of per-workspace DWOC regarding the HTTP client and basic router.

amisevsk

Looking good but needs a bit more testing. I've pushed a couple of commits to this branch as well:

Commit d2a85ca - fix an issue where we were trying to patch DevWorkspaceWithConfig
Commit 41929c2 - fix the logging to log the resolvedConfig rather than the internal

amisevsk · 2022-09-06T20:19:58Z

controllers/workspace/devworkspace_controller.go

+	config, err := wkspConfig.ResolveConfigForWorkspace(rawWorkspace, clusterAPI.Client)
+	if err != nil {
+		reqLogger.Error(err, "Error applying external DevWorkspace-Operator configuration")
+	}
 	configString := wkspConfig.GetCurrentConfigString()


wkspConfig.GetCurrentConfigString() should be updated to take a config as a parameter, to make it easier to print the resolved config. As it stands, using a custom config doesn't change the log below:

reqLogger.Info("Reconciling Workspace", "resolvedConfig", configString)

Fixed by 41929c2

amisevsk · 2022-09-06T20:26:50Z

pkg/config/sync.go

+// TODO: Improve this function description?
+// Checks for an external DevWorkspace-Operator configuration specified by the optional workspace attribute `controller.devfile.io/devworkspace-config`
+// If the `controller.devfile.io/devworkspace-config` attribute is not set, the internal/global DWOC is returned
+// If the `controller.devfile.io/devworkspace-config` attribute is incorrectly set, the internal/global DWOC and an error are returned.
+// If the external DWOC is found on the cluster, it is merged with the internal/global DWOC and returned.
+// If the external DWOC is not found on the cluster, the internal/global DWOC and an error are returned.
+func ResolveConfigForWorkspace(workspace *dw.DevWorkspace, client crclient.Client) (*controller.OperatorConfiguration, error) {


Exported function docs should start with the name of the function (see: https://go.dev/doc/comment). In addition, returning a non-nil OperatorConfiguration on error is a bit of an anti-pattern as it allows callers to easily ignore errors.

// ResolveConfigForWorkspace returns the OperatorConfiguration that should be used for the provided workspace. If the // workspace specifies a custom configuration in its attributes, that configuration is merged into the global operator // configuration and returned. Otherwise, the global operator configuration is returned. Returns an error if the custom // configuration cannot be resolved.

amisevsk · 2022-09-06T20:28:39Z

pkg/config/sync.go

+
+		err := workspace.Spec.Template.Attributes.GetInto(constants.ExternalDevWorkspaceConfiguration, &namespacedName)
+		if err != nil {
+			return internalConfigCopy, fmt.Errorf("failed to read attribute %s in DevWorkspace attributes: %w", constants.ExternalDevWorkspaceConfiguration, err)


Should return nil, [error] (here and elsewhere in the function to force the error to be addressed (and not hidden/ignored). Then, call sites should be something like

config, err := wkspConfig.ResolveConfigForWorkspace(rawWorkspace, clusterAPI.Client) if err != nil { reqLogger.Error(err, "Error applying external DevWorkspace-Operator configuration") config = wkspConfig.GetGlobalConfig() }

i.e. explicitly use the global configuration if we fail to read a custom one.

I didn't want to return an error in the case that the attribute is not set in the workspace (cause then it would get logged constantly) so I added another function ExternalConfigSet to check before trying to resolve the workspace config. However, I dislike the if/else case I had to resort to - maybe this could be done more elegantly

This has now been fixed

pkg/config/sync_test.go

amisevsk · 2022-09-06T20:34:29Z

pkg/constants/attributes.go

+	// ExternalDevWorkspaceConfiguration is an attribute that allows for specifying an (optional) external DevWorkspace-Operator configuration (DWOC)
+	// which will merged with the internal/global DevWorkspace-Operator configuration. The DWOC resulting from the merge will be used for the workspace.
+	// The fields which are set in the external DevWorkspace-Operator configuration will overwrite those existing in the
+	// internal/global DevWorkspace-Operator configuration during the merge.
+	// The structure of the attribute value should contain two strings: name and namespace.
+	// 'name' specifies the metadata.name of the external DevWorkspace-Operator configuration.
+	// 'namespace' specifies the metadata.namespace of the external DevWorkspace-Operator configuration.
+	// For example:
+	//
+	//   attributes:
+	//     controller.devfile.io/devworkspace-config:
+	//         name: external-dwoc-name
+	//         namespace: some-namespace
+	ExternalDevWorkspaceConfiguration = "controller.devfile.io/devworkspace-config"


For clarity, correct terms to use are DevWorkspace Operator for the name of the operator, DevWorkspaceOperatorConfig for the name of the custom resource, and just configuration or operator configuration for the general concept

amisevsk · 2022-09-06T20:36:24Z

docs/additional-configuration.adoc

@@ -156,6 +156,31 @@ Note: As for automatically mounting secrets, it is necessary to apply the `contr

 This will mount a file `/tmp/.git-credentials/credentials` in all workspace containers, and construct a git config to use this file as a credentials store. The mount path specified by the `controller.devfile.io/mount-path` annotation can by omitted, in which case `/` is used (mounting a file `/credentials`).

+## Setting an alternate configuration for a workspace
+It is possible to configure a workspace to use an alternate DevWorkspace-Operator configuration.


- DevWorkspace-Operator configuration + DevWorkspaceOperatorConfig

docs/additional-configuration.adoc

amisevsk · 2022-09-06T21:18:07Z

Also pushed d81bc71 to fix a bug I found, where DWO wasn't provisioning the common PVC. Finally got around to renaming checkForExistingCommonPVC, which was super confusing.

amisevsk

Pretty much looks good; will do one more quick round of testing tomorrow and then we should be good to merge.

controllers/workspace/devworkspace_controller.go

pkg/common/types.go

Introduce new struct that stores both the current DevWorkspace being worked on and the config when reconcile began. This struct will be used in place of the plain workspace where possible in order to allow per-workspace configuration through the DWOC. This commit breaks the Go build as it does not update any files that depend on the workspace. Signed-off-by: Angel Misevski <amisevsk@redhat.com> Co-authored-by: Andrew Obuchowicz <aobuchow@redhat.com>

Update all function signatures to use DevWorkspaceWithConfig instead of plain DevWorkspaces to allow config to be resolved per-workspace Signed-off-by: Angel Misevski <amisevsk@redhat.com> Co-authored-by: Andrew Obuchowicz <aobuchow@redhat.com>

Make sure we unpack the DevWorkspace from the DevWorkspaceWithConfig in all calls that involve the controller-runtime or the Kubernetes API to avoid errors due to DWWC not being present in the scheme. Signed-off-by: Angel Misevski <amisevsk@redhat.com> Co-authored-by: Andrew Obuchowicz <aobuchow@redhat.com>

Only log DevWorkspaceOperatorConfig storage size fields if they're different from the default values to avoid cluttering logs. Signed-off-by: Angel Misevski <amisevsk@redhat.com> Co-authored-by: Andrew Obuchowicz <aobuchow@redhat.com>

Replace usages of the global config (e.g. config.Workspace or config.Routing) with the DevWorkspace-specific config obtained from the DevWorkspaceWithConfig. This commit only changes config accesses where the function already has access to the DevWorkspace-scoped config through a function parameter. Signed-off-by: Angel Misevski <amisevsk@redhat.com> Co-authored-by: Andrew Obuchowicz <aobuchow@redhat.com>

Propagate specific config fields through the library and provision package functions to allow using DevWorkspace-scoped configuration in those functions. Where possible, this is done by making the dependency on configuration explicit (e.g. if a library function requires an ImagePullPolicy). Signed-off-by: Angel Misevski <amisevsk@redhat.com> Co-authored-by: Andrew Obuchowicz <aobuchow@redhat.com>

Remove global config.Routing and config.Workspace fields to force all accesses to configuration to either explicitly use the global config (ignoring DevWorkspace-specific configuration) or use the DevWorkspace-scoped config. Signed-off-by: Angel Misevski <amisevsk@redhat.com> Co-authored-by: Andrew Obuchowicz <aobuchow@redhat.com>

Signed-off-by: Angel Misevski <amisevsk@redhat.com> Co-authored-by: Andrew Obuchowicz <aobuchow@redhat.com>

Allow specifying an external DWOC that gets merged with the global DWOC for use by a workspace. During the merge, the fields which are set in the external DWOC will overwrite those existing in the internal/global DWOC, resulting in a merged DWOC to be used by the workspace. The internal/global DWOC will remain unchanged after the merge. The external DWOC's name and namespace are specified with the `controller.devfile.io/devworkspace-config` DevWorkspace attribute, which has the following structure: attributes: controller.devfile.io/devworkspace-config: name: <string> namespace: <string> Part of eclipse-che/che#21405 Signed-off-by: Andrew Obuchowicz <aobuchow@redhat.com>

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

Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Rename the checkForExistingPVC function to make functionality clearer and fix handling of common PVC when using DevWorkspaceWithConfig. This fixes an issue where DWO does not provision the default common PVC. Signed-off-by: Angel Misevski <amisevsk@redhat.com>

amisevsk · 2022-09-10T00:55:57Z

Rebased + squashed fixup commits

amisevsk

LGTM

openshift-ci · 2022-09-10T00:56:20Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

AObuchow requested review from amisevsk and ibuziuk as code owners September 2, 2022 22:30

AObuchow mentioned this pull request Sep 2, 2022

feat: Allow external DWOC to be merged with internal DWOC on a per-workspace basis #909

Closed

3 tasks

AObuchow force-pushed the rework-devworkspace-configuration branch from 5fba63f to 647f301 Compare September 6, 2022 15:47

AObuchow commented Sep 6, 2022

View reviewed changes

AObuchow mentioned this pull request Sep 6, 2022

Annotate DWO-created PVC's #920

Closed

amisevsk reviewed Sep 6, 2022

View reviewed changes

AObuchow force-pushed the rework-devworkspace-configuration branch 2 times, most recently from 5d11520 to 2b7f03e Compare September 7, 2022 20:17

This was referenced Sep 8, 2022

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

Closed

[Dashboard] Add external-DWOC attribute to workspaces eclipse-che/che#21692

Closed

amisevsk reviewed Sep 9, 2022

View reviewed changes

controllers/workspace/devworkspace_controller.go Show resolved Hide resolved

pkg/common/types.go Outdated Show resolved Hide resolved

AObuchow mentioned this pull request Sep 9, 2022

Set workspace status to warning if external DWOC could not be resolved #921

Closed

amisevsk mentioned this pull request Sep 10, 2022

Log all non-default fields when updating config #924

Merged

3 tasks

amisevsk and others added 12 commits September 9, 2022 20:55

Fix tests after moving to DevWorkspaceWithConfig

5513b67

Signed-off-by: Angel Misevski <amisevsk@redhat.com> Co-authored-by: Andrew Obuchowicz <aobuchow@redhat.com>

Document per-workspace external DWOC configuration

312198e

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

Log resolved config string instead of internal config

41d8cf1

Signed-off-by: Angel Misevski <amisevsk@redhat.com>

amisevsk force-pushed the rework-devworkspace-configuration branch from 0173428 to f97d412 Compare September 10, 2022 00:55

amisevsk approved these changes Sep 10, 2022

View reviewed changes

openshift-ci bot assigned amisevsk Sep 10, 2022

openshift-ci bot added the lgtm label Sep 10, 2022

openshift-ci bot added the approved label Sep 10, 2022

AObuchow merged commit c24f04e into main Sep 12, 2022

AObuchow deleted the rework-devworkspace-configuration branch September 12, 2022 14:27

amisevsk mentioned this pull request Feb 15, 2023

Add mechanism for overriding all DWO configuration per-namespace #843

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow external DWOC to be merged with internal DWOC on a per-workspace basis #918

Allow external DWOC to be merged with internal DWOC on a per-workspace basis #918

AObuchow commented Sep 2, 2022 •

edited

Loading

AObuchow commented Sep 2, 2022

AObuchow commented Sep 6, 2022

AObuchow Sep 6, 2022

amisevsk left a comment

amisevsk Sep 6, 2022

amisevsk Sep 6, 2022

amisevsk Sep 6, 2022

amisevsk Sep 6, 2022

AObuchow Sep 7, 2022

AObuchow Sep 7, 2022

amisevsk Sep 6, 2022

amisevsk Sep 6, 2022

amisevsk commented Sep 6, 2022

amisevsk left a comment

amisevsk commented Sep 10, 2022

amisevsk left a comment

openshift-ci bot commented Sep 10, 2022

Allow external DWOC to be merged with internal DWOC on a per-workspace basis #918

Allow external DWOC to be merged with internal DWOC on a per-workspace basis #918

Conversation

AObuchow commented Sep 2, 2022 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

Is it tested? How?

Some notes and limitations:

PR Checklist

AObuchow commented Sep 2, 2022

AObuchow commented Sep 6, 2022

Choose a reason for hiding this comment

amisevsk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amisevsk commented Sep 6, 2022

amisevsk left a comment

Choose a reason for hiding this comment

amisevsk commented Sep 10, 2022

amisevsk left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Sep 10, 2022

AObuchow commented Sep 2, 2022 •

edited

Loading