feat: ensure the boot config through one shared mechanism#3403
Conversation
|
@coderabbitai full_review, thanks! |
|
(=^･ω･^=) ✅ Action performedFull review finished. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary by CodeRabbit
WalkthroughThe PR introduces a shared ChangesEnsure boot configuration flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant attempt_state_transition
participant ensure_boot_config
participant BMC
participant resume_to
attempt_state_transition->>ensure_boot_config: dispatch step and resume target
ensure_boot_config->>BMC: check, unlock, or relock lockdown
ensure_boot_config->>BMC: apply boot order and pace reboot
ensure_boot_config->>resume_to: resume validation or reprovisioning state
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/tests/machine_states.rs`:
- Around line 3266-3313: Expand
test_pre_consolidation_validation_states_continue_via_ensure_boot_config into
table-driven cases covering RepairBootConfig, PrepareBootRepair,
UnlockForBootRepair, and LockAfterBootRepair. For each persisted legacy state,
assert the transition reaches the corresponding EnsureBootConfig recovery step
and that resume_to preserves the original validation ID at RebootHost; reuse the
shared setup and operation across cases.
🪄 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: 57712178-056c-4084-9277-5194c3a0a04b
📒 Files selected for processing (7)
crates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/dpu_reprovisioning.rscrates/api-core/src/tests/machine_states.rscrates/api-model/src/machine/mod.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/handler/machine_validation.rscrates/machine-controller/src/io.rs
|
@coderabbitai full_review, thanks! |
|
(=^･ω･^=) ✅ Action performedFull review finished. |
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/machine-controller/src/handler.rs`:
- Around line 11391-11399: Update ensure_boot_config_apply_start and the
EnsureBootConfig flow so selecting Apply first evaluates
should_skip_boot_order_remediation(). When the skip applies, bypass only
boot-order remediation while preserving the BIOS repair path; otherwise retain
the existing Apply initialization and boot-order 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: 6268c332-4c65-41bb-83bb-396885714e9c
📒 Files selected for processing (7)
crates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/dpu_reprovisioning.rscrates/api-core/src/tests/machine_states.rscrates/api-model/src/machine/mod.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/handler/machine_validation.rscrates/machine-controller/src/io.rs
|
@coderabbitai full_review, thanks! |
|
(=^･ω･^=) ✅ Action performedFull review finished. |
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/machine-controller/src/handler.rs`:
- Around line 3347-3356: Update the legacy compatibility transition using
ensure_for_reprovision_tail to select EnsureBootConfigStep::Relock when
should_skip_boot_order_remediation(state) is true, matching the current
SetBootOrder path; otherwise retain the existing EnsureBootConfigStep::Apply
with cloned set_boot_order_info.
🪄 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: c73d7ac9-7c7d-4dc1-81d3-320a2ce16e7f
📒 Files selected for processing (7)
crates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/dpu_reprovisioning.rscrates/api-core/src/tests/machine_states.rscrates/api-model/src/machine/mod.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/handler/machine_validation.rscrates/machine-controller/src/io.rs
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
|
@coderabbitai full_review, thanks! |
|
(=^･ω･^=) ✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/machine-controller/src/handler.rs (1)
11366-11644: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
CheckLockdown/Unlocklogic is now duplicated betweenensure_boot_configand the still-inlineReprovisionState::PrepareHostBootRepair/UnlockHostForBootRepairarms.The new step machine re-implements the same lockdown-check → unlock (with the Supermicro reboot special-case and
WaitForUefiBootpacing) thathandle_dpu_reprovision'sPrepareHostBootRepair/UnlockHostForBootRepairarms still run inline (lines ~3033-3146, unchanged). Only the boot-order/relock tail was migrated to the shared flow per this PR's scope, leaving two parallel implementations of the unlock choreography. Any future vendor-specific fix (e.g. another quirky BMC) now needs to be applied in both places to stay consistent.Worth tracking as a follow-up to fully consolidate DPU reprovision's host-boot-repair entry (
PrepareHostBootRepair/UnlockHostForBootRepair) ontoEnsureBootConfigStep::CheckLockdown/Unlockin a later pass, rather than as part of this PR.🤖 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 11366 - 11644, Consolidate the duplicated lockdown-check and unlock choreography by migrating the inline ReprovisionState::PrepareHostBootRepair and UnlockHostForBootRepair handling in handle_dpu_reprovision to EnsureBootConfigStep::CheckLockdown and Unlock. Preserve the Supermicro reboot special case and WaitForUefiBoot pacing, and remove the parallel inline implementation so future vendor-specific changes are centralized in ensure_boot_config.
🤖 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.
Nitpick comments:
In `@crates/machine-controller/src/handler.rs`:
- Around line 11366-11644: Consolidate the duplicated lockdown-check and unlock
choreography by migrating the inline ReprovisionState::PrepareHostBootRepair and
UnlockHostForBootRepair handling in handle_dpu_reprovision to
EnsureBootConfigStep::CheckLockdown and Unlock. Preserve the Supermicro reboot
special case and WaitForUefiBoot pacing, and remove the parallel inline
implementation so future vendor-specific changes are centralized in
ensure_boot_config.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d5cca6d7-35ad-48c2-8905-330a4b63c7d8
📒 Files selected for processing (7)
crates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/dpu_reprovisioning.rscrates/api-core/src/tests/machine_states.rscrates/api-model/src/machine/mod.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/handler/machine_validation.rscrates/machine-controller/src/io.rs
|
Ran the pre-merge agent-review pass (self-review + Codex) per our review workflow; three findings, all verified against the code and taken:
Also re-audited the review-round changes against the style guide (post-review house-conformance check): clean; one deliberate semantic worth reviewer eyes -- the engine-level skip means DGX H100/Viking hosts no longer attempt the boot-order write during first provisioning either (uniform policy, previously attempted). Full suite green (1449) with the changes. |
|
@coderabbitai full_review, thanks! |
|
(=^･ω･^=) ✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/rpc/src/model/instance/status/tenant.rs (1)
56-58: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMatch the effective state by reference instead of cloning it.
Every branch only inspects the state, so Line 58 unnecessarily clones structures such as the DPU reprovision map on each status projection.
Proposed change
- let machine_state = machine_state.effective_state().clone(); + let machine_state = machine_state.effective_state();As per coding guidelines and path instructions, “Avoid needless clones.”
🤖 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/rpc/src/model/instance/status/tenant.rs` around lines 56 - 58, Update the machine_state binding in the status projection to borrow the result of effective_state() rather than cloning it. Preserve the existing state inspection logic and adjust any downstream matching or references to operate on the borrowed state.Sources: Coding guidelines, 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.
Nitpick comments:
In `@crates/rpc/src/model/instance/status/tenant.rs`:
- Around line 56-58: Update the machine_state binding in the status projection
to borrow the result of effective_state() rather than cloning it. Preserve the
existing state inspection logic and adjust any downstream matching or references
to operate on the borrowed state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e255945a-787b-4489-8521-137cd5b16289
📒 Files selected for processing (8)
crates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/dpu_reprovisioning.rscrates/api-core/src/tests/machine_states.rscrates/api-model/src/machine/mod.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/handler/machine_validation.rscrates/machine-controller/src/io.rscrates/rpc/src/model/instance/status/tenant.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- crates/api-core/src/tests/common/api_fixtures/mod.rs
- crates/machine-controller/src/io.rs
- crates/api-model/src/machine/mod.rs
- crates/api-core/src/tests/machine_states.rs
- crates/api-core/src/tests/dpu_reprovisioning.rs
- crates/machine-controller/src/handler/machine_validation.rs
- crates/machine-controller/src/handler.rs
|
@coderabbitai full_review, thanks! |
|
(=^･ω･^=) ✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 7 minutes. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/rpc/src/model/instance/status/tenant.rs (1)
56-58: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid cloning the full effective state.
effective_state().clone()deep-clones the resumed state, including DPU reprovision maps, although this match only reads it. Match the borrowed effective state directly.Proposed fix
- let machine_state = machine_state.effective_state().clone(); + let machine_state = machine_state.effective_state();As per coding guidelines, “Avoid needless clones”; as per path instructions, Rust changes should avoid needless clones.
🤖 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/rpc/src/model/instance/status/tenant.rs` around lines 56 - 58, Update the machine-state handling near effective_state() to match or otherwise read the borrowed effective state directly instead of calling clone(). Preserve the existing resumed-state behavior while avoiding a deep clone of the effective state and its DPU reprovision maps.Sources: Coding guidelines, 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.rs`:
- Around line 11433-11449: Update the reboot-wait verification around the
is_bios_setup check to also validate boot order via is_boot_order_setup, while
honoring the existing hardware condition that intentionally skips boot-order
remediation. If either BIOS setup or applicable boot-order configuration has
drifted, transition to EnsureBootConfig with CheckLockdown instead of proceeding
to another reboot.
---
Nitpick comments:
In `@crates/rpc/src/model/instance/status/tenant.rs`:
- Around line 56-58: Update the machine-state handling near effective_state() to
match or otherwise read the borrowed effective state directly instead of calling
clone(). Preserve the existing resumed-state behavior while avoiding a deep
clone of the effective state and its DPU reprovision maps.
🪄 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: cee71d0a-d6ae-4bdf-b056-99b54e7cb81e
📒 Files selected for processing (8)
crates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/dpu_reprovisioning.rscrates/api-core/src/tests/machine_states.rscrates/api-model/src/machine/mod.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/handler/machine_validation.rscrates/machine-controller/src/io.rscrates/rpc/src/model/instance/status/tenant.rs
The state machine establishes a host's boot config once, early -- but the stages that reboot hosts each grew their own way of noticing the config drifted across a reboot and putting it back: machine validation carried four ensure states, DPU reprovision carried a five-state host boot repair, and every future reboot site was on its own. The mechanics were already shared; the noticing, the routing, and the resume were not. One mechanism now owns all of it. EnsureBootConfig is a top-level state with a resume continuation: it checks whether the BMC is locked and unlocks it if so, drives the boot-order flow to put the config back (verifying first and re-applying only what drifted), re-locks per the expected-machine policy, and resumes the interrupted state. - Machine validation and DPU reprovision's host boot repair now run through EnsureBootConfig; their old states remain only as thin compatibility shims that transition records parked in them into the shared flow. - Detection lives in one shared pacing call, pace_reboot_or_ensure_boot_config: pace reboots for a host that has not come back, and when its boot config reads drifted -- changed externally, a BIOS quirk, or the boot NIC dropping off the BMC's inventory during POST -- ensure it instead of rebooting forever. - The unlock step now tolerates vendors that report lockdown but cannot set it, matching the re-lock side. - State history and metrics show the activity under one name (ensurebootconfig) wherever it happens. Tests updated! This supports NVIDIA#3391 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai full_review, thanks! |
|
(=^･ω･^=) ✅ Action performedFull review finished. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/machine-controller/src/handler.rs (1)
1832-1873: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPass the resumed state into the reprovision resolver
The dispatch now useseffective_state(), butnext_state_with_all_dpus_updatedonly acceptsReady,DPUReprovision, orAssignedreprovision states. Passingstate.managed_statehere will reject anEnsureBootConfigwrapper on the restart path. Use the unwrapped state consistently.🤖 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 1832 - 1873, Update the DPUReprovision branch in the effective_state dispatch to pass the unwrapped resumed state into next_state_with_all_dpus_updated instead of state.managed_state. Use the same effective-state value resolved for dispatch so EnsureBootConfig wrappers are accepted while preserving existing handling for Ready, DPUReprovision, and Assigned states.Source: Path instructions
🧹 Nitpick comments (1)
crates/rpc/src/model/instance/status/tenant.rs (1)
56-58: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid cloning the resumed state for status projection.
effective_state()already returns a borrow; matching it directly avoids cloning potentially large DPU reprovision maps.Proposed change
- let machine_state = machine_state.effective_state().clone(); + let machine_state = machine_state.effective_state();As per coding guidelines and path instructions, “Avoid needless clones.”
🤖 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/rpc/src/model/instance/status/tenant.rs` around lines 56 - 58, Update the machine_state assignment in the status projection to retain the borrow returned by effective_state() instead of cloning the resumed state. Adjust subsequent matching or usage to work with the borrowed value while preserving the existing status behavior.Sources: Coding guidelines, 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.
Outside diff comments:
In `@crates/machine-controller/src/handler.rs`:
- Around line 1832-1873: Update the DPUReprovision branch in the effective_state
dispatch to pass the unwrapped resumed state into
next_state_with_all_dpus_updated instead of state.managed_state. Use the same
effective-state value resolved for dispatch so EnsureBootConfig wrappers are
accepted while preserving existing handling for Ready, DPUReprovision, and
Assigned states.
---
Nitpick comments:
In `@crates/rpc/src/model/instance/status/tenant.rs`:
- Around line 56-58: Update the machine_state assignment in the status
projection to retain the borrow returned by effective_state() instead of cloning
the resumed state. Adjust subsequent matching or usage to work with the borrowed
value while preserving the existing status behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 15de047d-ad8b-4173-91e3-53f0615f1fd0
📒 Files selected for processing (8)
crates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/dpu_reprovisioning.rscrates/api-core/src/tests/machine_states.rscrates/api-model/src/machine/mod.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/handler/machine_validation.rscrates/machine-controller/src/io.rscrates/rpc/src/model/instance/status/tenant.rs
1 participant
The state machine establishes a host's boot config once, early -- but the
stages that reboot hosts each grew their own way of noticing the config
drifted across a reboot and putting it back: machine validation carried four
ensure states, DPU reprovision carried a five-state host boot repair, and
every future reboot site was on its own. The mechanics were already shared;
the noticing, the routing, and the resume were not.
One mechanism now owns all of it. EnsureBootConfig is a top-level state with
a resume continuation: it checks whether the BMC is locked and unlocks it if
so, drives the boot-order flow to put the config back (verifying first and
re-applying only what drifted), re-locks per the expected-machine policy, and
resumes the interrupted state.
EnsureBootConfig; their old states remain only as thin compatibility shims
that transition records parked in them into the shared flow.
pace reboots for a host that has not come back, and when its boot config
reads drifted -- changed externally, a BIOS quirk, or the boot NIC dropping
off the BMC's inventory during POST -- ensure it instead of rebooting forever.
matching the re-lock side.
wherever it happens.
Tests updated!
This supports #3391
Signed-off-by: Chet Nichols III chetn@nvidia.com