feat: add affinity and nodeSelector to common daemonset config - #2645
feat: add affinity and nodeSelector to common daemonset config#2645amarkdotdev wants to merge 2 commits into
Conversation
f0aa669 to
530835d
Compare
530835d to
be752b6
Compare
|
Hey, this is waiting on CI workflow approval / first-time contributor vetting. Could a maintainer kick that off when convenient? Thanks! |
|
/ok-to-test be752b6 |
be752b6 to
9efb8b7
Compare
|
@rajathagasthya Fixed the generated GPUCluster CRDs and rebased the branch onto upstream/main. |
|
Thanks @amarkdotdev! We are in a bit of a code freeze right now for an upcoming release, so we're only taking release-critical PRs. You can expect this PR to get attention once the release is done. |
|
@rajathagasthya ok no problem:) |
Expose spec.daemonsets.affinity and spec.daemonsets.nodeSelector on ClusterPolicy and wire them through Helm. User nodeSelector labels are merged with existing operand nodeSelector entries. Use a schemaless CRD field for affinity so we do not embed the full corev1.Affinity OpenAPI schema in the ClusterPolicy CRD. Signed-off-by: amarkdotdev <amarkdotdev@users.noreply.github.com> # Conflicts: # controllers/transforms_test.go Signed-off-by: Aaron <amark@g.jct.ac.il> Signed-off-by: Aaron <amark@g.jct.ac.il>
Regenerate published GPUCluster CRDs for the common daemonset affinity and nodeSelector API fields. Signed-off-by: amarkdotdev <amarkdotdev@users.noreply.github.com> Signed-off-by: Aaron <amark@g.jct.ac.il>
9efb8b7 to
aebd56c
Compare
📝 WalkthroughWalkthrough
Merge Risk: 🟡 Moderate · up to User-provided nodeSelector values can currently overwrite protected GPU Operator selectors, potentially placing daemonset operands on unsuitable nodes or preventing expected GPU/NFD targeting. Merge should wait until those protected selectors remain authoritative. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 29c358f5-21e9-441c-8a35-005c6985e51e
⛔ Files ignored due to path filters (7)
api/nvidia/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated.*.gobundle/manifests/nvidia.com_clusterpolicies.yamlis excluded by!bundle/manifests/nvidia.com_*.yamlbundle/manifests/nvidia.com_gpuclusters.yamlis excluded by!bundle/manifests/nvidia.com_*.yamlconfig/crd/bases/nvidia.com_clusterpolicies.yamlis excluded by!config/crd/bases/**config/crd/bases/nvidia.com_gpuclusters.yamlis excluded by!config/crd/bases/**deployments/gpu-operator/crds/nvidia.com_clusterpolicies.yamlis excluded by!deployments/gpu-operator/crds/**deployments/gpu-operator/crds/nvidia.com_gpuclusters.yamlis excluded by!deployments/gpu-operator/crds/**
📒 Files selected for processing (5)
api/nvidia/v1/clusterpolicy_types.gocontrollers/object_controls.gocontrollers/transforms_test.godeployments/gpu-operator/templates/clusterpolicy.yamldeployments/gpu-operator/values.yaml
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.
| obj.Spec.Template.Spec.NodeSelector = make(map[string]string) | ||
| } | ||
| for key, value := range config.Daemonsets.NodeSelector { | ||
| obj.Spec.Template.Spec.NodeSelector[key] = value |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve protected operand node selectors.
Line 837 lets a user value overwrite an existing nvidia.com/gpu.deploy.* selector. For example, nvidia.com/gpu.deploy.device-plugin: "false" replaces the required "true" value and can schedule the operand outside its intended GPU/NFD selection.
controllers/object_controls.go#L837-L837: Skip user overrides fornvidia.com/gpu.deploy.*keys so operand selectors remain authoritative.controllers/transforms_test.go#L720-L734: Add a conflicting protected-key case and assert that the original operand selector value remains unchanged.
📍 Affects 2 files
controllers/object_controls.go#L837-L837(this comment)controllers/transforms_test.go#L720-L734
Source: Path instructions
|
rebased |
2 participants
Fixes #2626
ClusterPolicy already exposes common tolerations for operand daemonsets but there was no matching knob for affinity or nodeSelector. This adds both under
spec.daemonsetsand surfaces them in Helm asdaemonsets.affinityanddaemonsets.nodeSelector.User nodeSelector labels are merged into the operand nodeSelector, so the existing
nvidia.com/gpu.deploy.*labels stay put.The affinity CRD field is schemaless so we accept a normal Kubernetes affinity object without inlining the full OpenAPI schema into the ClusterPolicy CRD.
Testing