feat: make the stuck-machine mechanisms alertable#3272
Conversation
|
@coderabbitai full_review, thanks! |
|
(=^･ω･^=) ✅ Action performedFull review finished. |
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR adds shared wakeup-failure instrumentation, host firmware retry-budget tracking, machine-controller retry metrics, and an errored-task counter. Tests and observability docs were updated to match the new emitted labels and metric names. ChangesWakeup, retry, and metrics instrumentation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant dpu as dpu.rs
participant host_reprovisioning as host_reprovisioning.rs
participant machine_scout as machine_scout.rs
participant queued_objects as queued objects
participant carbide_instrument as carbide_instrument
dpu->>queued_objects: enqueue wakeup by DPU id
queued_objects-->>dpu: Err(err)
dpu->>carbide_instrument: emit StateHandlerWakeupFailed
host_reprovisioning->>queued_objects: enqueue wakeup for scout firmware status
queued_objects-->>host_reprovisioning: Err(err)
host_reprovisioning->>carbide_instrument: emit StateHandlerWakeupFailed
machine_scout->>carbide_instrument: emit StateHandlerWakeupFailed with trigger
sequenceDiagram
participant handler as handler.rs
participant ManagedHostState as ManagedHostState
participant carbide_instrument as carbide_instrument
participant carbide_host_reprovision_retries_total as carbide_host_reprovision_retries_total
handler->>ManagedHostState: evaluate FailedFirmwareUpgrade
alt retry allowed and interval elapsed
handler->>carbide_instrument: emit HostFirmwareUpgradeRetried
else retry budget exhausted
handler->>handler: do_nothing()
end
carbide_instrument->>carbide_host_reprovision_retries_total: increment counter
Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/api-core/src/handlers/utils.rs (1)
107-112: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate log-field lookup helper.
This inline
fieldclosure duplicates the equivalent helper already defined incrates/instrument/tests/matrix.rs(fn field<'a>(log: &'a CapturedLog, name: &str) -> Option<&'a str>). Consider promoting a shared helper intocarbide-test-supportso both test suites reuse one implementation instead of re-deriving it per crate.♻️ Illustrative dedupe (would need a shared test-support helper)
-use carbide_instrument::testing::{MetricsCapture, capture_logs}; +use carbide_instrument::testing::{MetricsCapture, capture_logs}; +use carbide_test_support::field; // hypothetical shared helper ... - let field = |log: &carbide_instrument::testing::CapturedLog, name: &str| { - log.fields - .iter() - .find(|(key, _)| key == name) - .map(|(_, value)| value.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/api-core/src/handlers/utils.rs` around lines 107 - 112, The inline field lookup closure in the test utility duplicates the same log-field helper already used elsewhere, so replace it with a shared helper from a common test-support location instead of keeping a local reimplementation. Move the reusable logic into carbide-test-support (or the shared equivalent) and update the code in utils.rs and the existing field helper in matrix.rs to call that shared function, keeping the CapturedLog/name lookup behavior consistent across both test suites.crates/machine-controller/src/handler.rs (2)
11953-12007: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftTest only covers the emit plumbing, not the retry-decision logic itself.
This test directly constructs and emits
HostReprovisionRetryDecided, verifying the log/metric wiring is correct — but it never exerciseshandle_host_reprovision'sFailedFirmwareUpgradearm (Lines 8355-8392), i.e., thecan_retry/waited_enoughbranching that decides which variant actually gets emitted in production. A regression in that branching (e.g., an off-by-one onMAX_FIRMWARE_UPGRADE_RETRIES, or a badwaited_enoughcomputation) would not be caught here even though it's the exact behavior this PR aims to make alertable.Separately, this test has two labeled cases (
Retried/Exhausted) each with several expected fields (log level, message, counter delta) — a good match forcheck_cases/check_valuesper the repo's table-driven testing convention.🤖 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 11953 - 12007, The current test only validates `HostReprovisionRetryDecided` emission plumbing and misses the actual retry-decision branch logic. Refactor this test in `handler.rs` to exercise `handle_host_reprovision`’s `FailedFirmwareUpgrade` path, specifically the `can_retry`/`waited_enough` branching, so a regression in `MAX_FIRMWARE_UPGRADE_RETRIES` handling or wait calculation is covered. Use the existing `check_cases`/`check_values` style to table-drive the `Retried` and `Exhausted` expectations while keeping assertions on the emitted log level, message, and counter deltas.Source: Coding guidelines
8364-8391: 🚀 Performance & Scalability | 🔵 TrivialConsider suppressing repeated WARN logs for the exhausted state.
The doc comment on
HostReprovisionRetryDecided(lines 165-168) makes clear theExhausteddecision is expected to fire on every handler pass while a machine is stuck. Since the event'slog = warn, this means a continuous stream of duplicate WARN log lines for as long as the machine stays stuck — potentially indefinitely, until an operator intervenes. The counter increment is the intended alerting signal per the design comment; consider whether the exhausted path specifically warrantslog = off(or a debounced/throttled log) to avoid drowning out other WARN-level signals while still keeping the metric intact for alerting.🤖 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 8364 - 8391, The exhausted retry path in the reprovision handler is emitting a WARN-level event on every pass, which can create repeated duplicate logs while the machine remains stuck. Update the `HostReprovisionRetryDecided` usage in `handler.rs` so the `Exhausted` case does not log at WARN repeatedly, while keeping the metric/event emission intact for alerting. A good fix is to change the exhausted event’s log behavior to `off` or apply throttling/debouncing specifically in the `can_retry` false branch. Keep the `Retried` path unchanged and target the `HostReprovisionRetryDecided` emission sites in the retry decision logic.
🤖 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/handlers/utils.rs`:
- Around line 107-112: The inline field lookup closure in the test utility
duplicates the same log-field helper already used elsewhere, so replace it with
a shared helper from a common test-support location instead of keeping a local
reimplementation. Move the reusable logic into carbide-test-support (or the
shared equivalent) and update the code in utils.rs and the existing field helper
in matrix.rs to call that shared function, keeping the CapturedLog/name lookup
behavior consistent across both test suites.
In `@crates/machine-controller/src/handler.rs`:
- Around line 11953-12007: The current test only validates
`HostReprovisionRetryDecided` emission plumbing and misses the actual
retry-decision branch logic. Refactor this test in `handler.rs` to exercise
`handle_host_reprovision`’s `FailedFirmwareUpgrade` path, specifically the
`can_retry`/`waited_enough` branching, so a regression in
`MAX_FIRMWARE_UPGRADE_RETRIES` handling or wait calculation is covered. Use the
existing `check_cases`/`check_values` style to table-drive the `Retried` and
`Exhausted` expectations while keeping assertions on the emitted log level,
message, and counter deltas.
- Around line 8364-8391: The exhausted retry path in the reprovision handler is
emitting a WARN-level event on every pass, which can create repeated duplicate
logs while the machine remains stuck. Update the `HostReprovisionRetryDecided`
usage in `handler.rs` so the `Exhausted` case does not log at WARN repeatedly,
while keeping the metric/event emission intact for alerting. A good fix is to
change the exhausted event’s log behavior to `off` or apply
throttling/debouncing specifically in the `can_retry` false branch. Keep the
`Retried` path unchanged and target the `HostReprovisionRetryDecided` emission
sites in the retry decision logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8c0b68aa-6215-428c-a9f6-ebe285701640
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
crates/api-core/src/handlers/dpu.rscrates/api-core/src/handlers/host_reprovisioning.rscrates/api-core/src/handlers/machine_scout.rscrates/api-core/src/handlers/utils.rscrates/api-core/src/tests/machine_network.rscrates/api-core/tests/integration/scout_firmware_upgrade_status.rscrates/machine-controller/Cargo.tomlcrates/machine-controller/src/handler.rscrates/state-controller/src/controller/processor.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/state-controller/src/controller/processor.rs (1)
564-570: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a focused test for the new counter increment.
No test in this diff exercises
errored_tasks_counterincrementing whentask_result.metrics.common.error.is_some(). Given the PR's stated goal of making failure modes "alertable," a small unit test asserting the counter delta (similar to howcompleted_tasks_counter/dispatched_tasks_counterbehavior is presumably tested) would guard against 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/state-controller/src/controller/processor.rs` around lines 564 - 570, Add a focused unit test for Processor::maybe_log_stats (or the closest task-finalization path in processor.rs) that covers task_result.metrics.common.error.is_some() and asserts metric_emitter.errored_tasks_counter is incremented by 1. Reuse the existing test style for completed_tasks_counter/dispatched_tasks_counter, and make sure the test drives the same Processor state so the errored_tasks_counter delta is observed through self.metric_emitter.
🤖 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/state-controller/src/controller/processor.rs`:
- Around line 564-570: Add a focused unit test for Processor::maybe_log_stats
(or the closest task-finalization path in processor.rs) that covers
task_result.metrics.common.error.is_some() and asserts
metric_emitter.errored_tasks_counter is incremented by 1. Reuse the existing
test style for completed_tasks_counter/dispatched_tasks_counter, and make sure
the test drives the same Processor state so the errored_tasks_counter delta is
observed through self.metric_emitter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7e581a71-0fff-49ee-a18b-9f63f429ad5a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
crates/api-core/src/handlers/dpu.rscrates/api-core/src/handlers/host_reprovisioning.rscrates/api-core/src/handlers/machine_scout.rscrates/api-core/src/handlers/utils.rscrates/api-core/src/tests/machine_network.rscrates/api-core/tests/integration/scout_firmware_upgrade_status.rscrates/machine-controller/Cargo.tomlcrates/machine-controller/src/handler.rscrates/state-controller/src/controller/processor.rs
|
@coderabbitai full_review, thanks! (CI catalogue check: regenerated core_metrics.md with the row the integration run now exposes.) |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
fd -i core_metrics
echo "---"
fd -i core_metrics -x cat {}Length of output: 26699 🏁 Script executed: #!/bin/bash
cd $(fd -t d '^\.git$' -d 1 . | head -1 | xargs dirname) 2>/dev/null || true
git log --oneline -1 -- docs/observability/core_metrics.md
git diff HEAD~5 -- docs/observability/core_metrics.md 2>/dev/null | head -100
echo "---diff against main---"
git fetch origin main --depth=50 2>&1 | tail -5
git diff origin/main -- docs/observability/core_metrics.mdLength of output: 1147 🏁 Script executed: #!/bin/bash
git rev-parse --show-toplevel
git status --short | head
git remote -v
git fetch origin main --depth=50 2>&1 | tail -5
git diff origin/main -- docs/observability/core_metrics.mdLength of output: 3489
Verified as expected:
One thing to double-check: the diff also shows Full review otherwise stands from the earlier pass — nothing else flagged. ✅ Action performedReview finished.
|
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3272.docs.buildwithfern.com/infra-controller |
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
|
@coderabbitai full_review, thanks! (Redesign per review consensus: exhaustion is state, not occurrences -- the per-pass exhausted emit is gone, replaced by the carbide_exhausted_reprovision_retry_count gauge beside the other firmware-update gauges; the retry counter counts only real retries at the site's historical info level; the DPU wake-up path counts every failure class and its host lookup reads through db_reader() without the rollback-only transaction. Metric descriptions say "Number of" per the docs review.) |
|
(•ᴗ•)⟢ ✏️ Learnings added
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/machine-controller/src/handler.rs (1)
8340-8354: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winEmit the retry metric after the transition is committed.
This
emit()runs before the outcome is durably persisted, so a failed commit or replay of the same handler pass can count a single logical retry more than once. Moving it to the committed side keeps the counter aligned with persisted retries.🤖 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 8340 - 8354, The HostFirmwareUpgradeRetried metric is emitted too early in the retry path, before the state transition is durably committed. Move the carbide_instrument::emit(HostFirmwareUpgradeRetried) call in handler.rs so it runs only after the retry decision has been successfully persisted, keeping the metric aligned with committed retries and preventing duplicate counts on failed commits or replayed handler passes.
🤖 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 1337-1347: `host_repro_retries_exhausted()` only checks
`ManagedHostState::HostReprovision` and misses `ManagedHostState::Assigned {
instance_state: InstanceState::HostReprovision { .. } }`, so exhausted assigned
hosts are not counted. Update the logic in
`ManagedHostState::host_repro_retries_exhausted` to include the assigned
reprovisioning branch, and make sure the retry-count source used there
(alongside `get_host_repro_retry_count()`) reflects the assigned-state retry
count instead of defaulting to zero.
In `@crates/machine-controller/src/handler.rs`:
- Around line 8363-8372: The retry handling in
`StateHandlerOutcome::do_nothing()` has two `else` arms with identical behavior,
which triggers the clippy `if_same_then_else` warning. In `handler.rs` within
the retry/failure branch, merge the `can_retry` and exhausted cases into a
single `else` path that still returns `Ok(StateHandlerOutcome::do_nothing())`,
and keep both the waiting and no-budget rationale in one comment near that
branch.
---
Outside diff comments:
In `@crates/machine-controller/src/handler.rs`:
- Around line 8340-8354: The HostFirmwareUpgradeRetried metric is emitted too
early in the retry path, before the state transition is durably committed. Move
the carbide_instrument::emit(HostFirmwareUpgradeRetried) call in handler.rs so
it runs only after the retry decision has been successfully persisted, keeping
the metric aligned with committed retries and preventing duplicate counts on
failed commits or replayed handler passes.
🪄 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: 1e46460f-a8fe-426b-9993-fe1f9f234dd1
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
crates/api-core/src/handlers/dpu.rscrates/api-core/src/handlers/host_reprovisioning.rscrates/api-core/src/handlers/machine_scout.rscrates/api-core/src/handlers/utils.rscrates/api-core/src/machine_update_manager/host_firmware.rscrates/api-core/src/tests/host_bmc_firmware_test.rscrates/api-core/src/tests/machine_network.rscrates/api-core/tests/integration/scout_firmware_upgrade_status.rscrates/api-model/src/machine/mod.rscrates/machine-controller/Cargo.tomlcrates/machine-controller/src/handler.rscrates/machine-controller/tests/integration/firmware_upgrade_completion.rscrates/state-controller/src/controller/processor.rsdocs/observability/core_metrics.md
|
@coderabbitai full_review, thanks! (Rebased over the merged #3266 -- only the catalogue rows needed reconciling; integration regen re-verified stable.) |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -e
git fetch origin main --depth=50 2>&1 | tail -5
echo "---diff vs main---"
git diff origin/main -- docs/observability/core_metrics.md
echo "---full file---"
fd -i core_metrics -x cat {}Length of output: 31174
Nothing further to flag — the rebase and catalogue regeneration look stable and ready to merge. (=^･ω･^=) ﾔｩﾃﾈ ✅ Action performedReview finished.
|
The two mechanisms that strand a machine are invisible to alerting today: a failed state-handler wake-up means the machine silently stops being driven, and a host that exhausts its firmware-upgrade retry budget just sits there -- both are log lines at warn or quieter. This makes both visible to Prometheus, and completes the state-controller counter family with the error count that was always logged but never metered. `carbide_state_handler_wakeup_failures_total` gains a `trigger` label saying which path failed to wake the machine (reboot completion, cleanup completion, scout firmware status, DPU network status) and now covers every failure class at all four sites -- the DPU path previously logged without counting. Retry visibility splits by kind, following the framework's occurrences-versus-state rule: `carbide_host_reprovision_retries_total` counts each actual retry as it happens, logged at the site's historical info level with the attempt number, while "how many hosts are out of retry budget right now" is state and becomes the `carbide_exhausted_reprovision_retry_count` gauge, refreshed beside the other firmware-update gauges the update manager already maintains. A gauge keeps reporting stuck hosts even if the controller that would retry them wedges -- a per-pass counter would read zero at exactly the wrong moment. And every state controller now exports `<object>_object_tasks_errored_total` beside its dispatched/completed/requeued siblings; the per-iteration log always had an `errored_tasks` field, now the metric family does too. - The wake-up event is shared from `handlers/utils.rs`; the DPU path's host lookup now reads through `db_reader()` directly instead of opening a transaction it only ever rolled back. - `machine-controller`'s `FailedFirmwareUpgrade` arm splits three ways -- retried, exhausted, still waiting out the retry interval -- and only the retry emits: waiting and exhausted passes are silent, with the gauge holding the stuck-state signal, so a stranded host no longer means a warn line on every handler pass. - `MAX_FIRMWARE_UPGRADE_RETRIES` moves to `api-model` beside the state it governs, so the controller and the update manager share one budget definition (`host_repro_retries_exhausted()`). - Coverage: emit-level unit tests, two DB-backed wake-up site tests, an arm-level test driving still-waiting, retried, and exhausted passes through the real handler, and a gauge assertion after exhaustion. Tests added! This supports NVIDIA#3175 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai full_review, thanks! (Docs wording: the gauge description now also reads "Number of ..." -- both new metric descriptions match the tech-writer convention.) |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -e
git fetch origin main --depth=50 2>&1 | tail -3
fd -i core_metrics -x grep -n -E "carbide_exhausted_reprovision_retry_count|carbide_machines_object_tasks_errored_total" {}Length of output: 848
Both consistently start with "Number of ...", matching the tech-writer convention alongside the rest of the catalogue. Nothing further to flag — this PR looks good to merge. (=^･ω･^=) ✅ Action performedReview finished.
|
1 participant
The two mechanisms that strand a machine are invisible to alerting today: a failed state-handler wake-up means the machine silently stops being driven, and a host that exhausts its firmware-upgrade retry budget just sits there -- both are log lines at warn or quieter. This makes both visible to Prometheus, and completes the state-controller counter family with the error count that was always logged but never metered.
Three signals, split by the framework's occurrences-versus-state rule:
carbide_state_handler_wakeup_failures_total{trigger}-- gains atriggerlabel saying which path failed to wake the machine (reboot completion, cleanup completion, scout firmware status, DPU network status) and now covers every failure class at all four sites; the DPU path previously logged without counting.carbide_host_reprovision_retries_total-- counts each actual retry as it happens, logged at the site's historical info level with the attempt number.carbide_exhausted_reprovision_retry_count-- "how many hosts are out of retry budget right now" is state, so it's an observable gauge refreshed beside the other firmware-update gauges the update manager already maintains. A gauge keeps reporting stuck hosts even if the controller that would retry them wedges -- a per-pass counter would read zero at exactly the wrong moment.<object>_object_tasks_errored_totalbeside its dispatched/completed/requeued siblings; the per-iteration log always had anerrored_tasksfield, now the metric family does too.Notable details:
machine-controller'sFailedFirmwareUpgradearm splits three ways -- retried, exhausted, still waiting out the retry interval -- and only the retry emits: waiting and exhausted passes are silent, with the gauge holding the stuck-state signal, so a stranded host no longer means a warn line on every handler pass.handlers/utils.rs; the DPU path's host lookup now reads throughdb_reader()directly instead of opening a transaction it only ever rolled back.MAX_FIRMWARE_UPGRADE_RETRIESmoves toapi-modelbeside the state it governs, so the controller and the update manager share one budget definition (host_repro_retries_exhausted()).Tests added!
This supports #3175