Add DRA driver for IMEX - #1143
cdesiniotis wants to merge 3 commits into
Conversation
| } | ||
|
|
||
| // DRADriverSpec defines the properties for the NVIDIA DRA Driver deployment | ||
| // TODO: add 'controller' and 'kubeletPlugin' structs to allow for per-component configuration |
There was a problem hiding this comment.
One question: Should we expose controller and kubeletPlugin as concepts to the user? These seem like internal details and including them here couples the operator and the DRA driver implementation more tightly.
There was a problem hiding this comment.
I don't believe we should at this point in time. But I can see where having the ability to control the controller / kubeletPlugin configuration independently could be useful. For example, one may need to bump the cpu / mem resources for the controller (and not the plugin) to account for larger sized clusters. Open to continue discussion on this and enumerate the list of fields we want to expose in Clusterpolicy.
| metadata: | ||
| name: nvidia-dra-driver | ||
| rules: | ||
| # TODO: restrict RBAC for DRA driver |
There was a problem hiding this comment.
Has this been done upstream yet?
There was a problem hiding this comment.
There was a problem hiding this comment.
I see @guptaNswati is working on restricting the RBAC upstream: kubernetes-sigs/dra-driver-nvidia-gpu#219. I will pull in those changes once they are finalized.
| - name: controller | ||
| image: "FILLED BY THE OPERATOR" | ||
| imagePullPolicy: IfNotPresent | ||
| command: ["nvidia-dra-controller", "-v", "6"] |
There was a problem hiding this comment.
Should we be setting the verbosity here?
There was a problem hiding this comment.
I don't have a strong opinion. I just copied the flags passed in the upstream DRA helm chart: https://github.com/NVIDIA/k8s-dra-driver/blob/32805fec62d6b3269e75ddb0eddeaa5c4d214564/deployments/helm/k8s-dra-driver/templates/controller.yaml#L55
| set -o allexport | ||
| cat /run/nvidia/validations/driver-ready | ||
| . /run/nvidia/validations/driver-ready | ||
| # TODO: add an alias for DRIVER_ROOT_CTR_PATH in the k8s-dra-driver and remove the below export |
There was a problem hiding this comment.
| - name: DEVICE_CLASSES | ||
| value: imex |
There was a problem hiding this comment.
Is it expected that the DEVICE_CLASSES for the controller and the plugin match?
There was a problem hiding this comment.
We can set from one of these gpu, mig and imex and plugin will automatically pick it up
-
name: DEVICE_CLASSES
value: {{ .Values.deviceClasses | join "," }}here, it seems we are only doing IMEX by default.
| - name: MASK_NVIDIA_DRIVER_PARAMS | ||
| value: "false" |
There was a problem hiding this comment.
Question: Can a user still override this if required?
There was a problem hiding this comment.
Yes. The user can override this variable in Clusterpolicy with draDriver.env.
| - mountPath: /var/lib/kubelet/plugins_registry | ||
| name: plugins-registry | ||
| - mountPath: /var/lib/kubelet/plugins | ||
| mountPropagation: Bidirectional |
There was a problem hiding this comment.
Question: Is bidirectional mount propagation really needed in the plugins folder?
There was a problem hiding this comment.
I am not sure. I copied this from the upstream DRA helm chart: https://github.com/NVIDIA/k8s-dra-driver/blob/32805fec62d6b3269e75ddb0eddeaa5c4d214564/deployments/helm/k8s-dra-driver/templates/kubeletplugin.yaml#L99
@klueska do you know if this is required?
| NvidiaCtrRuntimeCDIPrefixesEnvName = "NVIDIA_CONTAINER_RUNTIME_MODES_CDI_ANNOTATION_PREFIXES" | ||
| // CDIEnabledEnvName is the name of the envvar used to enable CDI in the operands | ||
| CDIEnabledEnvName = "CDI_ENABLED" | ||
| // NvidiaCTKHookPathEnvName is the name of the envvar specifying the path to the 'nvidia-ctk' binary |
There was a problem hiding this comment.
Note we shouldn't need the nvidia-ctk path in addition to the nvidia-cdi-hook path. They can be used interchangeably.
There was a problem hiding this comment.
Updated the comment. Will remove this once kubernetes-sigs/dra-driver-nvidia-gpu#210 is complete.
| } | ||
|
|
||
| // TransformDRADriverPlugin transforms nvidia-dra-driver-plugin daemonset with required config as per ClusterPolicy | ||
| func TransformDRADriverPlugin(obj *appsv1.DaemonSet, config *gpuv1.ClusterPolicySpec, n ClusterPolicyController) error { |
There was a problem hiding this comment.
Out of scope for this PR: How much merit is there in refactoring these Transform* functions to strip out the common logic that is performed for all containers?
There was a problem hiding this comment.
There is definitely some merit. We currently apply some common transforms for all DaemonSets here before calling the individual Transform* functions:
gpu-operator/controllers/object_controls.go
Lines 718 to 729 in 58b1954
We could strip out more, but I believe the main issue is that each Transform* function is reading configuration from a different data type. E.g. TransformDRADriverPlugin reads from a struct of type DRADriverSpec while TransformDriver reads from a struct of type DriverSpec.
| } | ||
|
|
||
| if config.Toolkit.IsEnabled() { | ||
| setContainerEnv(&(obj.Spec.Template.Spec.Containers[0]), NvidiaCTKPathEnvName, filepath.Join(config.Toolkit.InstallDir, "toolkit/nvidia-ctk")) |
There was a problem hiding this comment.
Let's rather update the driver to also use the nvidia-cdi-hook path. The following should be sufficient:
| setContainerEnv(&(obj.Spec.Template.Spec.Containers[0]), NvidiaCTKPathEnvName, filepath.Join(config.Toolkit.InstallDir, "toolkit/nvidia-ctk")) | |
| setContainerEnv(&(obj.Spec.Template.Spec.Containers[0]), NvidiaCTKPathEnvName, filepath.Join(config.Toolkit.InstallDir, "toolkit/nvidia-cdi-hook")) |
There was a problem hiding this comment.
Created kubernetes-sigs/dra-driver-nvidia-gpu#210 as a follow-up.
| return nil | ||
| } | ||
|
|
||
| func transformDeployment(obj *appsv1.Deployment, n ClusterPolicyController) error { |
There was a problem hiding this comment.
Question: Why is this not a function defined on a ClusterPolicyController?
There was a problem hiding this comment.
No particular reason other than following the precedent we have with similar transform functions we have in this file. Is there a good reason for defining this as a method instead?
| imagePullPolicy: IfNotPresent | ||
| command: ["nvidia-dra-controller", "-v", "6"] | ||
| env: | ||
| - name: DEVICE_CLASSES |
There was a problem hiding this comment.
are we only doing imex? not mig? we need to do some error handling in case when imex daemon is not running. rn we always expect the imex daemon to be running.
There was a problem hiding this comment.
The scope of this PR is to only enable IMEX.
I am not sure what type of error handling you are envisioning, but I believe we need to ensure that the DRA driver daemonset only ever gets scheduled on nodes that are in an IMEX domain. As is, my PR deploys the DRA driver daemonset on all GPU nodes, regardless if they are in an IMEX domain. I can look into updating the nodeAffinity to leverage the IMEX domain label that GFD adds.
There was a problem hiding this comment.
I have updated the nodeAffinity such that the IMEX DRA driver kubelet plugin only gets scheduled on nodes labeled with nvidia.com/gpu.imex-domain
| - name: DEVICE_CLASSES | ||
| value: imex |
There was a problem hiding this comment.
We can set from one of these gpu, mig and imex and plugin will automatically pick it up
-
name: DEVICE_CLASSES
value: {{ .Values.deviceClasses | join "," }}here, it seems we are only doing IMEX by default.
| type: DirectoryOrCreate | ||
| - name: driver-install-dir | ||
| hostPath: | ||
| path: "/run/nvidia/driver" |
There was a problem hiding this comment.
we are still doing host managed drivers for imex, so we need override this path to / with --set driver.enabled=false
There was a problem hiding this comment.
We mount both /run/nvidia/driver and the host root / into the container, to account for both the driver container and host-managed driver scenarios. In the case of host-managed drivers, we set the DRIVER_CTR_ROOT_PATH envvar to /host (this is the container path) during the container entrypoint script.
|
|
||
| draDriver: | ||
| enabled: true | ||
| repository: ghcr.io/nvidia |
There was a problem hiding this comment.
should we mirror it to nvcr.io similar to other images and may be retag with semvar versioning
There was a problem hiding this comment.
Yes, this is a placeholder for now. We will update this once we publish the DRA driver image to nvcr.io
| // +operator-sdk:gen-csv:customresourcedefinitions.specDescriptors=true | ||
| // +operator-sdk:gen-csv:customresourcedefinitions.specDescriptors.displayName="Resource Requirements" | ||
| // +operator-sdk:gen-csv:customresourcedefinitions.specDescriptors.x-descriptors="urn:alm:descriptor:com.tectonic.ui:advanced,urn:alm:descriptor:com.tectonic.ui:resourceRequirements" | ||
| Resources *ResourceRequirements `json:"resources,omitempty"` |
There was a problem hiding this comment.
Question -- should we include the resources, args, and env fields at this point in time?
There was a problem hiding this comment.
Since the IMEX DRA Driver consists of a controller and kubeletPlugin, it feels as if I should update this so that users can configure each component independently.
controller:
resources: {}
env: []
kubeletPlugin:
resources: {}
env: []
bfff07b to
2f5127f
Compare
Signed-off-by: Christopher Desiniotis <cdesiniotis@nvidia.com>
76894a2 to
6bbbd94
Compare
6bbbd94 to
6fad4fb
Compare
Signed-off-by: Christopher Desiniotis <cdesiniotis@nvidia.com>
Signed-off-by: Christopher Desiniotis <cdesiniotis@nvidia.com>
6fad4fb to
54e1b59
Compare
| DevicePlugin DevicePluginSpec `json:"devicePlugin"` | ||
| // DRADriver component spec | ||
| DRADriver DRADriverSpec `json:"draDriver"` | ||
| IMEXDRADriver IMEXDRADriverSpec `json:"imexDRADriver"` |
There was a problem hiding this comment.
This approach seems better.
| if d.Enabled == nil { | ||
| // default is true if not specified by user | ||
| return true |
There was a problem hiding this comment.
You've made it enabled by default. Does it mean that IMEX is common/expected to be widely used?
|
|
||
| // IMEXDRADriverSpec defines the properties for the NVIDIA IMEX DRA Driver deployment | ||
| // TODO: add 'controller' and 'kubeletPlugin' structs to allow for per-component configuration | ||
| type IMEXDRADriverSpec struct { |
There was a problem hiding this comment.
If the DRA driver will eventually manage other resources, not just IMEX channels, it will be difficult to move away from the IMEX-specific settings for a DRA image - because of backward compatibility. If it's something that's going to evolve (I just don't know), I'd probably have a general DRA config (controller image, etc.), and under that a section that would control what kinds of resources (classes?) and how the DRA driver could manage. IMEX channels in this case.
|
Closing in favor of #1541 |
4 participants
No description provided.