fix: Set workspace warning if error occurs when resolving external DWOC #925

AObuchow · 2022-09-12T17:22:39Z

What does this PR do?

Sets the workspace status to warning if an issue occurs while resolving the external DWOC (specified in the optional controller.devfile.io/devworkspace-config workspace attribute).

The warning will not appear if other active warnings exist for the workspace (eg. if there was an error during the flattening process of a workspace). This was an opinionated decision, as I think other warnings may have higher priority/urgency compared to not being able to resolve the external DWOC. I can change this if desired :)

What issues does this PR fix or reference?

Fix #921

Is it tested? How?

Deploy DWO
Apply the following workspace, which will reference a (currently non-existent on the cluster) DWOC

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

Get the workspaces for the devworkspace-controller namespace, you should see that warnings are present:

[aobuchow@fedora samples]$ kubectl get dw -n $NAMESPACE -w
NAME                       DEVWORKSPACE ID             PHASE     INFO
theia-next-external-dwoc   workspace7015525656304d3d   Running   [warnings present] https://workspace7015525656304d3d-theia-3100.192.168.49.2.nip.io/

A kubectl describe should list in the Status that the external DWOC was not found:
kubectl describe dw theia-next-external-dwoc -n $NAMESPACE

Name:         theia-next-external-dwoc
Namespace:    devworkspace-controller
Labels:       controller.devfile.io/creator=
Annotations:  controller.devfile.io/started-at: 1663001600348
API Version:  workspace.devfile.io/v1alpha2
Kind:         DevWorkspace
(...)
Status:
  Conditions:
    Last Transition Time:  2022-09-12T16:58:49Z
    Message:               Error applying external DevWorkspace-Operator configuration: could not fetch external DWOC with name external-dwoc-test in namespace default: DevWorkspaceOperatorConfig.controller.devfile.io "external-dwoc-test" not found
    Status:                True
    Type:                  DevWorkspaceWarning
    Last Transition Time:  2022-09-12T16:53:07Z
    Message:               DevWorkspace is starting
    Status:                True
    Type:                  Started
    Last Transition Time:  2022-09-12T16:53:07Z
    Message:               Resolved plugins and parents from DevWorkspace
    Status:                True
    Type:                  DevWorkspaceResolved
    Last Transition Time:  2022-09-12T16:53:07Z
    Message:               Storage ready
    Status:                True
    Type:                  StorageReady
    Last Transition Time:  2022-09-12T16:53:08Z
    Message:               Networking ready
    Status:                True
    Type:                  RoutingReady
    Last Transition Time:  2022-09-12T16:53:08Z
    Message:               DevWorkspace serviceaccount ready
    Status:                True
    Type:                  ServiceAccountReady
    Last Transition Time:  2022-09-12T16:53:13Z
    Message:               DevWorkspace secrets ready
    Status:                True
    Type:                  PullSecretsReady
    Last Transition Time:  2022-09-12T16:53:20Z
    Message:               DevWorkspace deployment ready
    Status:                True
    Type:                  DeploymentReady
    Last Transition Time:  2022-09-12T16:53:20Z
    Status:                True
    Type:                  Ready
  Devworkspace Id:         workspace7015525656304d3d
  Main URL:                https://workspace7015525656304d3d-theia-3100.192.168.49.2.nip.io/
  Message:                 [warnings present] https://workspace7015525656304d3d-theia-3100.192.168.49.2.nip.io/
  Phase:                   Running
Events:                    <none>

Now apply the following external DWOC to the cluster in the default namespace:

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

Ensure that there are no longer any active warnings are present for the workspace:

[aobuchow@fedora samples]$ kubectl get dw -n $NAMESPACE -w
NAME                       DEVWORKSPACE ID             PHASE     INFO
theia-next-external-dwoc   workspace7015525656304d3d   Running   https://workspace7015525656304d3d-theia-3100.192.168.49.2.nip.io/

kubectl describe dw theia-next-external-dwoc -n $NAMESPACE:

Name:         theia-next-external-dwoc
Namespace:    devworkspace-controller
Labels:       controller.devfile.io/creator=
Annotations:  controller.devfile.io/started-at: 1663001600348
              test: testt
API Version:  workspace.devfile.io/v1alpha2
Kind:         DevWorkspace
(...)
Status:
  Conditions:
    Last Transition Time:  2022-09-12T17:01:51Z
    Message:               External DevWorkspaceOperatorConfig successfully resolved
    Status:                False
    Type:                  DevWorkspaceWarning
    Last Transition Time:  2022-09-12T16:53:07Z
...

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-09-13T13:46:06Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: AObuchow, ibuziuk
Once this PR has been reviewed and has the lgtm label, please assign amisevsk for approval by writing /assign @amisevsk in a comment. For more information see:The Kubernetes Code Review Process.

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

amisevsk · 2022-09-13T14:10:21Z

controllers/workspace/devworkspace_controller.go

+	} else {
+		configResolveStatus.setConditionFalse(conditions.DevWorkspaceWarning, "External DevWorkspaceOperatorConfig successfully resolved")


I don't know if setting the condition false here is correct -- what this is saying is that there are no warnings (general) for the workspace, not that there's no issue with the config. I think it can be left undefined as it is for other ones (maybe in the future the last thing we do is explicitly set it false at the very end?) -- that way the warning condition doesn't show up at all until there's a warning.

amisevsk · 2022-09-13T14:14:14Z

controllers/workspace/devworkspace_controller.go

@@ -123,10 +123,13 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request
 		return reconcile.Result{}, err
 	}

+	configResolveStatus := &workspaceConditions{}


We could instead init the reconcileStatus struct here:

reconcileStatus := currentStatus{} // maybe, if warning: reconcileStatus.setConditionTrue(conditions.DevWorkspaceWarning, fmt.Sprint("Error applying external DevWorkspace-Operator configuration: ", err.Error()))

and potentially set the warning condition in it if necessary. Then later, at lines 214/215

- reconcileStatus := currentStatus{phase: dw.DevWorkspaceStatusStarting} + reconcileStatus.phase = dw.DevWorkspaceStatusStarting reconcileStatus.setConditionTrue(conditions.Started, "DevWorkspace is starting")

amisevsk · 2022-09-13T14:15:32Z

controllers/workspace/devworkspace_controller.go

+	// Notify if error occured when resolving external DevWorkspaceOperatorConfig, as long as no other active warnings are present
+	if condition, ok := reconcileStatus.conditions[conditions.DevWorkspaceWarning]; !ok || condition.Status == corev1.ConditionFalse {
+		reconcileStatus.setCondition(conditions.DevWorkspaceWarning, configResolveStatus.conditions[conditions.DevWorkspaceWarning])
+	}
+


This is somewhat awkward placement for this -- if we want it here to not lose e.g. a warning in flattening, then it should be even later in the function. What if a later step wants to add a warning?

See the note about initializing reconcileStatus earlier, that may be a cleaner option. Note that for now, setConditionTrue overwrites the condition if it's already present, so taking this approach would have the same functionality.

What if a later step wants to add a warning?

Good point 👍

Note that for now, setConditionTrue overwrites the condition if it's already present, so taking this approach would have the same functionality.

I think the issue (with setting the warning condition earlier) then becomes that the config resolution warning will be suppressed by the no-warning-case when flattening.

Perhaps the workspace conditions need to be modified a bit to allow for enabling/disabling different types of conditions with more granularity (eg. allow for a flattening warning condition set to false, while having a config resolution warning set to true)

Ah that's a good point... I might err on the side of removing that line in the flatten flow and never setting the warning condition as false 🤷

Up to you. I've got a TODO on my backlog somewhere to improve warnings handling.

Though it's not nice, I'm also +1 for removing that line from the flattening flow, for the time being.

I can create another issue to improve warnings handling, and I'm down to work on this whenever :)

I didn't realize the warning will get unset if no warnings occurred by the end of the reconcile function - so this isn't as big of a deal as I had initially thought

openshift-ci · 2022-09-14T19:56:26Z

New changes are detected. LGTM label has been removed.

ibuziuk

@AObuchow looks like some of the commits are missing Signed-off-by:, could you possibly squash in one?

Fix devfile#921 Signed-off-by: Andrew Obuchowicz <[email protected]>

Signed-off-by: Andrew Obuchowicz <[email protected]>

AObuchow · 2022-09-15T15:22:05Z

@AObuchow looks like some of the commits are missing Signed-off-by:, could you possibly squash in one?

Should be good now.
I'll give @amisevsk some time to check this out for final review. Will merge by the end of the day unless he merges it.

AObuchow requested review from amisevsk and ibuziuk as code owners September 12, 2022 17:22

ibuziuk approved these changes Sep 13, 2022

View reviewed changes

openshift-ci bot assigned ibuziuk Sep 13, 2022

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

amisevsk reviewed Sep 13, 2022

View reviewed changes

openshift-ci bot removed the lgtm label Sep 14, 2022

AObuchow requested a review from amisevsk September 14, 2022 19:58

ibuziuk reviewed Sep 15, 2022

View reviewed changes

AObuchow added 2 commits September 15, 2022 11:20

fix: warn if error occurs when resolving external DWOC

b5a2b00

Fix devfile#921 Signed-off-by: Andrew Obuchowicz <[email protected]>

remove duplicate import in DWO controller

1f87eb9

Signed-off-by: Andrew Obuchowicz <[email protected]>

AObuchow force-pushed the warning_if_external_dwoc_error branch from e1bc7b5 to 1f87eb9 Compare September 15, 2022 15:20

AObuchow merged commit 12c3f81 into devfile:main Sep 15, 2022

AObuchow deleted the warning_if_external_dwoc_error branch September 15, 2022 22:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: Set workspace warning if error occurs when resolving external DWOC #925

fix: Set workspace warning if error occurs when resolving external DWOC #925

Uh oh!

AObuchow commented Sep 12, 2022

Uh oh!

openshift-ci bot commented Sep 13, 2022

Uh oh!

amisevsk Sep 13, 2022

Uh oh!

amisevsk Sep 13, 2022

Uh oh!

amisevsk Sep 13, 2022

Uh oh!

AObuchow Sep 13, 2022

Uh oh!

amisevsk Sep 14, 2022

Uh oh!

AObuchow Sep 14, 2022

Uh oh!

AObuchow Sep 14, 2022

Uh oh!

openshift-ci bot commented Sep 14, 2022

Uh oh!

ibuziuk left a comment

Uh oh!

AObuchow commented Sep 15, 2022

Uh oh!

Uh oh!

		} else {
		configResolveStatus.setConditionFalse(conditions.DevWorkspaceWarning, "External DevWorkspaceOperatorConfig successfully resolved")

fix: Set workspace warning if error occurs when resolving external DWOC #925

fix: Set workspace warning if error occurs when resolving external DWOC #925

Uh oh!

Conversation

AObuchow commented Sep 12, 2022

What does this PR do?

What issues does this PR fix or reference?

Is it tested? How?

PR Checklist

Uh oh!

openshift-ci bot commented Sep 13, 2022

Uh oh!

amisevsk Sep 13, 2022

Choose a reason for hiding this comment

Uh oh!

amisevsk Sep 13, 2022

Choose a reason for hiding this comment

Uh oh!

amisevsk Sep 13, 2022

Choose a reason for hiding this comment

Uh oh!

AObuchow Sep 13, 2022

Choose a reason for hiding this comment

Uh oh!

amisevsk Sep 14, 2022

Choose a reason for hiding this comment

Uh oh!

AObuchow Sep 14, 2022

Choose a reason for hiding this comment

Uh oh!

AObuchow Sep 14, 2022

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Sep 14, 2022

Uh oh!

ibuziuk left a comment

Choose a reason for hiding this comment

Uh oh!

AObuchow commented Sep 15, 2022

Uh oh!

Uh oh!