Refactor driver discovery #825
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
elezar
force-pushed
the
refactor-driver-discovery
branch
2 times, most recently
from
8ad7493 to
a561a05
Compare
elezar
force-pushed
the
refactor-driver-discovery
branch
from
a561a05 to
841c7d1
Compare
elezar
commented
Jan 10, 2025
| @@ -49,6 +49,7 @@ TRANSLATOR: | |||
| const: | |||
| - {action: accept, from: "^NVSANDBOXUTILS_"} | |||
| - {action: accept, from: "^nvSandboxUtils"} | |||
| - {action: replace, from: "^NVSANDBOXUTILS_255_MASK_", to: "MASK255_" } | |||
There was a problem hiding this comment.
I don't think this was required.
Signed-off-by: Evan Lezar <[email protected]>
Signed-off-by: Evan Lezar <[email protected]>
This change aligns the driver file discovery with device discovery and allows other sources such as nvsandboxutils to be added. Signed-off-by: Evan Lezar <[email protected]>
elezar
force-pushed
the
refactor-driver-discovery
branch
from
841c7d1 to
be25223
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the driver discovery workflow to centralize version detection and entity lookup in a new dgpu package and integrate libnvsandboxutils for sandbox utils discovery. Key changes include:
- Replaced bespoke CUDA and NVML version lookups with a unified
getDriverVersionfallback chain. - Introduced
internal/platform-support/dgpuwith option-basedNewDriverDiscoverer. - Updated all
nvcdiconsumers to use the newdgpu.NewDriverDiscovererand removed old discovery helpers.
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/nvcdi/wrapper.go | No functional change; remains the CDI spec wrapper. |
| pkg/nvcdi/management.go | Swapped CUDA version lookup for getDriverVersion and new dgpu discoverer. |
| pkg/nvcdi/lib.go | Refactored driver-version helpers into nvcdilib. |
| pkg/nvcdi/lib-nvml.go | Updated common edits to call new getDriverVersion and pass version to NVML discoverer. |
| pkg/nvcdi/common-nvml.go | Signature change to include version in the common NVML discoverer. |
| internal/platform-support/dgpu/options.go | Added new option setters (WithDriver, WithLdconfigPath, WithVersion). |
| internal/platform-support/dgpu/driver.go | Implemented NewDriverDiscoverer using option-driven construction. |
| internal/platform-support/dgpu/driver-nvsandboxutils.go | Stub for nvsandboxutils discovery. |
| internal/platform-support/dgpu/driver-nvml.go | Moved NVML-based driver discovery into options-based methods. |
| internal/nvsandboxutils/gen/.../nvsandboxutils.yml | Allowed MASK255_ constant renaming for sandbox utils. |
Comments suppressed due to low confidence (4)
internal/platform-support/dgpu/options.go:89
- There is a typo in
WithNvsandboxuitilsLib(should beWithNvsandboxutilsLib). Correcting this will avoid confusion and maintain consistency.
func WithNvsandboxuitilsLib(nvsandboxutilslib nvsandboxutils.Interface) Option {
internal/platform-support/dgpu/driver.go:26
- This new
NewDriverDiscovererlogic is complex and currently untested—adding unit tests to cover option parsing and fallback behavior will help catch future regressions.
// NewDriverDiscoverer creates a discoverer for the libraries and binaries associated with a driver installation.
internal/platform-support/dgpu/driver.go:28
- The call to
new(opts...)is invalid Go syntax and won’t initialize youroptionsstruct. Consider creating a pointer tooptions(e.g.o := &options{}) and then applying each opt function.
o := new(opts...)
internal/platform-support/dgpu/driver-nvml.go:19
- The code references
lookup.NewFileLocator,lookup.WithLogger, etc., but thelookuppackage is not imported. Add"github.com/NVIDIA/nvidia-container-toolkit/internal/lookup"(andlookup/rootif needed).
import (
|
|
||
| nvsandboxutilsDiscoverer, err := o.newNvsandboxutilsDriverDiscoverer() | ||
| if err != nil { | ||
| // TODO: Log a warning |
There was a problem hiding this comment.
[nitpick] Leftover TODO comments for logging failure of sub-discoverers. Consider implementing real logging or removing the TODO if not needed.
Suggested change
| // TODO: Log a warning | |
| o.logger.Warn(fmt.Sprintf("Failed to create nvsandboxutils driver discoverer: %v", err)) |
Copilot uses AI. Check for mistakes.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change refactors driver discovery to prepare for the discovery of additional entities using libnvsandboxutils.