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

feat: add workspace.defaultSchedulerName to DWOC #976

Merged
merged 3 commits into from
Nov 17, 2022

Conversation

dkwon17
Copy link
Collaborator

@dkwon17 dkwon17 commented Nov 12, 2022

Signed-off-by: David Kwon [email protected]

What does this PR do?

Adds the workspace.schedulerName config to the DW operator config.

What issues does this PR fix or reference?

Part of eclipse-che/che#21803

Is it tested? How?

  1. Checkout this PR and run locally following these steps: https://github.com/devfile/devworkspace-operator#run-controller-locally
  2. Edit the workspace.defaultSchedulerName in the DWOC resource in the devworkspace-controllers namespace. Example:
config:
  workspace:
    defaultSchedulerName: testing123
  1. Run oc apply -f samples/theia-next.yaml
  2. Verify that the spec.schedulerName is the defaultSchedulerName from the DWOC resource:

image

(pod won't start of course, because the scheduler doesn't exists)
  1. Optional testing: if pod-overrides in the DW is used to set the schedulerName, confirm that the scheduler name from the pod-overrides is set instead of the scheduler name from the DWOC. For example, create this DW and confirm that the pod's spec.schedulerName is stork:
kind: DevWorkspace
apiVersion: workspace.devfile.io/v1alpha2
metadata:
  name: theia-next-stork
spec:
  started: true
  template:
    attributes:
      pod-overrides:
        spec:
          schedulerName: stork
    components:
      - name: my-container-1
        container:
          image: quay.io/wto/web-terminal-tooling:next

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

@codecov
Copy link

codecov bot commented Nov 12, 2022

Codecov Report

Base: 50.20% // Head: 50.28% // Increases project coverage by +0.08% 🎉

Coverage data is based on head (62aa6f1) compared to base (890b27c).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #976      +/-   ##
==========================================
+ Coverage   50.20%   50.28%   +0.08%     
==========================================
  Files          39       39              
  Lines        2484     2488       +4     
==========================================
+ Hits         1247     1251       +4     
  Misses       1107     1107              
  Partials      130      130              
Impacted Files Coverage Δ
pkg/config/sync.go 71.25% <100.00%> (+0.45%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

pkg/config/sync.go Outdated Show resolved Hide resolved
Comment on lines 156 to 158
// DefaultSchedulerName is the name of the pod scheduler for DevWorkspace pods.
// If not specified, the pod scheduler is set to the default scheduler.
DefaultSchedulerName string `json:"defaultSchedulerName,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Slightly unclear -- field is DefaultSchedulerName and value if not set is the default scheduler.

Suggested change
// DefaultSchedulerName is the name of the pod scheduler for DevWorkspace pods.
// If not specified, the pod scheduler is set to the default scheduler.
DefaultSchedulerName string `json:"defaultSchedulerName,omitempty"`
// DefaultSchedulerName is the name of the pod scheduler for DevWorkspace pods.
// If not specified, the pod scheduler is set to the default scheduler on the cluster.
DefaultSchedulerName string `json:"defaultSchedulerName,omitempty"`

We should also consider renaming field to just schedulerName to match some other fields (ImagePullPolicy, StorageClassName, etc.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good, changed to schedulerName for now cc @ibuziuk

@amisevsk
Copy link
Collaborator

Deployment templates also need to be regenerated:

make generate manifests fmt generate_default_deployment generate_olm_bundle_yaml

@dkwon17
Copy link
Collaborator Author

dkwon17 commented Nov 14, 2022

Thanks for the review @amisevsk , I've made the changes and updated workspace.defaultSchedulerName to workspace.schedulerName

Copy link
Collaborator

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @dkwon17 :)

Copy link
Contributor

@ibuziuk ibuziuk left a comment

Choose a reason for hiding this comment

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

👍

@openshift-ci
Copy link

openshift-ci bot commented Nov 15, 2022

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@ibuziuk ibuziuk merged commit 9a339c7 into devfile:main Nov 17, 2022
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.

4 participants