feat(power-control): route standalone vs rack-scale machines separately#3873
Conversation
Summary by CodeRabbit
WalkthroughThe component manager now classifies machine power-control requests, persists desired state before dispatch, routes rack-scale and standalone systems through distinct control paths, and maps failures per machine. Machine-state reconciliation defers power handling for pending maintenance, with integration coverage for queued power-off execution. ChangesCompute-tray power-control routing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ComponentManager
participant MachineStateHandler
participant StateController
participant ComputeTrayManager
participant SiteExploration
ComponentManager->>ComponentManager: Classify machines and persist desired power options
ComponentManager->>StateController: Route eligible rack-scale maintenance requests
ComponentManager->>ComputeTrayManager: Dispatch synchronous rack-scale or standalone power control
ComputeTrayManager-->>ComponentManager: Return per-machine results and dispatched BMC IPs
ComponentManager->>SiteExploration: Re-explore dispatched BMC IPs
MachineStateHandler->>MachineStateHandler: Defer power handling when maintenance is pending
MachineStateHandler->>StateController: Reconcile queued PowerOff maintenance
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai full review please |
|
✅ Action performedFull review finished. |
![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/api-core/src/handlers/component_manager.rs (1)
1645-1646: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffOptional: the same machine set is fetched from the database multiple times.
partition_compute_machines_by_rack_scaleperforms adb::machine::findhere, and each subsequentdispatch_compute_tray_power_controlcall re-queries the very same rows viaresolve_compute_tray_endpoints. For large targets this is up to three redundant round trips. Consider fetching once and threading the resolvedMachinerecords (or endpoints) through the partition and dispatch steps.🤖 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-core/src/handlers/component_manager.rs` around lines 1645 - 1646, Optimize the flow around partition_compute_machines_by_rack_scale and dispatch_compute_tray_power_control by fetching the target machines once and reusing the resolved Machine records or endpoints throughout partitioning and dispatch. Thread the fetched data through the relevant calls, including resolve_compute_tray_endpoints, while preserving the existing rack-scale and standalone grouping behavior.
🤖 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/api-core/src/handlers/component_manager.rs`:
- Around line 1660-1740: Track machine IDs that enter the final error arm of
update_power_option in a failed-ID set, while still appending their
error_result. Before the rack-scale and standalone dispatch blocks, filter
rack_scale_ids and standalone_ids to exclude those failed IDs, preserving
successful machines and ensuring each failed machine produces only its recorded
error result.
---
Nitpick comments:
In `@crates/api-core/src/handlers/component_manager.rs`:
- Around line 1645-1646: Optimize the flow around
partition_compute_machines_by_rack_scale and dispatch_compute_tray_power_control
by fetching the target machines once and reusing the resolved Machine records or
endpoints throughout partitioning and dispatch. Thread the fetched data through
the relevant calls, including resolve_compute_tray_endpoints, while preserving
the existing rack-scale and standalone grouping behavior.
🪄 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: 6d9fcb09-6cbb-4d42-aa95-2efac183ff2c
📒 Files selected for processing (1)
crates/api-core/src/handlers/component_manager.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/api-core/src/handlers/component_manager.rs`:
- Around line 1703-1740: Update the rack and standalone dispatch handling around
queue_machine_power_control_via_state_controller and
dispatch_compute_tray_power_control so an Err from either partition is converted
into per-machine error_result entries rather than propagated with ?. Preserve
results already collected from the other partition, and ensure every machine in
the failed partition is represented in the response while successful partition
results remain intact.
🪄 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: ee48b4ca-f3aa-4a1c-867c-de21df6bb971
📒 Files selected for processing (1)
crates/api-core/src/handlers/component_manager.rs
kunzhao-nv
left a comment
kunzhao-nv
left a comment
There was a problem hiding this comment.
Better add unit tests for cases:
- rack scale/ standalone/ mix
- compute_tray_use_state_controller on/off
- machine failed in update_power_option should not be dispatched
- unknown machine id handling
| // driven synchronously through NICo-core's Redfish stack, because | ||
| // RMS cannot power-control them. | ||
| let (rack_scale_ids, standalone_ids) = | ||
| partition_compute_machines_by_rack_scale(api, &list.machine_ids).await?; |
There was a problem hiding this comment.
Before, unknown id will go through unresolved branch in resolve_compute_tray_endpoints and generate one error_result, while the rest machines will continue, which could lead to a partial success; after, with partition_compute_machines_by_rack_scale, a single bad id will fail entire batch. Please confirm this patten change is as expected.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/api-core/src/handlers/component_manager.rs (1)
1687-1747: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffSequential per-machine persistence loop.
Each machine in the request is processed strictly sequentially: health-override insert (DB write),
update_power_option(its own transaction), then health-override removal (another DB write). For largermachine_idsbatches this compounds into serialized round trips before any dispatch begins, even though the subsequent dispatch to rack-scale/standalone backends is already batched. Consider bounding concurrency for this pre-dispatch loop (e.g.futures::stream::iter(...).for_each_concurrent(N, ...)) if large batches are a realistic use case for this endpoint.🤖 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-core/src/handlers/component_manager.rs` around lines 1687 - 1747, Bound concurrency for the per-machine persistence work in the loop handling list.machine_ids, using the project’s established concurrency pattern and an appropriate limit. Preserve per-machine error results, health-override cleanup, power-option idempotency handling, and classification into rack_scale_ids or standalone_ids before dispatch.
🤖 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/api-core/src/handlers/component_manager.rs`:
- Around line 1687-1747: Bound concurrency for the per-machine persistence work
in the loop handling list.machine_ids, using the project’s established
concurrency pattern and an appropriate limit. Preserve per-machine error
results, health-override cleanup, power-option idempotency handling, and
classification into rack_scale_ids or standalone_ids before dispatch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2828346a-3c45-49eb-80cc-5963b173cedb
📒 Files selected for processing (1)
crates/api-core/src/handlers/component_manager.rs
94e203e to
7a04b49
Compare
I added test cases that should cover the changes introduced in this MR |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3873.docs.buildwithfern.com/infra-controller |
| machine_id: Some(machine_id), | ||
| power_state: desired_state, | ||
| }; | ||
| let power_option_ok = match crate::handlers::power_options::update_power_option( |
There was a problem hiding this comment.
Setting desired_power_state = Off before queuing the state-controller operation can prevent that operation from ever running when the power manager is enabled.
For a Ready host, MachineStateHandler runs handle_power() before attempt_state_transition(). The desired-Off path always returns continue_state_machine = false, so attempt_state_transition() never reaches maintenance_transition_if_requested(). The host remains physically on while the queued PowerOff maintenance request is never consumed.
The synchronous path is safe because it dispatches the physical power action immediately after updating the desired state, but the state-controller path is asynchronous. I think we should change the ordering/precedence so a queued maintenance request can run, and add a test covering:
compute_tray_use_state_controller = truepower_manager_options.enabled = true- PowerOff / ForceOff
One possible fix is to make pending machine maintenance take precedence over power-manager handling for Ready hosts; alternatively, avoid persisting desired Off until the queued maintenance operation can actually execute.
| let is_rack_scale = match machine_is_rack_scale(&machines_by_id, machine_id) { | ||
| Ok(v) => v, | ||
| Err(status) => { | ||
| results.push(error_result( |
There was a problem hiding this comment.
machine_is_rack_scale() deliberately returns Status::not_found, but this converts it to ComponentManagerStatusCode::InternalError.
Should we use the existing status_result(&machine_id.to_string(), status) helper so the per-machine response preserves NotFound. The new helper test currently verifies the intermediate Status, but not the status returned by ComponentPowerControlResponse.
7a04b49 to
48b80ec
Compare
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/machine-controller/src/handler.rs (1)
9935-9972: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUse
dpu_machine_idconsistently in the DPU restart logs.handler_restart_dpualready usesdpu_machine_id; themachine_idfield inrestart_dpumakes correlation harder.🤖 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/machine-controller/src/handler.rs` around lines 9935 - 9972, Update the logging inside restart_dpu to use the dpu_machine_id field name consistently with handler_restart_dpu, while preserving the existing machine identifier value and log behavior.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.
Outside diff comments:
In `@crates/machine-controller/src/handler.rs`:
- Around line 9935-9972: Update the logging inside restart_dpu to use the
dpu_machine_id field name consistently with handler_restart_dpu, while
preserving the existing machine identifier value and log behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 64498049-2af2-4221-bb24-77ad387f338d
📒 Files selected for processing (4)
crates/api-core/src/handlers/component_manager.rscrates/machine-controller/src/handler.rscrates/machine-controller/tests/integration/maintenance.rscrates/machine-controller/tests/integration/power_management.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/api-core/src/handlers/component_manager.rs
3 participants
Compute-tray power control called cm.compute_tray.power_control for all machines regardless of type. On an RMS-backed deployment this fails for standalone (non-rack-scale) servers, since RMS cannot resolve them.
Partition requested machines the same way compute firmware updates do (is_mnnvl_capable):
Related issues
Type of Change
Breaking Changes
Testing
Additional Notes