Skip to content

Conversation

@jgehrcke
Copy link
Collaborator

@jgehrcke jgehrcke commented Mar 27, 2025

This is a patch for host-provided drivers located not directly under /.

With a host-provided driver at for example /home/kubernetes/... we choose an in-container devRoot of /:

func (r root) getDevRoot() string {
	if r.isDevRoot() {
		return string(r)
	}
	return "/"
}

The fallback to / takes effect because there is no dev directory underneath the driver's directory.

In many other cases, host-provided drivers are located at / (on the host). The function above then returns /driver-root (which is where hostDriverRoot is mounted to containerDriverRoot). Indeed, in those cases the host's /dev directory is mounted under /driver-root/dev in the container. The following loc then sets devRoot (in the container) to /driver-root:

devRoot: driverRoot.getDevRoot(),

Next, the following line of code constructs a path in the container under /driver-root that actually maps to a corresponding path in the host's /dev file system:

HostPath: filepath.Join(devRoot, info.path),

So far, so good.

However, for host-provided drivers located not directly under / the chosen devRoot of / does not correspond to any mountpoint where host's dev has been mounted into the container.

That is, any mutation in container's '/dev' does not apply to the host.

That may result in

Warning  Failed     0s (x18 over 3m24s)  kubelet            \
Error: failed to create containerd container: CDI device injection failed: failed to inject devices: failed to stat CDI host device "/dev/nvidia-caps/nvidia-cap4323": no such file or directory

(where the kubelet plugin did create that /dev node, just visible only in its container, and not on the host)

With this patch, any in-container mutation of /dev will take effect on the host. This may not be ideal, but is a pragmatic patch that we can iterate on later.

Testing:

  • didn't break basic Luna workflows (non-IMEX)
  • basic IMEX test green on a GB200 system

A cleaner version of this?

A cleaner variant would not return "/" as a fallback above.

Instead, we need to return a path (in the container filesystem) where the host's /dev (or parts thereof) are mounted in.

So, we could have (in the container) a path P where we may mount in all of host's /dev or (better) only the required subset and then return P as a fallback.

@copy-pr-bot
Copy link

copy-pr-bot bot commented Mar 27, 2025

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@jgehrcke jgehrcke force-pushed the jp/cdplugin-mount-dev-dev branch from 0004218 to 6815948 Compare March 27, 2025 00:18
@jgehrcke jgehrcke requested a review from klueska March 27, 2025 01:03
Copy link
Collaborator

@klueska klueska left a comment

Choose a reason for hiding this comment

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

Approved (for now)
Let’s revisit when @elezar is available again to see if we can come up with a solution that doesn’t require a mount over the containers /dev

@jgehrcke jgehrcke merged commit 75bf8c2 into NVIDIA:main Mar 27, 2025
7 checks passed
@klueska klueska added this to the v25.3.0 milestone Aug 13, 2025
klueska added a commit to klueska/k8s-dra-driver-gpu that referenced this pull request Aug 20, 2025
Currently, in the compute domain driver, we unconditionally mount the
host's /dev over the container's /dev regardless of what the devRoot of
the GPU driver is. This change was introduced in
NVIDIA#307.

Unfortunately, this causes some problems on certain systems as described
in NVIDIA#477.

This patch makes this mount contingent on devRoot == "/" (which is the
only case where we actually ever wanted / needed to have this mount in
the first place.

Signed-off-by: Kevin Klues <[email protected]>
klueska added a commit to klueska/k8s-dra-driver-gpu that referenced this pull request Aug 20, 2025
Currently, in the compute domain driver, we unconditionally mount the
host's /dev over the container's /dev regardless of what the devRoot of
the GPU driver is. This change was introduced in
NVIDIA#307.

Unfortunately, this causes some problems on certain systems as described
in NVIDIA#477.

This patch makes this mount contingent on devRoot == "/" (which is the
only case where we actually ever wanted / needed to have this mount in
the first place.

Signed-off-by: Kevin Klues <[email protected]>
klueska added a commit to klueska/k8s-dra-driver-gpu that referenced this pull request Aug 20, 2025
Currently, in the compute domain driver, we unconditionally mount the
host's /dev over the container's /dev regardless of what the devRoot of
the GPU driver is. This change was introduced in
NVIDIA#307.

Unfortunately, this causes some problems on certain systems as described
in NVIDIA#477.

This patch makes this mount contingent on devRoot == "/" (which is the
only case where we actually ever wanted / needed to have this mount in
the first place.

Signed-off-by: Kevin Klues <[email protected]>
klueska added a commit to klueska/k8s-dra-driver-gpu that referenced this pull request Aug 20, 2025
Currently, in the compute domain driver, we unconditionally mount the
host's /dev over the container's /dev regardless of what the devRoot of
the GPU driver is. This change was introduced in
NVIDIA#307.

Unfortunately, this causes some problems on certain systems as described
in NVIDIA#477.

This patch makes this mount contingent on devRoot == "/" (which is the
only case where we actually ever wanted / needed to have this mount in
the first place.

Signed-off-by: Kevin Klues <[email protected]>
klueska added a commit to klueska/k8s-dra-driver-gpu that referenced this pull request Aug 21, 2025
Currently, in the compute domain driver, we unconditionally mount the
host's /dev over the container's /dev regardless of what the devRoot of
the GPU driver is. This change was introduced in
NVIDIA#307.

Unfortunately, this causes some problems on certain systems as described
in NVIDIA#477.

This patch makes this mount contingent on devRoot == "/" (which is the
only case where we actually ever wanted / needed to have this mount in
the first place.

Signed-off-by: Kevin Klues <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

2 participants