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

Support for namespace-level configuration of pod tolerations and node selector #20884

Closed
Tracked by #21008
skabashnyuk opened this issue Dec 6, 2021 · 9 comments
Closed
Tracked by #21008
Labels
kind/task Internal things, technical debt, and to-do tasks to be performed. new&noteworthy For new and/or noteworthy issues that deserve a blog post, new docs, or emphasis in release notes severity/P1 Has a major impact to usage or development of the system. sprint/next
Milestone

Comments

@skabashnyuk
Copy link
Contributor

skabashnyuk commented Dec 6, 2021

Is your task related to a problem? Please describe

As a Eclipse Che admin, I want to be able to set up the configuration of pod tolerations and node selector. Devworkspace operator expected them as k8s namespace annotations:

    metadata:
      annotations:
        controller.devfile.io/node-selector: |
          {
            "test-node-selector": "test"
          }
        controller.devfile.io/pod-tolerations: |
          [
            {
              "key": "key1",
              "operator": "Equal",
              "value": "value1",
              "effect": "NoSchedule"
            }
          ]

see more devfile/devworkspace-operator#696

Describe the solution you'd like

Che-operator should grab existing configuration from che-server's env var configuration CHE_WORKSPACE_POD_NODE__SELECTOR and CHE_WORKSPACE_POD_TOLERATIONS__JSON and put them in a format expected by devworkspace operator.

Describe alternatives you've considered

n/a

Release Notes Text

It is now possible to specify pod tolerations and node selector for workspaces of a Che instance setting a CustomCheProperty in CheCluster CR. It's not possible to specify that for just one workspace or for one user: it's a global config.

@skabashnyuk skabashnyuk added kind/task Internal things, technical debt, and to-do tasks to be performed. area/devworkspace-che-operator labels Dec 6, 2021
@skabashnyuk skabashnyuk changed the title Adds support for namespace-level configuration of pod tolerations and node selector via namespace annotations Add support for namespace-level configuration of pod tolerations and node selector via namespace annotations Dec 6, 2021
@ibuziuk ibuziuk added the severity/P1 Has a major impact to usage or development of the system. label Dec 6, 2021
@ibuziuk
Copy link
Member

ibuziuk commented Dec 6, 2021

@skabashnyuk we probably need to add the issue to #20830

@skabashnyuk
Copy link
Contributor Author

skabashnyuk commented Dec 16, 2021

The source of configuration now is che-server configuration: CHE_WORKSPACE_POD_NODE__SELECTOR and CHE_WORKSPACE_POD_TOLERATIONS__JSON. @l0rd do you think we should keep this source or it should be configured somehow differently?

CC @amisevsk @tolusha

@l0rd
Copy link
Contributor

l0rd commented Dec 16, 2021

If I look at the new CheCluster draft I would say:

spec:
  server:
    workspacePodNodeSelector:
    workspacePodTolerationsJson:

@amisevsk @tolusha what's your take on that?

@tolusha
Copy link
Contributor

tolusha commented Dec 16, 2021

What about moving workspace related configuration to a dedicated section?

spec:
   server:
     workspace:
       podNodeSelector:
       podTolerationsJson:

or:

spec:
   workspace:
     podNodeSelector:
     podTolerationsJson:

@amisevsk
Copy link
Contributor

podTolerationsJson

If we're doing configuration on a CR level, we can simply reuse the k8s types in the configurations struct, i.e. make podNodeSelector a map[string]string and podTolerations a []corev1.Toleration. Defining these things as strings is error-prone (as it has to be valid json/yaml and a missed comma breaks things).

I do think that configuring it in a CR is more straightforward and user-friendly. For DWO, moving to a config CRD instead of a configmap has given us

  1. Autocomplete in VSCode/the OpenShift Console editor
  2. Easily accessible documentation through oc explain

In terms of scoping, I don't have any real opinion on workspace vs server, but it is worth noting that storing this config in the CR binds it to forwards/backwards compatibiliity, so whatever we choose here we're stuck with for a long time.

@skabashnyuk
Copy link
Contributor Author

@l0rd do you still want to put configuration under /spec/server/ as described here #20884 (comment) or maybe /spec/workspace/ as @tolusha #20884 (comment) suggested is better?

@skabashnyuk skabashnyuk modified the milestones: 7.42, 7.43 Jan 10, 2022
@l0rd
Copy link
Contributor

l0rd commented Jan 10, 2022

I am ok with both. The important thing though is to be consistent: currently workspaces related fields are under spec.server. If we create a new spec.workspaces those fields should be moved to there too.

@metlos
Copy link
Contributor

metlos commented Jan 26, 2022

For CheCluster v1, I'd definitely avoid any restructuring, because that would be breaking the compatibility with CheCluster resources preexisting in the cluster. So for v1, I'd slot these under spec.server. For v2, we can pick any place we want.

@metlos metlos closed this as completed Feb 1, 2022
@l0rd l0rd added new&noteworthy For new and/or noteworthy issues that deserve a blog post, new docs, or emphasis in release notes status/release-notes-review-needed Issues that needs to be reviewed by the doc team for the Release Notes wording labels Feb 3, 2022
@l0rd l0rd changed the title Add support for namespace-level configuration of pod tolerations and node selector via namespace annotations Support for namespace-level configuration of pod tolerations and node selector Feb 3, 2022
@devstudio-release
Copy link

sync'd to Red Hat JIRA https://issues.redhat.com/browse/CRW-2711

@max-cx max-cx removed the status/release-notes-review-needed Issues that needs to be reviewed by the doc team for the Release Notes wording label Jan 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/task Internal things, technical debt, and to-do tasks to be performed. new&noteworthy For new and/or noteworthy issues that deserve a blog post, new docs, or emphasis in release notes severity/P1 Has a major impact to usage or development of the system. sprint/next
Projects
None yet
Development

No branches or pull requests

9 participants