WIP: Add firmware-search-path option to nvcdi package#317
WIP: Add firmware-search-path option to nvcdi package#317cdesiniotis wants to merge 2 commits intoNVIDIA:mainfrom
Conversation
Signed-off-by: Christopher Desiniotis <cdesiniotis@nvidia.com>
Signed-off-by: Christopher Desiniotis <cdesiniotis@nvidia.com>
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new option to allow specifying custom firmware search paths for generating the CDI specification. Key changes include:
- Introducing a new CLI flag ("cdi-firmware-search-paths") in toolkit.go.
- Adding the WithFirmwareSearchPaths option and updating related function signatures across the nvcdi package.
- Propagating the firmware search paths parameter to NVML- and management-mode driver discoverers.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/container/toolkit/toolkit.go | Added a new CLI flag and updated generateCDISpec to pass custom firmware search paths |
| pkg/nvcdi/options.go | Introduced WithFirmwareSearchPaths to set firmware search paths in the nvcdilib structure |
| pkg/nvcdi/management.go | Modified newDriverVersionDiscoverer call to include firmware search paths |
| pkg/nvcdi/lib.go | Extended nvcdilib to include a firmwareSearchPaths field |
| pkg/nvcdi/driver-nvml.go | Updated NewDriverDiscoverer and related functions to accept and propagate firmware search paths |
| pkg/nvcdi/common-nvml.go | Passed firmware search paths to NewDriverDiscoverer from the NVML discoverer |
Comments suppressed due to low confidence (1)
tools/container/toolkit/toolkit.go:221
- Consider adding test cases to cover scenarios when custom firmware search paths are provided, ensuring they are correctly parsed and passed to generate the CDI specification.
&cli.StringSliceFlag{
|
|
||
| // WithFirmwareSearchPaths sets the firmware search paths. | ||
| // This is currently only used for NVML- and Management-Mode. | ||
| func WithFirmwareSearchPaths(paths []string) Option { |
There was a problem hiding this comment.
[nitpick] Augment the function comment to include expected input formats and examples for firmware search paths to help clarify usage for future maintainers.
| } | ||
|
|
||
| func getFirmwareSearchPaths(logger logger.Interface) ([]string, error) { | ||
| func getFirmwareSearchPaths(logger logger.Interface, fwSearchPaths ...string) ([]string, error) { |
There was a problem hiding this comment.
[nitpick] While the fallback behavior is clear from the implementation, adding a brief note in the function comment about how custom firmware search paths override defaults would improve clarity.
|
This PR is stale because it has been open 90 days with no activity. This PR will be closed in 30 days unless new comments are made or the stale label is removed. To skip these checks, apply the "lifecycle/frozen" label. |
github-actions
bot
added
the
lifecycle/stale
3 participants
Migrated from: https://gitlab.com/nvidia/container-toolkit/container-toolkit/-/merge_requests/511