feat: route compute-tray power control through machine state controller#3683
Open
vinodchitraliNVIDIA wants to merge 1 commit into
Open
feat: route compute-tray power control through machine state controller#3683vinodchitraliNVIDIA wants to merge 1 commit into
vinodchitraliNVIDIA wants to merge 1 commit into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (16)
🚧 Files skipped from review as they are similar to previous changes (15)
Summary by CodeRabbit
WalkthroughMachine maintenance requests are persisted on machines, exposed through the component manager, and executed by the machine state controller as compute-tray power operations. The workflow adds maintenance state modeling, credential lookup, success/failure transitions, metrics, and service wiring. ChangesMachine maintenance workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ComponentManager
participant StateController
participant CredentialManager
participant ComputeTray
participant Database
Client->>ComponentManager: request machine power action
ComponentManager->>Database: persist maintenance request
StateController->>Database: read pending request
StateController->>CredentialManager: fetch BMC credentials
StateController->>ComputeTray: execute power operation
ComputeTray-->>StateController: operation result
StateController->>Database: clear request and persist Ready or Failed
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
aswaroop-nv
approved these changes
Jul 17, 2026
![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: 3
🧹 Nitpick comments (2)
crates/machine-controller/src/handler/maintenance.rs (1)
64-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the maintenance operation typed through dispatch.
Passing
"PowerOn","PowerOff", and"Reset"separately fromPowerActionpermits labels and actions to diverge. PassMachineMaintenanceOperationinto the common driver and derive both representations from that enum.As per coding guidelines, “Avoid stringly typed values in Rust. For finite sets of possibilities, use enums or structs of enums and derive their string representation with
Display/FromStr.”🤖 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/maintenance.rs` around lines 64 - 118, Update handle_power_on, handle_power_off, handle_reset, and invoke_power_operation to accept a MachineMaintenanceOperation instead of separate PowerAction and operation_label arguments. Derive the corresponding PowerAction and display/log representation from that operation enum within the common driver, removing duplicated string labels and preventing action-label mismatches.Source: Coding guidelines
crates/api-core/src/handlers/component_manager.rs (1)
3363-3385: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test coverage for
GracefulRestartandAcPowercyclevariants.The test covers 4 of 6
PowerActionvariants but omitsGracefulRestartandAcPowercycle, both of which map toMachineMaintenanceOperation::Reset. As per coding guidelines, prefer table-driven tests for input-variant coverage on conversion functions. Consider usingcarbide-test-supportvalue_scenarios!orcheck_valuesto cover all variants concisely.♻️ Proposed test using check_values for all variants
#[test] fn map_machine_maintenance_operation_variants() { use model::machine::MachineMaintenanceOperation; use super::map_machine_maintenance_operation; - assert_eq!( - map_machine_maintenance_operation(PowerAction::On), - MachineMaintenanceOperation::PowerOn, - ); - assert_eq!( - map_machine_maintenance_operation(PowerAction::ForceOff), - MachineMaintenanceOperation::PowerOff, - ); - assert_eq!( - map_machine_maintenance_operation(PowerAction::GracefulShutdown), - MachineMaintenanceOperation::PowerOff, - ); - assert_eq!( - map_machine_maintenance_operation(PowerAction::ForceRestart), - MachineMaintenanceOperation::Reset, - ); + carbide_test_support::value_scenarios!(|action| map_machine_maintenance_operation(action); + "PowerAction maps to MachineMaintenanceOperation" { + PowerAction::On => MachineMaintenanceOperation::PowerOn, + PowerAction::ForceOff => MachineMaintenanceOperation::PowerOff, + PowerAction::GracefulShutdown => MachineMaintenanceOperation::PowerOff, + PowerAction::GracefulRestart => MachineMaintenanceOperation::Reset, + PowerAction::ForceRestart => MachineMaintenanceOperation::Reset, + PowerAction::AcPowercycle => MachineMaintenanceOperation::Reset, + } + ); }🤖 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 3363 - 3385, Update the map_machine_maintenance_operation test to cover the missing GracefulRestart and AcPowercycle PowerAction variants, asserting both map to MachineMaintenanceOperation::Reset. Convert the individual assertions to a table-driven value-scenario or check_values test that concisely covers all six variants.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/machine-controller/src/handler.rs`:
- Around line 1306-1309: Update the maintenance transition handling around
maintenance_transition_if_requested so accepting maintenance from Failed
transactionally clears the host’s stale failure details before returning the
maintenance outcome. Match the existing recovery branches’ failure-clearing
behavior, while preserving the current transition and return flow.
In `@crates/machine-controller/src/handler/maintenance.rs`:
- Around line 113-217: Add table-driven tests for maintenance reconciliation
using scenarios! with Outcome or explicit check_cases for Result-returning
variants. Cover every power operation across backend success, empty results,
non-success results, transport failures, missing component manager, missing
credentials, and entry states Ready and Failed; assert both request clearing and
the resulting state for each case, reusing existing maintenance test helpers and
symbols.
- Around line 153-178: Update the power-control flow around the backend call and
maintenance transition to persist a durable dispatch phase and idempotency key
before invoking the non-idempotent operation. Reconcile Pending, Dispatched, and
Completed outcomes after crashes or transaction conflicts, retrying only when
safe and never blindly replaying an ambiguous reset; ensure successful
reconciliation clears maintenance and transitions to Ready.
---
Nitpick comments:
In `@crates/api-core/src/handlers/component_manager.rs`:
- Around line 3363-3385: Update the map_machine_maintenance_operation test to
cover the missing GracefulRestart and AcPowercycle PowerAction variants,
asserting both map to MachineMaintenanceOperation::Reset. Convert the individual
assertions to a table-driven value-scenario or check_values test that concisely
covers all six variants.
In `@crates/machine-controller/src/handler/maintenance.rs`:
- Around line 64-118: Update handle_power_on, handle_power_off, handle_reset,
and invoke_power_operation to accept a MachineMaintenanceOperation instead of
separate PowerAction and operation_label arguments. Derive the corresponding
PowerAction and display/log representation from that operation enum within the
common driver, removing duplicated string labels and preventing action-label
mismatches.
🪄 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: 50426d8d-d99d-4f5a-afc2-a5acccc992f1
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
crates/api-core/src/handlers/component_manager.rscrates/api-core/src/setup.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-db/migrations/20260717120000_machine_maintenance_requested.sqlcrates/api-db/src/machine.rscrates/api-model/src/machine/json.rscrates/api-model/src/machine/mod.rscrates/api-model/src/machine/slas.rscrates/api-model/src/test_support/machine_snapshot.rscrates/component-manager/src/component_manager.rscrates/machine-controller/Cargo.tomlcrates/machine-controller/src/context.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/handler/maintenance.rscrates/machine-controller/src/io.rscrates/machine-controller/tests/integration/env.rs
Comment on lines
+1306
to
+1309
| if let Some(outcome) = maintenance::maintenance_transition_if_requested(mh_snapshot) | ||
| { | ||
| return Ok(outcome); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Clear stale failure details when accepting maintenance from Failed.
This transition preserves the existing failure details. On the next iteration, the generic failure check at Lines 739-765 runs before Maintenance dispatch and can immediately move the host back to Failed, so the requested operation never executes. Clear the failure details transactionally when entering maintenance, as the existing recovery branches do.
As per path instructions, review controller logic for “reconciliation correctness” and “state-machine transitions.”
🤖 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 1306 - 1309, Update
the maintenance transition handling around maintenance_transition_if_requested
so accepting maintenance from Failed transactionally clears the host’s stale
failure details before returning the maintenance outcome. Match the existing
recovery branches’ failure-clearing behavior, while preserving the current
transition and return flow.
Source: Path instructions
Comment on lines
+113
to
+217
| async fn invoke_power_operation( | ||
| host_machine_id: &MachineId, | ||
| mh_snapshot: &ManagedHostStateSnapshot, | ||
| ctx: &mut StateHandlerContext<'_, MachineStateHandlerContextObjects>, | ||
| action: PowerAction, | ||
| operation_label: &'static str, | ||
| ) -> Result<StateHandlerOutcome<ManagedHostState>, StateHandlerError> { | ||
| let machine = &mh_snapshot.host_snapshot; | ||
|
|
||
| let Some(component_manager) = ctx.services.component_manager.as_ref() else { | ||
| return finish_maintenance_with_error( | ||
| host_machine_id, | ||
| ctx, | ||
| format!( | ||
| "Machine {host_machine_id} maintenance ({operation_label}): component manager not configured" | ||
| ), | ||
| ) | ||
| .await; | ||
| }; | ||
|
|
||
| let endpoint = match build_compute_tray_endpoint( | ||
| host_machine_id, | ||
| machine, | ||
| ctx.services.credential_manager.as_ref(), | ||
| ) | ||
| .await | ||
| { | ||
| Ok(endpoint) => endpoint, | ||
| Err(cause) => { | ||
| return finish_maintenance_with_error( | ||
| host_machine_id, | ||
| ctx, | ||
| format!( | ||
| "Machine {host_machine_id} maintenance ({operation_label}): {cause}" | ||
| ), | ||
| ) | ||
| .await; | ||
| } | ||
| }; | ||
|
|
||
| match component_manager | ||
| .compute_tray | ||
| .power_control(std::slice::from_ref(&endpoint), action) | ||
| .await | ||
| { | ||
| Ok(results) => { | ||
| let result = results | ||
| .into_iter() | ||
| .next() | ||
| .unwrap_or(ComputeTrayResult { | ||
| bmc_ip: endpoint.bmc_ip, | ||
| success: false, | ||
| error: Some("component manager returned no result".into()), | ||
| }); | ||
|
|
||
| if result.success { | ||
| tracing::info!( | ||
| machine_id = %host_machine_id, | ||
| operation = operation_label, | ||
| backend = component_manager.compute_tray.name(), | ||
| "Machine power control succeeded; returning host to Ready" | ||
| ); | ||
| let mut txn = ctx.services.db_pool.begin().await?; | ||
| db_machine::clear_machine_maintenance_requested(&mut txn, *host_machine_id) | ||
| .await?; | ||
| return Ok(StateHandlerOutcome::transition(ManagedHostState::Ready).with_txn(txn)); | ||
| } | ||
|
|
||
| let summary = result | ||
| .error | ||
| .unwrap_or_else(|| "power control failed".into()); | ||
| tracing::warn!( | ||
| machine_id = %host_machine_id, | ||
| operation = operation_label, | ||
| backend = component_manager.compute_tray.name(), | ||
| summary = %summary, | ||
| "Machine power control returned a non-success result", | ||
| ); | ||
| finish_maintenance_with_error( | ||
| host_machine_id, | ||
| ctx, | ||
| format!( | ||
| "Machine {host_machine_id} maintenance ({operation_label}): power control failed: {summary}" | ||
| ), | ||
| ) | ||
| .await | ||
| } | ||
| Err(error) => { | ||
| tracing::warn!( | ||
| machine_id = %host_machine_id, | ||
| operation = operation_label, | ||
| backend = component_manager.compute_tray.name(), | ||
| error = %error, | ||
| "Machine power control transport error", | ||
| ); | ||
| finish_maintenance_with_error( | ||
| host_machine_id, | ||
| ctx, | ||
| format!( | ||
| "Machine {host_machine_id} maintenance ({operation_label}): power control failed: {error}" | ||
| ), | ||
| ) | ||
| .await | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Add table-driven coverage for maintenance reconciliation.
Cover each operation plus backend success, empty result, non-success result, transport failure, missing component manager, missing credentials, and entry from both Ready and Failed. Assert request clearing and final state in every case.
As per coding guidelines, use scenarios! with Outcome or explicit check_cases for Result-returning input variants.
🤖 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/maintenance.rs` around lines 113 - 217,
Add table-driven tests for maintenance reconciliation using scenarios! with
Outcome or explicit check_cases for Result-returning variants. Cover every power
operation across backend success, empty results, non-success results, transport
failures, missing component manager, missing credentials, and entry states Ready
and Failed; assert both request clearing and the resulting state for each case,
reusing existing maintenance test helpers and symbols.
Source: Coding guidelines
Comment on lines
+153
to
+178
| match component_manager | ||
| .compute_tray | ||
| .power_control(std::slice::from_ref(&endpoint), action) | ||
| .await | ||
| { | ||
| Ok(results) => { | ||
| let result = results | ||
| .into_iter() | ||
| .next() | ||
| .unwrap_or(ComputeTrayResult { | ||
| bmc_ip: endpoint.bmc_ip, | ||
| success: false, | ||
| error: Some("component manager returned no result".into()), | ||
| }); | ||
|
|
||
| if result.success { | ||
| tracing::info!( | ||
| machine_id = %host_machine_id, | ||
| operation = operation_label, | ||
| backend = component_manager.compute_tray.name(), | ||
| "Machine power control succeeded; returning host to Ready" | ||
| ); | ||
| let mut txn = ctx.services.db_pool.begin().await?; | ||
| db_machine::clear_machine_maintenance_requested(&mut txn, *host_machine_id) | ||
| .await?; | ||
| return Ok(StateHandlerOutcome::transition(ManagedHostState::Ready).with_txn(txn)); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Make power dispatch crash-safe before issuing non-idempotent resets.
The external operation completes before its request clear and state transition are committed. A crash, transaction error, or optimistic conflict in between leaves the host in Maintenance; the next iteration repeats the action, potentially causing repeated resets.
Persist a durable dispatch phase/idempotency key before calling the backend and reconcile Pending/Dispatched/Completed outcomes without blindly replaying ambiguous operations.
As per path instructions, controller logic must provide “idempotency” and “safe recovery from partial failures.”
🤖 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/maintenance.rs` around lines 153 - 178,
Update the power-control flow around the backend call and maintenance transition
to persist a durable dispatch phase and idempotency key before invoking the
non-idempotent operation. Reconcile Pending, Dispatched, and Completed outcomes
after crashes or transaction conflicts, retrying only when safe and never
blindly replaying an ambiguous reset; ensure successful reconciliation clears
maintenance and transitions to Ready.
Source: Path instructions
vinodchitraliNVIDIA
force-pushed
the
vc/ps
branch
from
July 17, 2026 18:52
bd7731d to
ba3a1d1
Compare
Add machine maintenance power operations mirroring the existing switch and power-shelf state-controller maintenance pattern. When compute_tray_use_state_controller is enabled, ComponentPowerControl for machines queues a maintenance request instead of issuing Redfish directly. Model and persistence: - Introduce MachineMaintenanceOperation (PowerOn, PowerOff, Reset) and MachineMaintenanceRequest. - Add ManagedHostState::Maintenance to execute queued operations. - Add machines.machine_maintenance_requested JSONB column and DB helpers to set/clear the pending request. Machine state handler: - New maintenance handler dispatches power actions via compute_tray backend. - Ready and Failed states transition into Maintenance when a request is pending; success returns to Ready, failure clears the request and moves to Failed to avoid retry loops. - Wire component_manager and credential_manager into machine controller services for endpoint/credential resolution. Component manager and API: - Implement request_machine_maintenance_via_state_controller. - Replace the unimplemented compute-tray state-controller path in component_power_control with the maintenance queue. - Map PowerAction to MachineMaintenanceOperation consistently with switches. Signed-off-by: Vinod Chitrali <vchitrali@nvidia.com>
vinodchitraliNVIDIA
force-pushed
the
vc/ps
branch
from
July 17, 2026 19:28
ba3a1d1 to
af682ea
Compare
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
2 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.
Add machine maintenance power operations mirroring the existing switch and power-shelf state-controller maintenance pattern. When compute_tray_use_state_controller is enabled, ComponentPowerControl for machines queues a maintenance request instead of issuing Redfish directly.
Model and persistence:
Machine state handler:
Component manager and API:
Related issues
Type of Change
Breaking Changes
Testing
Additional Notes