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.

Move the containerRoot type to a utils package so that this can be shared between hooks. 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]>

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.

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. 🚀

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 added 6 commits March 6, 2025 10:30

Move containerRoot to utils package

597359e

Move the containerRoot type to a utils package so that this can be shared between hooks. Signed-off-by: Evan Lezar <[email protected]>

Use ContainerRoot from enable-cuda-compat

d54a7f5

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

Move CreateLdsoconfdFile to ContainerRoot

e91a729

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

Move SafeExec logic to utils package

8124ab0

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

Move resolution of LDConfig to utils package

ff49044

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

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

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 6, 2025

elezar Mar 7, 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

elezar Feb 27, 2025

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

cdesiniotis Mar 6, 2025

Choose a reason for hiding this comment

elezar Mar 6, 2025

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

jgehrcke Mar 6, 2025

Choose a reason for hiding this comment

elezar Mar 7, 2025

Choose a reason for hiding this comment

jgehrcke Mar 6, 2025

Choose a reason for hiding this comment

elezar Mar 7, 2025

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