Support non-standard driver installs #666

elezar · 2024-04-19T12:26:47Z

Thes changes add support to the NVIDIA GPU Device Plugin for driver
installations that have the following properties:

driver files are installed at a hostDriverRoot, say: /host/nvidia/driver
device nodes are created at /dev

In addition to the mounts of driver libraries (e.g. libnvidia-ml.so.VERSION)
and device nodes that are handled by the NVIDIA Container Runtime or CDI to
allow the device plugin to detect and enumerate devices, the hostDriverRoot
is mounted into the device plugin at /driver-root. This allows the detection
of driver files that are not required for the device plugin to fuction, but may
be required by specific workloads.

From the perspective of the CDI spec generation being run in the device plugin
container, all driver files are rooted at /driver-root. Furthermore, since
no device nodes are detected at /driver-root/dev we assume that these are
at /dev in the container (and on the host). The generated CDI specifications
need to be transformed so that they are valid for container workloads started
on the host. This means that occurrences of /driver-root need to be
transformed to hostDriverRoot or /host/nvidia/driver as per our example.

cdesiniotis · 2024-05-06T18:08:24Z

internal/cdi/cdi.go

+	infolib := info.New(info.WithRoot(c.driverRoot))
+	hasNVML, _ := infolib.HasNvml()
+	if !hasNVML {
+		klog.Warning("No valid resources detected, creating a null CDI handler")


nit: should we mention that NVML was not found in the warning message here?

cdesiniotis · 2024-05-06T18:10:55Z

internal/cdi/cdi.go

@@ -66,6 +69,18 @@ func newHandler(opts ...Option) (Interface, error) {
 	if !c.deviceListStrategies.IsCDIEnabled() {
 		return &null{}, nil
 	}
+	if c.driverRoot == "" {


I do not see us setting a default value for devRoot if it is not explicitly set by the caller.

Yes, that's probably better to do, but is redundant since we 1. set it in the caller, and 2. also set it in the CDI api. Let me clean it up regardless.

cdesiniotis · 2024-05-06T18:13:10Z

internal/cdi/cdi.go

+		// The transformation above may change the required spec version.
+		// TODO: This should be integrated into the transformer.
+		minVersion, err := cdiapi.MinimumRequiredVersion(spec.Raw())
+		if err != nil {
+			return fmt.Errorf("failed to get minimum required CDI spec version: %v", err)
+		}
+		spec.Raw().Version = minVersion
+


Question: Is this change required for this PR? This doesn't appear to be related to the devRoot changes.

The reason this is required is that devices with different host paths to device paths require a v0.5.0 spec. This means that the transform changes the initial spec version. I will look at how we can handle this through transformer options instead.

api/config/v1/flags.go

internal/plugin/server.go

cdesiniotis · 2024-05-09T22:25:37Z

cmd/nvidia-device-plugin/plugin-manager.go

@@ -46,7 +51,8 @@ func NewPluginManager(config *spec.Config) (manager.Interface, error) {

 	cdiHandler, err := cdi.New(
 		cdi.WithDeviceListStrategies(deviceListStrategies),
-		cdi.WithDriverRoot(*config.Flags.Plugin.ContainerDriverRoot),
+		cdi.WithDriverRoot(string(driverRoot)),
+		cdi.WithDevRoot(driverRoot.getDevRoot()),


Question -- why are we not using the NVIDIA_DEVICE_ROOT option here that was added to the device plugin CLI? From what I gather, driverRoot.getDevRoot() will always resolve to either driverRoot or /. What if a custom dev root is configured that is neither of these paths?

The devRoot from the context of CDI in this case is the dev root in the container. This is determined from the driverRoot (in the container) by checking if {{ .driverRoot }}/dev exists and using that, otherwise using /dev.

Got it. Let's keep this as is for now.

One thing to consider in the context of the GPU Operator -- I currently set DEV_ROOT_CTR_PATH in the driver-ready status file, to indicate the container path of the device root. If it makes sense to do so, we could consider leveraging this here in the device-plugin (and other operands).

One thought that I had was that we could set this in the management CDI spec. Will have to check what that would mean though.

elezar · 2024-06-04T12:43:05Z

cmd/nvidia-device-plugin/plugin-manager.go

 		cdi.WithTargetDriverRoot(*config.Flags.NvidiaDriverRoot),
+		cdi.WithTargetDevRoot(*config.Flags.NvidiaDevRoot),


Note that the TargetDevRoot is the dev root on the host. This is set to NvidiaDevRoot.

cdesiniotis

LGTM

cdesiniotis · 2024-06-15T20:39:59Z

api/config/v1/strategy.go

 	for k, v := range s {
 		if strings.HasPrefix(k, "cdi-") && v {
 			return true
 		}
 	}
 	return false
 }
+
+// IsOnlyCDIEnabled returns whether all strategies being used require CDI.


Suggested change

// IsOnlyCDIEnabled returns whether all strategies being used require CDI.

// AllCDIEnabled returns whether all strategies being used require CDI.

cdesiniotis · 2024-06-15T20:49:52Z

cmd/nvidia-device-plugin/plugin-manager.go

@@ -46,7 +51,8 @@ func NewPluginManager(config *spec.Config) (manager.Interface, error) {

 	cdiHandler, err := cdi.New(
 		cdi.WithDeviceListStrategies(deviceListStrategies),
-		cdi.WithDriverRoot(*config.Flags.Plugin.ContainerDriverRoot),
+		cdi.WithDriverRoot(string(driverRoot)),
+		cdi.WithDevRoot(driverRoot.getDevRoot()),


Got it. Let's keep this as is for now.

One thing to consider in the context of the GPU Operator -- I currently set DEV_ROOT_CTR_PATH in the driver-ready status file, to indicate the container path of the device root. If it makes sense to do so, we could consider leveraging this here in the device-plugin (and other operands).

Signed-off-by: Evan Lezar <[email protected]>

This change adds a nvidaDevRoot config option to allow for driver installations where the device nodes are not rooted at the nvidiaDriverRoot. This option can be controlled using the HOST_DEV_ROOT environment variable or host-dev-root command line argument. If this value is not specified, the value of nvidiaDriverRoot will be used. Signed-off-by: Evan Lezar <[email protected]>

Signed-off-by: Evan Lezar <[email protected]>

In the case where ONLY CDI-based device-list-strategies are requested, the standard updates to the container allocate response do not make sense. This change skips these updates in this case. Signed-off-by: Evan Lezar <[email protected]>

elezar self-assigned this Apr 19, 2024

elezar force-pushed the gke-emulation branch 2 times, most recently from 15d8edb to 2a86769 Compare April 22, 2024 14:31

Dragoncell mentioned this pull request Apr 23, 2024

How to mount containerPath to a hostPath for discover NVIDIA libraries w/o CDI spec #632

Closed

elezar mentioned this pull request Apr 23, 2024

Create CDI spec error "libcuda.so.535.129.03 not found" in version "v0.15.0-rc.2" #621

Closed

elezar requested review from cdesiniotis and klueska April 23, 2024 19:02

elezar force-pushed the gke-emulation branch 2 times, most recently from 59cc999 to 6c8f72b Compare April 23, 2024 19:10

cdesiniotis reviewed May 6, 2024

View reviewed changes

cdesiniotis reviewed May 9, 2024

View reviewed changes

elezar force-pushed the gke-emulation branch from 6c8f72b to 91e1b01 Compare May 10, 2024 12:43

elezar force-pushed the gke-emulation branch 2 times, most recently from f5304a9 to c9721c1 Compare June 3, 2024 14:00

elezar marked this pull request as ready for review June 4, 2024 12:38

elezar commented Jun 4, 2024

View reviewed changes

elezar force-pushed the gke-emulation branch 3 times, most recently from 012fa78 to 23eddda Compare June 5, 2024 14:03

cdesiniotis approved these changes Jun 15, 2024

View reviewed changes

elezar force-pushed the gke-emulation branch from 23eddda to 914118f Compare June 17, 2024 11:18

elezar added 4 commits June 17, 2024 13:18

Detect devRoot for driver installation

a58296e

Signed-off-by: Evan Lezar <[email protected]>

Allow cdi hook path to be set

411cf49

Signed-off-by: Evan Lezar <[email protected]>

Skip container updates if only CDI is selected

352d64d

In the case where ONLY CDI-based device-list-strategies are requested, the standard updates to the container allocate response do not make sense. This change skips these updates in this case. Signed-off-by: Evan Lezar <[email protected]>

elezar force-pushed the gke-emulation branch from 914118f to 352d64d Compare June 17, 2024 11:19

elezar enabled auto-merge June 17, 2024 11:20

elezar merged commit 092b88d into NVIDIA:main Jun 17, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support non-standard driver installs #666

Support non-standard driver installs #666

elezar commented Apr 19, 2024 •

edited

Loading

cdesiniotis May 6, 2024

cdesiniotis May 6, 2024

elezar Jun 5, 2024

cdesiniotis May 6, 2024

elezar May 8, 2024

cdesiniotis May 9, 2024

elezar Jun 4, 2024

cdesiniotis Jun 15, 2024

elezar Jun 17, 2024

elezar Jun 4, 2024

cdesiniotis left a comment

cdesiniotis Jun 15, 2024

cdesiniotis Jun 15, 2024

		cdi.WithTargetDriverRoot(*config.Flags.NvidiaDriverRoot),
		cdi.WithTargetDevRoot(*config.Flags.NvidiaDevRoot),

	// IsOnlyCDIEnabled returns whether all strategies being used require CDI.
	// AllCDIEnabled returns whether all strategies being used require CDI.

Support non-standard driver installs #666

Support non-standard driver installs #666

Conversation

elezar commented Apr 19, 2024 • edited Loading

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

cdesiniotis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elezar commented Apr 19, 2024 •

edited

Loading