feat: ensure a correct boot config during machine validation#3369
Conversation
|
@coderabbitai full_review, thanks! |
|
✅ Action performedFull review finished. |
Summary by CodeRabbit
WalkthroughThe validation workflow detects reverted boot configuration, repairs it through new persisted substates, restores BMC lockdown when applicable, and resumes validation. An end-to-end test verifies repair ordering and state transitions. ChangesValidation boot repair
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ValidationController
participant RedfishBMC
participant BootOrderEngine
ValidationController->>RedfishBMC: Verify required boot configuration
RedfishBMC-->>ValidationController: Report reverted configuration
ValidationController->>RedfishBMC: Disable lockdown
ValidationController->>BootOrderEngine: Reapply host boot order
BootOrderEngine-->>ValidationController: Return repair outcome
ValidationController->>RedfishBMC: Restore lockdown
ValidationController->>ValidationController: Resume at RebootHost
Possibly related issues
Possibly related PRs
🚥 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
🧹 Nitpick comments (1)
crates/machine-controller/src/handler/machine_validation.rs (1)
240-431: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider extending coverage for the untested boot-repair branches.
The new E2E test only exercises "lockdown enabled + non-Supermicro" path. The Supermicro reboot sub-path (Lines 306-321), both
RedfishError::NotSupportedbranches, and thehost_profile.disable_lockdownskip (Lines 408-412) aren't covered by any test in this PR. Given the branch count and vendor-specific behavior, a focused table-driven test (or targeted unit tests per STYLE_GUIDE.md's table-driven testing guidance) aroundhandle_machine_validation_statefor these substates would reduce regression risk on vendor-specific quirks.🤖 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/machine_validation.rs` around lines 240 - 431, Add focused table-driven tests for handle_machine_validation_state covering Supermicro lockdown-disable flow with host reboot, RedfishError::NotSupported during lockdown status lookup, RedfishError::NotSupported during lockdown re-enable, and host_profile.disable_lockdown skipping re-enable; assert each branch’s resulting state, wait/error behavior, and relevant Redfish interactions.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/machine_validation.rs`:
- Around line 298-304: In the UnlockHostState::DisableLockdown branch, do not
propagate RedfishError::NotSupported from
lockdown_bmc(EnabledDisabled::Disabled) as a hard failure. Mirror the tolerant
handling implemented in LockAfterBootRepair: treat NotSupported as a successful
transition while continuing to return other errors through
redfish_error("lockdown_bmc", e).
---
Nitpick comments:
In `@crates/machine-controller/src/handler/machine_validation.rs`:
- Around line 240-431: Add focused table-driven tests for
handle_machine_validation_state covering Supermicro lockdown-disable flow with
host reboot, RedfishError::NotSupported during lockdown status lookup,
RedfishError::NotSupported during lockdown re-enable, and
host_profile.disable_lockdown skipping re-enable; assert each branch’s resulting
state, wait/error behavior, and relevant Redfish interactions.
🪄 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: 7c725ff8-c452-4593-b96d-56f456a52d65
📒 Files selected for processing (5)
crates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/machine_states.rscrates/api-model/src/machine/mod.rscrates/machine-controller/src/handler/machine_validation.rscrates/machine-controller/src/io.rs
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/machine-controller/src/handler/machine_validation.rs (2)
305-322: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftSupermicro reboot/UEFI-wait branch appears untested by the new e2e test.
The
vendor.is_supermicro()branch (routing throughRebootHost→WaitForUefiBootbeforeRepairBootConfig) is a distinct code path from the direct-to-RepairBootConfigpath exercised bytest_machine_validation_repairs_reverted_boot_config. Since that test's fixture doesn't appear to configure a Supermicro BMC vendor, this branch's behavior (extra reboot pacing,uefi_boot_wait) is currently unverified end-to-end.Consider adding a second scenario (or parameterizing the existing test) with a Supermicro vendor fixture to cover this path, given it changes both the state sequence and introduces an extra reboot.
🤖 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/machine_validation.rs` around lines 305 - 322, Add or parameterize an end-to-end machine-validation test to use a Supermicro BMC vendor fixture, covering the vendor.is_supermicro() branch in the validation state handler. Assert the resulting RebootHost → WaitForUefiBoot flow, including reboot pacing and uefi_boot_wait, before RepairBootConfig, alongside the existing non-Supermicro scenario.
148-187: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winVerification-gated repair trigger looks correct.
Detection logic is sound: transient Redfish errors are logged and retried, only a definitive
Ok(false)triggers thePrepareBootRepairtransition. No correctness issues found here.Given this flags a hardware/BIOS anomaly (boot NIC dropping off inventory, BIOS quirk) that operators would want to trend/alert on, consider emitting a
carbide_instrument::Event(e.g.,carbide_validation_boot_repair_totalcounter) at the point the repair is triggered, rather than only atracing::warn!. This gives site operators visibility into recurring hardware issues without needing to grep logs.🤖 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/machine_validation.rs` around lines 148 - 187, Add a Carbide instrumentation event or counter for validation boot repairs at the definitive Ok(false) branch in the boot-config verification logic, alongside the existing tracing::warn! and immediately before transitioning to PrepareBootRepair. Use the project’s established carbide_instrument::Event pattern and name it consistently, such as carbide_validation_boot_repair_total, including the machine identifier if supported.
🤖 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/machine_validation.rs`:
- Around line 305-322: Add or parameterize an end-to-end machine-validation test
to use a Supermicro BMC vendor fixture, covering the vendor.is_supermicro()
branch in the validation state handler. Assert the resulting RebootHost →
WaitForUefiBoot flow, including reboot pacing and uefi_boot_wait, before
RepairBootConfig, alongside the existing non-Supermicro scenario.
- Around line 148-187: Add a Carbide instrumentation event or counter for
validation boot repairs at the definitive Ok(false) branch in the boot-config
verification logic, alongside the existing tracing::warn! and immediately before
transitioning to PrepareBootRepair. Use the project’s established
carbide_instrument::Event pattern and name it consistently, such as
carbide_validation_boot_repair_total, including the machine identifier if
supported.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 14403b81-331f-4e1b-afef-c1250cc8094c
📒 Files selected for processing (5)
crates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/machine_states.rscrates/api-model/src/machine/mod.rscrates/machine-controller/src/handler/machine_validation.rscrates/machine-controller/src/io.rs
Machine validation now ensures the host's boot device config is what it should be, and corrects it when it isn't. While waiting for the host to boot into the validation environment, validation verifies the boot config; if it reads reverted -- however it drifted, whether someone changed it externally, a BIOS quirk, or the boot NIC dropping off the BMC's inventory during the reboot's POST -- validation unlocks the BMC, re-drives the boot-order flow (re-assert the HTTP-boot device, reboot to apply, set and verify the order), re-locks, and resumes from its reboot step. Without this, a drifted boot config was unrecoverable at this stage: the host can't boot into scout, more reboots can't restore a setting that is gone, and the BMC lockdown enabled earlier in HostInit blocks any fix -- so validation paced reboots indefinitely. - Verification lives in the wait-for-reboot path: an intact config keeps the normal reboot pacing; only a reverted one enters correction. - The correction reuses the boot-order flow and the host-boot-repair unlock/re-lock choreography, honoring the expected-machine lockdown policy. - New validation substates (PrepareBootRepair, UnlockForBootRepair, RepairBootConfig, LockAfterBootRepair) surface the correction in state history and metrics. Tests updated! This supports NVIDIA#3365 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai full_review, thanks! |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/machine-controller/src/handler/machine_validation.rs (2)
36-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider applying the new
validating()helper consistently.The helper is introduced specifically to avoid repeating the
ManagedHostState::Validation { validation_state: ValidationState::MachineValidation { .. } }wrapper, and the new repair arms use it correctly. However, the pre-existingRebootHostarm (~lines 119-129) andhandle_machine_validation_requested(~lines 479-485) still hand-construct the identical shape. Migrating those tovalidating(...)would fully realize the DRY benefit this helper was added for.🤖 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/machine_validation.rs` around lines 36 - 42, Use the new validating() helper consistently by replacing the duplicated ManagedHostState::Validation/ValidationState::MachineValidation construction in the RebootHost arm and handle_machine_validation_requested with validating(...), preserving each existing MachineValidatingState value.
148-187: 🚀 Performance & Scalability | 🔵 TrivialAdded Redfish call per waiting iteration — confirm BMC load impact at scale.
Every controller tick while waiting for the validation reboot now issues an additional
is_bios_setupRedfish query (on top of whatevertrigger_reboot_if_neededalready does), for every host in this state — not just ones that will actually drift. The window is bounded by reboot duration, so this is likely acceptable, but worth confirming polling frequency and concurrent host count don't create meaningful additional BMC query volume at fleet scale.🤖 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/machine_validation.rs` around lines 148 - 187, Review the validation waiting loop around the boot-config check and assess the added is_bios_setup call for fleet-scale BMC load. Confirm the controller tick frequency, expected concurrent hosts, and reboot-window duration; if volume is excessive, reduce polling or reuse the result from trigger_reboot_if_needed rather than issuing a duplicate Redfish query each iteration.crates/api-core/src/tests/machine_states.rs (1)
3231-3251: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen coverage by asserting the intermediate boot-repair substates.
The PR explicitly introduces
PrepareBootRepair,UnlockForBootRepair,RepairBootConfig, andLockAfterBootRepairto expose the repair process in state history/metrics, but this test only checks side effects (redfish actions, lockdown flags, finalRebootHostresumption) without verifying the host actually transitions through those substates in order. A regression that skips or reorders one of these substates (while still ending up unlocked → repaired → re-locked → resumed) could slip through undetected.Consider capturing
host.current_state()on each iteration of the second loop and asserting the expected substate sequence is observed at least once before resumption.♻️ Illustrative sketch of stronger substate coverage
let mut resumed = false; + let mut seen_prepare = false; + let mut seen_unlock = false; + let mut seen_repair = false; + let mut seen_lock = false; for _ in 0..20 { env.run_machine_state_controller_iteration().await; let mut txn = env.db_txn().await; let host = mh.host().db_machine(&mut txn).await; + match host.current_state() { + ManagedHostState::Validation { + validation_state: ValidationState::MachineValidation { + machine_validation: MachineValidatingState::PrepareBootRepair { .. }, + }, + } => seen_prepare = true, + ManagedHostState::Validation { + validation_state: ValidationState::MachineValidation { + machine_validation: MachineValidatingState::UnlockForBootRepair { .. }, + }, + } => seen_unlock = true, + ManagedHostState::Validation { + validation_state: ValidationState::MachineValidation { + machine_validation: MachineValidatingState::RepairBootConfig { .. }, + }, + } => seen_repair = true, + ManagedHostState::Validation { + validation_state: ValidationState::MachineValidation { + machine_validation: MachineValidatingState::LockAfterBootRepair { .. }, + }, + } => seen_lock = true, + _ => {} + } if matches!( host.current_state(), ManagedHostState::Validation { validation_state: ValidationState::MachineValidation { machine_validation: MachineValidatingState::RebootHost { .. }, }, } ) { resumed = true; break; } } + assert!(seen_prepare && seen_unlock && seen_repair && seen_lock, + "expected to observe all boot-repair substates");🤖 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/tests/machine_states.rs` around lines 3231 - 3251, Strengthen the second controller-iteration loop by recording each observed host state and asserting that PrepareBootRepair, UnlockForBootRepair, RepairBootConfig, and LockAfterBootRepair are each reached in that order before RebootHost resumption. Update the existing resumed assertion to verify the ordered substate sequence rather than only the final state, using the relevant ManagedHostState and machine validation state variants.
🤖 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/api-core/src/tests/machine_states.rs`:
- Around line 3231-3251: Strengthen the second controller-iteration loop by
recording each observed host state and asserting that PrepareBootRepair,
UnlockForBootRepair, RepairBootConfig, and LockAfterBootRepair are each reached
in that order before RebootHost resumption. Update the existing resumed
assertion to verify the ordered substate sequence rather than only the final
state, using the relevant ManagedHostState and machine validation state
variants.
In `@crates/machine-controller/src/handler/machine_validation.rs`:
- Around line 36-42: Use the new validating() helper consistently by replacing
the duplicated ManagedHostState::Validation/ValidationState::MachineValidation
construction in the RebootHost arm and handle_machine_validation_requested with
validating(...), preserving each existing MachineValidatingState value.
- Around line 148-187: Review the validation waiting loop around the boot-config
check and assess the added is_bios_setup call for fleet-scale BMC load. Confirm
the controller tick frequency, expected concurrent hosts, and reboot-window
duration; if volume is excessive, reduce polling or reuse the result from
trigger_reboot_if_needed rather than issuing a duplicate Redfish query each
iteration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9736dd1d-433b-44d5-88b8-e5346eeb7eb1
📒 Files selected for processing (5)
crates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/machine_states.rscrates/api-model/src/machine/mod.rscrates/machine-controller/src/handler/machine_validation.rscrates/machine-controller/src/io.rs
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
why is this specific to MachineValidating?
There was a problem hiding this comment.
I guess I'm really asking: why not make this generic so it can be used in other states?
There was a problem hiding this comment.
That's exactly where this is headed -- the issue (#3365) frames this as the first adopter of a shared "host didn't return from a reboot" recovery, with the other reboot-wait sites (instance start, UefiSetup, WaitingForLockdown, firmware updates, re-discovery) listed as future adopters.
Two reasons it starts validation-specific rather than generic: the repair already reuses the shared pieces end-to-end (set_host_boot_order drives the whole correction, and the unlock/re-lock mirrors host boot repair) -- what's state-specific is only the detection point and the resume-into-the-interrupted-stage transitions; and making THOSE generic means designing a return-to continuation across the state model, which deserves its own change rather than riding this one. Validation got it first because it's the spot we watched a host wedge on real hardware. Happy to take the generic step as the follow-up.
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
2 participants
Machine validation now ensures the host's boot device config is what it
should be, and corrects it when it isn't. While waiting for the host to boot
into the validation environment, validation verifies the boot config; if it
reads reverted -- however it drifted, whether someone changed it externally,
a BIOS quirk, or the boot NIC dropping off the BMC's inventory during the
reboot's POST -- validation unlocks the BMC, re-drives the boot-order flow
(re-assert the HTTP-boot device, reboot to apply, set and verify the order),
re-locks, and resumes from its reboot step.
Without this, a drifted boot config was unrecoverable at this stage: the host
can't boot into scout, more reboots can't restore a setting that is gone, and
the BMC lockdown enabled earlier in HostInit blocks any fix -- so validation
paced reboots indefinitely.
normal reboot pacing; only a reverted one enters correction.
unlock/re-lock choreography, honoring the expected-machine lockdown policy.
RepairBootConfig, LockAfterBootRepair) surface the correction in state
history and metrics.
Tests updated!
This supports #3365
Signed-off-by: Chet Nichols III chetn@nvidia.com