Expose nodeSelector for operator and webhook#187
Open
faganihajizada wants to merge 1 commit into
Open
Conversation
a6377ed to
80f542c
Compare
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>
80f542c to
fedad2c
Compare
25 tasks
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.
1 task
25 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The Helm chart did not document or wire
nodeSelectorfor either the operator or webhook Deployment. Helm parsed--set operator.nodeSelector.foo=bar(and the webhook equivalent) into.Valueswithout complaint and silently dropped the value at render time, since no template referenced it and the chart has novalues.schema.jsonfor validation.This PR wires
.Values.operator.nodeSelectorand.Values.webhook.nodeSelectorthroughtemplates/operator/deployment.yamlandtemplates/webhook/deployment.yamlusing the same{{- with ... }}/toYaml | nindent 8idiom already used foraffinityandtolerations, documents the field invalues.yamlwith wording aligned to the siblinghelm/slurmchart, regenerates the matching rows in the values table ofhelm/slurm-operator/README.md, and adds helm-unittest cases that assert the rendered field for both deployments.Reproduction
Checklist
Breaking Changes
Not a breaking API change —
nodeSelectorwas 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.nodeSelectororwebhook.nodeSelectorin 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 existingshould add affinity/should add tolerationstests in bothtests/operator_deployment_test.yamlandtests/webhook_deployment_test.yaml, following the same shape and depth.