feat: route compute-tray power control through machine state controller#3683
Merged
Merged
Conversation
Contributor
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 | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
vinodchitraliNVIDIA
force-pushed
the
vc/ps
branch
3 times, most recently
from
July 20, 2026 14:04
af682ea to
380c454
Compare
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3683.docs.buildwithfern.com/infra-controller |
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 20, 2026 14:13
380c454 to
2ee7cbd
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/machine-controller/src/handler/maintenance.rs (1)
113-212: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftNon-idempotent power dispatch is not crash-safe.
The external
power_controlcall executes before the DB transaction that clearsmachine_maintenance_requestedand commits the resulting state transition (success path: Lines 151-172; failure path: Lines 185-211). A crash, transaction error, or optimistic-lock conflict between the backend call and the commit leaves the host inMaintenance; the next reconciliation iteration will callpower_controlagain, potentially issuing a duplicateReset/PowerOffagainst the BMC.Persist a durable dispatch marker (phase/idempotency key) before invoking the backend, and reconcile ambiguous outcomes on resume rather than blindly replaying the action.
As per path instructions, controller logic must provide "safe recovery from partial failures" and idempotency guarantees.
🤖 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 - 212, The invoke_power_operation flow is not crash-safe because it calls power_control before durably recording dispatch state. Update invoke_power_operation and its maintenance persistence flow to write and commit a dispatch phase/idempotency key before invoking the backend, then persist the resulting success or failure transition. On reconciliation or resume, detect an existing in-flight marker and reconcile the ambiguous backend outcome instead of replaying Reset or PowerOff blindly, including handling transaction or optimistic-lock failures safely.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.
Inline comments:
In `@crates/machine-controller/src/handler/maintenance.rs`:
- Around line 126-128: Lowercase the leading “Machine” in the four maintenance
failure messages within the maintenance handler, including the format! message
near the component-manager configuration error and the corresponding messages at
the other referenced sites. Keep the existing message content and interpolation
unchanged while ensuring each starts with “machine”.
---
Duplicate comments:
In `@crates/machine-controller/src/handler/maintenance.rs`:
- Around line 113-212: The invoke_power_operation flow is not crash-safe because
it calls power_control before durably recording dispatch state. Update
invoke_power_operation and its maintenance persistence flow to write and commit
a dispatch phase/idempotency key before invoking the backend, then persist the
resulting success or failure transition. On reconciliation or resume, detect an
existing in-flight marker and reconcile the ambiguous backend outcome instead of
replaying Reset or PowerOff blindly, including handling transaction or
optimistic-lock failures safely.
🪄 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: 2ac2bda9-c2c6-4119-82be-5ef7ad9c344d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
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.rscrates/machine-controller/tests/integration/main.rscrates/machine-controller/tests/integration/maintenance.rs
🚧 Files skipped from review as they are similar to previous changes (12)
- crates/api-model/src/machine/json.rs
- crates/machine-controller/src/io.rs
- crates/machine-controller/Cargo.toml
- crates/api-model/src/test_support/machine_snapshot.rs
- crates/api-db/src/machine.rs
- crates/api-core/src/setup.rs
- crates/machine-controller/src/context.rs
- crates/api-model/src/machine/slas.rs
- crates/machine-controller/src/handler.rs
- crates/api-model/src/machine/mod.rs
- crates/api-core/src/handlers/component_manager.rs
- crates/component-manager/src/component_manager.rs
Comment on lines
+126
to
+128
| format!( | ||
| "Machine {host_machine_id} maintenance ({operation_label}): component manager not configured" | ||
| ), |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== maintenance.rs outline ==\n'
ast-grep outline crates/machine-controller/src/handler/maintenance.rs --view expanded || true
printf '\n== relevant lines in maintenance.rs ==\n'
sed -n '110,220p' crates/machine-controller/src/handler/maintenance.rs | cat -n
printf '\n== search FailureCause and lint-error-messages ==\n'
rg -n "FailureCause|lint-error-messages|error message|Display" crates .github xtask Cargo.toml -S
printf '\n== search for lowercase/error-display guidance in repo docs ==\n'
rg -n "lowercase phrases without trailing periods|Error Display messages|lint-error-messages" -S .Repository: NVIDIA/infra-controller
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== xtask error_message_case outline ==\n'
ast-grep outline crates/xtask/src/error_message_case.rs --view expanded || true
printf '\n== xtask main lint wiring ==\n'
sed -n '1,110p' crates/xtask/src/main.rs | cat -n
printf '\n== error_message_case relevant sections ==\n'
sed -n '1,140p' crates/xtask/src/error_message_case.rs | cat -n
printf '\n---\n'
sed -n '140,340p' crates/xtask/src/error_message_case.rs | cat -n
printf '\n== FailureCause Display section ==\n'
sed -n '2260,2310p' crates/api-model/src/machine/mod.rs | cat -n
printf '\n== maintenance error site contexts ==\n'
sed -n '120,220p' crates/machine-controller/src/handler/maintenance.rs | cat -nRepository: NVIDIA/infra-controller
Length of output: 28827
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== finish_maintenance_with_error and surrounding code ==\n'
sed -n '260,320p' crates/machine-controller/src/handler/maintenance.rs | cat -n
printf '\n== all maintenance.rs format! message sites ==\n'
rg -n 'format!\(' crates/machine-controller/src/handler/maintenance.rs -n -A2 -B2
printf '\n== check whether finish_maintenance_with_error is a recognized lint target elsewhere ==\n'
rg -n 'finish_maintenance_with_error\(' crates/xtask/src/error_message_case.rs crates -SRepository: NVIDIA/infra-controller
Length of output: 6378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== macro_message and leading_str_lit logic ==\n'
sed -n '400,520p' crates/xtask/src/error_message_case.rs | cat -n
printf '\n---\n'
sed -n '520,640p' crates/xtask/src/error_message_case.rs | cat -n
printf '\n== tests around format! and error-site detection ==\n'
sed -n '729,860p' crates/xtask/src/error_message_case.rs | cat -nRepository: NVIDIA/infra-controller
Length of output: 17837
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== collector visit_expr_call / visit_expr_method_call ==\n'
sed -n '360,460p' crates/xtask/src/error_message_case.rs | cat -n
printf '\n== any helper-call heuristics or known error sinks ==\n'
rg -n 'finish_maintenance_with_error|error sink|error slot|ERROR_TYPE_OWNERS|CONTEXT_METHODS|MSG_MACROS|COND_MSG_MACROS' crates/xtask/src/error_message_case.rs -n -A3 -B3Repository: NVIDIA/infra-controller
Length of output: 7606
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== FailureCause::UnhandledState definition ==\n'
sed -n '1660,1715p' crates/api-model/src/machine/mod.rs | cat -n
printf '\n== usages of UnhandledState.err ==\n'
rg -n 'UnhandledState \{ err|err: cause|err,' crates -S
printf '\n== failure details display and debug related impls ==\n'
sed -n '2298,2336p' crates/api-model/src/machine/mod.rs | cat -nRepository: NVIDIA/infra-controller
Length of output: 47155
Lowercase the maintenance failure messages
These format!-built error strings still need to start lowercase: the four sites at lines 126-128, 145, 188-190, and 205-207 all begin with Machine. cargo xtask lint-error-messages only checks recognized error sinks, so this helper path needs the manual machine ... fix to keep FailureCause::UnhandledState aligned with C-GOOD-ERR.
🤖 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 126 - 128,
Lowercase the leading “Machine” in the four maintenance failure messages within
the maintenance handler, including the format! message near the
component-manager configuration error and the corresponding messages at the
other referenced sites. Keep the existing message content and interpolation
unchanged while ensuring each starts with “machine”.
Source: Coding guidelines
zhaozhongn
approved these changes
Jul 20, 2026
amit-pabalkar
approved these changes
Jul 20, 2026
jayzhudev
approved these changes
Jul 20, 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
5 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
#3714
Type of Change
Breaking Changes
Testing
Additional Notes
Nonw