Skip to content

enable device plugin specific node selector #1467

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

enable device plugin specific node selector #1467

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion deployments/helm/nvidia-device-plugin/templates/daemonset-device-plugin.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
{{- $useServiceAccount := $options.hasConfigMap }}
{{- $configMapName := (include "nvidia-device-plugin.configMapName" .) | trim }}
{{- $daemonsetName := printf "%s" (include "nvidia-device-plugin.fullname" .) | trunc 63 | trimSuffix "-" }}
{{- $devicePluginNodeSelector := merge .Values.devicePlugin.nodeSelector .Values.nodeSelector }}
Copy link
Member

@elezar elezar Oct 31, 2025

Choose a reason for hiding this comment

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

Is merging what we want here? Should we not use devicePlugin.nodeSelector if specified and fallback to the top-level nodeSelector if it is not.

Copy link
Author

@omrishiv omrishiv Oct 31, 2025

Choose a reason for hiding this comment

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

let me give a better example of what we have:

- node 1 node 2 node 3
accelerator nvidia nvidia aws
ami al2023 bottlerocket bottlerocket
  • If we don't have any nodeselectors, we get pods scheduled on all nodes, but we don't need it on nodes 2 and 3
  • If we have nodeSelector for accelerator: nvidia, we get it on 1 and 2, but we don't need it on 2.

We do need it on 1, but we don't want it to schedule on any other node. This attempts to maintain backwards compatibility while filtering out nodes that don't need it. The idea is having a global selector (accelerator) and filtering using a device plugin specific one (ami)

apiVersion: apps/v1
kind: DaemonSet
metadata:
Expand Down Expand Up @@ -255,7 +256,7 @@ spec:
- name: config
emptyDir: {}
{{- end }}
{{- with .Values.nodeSelector }}
{{- with $devicePluginNodeSelector }}
nodeSelector:
{{- toYaml . | nindent 8 }}
{{- end }}
Expand Down
1 change: 1 addition & 0 deletions deployments/helm/nvidia-device-plugin/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ runtimeClassName: null

devicePlugin:
enabled: true
nodeSelector: {}
Copy link
Member

@elezar elezar Oct 31, 2025

Choose a reason for hiding this comment

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

My one question would be whether we wanted to do this for other top-level placement selectors such as affinity and also do the same for gfd?

Copy link
Author

@omrishiv omrishiv Oct 31, 2025

Choose a reason for hiding this comment

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

gfd is useful for all nvidia nodes, so I didn't see a need to extend it, but I'm happy to add it so it's usable if you want it


gfd:
enabled: false
Expand Down