feat: detect verified boot configuration drift - #4472
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
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 (10)
🚧 Files skipped from review as they are similar to previous changes (9)
Summary by CodeRabbit
WalkthroughThe change adds a positive boot-interface observation interval, periodic read-only Redfish checks, claim-validated drift reopening, and machine-state coverage for refresh, deferral, convergence, assigned hosts, and locked Supermicro systems. ChangesBoot-interface observation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MachineStateHandler
participant BootInterfaceObservation
participant Redfish
participant machine_desired_boot_interface
MachineStateHandler->>BootInterfaceObservation: run verified observation
BootInterfaceObservation->>Redfish: read boot order
Redfish-->>BootInterfaceObservation: observed boot interface
BootInterfaceObservation->>machine_desired_boot_interface: record observation or reopen drift
machine_desired_boot_interface-->>MachineStateHandler: return state-machine outcome
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai full_review, thanks! |
|
✅ 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
🧹 Nitpick comments (3)
crates/machine-controller/src/handler/boot_interface_observation.rs (1)
95-100: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider returning the desired version from the schedule guard to remove the
expect.The
expectat line 99 is provably unreachable today.should_check_scheduleat line 82 already rejects aNonedesired target, andmachineis not mutated in between. However, the invariant is spread over two independent reads of the sameOption, so a later change toshould_check_schedulecould turn this into a panic on a controller sweep.Returning the version from the guard makes the invariant type-enforced instead of comment-enforced.
♻️ Proposed refactor to remove the panic path
-/// Returns whether the snapshot needs a database scheduling or claim check. -fn should_check_schedule(machine: &Machine, now: DateTime<Utc>) -> bool { +/// Returns the due desired generation that needs a database scheduling or +/// claim check, if any. +fn due_desired_version(machine: &Machine, now: DateTime<Utc>) -> Option<ConfigVersion> { let Some(desired) = machine.config.desired_boot_interface.as_ref() else { - return false; + return None; }; let Some(verified) = machine.status.boot_interface_status_observation.as_ref() else { - return false; + return None; }; - desired.version == verified.config_version + let due = desired.version == verified.config_version && machine .status .boot_interface_next_observation_at - .is_none_or(|next_observation_at| next_observation_at <= now) + .is_none_or(|next_observation_at| next_observation_at <= now); + due.then_some(desired.version) }Then at the call site:
let machine = &mh_snapshot.host_snapshot; - if !should_check_schedule(machine, Utc::now()) { - return Ok(None); - } + let Some(desired_version) = due_desired_version(machine, Utc::now()) else { + return Ok(None); + }; @@ - let desired_version = machine - .config - .desired_boot_interface - .as_ref() - .expect("schedule check requires a desired boot interface") - .version; let config = &ctx.services.site_config.machine_state_controller;The Supermicro guard at lines 91-93 stays between the two, and the unit test at line 234 adapts to assert on
Optionpresence.🤖 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/boot_interface_observation.rs` around lines 95 - 100, Update should_check_schedule to return the desired boot interface version when a schedule check is required, rather than only indicating eligibility. At its caller, preserve the Supermicro guard, use the returned version directly, and remove the independent desired_boot_interface lookup and expect panic path. Adapt the unit test to verify the returned Option and version.crates/api-core/src/cfg/file.rs (1)
4059-4074: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExpress the rejection cases as a table-driven test.
The manual
forloop covers the four invalid inputs correctly. However, this module already importsCheckandcheck_values(line 3559) and neighbouring tests use them. A table gives each case a scenario label, so a failure names the offending field and value directly.♻️ Proposed table-driven rewrite
- /// Observation cadence must always schedule work strictly in the future. - #[test] - fn reject_nonpositive_boot_interface_observation_intervals() { - for (field, value) in [ - ("boot_interface_observation_interval", "0s"), - ("boot_interface_observation_interval", "-1s"), - ("boot_interface_observation_retry_interval", "0s"), - ("boot_interface_observation_retry_interval", "-1s"), - ] { - let config = format!(r#"{{"{field}": "{value}"}}"#); - assert!( - serde_json::from_str::<MachineStateControllerConfig>(&config).is_err(), - "{field}={value} must be rejected", - ); - } - } + /// Observation cadence must always schedule work strictly in the future. + #[test] + fn reject_nonpositive_boot_interface_observation_intervals() { + check_values( + [ + Check { + scenario: "zero observation interval", + input: ("boot_interface_observation_interval", "0s"), + expect: true, + }, + Check { + scenario: "negative observation interval", + input: ("boot_interface_observation_interval", "-1s"), + expect: true, + }, + Check { + scenario: "zero retry interval", + input: ("boot_interface_observation_retry_interval", "0s"), + expect: true, + }, + Check { + scenario: "negative retry interval", + input: ("boot_interface_observation_retry_interval", "-1s"), + expect: true, + }, + ], + |(field, value)| { + let config = format!(r#"{{"{field}": "{value}"}}"#); + serde_json::from_str::<MachineStateControllerConfig>(&config).is_err() + }, + ); + }As per coding guidelines: "Use table-driven tests for functions mapping inputs to outputs, errors, or observable results. Use scenarios! with Outcome for Result-returning operations, value_scenarios! for total operations, and direct check_cases/check_values when macros obscure the table."
🤖 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/cfg/file.rs` around lines 4059 - 4074, Rewrite reject_nonpositive_boot_interface_observation_intervals as a table-driven test using the module’s existing Check/check_values testing helpers, with a named scenario for each invalid field/value combination. Preserve the current assertion that every zero or negative observation interval causes MachineStateControllerConfig deserialization to fail, while ensuring failure output identifies the specific field and value.Source: Coding guidelines
crates/api-db/src/machine_desired_boot_interface.rs (1)
758-818: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider folding the claim status read into
load_for_update.
reopen_after_observed_driftlocks the row twice: once throughload_for_updateand again through theSELECT verified_version, next_observation_at ... FOR UPDATE. The second statement exists only because theDesiredBootInterfaceRowprojection omits the status columns. Extending that projection (or adding a dedicatedload_status_for_update) would remove one round trip and keep the claim comparison next to the target comparison. The current behavior is correct, so treat this as cleanup rather than a defect.🤖 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 758 - 818, Refactor reopen_after_observed_drift to obtain verified_version and next_observation_at through load_for_update, or a dedicated load_status_for_update helper, instead of issuing the second SELECT ... FOR UPDATE. Preserve the existing claim-status comparison and locking semantics while eliminating the redundant round trip.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/api-core/src/tests/machine_states.rs`:
- Around line 3709-3714: Update the assertion in the deferred-read test around
host.status.boot_interface_next_observation_at to compare the persisted deadline
against the backdated baseline value rather than Utc::now(). Preserve the
existing Some/deadline validation while asserting that the deadline advanced
past the original backdated timestamp, making the test independent of wall-clock
timing.
---
Nitpick comments:
In `@crates/api-core/src/cfg/file.rs`:
- Around line 4059-4074: Rewrite
reject_nonpositive_boot_interface_observation_intervals as a table-driven test
using the module’s existing Check/check_values testing helpers, with a named
scenario for each invalid field/value combination. Preserve the current
assertion that every zero or negative observation interval causes
MachineStateControllerConfig deserialization to fail, while ensuring failure
output identifies the specific field and value.
In `@crates/api-db/src/machine_desired_boot_interface.rs`:
- Around line 758-818: Refactor reopen_after_observed_drift to obtain
verified_version and next_observation_at through load_for_update, or a dedicated
load_status_for_update helper, instead of issuing the second SELECT ... FOR
UPDATE. Preserve the existing claim-status comparison and locking semantics
while eliminating the redundant round trip.
In `@crates/machine-controller/src/handler/boot_interface_observation.rs`:
- Around line 95-100: Update should_check_schedule to return the desired boot
interface version when a schedule check is required, rather than only indicating
eligibility. At its caller, preserve the Supermicro guard, use the returned
version directly, and remove the independent desired_boot_interface lookup and
expect panic path. Adapt the unit test to verify the returned Option and
version.
🪄 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: 06e31a8c-2e35-4a85-8531-39c99ffdaf4f
📒 Files selected for processing (16)
crates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/cfg/test_data/full_config.tomlcrates/api-core/src/cfg/test_data/full_config_post_migration.tomlcrates/api-core/src/cfg/test_data/site_config.tomlcrates/api-core/src/tests/machine_states.rscrates/api-db/migrations/20260731161510_machine_boot_interface_observation_schedule.sqlcrates/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/status.rscrates/api-model/src/test_support/machine_snapshot.rscrates/machine-controller/src/config/controller.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/handler/boot_interface_observation.rs
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/api-core/src/tests/machine_states.rs (1)
3709-3714: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMake this retry-deadline assertion independent of wall-clock timing.
The claimed deadline equals
CURRENT_TIMESTAMP + boot_interface_observation_retry_interval, andMachineStateControllerConfig::test_default()sets that retry interval to one second. This assertion passes only if the controller iteration, the commit, and this follow-up read complete within one second of the claim. A loaded CI runner can exceed that budget, which makes the test flaky.Assert that the deadline advanced past the backdated baseline value instead of comparing it against
Utc::now().💚 Suggested timing-independent assertion
+ let deadline_before = before + .host_snapshot + .status + .boot_interface_next_observation_at + .expect("the fixture deadline should be backdated and due");assert!( host.status .boot_interface_next_observation_at - .is_some_and(|deadline| deadline > Utc::now()), - "the deferred read should retain the persisted retry deadline" + .is_some_and(|deadline| deadline > deadline_before), + "the deferred read should persist a fresh retry deadline" );🤖 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 3709 - 3714, Update the retry-deadline assertion in the machine state test to compare boot_interface_next_observation_at against the backdated baseline timestamp used before claiming, asserting it advanced beyond that value rather than comparing with Utc::now(). Preserve the existing validation that the deadline is present and the deferred read retains the persisted retry deadline.
🧹 Nitpick comments (4)
crates/api-core/src/cfg/file.rs (1)
4059-4075: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
check_valuesfor this table-style rejection test.This test loops over four input pairs and asserts a boolean outcome for each. This file already uses
check_valuesfor the same pattern, for example ininsecure_discovery_configuration_is_opt_in. Convert this test tocheck_valuesfor a namedscenarioper case and for consistency with the rest of the file.♻️ Proposed refactor to `check_values`
- /// Observation cadence must always schedule work strictly in the future. - #[test] - fn reject_nonpositive_boot_interface_observation_intervals() { - for (field, value) in [ - ("boot_interface_observation_interval", "0s"), - ("boot_interface_observation_interval", "-1s"), - ("boot_interface_observation_retry_interval", "0s"), - ("boot_interface_observation_retry_interval", "-1s"), - ] { - let config = format!(r#"{{"{field}": "{value}"}}"#); - assert!( - serde_json::from_str::<MachineStateControllerConfig>(&config).is_err(), - "{field}={value} must be rejected", - ); - } - } + /// Observation cadence must always schedule work strictly in the future. + #[test] + fn reject_nonpositive_boot_interface_observation_intervals() { + check_values( + [ + Check { + scenario: "zero observation interval", + input: ("boot_interface_observation_interval", "0s"), + expect: true, + }, + Check { + scenario: "negative observation interval", + input: ("boot_interface_observation_interval", "-1s"), + expect: true, + }, + Check { + scenario: "zero retry interval", + input: ("boot_interface_observation_retry_interval", "0s"), + expect: true, + }, + Check { + scenario: "negative retry interval", + input: ("boot_interface_observation_retry_interval", "-1s"), + expect: true, + }, + ], + |(field, value)| { + let config = format!(r#"{{"{field}": "{value}"}}"#); + serde_json::from_str::<MachineStateControllerConfig>(&config).is_err() + }, + ); + }As per coding guidelines, "Use table-driven tests for functions mapping inputs to outputs, errors, or observable results... direct check_cases/check_values when macros obscure the table."
🤖 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/cfg/file.rs` around lines 4059 - 4075, Refactor reject_nonpositive_boot_interface_observation_intervals to use the existing check_values helper, defining a named scenario for each interval field/value pair and its expected rejection outcome. Match the established usage in insecure_discovery_configuration_is_opt_in, preserving the current assertion that all zero and negative intervals are rejected.Source: Coding guidelines
crates/api-db/src/machine_desired_boot_interface.rs (1)
782-795: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider folding the claim status read into
load_for_update.
load_for_updatealready locks and projects the row at Line 773. The additionalSELECT ... FOR UPDATEre-reads the same row for two columns. Ifload_for_updatealso projectedverified_versionandnext_observation_at, this second round trip would disappear and the claim check would stay in one place. This is optional; the current form is correct.🤖 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 782 - 795, Optionally extend load_for_update to project verified_version and next_observation_at, then reuse those returned values for the claim status comparison instead of issuing the separate claim_query SELECT ... FOR UPDATE. Preserve the existing mismatch behavior that returns Ok(None).crates/machine-controller/src/handler/boot_interface_observation.rs (2)
232-290: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConvert both unit tests to the repository's table-driven helpers.
should_check_scheduleanddpu_observation_is_currentare total functions that map inputs to abool. The repository guideline requires table-driven coverage for this shape.value_scenarios!orcheck_valueswould express the deadline variants (None, past, future, superseded version) and the freshness boundary as an explicit table, and would report each failing case individually instead of stopping at the first assertion.Based on the coding guideline "Use table-driven tests for functions mapping inputs to outputs, errors, or observable results. Use
scenarios!withOutcomeforResult-returning operations,value_scenarios!for total operations, and directcheck_cases/check_valueswhen macros obscure the table."🤖 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/boot_interface_observation.rs` around lines 232 - 290, Convert schedule_check_only_runs_for_due_verified_intent and managed_dpu_observation_must_also_be_recent to table-driven tests using the repository’s value_scenarios! or check_values helpers. Represent the None, past, future, and superseded-version cases for should_check_schedule, plus the freshness boundary cases for dpu_observation_is_current, as explicit independent scenarios with expected boolean results.Source: Coding guidelines
82-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn the desired version from the schedule check to remove the
expect.
should_check_schedulealready proves thatdesired_boot_interfaceisSome. Theexpectat Line 99 re-derives that fact and leaves a panic that depends on an invariant held in another function. Returning the version makes the invariant explicit and hard to misuse.♻️ Proposed refactor
-/// Returns whether the snapshot needs a database scheduling or claim check. -fn should_check_schedule(machine: &Machine, now: DateTime<Utc>) -> bool { +/// Returns the desired version that needs a database scheduling or claim +/// check, or `None` when the snapshot is not eligible. +fn schedule_check_version(machine: &Machine, now: DateTime<Utc>) -> Option<ConfigVersion> { let Some(desired) = machine.config.desired_boot_interface.as_ref() else { - return false; + return None; }; let Some(verified) = machine.status.boot_interface_status_observation.as_ref() else { - return false; + return None; }; - desired.version == verified.config_version + (desired.version == verified.config_version && machine .status .boot_interface_next_observation_at - .is_none_or(|next_observation_at| next_observation_at <= now) + .is_none_or(|next_observation_at| next_observation_at <= now)) + .then_some(desired.version) }let machine = &mh_snapshot.host_snapshot; - if !should_check_schedule(machine, Utc::now()) { - return Ok(None); - } + let Some(desired_version) = schedule_check_version(machine, Utc::now()) else { + return Ok(None); + }; @@ - let desired_version = machine - .config - .desired_boot_interface - .as_ref() - .expect("schedule check requires a desired boot interface") - .version; let config = &ctx.services.site_config.machine_state_controller;🤖 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/boot_interface_observation.rs` around lines 82 - 100, Update should_check_schedule to return the desired boot-interface version when a schedule check is required, and adjust its callers to use that returned value. In the boot observation flow, replace the config lookup and expect call with the version from should_check_schedule while preserving the existing early-return behavior.
🤖 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.
Duplicate comments:
In `@crates/api-core/src/tests/machine_states.rs`:
- Around line 3709-3714: Update the retry-deadline assertion in the machine
state test to compare boot_interface_next_observation_at against the backdated
baseline timestamp used before claiming, asserting it advanced beyond that value
rather than comparing with Utc::now(). Preserve the existing validation that the
deadline is present and the deferred read retains the persisted retry deadline.
---
Nitpick comments:
In `@crates/api-core/src/cfg/file.rs`:
- Around line 4059-4075: Refactor
reject_nonpositive_boot_interface_observation_intervals to use the existing
check_values helper, defining a named scenario for each interval field/value
pair and its expected rejection outcome. Match the established usage in
insecure_discovery_configuration_is_opt_in, preserving the current assertion
that all zero and negative intervals are rejected.
In `@crates/api-db/src/machine_desired_boot_interface.rs`:
- Around line 782-795: Optionally extend load_for_update to project
verified_version and next_observation_at, then reuse those returned values for
the claim status comparison instead of issuing the separate claim_query SELECT
... FOR UPDATE. Preserve the existing mismatch behavior that returns Ok(None).
In `@crates/machine-controller/src/handler/boot_interface_observation.rs`:
- Around line 232-290: Convert schedule_check_only_runs_for_due_verified_intent
and managed_dpu_observation_must_also_be_recent to table-driven tests using the
repository’s value_scenarios! or check_values helpers. Represent the None, past,
future, and superseded-version cases for should_check_schedule, plus the
freshness boundary cases for dpu_observation_is_current, as explicit independent
scenarios with expected boolean results.
- Around line 82-100: Update should_check_schedule to return the desired
boot-interface version when a schedule check is required, and adjust its callers
to use that returned value. In the boot observation flow, replace the config
lookup and expect call with the version from should_check_schedule while
preserving the existing early-return behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: dea2a8ec-fd9e-4173-9546-ad3cda0afe2a
📒 Files selected for processing (16)
crates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/cfg/test_data/full_config.tomlcrates/api-core/src/cfg/test_data/full_config_post_migration.tomlcrates/api-core/src/cfg/test_data/site_config.tomlcrates/api-core/src/tests/machine_states.rscrates/api-db/migrations/20260731161510_machine_boot_interface_observation_schedule.sqlcrates/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/status.rscrates/api-model/src/test_support/machine_snapshot.rscrates/machine-controller/src/config/controller.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/handler/boot_interface_observation.rs
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4472.docs.buildwithfern.com/infra-controller |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/machine-controller/src/handler/boot_interface_observation.rs (3)
114-145: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a
carbide_instrument::Eventfor observation outcomes.This observer runs on every host on a fixed cadence. Redfish failures and detected drift are currently visible only in logs. A counter makes the fleet-wide drift rate and the BMC failure rate alertable, which is the main operational value of periodic observation.
If you add an event, keep
machine_idanderroras#[context]fields, not labels, and put only the bounded outcome andoperationin labels.Based on the coding guideline "declare and emit a
carbide_instrument::Eventwhen an event needs a count, rate, or duration" and "use#[context]rather than#[label]for high-cardinality values such as machine IDs, IPs, and error text".🤖 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/boot_interface_observation.rs` around lines 114 - 145, Add and emit a carbide_instrument::Event for each boot observation outcome, including Redfish client creation failures and host boot configuration inspection results or drift. Use bounded outcome and operation values as #[label] fields, while keeping machine_id and error as #[context] fields; emit the event at the corresponding success or failure points in the observation flow.Source: Coding guidelines
79-101: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider returning the due target from
should_observeto remove theexpect.The
expectat Line 100 relies on an invariant thatshould_observeestablished at Line 38. The invariant holds today, but the panic is only safe as long as the two functions stay in agreement. A future edit toshould_observecould make this path panic in production.If
should_observereturnsOption<&Versioned<MachineBootInterfaceTarget>>, the panic and the duplicate lookup both disappear.♻️ Proposed refactor
-/// Returns whether an already-verified target is due for another observation. -fn should_observe(machine: &Machine, now: DateTime<Utc>, observation_interval: Duration) -> bool { +/// Returns the already-verified target when it is due for another observation. +fn due_target( + machine: &Machine, + now: DateTime<Utc>, + observation_interval: Duration, +) -> Option<&Versioned<MachineBootInterfaceTarget>> { let Some(desired) = machine.config.desired_boot_interface.as_ref() else { - return false; + return None; }; let Some(verified) = machine.status.boot_interface_status_observation.as_ref() else { - return false; + return None; }; - desired.version == verified.config_version + (desired.version == verified.config_version && now.signed_duration_since(verified.observed_at) >= observation_interval + ) + .then_some(desired) }Then at the call site:
- if !should_observe( - machine, - Utc::now(), - config.boot_interface_observation_interval, - ) { + let Some(desired) = due_target( + machine, + Utc::now(), + config.boot_interface_observation_interval, + ) else { return Ok(None); - } + }; + let desired = desired.clone();The block at Lines 96-101 is then removed.
🤖 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/boot_interface_observation.rs` around lines 79 - 101, Update should_observe to return the due Versioned<MachineBootInterfaceTarget> target as an Option instead of only a boolean. In the observation flow, consume that returned target for the early-return check and continue using it as desired, removing the duplicate desired_boot_interface lookup and its expect while preserving the existing eligibility and Supermicro lockdown behavior.
217-256: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConvert
should_observecoverage to a table and add the two early-return cases.
should_observeis a total function that maps(machine, now, interval)tobool. The current test mutates one sharedmachineacross three assertions, so a failure in the first assertion cascades into the rest, and the exercised precondition is not visible at the failure site.Two branches are also untested: the
Nonedesired target at Line 38 and theNonestatus observation at Line 41. The first branch is the invariant that theexpectat Line 100 depends on, so it deserves explicit coverage.Use
value_scenarios!with one row per variant: due, not yet due, version mismatch, no desired target, no status observation. Apply the same treatment tomanaged_dpu_observation_must_also_be_recent.As per path instructions: "Prefer table-driven tests, using
scenarios!,value_scenarios!,check_cases, orcheck_valuesto cover input variants."🤖 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/boot_interface_observation.rs` around lines 217 - 256, Convert the `observation_only_runs_for_due_verified_intent` test to a `value_scenarios!` table with independent rows for due, not-yet-due, version mismatch, no desired target, and no status observation, asserting each expected `should_observe` result. Apply the same table-driven structure to `managed_dpu_observation_must_also_be_recent`, covering its relevant input variants and expected outcomes.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/machine-controller/src/handler/boot_interface_observation.rs`:
- Around line 114-145: Add and emit a carbide_instrument::Event for each boot
observation outcome, including Redfish client creation failures and host boot
configuration inspection results or drift. Use bounded outcome and operation
values as #[label] fields, while keeping machine_id and error as #[context]
fields; emit the event at the corresponding success or failure points in the
observation flow.
- Around line 79-101: Update should_observe to return the due
Versioned<MachineBootInterfaceTarget> target as an Option instead of only a
boolean. In the observation flow, consume that returned target for the
early-return check and continue using it as desired, removing the duplicate
desired_boot_interface lookup and its expect while preserving the existing
eligibility and Supermicro lockdown behavior.
- Around line 217-256: Convert the
`observation_only_runs_for_due_verified_intent` test to a `value_scenarios!`
table with independent rows for due, not-yet-due, version mismatch, no desired
target, and no status observation, asserting each expected `should_observe`
result. Apply the same table-driven structure to
`managed_dpu_observation_must_also_be_recent`, covering its relevant input
variants and expected outcomes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 86372a17-18a2-462d-98c8-ec56a279c571
📒 Files selected for processing (10)
crates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/cfg/test_data/full_config.tomlcrates/api-core/src/cfg/test_data/full_config_post_migration.tomlcrates/api-core/src/cfg/test_data/site_config.tomlcrates/api-core/src/tests/machine_states.rscrates/api-db/src/machine_desired_boot_interface.rscrates/machine-controller/src/config/controller.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/handler/boot_interface_observation.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- crates/api-core/src/cfg/test_data/full_config_post_migration.toml
- crates/api-core/src/cfg/test_data/full_config.toml
- crates/api-core/src/cfg/file.rs
- crates/machine-controller/src/handler.rs
- crates/api-core/src/cfg/README.md
- crates/api-core/src/tests/machine_states.rs
- crates/api-core/src/cfg/test_data/site_config.toml
|
Review follow-up for eb6ac57:
The older deadline, claim, retry, and migration comments are obsolete after the simplification: this revision removes that entire persistence layer and derives cadence from the existing successful observation timestamp. |
Reinspect verified host boot targets after a configurable interval derived from the last successful observation. Reopen the exact desired target on mismatch so Ready reuses existing boot convergence while Assigned defers remediation until release. Keep observation read-only and fence result writes by the desired configuration generation. This supports NVIDIA#4248 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
3 participants
An already-verified desired boot interface was not observed again, so later Redfish drift could remain invisible. This adds a paced, restart-safe observer that reuses the existing boot-configuration state machine instead of creating a second remediation path.
The observer lazily schedules verified rows across the normal interval, claims due work by persisting its retry deadline before Redfish I/O, and compares the exact stored
PairorMacOnlytarget. A correct read refreshes the observation. A mismatch atomically reopens the same target as pending intent: Ready enters the existingBootConfiguringflow on its next sweep, while Assigned records the drift without disruption until release.The schema migration is a nullable, data-free column addition with no backfill. Managed-DPU hosts wait for reports that are both newer than the current host state and inside the configured DPU health window. Locked Supermicro hosts are excluded because their boot-order view is not trustworthy without a disruptive unlock and reboot.
Related issues
This supports #4248
Type of Change
Breaking Changes
Testing
Validation completed on base
12bd2cc2c:Additional Notes
Defaults are 24 hours between successful observations and 15 minutes after an inconclusive attempt. Both intervals are configurable and must be positive. The persisted retry deadline preserves pacing and last-known-good verification across BMC failures and controller restarts.