Skip to content
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

Add new field for DWOC for DevWorkspace pod's runtimeClass #1303

Merged
merged 3 commits into from
Aug 22, 2024

Conversation

dkwon17
Copy link
Collaborator

@dkwon17 dkwon17 commented Aug 12, 2024

What does this PR do?

Introduces a new field for the DWOC for the DevWorkspace pod's runtimeClass

What issues does this PR fix or reference?

#1286

Is it tested? How?

To test this PR,

  1. Create the plain-devworkspace DevWorkspace on the cluster
kind: DevWorkspace
apiVersion: workspace.devfile.io/v1alpha2
metadata:
  name: plain-devworkspace
spec:
  started: true
  routingClass: 'basic'
  template:
    components:
      - name: web-terminal
        container:
          image: quay.io/wto/web-terminal-tooling:next
          memoryRequest: 256Mi
          memoryLimit: 512Mi
          mountSources: true
          command:
           - "tail"
           - "-f"
           - "/dev/null"
  1. Verify that the workspace pod starts and runs properly.

  2. Create this DWOC:

apiVersion: controller.devfile.io/v1alpha1
config:
  workspace:
    runtimeClassName: test
kind: DevWorkspaceOperatorConfig
metadata:
  name: devworkspace-operator-config
  namespace: openshift-operators
  1. Restart the plain-devworkspace DevWorkspace.
    The DevWorkspace startup should fail with:
'Error creating DevWorkspace deployment: Detected unrecoverable deployment condition: FailedCreate pods "workspace9b9d2d153f3747de-55bb88dd5b-" is forbidden: pod rejected: RuntimeClass "test" not found'
  1. To verify that the runtimeClassName attribute takes precedence over the DWOC-defined runtimeClassName, create the following DevWorkspace:
kind: DevWorkspace
apiVersion: workspace.devfile.io/v1alpha2
metadata:
  name: plain-devworkspace-2
spec:
  started: true
  routingClass: 'basic'
  template:
    attributes:
      controller.devfile.io/runtime-class: dw-class
    components:
      - name: web-terminal
        container:
          image: quay.io/wto/web-terminal-tooling:next
          memoryRequest: 256Mi
          memoryLimit: 512Mi
          mountSources: true
          command:
           - "tail"
           - "-f"
           - "/dev/null"
  1. The DevWorkspace startup should fail with:
'Error creating DevWorkspace deployment: Detected unrecoverable deployment condition: FailedCreate pods "workspaceb61b62899c1549aa-55bcc6dbfc-" is forbidden: pod rejected: RuntimeClass "dw-class" not found'

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

Copy link

openshift-ci bot commented Aug 12, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@dkwon17 dkwon17 marked this pull request as ready for review August 19, 2024 13:22
@dkwon17 dkwon17 requested review from AObuchow and removed request for AObuchow August 19, 2024 13:22
Copy link
Collaborator

@AObuchow AObuchow left a comment

Choose a reason for hiding this comment

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

Looks great to me so far, glad to see you're adding DevWorkspace Controller tests 😁 Left some minor comments.

controllers/workspace/util_test.go Outdated Show resolved Hide resolved
controllers/workspace/util_test.go Outdated Show resolved Hide resolved
controllers/workspace/devworkspace_controller_test.go Outdated Show resolved Hide resolved
controllers/workspace/util_test.go Outdated Show resolved Hide resolved
@dkwon17
Copy link
Collaborator Author

dkwon17 commented Aug 19, 2024

Thank you @AObuchow , I've updated this PR

Copy link
Collaborator

@AObuchow AObuchow left a comment

Choose a reason for hiding this comment

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

I still have to do a bit of manual testing, but this looks great to me :) I left some very minor comments.

controllers/workspace/devworkspace_controller_test.go Outdated Show resolved Hide resolved
controllers/workspace/devworkspace_controller_test.go Outdated Show resolved Hide resolved
@AObuchow
Copy link
Collaborator

/retest

@AObuchow
Copy link
Collaborator

@musienko-maxim Any idea what's going wrong with the CI prow tests here?

Copy link
Collaborator

@AObuchow AObuchow left a comment

Choose a reason for hiding this comment

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

Tested on OpenShift 4.16 and works as expected, great work @dkwon17 😁 🙏
My last suggestion is to squash your feedback commits, and maybe rename the "Add tests" commit to something a bit more specific like "Adds controller environment tests for pod runtimeClass"

Copy link

openshift-ci bot commented Aug 21, 2024

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

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

Copy link

openshift-ci bot commented Aug 22, 2024

New changes are detected. LGTM label has been removed.

@dkwon17
Copy link
Collaborator Author

dkwon17 commented Aug 22, 2024

@AObuchow thanks, I've squashed the commits

@AObuchow
Copy link
Collaborator

@dkwon17 Awesome, this looks good to merge :) Once all the CI checks pass, feel free to merge :)

@dkwon17 dkwon17 merged commit 58cd8f0 into devfile:main Aug 22, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants