Gpu resource driver chart improvements #76

rouke-broersma · 2025-03-29T20:06:04Z

The helm chart contains a lot of hard coded values, and in some places hard coded values that actual have configurable values in other places. I need these values configurable in my environment. Let me know if any changes are unwelcome.

Signed-off-by: Rouke Broersma <[email protected]>

…requires privileged Signed-off-by: Rouke Broersma <[email protected]>

Signed-off-by: Rouke Broersma <[email protected]>

…, use template functions instead Signed-off-by: Rouke Broersma <[email protected]>

Signed-off-by: Rouke Broersma <[email protected]>

…untName is sufficient Signed-off-by: Rouke Broersma <[email protected]>

… default helper functions instead Signed-off-by: Rouke Broersma <[email protected]>

rouke-broersma · 2025-03-29T20:07:40Z

charts/intel-gpu-resource-driver/templates/resource-driver-namespace.yaml

-  name: intel-gpu-resource-driver
+  name: {{ include "intel-gpu-resource-driver.namespace" . }}
+  labels:
+    pod-security.kubernetes.io/enforce: privileged


Privileged namespace security is required to mount host path files, so if the Chart managed the namespace it should by default set the required permissions. Let me know if this should be templated instead, I personally don't think so since it's a requirement for the pod to start.

Privileged namespace security is required to mount host path files

Could you expand a bit on why, where that applies?

See https://kubernetes.io/docs/concepts/security/pod-security-standards/

Pod Security Standards is the replacement for Pod Security Policy. It defines three standard security levels (privileged, baseline and restricted). When Pod Security Standards is enabled on a namespace, Host Path volume mounts are only allowed in the privileged profile.

All volumes mounted by the intel DRA driver are host path volumes:

volumes: - name: plugins-registry hostPath: path: /var/lib/kubelet/plugins_registry - name: plugins hostPath: path: /var/lib/kubelet/plugins - name: cdi hostPath: path: {{ .Values.cdi.staticPath }} - name: varruncdi hostPath: path: {{ .Values.cdi.dynamicPath}} - name: sysfs hostPath: path: /sys

So any cluster using Pod Security Standards (a default component of Kubernetes) will require this annotation, otherwise admission control will deny the daemonset.

Thanks, that's good to know!

The annotation seems to be documented e.g. here: https://kubernetes.io/docs/tasks/configure-pod-container/enforce-standards-namespace-labels/

EDIT: please resolve.

rouke-broersma · 2025-03-29T20:08:05Z

charts/intel-gpu-resource-driver/templates/node-feature-rules.yaml

@@ -1,4 +1,4 @@
-{{- if .Values.nfd.enabled }}
+{{- if or .Values.nodeFeatureRules.enabled .Values.nfd.enabled }}


So node feature rules can be deployed when you already have an existing NFD running in the cluster.

rouke-broersma · 2025-03-29T20:08:45Z

charts/intel-gpu-resource-driver/values.yaml

+cdi:
+  staticPath: /etc/cdi
+  dynamicPath: /var/run/cdi


My environment requires alternative paths for cdi, so I need to be able to configure the host paths

oxxenix · 2025-03-31T09:01:21Z

@byako, @eero-t PTAL

eero-t · 2025-03-31T13:41:01Z

charts/intel-gpu-resource-driver/templates/resource-driver-namespace.yaml

-  name: intel-gpu-resource-driver
+  name: {{ include "intel-gpu-resource-driver.namespace" . }}
+  labels:
+    pod-security.kubernetes.io/enforce: privileged


Privileged namespace security is required to mount host path files

Could you expand a bit on why, where that applies?

charts/intel-gpu-resource-driver/templates/validating-admission-policy.yaml

charts/intel-gpu-resource-driver/templates/resource-driver.yaml

eero-t · 2025-03-31T13:49:36Z

charts/intel-gpu-resource-driver/values.yaml

@@ -1,6 +1,6 @@
 # Default values for intel-gpu-resource-driver.
 nameOverride: ""
-namespaceOverride: "intel-gpu-resource-driver"
+namespaceOverride: ""


Why you're removing the current default?

You have an existing function:

{{- define "intel-gpu-resource-driver.namespace" -}} {{- default .Release.Namespace .Values.namespaceOverride }} {{- end }}

If you set a namespaceoverride by default, this function is useless. Namespace override should be used when you want to not use the default.

The default function is now correctly used.

At least you need to add the necessary Helm options to README, so that users do not install it to default ns by default, see: https://github.com/intel/intel-resource-drivers-for-kubernetes/blob/main/doc/gpu/USAGE.md#helm-charts

eero-t · 2025-03-31T13:49:43Z

charts/intel-gpu-resource-driver/values.yaml

@@ -14,14 +14,12 @@ image:
 serviceAccount:
  create: true
  annotations: {}
-  name: intel-gpu-resource-driver-service-account
+  name: ""


Why you're removing the current default?

Because you have a default function that you're not fully utilizing:

{{- define "intel-gpu-resource-driver.serviceAccountName" -}} {{- if .Values.serviceAccount.create -}} {{- default "intel-gpu-sa" .Values.serviceAccount.name -}} {{- end -}} {{- end }}

If you want I can set the default in the default helper function to the previous value. It doesn't really matter, the service account name is now fully generated using the template helpers in all usages.

Yes, updating the helper default would make this change clearer in the commit diff.

eero-t · 2025-03-31T13:56:00Z

charts/intel-gpu-resource-driver/values.yaml

+nodeFeatureRules:
+  enabled: false
+
 nfd:
  enabled: false # change to true to install NFD to the cluster


NFD settings should be under same heading[1], e.g:

nfdRules: use: true install: true

Then you can use just:
{{- if .Values.nfdRules.use }}
instead of:
{{- if or .Values.nodeFeatureRules.enabled .Values.nfd.enabled }}

[1] If Helm variable names are changed, they should be changed consistently for all 3 Intel DRA helm charts (gaudi, gpu, qat), not just GPU one.

You pass all values under the nfd key through to the nfd helm chart, so I did not want to potentially break something by passing nonexisting values through to nfd. I added a new key for backwards compatibility with the previous behavior. Let me know if this is fine or if you want a breaking change.

If you want to keep all three helm charts consistent, have you considered making it one chart?

Good point. Having 2 separate variable hierarchies for NFD is rather ugly though => @oxxenix do you see problems in adding/changing variables under nfd?

If you want to keep all three helm charts consistent, have you considered making it one chart?

Not really my decision as I'm not involved with this project currently, except for doing a bit of review for the PRs. @byako, any comments on this?

I'm fine with this proposal.

byako

Thanks for the contribution!
FYI - we're migrated to use GH OCI-registry for Helm charts: https://github.com/orgs/intel/packages?repo_name=intel-resource-drivers-for-kubernetes , and the chart source is moving (back to where it was originally) to the DRA drivers repo, so our charts in this repository are getting deprecated. I can apply this PR to the helm chart in the DRA drivers repo later this week when we do the release.

byako · 2025-04-01T10:45:18Z

charts/intel-gpu-resource-driver/values.yaml

+nodeFeatureRules:
+  enabled: false
+
 nfd:
  enabled: false # change to true to install NFD to the cluster


I'm fine with this proposal.

rouke-broersma · 2025-04-01T11:54:05Z

@byako just to confirm is anything still needed from me at this point or do you take care of any remaining changes when you move the chart? Happy to help, just unsure what's left.

byako · 2025-04-01T12:27:37Z

@rouke-broersma nothing is needed, I can update the Helm doc section when pulling this into the repo.

rouke-broersma added 7 commits March 29, 2025 20:37

Namespace should use namespace template

3230139

Signed-off-by: Rouke Broersma <[email protected]>

Add pod security label to resource driver namespace because hostpath …

e2ab5d8

…requires privileged Signed-off-by: Rouke Broersma <[email protected]>

nfd: Allow deploying node feature rules without deploying nfd

4722f57

Signed-off-by: Rouke Broersma <[email protected]>

Validating admission policy should not have hardcoded service account…

9574294

…, use template functions instead Signed-off-by: Rouke Broersma <[email protected]>

Make cdi spec dirs paths configurable

9665cb4

Signed-off-by: Rouke Broersma <[email protected]>

serviceAccount is deprecated and should not be hardcoded, serviceAcco…

7bbd7e5

…untName is sufficient Signed-off-by: Rouke Broersma <[email protected]>

Remove default values for namespaceOverride and serviceAccount to use…

63df78c

… default helper functions instead Signed-off-by: Rouke Broersma <[email protected]>

rouke-broersma requested a review from softwarerecipes as a code owner March 29, 2025 20:06

rouke-broersma commented Mar 29, 2025

View reviewed changes

mythi requested a review from oxxenix March 31, 2025 05:04

oxxenix removed the request for review from softwarerecipes March 31, 2025 09:01

eero-t reviewed Mar 31, 2025

View reviewed changes

byako approved these changes Apr 1, 2025

View reviewed changes

eero-t approved these changes Apr 10, 2025

View reviewed changes

byako mentioned this pull request Apr 14, 2025

Update gpu chart, pull intel/intel-resource-drivers-for-kubernetes#29

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gpu resource driver chart improvements #76

Gpu resource driver chart improvements #76

rouke-broersma commented Mar 29, 2025

rouke-broersma Mar 29, 2025

eero-t Mar 31, 2025

rouke-broersma Mar 31, 2025

eero-t Mar 31, 2025 •

edited

Loading

rouke-broersma Mar 29, 2025

rouke-broersma Mar 29, 2025

oxxenix commented Mar 31, 2025

eero-t Mar 31, 2025

eero-t Mar 31, 2025

rouke-broersma Mar 31, 2025

eero-t Mar 31, 2025

eero-t Mar 31, 2025

rouke-broersma Mar 31, 2025

eero-t Mar 31, 2025

eero-t Mar 31, 2025

rouke-broersma Mar 31, 2025

eero-t Mar 31, 2025

byako Apr 1, 2025

byako left a comment

byako Apr 1, 2025

rouke-broersma commented Apr 1, 2025

byako commented Apr 1, 2025

		@@ -1,4 +1,4 @@
		{{- if .Values.nfd.enabled }}
		{{- if or .Values.nodeFeatureRules.enabled .Values.nfd.enabled }}

Gpu resource driver chart improvements #76

Are you sure you want to change the base?

Gpu resource driver chart improvements #76

Conversation

rouke-broersma commented Mar 29, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eero-t Mar 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxxenix commented Mar 31, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

byako left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rouke-broersma commented Apr 1, 2025

byako commented Apr 1, 2025

eero-t Mar 31, 2025 •

edited

Loading