fix: separate SKU from operating mode in mlx-devices#3346
Conversation
|
@coderabbitai full_review, thanks! |
WalkthroughThe PR separates factory-SKU device classification from current reported NIC/DPU operating mode. CLI filtering now prioritizes ChangesMLX device mode semantics
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant MlxDevicesCommand
participant ExploredMlxDevice
participant MlxDeviceKind
MlxDevicesCommand->>ExploredMlxDevice: evaluate operating_as_nic
ExploredMlxDevice->>ExploredMlxDevice: use reported nic_mode when present
ExploredMlxDevice->>MlxDeviceKind: use device_kind when nic_mode is unknown
MlxDeviceKind-->>MlxDevicesCommand: return NIC classification
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
(=^･ω･^=) ✅ Action performedFull review finished. |
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-07-10 00:14:45 UTC | Commit: 6ceba92 |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3346.docs.buildwithfern.com/infra-controller |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/admin-cli/src/site_explorer/mlx_devices/cmd.rs (1)
195-255: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer the repository’s shared table-test helpers.
This test is table-driven, but total boolean mappings should use
value_scenarios!orcheck_valuesto keep scenario labeling and test structure consistent.As per coding guidelines, prefer
value_scenarios!for total operations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/admin-cli/src/site_explorer/mlx_devices/cmd.rs` around lines 195 - 255, Refactor the table-driven test operating_as_nic_prefers_reported_mode_over_sku to use the repository’s shared value_scenarios! or check_values helper for this total boolean mapping. Preserve all existing device_kind, nic_mode, expected-result combinations and scenario labels while removing the custom Case struct and manual loop.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/admin-cli/src/site_explorer/mlx_devices/cmd.rs`:
- Around line 112-119: Update operating_as_nic to convert nic_mode with
NicMode::try_from(mode).ok(), treating invalid or unknown values like None;
return true for NicMode::Nic, otherwise apply the existing BF3 SKU fallback for
absent or unrecognized modes.
---
Nitpick comments:
In `@crates/admin-cli/src/site_explorer/mlx_devices/cmd.rs`:
- Around line 195-255: Refactor the table-driven test
operating_as_nic_prefers_reported_mode_over_sku to use the repository’s shared
value_scenarios! or check_values helper for this total boolean mapping. Preserve
all existing device_kind, nic_mode, expected-result combinations and scenario
labels while removing the custom Case struct and manual loop.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ed6b2d73-9219-4eef-92ad-2a8824736535
⛔ Files ignored due to path filters (1)
rest-api/proto/core/gen/v1/site_explorer_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (6)
crates/admin-cli/src/site_explorer/mlx_devices/args.rscrates/admin-cli/src/site_explorer/mlx_devices/cmd.rscrates/api-model/src/site_explorer/mod.rscrates/rpc/proto/site_explorer.protodocs/provisioning/boot-interfaces-and-dpu-modes.mdrest-api/proto/core/src/v1/site_explorer_nico.proto
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
chet
changed the title
`site-explorer mlx-devices` now reports what its part-number classification actually is -- the card's factory SKU -- and leaves the operating mode to `nic_mode`, the value read from the DPU's own BMC. The old labels presented the SKU as the mode ("BlueField-3 (DPU mode)" / "(NIC mode)"), built on a false premise: part numbers never change when a card flips between DPU and NIC mode. QA2 has the live counterexample -- a `900-9D3B6` DPU flipped into NIC mode (OEM `Mode = NicMode`, Arm complex `Paused`, host booted through its PF) that the CLI printed as `kind: "BlueField-3 (DPU mode)"` next to `nic_mode: "NIC"`, which read as NICo contradicting itself and got filed as a mode-mismatch bug.
`--nic-mode-only` now follows the authoritative signal too: a flipped `900-9D3B6` running as a NIC (exactly the card whose firmware scout can't see) is included, a `900-9D3B4` running as a DPU is not, and the SuperNIC SKUs still match when the mode is unknown. The new `kind` labels are just NVIDIA's product names ("BlueField-3 DPU" / "BlueField-3 SuperNIC"), with the part-number column as the discriminator. The `MlxDeviceKind` doc comments (api-model + proto, with the regenerated REST snapshots) explain the SKU-vs-mode split, and the dead `MlxDeviceKind::is_nic_mode()` is dropped. The `nic_mode` NICo reads is the OEM extension (`Systems/Bluefield/Oem/Nvidia` `Mode`) -- the surface the BlueField BMC manual documents for querying the mode, and the same OEM-for-BF3 / BIOS-for-BF2 precedence `bmc-explorer` already codifies.
Tests added!
This supports NVIDIA#3345
Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai full_review, thanks! |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/admin-cli/src/site_explorer/mlx_devices/cmd.rs (2)
195-256: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the repository’s standard table-driven test helper.
This total boolean operation uses a bespoke local case table and loop. Prefer
value_scenarios!, orcheck_valuesif the multi-field setup would obscure the table, to keep test diagnostics and conventions consistent.As per coding guidelines, total operations should prefer
value_scenarios!and table-driven helpers fromcarbide-test-support.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/admin-cli/src/site_explorer/mlx_devices/cmd.rs` around lines 195 - 256, Replace the bespoke Case array and manual loop in operating_as_nic_prefers_reported_mode_over_sku with the repository-standard value_scenarios! helper, using each scenario name, device_kind, nic_mode, and expected boolean as table inputs and preserving the assertion diagnostics. Use check_values only if value_scenarios! cannot express the multi-field setup cleanly.Source: Coding guidelines
122-127: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the new labels.
kind_labelnow changes user-visible output, but the added tests only cover filtering and firmware comparison. Assert that both SuperNIC variants render asBlueField-3 SuperNICandBf3DpuModerenders asBlueField-3 DPU.As per path instructions, changed Rust behavior should have focused test coverage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/admin-cli/src/site_explorer/mlx_devices/cmd.rs` around lines 122 - 127, Add focused regression tests for the user-visible labels returned by kind_label: assert Bf3NicMode and Bf3SuperNic both produce "BlueField-3 SuperNIC", and Bf3DpuMode produces "BlueField-3 DPU". Place the assertions with the existing mlx_devices tests, covering the enum-to-i32 conversion expected by kind_label.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/admin-cli/src/site_explorer/mlx_devices/cmd.rs`:
- Around line 195-256: Replace the bespoke Case array and manual loop in
operating_as_nic_prefers_reported_mode_over_sku with the repository-standard
value_scenarios! helper, using each scenario name, device_kind, nic_mode, and
expected boolean as table inputs and preserving the assertion diagnostics. Use
check_values only if value_scenarios! cannot express the multi-field setup
cleanly.
- Around line 122-127: Add focused regression tests for the user-visible labels
returned by kind_label: assert Bf3NicMode and Bf3SuperNic both produce
"BlueField-3 SuperNIC", and Bf3DpuMode produces "BlueField-3 DPU". Place the
assertions with the existing mlx_devices tests, covering the enum-to-i32
conversion expected by kind_label.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 81eac3d4-d179-4dbf-be2f-ff82deb70d84
⛔ Files ignored due to path filters (1)
rest-api/proto/core/gen/v1/site_explorer_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (5)
crates/admin-cli/src/site_explorer/mlx_devices/args.rscrates/admin-cli/src/site_explorer/mlx_devices/cmd.rscrates/api-model/src/site_explorer/mod.rscrates/rpc/proto/site_explorer.protorest-api/proto/core/src/v1/site_explorer_nico.proto
2 participants
NICo's mode detection was right and the operator surfaces were reporting incorrectly:
mlx-devicespresented its part-number classification as the card's operating mode, which can make a correctly-detected NIC-mode flip look like a mode-mismatch bug (kind: "BlueField-3 (DPU mode)"next tonic_mode: "NIC"on the same card). Part numbers are factory SKUs and never change with the mode --nic_mode, read from the DPU BMC, is the operating mode.Primary callouts are:
kindlabels are now NVIDIA product names ("BlueField-3 DPU" / "BlueField-3 SuperNIC") -- the SKU, with the part-number column as the discriminator.--nic-mode-onlyfilters on the authoritativenic_mode, falling back to the SuperNIC SKUs only when the mode is unknown -- a flipped900-9D3B6running as a NIC is now included, a900-9D3B4running as a DPU no longer is.MlxDeviceKind::is_nic_mode()dropped;MlxDeviceKinddoc comments corrected in api-model + proto (REST snapshots regenerated).nic_modeNICo reads is the documented query surface -- the OEM extension (Systems/Bluefield/Oem/NvidiaMode), the same OEM-for-BF3 / BIOS-for-BF2 precedencebmc-exploreralready codifies. (An operator-doc note on verifying the mode was dropped from this PR; it can land separately after tech-writer review.)Tests added!
This supports #3345