Ensure that libcuda.so is in the ldcache #947

elezar · 2025-02-27T13:07:42Z

These changes add a hook to create soname symlinks (e.g. libcuda.so.1 -> libcuda.so.RM_VERSION) to ensure that the libcuda.so -> libcuda.so.1 -> libcuda.so.RM_VERSION symlink chain exists when the ldcache is updated. This allows libcuda.so to be present in the ldcache when ldconfig is run.

Note that since we're adding a new hook, a generating client such as the k8s-device plugin or the k8s-dra-driver-gpu must be used with a nvidia-cdi-hook binary with a sufficient version.

ArangoGutierrez · 2025-02-27T13:17:40Z

/ok to test

Copilot

PR Overview

This PR adds an end-to-end test to verify that the libcuda symlink chain is properly present in the ldcache when running via Docker with NVIDIA runtime. Key changes include:

Addition of the "strings" import to support string splitting.
New test block that pulls an Ubuntu image and runs a container to inspect the ldcache output for libcuda.
Parsing and validation of the ldcache output to ensure both "libcuda.so" and "libcuda.so.1" are present.

Reviewed Changes

File	Description
tests/e2e/nvidia-container-toolkit_test.go	Added an end-to-end test to validate the presence of libcuda symlink entries in the ldcache

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

tests/e2e/nvidia-container-toolkit_test.go

elezar · 2025-02-27T13:32:11Z

cmd/nvidia-cdi-hook/create-soname-symlinks/soname-symlinks.go

+
+	// Create the 'create-soname-symlinks' command
+	c := cli.Command{
+		Name:  "create-soname-symlinks",


@klueska I elected to add a new hook entirely instead of modifying the existing update-ldcache. This is in keeping with "purpose-built hooks" and also means that the hook name can be used to indicate the intent.

ArangoGutierrez

lgtm, just one nit

cmd/nvidia-cdi-hook/create-soname-symlinks/soname-symlinks.go

cdesiniotis

Left one clarifying question, but not a blocker.

cdesiniotis · 2025-03-06T01:16:25Z

cmd/nvidia-cdi-hook/create-soname-symlinks/soname-symlinks.go

+		"-N",
+	)
+	// Explicitly specific the directories to add.
+	args = append(args, dirs...)


Question -- does this create .so symlinks for all libraries present in the specified directories? Does this differ from the behavior of the legacy libnvidia-container implementation, which IIRC would only create the .so symlinks for a small list of libraries (like libcuda.so)?

This is not the .so symlinks, but the SONAME symlinks i.e. libcuda.so.1 -> libcuda.so.RM_VERSION in the case of libcuda. The .so symlinks are created using the "standard" create-symlinks hook.

copy-pr-bot · 2025-03-06T08:30:05Z

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.

elezar · 2025-03-06T08:52:34Z

@cdesiniotis I am removing the must-backport label for this. Although this was triggered by a customer request, there is a workaround available and I would rather not backport another new hook to the release-1.17 branch.

jgehrcke · 2025-03-06T10:29:39Z

and I would rather not backport another new hook to the release-1.17 branch.

Sounds good.

jgehrcke · 2025-03-06T10:39:18Z

cmd/nvidia-cdi-hook/cudacompat/cudacompat.go

 	if hostDriverVersion == "" {
 		m.logger.Debugf("Host driver version not specified")
 		return "", nil
 	}
-	if !containerRoot.hasPath(cudaCompatPath) {
+	if !containerRoot.HasPath(cudaCompatPath) {


I have picked a more or less arbitrary place in the code to put the following comment. Having more expressive variable names would make review for me easier. You can postpone looking at this comment until after merge, that's fine with me! In the spirit of learning what we do here however I'd appreciate an answer at some point :)

In my software engineering life I have always deeply cared about file system terminology and operations, and about making related code readable and self-expressive. For example,

I like to distinguish a "relative path" from an "absolute path" via variable name (if possible)

I like to distinguish a file object from a file path via variable name

I like to distinguish a path to a file from a path to a directory (expressing intent, of course these are technically the same)

I like to call things a "base name" for expressing intent a well (when there are no path separators, and this is supposed to be a "file name" or "directory name").

What is containerRoot in canonical unix file path terminology?

Is it guaranteed to point to a directory?

Is it an absolute path?

Is it always just /?

What are some properties about cudaCompatPath that we know/guarantee, that we could also express in the variable name or in the type?

Is it always a relative path (not starting with `/)?

Is it always a path to a directory?

Thanks for calling this out. I think we could do a pass through this and improve readability greatly. There is no rush on this particular PR (I've elected to hold the changes back from the v1.17.5 release) and as such, I think we can address these concerns here instead of as a follow-up.

The containerRootDir is the absolute path on the host filesystem to the root (/) of the container filesystem. It is specified in the OCI Runtime Specification as Root which informs the slightly ambiguous name. The containerRoot variable is the typed representation of this directory that allows us to attached helper functions such as HasPath / Resolve / Glob to it.

The cudaCompatPath is the absolute path to the directory containing the CUDA compat libraries in the container (if it exists). It defined as a constant with value /usr/local/cuda/compat. That is to say, it is always an absolute path to a directory in the container, and we're confirming that this exists, but have to calculate the path to this folder on the host filesystem to do this check.

I think we can address these concerns here instead of as a follow-up.

Thank you for that!

The containerRootDir is the absolute path on the host filesystem to the root (/) of the container filesystem

Is the container filesystem always accessible from within the host filesystem?

It is specified in the OCI Runtime Specification as Root which informs the slightly ambiguous name.

gotcha. Thanks for that background.

The cudaCompatPath is the absolute path to the directory containing the CUDA compat libraries in the container (if it exists).

Thank you for that precision. Now, that is quickly&easily understandable.

but have to calculate the path to this folder on the host filesystem to do this check

meaning that it's always mounted into the container, and never baked into it?

jgehrcke · 2025-03-06T10:41:26Z

cmd/nvidia-cdi-hook/utils/container-root.go

+)
+
+// A ContainerRoot represents the root filesystem of a container.
+type ContainerRoot string


Oh, it's just a string.

Relates to my previous comment: I'd love to see the intent expressed in the variable name: path to a directory?

And this is probably really just my lack of knowledge: I wonder: when is the container's root file system not located at just /?

I have added som context above. Let me know if it's not clear and I can add more information.

jgehrcke

Thanks for all the work! I understand very little but you have my full emotional support! And I know you will repair everything you destroy. In that sense: approved with honor. 🚀

Thsi change moves the ContainerRoot type to the oci package and updates state.GetContainerRootDirPath to return a variable of type ContainerRoot. This enabled better reuse between hooks. Signed-off-by: Evan Lezar <[email protected]>

This change updates the enable-cuda-compat implementation to also use oci.ContainerRoot. Signed-off-by: Evan Lezar <[email protected]>

Since the creation of a .conf file in /etc/ld.so.conf.d is shared by both the update-ldcache and enable-cuda-compat hooks, this is moved to the ContainerRoot type. Signed-off-by: Evan Lezar <[email protected]>

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

This change adds a create-soname-symlinks hook that can be used to ensure that the soname symlinks for injected libraries exist in a container. This is done by calling ldconfig -n -N for the folders containing the injected libraries. This also ensures that libcuda.so is present in the ldcache when the update-ldcache hook is run. Signed-off-by: Evan Lezar <[email protected]>

jgehrcke · 2025-03-21T13:18:22Z

cmd/nvidia-cdi-hook/chmod/chmod.go

@@ -113,15 +113,15 @@ func (m command) run(c *cli.Context, cfg *config) error {
 		return fmt.Errorf("failed to load container state: %v", err)
 	}

-	containerRoot, err := s.GetContainerRoot()
+	containerRoot, err := s.GetContainerRootDirPath()


❤️ Thanks for adding clarity here.

And now that I have read your explanation.. this is a path that is valid in the host filesystem, and points to the container filesystem's root..? :)

jgehrcke · 2025-03-21T13:18:45Z

cmd/nvidia-cdi-hook/chmod/chmod.go

@@ -140,6 +140,7 @@ func (m command) run(c *cli.Context, cfg *config) error {
 }

 // getPaths updates the specified paths relative to the root.
+// TODO(elezar): This function should be updated to make use of the oci.ContainerRoot type.


jgehrcke · 2025-03-21T13:20:19Z

cmd/nvidia-cdi-hook/create-soname-symlinks/soname-symlinks.go

+		return fmt.Errorf("failed to load container state: %v", err)
+	}
+
+	containerRoot, err := s.GetContainerRootDirPath()


Below I see we sometimes have a nice OnHost suffix. Could this be applicable here, too?

jgehrcke · 2025-03-21T13:20:39Z

cmd/nvidia-cdi-hook/create-soname-symlinks/soname-symlinks.go

+		return fmt.Errorf("failed to determined container root: %v", err)
+	}
+	if containerRoot == "" {
+		m.logger.Warningf("No container root detected")


when can/does this happen in practice?
(for my own curiosity/understanding)

jgehrcke · 2025-03-21T13:22:39Z

cmd/nvidia-cdi-hook/cudacompat/cudacompat.go

 	if err != nil {
 		return fmt.Errorf("failed to determined container root: %w", err)
 	}

-	containerForwardCompatDir, err := m.getContainerForwardCompatDir(containerRoot(containerRootDir), cfg.hostDriverVersion)
+	containerForwardCompatDir, err := m.getContainerForwardCompatDirPathInContainer(containerRootDirPath, cfg.hostDriverVersion)


Thanks for having added the InContainer suffix here. I know these things get lengthy but the information density is really good and this makes stuff no-brainer readable, which was the goal.

And again, the containerRootDirPath can be thought of as a containerRootDirPathOnHost, right?

jgehrcke

Thank you for all the iteration. Let's land this. I have left a few more questions while browsing this, but please don't slow down landing this because of my remarks.

ArangoGutierrez requested review from ArangoGutierrez and Copilot February 27, 2025 13:21

Copilot AI reviewed Feb 27, 2025

View reviewed changes

tests/e2e/nvidia-container-toolkit_test.go Outdated Show resolved Hide resolved

tests/e2e/nvidia-container-toolkit_test.go Outdated Show resolved Hide resolved

elezar force-pushed the ensure-libcuda.so-in-ldcache branch from 6e7e1a8 to 44e6630 Compare February 27, 2025 13:25

elezar commented Feb 27, 2025

View reviewed changes

ArangoGutierrez reviewed Feb 27, 2025

View reviewed changes

cmd/nvidia-cdi-hook/create-soname-symlinks/soname-symlinks.go Outdated Show resolved Hide resolved

elezar linked an issue Feb 27, 2025 that may be closed by this pull request

LDCache does not include libcuda.so #944

Open

elezar force-pushed the ensure-libcuda.so-in-ldcache branch from 44e6630 to e43da10 Compare February 27, 2025 13:48

elezar added the must-backport The changes in PR need to be backported to at least one stable release branch. label Feb 27, 2025

elezar force-pushed the ensure-libcuda.so-in-ldcache branch 3 times, most recently from daadba9 to 9daa179 Compare February 28, 2025 12:45

elezar requested review from klueska and jgehrcke February 28, 2025 12:52

ArangoGutierrez approved these changes Mar 5, 2025

View reviewed changes

cdesiniotis reviewed Mar 6, 2025

View reviewed changes

elezar force-pushed the ensure-libcuda.so-in-ldcache branch from 9daa179 to 035abe0 Compare March 6, 2025 08:30

elezar removed the must-backport The changes in PR need to be backported to at least one stable release branch. label Mar 6, 2025

jgehrcke reviewed Mar 6, 2025

View reviewed changes

jgehrcke approved these changes Mar 6, 2025

View reviewed changes

elezar added this to the v1.18.0 milestone Mar 19, 2025

elezar force-pushed the ensure-libcuda.so-in-ldcache branch from 035abe0 to 305c2e2 Compare March 19, 2025 14:55

elezar added 3 commits March 20, 2025 13:21

Move ContainerRoot type to oci package

13bbf71

Thsi change moves the ContainerRoot type to the oci package and updates state.GetContainerRootDirPath to return a variable of type ContainerRoot. This enabled better reuse between hooks. Signed-off-by: Evan Lezar <[email protected]>

Use oci.ContainerRoot from enable-cuda-compat

5d2f48c

This change updates the enable-cuda-compat implementation to also use oci.ContainerRoot. Signed-off-by: Evan Lezar <[email protected]>

Move CreateLdsoconfdFile to ContainerRoot

715e09f

Since the creation of a .conf file in /etc/ld.so.conf.d is shared by both the update-ldcache and enable-cuda-compat hooks, this is moved to the ContainerRoot type. Signed-off-by: Evan Lezar <[email protected]>

elezar added 2 commits March 20, 2025 13:22

Move SafeExec logic to internal safeexec package

4088430

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

Move resolution of host LDConfig to config package

15645e6

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

elezar force-pushed the ensure-libcuda.so-in-ldcache branch from 305c2e2 to 7229201 Compare March 20, 2025 11:24

elezar force-pushed the ensure-libcuda.so-in-ldcache branch from 7229201 to bdfaea4 Compare March 20, 2025 12:56

jgehrcke reviewed Mar 21, 2025

View reviewed changes

jgehrcke approved these changes Mar 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure that libcuda.so is in the ldcache #947

Ensure that libcuda.so is in the ldcache #947

elezar commented Feb 27, 2025 •

edited

Loading

ArangoGutierrez commented Feb 27, 2025

Copilot AI left a comment

elezar Feb 27, 2025

ArangoGutierrez left a comment

cdesiniotis left a comment

cdesiniotis Mar 6, 2025

elezar Mar 6, 2025

copy-pr-bot bot commented Mar 6, 2025

elezar commented Mar 6, 2025

jgehrcke commented Mar 6, 2025

jgehrcke Mar 6, 2025

elezar Mar 7, 2025

jgehrcke Mar 21, 2025

jgehrcke Mar 21, 2025

jgehrcke Mar 6, 2025

elezar Mar 7, 2025

jgehrcke left a comment

jgehrcke Mar 21, 2025

jgehrcke Mar 21, 2025

jgehrcke Mar 21, 2025

jgehrcke Mar 21, 2025 •

edited

Loading

jgehrcke Mar 21, 2025

jgehrcke left a comment

Ensure that libcuda.so is in the ldcache #947

Are you sure you want to change the base?

Ensure that libcuda.so is in the ldcache #947

Conversation

elezar commented Feb 27, 2025 • edited Loading

ArangoGutierrez commented Feb 27, 2025

Copilot AI left a comment

Choose a reason for hiding this comment

PR Overview

Reviewed Changes

Choose a reason for hiding this comment

ArangoGutierrez left a 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

copy-pr-bot bot commented Mar 6, 2025

elezar commented Mar 6, 2025

jgehrcke commented Mar 6, 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

jgehrcke left a 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

jgehrcke Mar 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgehrcke left a comment

Choose a reason for hiding this comment

elezar commented Feb 27, 2025 •

edited

Loading

jgehrcke Mar 21, 2025 •

edited

Loading