fix(CICD, machine-controller): wait for host power-off before power-on - #4454
Merged
Conversation
After NVIDIA#4430 fixed the stale Machine-a-Tron action queue, the rack integration test exposed a second ordering problem: the controller could enter the `On` phase before the BMC had actually applied `ForceOff`. So, this gives the observed power-off its own persisted `WaitingForPlatformPowerOff` phase. `ForceOff` is issued once, stale power readings stay parked there, and the existing idempotent `On` phase remains restart-safe if the process exits between the Redfish request and state commit. Tests added! This supports NVIDIA#4446 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
Contributor
Summary by CodeRabbit
WalkthroughThe DPU initialization flow adds a persisted ChangesHost power-cycle sequencing
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant DpuInitController
participant Redfish
participant PersistedDpuState
DpuInitController->>Redfish: issue ForceOff
DpuInitController->>PersistedDpuState: persist WaitingForPlatformPowerOff
DpuInitController->>Redfish: check host power state
Redfish-->>DpuInitController: report On or Off
DpuInitController->>PersistedDpuState: persist On after Off
Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
chet
changed the title
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
✅ Action performedFull review finished. |
bcavnvidia
approved these changes
Jul 31, 2026
chet
enabled auto-merge (squash)
July 31, 2026 19:20
![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/machine-controller/tests/integration/power_management.rs (1)
146-261: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueTest covers the required scenarios; consider extending stale-reading coverage.
This test satisfies the regression-coverage objectives for multi-DPU hosts, a stale
Onreading, theOff→Ontransition, and restart recovery afterOnsucceeds.The stale-reading check at Line 199-214 only exercises
On. The PR objectives also require that the controller does not issue additional power requests while the BMC reportsPoweringOfforUnknown. If those states are not already covered at the unit level, add one morerun_single_iterationcycle for each to close the gap here.🤖 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/tests/integration/power_management.rs` around lines 146 - 261, Extend dpu_init_power_cycle_waits_for_observed_host_power_off after the existing stale On check to simulate BMC PoweringOff and Unknown readings, running one run_single_iteration cycle for each. Assert the persisted state remains waiting_for_power_off and actions_since the corresponding timepoint contains no additional power request, while preserving the existing Off-to-On and restart-recovery assertions.
🤖 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 5010-5035: Add an all-DPU convergence check at the start of
handle_waiting_for_platform_power_off, returning StateHandlerOutcome::wait
unless state.managed_state.all_dpu_states_in_sync()? is true. Only perform
is_host_powered_off and transition via next_state_with_all_dpus_updated after
synchronization is confirmed.
---
Nitpick comments:
In `@crates/machine-controller/tests/integration/power_management.rs`:
- Around line 146-261: Extend
dpu_init_power_cycle_waits_for_observed_host_power_off after the existing stale
On check to simulate BMC PoweringOff and Unknown readings, running one
run_single_iteration cycle for each. Assert the persisted state remains
waiting_for_power_off and actions_since the corresponding timepoint contains no
additional power request, while preserving the existing Off-to-On and
restart-recovery assertions.
🪄 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: ce4fe919-8dc4-477e-9208-d3b80c96d80e
📒 Files selected for processing (5)
crates/api-core/src/tests/machine_history.rscrates/api-model/src/machine/mod.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/io.rscrates/machine-controller/tests/integration/power_management.rs
Comment on lines
+5010
to
+5035
|
|
||
| /// Waits for the one host-wide `ForceOff` to become visible through Redfish. | ||
| /// | ||
| /// Normal dispatch calls this before walking individual DPUs so a | ||
| /// multi-DPU host performs one BMC read per controller iteration. | ||
| async fn handle_waiting_for_platform_power_off( | ||
| &self, | ||
| state: &ManagedHostStateSnapshot, | ||
| ctx: &mut StateHandlerContext<'_, MachineStateHandlerContextObjects>, | ||
| ) -> Result<StateHandlerOutcome<ManagedHostState>, StateHandlerError> { | ||
| // Redfish power actions are asynchronous. Persist this wait before | ||
| // trusting a new reading so stale `On` cannot skip the power cycle. | ||
| if !is_host_powered_off(state, ctx).await? { | ||
| return Ok(StateHandlerOutcome::wait(format!( | ||
| "Waiting for host {} to power off before powering it on", | ||
| state.host_snapshot.id, | ||
| ))); | ||
| } | ||
|
|
||
| let next_state = DpuInitState::WaitingForPlatformPowercycle { | ||
| substate: PerformPowerOperation::On, | ||
| } | ||
| .next_state_with_all_dpus_updated(&state.managed_state)?; | ||
|
|
||
| Ok(StateHandlerOutcome::transition(next_state)) | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== all_dpu_states_in_sync ==="
rg -n -A 25 'fn all_dpu_states_in_sync' --type rust
echo "=== next_state_with_all_dpus_updated ==="
rg -n -A 25 'fn next_state_with_all_dpus_updated' --type rust
echo "=== is_host_powered_off ==="
rg -n -A 25 'fn is_host_powered_off' --type rust
echo "=== callers of handle_waiting_for_platform_power_off ==="
rg -n -B2 -A2 'handle_waiting_for_platform_power_off' --type rustRepository: NVIDIA/infra-controller
Length of output: 192
🏁 Script executed:
set -euo pipefail
echo "=== candidate files ==="
git ls-files | rg '(^|/)(handler|.*state.*|.*managed.*)\.(rs|rs\.in)$' | head -200
echo "=== symbol references ==="
rg -n --hidden --glob '*.rs' \
'all_dpu_states_in_sync|next_state_with_all_dpus_updated|is_host_powered_off|handle_waiting_for_platform_power_off|WaitingForPlatformPowercycle|WaitingForPlatformPowerOff' .Repository: NVIDIA/infra-controller
Length of output: 14242
🏁 Script executed:
set -euo pipefail
echo "=== helper trait and implementations ==="
cat -n crates/machine-controller/src/handler/helpers.rs | sed -n '1,180p'
cat -n crates/machine-controller/src/handler/helpers.rs | sed -n '190,285p'
cat -n crates/machine-controller/src/handler/helpers.rs | sed -n '470,610p'
echo "=== DPU state dispatch and power transitions ==="
cat -n crates/machine-controller/src/handler.rs | sed -n '4200,4410p'
cat -n crates/machine-controller/src/handler.rs | sed -n '5000,5080p'
echo "=== power-state helper ==="
cat -n crates/machine-controller/src/handler.rs | sed -n '11200,11265p'Repository: NVIDIA/infra-controller
Length of output: 36349
🏁 Script executed:
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
handler = Path("crates/machine-controller/src/handler.rs").read_text()
helpers = Path("crates/machine-controller/src/handler/helpers.rs").read_text()
start = handler.index("async fn handle_waiting_for_platform_power_off")
end = handler.index("\n }\n}", start) + len("\n }")
body = handler[start:end]
print("=== power-off handler guard check ===")
print("calls all_dpu_states_in_sync:", "all_dpu_states_in_sync" in body)
print("calls is_host_powered_off:", "is_host_powered_off" in body)
print("calls next_state_with_all_dpus_updated:", "next_state_with_all_dpus_updated" in body)
start = helpers.index("fn next_state_with_all_dpus_updated(\n self,\n current_state: &ManagedHostState,")
end = helpers.index("\n }\n}", start) + len("\n }")
body = helpers[start:end]
print("=== DpuInitState bulk transition behavior ===")
print("matches current DPUInit state:", "ManagedHostState::DPUInit { dpu_states }" in body)
print("checks current DPU values before replacement:",
bool(re.search(r"states\\.values\\(\\)|all_dpu_states_in_sync|matches!\\(", body)))
print("replaces every DPU value:", ".states\n .keys()" in body)
PY
echo "=== all_equal implementation ==="
rg -n -A 20 '^fn all_equal|^pub fn all_equal|all_equal' crates/machine-controller/src/handler/helpers.rs
echo "=== power-management tests around convergence ==="
cat -n crates/machine-controller/tests/integration/power_management.rs | sed -n '130,205p'Repository: NVIDIA/infra-controller
Length of output: 1218
🏁 Script executed:
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
helpers = Path("crates/machine-controller/src/handler/helpers.rs").read_text()
start = helpers.index(
"fn next_state_with_all_dpus_updated(\n self,\n current_state: &ManagedHostState,"
)
end = helpers.index("\n }\n}", start) + len("\n }")
body = helpers[start:end]
print("=== DpuInitState bulk transition behavior ===")
print("checks DPU state values before replacement:",
"states.values()" in body or "all_dpu_states_in_sync" in body or "matches!" in body)
print("iterates all DPU keys:", ".states" in body and ".keys()" in body)
print("constructs DPUInit state:", "ManagedHostState::DPUInit" in body)
print("=== mixed-state model ===")
states = {"dpu-a": "WaitingForPlatformPowerOff", "dpu-b": "Init"}
print("before:", states)
if not ("states.values()" in body or "all_dpu_states_in_sync" in body or "matches!" in body):
states = {key: "WaitingForPlatformPowercycle(On)" for key in states}
print("after bulk transition:", states)
PY
echo "=== all_equal implementation and usages ==="
rg -n -A 25 'all_equal' crates/machine-controller/src/handler/helpers.rs
echo "=== power-management tests around convergence ==="
cat -n crates/machine-controller/tests/integration/power_management.rs | sed -n '130,205p'Repository: NVIDIA/infra-controller
Length of output: 5766
Add an all-DPU convergence guard before the host power-off check.
next_state_with_all_dpus_updated replaces every DPU state without validating the current states. The per-DPU path can therefore advance all DPUs to WaitingForPlatformPowercycle { On } while another DPU remains in a different state. Return wait unless state.managed_state.all_dpu_states_in_sync()? is true.
🤖 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 5010 - 5035, Add an
all-DPU convergence check at the start of handle_waiting_for_platform_power_off,
returning StateHandlerOutcome::wait unless
state.managed_state.all_dpu_states_in_sync()? is true. Only perform
is_host_powered_off and transition via next_state_with_all_dpus_updated after
synchronization is confirmed.
Source: Path instructions
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.
After #4430 fixed the stale Machine-a-Tron action queue, the rack integration test exposed a second ordering problem: the controller could enter the
Onphase before the BMC had actually appliedForceOff.So, this gives the observed power-off its own persisted
WaitingForPlatformPowerOffphase.ForceOffis issued once, stale power readings stay parked there, and the existing idempotentOnphase remains restart-safe if the process exits between the Redfish request and state commit.Tests added!
Related issues
Type of Change
Breaking Changes
No configuration or API changes are required.
Testing
Unit tests added/updated
Integration tests added/updated
Manual testing performed
No testing required (docs, internal refactor, etc.)
cargo test -p carbide-machine-controller --test integration dpu_init_power_cycle_waits_for_observed_host_power_off -- --nocapturecargo test -p carbide-api-core tests::machine_history::test_machine_state_history --lib -- --nocapturecargo make check-format-nightlycargo make clippycargo make carbide-lintsAdditional Notes
WaitingForPlatformPowerOffremains part of the existingwaitingforplatformpowercyclemetric so dashboards and label cardinality do not change.ForceOff, matching the existing behavior. A separate retry policy is outside this fix.