Skip to content

Add envvar for libcuda.so parent dir to CDI spec #1030

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

elezar
Copy link
Member

@elezar elezar commented Apr 9, 2025

This change adds a LIBCUDA_SO_PARENT_DIRECTORY_CONTAINER_PATH envvar to a generated CDI specification. This reports where the libcuda.so.* libraries will be injected into the container.

Fixes #1028

@elezar elezar force-pushed the add-driver-lib-root-envvar branch from 7b58a3e to 5349a84 Compare April 9, 2025 12:04
@elezar elezar marked this pull request as draft April 14, 2025 12:02
@elezar elezar added this to the v1.18.0 milestone May 12, 2025
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a LIBCUDA_SO_PARENT_DIRECTORY_CONTAINER_PATH environment variable to the generated CDI specification and implements the required EnvVars interface across multiple discoverer types.

  • Adds an EnvVars() method to several discoverers returning empty environment variable lists where appropriate
  • Updates the driver library discoverer to extract and report the libcuda.so parent directory
  • Adjusts tests to validate that the new environment variable is being injected into container edits

Reviewed Changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.

Show a summary per file
File Description
pkg/nvcdi/workarounds-device-folder-permissions.go Adds EnvVars() method returning nil, consistent with other discoverers
pkg/nvcdi/driver-nvml.go Updates NewDriverLibraryDiscoverer and getVersionLibs to extract the libcuda.so parent directory and append the environment variable
internal/platform-support/dgpu/nvsandboxutils.go; internal/platform-support/dgpu/by-path-hooks.go Implements EnvVars() returning nil, maintaining interface consistency
internal/edits/edits.go Adds processing of EnvVars into container edits
internal/discover/* Updates Discover implementations (none, list, hooks, first-valid, envvar, cache, discover_mock, discover) to support the new EnvVars() interface method
cmd/nvidia-ctk/cdi/generate/generate_test.go; cmd/nvidia-ctk-installer/container/toolkit/toolkit_test.go Updates tests to verify injection of LIBCUDA_SO_PARENT_DIRECTORY_CONTAINER_PATH in container edits

@@ -22,6 +22,12 @@ type Device struct {
Path string
}

// EnvVar represents a discovered environment variable.
type EnvVar struct {
Name string
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am more of a Key/Value mind, but not a blocker.

Copy link
Member Author

Choose a reason for hiding this comment

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

From https://wiki.debian.org/EnvironmentVariables#:~:text=Variables%20are%20defined,the%20terminal%20session.

Variables are defined with name-value pairs: NAME=any string as value. The variable name is usually in capital letters. Anything that follows the equal-sign is considered the variable's value until the terminating line feed character. Variables can be defined ad hoc in a terminal by writing the appropriate command. In Bash this would be export MYVAL="Hello world". In this case the variable stays defined until the end of the terminal session.

@elezar elezar force-pushed the add-driver-lib-root-envvar branch from 5349a84 to 2751299 Compare June 11, 2025 12:58
@elezar elezar marked this pull request as ready for review June 11, 2025 12:59
@elezar elezar requested review from klueska and jgehrcke June 11, 2025 12:59
elezar added 2 commits June 11, 2025 15:07
This change adds environment variables to the Discover interface.

Signed-off-by: Evan Lezar <[email protected]>
This change adds a LIBCUDA_SO_PARENT_DIRECTORY_CONTAINER_PATH envvar to
a generated CDI specification. This reports where the `libcuda.so.*`
libraries will be injected into the container.

Signed-off-by: Evan Lezar <[email protected]>
@elezar elezar force-pushed the add-driver-lib-root-envvar branch from 2751299 to d2ca583 Compare June 11, 2025 13:07
@coveralls
Copy link

Pull Request Test Coverage Report for Build 15585739792

Details

  • 3 of 89 (3.37%) changed or added relevant lines in 12 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.2%) to 33.367%

Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/discover/hooks.go 0 3 0.0%
internal/discover/none.go 0 3 0.0%
internal/platform-support/dgpu/by-path-hooks.go 0 3 0.0%
internal/platform-support/dgpu/nvsandboxutils.go 0 3 0.0%
pkg/nvcdi/workarounds-device-folder-permissions.go 0 3 0.0%
internal/edits/edits.go 3 7 42.86%
internal/edits/envvar.go 0 8 0.0%
internal/discover/first-valid.go 0 9 0.0%
internal/discover/list.go 0 10 0.0%
internal/discover/cache.go 0 11 0.0%
Totals Coverage Status
Change from base Build 15585515442: -0.2%
Covered Lines: 4275
Relevant Lines: 12812

💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add envvar to CDI spec that shows libcuda.so.RM_VERSION parent directory in the container
3 participants