(DRAFT) fix: add support to bmc-explorer for Delta powershelves#3235
(DRAFT) fix: add support to bmc-explorer for Delta powershelves#3235spydaNVIDIA wants to merge 1 commit into
Conversation
WalkthroughThis PR adds support for Delta power shelves in bmc-explorer: chassis classification detects Delta manufacturers, aggregates per-PSU commanded power state from OEM data, introduces a ChangesDelta Power Shelf Support
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Caller as nv_generate_exploration_report
participant Chassis as ExploredChassis
participant BMC as Chassis BMC
participant Builder as build_delta_powershelf_report
participant Manager as Manager Explorer
Caller->>Chassis: explore()
Chassis->>BMC: power_supplies() (if manufacturer contains "delta")
BMC-->>Chassis: NvPowerSupply list
Chassis->>Chassis: aggregate PSU OEM flags into ModelPowerState
Caller->>Chassis: is_delta_powershelf()
Chassis-->>Caller: true
Caller->>Builder: build_delta_powershelf_report(chassis, inventories)
Builder->>Manager: explore first manager
Manager-->>Builder: manager info
Builder->>Chassis: synthesized_powershelf_system()
Chassis-->>Builder: ModelComputerSystem
Builder-->>Caller: EndpointExplorationReport (machine_setup_status done)
Related issues: None specified. Related PRs: None specified. Suggested labels: enhancement, rust, bmc-explorer Suggested reviewers: Team members familiar with bmc-explorer chassis exploration and Redfish OEM data handling. Poem A rabbit hopped through chassis wires so fine, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/bmc-explorer/src/chassis.rs (1)
150-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEmit the diagnostic as a structured field rather than an interpolated string.
The warning embeds
detailvia string interpolation, which defeats key/value log searchability. Per the project's logging conventions, pass it as an attribute.♻️ Proposed structured-logging fix
- tracing::warn!("Delta power shelf power state is unknown: {detail}"); + tracing::warn!(%detail, "Delta power shelf power state is unknown");As per coding guidelines: "prefer placing common fields as attributes passed to tracing functions instead of using string interpolation" and services should "emit logs in 'logfmt' syntax".
🤖 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/chassis.rs` at line 150, The warning in the chassis power-state handling uses string interpolation for detail, which breaks structured logging/searchability. Update the tracing::warn call in the Delta power shelf branch to pass detail as a structured attribute instead of embedding it in the message, following the existing tracing/logfmt conventions used in chassis.rs.Source: Coding guidelines
crates/bmc-explorer/src/lib.rs (1)
292-347: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the duplicated "fetch first manager" plumbing.
Lines 305-315 reproduce the manager-acquisition and error-mapping block from
nv_generate_exploration_report(lines 143-153) verbatim. Two identical copies of this fallible BMC I/O sequence are liable to drift independently. Extracting a small helper keeps the error-mapping contract in one place.♻️ Illustrative helper extraction (confirm the concrete manager type)
async fn first_manager<B: Bmc>(root: &ServiceRoot<B>) -> Result<Manager<B>, Error<B>> { root.managers() .await .map_err(Error::nv_redfish("managers"))? .ok_or_else(Error::bmc_not_provided("managers"))? .members() .await .map_err(Error::nv_redfish("managers members"))? .into_iter() .next() .ok_or_else(Error::bmc_not_provided("at least one manager")) }Both call sites then reduce to
let manager = first_manager(&root).await?;. Please verify the exact returned manager type/generics before applying.🤖 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/lib.rs` around lines 292 - 347, The manager-fetching and error-mapping logic in build_delta_powershelf_report is duplicated from nv_generate_exploration_report, so extract it into a shared helper such as first_manager that performs the root.managers() / members() / next() flow and preserves the same Error::nv_redfish and Error::bmc_not_provided behavior. Update both build_delta_powershelf_report and nv_generate_exploration_report to call the helper, and verify the helper’s return type matches the concrete manager type used by ExploredManager::explore.
🤖 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/bmc-explorer/src/chassis.rs`:
- Line 150: The warning in the chassis power-state handling uses string
interpolation for detail, which breaks structured logging/searchability. Update
the tracing::warn call in the Delta power shelf branch to pass detail as a
structured attribute instead of embedding it in the message, following the
existing tracing/logfmt conventions used in chassis.rs.
In `@crates/bmc-explorer/src/lib.rs`:
- Around line 292-347: The manager-fetching and error-mapping logic in
build_delta_powershelf_report is duplicated from nv_generate_exploration_report,
so extract it into a shared helper such as first_manager that performs the
root.managers() / members() / next() flow and preserves the same
Error::nv_redfish and Error::bmc_not_provided behavior. Update both
build_delta_powershelf_report and nv_generate_exploration_report to call the
helper, and verify the helper’s return type matches the concrete manager type
used by ExploredManager::explore.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 29f2158b-6218-4f6a-80bf-732429fdd488
📒 Files selected for processing (4)
crates/bmc-explorer/Cargo.tomlcrates/bmc-explorer/src/chassis.rscrates/bmc-explorer/src/hw/mod.rscrates/bmc-explorer/src/lib.rs
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
poroh
left a comment
poroh
left a comment
There was a problem hiding this comment.
I would prefer more clean way of implementing it with:
- Adding OEM support for Delta powershelf in nv-redfish
- Adding bmc-mock simulation of Delta powershelf
- Adding unit tests for bmc-explorer that verifies important features of this hardware support.
Please let me know if you need any help with it.
|
spydaNVIDIA
changed the title
|
Closing in favor of #3455 |
2 participants
fix: add support to bmc-explorer for Delta powershelves
Related issues
Type of Change
Breaking Changes
Testing
Additional Notes