fix(bmc-mock): better support of BF4 Nvidia OEM endpoint#3072
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
WalkthroughBluefieldState now models BF3 and BF4 as separate variants, with constructors and mode handling split accordingly. OEM state generation is delegated through DpuMachineInfo, and the site-explorer BF4 regression test was removed. ChangesBlueField OEM state refactor
Site explorer BF4 regression removal
Estimated code review effort: 2 (Simple) | ~15 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/bmc-mock/src/redfish/oem/nvidia/bluefield.rs (1)
200-203: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a BF4 regression case for the changed endpoint contract.
The updated test still only exercises BF3 staging. Add a BF4 case verifying
/Oem/NvidiaomitsMode/BaseMACand covers the chosenMode.Setbehavior for BF4. Prefer table-driven coverage if setup is shared.As per coding guidelines, verification should exercise the behavior that changed and Rust tests should prefer table-driven style.
🤖 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 200 - 203, The current test coverage in BluefieldState only exercises BF3 staging, so add a BF4 regression case for the changed /Oem/Nvidia contract. Update the existing mode-setting coverage around mode_set_is_staged_and_applied_on_power_on, or factor shared setup into a table-driven test, to verify that BF4 omits Mode and BaseMAC from the OEM payload and that the chosen Mode.Set behavior is exercised for BF4. Keep the shared assertions in BluefieldState-related tests so both BF3 and BF4 behavior are covered clearly.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/bmc-mock/src/redfish/oem/nvidia/bluefield.rs`:
- Around line 64-79: Bluefield4 mode handling currently treats unsupported
operations as a successful no-op by returning false from nic_mode() and
accepting stage_mode() without error. Update the Bluefield enum handling so
Bluefield3 and Bluefield4 are distinguished in nic_mode() and stage_mode(), and
make Bluefield4 return an unsupported/error result for Mode.Set instead of
silently mapping absence of state to false; if needed, change the API to surface
Option<bool> so callers can tell “no mode” apart from DPU mode.
---
Nitpick comments:
In `@crates/bmc-mock/src/redfish/oem/nvidia/bluefield.rs`:
- Around line 200-203: The current test coverage in BluefieldState only
exercises BF3 staging, so add a BF4 regression case for the changed /Oem/Nvidia
contract. Update the existing mode-setting coverage around
mode_set_is_staged_and_applied_on_power_on, or factor shared setup into a
table-driven test, to verify that BF4 omits Mode and BaseMAC from the OEM
payload and that the chosen Mode.Set behavior is exercised for BF4. Keep the
shared assertions in BluefieldState-related tests so both BF3 and BF4 behavior
are covered clearly.
🪄 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: 8f3f5de2-7402-4cf8-8a18-fc0c8b51813a
📒 Files selected for processing (2)
crates/bmc-mock/src/machine_info.rscrates/bmc-mock/src/redfish/oem/nvidia/bluefield.rs
| /// Whether the BlueField currently reports NIC mode. | ||
| pub fn nic_mode(&self) -> bool { | ||
| self.mode.lock().unwrap().nic_mode | ||
| match self { | ||
| Self::Bluefield3 { mode, .. } => mode.lock().unwrap().nic_mode, | ||
| Self::Bluefield4 => false, | ||
| } | ||
| } | ||
|
|
||
| /// Queue a `Mode.Set`; it takes effect on the next power cycle. | ||
| fn stage_mode(&self, nic_mode: bool) { | ||
| self.mode.lock().unwrap().pending_nic_mode = Some(nic_mode); | ||
| match self { | ||
| Self::Bluefield3 { mode, .. } => { | ||
| mode.lock().unwrap().pending_nic_mode = Some(nic_mode); | ||
| } | ||
| Self::Bluefield4 => (), | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify BF4 Mode.Set handling and downstream interpretation.
sed -n '108,131p' crates/bmc-mock/src/redfish/oem/nvidia/bluefield.rs
rg -nP -C3 '\bstage_mode\s*\(' crates/bmc-mock/src
rg -nP -C3 '\bbluefield_nic_mode\s*\(' cratesRepository: NVIDIA/infra-controller
Length of output: 4892
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the PATCH handler and surrounding BlueField state model.
sed -n '1,260p' crates/bmc-mock/src/redfish/oem/nvidia/bluefield.rs
printf '\n---\n'
sed -n '1,140p' crates/bmc-mock/src/bmc_state.rs
printf '\n---\n'
sed -n '380,430p' crates/machine-a-tron/src/machine_state_machine.rs
sed -n '800,830p' crates/machine-a-tron/src/machine_state_machine.rs
# Look for any BF4-specific handling or rejection in the mock BMC redfish code.
rg -nC2 'Bluefield4|Mode\.Set|patch_managers_oem_nvidia|parse_requested_mode|nic_mode\(' crates/bmc-mock/srcRepository: NVIDIA/infra-controller
Length of output: 34628
Reject unsupported BF4 mode changes instead of silently mapping them to false. Bluefield4 has no mode state, but nic_mode() collapses that to false and stage_mode() accepts Mode.Set as a no-op, so callers get a successful write for an unsupported operation. Return an unsupported/error result for BF4, or surface Option<bool> so absence stays distinct from DPU mode.
🤖 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 64 - 79,
Bluefield4 mode handling currently treats unsupported operations as a successful
no-op by returning false from nic_mode() and accepting stage_mode() without
error. Update the Bluefield enum handling so Bluefield3 and Bluefield4 are
distinguished in nic_mode() and stage_mode(), and make Bluefield4 return an
unsupported/error result for Mode.Set instead of silently mapping absence of
state to false; if needed, change the API to surface Option<bool> so callers can
tell “no mode” apart from DPU mode.
Source: Path instructions
poroh
force-pushed
the
bmc-mock-bf4-nvidia-oem
branch
from
158ac80 to
e59bdf4
Compare
BF4 doesn't provide BaseMac and NicMode attributes in its /redfish/v1/Systems/Bluefield/Oem/Nvidia path. Signed-off-by: Dmitry Porokh <dporokh@nvidia.com>
poroh
force-pushed
the
bmc-mock-bf4-nvidia-oem
branch
from
e59bdf4 to
80097bc
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/bmc-mock/src/redfish/oem/nvidia/bluefield.rs (1)
60-94: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUnresolved: BF4
Mode.Setstill silently succeeds instead of rejecting.The enum refactor preserves the exact behavior flagged in a prior review:
nic_mode()returnsfalseforBluefield4andstage_mode()/apply_pending_mode()are silent no-ops. A caller issuingMode.Setagainst a BF4 system therefore gets what looks like a successful write for an operation BF4 doesn't support, rather than a rejected/unsupported result. Since BF4 genuinely has no mode state (per PR objective), consider surfacing this at the PATCH handler level (rejectMode.SetforBluefield4with an error) instead of quietly absorbing it instage_mode().As per path instructions, BMC-facing code should account for vendor differences and avoid masking those differences behind uniform success responses.
🤖 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 60 - 94, BF4 Mode.Set is still being treated as a no-op, so the PATCH path returns success for an unsupported operation. Update the Redfish handler that calls `stage_mode()` for Bluefield systems to explicitly reject `Mode.Set` when the target is `Bluefield4`, and return an unsupported/error response instead of silently absorbing it. Use the `Bluefield` enum match and the `stage_mode`/`apply_pending_mode` flow to locate the change, and keep BF3’s pending-mode behavior unchanged.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.
Duplicate comments:
In `@crates/bmc-mock/src/redfish/oem/nvidia/bluefield.rs`:
- Around line 60-94: BF4 Mode.Set is still being treated as a no-op, so the
PATCH path returns success for an unsupported operation. Update the Redfish
handler that calls `stage_mode()` for Bluefield systems to explicitly reject
`Mode.Set` when the target is `Bluefield4`, and return an unsupported/error
response instead of silently absorbing it. Use the `Bluefield` enum match and
the `stage_mode`/`apply_pending_mode` flow to locate the change, and keep BF3’s
pending-mode behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 37c29eac-7c6f-4eec-bd07-0926675dd142
📒 Files selected for processing (3)
crates/bmc-mock/src/machine_info.rscrates/bmc-mock/src/redfish/oem/nvidia/bluefield.rscrates/site-explorer/tests/integration/site_explorer.rs
💤 Files with no reviewable changes (1)
- crates/site-explorer/tests/integration/site_explorer.rs
✅ Files skipped from review due to trivial changes (1)
- crates/bmc-mock/src/machine_info.rs
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
2 participants
BF4 doesn't provide BaseMac and NicMode attributes in its /redfish/v1/Systems/Bluefield/Oem/Nvidia path.
Related issues
Type of Change
Breaking Changes
Testing
Additional Notes