fix(redfish): support new BlueField-4 Redfish IDs#3117
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Summary by CodeRabbit
WalkthroughBlueField and BF4 identity handling now accepts renamed Redfish IDs across explorer, mock, and test paths. Redfish boot options now use a direct vector shape, hardware mocks were aligned to it, and the Nvidia Bluefield OEM mock now resolves resources from system and manager path identifiers. ChangesBlueField Identifier Generalization
Boot Options Representation and OEM Bluefield Routing Refactor
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/bmc-mock/src/redfish/oem/nvidia/bluefield.rs (1)
136-164: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
get_oem_nvidianever validates thatsystem_idcorresponds to a known system.Unlike
get_systemincomputer_system.rs, which doesstate.system_state.find(&system_id)and returns 404 for unrecognized IDs,get_oem_nvidiaaccepts an arbitrarysystem_idpath segment and echoes it straight into the fabricatedodata_idviaresource(&system_id)regardless of whether such a system actually exists in the mock state. A request to/redfish/v1/Systems/BogusId/Oem/Nvidiawill still return200 OKwith a plausible-looking (but wrong) resource, masking test bugs that hit the wrong system ID.As per path instructions for
crates/*bmc*/**, BMC-facing mocks should avoid leaking incorrect/misleading responses for invalid identifiers.🛠️ Proposed fix
async fn get_oem_nvidia( State(state): State<BmcState>, axum::extract::Path(system_id): axum::extract::Path<String>, ) -> Response { + if state.system_state.find(&system_id).is_none() { + return http::not_found(); + } let redfish::oem::State::NvidiaBluefield(state) = state.oem_state else { return http::not_found(); };🤖 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/bmc-mock/src/redfish/oem/nvidia/bluefield.rs` around lines 136 - 164, `get_oem_nvidia` currently builds an OEM response for any `system_id` without checking whether that system exists, which can return misleading 200s for invalid IDs. Update the `get_oem_nvidia` handler to validate `system_id` against the mock system state first, similar to `get_system` in `computer_system.rs` (for example by using `state.system_state.find(&system_id)`), and return `http::not_found()` when the ID is unknown before calling `resource(&system_id)`.Source: Path instructions
🧹 Nitpick comments (4)
crates/bmc-mock/src/redfish/oem/nvidia/bluefield.rs (1)
136-139: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFully-qualified
axum::extract::Pathvs importedPathalias — inconsistent style.
get_oem_nvidiaspells outaxum::extract::Path(system_id): axum::extract::Path<String>whilepatch_managers_oem_nvidia(right below) uses the already-importedPath(manager_id): Path<String>. SincePathis imported at the top of the file (use axum::extract::{Path, State};), the fully-qualified form inget_oem_nvidiais redundant.♻️ Proposed fix
async fn get_oem_nvidia( State(state): State<BmcState>, - axum::extract::Path(system_id): axum::extract::Path<String>, + Path(system_id): Path<String>, ) -> Response {Also applies to: 166-169
🤖 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/bmc-mock/src/redfish/oem/nvidia/bluefield.rs` around lines 136 - 139, Use the imported Path alias consistently in get_oem_nvidia instead of spelling out axum::extract::Path twice; update the function signature to match the style already used by patch_managers_oem_nvidia and any other similar handlers in bluefield.rs. Keep the existing Path import at the top and rely on Path(system_id): Path<String> for the extractor pattern so the codebase stays stylistically uniform.crates/bmc-mock/src/hw/bluefield4.rs (2)
44-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
MANAGER_IDandBMC_CHASSIS_IDshare the same literal value.Both constants are
"BlueField_0_BMC_0"... actually"BlueField_BMC_0". If this coincidence is intentional (manager id and chassis id happen to match on real hardware), consider a one-line comment to make it explicit; otherwise a single shared constant would remove the risk of the two drifting apart independently.🤖 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/bmc-mock/src/hw/bluefield4.rs` around lines 44 - 46, In the BlueField4 mock identifiers, MANAGER_ID and BMC_CHASSIS_ID currently use the same literal and may drift if edited separately. In the bluefield4 constants block, either introduce a single shared constant used by both MANAGER_ID and BMC_CHASSIS_ID, or add a brief comment near those definitions if the matching values are intentional on real hardware. Keep SYSTEM_ID unchanged and make the relationship between the manager and chassis IDs explicit.
44-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReference
Self::SYSTEM_IDinstead of re-hardcoding"BlueField_0".
SYSTEM_IDis defined as"BlueField_0"(Line 44) specifically to avoid scattering this literal, but the chassis config (Lines 67, 75) still hardcodes"BlueField_0"directly instead of using the constant. IfSYSTEM_IDever changes, this chassis entry (and its sensor generation) will silently drift out of sync with the system id.🔧 Proposed fix
redfish::chassis::SingleChassisConfig { - id: "BlueField_0".into(), + id: Self::SYSTEM_ID.into(), chassis_type: "Component".into(), manufacturer: Some("NVIDIA".into()), model: Some("NA".into()), part_number: Some(self.part_number().into()), serial_number: Some(self.product_serial_number.to_string().into()), pcie_devices: Some(vec![]), sensors: Some(redfish::sensor::generate_chassis_sensors( - "BlueField_0", + Self::SYSTEM_ID, Self::sensor_layout(), )), ..redfish::chassis::SingleChassisConfig::defaults() },Also applies to: 67-77, 117-120
🤖 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/bmc-mock/src/hw/bluefield4.rs` around lines 44 - 46, The BlueField chassis configuration still hardcodes the system identifier instead of reusing the existing SYSTEM_ID constant. Update the relevant BlueField4 config/build logic to reference Self::SYSTEM_ID wherever "BlueField_0" is currently repeated, including the chassis entry and sensor generation paths, so all uses stay in sync if the system id changes.crates/api-model/src/site_explorer/mod.rs (1)
642-665: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated "Card1-or-BF4-BMC-chassis fallback" pattern into a shared helper.
The same three-step lookup — check
Card1in the chassis map, else fall back tofind_mapover chassis matchingis_dpu_product_chassis_id— is now duplicated acrossDpuDataconstruction (Lines 642-665),dpu_part_number(Lines 844-856),identify_dpu(Lines 891-915),machine_id_serial_number(Lines 957-979), anddpu_pairing_serial_number(Lines 1981-2007). Any future change to the fallback rule (e.g. adding a new BF4 chassis id variant) now requires touching five call sites in lockstep, which is easy to get wrong.Consider a single private helper, e.g.:
fn dpu_chassis_field<'a, T>( &'a self, primary_id: &str, extractor: impl Fn(&'a Chassis) -> Option<T>, ) -> Option<T> { let chassis_map: HashMap<&str, &Chassis> = self.chassis.iter().map(|c| (c.id.as_str(), c)).collect(); chassis_map .get(primary_id) .and_then(|c| extractor(c)) .or_else(|| { self.chassis .iter() .filter(|c| is_dpu_product_chassis_id(&c.id)) .find_map(&extractor) }) }and have each call site delegate to it.
Also applies to: 844-856, 891-915, 957-979, 1981-2007
🤖 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/api-model/src/site_explorer/mod.rs` around lines 642 - 665, The Card1-or-BF4-BMC-chassis fallback logic is duplicated in DpuData construction, dpu_part_number, identify_dpu, machine_id_serial_number, and dpu_pairing_serial_number. Extract that three-step lookup into one private helper on SiteExplorer (or the enclosing impl) that takes the primary chassis id and an extractor closure, then have each of those call sites delegate to it. Keep the helper responsible for checking chassis_map.get("Card1") first, then falling back to iterating self.chassis filtered by is_dpu_product_chassis_id, so any future fallback rule changes happen in one place.
🤖 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.
Outside diff comments:
In `@crates/bmc-mock/src/redfish/oem/nvidia/bluefield.rs`:
- Around line 136-164: `get_oem_nvidia` currently builds an OEM response for any
`system_id` without checking whether that system exists, which can return
misleading 200s for invalid IDs. Update the `get_oem_nvidia` handler to validate
`system_id` against the mock system state first, similar to `get_system` in
`computer_system.rs` (for example by using
`state.system_state.find(&system_id)`), and return `http::not_found()` when the
ID is unknown before calling `resource(&system_id)`.
---
Nitpick comments:
In `@crates/api-model/src/site_explorer/mod.rs`:
- Around line 642-665: The Card1-or-BF4-BMC-chassis fallback logic is duplicated
in DpuData construction, dpu_part_number, identify_dpu,
machine_id_serial_number, and dpu_pairing_serial_number. Extract that three-step
lookup into one private helper on SiteExplorer (or the enclosing impl) that
takes the primary chassis id and an extractor closure, then have each of those
call sites delegate to it. Keep the helper responsible for checking
chassis_map.get("Card1") first, then falling back to iterating self.chassis
filtered by is_dpu_product_chassis_id, so any future fallback rule changes
happen in one place.
In `@crates/bmc-mock/src/hw/bluefield4.rs`:
- Around line 44-46: In the BlueField4 mock identifiers, MANAGER_ID and
BMC_CHASSIS_ID currently use the same literal and may drift if edited
separately. In the bluefield4 constants block, either introduce a single shared
constant used by both MANAGER_ID and BMC_CHASSIS_ID, or add a brief comment near
those definitions if the matching values are intentional on real hardware. Keep
SYSTEM_ID unchanged and make the relationship between the manager and chassis
IDs explicit.
- Around line 44-46: The BlueField chassis configuration still hardcodes the
system identifier instead of reusing the existing SYSTEM_ID constant. Update the
relevant BlueField4 config/build logic to reference Self::SYSTEM_ID wherever
"BlueField_0" is currently repeated, including the chassis entry and sensor
generation paths, so all uses stay in sync if the system id changes.
In `@crates/bmc-mock/src/redfish/oem/nvidia/bluefield.rs`:
- Around line 136-139: Use the imported Path alias consistently in
get_oem_nvidia instead of spelling out axum::extract::Path twice; update the
function signature to match the style already used by patch_managers_oem_nvidia
and any other similar handlers in bluefield.rs. Keep the existing Path import at
the top and rely on Path(system_id): Path<String> for the extractor pattern so
the codebase stays stylistically uniform.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ce7de090-0369-4d9f-86ce-39bb1eed36d1
📒 Files selected for processing (18)
crates/api-model/src/site_explorer/mod.rscrates/bmc-explorer/src/computer_system.rscrates/bmc-explorer/src/lib.rscrates/bmc-explorer/src/test_support.rscrates/bmc-explorer/tests/bluefield4_explore.rscrates/bmc-mock/src/hw/bluefield3.rscrates/bmc-mock/src/hw/bluefield4.rscrates/bmc-mock/src/hw/dell_poweredge_r750.rscrates/bmc-mock/src/hw/dell_poweredge_r760_bf4.rscrates/bmc-mock/src/hw/dgx_gb300_nvl.rscrates/bmc-mock/src/hw/dgx_vr_nvl.rscrates/bmc-mock/src/hw/generic_ami.rscrates/bmc-mock/src/hw/lenovo_gb300_nvl.rscrates/bmc-mock/src/hw/nvidia_dgx_h100.rscrates/bmc-mock/src/hw/supermicro_gb300_nvl.rscrates/bmc-mock/src/hw/wiwynn_gb200_nvl.rscrates/bmc-mock/src/redfish/computer_system.rscrates/bmc-mock/src/redfish/oem/nvidia/bluefield.rs
|
@coderabbitai full_review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/bmc-mock/src/redfish/oem/nvidia/bluefield.rs (1)
136-164: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate
system_idbefore returning the BlueField OEM resource.get_oem_nvidiastill accepts any/redfish/v1/Systems/{system_id}/Oem/Nvidiapath and returns a 200 with a synthetic@odata.id. Add the samestate.system_state.find(&system_id)guard used by the other system handlers so bogus IDs return 404 instead of hiding client path bugs.🤖 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/bmc-mock/src/redfish/oem/nvidia/bluefield.rs` around lines 136 - 164, The get_oem_nvidia handler currently returns a BlueField OEM resource for any system_id, which can mask invalid client paths. Add the same state.system_state.find(&system_id) validation used by the other system handlers before building the response, and return http::not_found() when the system ID does not exist. Keep the check in get_oem_nvidia alongside the existing NvidiaBluefield state match so only valid system IDs produce a 200 response.crates/bmc-explorer/src/computer_system.rs (1)
72-87: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winKeep boot-options retrieval lazy
boot_optionsis only used for the Bluefield OOB fallback, but the fetch now runs on every system up front. Reintroduce the hardware gate or move the call into the fallback path to avoid an extra Redfish round-trip and avoidable failure surface on non-Bluefield systems.🤖 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/bmc-explorer/src/computer_system.rs` around lines 72 - 87, The eager boot-options fetch in ComputerSystem::explore should be removed or gated so it only happens for the Bluefield OOB fallback path. Move the system.boot_options() lookup into the fallback branch that actually needs it, or guard it with the same hardware check used there, to avoid unnecessary Redfish calls and failures on non-Bluefield systems.Source: Path instructions
🧹 Nitpick comments (3)
crates/bmc-mock/src/redfish/oem/nvidia/bluefield.rs (1)
137-138: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent extractor qualification.
Pathis imported directly (Line 22) and used bare inpatch_managers_oem_nvidia(Line 168), butget_oem_nvidiastill spells it out asaxum::extract::Path. Purely stylistic, but worth aligning for readability.✏️ Suggested tidy-up
-async fn get_oem_nvidia( - State(state): State<BmcState>, - axum::extract::Path(system_id): axum::extract::Path<String>, -) -> Response { +async fn get_oem_nvidia( + State(state): State<BmcState>, + Path(system_id): Path<String>, +) -> Response {🤖 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/bmc-mock/src/redfish/oem/nvidia/bluefield.rs` around lines 137 - 138, Align the extractor style in get_oem_nvidia with patch_managers_oem_nvidia by using the already imported Path directly instead of the fully qualified axum::extract::Path. This is a readability-only cleanup in the bluefield redfish handlers, so update the parameter pattern alongside State<BmcState> to keep extractor usage consistent throughout the file.crates/api-model/src/site_explorer/mod.rs (2)
637-665: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the repeated "primary chassis field, else scan recognized BlueField chassis" pattern.
The same fallback shape — try
Card1(or the system serial), elsefind_mapoverchassis.iter().filter(|c| is_dpu_product_chassis_id(&c.id))— is now duplicated five times:hardware_info()(part_number/part_description),dpu_part_number(),identify_dpu()'s model lookup,machine_id_serial_number(), anddpu_pairing_serial_number(). Extracting a small generic helper would reduce the surface area for drift (e.g. if a sixth BF4 chassis-id variant needs to be added later, five call sites must be kept in sync).♻️ Sketch of a shared helper
+fn dpu_product_field<'a, T>( + chassis: &'a [Chassis], + primary: Option<&'a Chassis>, + extract: impl Fn(&'a Chassis) -> Option<T>, +) -> Option<T> { + primary + .and_then(&extract) + .or_else(|| { + chassis + .iter() + .filter(|c| is_dpu_product_chassis_id(&c.id)) + .find_map(&extract) + }) +}As per coding guidelines: "Avoid needless clones... the ownership model may need some rethinking" and the broader DRY principle called out for essential refactors in this repo's style guide.
Also applies to: 839-857, 900-915, 957-979, 1981-2007
🤖 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/api-model/src/site_explorer/mod.rs` around lines 637 - 665, The repeated “primary field, else scan recognized BlueField chassis” fallback is duplicated across hardware_info(), dpu_part_number(), identify_dpu(), machine_id_serial_number(), and dpu_pairing_serial_number(), so extract a small shared helper in site_explorer/mod.rs to centralize the Card1/system-serial lookup plus chassis.iter().filter(|c| is_dpu_product_chassis_id(&c.id)).find_map(...) logic. Update the affected call sites to use the helper and preserve the existing behavior for part_number, part_description, model, and serial lookups while avoiding extra clones or ownership churn.Source: Coding guidelines
1781-1789: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRecognized BlueField ID literals are duplicated across crates.
is_dpu_product_chassis_id(chassis ids) andis_bluefield_system(system ids) hard-code the legacy/new identifier pairs here, whilecrates/bmc-explorer/src/lib.rsindependently hard-codes the equivalentis_bluefield_system_idmatch on"Bluefield" | "BlueField_0". A future firmware revision introducing a third identifier would require updates in both crates with no compiler-enforced link between them.🤖 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/api-model/src/site_explorer/mod.rs` around lines 1781 - 1789, The BlueField identifier literals are duplicated in the site explorer and BMC explorer match helpers, so update the matching logic to use a single shared source of truth instead of hard-coding the same legacy/new pairs in multiple places. Introduce or reuse a common helper/constants for the BlueField chassis and system ID checks, then have is_dpu_product_chassis_id, is_bluefield_system, and is_bluefield_system_id delegate to that shared logic so future identifier additions only need one update.
🤖 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/bmc-mock/src/hw/bluefield4.rs`:
- Around line 44-50: The BlueField4 mock still hardcodes the system identifier
instead of reusing the shared constant, which can drift from system_config().
Update chassis_config() and its generate_chassis_sensors call to reference
Self::SYSTEM_ID rather than the "BlueField_0" literal, keeping the chassis ID
aligned with the SYSTEM_ID constant already defined in bluefield4.rs.
In `@crates/bmc-mock/src/redfish/computer_system.rs`:
- Around line 620-633: Update the Dell-specific sort in
computer_system::boot_options_order so it matches against each boot option’s
boot_reference() instead of boot_options[i].id, since BootOrder and override
resolution already use boot_reference() and the current comparison can misorder
Dell fixtures with differing resource ids. Keep the change localized to the
BootOrderMode::DellOem branch and preserve the existing sort-by-boot-order
behavior, just switching the identifier used in the lookup.
---
Outside diff comments:
In `@crates/bmc-explorer/src/computer_system.rs`:
- Around line 72-87: The eager boot-options fetch in ComputerSystem::explore
should be removed or gated so it only happens for the Bluefield OOB fallback
path. Move the system.boot_options() lookup into the fallback branch that
actually needs it, or guard it with the same hardware check used there, to avoid
unnecessary Redfish calls and failures on non-Bluefield systems.
In `@crates/bmc-mock/src/redfish/oem/nvidia/bluefield.rs`:
- Around line 136-164: The get_oem_nvidia handler currently returns a BlueField
OEM resource for any system_id, which can mask invalid client paths. Add the
same state.system_state.find(&system_id) validation used by the other system
handlers before building the response, and return http::not_found() when the
system ID does not exist. Keep the check in get_oem_nvidia alongside the
existing NvidiaBluefield state match so only valid system IDs produce a 200
response.
---
Nitpick comments:
In `@crates/api-model/src/site_explorer/mod.rs`:
- Around line 637-665: The repeated “primary field, else scan recognized
BlueField chassis” fallback is duplicated across hardware_info(),
dpu_part_number(), identify_dpu(), machine_id_serial_number(), and
dpu_pairing_serial_number(), so extract a small shared helper in
site_explorer/mod.rs to centralize the Card1/system-serial lookup plus
chassis.iter().filter(|c| is_dpu_product_chassis_id(&c.id)).find_map(...) logic.
Update the affected call sites to use the helper and preserve the existing
behavior for part_number, part_description, model, and serial lookups while
avoiding extra clones or ownership churn.
- Around line 1781-1789: The BlueField identifier literals are duplicated in the
site explorer and BMC explorer match helpers, so update the matching logic to
use a single shared source of truth instead of hard-coding the same legacy/new
pairs in multiple places. Introduce or reuse a common helper/constants for the
BlueField chassis and system ID checks, then have is_dpu_product_chassis_id,
is_bluefield_system, and is_bluefield_system_id delegate to that shared logic so
future identifier additions only need one update.
In `@crates/bmc-mock/src/redfish/oem/nvidia/bluefield.rs`:
- Around line 137-138: Align the extractor style in get_oem_nvidia with
patch_managers_oem_nvidia by using the already imported Path directly instead of
the fully qualified axum::extract::Path. This is a readability-only cleanup in
the bluefield redfish handlers, so update the parameter pattern alongside
State<BmcState> to keep extractor usage consistent throughout the file.
🪄 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: b8726714-da02-4fb2-adfa-b2e3b6f90f39
📒 Files selected for processing (18)
crates/api-model/src/site_explorer/mod.rscrates/bmc-explorer/src/computer_system.rscrates/bmc-explorer/src/lib.rscrates/bmc-explorer/src/test_support.rscrates/bmc-explorer/tests/bluefield4_explore.rscrates/bmc-mock/src/hw/bluefield3.rscrates/bmc-mock/src/hw/bluefield4.rscrates/bmc-mock/src/hw/dell_poweredge_r750.rscrates/bmc-mock/src/hw/dell_poweredge_r760_bf4.rscrates/bmc-mock/src/hw/dgx_gb300_nvl.rscrates/bmc-mock/src/hw/dgx_vr_nvl.rscrates/bmc-mock/src/hw/generic_ami.rscrates/bmc-mock/src/hw/lenovo_gb300_nvl.rscrates/bmc-mock/src/hw/nvidia_dgx_h100.rscrates/bmc-mock/src/hw/supermicro_gb300_nvl.rscrates/bmc-mock/src/hw/wiwynn_gb200_nvl.rscrates/bmc-mock/src/redfish/computer_system.rscrates/bmc-mock/src/redfish/oem/nvidia/bluefield.rs
| const SYSTEM_ID: &'static str = "BlueField_0"; | ||
| const MANAGER_ID: &'static str = "BlueField_BMC_0"; | ||
| const BMC_CHASSIS_ID: &'static str = "BlueField_BMC_0"; | ||
|
|
||
| fn sensor_layout() -> redfish::sensor::Layout { | ||
| // BF4 Card1 dump contains 96 sensors total. The generic mock | ||
| // The older BF4 layout exposed these sensors below Card1. Newer | ||
| // firmware renamed the main card chassis to BlueField_0. The generic mock |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use Self::SYSTEM_ID instead of the "BlueField_0" literal.
The new SYSTEM_ID constant is faithfully used in system_config() (Lines 117, 120), but chassis_config() still hardcodes the same literal at Line 67 and again in the generate_chassis_sensors call at Line 75. This defeats the purpose of introducing the constant — a future rename of SYSTEM_ID would silently desync the chassis id from the system id.
♻️ Proposed fix
redfish::chassis::SingleChassisConfig {
- id: "BlueField_0".into(),
+ id: Self::SYSTEM_ID.into(),
chassis_type: "Component".into(),
manufacturer: Some("NVIDIA".into()),
model: Some("NA".into()),
part_number: Some(self.part_number().into()),
serial_number: Some(self.product_serial_number.to_string().into()),
pcie_devices: Some(vec![]),
sensors: Some(redfish::sensor::generate_chassis_sensors(
- "BlueField_0",
+ Self::SYSTEM_ID,
Self::sensor_layout(),
)),
..redfish::chassis::SingleChassisConfig::defaults()
},Also applies to: 67-77
🤖 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/bmc-mock/src/hw/bluefield4.rs` around lines 44 - 50, The BlueField4
mock still hardcodes the system identifier instead of reusing the shared
constant, which can drift from system_config(). Update chassis_config() and its
generate_chassis_sensors call to reference Self::SYSTEM_ID rather than the
"BlueField_0" literal, keeping the chassis ID aligned with the SYSTEM_ID
constant already defined in bluefield4.rs.
| let boot_options_order = match system_state.config.boot_order_mode { | ||
| BootOrderMode::DellOem => { | ||
| // Carbide relies that Dell sorts boot options in according to boot | ||
| // order. Code below simulates the same. | ||
| if let Some(boot_order) = system_state.boot_order_override() { | ||
| let mut indices = (0..boot_options.len()).collect::<Vec<_>>(); | ||
| indices.sort_by_key(|&i| { | ||
| boot_order | ||
| .iter() | ||
| .enumerate() | ||
| .find(|(_, id)| *id == &boot_options[i].id) | ||
| .map(|(idx, _)| idx) | ||
| .unwrap_or(boot_options.len()) | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Sort Dell boot-option members by boot reference, not resource id.
Lines 346-352 publish BootOrder from boot_reference(), and Lines 296-301 resolve overrides with boot_reference(), but this Dell sort compares overrides to boot_options[i].id. If a Dell fixture ever has different resource ids and boot references, the mock will ignore the requested order.
Proposed fix
indices.sort_by_key(|&i| {
boot_order
.iter()
.enumerate()
- .find(|(_, id)| *id == &boot_options[i].id)
+ .find(|(_, id)| id.as_str() == boot_options[i].boot_reference())
.map(|(idx, _)| idx)
.unwrap_or(boot_options.len())
});As per path instructions, BMC-facing code should be reviewed for vendor differences and behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let boot_options_order = match system_state.config.boot_order_mode { | |
| BootOrderMode::DellOem => { | |
| // Carbide relies that Dell sorts boot options in according to boot | |
| // order. Code below simulates the same. | |
| if let Some(boot_order) = system_state.boot_order_override() { | |
| let mut indices = (0..boot_options.len()).collect::<Vec<_>>(); | |
| indices.sort_by_key(|&i| { | |
| boot_order | |
| .iter() | |
| .enumerate() | |
| .find(|(_, id)| *id == &boot_options[i].id) | |
| .map(|(idx, _)| idx) | |
| .unwrap_or(boot_options.len()) | |
| }); | |
| let boot_options_order = match system_state.config.boot_order_mode { | |
| BootOrderMode::DellOem => { | |
| // Carbide relies that Dell sorts boot options in according to boot | |
| // order. Code below simulates the same. | |
| if let Some(boot_order) = system_state.boot_order_override() { | |
| let mut indices = (0..boot_options.len()).collect::<Vec<_>>(); | |
| indices.sort_by_key(|&i| { | |
| boot_order | |
| .iter() | |
| .enumerate() | |
| .find(|(_, id)| id.as_str() == boot_options[i].boot_reference()) | |
| .map(|(idx, _)| idx) | |
| .unwrap_or(boot_options.len()) | |
| }); |
🤖 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/bmc-mock/src/redfish/computer_system.rs` around lines 620 - 633,
Update the Dell-specific sort in computer_system::boot_options_order so it
matches against each boot option’s boot_reference() instead of
boot_options[i].id, since BootOrder and override resolution already use
boot_reference() and the current comparison can misorder Dell fixtures with
differing resource ids. Keep the change localized to the BootOrderMode::DellOem
branch and preserve the existing sort-by-boot-order behavior, just switching the
identifier used in the lookup.
Source: Path instructions
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
e15598b to
f9c95ae
Compare
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3117.docs.buildwithfern.com/infra-controller |
Handle newer BF4 firmware Redfish IDs for systems, managers, and product chassis while preserving legacy BlueField IDs. Use the product BMC chassis for BF4 identity/pairing serials and update mocks/tests to match newer firmware behavior. Signed-off-by: Dmitry Porokh <dporokh@nvidia.com>
Signed-off-by: Dmitry Porokh <dporokh@nvidia.com>
bb19f17 to
253c592
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/site-explorer/src/lib.rs (3)
3562-3567: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAttach
legacy_erras a structured tracing field.The BF4 fallback-skip warning drops the underlying
legacy_errdetail entirely, even though it is available and would help diagnose why the DPU_SYS_IMAGE path failed for a given endpoint.♻️ Proposed fix
if is_bf4_dpu_report(&dpu_ep.report) { tracing::warn!( - "DPU_SYS_IMAGE derivation failed for BF4; expected PF0 base MAC from NDF0-patched systems[].base_mac, skipping BMC eth0 offset fallback" + error = %legacy_err, + "DPU_SYS_IMAGE derivation failed for BF4; expected PF0 base MAC from NDF0-patched systems[].base_mac, skipping BMC eth0 offset fallback" ); return Err(legacy_err); }As per coding guidelines, "prefer placing common fields as attributes passed to tracing functions instead of using string interpolation" and
erroris called out as one of the most important fields to expose.🤖 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/site-explorer/src/lib.rs` around lines 3562 - 3567, The BF4 fallback-skip warning in the `is_bf4_dpu_report` branch drops the available `legacy_err` detail, so update the `tracing::warn!` call to attach `legacy_err` as a structured field instead of only logging the static message. Use the existing `legacy_err` value in that warning path so the `DPU_SYS_IMAGE` failure context is preserved when the code returns `Err(legacy_err)`.Source: Coding guidelines
3573-3595: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider named constants for the BF4 topology IDs.
is_bf4_dpu_reportrepeats raw literals ("BlueField_0","BlueField_NIC_0","BlueField_BMC_0") inline. Note that"BlueField_0"here means "chassis id", which is a different Redfish resource than the"BlueField_0"system id checked byis_bluefield_system— extracting named constants (e.g.const BF4_CHASSIS_ID,BF4_NIC_ID,BF4_BMC_MANAGER_ID) would make the distinction self-documenting and reduce the risk of a future typo silently breaking detection.As per coding guidelines, "when a value has a finite set of possibilities, model it as an enum... Reserve raw strings for genuinely open-ended values," though this mirrors an existing pattern (
is_bluefield_system) elsewhere in the codebase, so this is optional polish rather than a blocker.🤖 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/site-explorer/src/lib.rs` around lines 3573 - 3595, The BF4 detection logic in is_bf4_dpu_report uses repeated raw topology ID strings inline, which makes the chassis/system distinction easy to miss and prone to typos. Refactor the literals used for the chassis, NIC, and BMC manager IDs into named constants (for example, BF4_CHASSIS_ID, BF4_NIC_ID, and BF4_BMC_MANAGER_ID) and update the comparisons in is_bf4_dpu_report to use them, keeping the existing is_bluefield_system check unchanged.Source: Coding guidelines
3921-3983: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the three
is_bf4_dpu_reporttests into a table.
is_bf4_dpu_report_detects_zero_suffix_ids_without_model_string,is_bf4_dpu_report_rejects_zero_suffix_ids_without_bf4_nic_topology, andis_bf4_dpu_report_does_not_match_bf3_shapeall exercise the same total operation (is_bf4_dpu_report: &EndpointExplorationReport -> bool) with different report shapes. This is exactly the case the repo's table-driven testing convention targets.♻️ Suggested direction
check_values( [ Case { scenario: "zero-suffix Bluefield system id", input: bf4_report_with_zero_suffix_ids("Bluefield"), expect: true }, Case { scenario: "zero-suffix BlueField_0 system id", input: bf4_report_with_zero_suffix_ids("BlueField_0"), expect: true }, Case { scenario: "missing BF4 NIC topology", input: /* report without BlueField_NIC_0 */, expect: false }, Case { scenario: "BF3 shape", input: bf3_report_with_eth0("5c:25:73:9e:ac:eb"), expect: false }, ], is_bf4_dpu_report, );As per coding guidelines, "Reach for a table whenever two or more tests call the same operation with different inputs," and
value_scenarios!/check_valuesare the preferred tools for total (bool-returning) operations like this one.🤖 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/site-explorer/src/lib.rs` around lines 3921 - 3983, Consolidate the three `is_bf4_dpu_report` tests into a single table-driven test using the repo’s `check_values` or `value_scenarios!` pattern, since they all exercise the same boolean operation with different `EndpointExplorationReport` inputs. Keep the existing scenarios from `bf4_report_with_zero_suffix_ids`, the missing `BlueField_NIC_0` topology case, and `bf3_report_with_eth0`, and verify them through one table that names each scenario and expected result. Use `is_bf4_dpu_report` as the function under test so the test suite matches the project’s table-testing convention.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.
Nitpick comments:
In `@crates/site-explorer/src/lib.rs`:
- Around line 3562-3567: The BF4 fallback-skip warning in the
`is_bf4_dpu_report` branch drops the available `legacy_err` detail, so update
the `tracing::warn!` call to attach `legacy_err` as a structured field instead
of only logging the static message. Use the existing `legacy_err` value in that
warning path so the `DPU_SYS_IMAGE` failure context is preserved when the code
returns `Err(legacy_err)`.
- Around line 3573-3595: The BF4 detection logic in is_bf4_dpu_report uses
repeated raw topology ID strings inline, which makes the chassis/system
distinction easy to miss and prone to typos. Refactor the literals used for the
chassis, NIC, and BMC manager IDs into named constants (for example,
BF4_CHASSIS_ID, BF4_NIC_ID, and BF4_BMC_MANAGER_ID) and update the comparisons
in is_bf4_dpu_report to use them, keeping the existing is_bluefield_system check
unchanged.
- Around line 3921-3983: Consolidate the three `is_bf4_dpu_report` tests into a
single table-driven test using the repo’s `check_values` or `value_scenarios!`
pattern, since they all exercise the same boolean operation with different
`EndpointExplorationReport` inputs. Keep the existing scenarios from
`bf4_report_with_zero_suffix_ids`, the missing `BlueField_NIC_0` topology case,
and `bf3_report_with_eth0`, and verify them through one table that names each
scenario and expected result. Use `is_bf4_dpu_report` as the function under test
so the test suite matches the project’s table-testing convention.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 08c31c80-91c8-42a9-85d6-eaea55745943
📒 Files selected for processing (20)
crates/api-model/src/site_explorer/mod.rscrates/bmc-explorer/src/computer_system.rscrates/bmc-explorer/src/lib.rscrates/bmc-explorer/src/test_support.rscrates/bmc-explorer/tests/bluefield4_explore.rscrates/bmc-mock/src/hw/bluefield3.rscrates/bmc-mock/src/hw/bluefield4.rscrates/bmc-mock/src/hw/dell_poweredge_r750.rscrates/bmc-mock/src/hw/dell_poweredge_r760_bf4.rscrates/bmc-mock/src/hw/dgx_gb300_nvl.rscrates/bmc-mock/src/hw/dgx_vr_nvl.rscrates/bmc-mock/src/hw/generic_ami.rscrates/bmc-mock/src/hw/hpe_proliant_dl380a_gen11.rscrates/bmc-mock/src/hw/lenovo_gb300_nvl.rscrates/bmc-mock/src/hw/nvidia_dgx_h100.rscrates/bmc-mock/src/hw/supermicro_gb300_nvl.rscrates/bmc-mock/src/hw/wiwynn_gb200_nvl.rscrates/bmc-mock/src/redfish/computer_system.rscrates/bmc-mock/src/redfish/oem/nvidia/bluefield.rscrates/site-explorer/src/lib.rs
✅ Files skipped from review due to trivial changes (3)
- crates/bmc-mock/src/hw/dell_poweredge_r760_bf4.rs
- crates/bmc-mock/src/hw/lenovo_gb300_nvl.rs
- crates/bmc-mock/src/hw/bluefield3.rs
🚧 Files skipped from review as they are similar to previous changes (16)
- crates/bmc-mock/src/hw/nvidia_dgx_h100.rs
- crates/bmc-mock/src/hw/hpe_proliant_dl380a_gen11.rs
- crates/bmc-mock/src/hw/generic_ami.rs
- crates/bmc-explorer/src/test_support.rs
- crates/bmc-mock/src/hw/dell_poweredge_r750.rs
- crates/bmc-mock/src/hw/wiwynn_gb200_nvl.rs
- crates/bmc-mock/src/hw/dgx_gb300_nvl.rs
- crates/bmc-explorer/src/computer_system.rs
- crates/bmc-mock/src/hw/dgx_vr_nvl.rs
- crates/bmc-mock/src/hw/supermicro_gb300_nvl.rs
- crates/bmc-mock/src/redfish/oem/nvidia/bluefield.rs
- crates/bmc-explorer/tests/bluefield4_explore.rs
- crates/bmc-mock/src/redfish/computer_system.rs
- crates/bmc-explorer/src/lib.rs
- crates/bmc-mock/src/hw/bluefield4.rs
- crates/api-model/src/site_explorer/mod.rs
2 participants
Handle newer BF4 firmware Redfish IDs for systems, managers, and product chassis while preserving legacy BlueField IDs. Use the product BMC chassis for BF4 identity/pairing serials and update mocks/tests to match newer firmware behavior.
Related issues
Type of Change
Breaking Changes
Testing
Additional Notes