fix: Use states to manage reboot operation to avoid race condition.#3779
Merged
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 selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
Summary by CodeRabbit
WalkthroughThe DPF state machine now persists reboot operations and retry counts, performs delayed and bounded host power transitions, reports reboot completion, and integrates the new flow into reprovisioning and DPU initialization. ChangesDPF reboot handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Workflow
participant DpfHandler
participant Redfish
participant DpfSdk
Workflow->>DpfHandler: Dispatch HandleReboot(op, retry_count)
DpfHandler->>DpfHandler: Wait for power_down_wait
DpfHandler->>Redfish: Issue ForceOff or On command
Redfish-->>DpfHandler: Return power state
DpfHandler->>DpfSdk: Report reboot_complete when powered On
DpfHandler-->>Workflow: Persist next DPF state
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
chet
approved these changes
Jul 21, 2026
abvarshney-nv
force-pushed
the
bug_6482334
branch
from
July 21, 2026 17:00
f0f0ca2 to
88a7443
Compare
![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 (2)
crates/api-model/src/machine/mod.rs (1)
1764-1773: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDoc comments hardcode "30 s" for a configurable delay.
The
Off/Onvariant docs and theHandleRebootdoc all state a fixed "30 s delay," but the actual value isreachability_params.power_down_wait(a configuredchrono::Durationthreaded intohandle_dpf_state/handle_dpf_handle_reboot). If the default changes or an environment overrides it, these docs on a persisted enum become misleading.📝 Suggested doc wording
pub enum DpfRebootOp { - /// ForceOff issued; waiting for `power_state == Off` then 30 s delay. + /// ForceOff issued; waiting for `power_state == Off` then the configured + /// `power_down_wait` delay. Off, - /// On issued; waiting for `power_state == On` then 30 s delay. + /// On issued; waiting for `power_state == On` then the configured + /// `power_down_wait` delay. On, }Also applies to: 1791-1797
🤖 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-model/src/machine/mod.rs` around lines 1764 - 1773, Update the documentation for DpfRebootOp::Off, DpfRebootOp::On, and HandleReboot to describe a configured power-down wait from reachability_params.power_down_wait rather than hardcoding “30 s”; keep the docs accurate for overridden or changed durations without altering runtime behavior.crates/machine-controller/src/handler/dpf.rs (1)
309-316: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale doc:
HandleReboot { On, now }doesn't match the implementation.The transition description mentions a
nowfield, butHandleRebootonly hasopandretry_count— the successfulOff → Ontransition actually setsretry_count: 0(line 350-353). This looks like it wasn't updated when the design moved from timestamp-based tracking to retry-count tracking (per the library summary).📝 Suggested fix
/// Transitions: /// - `Off`: wait for `power_state == Off` + 30 s → issue On → -/// `HandleReboot { On, now }` +/// `HandleReboot { op: On, retry_count: 0 }` /// - `On`: wait for `power_state == On` + 30 s → `reboot_complete` → /// `WaitingForReady`🤖 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/dpf.rs` around lines 309 - 316, Update the `HandleReboot` transition documentation to match the current `DpfState` fields and implementation: describe the successful `Off → On` transition as `HandleReboot { op: On, retry_count: 0 }` instead of referencing `now`, while preserving the documented timing and subsequent `On` transition.
🤖 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/dpf.rs`:
- Around line 372-380: Update both retry-exhaustion branches in the power-down
handler to return StateHandlerError::ManualInterventionRequired instead of
StateHandlerOutcome::wait, preserving the existing manual-intervention context.
Change both tracing::error! calls to emit MAX_RETRIES as a structured tracing
attribute rather than interpolating it into the message, while retaining the
host identifier and logfmt-style fields.
---
Nitpick comments:
In `@crates/api-model/src/machine/mod.rs`:
- Around line 1764-1773: Update the documentation for DpfRebootOp::Off,
DpfRebootOp::On, and HandleReboot to describe a configured power-down wait from
reachability_params.power_down_wait rather than hardcoding “30 s”; keep the docs
accurate for overridden or changed durations without altering runtime behavior.
In `@crates/machine-controller/src/handler/dpf.rs`:
- Around line 309-316: Update the `HandleReboot` transition documentation to
match the current `DpfState` fields and implementation: describe the successful
`Off → On` transition as `HandleReboot { op: On, retry_count: 0 }` instead of
referencing `now`, while preserving the documented timing and subsequent `On`
transition.
🪄 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: 2a1b5ebd-40e2-404c-b971-48f107276bda
📒 Files selected for processing (4)
crates/api-model/src/machine/mod.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/handler/dpf.rscrates/machine-controller/src/handler/helpers.rs
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-model/src/machine/mod.rs`:
- Around line 1781-1782: Update the documentation comment immediately above
handle_dpf_handle_reboot to describe the delay as the configured
power-transition delay derived from reachability_params.power_down_wait, rather
than referring to a fixed 30 seconds. Preserve the explanation that
state.version.timestamp() records when the step started.
🪄 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: a628a9be-5caa-4a09-8500-5c7a7011d686
📒 Files selected for processing (4)
crates/api-model/src/machine/mod.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/handler/dpf.rscrates/machine-controller/src/handler/helpers.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/machine-controller/src/handler/helpers.rs
- crates/machine-controller/src/handler.rs
- crates/machine-controller/src/handler/dpf.rs
abvarshney-nv
force-pushed
the
bug_6482334
branch
from
July 21, 2026 17:18
88a7443 to
365f106
Compare
bcavnvidia
approved these changes
Jul 21, 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.
Three fixes to the DPF-requested power cycle:
No delay between power commands and status check:
ForceOff and On were issued and their result checked in the same tick, giving the BMC no time to act. A delay (reachability_params.power_down_wait, consistent with ReprovisionState::PowerDown) is now enforced after each command before reading back the power state.
No retry when power state does not change:
If the host failed to respond to ForceOff or On, the handler would wait indefinitely with no recovery. The handler now retries the command up to 3 times before stopping and waiting for manual intervention.
Only the current DPU's state was advanced on power off/on:
A host power cycle affects all DPUs simultaneously, but the handler was calling set_one_dpu_dpf_state instead of transition_all_dpus_to_dpf_state. With multiple DPUs, the second DPU would enter the same reboot branch independently and issue another ForceOff/On cycle against the host, causing repeated unintended reboots. All DPUs now advance together when a host power transition completes.
Related issues
Type of Change
Breaking Changes
Testing
Additional Notes