Skip to content

Propagate DevWorkspace .spec.started status to an annotation on routings #617

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 30, 2021

Conversation

amisevsk
Copy link
Collaborator

This is a backwards-compatible re-do of #604

What does this PR do?

Add annotation 'controller.devfile.io/devworkspace-started' to be applied to DevWorkspaceRoutings. If a workspace is stopped, the annotation is set to "false", otherwise it is "true". If the DevWorkspaceRouting reconciler encounters a DevWorkspaceRouting with the annotation set to false, it ends the reconcile early.

This ensures that every workspace start/stop event triggers a reconcile, and allows the routing to clean up resources if necessary.

What issues does this PR fix or reference?

Re-closes #602

Is it tested? How?

Check that DevWorkspaceRoutings get the annotation, and that this doesn't break the Che operator.

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

Add annotation 'controller.devfile.io/devworkspace-started' to be
applied to DevWorkspaceRoutings. If a workspace is stopped, the
annotation is set to "false", otherwise it is "true". If the
DevWorkspaceRouting reconciler encounters a DevWorkspaceRouting with the
annotation set to false, it ends the reconcile early.

This ensures that every workspace start/stop event triggers a
reconcile, and allows the routing to clean up resources if necessary.

Signed-off-by: Angel Misevski <[email protected]>
@sleshchenko
Copy link
Member

/test v8-devworkspace-operator-e2e, v8-che-happy-path

@sleshchenko
Copy link
Member

Che Happy path test fails with the following error on DWO side:

{"level":"info","ts":1632548602.532224,"logger":"controllers.DevWorkspace","msg":"Reconciling Workspace","Request.Namespace":"che-user-che","Request.Name":"spring-petclinic-ewgp","devworkspace_id":"workspace94dadae88e684355"}
panic: assignment to entry in nil map

goroutine 1235 [running]:
github.com/devfile/devworkspace-operator/pkg/provision/workspace.getSpecRouting(0xc000224900, 0xc0004ed1f0, 0xc0009f6880, 0x1cce8df, 0x1ca63c0)
	/devworkspace-operator/pkg/provision/workspace/routing.go:157 +0x1a5
github.com/devfile/devworkspace-operator/pkg/provision/workspace.SyncRoutingToCluster(0xc000224900, 0x1f235e0, 0xc0005e1270, 0xc0004ed1f0, 0x1f17860, 0xc000230e20, 0x1f0da20, 0xc001952120, 0x0, 0x0, ...)
	/devworkspace-operator/pkg/provision/workspace/routing.go:69 +0x77
github.com/devfile/devworkspa

Copy link
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Probably it's too late to name that alternative but we could do spec.started as well, with help of mutating webhooks and restoring value from the existing state. It's the same we do for creator annotation.
I have no idea why it did not appear for me earlier.

But I don't have a strong opinion, I let you decide which you like more.

@sleshchenko
Copy link
Member

/test v8-devworkspace-operator-e2e, v8-che-happy-path

@openshift-ci
Copy link

openshift-ci bot commented Sep 28, 2021

@amisevsk: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/v8-che-happy-path ad1f8cc link true /test v8-che-happy-path

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@openshift-ci
Copy link

openshift-ci bot commented Sep 28, 2021

[APPROVALNOTIFIER] This PR is APPROVED

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

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,sleshchenko]

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

@amisevsk amisevsk merged commit 24bf5b8 into devfile:main Sep 30, 2021
@amisevsk amisevsk deleted the routing-started-annotation branch September 30, 2021 01:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Propagate workspace started state to DevWorkspaceRouting
3 participants