WIP: Add firmware-search-path option to nvcdi package #317
Conversation
Signed-off-by: Christopher Desiniotis <[email protected]>
Signed-off-by: Christopher Desiniotis <[email protected]>
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.
Copilot uses AI. Check for mistakes.
| @@ -114,7 +115,11 @@ func getUTSRelease() (string, error) { | |||
| return unix.ByteSliceToString(utsname.Release[:]), nil | |||
| } | |||
|
|
|||
| 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.
Copilot uses AI. Check for mistakes.
Migrated from: https://gitlab.com/nvidia/container-toolkit/container-toolkit/-/merge_requests/511