feat(machine-controller): converge pending boot interface intent - #4389
Conversation
|
@coderabbitai full_review, thanks! |
|
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:
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughThis change adds persisted boot-interface verification, a ChangesBoot interface convergence
Network configuration migration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ReadyState
participant MachineStateHandler
participant Redfish
participant BootInterfaceDB
ReadyState->>MachineStateHandler: detect pending boot-interface version
MachineStateHandler->>Redfish: reconcile target and lockdown
Redfish-->>MachineStateHandler: return observed configuration
MachineStateHandler->>BootInterfaceDB: mark matching version verified
BootInterfaceDB-->>ReadyState: persist observation and return to Ready
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
🐇🔍 ✅ Action performedFull review finished. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/api-core/src/instance/mod.rs (1)
1540-1546: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winConsider demoting the transient reason below ERROR.
PendingBootConfigurationis an expected, self-healing race between an operator boot-interface change and an allocation attempt, yet it now flows through the sametracing::error!as genuine misuse. Every racing allocation will therefore emit an ERROR, which erodes the signal value of that level.♻️ Proposed severity split
if let Err(e) = mh_snapshot.is_usable_as_instance(request.allow_unhealthy_machine) { - tracing::error!( - %machine_id, - error = %e, - "Host can not be used as instance due to reason", - ); + if matches!(e, NotAllocatableReason::PendingBootConfiguration) { + tracing::info!( + %machine_id, + error = %e, + "Host can not be used as instance due to reason", + ); + } else { + tracing::error!( + %machine_id, + error = %e, + "Host can not be used as instance due to reason", + ); + } return Err(not_allocatable_error(machine_id, e)); }🤖 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/instance/mod.rs` around lines 1540 - 1546, Update the error handling around mh_snapshot.is_usable_as_instance to distinguish PendingBootConfiguration from genuine unusable-machine failures. Log the transient PendingBootConfiguration case at a lower severity such as debug or warn, while retaining tracing::error! for all other reasons and preserving the existing not_allocatable_error return behavior.
🧹 Nitpick comments (6)
crates/machine-controller/src/io.rs (1)
281-294: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the metric-state label contract.
ready_boot_config_state_namedefines nine externally consumed substate labels, but the added test only exercisesmanual_intervention_reason. Add table-driven coverage for everyReadyBootConfigStatevariant and its expected(state, substate)metric labels to prevent silent dashboard or alert regressions.🤖 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/io.rs` around lines 281 - 294, Extend the tests around ready boot configuration metrics to table-drive all nine ReadyBootConfigState variants through ready_boot_config_state_name, asserting each expected (state, substate) label pair, while retaining the existing manual_intervention_reason coverage.Source: Coding guidelines
crates/api-db/src/machine_desired_boot_interface.rs (2)
104-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared projection to prevent the two readers from drifting.
loadandload_for_updatenow carry nine identical projection lines and differ solely by the trailingFOR UPDATE OF machine. The next column added to this row shape will almost certainly land in one query and not the other, producing a silent behavioural split between the locked and unlocked read paths — the hardest class of bug to spot in review.♻️ Suggested consolidation
+const DESIRED_BOOT_INTERFACE_SELECT: &str = r#" + SELECT + machine.version AS machine_version, + boot_interface.desired_mac_address, + boot_interface.desired_interface_id, + boot_interface.desired_version, + COALESCE( + machine.controller_state->>'state' IN ('ready', 'assigned'), + false + ) AS rollout_baseline_eligible + FROM machines machine + LEFT JOIN machine_boot_interfaces boot_interface + ON boot_interface.machine_id = machine.id + WHERE machine.id = $1 +"#;
loadthen bindsDESIRED_BOOT_INTERFACE_SELECT, andload_for_updatebinds&format!("{DESIRED_BOOT_INTERFACE_SELECT} FOR UPDATE OF machine")(or a second const composed from it), keeping one definition of the row shape.🤖 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-db/src/machine_desired_boot_interface.rs` around lines 104 - 150, Extract the duplicated SELECT projection shared by load and load_for_update into a single constant such as DESIRED_BOOT_INTERFACE_SELECT. Update both query paths to reuse that projection, appending FOR UPDATE OF machine only in load_for_update, while preserving their existing bindings and error handling.
235-240: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the policy booleans from one exhaustive
match.Both query branches test a single variant with
matches!, soVerificationPolicyis never matched exhaustively. A fourth policy added later would compile cleanly and silently behave asPendingin both branches — a quiet downgrade of a verification decision. Naming the booleans once also spares the reader from correlating$5/$6with amatches!call twenty lines away.♻️ Suggested hardening
+ // Exhaustive so a new policy cannot silently fall back to `Pending`. + let (assume_verified, carry_current_forward) = match verification_policy { + VerificationPolicy::Pending => (false, false), + VerificationPolicy::AssumeVerified => (true, false), + VerificationPolicy::CarryCurrentForward => (false, true), + }; let desired_version = next_version(expected_version);.bind(expected_version) - .bind(matches!( - verification_policy, - VerificationPolicy::CarryCurrentForward - )) + .bind(carry_current_forward).bind(desired_version) - .bind(matches!( - verification_policy, - VerificationPolicy::AssumeVerified - )) + .bind(assume_verified)Also applies to: 272-275, 307-310
🤖 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-db/src/machine_desired_boot_interface.rs` around lines 235 - 240, Update the logic around VerificationPolicy and both query branches to derive the policy booleans once through a single exhaustive match over all enum variants, then reuse those named booleans instead of separate matches! checks. Ensure adding a new policy variant requires an explicit match arm and preserves the existing Pending, AssumeVerified, and CarryCurrentForward behavior.crates/api-model/src/machine/json.rs (1)
199-205: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: fold the inconsistency error into a shared constructor.
decode_desired_boot_interfaceroutes its equivalent failure throughdesired_boot_interface_decode_error; this arm reconstructs the sameColumnDecode/io::Errorshape inline. A small helper parameterised by index and message would keep both decoders symmetric and give future column groups one place to hook into.♻️ Suggested shared helper
-fn desired_boot_interface_decode_error(message: impl Into<String>) -> sqlx::Error { - sqlx::Error::ColumnDecode { - index: "desired_boot_interface_(mac,id,version)".to_string(), +fn column_group_decode_error(index: &str, message: impl Into<String>) -> sqlx::Error { + sqlx::Error::ColumnDecode { + index: index.to_string(), source: Box::new(std::io::Error::new( std::io::ErrorKind::InvalidData, message.into(), )), } }- _ => Err(sqlx::Error::ColumnDecode { - index: "boot_interface_(verified_version,observed_at,assumed)".to_string(), - source: Box::new(std::io::Error::new( - std::io::ErrorKind::InvalidData, - "boot interface verified version and observation time must both be set or both be null, and assumed requires an observation", - )), - }), + _ => Err(column_group_decode_error( + "boot_interface_(verified_version,observed_at,assumed)", + "boot interface verified version and observation time must both be set or both be null, and assumed requires an observation", + )),🤖 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/json.rs` around lines 199 - 205, Extract the repeated ColumnDecode/InvalidData construction into a shared helper parameterized by the column index and error message. Update the inline inconsistency arm and desired_boot_interface_decode_error to use this helper, preserving their existing indices and messages while keeping both decoders’ behavior unchanged.crates/machine-controller/src/handler.rs (2)
13275-13345: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider the table-driven helpers for these input-variant assertions.
ready_boot_config_adopts_targets_only_at_safe_boundariesandfailed_ready_boot_config_restarts_only_for_superseding_intentboth invoke one total function across several inputs, which is precisely whatvalue_scenarios!/check_valuesare for; named scenarios would also improve failure output over{state:?}.As per coding guidelines: "Use the
carbide-test-supporttable-driven testing helpers for functions mapping inputs to outputs or errors; usescenarios!forResult,value_scenarios!for total operations".🤖 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 13275 - 13345, Refactor the tests ready_boot_config_adopts_targets_only_at_safe_boundaries and failed_ready_boot_config_restarts_only_for_superseding_intent to use carbide-test-support value_scenarios!/check_values helpers for these total input-to-output assertions. Replace the manual state loops and direct assertions with named scenarios so failures identify the specific case, while preserving all current inputs and expected results.Source: Coding guidelines
6483-6537: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSkip the inspection when the desired version is already verified.
The guard only checks that
desired_boot_interfaceisSome, so every HostInit lockdown completion pays for a Redfish client creation plusis_bios_setup/is_boot_order_setupreads — even for hosts whoseverified_versionalready matches the desired version, wheremark_verifiedis a no-op rewrite.Machine::pending_boot_interface_config_version()(already used in theReadybranch at Line 913) expresses the precondition directly and removes the redundant round trips from the ingestion path.♻️ Proposed gate on pending intent
let outcome = StateHandlerOutcome::transition(next_state); + if mh_snapshot + .host_snapshot + .pending_boot_interface_config_version() + .is_none() + { + // Already verified for the current desired version; nothing to observe. + return Ok(outcome); + } let Some(desired) = mh_snapshot .host_snapshot .config .desired_boot_interface .as_ref() else { return Ok(outcome); };🤖 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 6483 - 6537, Gate the HostInit boot verification flow using Machine::pending_boot_interface_config_version() before creating the Redfish client or calling inspect_host_boot_config. Return the current outcome when no pending version exists, while preserving the existing verification and warning behavior when pending intent is present; use the pending version for the desired-version context where appropriate.
🤖 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 2463-2467: Update ReadyBootConfigState::fmt to render only the
enum variant name instead of delegating to Debug::fmt, excluding payload fields
such as job IDs and failure text. Preserve the existing Display integration used
by ManagedHostState::fmt and dpu_state_string while ensuring labels remain
stable across payload changes.
---
Outside diff comments:
In `@crates/api-core/src/instance/mod.rs`:
- Around line 1540-1546: Update the error handling around
mh_snapshot.is_usable_as_instance to distinguish PendingBootConfiguration from
genuine unusable-machine failures. Log the transient PendingBootConfiguration
case at a lower severity such as debug or warn, while retaining tracing::error!
for all other reasons and preserving the existing not_allocatable_error return
behavior.
---
Nitpick comments:
In `@crates/api-db/src/machine_desired_boot_interface.rs`:
- Around line 104-150: Extract the duplicated SELECT projection shared by load
and load_for_update into a single constant such as
DESIRED_BOOT_INTERFACE_SELECT. Update both query paths to reuse that projection,
appending FOR UPDATE OF machine only in load_for_update, while preserving their
existing bindings and error handling.
- Around line 235-240: Update the logic around VerificationPolicy and both query
branches to derive the policy booleans once through a single exhaustive match
over all enum variants, then reuse those named booleans instead of separate
matches! checks. Ensure adding a new policy variant requires an explicit match
arm and preserves the existing Pending, AssumeVerified, and CarryCurrentForward
behavior.
In `@crates/api-model/src/machine/json.rs`:
- Around line 199-205: Extract the repeated ColumnDecode/InvalidData
construction into a shared helper parameterized by the column index and error
message. Update the inline inconsistency arm and
desired_boot_interface_decode_error to use this helper, preserving their
existing indices and messages while keeping both decoders’ behavior unchanged.
In `@crates/machine-controller/src/handler.rs`:
- Around line 13275-13345: Refactor the tests
ready_boot_config_adopts_targets_only_at_safe_boundaries and
failed_ready_boot_config_restarts_only_for_superseding_intent to use
carbide-test-support value_scenarios!/check_values helpers for these total
input-to-output assertions. Replace the manual state loops and direct assertions
with named scenarios so failures identify the specific case, while preserving
all current inputs and expected results.
- Around line 6483-6537: Gate the HostInit boot verification flow using
Machine::pending_boot_interface_config_version() before creating the Redfish
client or calling inspect_host_boot_config. Return the current outcome when no
pending version exists, while preserving the existing verification and warning
behavior when pending intent is present; use the pending version for the
desired-version context where appropriate.
In `@crates/machine-controller/src/io.rs`:
- Around line 281-294: Extend the tests around ready boot configuration metrics
to table-drive all nine ReadyBootConfigState variants through
ready_boot_config_state_name, asserting each expected (state, substate) label
pair, while retaining the existing manual_intervention_reason coverage.
🪄 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: 7399eb3b-d623-4849-b4d1-d0c4e430fbe6
📒 Files selected for processing (19)
crates/api-core/src/instance/mod.rscrates/api-core/src/ipxe.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/ipxe.rscrates/api-core/src/tests/machine_states.rscrates/api-db/migrations/20260730120000_machine_boot_interface_status.sqlcrates/api-db/src/machine.rscrates/api-db/src/machine_desired_boot_interface.rscrates/api-db/src/sql/machine_snapshots.sql.templatecrates/api-db/src/sql/managed_hosts.sql.templatecrates/api-model/src/machine/json.rscrates/api-model/src/machine/mod.rscrates/api-model/src/machine/slas.rscrates/api-model/src/machine/status.rscrates/api-model/src/machine_boot_interface.rscrates/api-model/src/test_support/machine_snapshot.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/io.rscrates/redfish/src/libredfish/test_support.rs
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
crates/machine-controller/src/io.rs (1)
430-447: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the negative case for non-terminal substates.
The test pins the terminal reason but not the complement: an active substate such as
ReadyBootConfigState::Preparemust yieldNone, otherwise an over-broad future match arm would page operators for every host merely converging. Two extra lines close the loop.💚 Proposed addition
assert_eq!( <MachineStateControllerIO as StateControllerIO>::manual_intervention_reason(&state), Some("boot_config_convergence_failed"), ); + + let converging = ManagedHostState::BootConfiguring { + boot_config_state: ReadyBootConfigState::Prepare, + ..state + }; + assert_eq!( + <MachineStateControllerIO as StateControllerIO>::manual_intervention_reason( + &converging + ), + None, + ); }🤖 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/io.rs` around lines 430 - 447, Extend terminal_ready_boot_config_requires_manual_intervention to also construct a BootConfiguring state with an active ReadyBootConfigState::Prepare substate and assert that manual_intervention_reason returns None. Keep the existing Failed assertion unchanged so both terminal and non-terminal behavior are covered.crates/api-db/src/machine_desired_boot_interface.rs (1)
235-249: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider making the policy/branch pairing explicit.
AssumeVerifiedis only honoured on the INSERT path andCarryCurrentForwardonly on the UPDATE path; the other combination is silently dropped rather than rejected. No current caller mixes them, but the coupling is invisible at the call site, so a future one would get quiet mis-verification instead of an error. A short doc comment on each variant stating which branch honours it — or splitting into insert-time and update-time policies — would make the API harder to misuse without changing behaviour.🤖 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-db/src/machine_desired_boot_interface.rs` around lines 235 - 249, The VerificationPolicy contract in update should explicitly document that AssumeVerified is honored only on the INSERT branch and CarryCurrentForward only on the UPDATE branch; add concise doc comments to those enum variants (and clarify Pending as appropriate) without changing behavior.crates/machine-controller/src/handler.rs (3)
6471-6548: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect best-effort design.
Every failure mode — absent target, no Redfish client, failed inspection, mismatch — leaves the desired version pending and lets Ready reconciliation own it, so ingestion is never blocked by a verification read. Attaching the transaction via
with_txnalso makes the verification atomic with the state transition.One optional nit: the discarded
boolfrommark_verifiedat line 6540 deserves a one-line comment stating that afalse(target replaced mid-ingestion) is intentionally left to the Ready path, matching the reasoning already spelled out in the doc comment.🤖 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 6471 - 6548, Add a brief comment at the `mark_verified` call in `complete_host_init_lockdown` documenting that its discarded boolean is intentionally ignored: a false result means the target was replaced during ingestion and Ready reconciliation should handle verification. Preserve the existing transaction and state-transition behavior.
13275-13312: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer the table-driven helpers for these predicate sweeps.
Both tests call a single predicate across many inputs via ad-hoc
forloops.value_scenarios!/check_valuesfromcarbide-test-supportwould name each case and produce a scenario-labelled failure, as already done forpending_boot_interface_config_versionincrates/api-model/src/machine/mod.rs. The current"{state:?}"message is serviceable but less descriptive.As per coding guidelines, "Use tables whenever multiple tests call the same operation with different inputs, but keep genuinely distinct tests as standalone tests."
Also applies to: 13212-13240
🤖 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 13275 - 13312, Update the predicate sweep tests around ready_boot_config_can_adopt_latest to use the carbide-test-support value_scenarios! and check_values helpers instead of ad-hoc for loops. Name each state scenario so failures identify the specific case, while preserving the existing expected true and false classifications.Source: Coding guidelines
6294-6398: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the repeated "lock, re-check supersession, else fall back" block.
This transactional shape now appears three times — the
disable_lockdownconvergence short-circuit (lines 6160-6179), the convergence branch here (6306-6322), and the post-lock drift branch (6339-6371). Each opens a transaction, callsmachine_desired_boot_interface::lock, feeds the result toready_boot_config_superseded_state, and applies a per-site fallback. A small helper taking the fallback as a closure would keep the transaction and supersession handling in one place and make a future divergence impossible.🤖 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 6294 - 6398, Extract the repeated transaction and supersession logic from the convergence branches and post-lock drift handling into a shared helper near the relevant state helpers. Have it begin the transaction, lock the current desired boot interface, call ready_boot_config_superseded_state, and use a caller-provided fallback closure when no superseding state exists; update the disable_lockdown convergence path, the ReadyBootConfigTerminalFailure::Convergence branch, and the post-lock drift branch to use it while preserving each existing fallback.
🤖 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-db/migrations/20260730120000_machine_boot_interface_status.sql`:
- Around line 22-28: The migration backfill must only initialize rows for
machines whose controller state is ready or assigned, matching
rollout_baseline_eligible. In
crates/api-db/migrations/20260730120000_machine_boot_interface_status.sql lines
22-28, add that state predicate to the UPDATE. In
crates/api-db/src/machine_desired_boot_interface.rs lines 1348-1438, update
status_migration_assumes_existing_rows_and_constrains_new_status to set
existing_host to ManagedHostState::Ready before STATUS_MIGRATION, seed a second
HostInit host, and assert the ineligible host remains (None, None, false).
In `@crates/machine-controller/src/handler.rs`:
- Around line 5640-5647: The lockdown-safety substate logic is duplicated with
non-exhaustive matches that can fail open when new variants are added. In
crates/machine-controller/src/handler.rs#L5640-L5647, add an exhaustive
predicate such as ready_boot_config_may_have_opened_lockdown over
ReadyBootConfigState, then use it in the early return of
ready_boot_config_missing_dpu_recovery; replace the equivalent inline match at
crates/machine-controller/src/handler.rs#L2520-L2528 and the negated match in
ready_boot_config_requires_timeout_cleanup at
crates/machine-controller/src/handler.rs#L5669-L5675 with this shared predicate.
---
Nitpick comments:
In `@crates/api-db/src/machine_desired_boot_interface.rs`:
- Around line 235-249: The VerificationPolicy contract in update should
explicitly document that AssumeVerified is honored only on the INSERT branch and
CarryCurrentForward only on the UPDATE branch; add concise doc comments to those
enum variants (and clarify Pending as appropriate) without changing behavior.
In `@crates/machine-controller/src/handler.rs`:
- Around line 6471-6548: Add a brief comment at the `mark_verified` call in
`complete_host_init_lockdown` documenting that its discarded boolean is
intentionally ignored: a false result means the target was replaced during
ingestion and Ready reconciliation should handle verification. Preserve the
existing transaction and state-transition behavior.
- Around line 13275-13312: Update the predicate sweep tests around
ready_boot_config_can_adopt_latest to use the carbide-test-support
value_scenarios! and check_values helpers instead of ad-hoc for loops. Name each
state scenario so failures identify the specific case, while preserving the
existing expected true and false classifications.
- Around line 6294-6398: Extract the repeated transaction and supersession logic
from the convergence branches and post-lock drift handling into a shared helper
near the relevant state helpers. Have it begin the transaction, lock the current
desired boot interface, call ready_boot_config_superseded_state, and use a
caller-provided fallback closure when no superseding state exists; update the
disable_lockdown convergence path, the
ReadyBootConfigTerminalFailure::Convergence branch, and the post-lock drift
branch to use it while preserving each existing fallback.
In `@crates/machine-controller/src/io.rs`:
- Around line 430-447: Extend
terminal_ready_boot_config_requires_manual_intervention to also construct a
BootConfiguring state with an active ReadyBootConfigState::Prepare substate and
assert that manual_intervention_reason returns None. Keep the existing Failed
assertion unchanged so both terminal and non-terminal behavior are covered.
🪄 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: fc850e3a-19b9-4976-92b4-b158a63a4b59
📒 Files selected for processing (19)
crates/api-core/src/instance/mod.rscrates/api-core/src/ipxe.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/ipxe.rscrates/api-core/src/tests/machine_states.rscrates/api-db/migrations/20260730120000_machine_boot_interface_status.sqlcrates/api-db/src/machine.rscrates/api-db/src/machine_desired_boot_interface.rscrates/api-db/src/sql/machine_snapshots.sql.templatecrates/api-db/src/sql/managed_hosts.sql.templatecrates/api-model/src/machine/json.rscrates/api-model/src/machine/mod.rscrates/api-model/src/machine/slas.rscrates/api-model/src/machine/status.rscrates/api-model/src/machine_boot_interface.rscrates/api-model/src/test_support/machine_snapshot.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/io.rscrates/redfish/src/libredfish/test_support.rs
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4389.docs.buildwithfern.com/infra-controller |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/api-db/migrations/20260730120000_machine_boot_interface_status.sql (1)
9-20: 🗄️ Data Integrity & Integration | 🔵 TrivialConsider
NOT VALID+ separateVALIDATE CONSTRAINTfor the new CHECK.Adding this constraint without
NOT VALIDforces Postgres to scan and hold anACCESS EXCLUSIVElock onmachine_boot_interfacesfor the duration of validation. In this specific migration the risk is low since the constraint is validated before the backfill runs (all pre-existing rows are trivially(NULL, NULL, false)), but the scan itself is still lock-holding and scales with table size. For consistency with defensive migration practice on a table that may grow with the fleet, consider splitting intoADD CONSTRAINT ... NOT VALIDfollowed by a subsequentVALIDATE CONSTRAINT(which only takes aSHARE UPDATE EXCLUSIVElock).Based on static analysis hints ("By default new constraints require a table scan and block writes to the table while that scan occurs. Use
NOT VALIDwith a laterVALIDATE CONSTRAINTcall.").🤖 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-db/migrations/20260730120000_machine_boot_interface_status.sql` around lines 9 - 20, Update the machine_boot_interfaces_status_consistent CHECK constraint definition to add it with NOT VALID, then add a separate ALTER TABLE VALIDATE CONSTRAINT statement afterward. Preserve the existing constraint expression and ensure validation occurs before the migration’s backfill logic.Source: Linters/SAST tools
🤖 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 913-939: Update the recovery transition handling around
ManagedHostState::BootConfiguring::Failed so reprovision, validation, DPU
reprovision, and BMC rotation requests are evaluated with the same priority as
maintenance or superseding boot-interface intent. Ensure a pending operator
recovery request can transition the host out of the failed boot-config state
instead of remaining blocked indefinitely.
---
Nitpick comments:
In `@crates/api-db/migrations/20260730120000_machine_boot_interface_status.sql`:
- Around line 9-20: Update the machine_boot_interfaces_status_consistent CHECK
constraint definition to add it with NOT VALID, then add a separate ALTER TABLE
VALIDATE CONSTRAINT statement afterward. Preserve the existing constraint
expression and ensure validation occurs before the migration’s backfill logic.
🪄 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: 815060dc-89b1-42a2-bba9-e979d6dd5f90
📒 Files selected for processing (19)
crates/api-core/src/instance/mod.rscrates/api-core/src/ipxe.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/ipxe.rscrates/api-core/src/tests/machine_states.rscrates/api-db/migrations/20260730120000_machine_boot_interface_status.sqlcrates/api-db/src/machine.rscrates/api-db/src/machine_desired_boot_interface.rscrates/api-db/src/sql/machine_snapshots.sql.templatecrates/api-db/src/sql/managed_hosts.sql.templatecrates/api-model/src/machine/json.rscrates/api-model/src/machine/mod.rscrates/api-model/src/machine/slas.rscrates/api-model/src/machine/status.rscrates/api-model/src/machine_boot_interface.rscrates/api-model/src/test_support/machine_snapshot.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/io.rscrates/redfish/src/libredfish/test_support.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/api-db/src/sql/managed_hosts.sql.template
There was a problem hiding this comment.
🧹 Nitpick comments (4)
crates/api-db/src/machine_desired_boot_interface.rs (2)
434-439: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the policy computation below the early return.
assume_verifiedandverification_policyare computed unconditionally, then discarded whenever the row already exists — the common case for an already-initialized host. Purely cosmetic, but it puts the decision next to its only consumer.♻️ Proposed reordering
let row = load_for_update(txn, machine_id).await?; let current_machine_version = row.machine_version; - let assume_verified = machine_id.machine_type().is_host() && row.rollout_baseline_eligible; - let verification_policy = if assume_verified { - VerificationPolicy::AssumeVerified - } else { - VerificationPolicy::Pending - }; + let rollout_baseline_eligible = row.rollout_baseline_eligible; if let Some(current) = row.decode(machine_id)? { return Ok(current); } + let verification_policy = + if machine_id.machine_type().is_host() && rollout_baseline_eligible { + VerificationPolicy::AssumeVerified + } else { + VerificationPolicy::Pending + }; let Some(version) = update(Also applies to: 450-468
🤖 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-db/src/machine_desired_boot_interface.rs` around lines 434 - 439, Move the unconditional computation of assume_verified and verification_policy in the existing-host handling flow below the early return for rows that already exist. Keep the early return behavior unchanged, and compute the policy immediately before the code that consumes it for newly initialized hosts.
109-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider deriving the eligibility predicate from a single shared SQL fragment. The five-line
COALESCE(machine.controller_state->>'state' IN ('ready', 'assigned'), false)expression is duplicated verbatim acrossloadandload_for_update, and the state names are stringly-typed againstManagedHostState. A rename would silently degrade the expression tofalserather than fail to compile; the new SQLx test does cover it, which is why this is a nit rather than a defect.♻️ Optional: hoist the fragment into a shared constant
+const ROLLOUT_BASELINE_ELIGIBLE_SQL: &str = r#" + COALESCE( + machine.controller_state->>'state' IN ('ready', 'assigned'), + false + ) AS rollout_baseline_eligible +"#;Both query strings can then be assembled with
format!(or a small helper) so the predicate exists in exactly one place.Also applies to: 140-144
🤖 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-db/src/machine_desired_boot_interface.rs` around lines 109 - 113, Define one shared SQL fragment or helper for the rollout-baseline eligibility predicate, including the current ready and assigned state values, and reuse it when constructing both load and load_for_update queries. Replace the duplicated inline expressions while preserving the existing COALESCE behavior and SQLx parameter handling.crates/machine-controller/src/handler.rs (2)
902-939: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo observations on the new Ready branch.
The
InvalidStateerror path is unreachable by construction:pending_boot_interface_config_version()derives itsSomedirectly fromconfig.desired_boot_interface, so the target is guaranteed present whenever a version is returned. Resolving both from a single read removes the impossible error arm.Placement gives pending boot intent precedence over
handle_bom_validation_requested,host_reprovisioning_requested,handle_machine_validation_requested, DPU reprovisioning, and BMC rotation. An operator-requested reprovision — which itself reconfigures boot — will now queue behind convergence. Please confirm that ordering is deliberate; the comment at Line 902 only justifies precedence relative to instance allocation.♻️ Optional: resolve the target and version together
- if let Some(desired_version) = mh_snapshot - .host_snapshot - .pending_boot_interface_config_version() - { - let desired_boot_interface = mh_snapshot - .host_snapshot - .config - .desired_boot_interface - .as_ref() - .ok_or_else(|| { - StateHandlerError::InvalidState(format!( - "host {} has a pending desired boot-interface version without a target", - mh_snapshot.host_snapshot.id - )) - })? - .value - .clone(); + if let Some(desired) = mh_snapshot.host_snapshot.pending_desired_boot_interface() { return Ok(StateHandlerOutcome::transition( - ManagedHostState::BootConfiguring { - desired_version, - desired_boot_interface, - post_lock_verification_retry_count: 0, - boot_config_state: ReadyBootConfigState::Prepare, - }, + ready_boot_configuring(desired, 0, ReadyBootConfigState::Prepare), )); }This assumes a small companion accessor on
Machinealongsidepending_boot_interface_config_version:/// The desired boot interface whose persisted convergence status is not current. pub fn pending_desired_boot_interface(&self) -> Option<Versioned<MachineBootInterfaceTarget>> { let desired = self.config.desired_boot_interface.as_ref()?; self.pending_boot_interface_config_version() .map(|_| desired.clone()) }🤖 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 902 - 939, Update the Ready-branch pending boot handling to obtain the desired boot interface and pending version through a single consistent accessor, removing the unreachable InvalidState path; use the existing Machine configuration/accessor symbols and preserve the BootConfiguring transition values. Confirm the ordering explicitly by either documenting that pending boot convergence intentionally precedes operator-requested reprovision, validation, DPU reprovisioning, and BMC rotation, or move this branch below those requests if that precedence is not intended.
6480-6558: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDiscarding
mark_verified's boolean here is correct but silent. Afalsereturn means intent changed between the inspection and the commit — indistinguishable from success in the logs. Given the surrounding branches all log their bail-out reasons, a matchingtracing::debug!on the superseded case would keep the diagnostic story complete.🔍 Optional: log the superseded case
let mut txn = ctx.services.db_pool.begin().await?; - db::machine_desired_boot_interface::mark_verified( + let verified = db::machine_desired_boot_interface::mark_verified( txn.as_mut(), &mh_snapshot.host_snapshot.id, desired.version, Utc::now(), ) .await?; + if !verified { + tracing::debug!( + machine_id = %mh_snapshot.host_snapshot.id, + desired_version = %desired.version, + "Boot target changed during HostInit verification; leaving it pending for Ready reconciliation", + ); + } Ok(outcome.with_txn(txn))Also applies to: 7145-7159, 7219-7223, 7337-7346, 7359-7364
🤖 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 6480 - 6558, Handle the boolean returned by db::machine_desired_boot_interface::mark_verified in complete_host_init_lockdown and the corresponding call sites, rather than discarding it. When it returns false because the desired intent was superseded, emit a tracing::debug! message with the machine and desired-version context; preserve the existing transaction and outcome behavior for both true and false results.
🤖 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-db/src/machine_desired_boot_interface.rs`:
- Around line 434-439: Move the unconditional computation of assume_verified and
verification_policy in the existing-host handling flow below the early return
for rows that already exist. Keep the early return behavior unchanged, and
compute the policy immediately before the code that consumes it for newly
initialized hosts.
- Around line 109-113: Define one shared SQL fragment or helper for the
rollout-baseline eligibility predicate, including the current ready and assigned
state values, and reuse it when constructing both load and load_for_update
queries. Replace the duplicated inline expressions while preserving the existing
COALESCE behavior and SQLx parameter handling.
In `@crates/machine-controller/src/handler.rs`:
- Around line 902-939: Update the Ready-branch pending boot handling to obtain
the desired boot interface and pending version through a single consistent
accessor, removing the unreachable InvalidState path; use the existing Machine
configuration/accessor symbols and preserve the BootConfiguring transition
values. Confirm the ordering explicitly by either documenting that pending boot
convergence intentionally precedes operator-requested reprovision, validation,
DPU reprovisioning, and BMC rotation, or move this branch below those requests
if that precedence is not intended.
- Around line 6480-6558: Handle the boolean returned by
db::machine_desired_boot_interface::mark_verified in complete_host_init_lockdown
and the corresponding call sites, rather than discarding it. When it returns
false because the desired intent was superseded, emit a tracing::debug! message
with the machine and desired-version context; preserve the existing transaction
and outcome behavior for both true and false results.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 71d76283-ad48-469e-b706-725abc8824ef
📒 Files selected for processing (19)
crates/api-core/src/instance/mod.rscrates/api-core/src/ipxe.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/ipxe.rscrates/api-core/src/tests/machine_states.rscrates/api-db/migrations/20260730120000_machine_boot_interface_status.sqlcrates/api-db/src/machine.rscrates/api-db/src/machine_desired_boot_interface.rscrates/api-db/src/sql/machine_snapshots.sql.templatecrates/api-db/src/sql/managed_hosts.sql.templatecrates/api-model/src/machine/json.rscrates/api-model/src/machine/mod.rscrates/api-model/src/machine/slas.rscrates/api-model/src/machine/status.rscrates/api-model/src/machine_boot_interface.rscrates/api-model/src/test_support/machine_snapshot.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/io.rscrates/redfish/src/libredfish/test_support.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/api-core/src/instance/mod.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/machine-controller/src/handler.rs (1)
6149-6333: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the duplicated terminal-failure publication and the lockdown-restoration block.
The
LockHostarm now spans ~260 lines, and theReadyBootConfigTerminalFailurepublication appears twice in near-identical form: once in thedisable_lockdownshort-circuit (Lines 6156-6189) and once after lockdown restoration (Lines 6302-6333). Two copies of the sameMachine→Failed/Convergence→lock+ready_boot_config_superseded_statedecision is exactly the kind of duplication that drifts when one branch gains a nuance.Extracting a
publish_terminal_failure(ctx, mh_snapshot, desired, retry_count, terminal_failure)helper (and, separately, the lockdown-restoration triple into its ownrestore_lockdown(...) -> Result<LockdownRestoreOutcome, _>) would collapse both call sites to a single line each and make the arm's control flow readable at a glance. Behaviour-preserving, so entirely deferrable if you would rather land this as-is.🤖 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 6149 - 6333, Extract the duplicated ReadyBootConfigTerminalFailure handling from the LockHost arm into a shared publish_terminal_failure helper, preserving the existing Machine failure transition and Convergence supersession/failed-state logic at both call sites. Separately extract the lockdown restoration sequence into a restore_lockdown helper returning the required outcome, while preserving all current wait, verification, and error behavior; update LockHost to call these helpers so its control flow is concise.
🤖 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 6149-6333: Extract the duplicated ReadyBootConfigTerminalFailure
handling from the LockHost arm into a shared publish_terminal_failure helper,
preserving the existing Machine failure transition and Convergence
supersession/failed-state logic at both call sites. Separately extract the
lockdown restoration sequence into a restore_lockdown helper returning the
required outcome, while preserving all current wait, verification, and error
behavior; update LockHost to call these helpers so its control flow is concise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6c7bbfcb-b999-4c21-9a04-430f423a45f9
📒 Files selected for processing (19)
crates/api-core/src/instance/mod.rscrates/api-core/src/ipxe.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/ipxe.rscrates/api-core/src/tests/machine_states.rscrates/api-db/migrations/20260730120000_machine_boot_interface_status.sqlcrates/api-db/src/machine.rscrates/api-db/src/machine_desired_boot_interface.rscrates/api-db/src/sql/machine_snapshots.sql.templatecrates/api-db/src/sql/managed_hosts.sql.templatecrates/api-model/src/machine/json.rscrates/api-model/src/machine/mod.rscrates/api-model/src/machine/slas.rscrates/api-model/src/machine/status.rscrates/api-model/src/machine_boot_interface.rscrates/api-model/src/test_support/machine_snapshot.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/io.rscrates/redfish/src/libredfish/test_support.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/api-db/src/sql/machine_snapshots.sql.template
|
@coderabbitai full_review, please review the latest fix. Thanks! |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (7)
crates/api-db/src/machine_desired_boot_interface.rs (1)
250-254: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe flattened policy tuple lets a mismatched policy fail silently.
assume_verifiedis only consumed by the INSERT branch andcarry_current_forwardonly by the UPDATE branch. Consequently,AssumeVerifiedreaching the UPDATE path (orCarryCurrentForwardreaching the INSERT path) degrades toPendingsemantics with no diagnostic — a subtle failure mode for a function whose entire purpose is preserving verification invariants. Matching on the policy inside each branch keeps the intent local and makes an unsupported combination explicit.🤖 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-db/src/machine_desired_boot_interface.rs` around lines 250 - 254, Replace the flattened `assume_verified`/`carry_current_forward` tuple derived from `verification_policy` with branch-local matching in the INSERT and UPDATE paths. In each branch, handle only the policy applicable to that operation and explicitly reject or report mismatched policies instead of silently applying `Pending` semantics.crates/api-core/src/tests/machine_states.rs (1)
3377-3381: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer an upper bound over an exact client-call count.
client_calls_after_preflight + 1couples the test to the precise number of Redfish connectionsLockHosthappens to open today. The invariant being protected is "at most one reconnect for the final observation"; expressing it as<=keeps the guarantee while surviving unrelated, benign changes to the cleanup path.🤖 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 3377 - 3381, Update the client-call assertion in the LockHost test to enforce an upper bound rather than exact equality: the final count must be no greater than client_calls_after_preflight + 1. Preserve the existing message and the invariant that at most one reconnect occurs for the final exact-target observation.crates/api-model/src/machine/mod.rs (1)
3349-3399: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider yielding the round-tripped value rather than a boolean.
check_valueswithexpect: truecollapses the assertion totrue == true; on regression the failure output reveals the scenario name but not the divergence. Yielding the parsed value (scenarios!withYields(state)) gives a diff-quality failure message for free.♻️ Suggested shape
- |state| { - serde_json::from_str::<ReadyBootConfigState>( - &serde_json::to_string(&state).unwrap(), - ) - .unwrap() - == state - }, + |state| { + serde_json::from_str::<ReadyBootConfigState>( + &serde_json::to_string(&state).unwrap(), + ) + .unwrap() + },with each
expectset to the correspondingReadyBootConfigStatevalue.🤖 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 3349 - 3399, Update ready_boot_config_terminal_outcomes_round_trip to have the check_values closure yield the deserialized ReadyBootConfigState instead of a boolean, using the scenarios!/Yields(state) pattern and setting each expected value to its corresponding input state. Preserve all existing scenarios and round-trip serialization behavior while enabling value diffs on failure.Source: Coding guidelines
crates/machine-controller/src/handler.rs (3)
5651-5658: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider removing the panic surface from the cleanup-deadline computation.
chrono::Duration::from_stdon a compile-time SLA constant cannot realistically fail, yet theexpectplaces a panic in a per-tick reconciliation path. Hoisting the conversion into aLazyLock/const, or degrading to a saturating value, keeps the hot path panic-free without changing behaviour.🤖 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 5651 - 5658, Update ready_boot_config_requires_timeout_cleanup to remove the expect panic from converting the BOOT_CONFIGURING SLA; hoist a guaranteed conversion into a reusable LazyLock/const or use an equivalent saturating fallback, while preserving the existing timeout comparison and lockdown condition.
6290-6321: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated Convergence-failure publication block.
Lines 6156-6176 and 6302-6320 are byte-for-byte the same choreography: open a transaction,
lockthe desired target, prefer a superseding version, otherwise park inFailed { failure }, return with the transaction. Two copies of a correctness-critical sequence will drift. A small helper —async fn park_or_supersede(ctx, machine_id, desired, retry_count, failure) -> Result<StateHandlerOutcome<ManagedHostState>, StateHandlerError>— collapses both sites and gives the invariant a single home.🤖 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 6290 - 6321, Extract the duplicated Convergence-failure handling into an async helper named park_or_supersede, accepting ctx, machine_id, desired, retry_count, and failure and returning the existing StateHandlerOutcome result type. Move the transaction creation, desired-interface lock, superseding-version selection, Failed fallback, and transaction attachment into this helper, then replace both convergence-failure call sites with it while preserving their current behavior.
13270-13340: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExpress the multi-input predicate tests as table-driven scenarios.
ready_boot_config_adopts_targets_only_at_safe_boundariesandfailed_ready_boot_config_restarts_only_for_superseding_intentboth drive one total function across several inputs — precisely the shape the repository reserves for the shared helpers. Replacing the hand-rolledforloops and repeatedassert_eq!withvalue_scenarios!(orcheck_values) yields per-case naming and failure attribution for free.As per coding guidelines: "Use tables whenever multiple tests call the same operation with different inputs" and "prefer table-driven tests using
carbide-test-supportscenarios such asscenarios!/value_scenarios!or explicitcheck_cases/check_values".🤖 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 13270 - 13340, Convert the multi-input tests ready_boot_config_adopts_targets_only_at_safe_boundaries and failed_ready_boot_config_restarts_only_for_superseding_intent to table-driven scenarios using the repository’s established value_scenarios!, check_values, or equivalent carbide-test-support helper. Represent each input and expected result as a named case, replacing the hand-written loops and repeated assertions while preserving all current coverage and outcomes.Source: Coding guidelines
crates/test-harness/src/managed_host.rs (1)
114-124: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLift the effectful call out of the assertion expression.
Embedding an awaited database write inside
assert!(...)obscures the side effect and makes the failure site harder to read. Binding the result first keeps the mutation explicit and the assertion trivial.♻️ Suggested restructuring
- assert!( - db::machine_desired_boot_interface::mark_verified( - txn.as_mut(), - &self.host.id, - desired_version, - Utc::now(), - ) - .await - .expect("boot-interface verification should be recorded"), - "test host's desired boot interface should still be current" - ); + let verified = db::machine_desired_boot_interface::mark_verified( + txn.as_mut(), + &self.host.id, + desired_version, + Utc::now(), + ) + .await + .expect("boot-interface verification should be recorded"); + assert!( + verified, + "test host's desired boot interface should still be current" + );🤖 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/test-harness/src/managed_host.rs` around lines 114 - 124, In the managed host flow, update the call to db::machine_desired_boot_interface::mark_verified by awaiting it and binding its boolean result before the assert! invocation. Keep the existing expect message, assertion message, and transaction, host ID, desired version, and timestamp arguments unchanged; make the assertion evaluate only the bound result.
🤖 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 3377-3381: Update the client-call assertion in the LockHost test
to enforce an upper bound rather than exact equality: the final count must be no
greater than client_calls_after_preflight + 1. Preserve the existing message and
the invariant that at most one reconnect occurs for the final exact-target
observation.
In `@crates/api-db/src/machine_desired_boot_interface.rs`:
- Around line 250-254: Replace the flattened
`assume_verified`/`carry_current_forward` tuple derived from
`verification_policy` with branch-local matching in the INSERT and UPDATE paths.
In each branch, handle only the policy applicable to that operation and
explicitly reject or report mismatched policies instead of silently applying
`Pending` semantics.
In `@crates/api-model/src/machine/mod.rs`:
- Around line 3349-3399: Update ready_boot_config_terminal_outcomes_round_trip
to have the check_values closure yield the deserialized ReadyBootConfigState
instead of a boolean, using the scenarios!/Yields(state) pattern and setting
each expected value to its corresponding input state. Preserve all existing
scenarios and round-trip serialization behavior while enabling value diffs on
failure.
In `@crates/machine-controller/src/handler.rs`:
- Around line 5651-5658: Update ready_boot_config_requires_timeout_cleanup to
remove the expect panic from converting the BOOT_CONFIGURING SLA; hoist a
guaranteed conversion into a reusable LazyLock/const or use an equivalent
saturating fallback, while preserving the existing timeout comparison and
lockdown condition.
- Around line 6290-6321: Extract the duplicated Convergence-failure handling
into an async helper named park_or_supersede, accepting ctx, machine_id,
desired, retry_count, and failure and returning the existing StateHandlerOutcome
result type. Move the transaction creation, desired-interface lock,
superseding-version selection, Failed fallback, and transaction attachment into
this helper, then replace both convergence-failure call sites with it while
preserving their current behavior.
- Around line 13270-13340: Convert the multi-input tests
ready_boot_config_adopts_targets_only_at_safe_boundaries and
failed_ready_boot_config_restarts_only_for_superseding_intent to table-driven
scenarios using the repository’s established value_scenarios!, check_values, or
equivalent carbide-test-support helper. Represent each input and expected result
as a named case, replacing the hand-written loops and repeated assertions while
preserving all current coverage and outcomes.
In `@crates/test-harness/src/managed_host.rs`:
- Around line 114-124: In the managed host flow, update the call to
db::machine_desired_boot_interface::mark_verified by awaiting it and binding its
boolean result before the assert! invocation. Keep the existing expect message,
assertion message, and transaction, host ID, desired version, and timestamp
arguments unchanged; make the assertion evaluate only the bound result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ef8e5c6d-f808-4da9-b0f3-807f446ce866
📒 Files selected for processing (26)
crates/api-core/src/instance/mod.rscrates/api-core/src/ipxe.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/ipxe.rscrates/api-core/src/tests/machine_states.rscrates/api-core/tests/integration/compute_allocation.rscrates/api-core/tests/integration/forge_agent_control.rscrates/api-db/migrations/20260730120000_machine_boot_interface_status.sqlcrates/api-db/src/machine.rscrates/api-db/src/machine_desired_boot_interface.rscrates/api-db/src/sql/machine_snapshots.sql.templatecrates/api-db/src/sql/managed_hosts.sql.templatecrates/api-model/src/machine/json.rscrates/api-model/src/machine/mod.rscrates/api-model/src/machine/slas.rscrates/api-model/src/machine/status.rscrates/api-model/src/machine_boot_interface.rscrates/api-model/src/test_support/machine_snapshot.rscrates/api-web/src/tests/env.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/io.rscrates/machine-controller/tests/integration/bmc_rotation.rscrates/machine-controller/tests/integration/maintenance.rscrates/machine-controller/tests/integration/power_management.rscrates/redfish/src/libredfish/test_support.rscrates/test-harness/src/managed_host.rs
A desired boot interface could be persisted while a machine was Ready, but Ready had no durable way to distinguish desired intent from a configuration actually verified through Redfish. That left allocation and restart safety dependent on the caller that changed the target. Persist the boot-interface status version and add a BootConfiguring state for unassigned Ready hosts. The state captures one desired generation, reuses the existing HostInit BIOS, job, and boot-order work, restores and verifies lockdown, then marks only the exact version current after a fresh Redfish read. Stable Ready and Assigned rows receive an explicitly assumed rollout baseline, while in-flight rows remain pending real observation. MacOnly-to-Pair metadata enrichment advances status only when the prior generation was current. Assigned-host convergence and periodic external-drift detection remain separate follow-ups. PostgreSQL, controller, restart, cleanup, allocation, and iPXE tests cover this change. This supports NVIDIA#4246 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
Synthetic Ready hosts bypass the machine controller stage that verifies the desired boot interface. Add an explicit fixture transition that records the current desired version and enters Ready atomically, then use it wherever tests require a stable converged host. Signed-off-by: Chet Nichols III <chetn@nvidia.com>
Keep NIC and Ignore hosts in the existing lockdown-status poll so disable returns to platform configuration and enable is observed before lifecycle progression. Route discovery completion with pending boot intent directly into convergence instead of briefly publishing Ready. Signed-off-by: Chet Nichols III <chetn@nvidia.com>
Add the HPE iLO boot resource used by pinned libredfish for persistent ordering, while keeping its structured OEM values separate from standard BootOption ids. A focused round-trip regression covers the exact trailing-slash GET and PATCH endpoints. Signed-off-by: Chet Nichols III <chetn@nvidia.com>
Persist the Supermicro unlock and reboot path before exact boot-order verification, then carry that durable proof across lockdown restoration instead of trusting a stale locked read. Host initialization leaves the desired generation pending for Ready convergence, with regressions covering the HostInit handoff and restart-safe Ready flow. Signed-off-by: Chet Nichols III <chetn@nvidia.com>
Create the new status consistency constraint directly instead of marking and immediately validating it. Document that allocation eligibility and its metric also require a matching desired boot-interface observation. Signed-off-by: Chet Nichols III <chetn@nvidia.com>
Current main assigned 20260722120000 to BMC suppressions. Move the unpublished IPv6 loopback guard to the immediately following version so SQLx can apply both migrations without changing their intended order. Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai full_review, please review the latest rebased fixes. Thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/api-db/migrations/20260730120000_machine_boot_interface_status.sql (1)
9-20: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider
NOT VALIDfor the new CHECK constraint on large tables.Squawk flags that adding a validated
CHECKconstraint requires a full table scan under the lock acquired by thisALTER TABLE, blocking writes for its duration. Ifmachine_boot_interfacesis large in production, splitting intoADD CONSTRAINT ... CHECK (...) NOT VALIDfollowed by a separateVALIDATE CONSTRAINT(ideally in a later migration, once the backfill has landed) reduces the blocking window.Squawk (2.61.0): "By default new constraints require a table scan and block writes to the table while that scan occurs. Use
NOT VALIDwith a laterVALIDATE CONSTRAINTcall." (constraint-missing-not-valid)🤖 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-db/migrations/20260730120000_machine_boot_interface_status.sql` around lines 9 - 20, Update the machine_boot_interfaces_status_consistent CHECK constraint to be added with NOT VALID, and arrange a separate later validation step or migration using VALIDATE CONSTRAINT after the required backfill has completed. Preserve the existing constraint expression and name.Source: Linters/SAST tools
crates/api-db/src/machine_desired_boot_interface.rs (1)
109-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider centralizing the hardcoded controller-state tag predicate.
The
IN ('ready', 'assigned')literal is now duplicated acrossload,load_for_update, and (per the status migration test) the migration itself. It silently couples three SQL sites to serde's lowercase tag encoding ofManagedHostState; a future rename of either variant would degrade torollout_baseline_eligible = falserather than a compile or query error. The in-repo tests do pin the current behavior, so this is a maintainability hardening rather than a defect — a shared SQL fragment constant would keep the contract in one place.Also applies to: 140-144
🤖 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-db/src/machine_desired_boot_interface.rs` around lines 109 - 113, Centralize the controller-state predicate used by load, load_for_update, and the status migration test into one shared SQL fragment constant. Replace each duplicated IN ('ready', 'assigned') expression with that constant while preserving the existing rollout_baseline_eligible behavior and lowercase serde tag values.crates/machine-controller/src/handler.rs (1)
5920-5966: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unreachable
preflight_completebranches inside theelsearm.The
elseblock at Line 5927 is entered only whenpreflight_complete == false, soif preflight_completeat Line 5934 and the guardpreflight_complete && lockdown_status.is_fully_disabled()at Line 5953 can never hold. The dead arms make the intended lockdown decision harder to read and invite a future reader to assume a semantic difference that does not exist.♻️ Suggested simplification
- let next_state = if preflight_complete { + let next_state = if matches!(preflight_decision, HostBootConfigDecision::Complete) { // Avoid opening an ordinary host that is already correct. ReadyBootConfigState::LockHost { terminal_failure: None, } } else { match redfish_client.lockdown_status().await { Err(RedfishError::NotSupported(_)) => { tracing::info!( machine_id = %mh_snapshot.host_snapshot.id, "BMC vendor does not support checking lockdown status during Ready boot repair", ); - if preflight_complete { - ReadyBootConfigState::LockHost { - terminal_failure: None, - } - } else { - ReadyBootConfigState::CheckHostConfig - } + ReadyBootConfigState::CheckHostConfig } @@ - Ok(lockdown_status) - if preflight_complete && lockdown_status.is_fully_disabled() => - { - ReadyBootConfigState::LockHost { - terminal_failure: None, - } - } Ok(lockdown_status) if !lockdown_status.is_fully_disabled() => {With the dead arms gone, the local
preflight_completebinding can also be dropped.🤖 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 5920 - 5966, Remove the unreachable preflight_complete branches from the else arm that builds next_state: simplify RedfishError::NotSupported to return CheckHostConfig, remove the preflight_complete guard from the successful lockdown-status match arm, and drop the now-unused preflight_complete binding if no longer needed.
🤖 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-db/migrations/20260730120000_machine_boot_interface_status.sql`:
- Around line 9-20: Update the machine_boot_interfaces_status_consistent CHECK
constraint to be added with NOT VALID, and arrange a separate later validation
step or migration using VALIDATE CONSTRAINT after the required backfill has
completed. Preserve the existing constraint expression and name.
In `@crates/api-db/src/machine_desired_boot_interface.rs`:
- Around line 109-113: Centralize the controller-state predicate used by load,
load_for_update, and the status migration test into one shared SQL fragment
constant. Replace each duplicated IN ('ready', 'assigned') expression with that
constant while preserving the existing rollout_baseline_eligible behavior and
lowercase serde tag values.
In `@crates/machine-controller/src/handler.rs`:
- Around line 5920-5966: Remove the unreachable preflight_complete branches from
the else arm that builds next_state: simplify RedfishError::NotSupported to
return CheckHostConfig, remove the preflight_complete guard from the successful
lockdown-status match arm, and drop the now-unused preflight_complete binding if
no longer needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5a7614c1-40b9-47f4-9dc0-17a2ce88b6c8
📒 Files selected for processing (29)
crates/api-core/src/instance/mod.rscrates/api-core/src/ipxe.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/ipxe.rscrates/api-core/src/tests/machine_states.rscrates/api-core/tests/integration/compute_allocation.rscrates/api-core/tests/integration/forge_agent_control.rscrates/api-db/migrations/20260722120001_preserve_machine_ipv6_loopback.sqlcrates/api-db/migrations/20260730120000_machine_boot_interface_status.sqlcrates/api-db/src/machine.rscrates/api-db/src/machine_desired_boot_interface.rscrates/api-db/src/sql/machine_snapshots.sql.templatecrates/api-db/src/sql/managed_hosts.sql.templatecrates/api-model/src/machine/json.rscrates/api-model/src/machine/mod.rscrates/api-model/src/machine/slas.rscrates/api-model/src/machine/status.rscrates/api-model/src/machine_boot_interface.rscrates/api-model/src/test_support/machine_snapshot.rscrates/api-web/src/tests/env.rscrates/bmc-mock/src/redfish/computer_system.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/io.rscrates/machine-controller/src/metrics.rscrates/machine-controller/tests/integration/bmc_rotation.rscrates/machine-controller/tests/integration/maintenance.rscrates/machine-controller/tests/integration/power_management.rscrates/redfish/src/libredfish/test_support.rscrates/test-harness/src/managed_host.rs
CombinedServer strips trailing slashes before routing, so register the HPE GET and PATCH routes in normalized form while retaining canonical OData identifiers. Exercise the regression through the same normalization layer used by the real mock server. Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai full_review, please review the HPE route-normalization fix in the latest commit. The regression test now exercises the same trailing-slash normalization layer used by the real BMC mock server. Thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
crates/api-db/migrations/20260730120000_machine_boot_interface_status.sql (1)
5-20: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSplit the CHECK constraint into
NOT VALID+VALIDATE CONSTRAINTto avoid blocking writes.
ADD CONSTRAINT ... CHECK (...)in the sameALTER TABLEstatement takes anACCESS EXCLUSIVElock for the full validation scan. Adding itNOT VALIDfirst, then validating separately, only needs a briefACCESS EXCLUSIVEfor the metadata change and a non-blockingSHARE UPDATE EXCLUSIVEfor the scan — standard practice for zero-downtime migrations on a live table.As per path instructions, review migrations for "lock/transaction impact."
🔧 Proposed fix
ALTER TABLE machine_boot_interfaces ADD COLUMN verified_version varchar(64), ADD COLUMN observed_at timestamp with time zone, ADD COLUMN assumed boolean NOT NULL DEFAULT false, ADD CONSTRAINT machine_boot_interfaces_status_consistent CHECK ( ( verified_version IS NULL AND observed_at IS NULL AND NOT assumed ) OR ( verified_version IS NOT NULL AND observed_at IS NOT NULL ) - ); + ) NOT VALID; + +-- Validate without holding an ACCESS EXCLUSIVE lock for the full table scan. +ALTER TABLE machine_boot_interfaces + VALIDATE CONSTRAINT machine_boot_interfaces_status_consistent;🤖 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-db/migrations/20260730120000_machine_boot_interface_status.sql` around lines 5 - 20, Split the machine_boot_interfaces CHECK constraint creation from validation: add machine_boot_interfaces_status_consistent with NOT VALID in the ALTER TABLE statement, then run a separate VALIDATE CONSTRAINT statement for it. Preserve the existing constraint expression and column definitions while ensuring validation occurs outside the blocking constraint-add operation.Sources: Path instructions, Linters/SAST tools
crates/api-db/src/machine_desired_boot_interface.rs (2)
450-468: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCompute the verification policy only after the early return.
assume_verifiedandverification_policyare derived before therow.decode(...)?short-circuit, so an already-initialized host pays for work it discards. Moving the derivation below the early return also makes it obvious that the policy applies exclusively to the insert path.♻️ Proposed tidy-up
- let assume_verified = machine_id.machine_type().is_host() && row.rollout_baseline_eligible; - let verification_policy = if assume_verified { - VerificationPolicy::AssumeVerified - } else { - VerificationPolicy::Pending - }; + let rollout_baseline_eligible = + machine_id.machine_type().is_host() && row.rollout_baseline_eligible; if let Some(current) = row.decode(machine_id)? { return Ok(current); } + let verification_policy = if rollout_baseline_eligible { + VerificationPolicy::AssumeVerified + } else { + VerificationPolicy::Pending + };🤖 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-db/src/machine_desired_boot_interface.rs` around lines 450 - 468, Move the assume_verified and verification_policy derivation below the row.decode(machine_id)? early return in the surrounding update flow. Keep the policy values and update call unchanged, ensuring verification policy computation occurs only on the insert path.
109-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the shared projection to avoid divergence between
loadandload_for_update.The two queries are now identical except for
FOR UPDATE OF machine. The eligibility expression in particular is a semantic contract (ready/assigned) that must stay byte-identical in both places; a future edit to one is easy to miss.♻️ Suggested consolidation
const DESIRED_BOOT_INTERFACE_PROJECTION: &str = r#" SELECT machine.version AS machine_version, boot_interface.desired_mac_address, boot_interface.desired_interface_id, boot_interface.desired_version, COALESCE( machine.controller_state->>'state' IN ('ready', 'assigned'), false ) AS rollout_baseline_eligible FROM machines machine LEFT JOIN machine_boot_interfaces boot_interface ON boot_interface.machine_id = machine.id WHERE machine.id = $1 "#;
loaduses it verbatim;load_for_updateappends"\nFOR UPDATE OF machine".Also applies to: 140-144
🤖 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-db/src/machine_desired_boot_interface.rs` around lines 109 - 113, Extract the shared SQL projection used by load and load_for_update into a single constant, including the identical rollout_baseline_eligible expression and query structure. Have load use the constant verbatim, while load_for_update append only the FOR UPDATE OF machine clause, preserving both methods’ existing behavior.crates/machine-controller/src/handler.rs (2)
6574-6582: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSurface the discarded
mark_verifiedoutcome.The compare-and-swap result is dropped here. Behaviourally this is correct — a superseded generation must stay pending — but the operator loses the one signal distinguishing "verified during HostInit" from "intent changed mid-flight", while every other call site logs its decision. A single
debug/infoline keeps the diagnostic story complete at negligible cost.♻️ Proposed adjustment
let mut txn = ctx.services.db_pool.begin().await?; - db::machine_desired_boot_interface::mark_verified( + let verified = db::machine_desired_boot_interface::mark_verified( txn.as_mut(), &mh_snapshot.host_snapshot.id, desired.version, Utc::now(), ) .await?; + if !verified { + tracing::info!( + machine_id = %mh_snapshot.host_snapshot.id, + desired_version = %desired.version, + "Desired boot interface changed during HostInit verification; leaving it pending", + ); + } Ok(outcome.with_txn(txn))🤖 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 6574 - 6582, Capture the return value from db::machine_desired_boot_interface::mark_verified in the surrounding transaction flow and emit a debug or info log recording the compare-and-swap decision, including whether verification succeeded or the generation was superseded. Preserve the existing transaction and outcome handling unchanged.
5920-5966: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unreachable
preflight_completebranches inside theelsearm.The
elseblock at Line 5927 is entered only whenpreflight_completeisfalse, so theif preflight_completeat Line 5934 and the guardOk(lockdown_status) if preflight_complete && ...at Line 5952 can never evaluate to true. They read as meaningful safety logic while contributing nothing, which is precisely the kind of dead branch that misleads a future reader auditing this lockdown decision.♻️ Proposed simplification
match redfish_client.lockdown_status().await { Err(RedfishError::NotSupported(_)) => { tracing::info!( machine_id = %mh_snapshot.host_snapshot.id, "BMC vendor does not support checking lockdown status during Ready boot repair", ); - if preflight_complete { - ReadyBootConfigState::LockHost { - terminal_failure: None, - } - } else { - ReadyBootConfigState::CheckHostConfig - } + ReadyBootConfigState::CheckHostConfig } Err(error) => { ... } - Ok(lockdown_status) - if preflight_complete && lockdown_status.is_fully_disabled() => - { - ReadyBootConfigState::LockHost { - terminal_failure: None, - } - } Ok(lockdown_status) if !lockdown_status.is_fully_disabled() => { ReadyBootConfigState::UnlockHost { unlock_host_state: UnlockHostState::DisableLockdown, } } Ok(_) => ReadyBootConfigState::CheckHostConfig, }🤖 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 5920 - 5966, Remove the unreachable preflight_complete checks within the else branch that constructs next_state. In the NotSupported arm, return ReadyBootConfigState::CheckHostConfig directly, and simplify the successful lockdown-status guard to check only is_fully_disabled() while preserving its existing LockHost transition; leave the surrounding preflight_complete branch unchanged.
🤖 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/bmc-mock/src/redfish/computer_system.rs`:
- Around line 74-79: The HPE boot-order PATCH payload must tolerate missing or
malformed PersistentBootConfigOrder values instead of rejecting the request.
Update HpeBootSettingsPatch and the associated PATCH handler to deserialize a
JSON value, extract only string elements when the value is an array, and leave
the existing stored order unchanged when the field is absent, non-array, or
contains invalid elements.
---
Nitpick comments:
In `@crates/api-db/migrations/20260730120000_machine_boot_interface_status.sql`:
- Around line 5-20: Split the machine_boot_interfaces CHECK constraint creation
from validation: add machine_boot_interfaces_status_consistent with NOT VALID in
the ALTER TABLE statement, then run a separate VALIDATE CONSTRAINT statement for
it. Preserve the existing constraint expression and column definitions while
ensuring validation occurs outside the blocking constraint-add operation.
In `@crates/api-db/src/machine_desired_boot_interface.rs`:
- Around line 450-468: Move the assume_verified and verification_policy
derivation below the row.decode(machine_id)? early return in the surrounding
update flow. Keep the policy values and update call unchanged, ensuring
verification policy computation occurs only on the insert path.
- Around line 109-113: Extract the shared SQL projection used by load and
load_for_update into a single constant, including the identical
rollout_baseline_eligible expression and query structure. Have load use the
constant verbatim, while load_for_update append only the FOR UPDATE OF machine
clause, preserving both methods’ existing behavior.
In `@crates/machine-controller/src/handler.rs`:
- Around line 6574-6582: Capture the return value from
db::machine_desired_boot_interface::mark_verified in the surrounding transaction
flow and emit a debug or info log recording the compare-and-swap decision,
including whether verification succeeded or the generation was superseded.
Preserve the existing transaction and outcome handling unchanged.
- Around line 5920-5966: Remove the unreachable preflight_complete checks within
the else branch that constructs next_state. In the NotSupported arm, return
ReadyBootConfigState::CheckHostConfig directly, and simplify the successful
lockdown-status guard to check only is_fully_disabled() while preserving its
existing LockHost transition; leave the surrounding preflight_complete branch
unchanged.
🪄 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: 5650f36e-16f5-4d43-97b6-2e1910de1f0c
📒 Files selected for processing (29)
crates/api-core/src/instance/mod.rscrates/api-core/src/ipxe.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/ipxe.rscrates/api-core/src/tests/machine_states.rscrates/api-core/tests/integration/compute_allocation.rscrates/api-core/tests/integration/forge_agent_control.rscrates/api-db/migrations/20260722120001_preserve_machine_ipv6_loopback.sqlcrates/api-db/migrations/20260730120000_machine_boot_interface_status.sqlcrates/api-db/src/machine.rscrates/api-db/src/machine_desired_boot_interface.rscrates/api-db/src/sql/machine_snapshots.sql.templatecrates/api-db/src/sql/managed_hosts.sql.templatecrates/api-model/src/machine/json.rscrates/api-model/src/machine/mod.rscrates/api-model/src/machine/slas.rscrates/api-model/src/machine/status.rscrates/api-model/src/machine_boot_interface.rscrates/api-model/src/test_support/machine_snapshot.rscrates/api-web/src/tests/env.rscrates/bmc-mock/src/redfish/computer_system.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/io.rscrates/machine-controller/src/metrics.rscrates/machine-controller/tests/integration/bmc_rotation.rscrates/machine-controller/tests/integration/maintenance.rscrates/machine-controller/tests/integration/power_management.rscrates/redfish/src/libredfish/test_support.rscrates/test-harness/src/managed_host.rs
Route only zero-DPU lockdown disable through status polling so platform configuration still runs, while preserving the established enable path when BMC status is stale or unreadable. Also apply the final review cleanups for constraint validation, HostInit verification diagnostics, dead preflight branches, and compatibility-baseline documentation. Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai full_review, please review the latest head (
I kept the verification-policy derivation where it is because |
|
🐇🔍 ✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
crates/api-db/src/machine_desired_boot_interface.rs (3)
104-118: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the shared projection to avoid divergence.
loadandload_for_updatenow duplicate the full column list and therollout_baseline_eligibleexpression; a future change to the eligibility predicate must be applied twice or the locked and unlocked reads silently disagree. A singleconst DESIRED_BOOT_INTERFACE_PROJECTION: &strinterpolated into both queries would remove that hazard at negligible cost.Also applies to: 135-150
🤖 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-db/src/machine_desired_boot_interface.rs` around lines 104 - 118, Extract the duplicated SELECT column list and rollout_baseline_eligible expression from load and load_for_update into a shared DESIRED_BOOT_INTERFACE_PROJECTION constant, then interpolate it into both queries so locked and unlocked reads remain consistent.
235-266: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
AssumeVerifiedis silently inert on the update path.
assume_verifiedis only consumed by the insert branch andcarry_current_forwardonly by the update branch. Today that is safe (initialize_if_unsetis the soleAssumeVerifiedcaller and always passesexpected_version: None), but a future caller combiningAssumeVerifiedwith an existing row would have its intent discarded without a trace. Adebug_assert!— or better, encoding the branch in the policy match itself — would make the invariant enforceable rather than conventional.🤖 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-db/src/machine_desired_boot_interface.rs` around lines 235 - 266, Make the update path in update enforce that VerificationPolicy::AssumeVerified cannot be used with an existing expected_version, rather than silently ignoring assume_verified. Prefer encoding this constraint in the verification_policy match or add a debug assertion at the update-branch boundary, while preserving CarryCurrentForward behavior.
705-758: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider the workspace table-driven helpers for this case table.
The case list is precisely the shape the repository's
carbide-test-supporthelpers exist for; the hand-rolledforloop re-implements scenario labelling, so a failure reports only a bare assertion rather than a named case. If the helpers accommodate the required async setup (seed_machine/set_controller_state),check_caseswould give per-case attribution for free; otherwise ascenariolabel in eachassert_*message is the cheap substitute.As per coding guidelines: "When writing tests, prefer table-driven tests using
carbide-test-supportscenarios such asscenarios!/value_scenarios!or explicitcheck_cases/check_values".🤖 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-db/src/machine_desired_boot_interface.rs` around lines 705 - 758, Improve rollout_baseline_only_applies_to_existing_stable_ready_or_assigned_hosts so failures identify the specific case: use the repository’s carbide-test-support table-driven helpers such as scenarios!/check_cases if they support the async seed_machine and set_controller_state setup; otherwise add a scenario label to each assertion message while preserving the existing cases and expectations.Source: Coding guidelines
crates/redfish/src/libredfish/test_support.rs (1)
61-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNote the flag's global scope in its documentation.
The default-preserving
unwrap_or(true)is exactly right and leaves existing tests untouched. One asymmetry worth recording: sibling boot-related knobs such asis_boot_order_setupandhttp_dev1_enabledare per-host, whereas this override is simulator-wide. A multi-host test enabling it would silently affect every endpoint. A short clause on the doc comment ("applies to every simulated endpoint") would spare the next author that discovery.Also applies to: 316-320, 1679-1682
🤖 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/redfish/src/libredfish/test_support.rs` around lines 61 - 64, Update the documentation for the lockdown_bmc_applies field and its corresponding configuration points to explicitly state that the override applies to every simulated endpoint, distinguishing its simulator-wide scope from per-host boot-related settings.crates/machine-controller/src/handler.rs (2)
6139-6172: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated terminal-failure publication.
The
disable_lockdownshortcut and the post-restoration path contain byte-for-byte identical publication logic: begin a transaction, lock the desired target, resolve supersession or park inFailed, attach the transaction. This is a correctness-sensitive sequence — a future fix applied to one copy and not the other would silently change behaviour for exactly one class of host. A single helper taking(ctx, mh_snapshot, desired, post_lock_verification_retry_count, terminal_failure)and returning the outcome would make the two call sites one-liners.Also applies to: 6285-6316
🤖 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 6139 - 6172, Extract the duplicated Convergence terminal-failure publication sequence into a shared helper accepting ctx, mh_snapshot, desired, post_lock_verification_retry_count, and terminal_failure, returning the appropriate StateHandlerOutcome with the transaction attached. Update both the lockdown_disabled branch and the post-restoration path to call this helper, while leaving Machine failures on their existing path.
6064-6131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCollapse the four near-identical stage arms.
ConfigureBios,WaitingForBiosJob,PollingBiosSetup, andSetBootOrderdiffer solely in theHostBootConfigStagethey construct, yet each repeats the Redfish client acquisition and the full seven-argument call. Mapping the substate to a stage first, then acquiring the client once, removes roughly forty lines of copy-paste from an already long match.♻️ Suggested consolidation
- ReadyBootConfigState::ConfigureBios { retry_count } => { - let redfish_client = ctx - .services - .create_redfish_client_from_machine(&mh_snapshot.host_snapshot) - .await?; - handle_ready_boot_config_stage( - ctx, - mh_snapshot, - reachability_params, - redfish_client.as_ref(), - desired, - post_lock_verification_retry_count, - HostBootConfigStage::ConfigureBios { retry_count }, - ) - .await - } - ReadyBootConfigState::WaitingForBiosJob { bios_config_info } => { - /* ... three more copies ... */ - } + ReadyBootConfigState::ConfigureBios { .. } + | ReadyBootConfigState::WaitingForBiosJob { .. } + | ReadyBootConfigState::PollingBiosSetup { .. } + | ReadyBootConfigState::SetBootOrder { .. } => { + let stage = host_boot_config_stage_from_ready_state(boot_config_state.clone()) + .expect("only shared-stage substates reach this arm"); + let redfish_client = ctx + .services + .create_redfish_client_from_machine(&mh_snapshot.host_snapshot) + .await?; + handle_ready_boot_config_stage( + ctx, + mh_snapshot, + reachability_params, + redfish_client.as_ref(), + desired, + post_lock_verification_retry_count, + stage, + ) + .await + }An exhaustive inverse of the existing
ready_boot_config_state_from_stagekeeps the mapping in one place; prefer returningOption<HostBootConfigStage>over a panic if the arm pattern and the helper can drift.🤖 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 6064 - 6131, Consolidate the four arms in the ready boot configuration match by first mapping each `ReadyBootConfigState` variant (`ConfigureBios`, `WaitingForBiosJob`, `PollingBiosSetup`, and `SetBootOrder`) to its corresponding `HostBootConfigStage`, then acquire the Redfish client once and invoke `handle_ready_boot_config_stage` once. Use an exhaustive inverse of `ready_boot_config_state_from_stage` that returns `Option<HostBootConfigStage>` rather than panicking if the mappings drift.
🤖 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-db/migrations/20260730120000_machine_boot_interface_status.sql`:
- Around line 1-36: Keep the column additions and NOT VALID constraint in this
migration, but move both VALIDATE CONSTRAINT for
machine_boot_interfaces_status_consistent and the existing
machine_boot_interfaces backfill UPDATE into a subsequent migration. Ensure the
follow-up migration performs validation before the backfill so the initial ALTER
TABLE lock is released before either operation runs.
---
Nitpick comments:
In `@crates/api-db/src/machine_desired_boot_interface.rs`:
- Around line 104-118: Extract the duplicated SELECT column list and
rollout_baseline_eligible expression from load and load_for_update into a shared
DESIRED_BOOT_INTERFACE_PROJECTION constant, then interpolate it into both
queries so locked and unlocked reads remain consistent.
- Around line 235-266: Make the update path in update enforce that
VerificationPolicy::AssumeVerified cannot be used with an existing
expected_version, rather than silently ignoring assume_verified. Prefer encoding
this constraint in the verification_policy match or add a debug assertion at the
update-branch boundary, while preserving CarryCurrentForward behavior.
- Around line 705-758: Improve
rollout_baseline_only_applies_to_existing_stable_ready_or_assigned_hosts so
failures identify the specific case: use the repository’s carbide-test-support
table-driven helpers such as scenarios!/check_cases if they support the async
seed_machine and set_controller_state setup; otherwise add a scenario label to
each assertion message while preserving the existing cases and expectations.
In `@crates/machine-controller/src/handler.rs`:
- Around line 6139-6172: Extract the duplicated Convergence terminal-failure
publication sequence into a shared helper accepting ctx, mh_snapshot, desired,
post_lock_verification_retry_count, and terminal_failure, returning the
appropriate StateHandlerOutcome with the transaction attached. Update both the
lockdown_disabled branch and the post-restoration path to call this helper,
while leaving Machine failures on their existing path.
- Around line 6064-6131: Consolidate the four arms in the ready boot
configuration match by first mapping each `ReadyBootConfigState` variant
(`ConfigureBios`, `WaitingForBiosJob`, `PollingBiosSetup`, and `SetBootOrder`)
to its corresponding `HostBootConfigStage`, then acquire the Redfish client once
and invoke `handle_ready_boot_config_stage` once. Use an exhaustive inverse of
`ready_boot_config_state_from_stage` that returns `Option<HostBootConfigStage>`
rather than panicking if the mappings drift.
In `@crates/redfish/src/libredfish/test_support.rs`:
- Around line 61-64: Update the documentation for the lockdown_bmc_applies field
and its corresponding configuration points to explicitly state that the override
applies to every simulated endpoint, distinguishing its simulator-wide scope
from per-host boot-related settings.
🪄 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: 14924e6e-3e5c-4942-9b0e-1caf90bc46ec
📒 Files selected for processing (29)
crates/api-core/src/instance/mod.rscrates/api-core/src/ipxe.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/ipxe.rscrates/api-core/src/tests/machine_states.rscrates/api-core/tests/integration/compute_allocation.rscrates/api-core/tests/integration/forge_agent_control.rscrates/api-db/migrations/20260722120001_preserve_machine_ipv6_loopback.sqlcrates/api-db/migrations/20260730120000_machine_boot_interface_status.sqlcrates/api-db/src/machine.rscrates/api-db/src/machine_desired_boot_interface.rscrates/api-db/src/sql/machine_snapshots.sql.templatecrates/api-db/src/sql/managed_hosts.sql.templatecrates/api-model/src/machine/json.rscrates/api-model/src/machine/mod.rscrates/api-model/src/machine/slas.rscrates/api-model/src/machine/status.rscrates/api-model/src/machine_boot_interface.rscrates/api-model/src/test_support/machine_snapshot.rscrates/api-web/src/tests/env.rscrates/bmc-mock/src/redfish/computer_system.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/io.rscrates/machine-controller/src/metrics.rscrates/machine-controller/tests/integration/bmc_rotation.rscrates/machine-controller/tests/integration/maintenance.rscrates/machine-controller/tests/integration/power_management.rscrates/redfish/src/libredfish/test_support.rscrates/test-harness/src/managed_host.rs
Treat allocation rejection for pending boot configuration as expected admission control instead of an error. Keep the status schema change atomic while removing ineffective same-transaction deferred validation. Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai I also assessed the six low-value/nitpick suggestions from the latest full-review summary so none are left implicit:
These are all useful future refactor ideas, but none changes behavior or closes a correctness gap, so I’m declining them for this already-large, fully validated PR. The actionable migration finding was fixed, acknowledged, and resolved. Thanks! |
|
Tip For best results, initiate chat on the files or code changes.
The retained implementations preserve explicit SQL locking behavior, ownership semantics, and cleanup barriers. The proposed refactors do not close a correctness, safety, or performance gap. The migration finding is addressed. I will treat the six items as non-blocking future refactor options. 🐇✅ You are interacting with an AI system. |
…4432) PR #4389 and PR #4428 independently fixed the same duplicate migration version from opposite sides. The loopback preservation migration first reached `main` in #3913 as `20260722120000`, while the BMC suppression migration came later. #4428 correctly moved the newer BMC migration to `20260722120001`, but #4389 had already renamed the older loopback migration to that same version on an earlier base. Since #4428 merged first, #4389 left `main` with two `20260722120001` migrations and removed the identity existing databases may already have in `_sqlx_migrations`. So, restore `20260722120000_preserve_machine_ipv6_loopback.sql` and leave `20260722120001_bmc_suppressions.sql` where it is. The loopback migration SQL is byte-for-byte identical to #3913, existing database histories keep matching the published version and checksum, and SQLx sees each migration version exactly once again. ## Related issues None -- urgent post-merge repair for #4389 and #4428. ## Type of Change - [ ] **Add** - New feature or capability - [ ] **Change** - Changes in existing functionality - [x] **Fix** - Bug fixes - [ ] **Remove** - Removed features or deprecated functionality - [ ] **Internal** - Internal changes (refactoring, tests, docs, etc.) ## Breaking Changes - [ ] **This PR contains breaking changes** ## Testing - [ ] Unit tests added/updated - [ ] Integration tests added/updated - [x] Manual testing performed - [ ] No testing required (docs, internal refactor, etc.) ## Additional Notes - `cargo test -p carbide-api-db migrations::tests --lib` (4 passed) - `cargo make format-nightly` - `cargo make clippy` - `cargo make carbide-lints` - `git diff --check` - Restored migration blob matches the original #3913 blob exactly (`eaff208e4ea6d3956bdb158a84c45ee6c1362ecf`) - This restores the migration identities intended by #4428. A database first initialized during either brief duplicate-version window may have recorded the wrong checksum at `20260722120000` or `20260722120001`; inspect its schema and migration history and perform site-specific repair before retrying rather than blindly replaying these non-idempotent migrations Signed-off-by: Chet Nichols III <chetn@nvidia.com>
…DIA#4389) A desired boot interface can change while an unassigned host is already `Ready`, but the machine controller did not have a durable answer to "has Redfish actually been checked for this desired version?" Treating the desired row itself as proof would make restart and race behavior ambiguous, and could let allocation win while repair is pending. So, this adds a persisted verification observation and a restart-safe `BootConfiguring` path. A `Ready` host captures the exact desired target and version, observes before mutation, reuses the shared HostInit BIOS/job/boot-order work, restores and verifies lockdown, then rereads Redfish and compare-and-swaps only the version it actually checked to verified. An ordinary host that is already correct stays observation-only. The migration gives rows whose machines are already `Ready` or `Assigned` an explicitly assumed baseline instead of scheduling fleet-wide Redfish work. In-flight lifecycle rows remain pending a real observation. This is a small machine-state-gated update with no discovery-dependent joins. MAC-only to full-pair enrichment also carries a current verification forward because adding the Redfish id still names the same physical NIC. This intentionally leaves assigned hosts pending and does not periodically look for later external drift; those are separate follow-ups. It also keeps `BootConfiguring` hosts on discovery iPXE during required reboots and keeps a pending host out of allocation until its desired version is verified. ## Related issues Closes NVIDIA#4246 ## Type of Change - [x] **Add** - New feature or capability - [ ] **Change** - Changes in existing functionality - [ ] **Fix** - Bug fixes - [ ] **Remove** - Removed features or deprecated functionality - [ ] **Internal** - Internal changes (refactoring, tests, docs, etc.) ## Breaking Changes - [ ] **This PR contains breaking changes** ## Testing - [x] Unit tests added/updated - [x] Integration tests added/updated - [ ] Manual testing performed - [ ] No testing required (docs, internal refactor, etc.) ## Additional Notes - `cargo make check-format-nightly` - `cargo make clippy` - `cargo make carbide-lints` - 17 focused API-model boot-interface tests plus the state-label regression - 6 machine-controller Ready boot-config tests - 6 PostgreSQL-backed Ready recovery tests - 12 PostgreSQL migration, versioning, compare-and-swap, and lock-order tests - End-to-end desired-change to verified-Ready progression test - iPXE reboot-state regression test - Independent state-controller, Rust-simplicity, and comment-voice audits - Cloud CodeRabbit review with all substantive threads addressed --------- Signed-off-by: Chet Nichols III <chetn@nvidia.com> Signed-off-by: Alex Ball <aball@nvidia.com>
…VIDIA#4432) PR NVIDIA#4389 and PR NVIDIA#4428 independently fixed the same duplicate migration version from opposite sides. The loopback preservation migration first reached `main` in NVIDIA#3913 as `20260722120000`, while the BMC suppression migration came later. NVIDIA#4428 correctly moved the newer BMC migration to `20260722120001`, but NVIDIA#4389 had already renamed the older loopback migration to that same version on an earlier base. Since NVIDIA#4428 merged first, NVIDIA#4389 left `main` with two `20260722120001` migrations and removed the identity existing databases may already have in `_sqlx_migrations`. So, restore `20260722120000_preserve_machine_ipv6_loopback.sql` and leave `20260722120001_bmc_suppressions.sql` where it is. The loopback migration SQL is byte-for-byte identical to NVIDIA#3913, existing database histories keep matching the published version and checksum, and SQLx sees each migration version exactly once again. ## Related issues None -- urgent post-merge repair for NVIDIA#4389 and NVIDIA#4428. ## Type of Change - [ ] **Add** - New feature or capability - [ ] **Change** - Changes in existing functionality - [x] **Fix** - Bug fixes - [ ] **Remove** - Removed features or deprecated functionality - [ ] **Internal** - Internal changes (refactoring, tests, docs, etc.) ## Breaking Changes - [ ] **This PR contains breaking changes** ## Testing - [ ] Unit tests added/updated - [ ] Integration tests added/updated - [x] Manual testing performed - [ ] No testing required (docs, internal refactor, etc.) ## Additional Notes - `cargo test -p carbide-api-db migrations::tests --lib` (4 passed) - `cargo make format-nightly` - `cargo make clippy` - `cargo make carbide-lints` - `git diff --check` - Restored migration blob matches the original NVIDIA#3913 blob exactly (`eaff208e4ea6d3956bdb158a84c45ee6c1362ecf`) - This restores the migration identities intended by NVIDIA#4428. A database first initialized during either brief duplicate-version window may have recorded the wrong checksum at `20260722120000` or `20260722120001`; inspect its schema and migration history and perform site-specific repair before retrying rather than blindly replaying these non-idempotent migrations Signed-off-by: Chet Nichols III <chetn@nvidia.com> Signed-off-by: Alex Ball <aball@nvidia.com>
2 participants
A desired boot interface can change while an unassigned host is already
Ready, but the machine controller did not have a durable answer to "has Redfish actually been checked for this desired version?" Treating the desired row itself as proof would make restart and race behavior ambiguous, and could let allocation win while repair is pending.So, this adds a persisted verification observation and a restart-safe
BootConfiguringpath. AReadyhost captures the exact desired target and version, observes before mutation, reuses the shared HostInit BIOS/job/boot-order work, restores and verifies lockdown, then rereads Redfish and compare-and-swaps only the version it actually checked to verified. An ordinary host that is already correct stays observation-only.The migration gives rows whose machines are already
ReadyorAssignedan explicitly assumed baseline instead of scheduling fleet-wide Redfish work. In-flight lifecycle rows remain pending a real observation. This is a small machine-state-gated update with no discovery-dependent joins. MAC-only to full-pair enrichment also carries a current verification forward because adding the Redfish id still names the same physical NIC.This intentionally leaves assigned hosts pending and does not periodically look for later external drift; those are separate follow-ups. It also keeps
BootConfiguringhosts on discovery iPXE during required reboots and keeps a pending host out of allocation until its desired version is verified.Related issues
Closes #4246
Type of Change
Breaking Changes
Testing
Additional Notes
cargo make check-format-nightlycargo make clippycargo make carbide-lints