Skip to content

Expose nodeSelector for operator and webhook#187

Open
faganihajizada wants to merge 1 commit into
SlinkyProject:mainfrom
faganihajizada:fhajizada/charts-nodeselector
Open

Expose nodeSelector for operator and webhook#187
faganihajizada wants to merge 1 commit into
SlinkyProject:mainfrom
faganihajizada:fhajizada/charts-nodeselector

Conversation

@faganihajizada
Copy link
Copy Markdown
Contributor

@faganihajizada faganihajizada commented Apr 30, 2026

Summary

The Helm chart did not document or wire nodeSelector for either the operator or webhook Deployment. Helm parsed --set operator.nodeSelector.foo=bar (and the webhook equivalent) into .Values without complaint and silently dropped the value at render time, since no template referenced it and the chart has no values.schema.json for validation.

This PR wires .Values.operator.nodeSelector and .Values.webhook.nodeSelector through templates/operator/deployment.yaml and templates/webhook/deployment.yaml using the same {{- with ... }} / toYaml | nindent 8 idiom already used for affinity and tolerations, documents the field in values.yaml with wording aligned to the sibling helm/slurm chart, regenerates the matching rows in the values table of helm/slurm-operator/README.md, and adds helm-unittest cases that assert the rendered field for both deployments.

Reproduction

helm template slinky-slurm-op helm/slurm-operator \
  --set operator.nodeSelector.foo=bar \
  --set webhook.nodeSelector.baz=qux | grep nodeSelector
# (no output — value silently dropped)

Checklist

  • I have read CONTRIBUTING.md and the Code of Conduct.
  • New or existing tests cover these changes (where applicable).
  • Documentation is updated if user-visible behavior changes.

Breaking Changes

Not a breaking API change — nodeSelector was never a documented or supported field on either deployment, and default renders are unchanged (the field is omitted unless the user sets it).

There is a latent behavior change worth flagging: any operator who had been setting operator.nodeSelector or webhook.nodeSelector in their values file under the assumption it was being honored will now see the constraint take effect. They should verify their value does not pin pods to nodes that no longer match.

Testing Notes

The new helm-unittest cases (should add nodeSelector) sit alongside the existing should add affinity / should add tolerations tests in both tests/operator_deployment_test.yaml and tests/webhook_deployment_test.yaml, following the same shape and depth.

@faganihajizada faganihajizada force-pushed the fhajizada/charts-nodeselector branch 3 times, most recently from a6377ed to 80f542c Compare April 30, 2026 18:43
The slurm-operator Helm chart did not document or wire `nodeSelector` for
either the operator or webhook Deployment. Helm parsed values such as
`--set operator.nodeSelector.foo=bar` into `.Values` without complaint
and silently dropped them at render time, since no template referenced
them and the chart has no `values.schema.json` to validate against.

Changelog: Added - Helm chart fields `operator.nodeSelector` and `webhook.nodeSelector` for `slurm-operator`, mirroring the existing `affinity` and `tolerations` fields.
Signed-off-by: Fagani Hajizada <fhajizada@nvidia.com>
@faganihajizada faganihajizada force-pushed the fhajizada/charts-nodeselector branch from 80f542c to fedad2c Compare April 30, 2026 18:44
@vivian-hafener vivian-hafener self-assigned this Apr 30, 2026
faganihajizada added a commit to faganihajizada/aicr that referenced this pull request May 13, 2026
…slurm

Adds the Slinky Slurm operator (chart v1.1.0 from
oci://ghcr.io/slinkyproject/charts) as a first-class AICR platform under
`--platform slurm`. Ships two components: `slinky-slurm-operator-crds`
and `slinky-slurm-operator`, composed by a new `platform-slurm` mixin.
Operator depends on cert-manager and the CRDs.

Mirrors the operator-only pattern of `--platform dynamo` and
`--platform kubeflow`: AICR ships only the platform; cluster-instance
CRs (Controller, LoginSet, NodeSet, ...) remain user-authored. Leaf
overlays (CSP x accelerator x OS) follow in a separate PR.

Notable: chart v1.1.0 silently ignores operator.nodeSelector and
webhook.nodeSelector. The registry comment links
SlinkyProject/slurm-operator#187 with the
verbatim follow-up action once that upstream PR merges and a new chart
is published. The `slurm` cluster chart is intentionally deferred to a
follow-up PR with its own short alias (e.g. `slurm-cluster`) so `slurm`
continues to route to the operator.

Verified: make qualify (test-coverage + lint + e2e + scan +
license-check) PASS; make check-health COMPONENT=slinky-slurm-operator
against a live Kind cluster + chart v1.1.0 PASS in 5.42s.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants