feat: For BF4 get the pf0 mac address from NDF0 path#3115
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
WalkthroughAdds BF4-specific handling to DPU PF0 base-MAC derivation. In redfish.rs, a new fallback queries BF4 NIC inventory resources for permanent MAC when standard retrieval fails. In lib.rs, a helper classifies BF4 reports and skips the legacy BMC eth0 offset fallback for them, with corresponding tests. ChangesBF4 base-MAC derivation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
PoemA rabbit hopped through Redfish trees, 🚥 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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/site-explorer/src/lib.rs (1)
3583-3615: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider consolidating the
is_bf4_dpu_reportcases into a single value-driven table.These three
#[test]functions exercise the same total operation (is_bf4_dpu_report→bool) with differing report shapes — precisely the patternvalue_scenarios!targets. A single table (accepts BF4, rejects missing NIC topology, rejects BF3 shape) keeps each case one labeled row and localizes future additions. This is optional; the current tests are correct.As per coding guidelines: "Use
value_scenarios!for total operations (those returning a plain value,Option, orbool)."🤖 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 3583 - 3615, Consolidate the three `is_bf4_dpu_report` test cases into a single `value_scenarios!`-driven table test, since this function is a total operation returning a bool. Replace the separate `#[test]` functions with one scenario table covering the accepted BF4 report, the rejected zero-suffix case without BF4 NIC topology, and the rejected BF3-shaped report, using the existing helpers like `bf4_report_with_zero_suffix_ids`, `bf3_report_with_eth0`, and `is_bf4_dpu_report`.Source: Coding guidelines
crates/site-explorer/src/redfish.rs (1)
1155-1160: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCollapse the identical
Errarms.The
Err(RedfishError::NotSupported(_)) => continuearm is behaviorally indistinguishable from the catch-allErr(_) => continue; the explicit variant adds no value and will tripclippy::match_same_arms, which this workspace treats as an error. Consider folding the two, and if the distinction was intended to surface diagnostics, log the swallowed error atdebugbefore continuing rather than discarding it silently.♻️ Proposed simplification
- let resource = match client.get_resource(ODataId::from(path)).await { - Ok(resource) => resource, - Err(RedfishError::NotSupported(_)) => continue, - Err(_) => continue, - }; + let resource = match client.get_resource(ODataId::from(path)).await { + Ok(resource) => resource, + Err(err) => { + tracing::debug!(%path, error = %err, "BF4 NDF0 resource fetch failed"); + continue; + } + };As per coding guidelines: "Enable all clippy lints by default and treat all warnings as errors."
🤖 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/redfish.rs` around lines 1155 - 1160, The `match` on `client.get_resource(...).await` in `redfish.rs` has two `Err` arms that both do `continue`, which will trigger `clippy::match_same_arms`. Collapse the `Err(RedfishError::NotSupported(_))` and `Err(_)` handling into a single branch in the `ndf0_paths` loop, or if you need to preserve the distinction, emit a `debug` log before continuing. Keep the behavior in `client.get_resource` and the surrounding loop unchanged.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/site-explorer/src/redfish.rs`:
- Around line 829-847: The BF4 NDF0 fallback in the base_mac resolution path is
currently applied for every BlueField when base_mac is missing, which can
trigger unnecessary lookups and BF4-specific warnings on BF2/BF3. Update the
logic around get_base_mac_from_bf4_ndf0 in redfish.rs to first verify BF4
topology before attempting the fallback, or otherwise make the warning generic
and less noisy if the fallback remains unconditional. Keep the later
BF3/BMC-eth0 handling in mind so base_mac remains absent unless the BF4 path
actually applies.
---
Nitpick comments:
In `@crates/site-explorer/src/lib.rs`:
- Around line 3583-3615: Consolidate the three `is_bf4_dpu_report` test cases
into a single `value_scenarios!`-driven table test, since this function is a
total operation returning a bool. Replace the separate `#[test]` functions with
one scenario table covering the accepted BF4 report, the rejected zero-suffix
case without BF4 NIC topology, and the rejected BF3-shaped report, using the
existing helpers like `bf4_report_with_zero_suffix_ids`, `bf3_report_with_eth0`,
and `is_bf4_dpu_report`.
In `@crates/site-explorer/src/redfish.rs`:
- Around line 1155-1160: The `match` on `client.get_resource(...).await` in
`redfish.rs` has two `Err` arms that both do `continue`, which will trigger
`clippy::match_same_arms`. Collapse the `Err(RedfishError::NotSupported(_))` and
`Err(_)` handling into a single branch in the `ndf0_paths` loop, or if you need
to preserve the distinction, emit a `debug` log before continuing. Keep the
behavior in `client.get_resource` and the surrounding loop unchanged.
🪄 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: c73fdb0c-2348-492f-9d4c-9bdb1f98b0c6
📒 Files selected for processing (2)
crates/site-explorer/src/lib.rscrates/site-explorer/src/redfish.rs
| if base_mac.is_none() { | ||
| // BF4 temporary patch: | ||
| // BF4 BMC reports do not expose PF0 base MAC via the usual | ||
| // ComputerSystem BaseMAC path, so we patch `systems[].base_mac` by | ||
| // reading NDF0 PermanentMACAddress from the BF4 NIC subtree. | ||
| // | ||
| // Remove this fallback once BF4 BMC exposes PF0 base MAC directly in | ||
| // the standard system/base_mac report path. | ||
| // | ||
| // This path depends on NIC inventory being up and queryable; it may | ||
| // be absent when NIC firmware is in recovery/uninitialized states or | ||
| // when NIC-side inventory endpoints are not populated/responding. | ||
| base_mac = get_base_mac_from_bf4_ndf0(client).await; | ||
| if base_mac.is_none() { | ||
| tracing::warn!( | ||
| "BF4 NDF0 fallback did not provide PF0 base MAC (NIC inventory unavailable/uninitialized?)" | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether non-BF4 (BF2/BF3) DPUs reach this fallback with base_mac == None.
rg -nP --type=rust -C4 'get_base_mac_address|base_mac\s*=\s*None|get_base_mac_from_bf4_ndf0' crates/site-explorer/src/redfish.rsRepository: NVIDIA/infra-controller
Length of output: 1857
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the surrounding logic in crates/site-explorer/src/redfish.rs
sed -n '780,855p' crates/site-explorer/src/redfish.rs
printf '\n---\n'
# Inspect the BF3/BF4 handling that determines whether base_mac being None is expected.
rg -n -C4 'base_mac|eth0|BF3|BF4|NDF0|nic_mode|is_dpu' crates/site-explorer/src/lib.rs crates/site-explorer/src/redfish.rsRepository: NVIDIA/infra-controller
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the full control flow around the base_mac handling in redfish.rs.
nl -ba crates/site-explorer/src/redfish.rs | sed -n '798,848p'
printf '\n---\n'
# Show how base_mac is consumed later, especially for BF3/BF4-specific logic.
nl -ba crates/site-explorer/src/lib.rs | sed -n '1,260p' | rg -n -C4 'base_mac|eth0|BF3|BF4|DPU|nic_mode'Repository: NVIDIA/infra-controller
Length of output: 201
Gate the BF4 fallback on BF4 hardware.
This branch runs for every BlueField DPU with a missing base_mac, not just BF4. BF2/BF3 will still pay the two NDF0 lookups and can emit a BF4-specific warning even though the later BF3/BMC-eth0 path expects base_mac to be absent here. Gate the fallback on BF4 topology, or at least drop the BF4 wording and lower the log level.
🤖 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/redfish.rs` around lines 829 - 847, The BF4 NDF0
fallback in the base_mac resolution path is currently applied for every
BlueField when base_mac is missing, which can trigger unnecessary lookups and
BF4-specific warnings on BF2/BF3. Update the logic around
get_base_mac_from_bf4_ndf0 in redfish.rs to first verify BF4 topology before
attempting the fallback, or otherwise make the warning generic and less noisy if
the fallback remains unconditional. Keep the later BF3/BMC-eth0 handling in mind
so base_mac remains absent unless the BF4 path actually applies.
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
2 participants
For BF4 none of the legacy paths to get the host facing PCIe function (PF0) from the BMC redfish is valid. This PR changes the code to get the PF0 base mac from the following path:
/redfish/v1/Chassis/BlueField_0/NetworkAdapters/BlueField_NIC_0/NetworkDeviceFunctions/0Related issues
Path for PF0 base mac address on the BF4 BMC is now different.
Type of Change
Additional Notes
Ports match on the host and DPU bmcs:
Host BMC: /.../NetworkDeviceFunctions/0 f4:20:4d:49:53:b4 /.../NetworkDeviceFunctions/1. f4:20:4d:49:53:b5 DPU BMC: /redfish/v1/Chassis/BlueField_0/NetworkAdapters/BlueField_NIC_0/NetworkDeviceFunctions/{0,1} f4:20:4d:49:53:b4 f4:20:4d:49:53:b5