feat(power-control): route standalone vs rack-scale machines separately#3873
Open
spydaNVIDIA wants to merge 1 commit into
Open
feat(power-control): route standalone vs rack-scale machines separately#3873spydaNVIDIA wants to merge 1 commit into
spydaNVIDIA wants to merge 1 commit into
Conversation
Contributor
Summary by CodeRabbit
WalkthroughThe component manager now partitions machine power-control requests by system type, updates desired power state before dispatch, routes rack-scale systems through maintenance or configured backends, and routes standalone systems through NICo-core. A shared helper handles endpoint resolution, backend results, and exploration refresh IPs. ChangesCompute-tray power-control routing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ComponentManager
participant StateController
participant ComputeTrayManager
participant SiteExploration
ComponentManager->>ComponentManager: Partition rack-scale and standalone machines
ComponentManager->>StateController: Use maintenance flow when configured
ComponentManager->>ComputeTrayManager: Dispatch synchronous power control
ComputeTrayManager-->>ComponentManager: Return results and BMC IPs
ComponentManager->>SiteExploration: Refresh exploration for returned BMC IPs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
@coderabbitai full review please |
Contributor
|
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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
Comment on lines
+1660
to
1740
| let mut results: Vec<rpc::ComponentResult> = Vec::new(); | ||
| for &machine_id in &list.machine_ids { | ||
| let override_inserted = power_control_health_override(api, machine_id, true).await; | ||
|
|
||
| let power_req = rpc::PowerOptionUpdateRequest { | ||
| machine_id: Some(machine_id), | ||
| power_state: desired_state, | ||
| }; | ||
| match crate::handlers::power_options::update_power_option( | ||
| api, | ||
| cm, | ||
| &list.machine_ids, | ||
| action, | ||
| Request::new(power_req), | ||
| ) | ||
| .await?; | ||
| let ips = Vec::new(); | ||
| (results, ips) | ||
| } else { | ||
| let resolved = resolve_compute_tray_endpoints(api, &list.machine_ids).await?; | ||
|
|
||
| let mut results: Vec<_> = resolved | ||
| .unresolved | ||
| .iter() | ||
| .map(|u| error_result(&u.id.to_string(), u.reason.clone())) | ||
| .collect(); | ||
|
|
||
| let resolved_machine_ids: Vec<_> = resolved | ||
| .resolved | ||
| .endpoints | ||
| .iter() | ||
| .filter_map(|ep| resolved.resolved.ip_to_machine_id.get(&ep.bmc_ip).copied()) | ||
| .collect(); | ||
|
|
||
| // Insert health overrides and update power-manager desired state | ||
| // before issuing Redfish commands. | ||
| let desired_state = desired_power_state(action) as i32; | ||
| let mut overrides_inserted = Vec::new(); | ||
| for &machine_id in &resolved_machine_ids { | ||
| let inserted = power_control_health_override(api, machine_id, true).await; | ||
| if inserted { | ||
| overrides_inserted.push(machine_id); | ||
| } | ||
|
|
||
| let power_req = rpc::PowerOptionUpdateRequest { | ||
| machine_id: Some(machine_id), | ||
| power_state: desired_state, | ||
| }; | ||
| match crate::handlers::power_options::update_power_option( | ||
| api, | ||
| Request::new(power_req), | ||
| ) | ||
| .await | ||
| .await | ||
| { | ||
| Ok(_) => {} | ||
| Err(e) | ||
| if e.code() == Code::InvalidArgument | ||
| && e.message().contains("already set as") => | ||
| { | ||
| Ok(_) => {} | ||
| Err(e) | ||
| if e.code() == Code::InvalidArgument | ||
| && e.message().contains("already set as") => | ||
| { | ||
| tracing::debug!( | ||
| %machine_id, | ||
| desired_state, | ||
| "power option already in desired state, skipping" | ||
| ); | ||
| } | ||
| Err(e) => { | ||
| results.push(error_result( | ||
| &machine_id.to_string(), | ||
| format!("failed to update power option: {e}"), | ||
| )); | ||
| } | ||
| tracing::debug!( | ||
| %machine_id, | ||
| desired_state, | ||
| "power option already in desired state, skipping" | ||
| ); | ||
| } | ||
| Err(e) => { | ||
| results.push(error_result( | ||
| &machine_id.to_string(), | ||
| format!("failed to update power option: {e}"), | ||
| )); | ||
| } | ||
| } | ||
|
|
||
| tracing::info!( | ||
| backend = cm.compute_tray.name(), | ||
| compute_tray_count = resolved.resolved.endpoints.len(), | ||
| ?action, | ||
| "power control for compute trays" | ||
| ); | ||
| let backend_results = cm | ||
| .compute_tray | ||
| .power_control(&resolved.resolved.endpoints, action) | ||
| .await | ||
| .map_err(component_manager_error_to_status)?; | ||
|
|
||
| // Clear health overrides after Redfish dispatch. | ||
| for machine_id in &overrides_inserted { | ||
| power_control_health_override(api, *machine_id, false).await; | ||
| if override_inserted { | ||
| power_control_health_override(api, machine_id, false).await; | ||
| } | ||
| } | ||
|
|
||
| let ips: Vec<IpAddr> = resolved | ||
| .resolved | ||
| .endpoints | ||
| .iter() | ||
| .map(|ep| ep.bmc_ip) | ||
| .collect(); | ||
| let mut ips: Vec<IpAddr> = Vec::new(); | ||
|
|
||
| results.extend(backend_results.into_iter().map(|r| { | ||
| let id = resolved | ||
| .resolved | ||
| .ip_to_machine_id | ||
| .get(&r.bmc_ip) | ||
| .map(|id| id.to_string()) | ||
| .unwrap_or_else(|| r.bmc_ip.to_string()); | ||
| if r.success { | ||
| success_result(&id) | ||
| } else { | ||
| error_result(&id, r.error.unwrap_or_default()) | ||
| } | ||
| })); | ||
| // Rack-scale systems: the state-controller maintenance flow when | ||
| // enabled, otherwise a synchronous dispatch through the configured | ||
| // backend (RMS). | ||
| if !rack_scale_ids.is_empty() { | ||
| if cm.compute_tray_use_state_controller && !bypass_state_controller { | ||
| let sc_results = queue_machine_power_control_via_state_controller( | ||
| api, | ||
| cm, | ||
| &rack_scale_ids, | ||
| action, | ||
| ) | ||
| .await?; | ||
| results.extend(sc_results); | ||
| } else { | ||
| let (rack_results, rack_ips) = dispatch_compute_tray_power_control( | ||
| api, | ||
| cm.compute_tray.as_ref(), | ||
| &rack_scale_ids, | ||
| action, | ||
| ) | ||
| .await?; | ||
| results.extend(rack_results); | ||
| ips.extend(rack_ips); | ||
| } | ||
| } | ||
|
|
||
| (results, ips) | ||
| // Standalone servers: always synchronous, always NICo-core's Redfish | ||
| // stack (never the state machine, which has no non-rack path), so | ||
| // power control works even when the configured backend is RMS. | ||
| if !standalone_ids.is_empty() { | ||
| let core_backend = CoreComputeTrayManager::new(api.redfish_pool.clone()); | ||
| let (standalone_results, standalone_ips) = dispatch_compute_tray_power_control( | ||
| api, | ||
| &core_backend, | ||
| &standalone_ids, | ||
| action, | ||
| ) | ||
| .await?; | ||
| results.extend(standalone_results); | ||
| ips.extend(standalone_ips); | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Machines that fail update_power_option are still dispatched, producing duplicate results and inconsistent state.
When the final Err(e) arm (Line 1685) records an error_result for a machine, that machine's id remains in rack_scale_ids/standalone_ids. Consequently the downstream dispatch at Lines 1703 and 1729 will:
- Emit a second
ComponentResultfor the same machine id, violating the one-result-per-target contract of the response. - Physically actuate power control even though the power-manager desired state was never persisted, leaving the recorded intent and the hardware out of sync.
Recommend collecting the failed ids and excluding them from both dispatch partitions before proceeding.
🛠️ Proposed fix — record failures and filter before dispatch
let desired_state = desired_power_state(action) as i32;
+ let mut failed_ids: HashSet<MachineId> = HashSet::new();
let mut results: Vec<rpc::ComponentResult> = Vec::new();
for &machine_id in &list.machine_ids { Err(e) => {
+ failed_ids.insert(machine_id);
results.push(error_result(
&machine_id.to_string(),
format!("failed to update power option: {e}"),
));
}Then, before the dispatch blocks, drop the failed machines from each partition (adjust HashSet import as needed):
let rack_scale_ids: Vec<_> = rack_scale_ids
.into_iter()
.filter(|id| !failed_ids.contains(id))
.collect();
let standalone_ids: Vec<_> = standalone_ids
.into_iter()
.filter(|id| !failed_ids.contains(id))
.collect();📝 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.
Suggested change
| let mut results: Vec<rpc::ComponentResult> = Vec::new(); | |
| for &machine_id in &list.machine_ids { | |
| let override_inserted = power_control_health_override(api, machine_id, true).await; | |
| let power_req = rpc::PowerOptionUpdateRequest { | |
| machine_id: Some(machine_id), | |
| power_state: desired_state, | |
| }; | |
| match crate::handlers::power_options::update_power_option( | |
| api, | |
| cm, | |
| &list.machine_ids, | |
| action, | |
| Request::new(power_req), | |
| ) | |
| .await?; | |
| let ips = Vec::new(); | |
| (results, ips) | |
| } else { | |
| let resolved = resolve_compute_tray_endpoints(api, &list.machine_ids).await?; | |
| let mut results: Vec<_> = resolved | |
| .unresolved | |
| .iter() | |
| .map(|u| error_result(&u.id.to_string(), u.reason.clone())) | |
| .collect(); | |
| let resolved_machine_ids: Vec<_> = resolved | |
| .resolved | |
| .endpoints | |
| .iter() | |
| .filter_map(|ep| resolved.resolved.ip_to_machine_id.get(&ep.bmc_ip).copied()) | |
| .collect(); | |
| // Insert health overrides and update power-manager desired state | |
| // before issuing Redfish commands. | |
| let desired_state = desired_power_state(action) as i32; | |
| let mut overrides_inserted = Vec::new(); | |
| for &machine_id in &resolved_machine_ids { | |
| let inserted = power_control_health_override(api, machine_id, true).await; | |
| if inserted { | |
| overrides_inserted.push(machine_id); | |
| } | |
| let power_req = rpc::PowerOptionUpdateRequest { | |
| machine_id: Some(machine_id), | |
| power_state: desired_state, | |
| }; | |
| match crate::handlers::power_options::update_power_option( | |
| api, | |
| Request::new(power_req), | |
| ) | |
| .await | |
| .await | |
| { | |
| Ok(_) => {} | |
| Err(e) | |
| if e.code() == Code::InvalidArgument | |
| && e.message().contains("already set as") => | |
| { | |
| Ok(_) => {} | |
| Err(e) | |
| if e.code() == Code::InvalidArgument | |
| && e.message().contains("already set as") => | |
| { | |
| tracing::debug!( | |
| %machine_id, | |
| desired_state, | |
| "power option already in desired state, skipping" | |
| ); | |
| } | |
| Err(e) => { | |
| results.push(error_result( | |
| &machine_id.to_string(), | |
| format!("failed to update power option: {e}"), | |
| )); | |
| } | |
| tracing::debug!( | |
| %machine_id, | |
| desired_state, | |
| "power option already in desired state, skipping" | |
| ); | |
| } | |
| Err(e) => { | |
| results.push(error_result( | |
| &machine_id.to_string(), | |
| format!("failed to update power option: {e}"), | |
| )); | |
| } | |
| } | |
| tracing::info!( | |
| backend = cm.compute_tray.name(), | |
| compute_tray_count = resolved.resolved.endpoints.len(), | |
| ?action, | |
| "power control for compute trays" | |
| ); | |
| let backend_results = cm | |
| .compute_tray | |
| .power_control(&resolved.resolved.endpoints, action) | |
| .await | |
| .map_err(component_manager_error_to_status)?; | |
| // Clear health overrides after Redfish dispatch. | |
| for machine_id in &overrides_inserted { | |
| power_control_health_override(api, *machine_id, false).await; | |
| if override_inserted { | |
| power_control_health_override(api, machine_id, false).await; | |
| } | |
| } | |
| let ips: Vec<IpAddr> = resolved | |
| .resolved | |
| .endpoints | |
| .iter() | |
| .map(|ep| ep.bmc_ip) | |
| .collect(); | |
| let mut ips: Vec<IpAddr> = Vec::new(); | |
| results.extend(backend_results.into_iter().map(|r| { | |
| let id = resolved | |
| .resolved | |
| .ip_to_machine_id | |
| .get(&r.bmc_ip) | |
| .map(|id| id.to_string()) | |
| .unwrap_or_else(|| r.bmc_ip.to_string()); | |
| if r.success { | |
| success_result(&id) | |
| } else { | |
| error_result(&id, r.error.unwrap_or_default()) | |
| } | |
| })); | |
| // Rack-scale systems: the state-controller maintenance flow when | |
| // enabled, otherwise a synchronous dispatch through the configured | |
| // backend (RMS). | |
| if !rack_scale_ids.is_empty() { | |
| if cm.compute_tray_use_state_controller && !bypass_state_controller { | |
| let sc_results = queue_machine_power_control_via_state_controller( | |
| api, | |
| cm, | |
| &rack_scale_ids, | |
| action, | |
| ) | |
| .await?; | |
| results.extend(sc_results); | |
| } else { | |
| let (rack_results, rack_ips) = dispatch_compute_tray_power_control( | |
| api, | |
| cm.compute_tray.as_ref(), | |
| &rack_scale_ids, | |
| action, | |
| ) | |
| .await?; | |
| results.extend(rack_results); | |
| ips.extend(rack_ips); | |
| } | |
| } | |
| (results, ips) | |
| // Standalone servers: always synchronous, always NICo-core's Redfish | |
| // stack (never the state machine, which has no non-rack path), so | |
| // power control works even when the configured backend is RMS. | |
| if !standalone_ids.is_empty() { | |
| let core_backend = CoreComputeTrayManager::new(api.redfish_pool.clone()); | |
| let (standalone_results, standalone_ips) = dispatch_compute_tray_power_control( | |
| api, | |
| &core_backend, | |
| &standalone_ids, | |
| action, | |
| ) | |
| .await?; | |
| results.extend(standalone_results); | |
| ips.extend(standalone_ips); | |
| } | |
| let desired_state = desired_power_state(action) as i32; | |
| let mut failed_ids: HashSet<MachineId> = HashSet::new(); | |
| let mut results: Vec<rpc::ComponentResult> = Vec::new(); | |
| for &machine_id in &list.machine_ids { | |
| let override_inserted = power_control_health_override(api, machine_id, true).await; | |
| let power_req = rpc::PowerOptionUpdateRequest { | |
| machine_id: Some(machine_id), | |
| power_state: desired_state, | |
| }; | |
| match crate::handlers::power_options::update_power_option( | |
| api, | |
| Request::new(power_req), | |
| ) | |
| .await | |
| { | |
| Ok(_) => {} | |
| Err(e) | |
| if e.code() == Code::InvalidArgument | |
| && e.message().contains("already set as") => | |
| { | |
| tracing::debug!( | |
| %machine_id, | |
| desired_state, | |
| "power option already in desired state, skipping" | |
| ); | |
| } | |
| Err(e) => { | |
| failed_ids.insert(machine_id); | |
| results.push(error_result( | |
| &machine_id.to_string(), | |
| format!("failed to update power option: {e}"), | |
| )); | |
| } | |
| } | |
| if override_inserted { | |
| power_control_health_override(api, machine_id, false).await; | |
| } | |
| } | |
| let mut ips: Vec<IpAddr> = Vec::new(); | |
| let rack_scale_ids: Vec<_> = rack_scale_ids | |
| .into_iter() | |
| .filter(|id| !failed_ids.contains(id)) | |
| .collect(); | |
| let standalone_ids: Vec<_> = standalone_ids | |
| .into_iter() | |
| .filter(|id| !failed_ids.contains(id)) | |
| .collect(); | |
| // Rack-scale systems: the state-controller maintenance flow when | |
| // enabled, otherwise a synchronous dispatch through the configured | |
| // backend (RMS). | |
| if !rack_scale_ids.is_empty() { | |
| if cm.compute_tray_use_state_controller && !bypass_state_controller { | |
| let sc_results = queue_machine_power_control_via_state_controller( | |
| api, | |
| cm, | |
| &rack_scale_ids, | |
| action, | |
| ) | |
| .await?; | |
| results.extend(sc_results); | |
| } else { | |
| let (rack_results, rack_ips) = dispatch_compute_tray_power_control( | |
| api, | |
| cm.compute_tray.as_ref(), | |
| &rack_scale_ids, | |
| action, | |
| ) | |
| .await?; | |
| results.extend(rack_results); | |
| ips.extend(rack_ips); | |
| } | |
| } | |
| // Standalone servers: always synchronous, always NICo-core's Redfish | |
| // stack (never the state machine, which has no non-rack path), so | |
| // power control works even when the configured backend is RMS. | |
| if !standalone_ids.is_empty() { | |
| let core_backend = CoreComputeTrayManager::new(api.redfish_pool.clone()); | |
| let (standalone_results, standalone_ips) = dispatch_compute_tray_power_control( | |
| api, | |
| &core_backend, | |
| &standalone_ids, | |
| action, | |
| ) | |
| .await?; | |
| results.extend(standalone_results); | |
| ips.extend(standalone_ips); | |
| } |
🤖 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 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.
Contributor
There was a problem hiding this comment.
@kunzhao-nv, acknowledged. The failed desired-state updates should be excluded from both dispatch partitions so each target receives one result and no hardware action occurs without persisted intent. 🐇
Contributor
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
Comment on lines
+1703
to
1740
| if !rack_scale_ids.is_empty() { | ||
| if cm.compute_tray_use_state_controller && !bypass_state_controller { | ||
| let sc_results = queue_machine_power_control_via_state_controller( | ||
| api, | ||
| cm, | ||
| &rack_scale_ids, | ||
| action, | ||
| ) | ||
| .await?; | ||
| results.extend(sc_results); | ||
| } else { | ||
| let (rack_results, rack_ips) = dispatch_compute_tray_power_control( | ||
| api, | ||
| cm.compute_tray.as_ref(), | ||
| &rack_scale_ids, | ||
| action, | ||
| ) | ||
| .await?; | ||
| results.extend(rack_results); | ||
| ips.extend(rack_ips); | ||
| } | ||
| } | ||
|
|
||
| (results, ips) | ||
| // Standalone servers: always synchronous, always NICo-core's Redfish | ||
| // stack (never the state machine, which has no non-rack path), so | ||
| // power control works even when the configured backend is RMS. | ||
| if !standalone_ids.is_empty() { | ||
| let core_backend = CoreComputeTrayManager::new(api.redfish_pool.clone()); | ||
| let (standalone_results, standalone_ips) = dispatch_compute_tray_power_control( | ||
| api, | ||
| &core_backend, | ||
| &standalone_ids, | ||
| action, | ||
| ) | ||
| .await?; | ||
| results.extend(standalone_results); | ||
| ips.extend(standalone_ips); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect power_control implementations to see when a batch-level Err is returned.
fd -e rs . crates/component-manager | xargs rg -nP -C4 '\bfn\s+power_control\b'Repository: NVIDIA/infra-controller
Length of output: 8153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant types and methods before reading targeted slices.
ast-grep outline crates/component-manager/src/compute_tray_manager.rs --view expanded
printf '\n---\n'
ast-grep outline crates/api-core/src/handlers/component_manager.rs --view expanded | sed -n '1,240p'
printf '\n---\n'
ast-grep outline crates/component-manager/src/core_compute_manager.rs --view expandedRepository: NVIDIA/infra-controller
Length of output: 11578
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1532,1812p' crates/api-core/src/handlers/component_manager.rs
printf '\n--- core_compute_manager ---\n'
sed -n '57,120p' crates/component-manager/src/core_compute_manager.rs
printf '\n--- component trait ---\n'
sed -n '80,110p' crates/component-manager/src/compute_tray_manager.rsRepository: NVIDIA/infra-controller
Length of output: 14583
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1236,1320p' crates/api-core/src/handlers/component_manager.rs
printf '\n--- resolve_compute_tray_endpoints ---\n'
sed -n '1320,1438p' crates/api-core/src/handlers/component_manager.rs
printf '\n--- rms compute_tray power_control ---\n'
sed -n '2377,2438p' crates/component-manager/src/rms.rsRepository: NVIDIA/infra-controller
Length of output: 9589
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '2438,2498p' crates/component-manager/src/rms.rs
printf '\n--- search for Result-returning call sites in compute_tray power_control paths ---\n'
rg -n "\.power_control\(" crates/api-core/src/handlers/component_manager.rs crates/component-manager/src/{core_compute_manager.rs,rms.rs,compute_tray_manager.rs}Repository: NVIDIA/infra-controller
Length of output: 2901
Preserve partial power-control results across partition failures. The compute-tray dispatches return per-endpoint failures in-band; if either partition returns Err, the ? here still drops any results already collected from the other partition and turns a partial success into a top-level Status. Convert that partition failure into per-machine error_results so the response always reflects committed work.
🤖 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 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.
Contributor
There was a problem hiding this comment.
In L1711 / 1720 / 1737.
Contributor
There was a problem hiding this comment.
@kunzhao-nv, yes—those are the three relevant dispatch points:
- Line 1711: rack-scale state-controller maintenance request
- Line 1720: rack-scale synchronous configured-backend dispatch
- Line 1737: standalone synchronous NICo-core/Redfish dispatch
An error from any of these currently propagates via ?, discarding results already accumulated from desired-state updates or the other partition. Each failure should instead append an error_result for every machine in its corresponding partition and allow the response to retain all other per-machine outcomes.
kunzhao-nv
reviewed
Jul 23, 2026
kunzhao-nv
left a comment
kunzhao-nv
left a comment
Contributor
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?; |
Contributor
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.
Comment on lines
+1703
to
1740
| if !rack_scale_ids.is_empty() { | ||
| if cm.compute_tray_use_state_controller && !bypass_state_controller { | ||
| let sc_results = queue_machine_power_control_via_state_controller( | ||
| api, | ||
| cm, | ||
| &rack_scale_ids, | ||
| action, | ||
| ) | ||
| .await?; | ||
| results.extend(sc_results); | ||
| } else { | ||
| let (rack_results, rack_ips) = dispatch_compute_tray_power_control( | ||
| api, | ||
| cm.compute_tray.as_ref(), | ||
| &rack_scale_ids, | ||
| action, | ||
| ) | ||
| .await?; | ||
| results.extend(rack_results); | ||
| ips.extend(rack_ips); | ||
| } | ||
| } | ||
|
|
||
| (results, ips) | ||
| // Standalone servers: always synchronous, always NICo-core's Redfish | ||
| // stack (never the state machine, which has no non-rack path), so | ||
| // power control works even when the configured backend is RMS. | ||
| if !standalone_ids.is_empty() { | ||
| let core_backend = CoreComputeTrayManager::new(api.redfish_pool.clone()); | ||
| let (standalone_results, standalone_ips) = dispatch_compute_tray_power_control( | ||
| api, | ||
| &core_backend, | ||
| &standalone_ids, | ||
| action, | ||
| ) | ||
| .await?; | ||
| results.extend(standalone_results); | ||
| ips.extend(standalone_ips); | ||
| } |
Contributor
There was a problem hiding this comment.
In L1711 / 1720 / 1737.
zhaozhongn
approved these changes
Jul 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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