Add support for gated modifications jit-cdi mode #1230
Conversation
Pull Request Test Coverage Report for Build 16831546401Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
elezar
force-pushed
the
fix-gdrcopy-in-jit-cdi
branch
from
0af2c9e to
086955b
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Signed-off-by: Evan Lezar <[email protected]>
Signed-off-by: Evan Lezar <[email protected]>
Signed-off-by: Evan Lezar <[email protected]>
Signed-off-by: Evan Lezar <[email protected]>
This change ensures that NVIDIA_GDS=enabled, NVIDIA_MOFED=enabled, NVIDIA_GDRCOPY=enabled, and NVIDIA_NVSWITCH=enabled are honored by jit-cdi mode. Signed-off-by: Evan Lezar <[email protected]>
elezar
force-pushed
the
fix-gdrcopy-in-jit-cdi
branch
from
086955b to
3fcc351
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for gated modifications in jit-cdi mode by allowing NVIDIA_GDS=enabled, NVIDIA_MOFED=enabled, and NVIDIA_GDRCOPY=enabled environment variables to be honored. The changes refactor existing mode-specific libraries into a unified gated approach and extend automatic CDI device generation to support multiple modes.
- Consolidates GDS, MOFED, and GDRCOPY modes into a unified gated library approach
- Extends automatic CDI device generation to support multiple modes simultaneously
- Adds support for NVSWITCH devices through the new gated system
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 |
|---|---|
| pkg/nvcdi/mode.go | Adds new mode constants (ModeGdrcopy, ModeNvswitch), reorders mode list, and fixes mode assignment in resolveMode |
| pkg/nvcdi/lib.go | Consolidates gated modes (GDS, MOFED, GDRCOPY) to use unified gatedlib factory |
| pkg/nvcdi/gds.go | Removes GDS-specific library implementation (deleted file) |
| pkg/nvcdi/gated.go | Renames mofedlib to gatedlib and adds support for multiple gated modes via getModeDiscoverer |
| internal/oci/spec.go | Adds SpecModifiers type to handle collections of spec modifiers |
| internal/modifier/cdi.go | Extends automatic CDI generation to support gated devices and multiple modes per request |
| case ModeNvswitch: | ||
| return discover.NewNvSwitchDiscoverer(l.logger, l.devRoot) | ||
| default: | ||
| return nil, fmt.Errorf("unrecognized mode") |
There was a problem hiding this comment.
The error message "unrecognized mode" is not helpful as it doesn't specify which mode was unrecognized. Consider including the actual mode value in the error message.
| return nil, fmt.Errorf("unrecognized mode") | |
| return nil, fmt.Errorf("unrecognized mode %q", l.mode) |
Copilot uses AI. Check for mistakes.
| nvcdi.WithNVIDIACDIHookPath(cfg.NVIDIACTKConfig.Path), | ||
| nvcdi.WithDriverRoot(cfg.NVIDIAContainerCLIConfig.Root), | ||
| nvcdi.WithVendor(automaticDeviceVendor), | ||
| nvcdi.WithClass(perModeDeviceClass[mode]), |
There was a problem hiding this comment.
Accessing perModeDeviceClass[mode] without checking if the key exists could cause a panic or unexpected behavior. The map only contains "auto" key, but modes can include "gds", "mofed", "gdrcopy", "nvswitch".
Copilot uses AI. Check for mistakes.
This change ensures that NVIDIA_GDS=enabled, NVIDIA_MOFED=enabled, and NVIDIA_GDRCOPY=enabled are honored by jit-cdi mode.