fix(machine-controller): escalate WaitingForMeasurements to Failed after timeout#4027
fix(machine-controller): escalate WaitingForMeasurements to Failed after timeout#4027wminckler wants to merge 5 commits into
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. |
|
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 (1)
Summary by CodeRabbit
WalkthroughAdds a configurable timeout for hosts waiting for measurements. The controller tracks elapsed time in measuring states and returns ChangesMeasurement timeout handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant MachineStateHandler
participant handle_measuring_state
participant MeasurementMachineState
participant FailureCause
MachineStateHandler->>handle_measuring_state: pass state entry time and timeout
handle_measuring_state->>MeasurementMachineState: evaluate Discovered state
handle_measuring_state->>FailureCause: create MeasurementsNotReceived after timeout
handle_measuring_state-->>MachineStateHandler: return NoChange or Unsuccessful
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
79129dd to
b17daca
Compare
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-07-24 12:40:23 UTC | Commit: b17daca |
…ter timeout A host that stops network-booting after the Measuring state is entered will wait in WaitingForMeasurements indefinitely with no alert, no Failed transition, and nothing in the measurement journal — invisible to operators and fleet tooling. Healthy fleets complete this state in ~10 minutes. Add a configurable `waiting_for_measurements_timeout` (default 4 h) to `ReachabilityParams` and wire it through `MachineStateControllerConfig` and the builder. When the timeout elapses while `MeasurementMachineState` is still `Discovered`, `handle_measuring_state` returns `MeasuringOutcome::Unsuccessful` with the new `FailureCause::MeasurementsNotReceived`, causing all three Measuring call-sites (HostInit, Measuring, PostAssignedMeasuring) to transition to `Failed` and become visible. The `has_passed_attestation` utility call site passes `Duration::MAX` so the timeout never fires there (it checks a just-received report). Fixes: https://nvbugspro.nvidia.com/bug/6447134 Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
… to struct literals Fills in the new ReachabilityParams and MachineStateControllerConfig field in test fixtures and config-deserialization tests missed by the initial commit. Also carries the carbide-health fix (Option<String> id) forward on this branch to keep lint-police clean. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
b17daca to
5ba0062
Compare
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4027.docs.buildwithfern.com/infra-controller |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/api-core/src/cfg/file.rs (1)
948-969: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftDo not expose unbounded per-object metrics as a Prometheus configuration option.
This configuration enables O(fleet) state series and defaults the selection to every object type. Filtering by type does not bound machine/switch/rack counts, and the existing per-object metrics pattern uses object identifiers as labels. Use bounded aggregate state metrics, or redesign this as a capped diagnostic snapshot rather than an unbounded metrics endpoint.
As per coding guidelines,
**/*.{rs,proto}must keep metrics cardinality bounded and must not use highly unique labels such as machine or instance identifiers.Also applies to: 972-988, 990-1030, 4221-4228, 4584-4591
🤖 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 948 - 969, Remove the per-object metrics feature centered on PerObjectStateMetricsConfig and its per_object_state_metrics field, including the dedicated endpoint, emission logic, and related defaults or serialization paths. Do not expose machine, switch, rack, or other object identifiers as metric labels; replace the functionality with bounded aggregate state metrics, or convert it to a capped diagnostic snapshot that enforces an explicit size limit.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/api-core/src/cfg/file.rs`:
- Around line 948-969: Remove the per-object metrics feature centered on
PerObjectStateMetricsConfig and its per_object_state_metrics field, including
the dedicated endpoint, emission logic, and related defaults or serialization
paths. Do not expose machine, switch, rack, or other object identifiers as
metric labels; replace the functionality with bounded aggregate state metrics,
or convert it to a capped diagnostic snapshot that enforces an explicit size
limit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1baddcd0-812f-4ee9-bf89-efeb063c62c9
⛔ Files ignored due to path filters (1)
rest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (9)
crates/api-core/src/attestation/measured_boot.rscrates/api-core/src/cfg/file.rscrates/api-core/src/setup.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-model/src/machine/mod.rscrates/machine-controller/src/config/controller.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/lib.rscrates/machine-controller/tests/integration/env.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- crates/api-core/src/setup.rs
- crates/api-core/src/attestation/measured_boot.rs
- crates/api-model/src/machine/mod.rs
- crates/machine-controller/src/lib.rs
- crates/machine-controller/src/config/controller.rs
- crates/machine-controller/src/handler.rs
…match Rebase introduced a new metric_label() method on FailureCause that requires exhaustive matching; add the missing arm for MeasurementsNotReceived. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
744a563 to
e0467e3
Compare
CI's buf/go fmt version strips trailing alignment spaces on two lines in DNSMessage_DNSQuestion that local tooling left intact. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
… cfg/README.md Required by every_serialized_section_key_is_documented. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
2 participants
A host that stops network-booting after entering
HostInitializing/Measuring { WaitingForMeasurements }sits there indefinitely with no alert, noFailedtransition, and nothing in the measurement journal — completely invisible to operators and fleet tooling. Healthy fleets complete this state in ~10 minutes; the bug was observed holding a host for 20+ hours. The condition is reachable in practice: 1 of 16 hosts hit it within one weekend on its 30th consecutive reprovision cycle.The root cause is that
handle_measuring_statereturnsMeasuringOutcome::NoChangeunconditionally whenMeasurementMachineStateisDiscovered(Scout hasn't submitted a report yet), with no timeout, no counter, and no escalation path. This affects all three call sites that share the function:HostInit/Measuring,Measuring(post-Ready re-attestation), andPostAssignedMeasuring.This change adds a configurable
waiting_for_measurements_timeout(default 4 h) toReachabilityParamsandMachineStateControllerConfig. When the timeout elapses and the DB state is stillDiscovered,handle_measuring_statereturnsUnsuccessfulwith the newFailureCause::MeasurementsNotReceived, causing all three call sites to transition toFailedand become visible to operators and remediation tooling.The
has_passed_attestationutility call site (which checks a just-received report, not a stuck state) passesDuration::MAXso the timeout never fires there.Related issues
Fixes https://nvbugspro.nvidia.com/bug/6447134
Type of Change
Breaking Changes
Testing
Additional Notes
The 4-hour default is intentionally generous — it is 24× the healthy completion time — so it catches stuck hosts without risk of false positives on slow but healthy fleets. Sites can tune it via
machine_state_controller.waiting_for_measurements_timeoutin config.