NVIDIA device nodes created at host's /dev break running privileged containers with cri-o

[klueska]

In #307 we introduced a change that mounts the hosts /dev over the DRA driver's /dev regardless of whether we are using a host-managed GPU driver or an operator-managed GPU driver.

In doing so, we inadvertently create device nodes for all NVIDIA devices at the host's /dev whenever we run any NVML related code.

Unfortunately, this causes problems with cri-o when trying to run privileged containers with CDI injected GPU devices.

The root of the problem stems from the following check that cri-o makes:

for _, device := range c.Config().GetDevices() {
		// If we are privileged, we have access to devices on the host.
		// If the requested container path already exists on the host, the container won't see the expected host path.
		// Therefore, we must error out if the container path already exists
		if c.Privileged() && device.GetContainerPath() != device.GetHostPath() {
			// we expect this to not exist
			_, err := os.Stat(device.GetContainerPath())
			if err == nil {
				return errors.New("privileged container was configured with a device container path that already exists on the host")
			}

			if !os.IsNotExist(err) {
				return fmt.Errorf("error checking if container path exists on host: %w", err)
			}
		}

When a container is started as privileged it sees all of the NVIDIA device nodes at the host's /dev and has them injected due to the privileged setting. However when CDI then tries to inject the same device nodes from /run/nvidia/driver/dev/, it triggers the block of code above to error out the starting of the container with the following:

privileged container was configured with a device container path that already exists on the host

This happens because the container already has a path mounted for /dev/nvidia* due to our inadvertent creation of these device nodes and the fact that the container is running privileged.

We need to either:

1. Stop mounting the host's /dev over our own /dev in all cases; OR
2. Prevent our calls to NVML to create device nodes inadvertently in our local /dev directory (which is still mounted from the host).

[klueska]

/cc @jgehrcke, @elezar , @faganihajizada

[klueska]

I minimal diff to address this would be:

diff --git a/cmd/compute-domain-kubelet-plugin/nvlib.go b/cmd/compute-domain-kubelet-plugin/nvlib.go
index d798b3a4..cee70a8e 100644
--- a/cmd/compute-domain-kubelet-plugin/nvlib.go
+++ b/cmd/compute-domain-kubelet-plugin/nvlib.go
@@ -43,6 +43,7 @@ const (
        nvidiaCapsDeviceName             = "nvidia-caps"
        nvidiaCapsImexChannelsDeviceName = "nvidia-caps-imex-channels"
        nvidiaCapFabricImexMgmtPath      = "/proc/driver/nvidia/capabilities/fabric-imex-mgmt"
+       hostDevPath                      = "/host/dev"
 )
 
 type deviceLib struct {
@@ -89,6 +90,10 @@ func newDeviceLib(driverRoot root) (*deviceLib, error) {
                return nil, fmt.Errorf("error recursively unmounting %s: %w", procDriverNvidiaPath, err)
        }
 
+       if err := d.conditionallyBindMountHostDev(); err != nil {
+               return nil, fmt.Errorf("error conditionally bind mounting host dev: %w", err)
+       }
+
        return &d, nil
 }
 
@@ -418,3 +423,25 @@ func (l deviceLib) unmountRecursively(root string) error {
 
        return helper(root)
 }
+
+// conditionallyBindMountHostDev bind mounts hostDevPath over /dev when devRoot is "/".
+func (l deviceLib) conditionallyBindMountHostDev() error {
+       // If devRoot != "/" then we don't need to do the mount
+       if l.devRoot != "/" {
+               return nil
+       }
+
+       // Get a reference to the mount executable.
+       mountExecutable, err := exec.LookPath("mount")
+       if err != nil {
+               return fmt.Errorf("error looking up mount executable: %w", err)
+       }
+       mounter := mount.New(mountExecutable)
+
+       // Bind mount hostDevPath over /dev
+       if err := mounter.Mount(hostDevPath, "/dev", "", []string{"bind"}); err != nil {
+               return fmt.Errorf("failed to bind mount %s over /dev: %w", hostDevPath, err)
+       }
+
+       return nil
+}
diff --git a/deployments/helm/nvidia-dra-driver-gpu/templates/kubeletplugin.yaml b/deployments/helm/nvidia-dra-driver-gpu/templates/kubeletplugin.yaml
index d75b432b..d4def08c 100644
--- a/deployments/helm/nvidia-dra-driver-gpu/templates/kubeletplugin.yaml
+++ b/deployments/helm/nvidia-dra-driver-gpu/templates/kubeletplugin.yaml
@@ -141,7 +141,7 @@ spec:
         # TODO: make this more surgical, see discussion in
         # https://github.com/NVIDIA/k8s-dra-driver-gpu/pull/307.
         - name: host-dev
-          mountPath: /dev
+          mountPath: /host/dev
       {{- end }}
       {{- if .Values.resources.gpus.enabled }}
       - name: gpus

This may not still be the "right" way (as suggested in #307), but this is the minimal change in the sense that it is identical to what we have today, except that we only mount the host's /dev over the container's /dev if there is no devRoot inside driverRoot (which is actually what we care about anyway).

As such, this feels less risky than any other change that would remove this mount over the container's /dev directory altogether.