feat: auto-detect NVIDIA GPU via CDI#1658
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the CDI device retrieval logic to support returning all available devices when no specific devices are requested and introduces auto-detection for NVIDIA CDI devices in the CLI. When an NVIDIA CDI device is detected, the legacy driver detection is bypassed. Feedback indicates a logic flaw where the nvidiaCdiFound flag is not set when a user manually specifies NVIDIA devices, which incorrectly triggers the legacy driver detection path.
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the getCDIDevices function to support retrieving all available CDI devices when no specific devices are provided and introduces automatic NVIDIA CDI device detection in the CLI. A significant logic flaw was identified in the CLI implementation where the check for manually specified NVIDIA devices is nested inside a block that only executes when the device list is empty, which prevents the flag from being correctly set and causes legacy driver detection to be triggered incorrectly.
When no CDI devices are explicitly specified, automatically detect nvidia.com/gpu=all and use it for GPU support, skipping the legacy detectDrivers() call. Signed-off-by: reddevillg <reddevillg@gmail.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces auto-detection for NVIDIA CDI devices and updates the getCDIDevices function to support retrieving all available devices when no specific list is provided. The review feedback highlights several logic improvements: the auto-detection criteria should be less restrictive to allow for other CDI devices, auto-detected devices should be merged with user-provided ones rather than being mutually exclusive, and logging levels should be adjusted to reduce noise. Additionally, a minor simplification for string formatting in the CDI library was suggested.
| }); | ||
| std::optional<std::vector<api::types::v1::CdiDeviceEntry>> autoDetectedCdiDevices; | ||
|
|
||
| if (!nvidiaCdiFound && options.cdiDevices.empty()) { |
There was a problem hiding this comment.
The condition options.cdiDevices.empty() is too restrictive. If a user provides other CDI devices (e.g., for storage or other accelerators), auto-detection of the NVIDIA GPU via CDI will be skipped, and the code will fall back to the legacy detectDrivers() call. This contradicts the goal of moving towards CDI-based device management. The check should ideally only be !nvidiaCdiFound.
if (!nvidiaCdiFound) {| fmt::join(specDirs, ", "))); | ||
| std::string devicesStr; | ||
| if (devices) { | ||
| devicesStr = fmt::format("{}", fmt::join(*devices, ", ")); |
There was a problem hiding this comment.
The use of fmt::format here is redundant because fmt::join already returns a formattable object. You can simplify this to fmt::to_string or just pass the join object directly to the outer fmt::format call.
| devicesStr = fmt::format("{}", fmt::join(*devices, ", ")); | |
| devicesStr = fmt::to_string(fmt::join(*devices, ", ")); |
| auto allCdiDevices = cdi::getCDIDevices(options.cdiSpecDir, std::nullopt); | ||
| if (allCdiDevices) { | ||
| for (const auto &device : *allCdiDevices) { | ||
| LogD("{}={} detected", device.kind, device.name); |
There was a problem hiding this comment.
Logging every detected CDI device at LogD level during auto-detection might be too noisy on systems with many CDI specifications. Consider changing this to LINGLONG_TRACE or only logging when the specific NVIDIA device is found.
LINGLONG_TRACE(fmt::format("{}={} detected", device.kind, device.name));| } else if (autoDetectedCdiDevices) { | ||
| opts.cdiDevices = std::move(*autoDetectedCdiDevices); |
There was a problem hiding this comment.
If options.cdiDevices is not empty, the autoDetectedCdiDevices are currently ignored. If the user provided other CDI devices but we also auto-detected an NVIDIA GPU, both should probably be included in opts.cdiDevices. The current logic forces an 'either-or' scenario which might not be what the user expects.
deepin pr auto review你好！我是CodeGeeX。我已仔细审查了你提供的Git Diff。本次修改的主要目的是将CDI（Container Device Interface）设备的获取方式从“仅限指定列表”扩展为“支持获取全部设备（通过 整体来看，修改思路清晰，逻辑基本自洽。但在代码性能、逻辑严谨性、代码安全与质量方面，有一些值得注意和改进的地方。以下是详细的审查意见： 1. 逻辑严谨性问题：NVIDIA自动检测逻辑存在漏洞 if (!nvidiaCdiFound && options.cdiDevices.empty()) { ... }这里的意图是：如果用户没有指定任何CDI设备，且没有显式指定nvidia设备，则尝试自动检测。 bool nvidiaCdiFound = std::any_of(options.cdiDevices.begin(), options.cdiDevices.end(), ...);如果 2. 代码性能问题： utils::error::Result<std::vector<api::types::v1::CdiDeviceEntry>> getCDIDevices(
const std::vector<std::string> &specDirs, const std::optional<std::vector<std::string>> &devices)
auto allCdiDevices = cdi::getCDIDevices(options.cdiSpecDir, std::nullopt);改进建议：
3. 代码安全问题： auto userContainerDir = std::filesystem::path{ "/run/linglong" } / std::to_string(getuid());
4. 代码质量与可读性问题一：自动检测代码块过长，且职责混杂 问题二：硬编码的字符串常量 return d.find("nvidia.com/gpu") != std::string::npos;
// ...
if (device.kind == "nvidia.com/gpu" && device.name == "all") {改进建议： 将这些字符串提取为常量或 constexpr std::string_view NVIDIA_GPU_KIND = "nvidia.com/gpu";
constexpr std::string_view ALL_DEVICES_NAME = "all";问题三： result.emplace_back(api::types::v1::CdiDeviceEntry{
.kind = spec.spec.kind,
.name = dev.name,
.spec = api::types::v1::CdiSpec{ .checksum = spec.checksum, .path = spec.path.string() },
});这段逻辑与下方 综合改进后的代码示例基于上述意见，以下是对 // 在 cli.h 或 cli.cpp 开头定义常量
constexpr std::string_view NVIDIA_GPU_KIND = "nvidia.com/gpu";
constexpr std::string_view ALL_DEVICES_NAME = "all";
// 提取出的自动检测函数
std::optional<api::types::v1::CdiDeviceEntry> Cli::tryAutoDetectNvidiaCdi(const std::vector<std::string> &specDirs) {
auto allCdiDevices = cdi::getCDIDevices(specDirs, std::nullopt);
if (!allCdiDevices) {
return std::nullopt;
}
for (const auto &device : *allCdiDevices) {
LogD("{}={} detected", device.kind, device.name);
if (device.kind == NVIDIA_GPU_KIND && device.name == ALL_DEVICES_NAME) {
return device; // 直接返回找到的设备，避免多余的拷贝
}
}
return std::nullopt;
}
int Cli::run(const RunOptions &options)
{
LINGLONG_TRACE("command run");
// 判断用户是否显式指定了 nvidia 设备
bool nvidiaCdiFound = std::any_of(options.cdiDevices.cbegin(), options.cdiDevices.cend(),
[](const std::string &d) {
return d.find(NVIDIA_GPU_KIND) != std::string::npos;
}
);
std::optional<api::types::v1::CdiDeviceEntry> autoDetectedCdiDevice;
// 仅当用户未显式指定任何 CDI 设备时，才尝试自动检测
if (!nvidiaCdiFound && options.cdiDevices.empty()) {
autoDetectedCdiDevice = tryAutoDetectNvidiaCdi(options.cdiSpecDir);
if (autoDetectedCdiDevice) {
nvidiaCdiFound = true;
}
}
// 如果没有找到任何 nvidia CDI 配置，回退到旧的驱动检测逻辑
if (!nvidiaCdiFound) {
detectDrivers();
}
auto userContainerDir = std::filesystem::path{ "/run/linglong" } / std::to_string(getuid());
// ... 保持后续逻辑不变 ...
// 在赋值处使用移动语义提升性能
} else if (autoDetectedCdiDevice) {
opts.cdiDevices = std::vector<api::types::v1::CdiDeviceEntry>{ std::move(*autoDetectedCdiDevice) };
}
}对于 // cdi.cpp 中提取构建 CdiDeviceEntry 的逻辑（假设下方循环也有类似代码）
auto buildEntry = [](const ParsedSpec& spec, const auto& dev) {
return api::types::v1::CdiDeviceEntry{
.kind = spec.spec.kind,
.name = dev.name,
.spec = api::types::v1::CdiSpec{
.checksum = spec.checksum,
.path = spec.path.string(),
},
};
};
if (!devices) {
for (const auto &spec : specs) {
for (const auto &dev : spec.spec.devices) {
result.emplace_back(buildEntry(spec, dev));
}
}
return result;
}
for (const auto &deviceStr : *devices) {
// ... 查找逻辑 ...
// 使用 buildEntry 替代手写构造
result.emplace_back(buildEntry(spec, dev));
}希望这些审查意见和重构建议对你有所帮助！如果有任何疑问，欢迎随时提问。 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dengbo11, reddevillg The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
3 participants
When no CDI devices are explicitly specified, automatically detect nvidia.com/gpu=all and use it for GPU support, skipping the legacy detectDrivers() call.