feat: count firmware update outcomes across every target#3319
Conversation
|
@coderabbitai full_review, thanks! |
|
(๑>ᴗ<๑) ✅ Action performedFull review finished. |
|
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 (7)
Summary by CodeRabbit
WalkthroughThis PR adds structured firmware-update metrics for host, DPU NIC, and SuperNic update paths, including progress and failure counters, wrong-version deduplication, validation tests, and observability documentation. ChangesFirmware update instrumentation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant HostReprovisioning
participant HostFirmwareUpdate
participant DpuNicFirmwareUpdate
participant SvpcHandler
participant MetricsModule
HostReprovisioning->>MetricsModule: emit FirmwareUpdateProgress(Host, Started)
HostFirmwareUpdate->>MetricsModule: emit FirmwareUpdateProgress(Host, Started/Completed)
DpuNicFirmwareUpdate->>MetricsModule: emit FirmwareUpdateProgress(DpuNic, Started)
DpuNicFirmwareUpdate->>MetricsModule: emit FirmwareUpdateFailed(DpuNic, NoUpdateMatch/WrongVersionAfterUpdate)
SvpcHandler->>MetricsModule: emit FirmwareUpdateProgress(SuperNic, Started)
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3319.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.
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/api-core/src/machine_update_manager/host_firmware.rs (1)
70-115: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMove the
Startedemit after the batch commit.
FirmwareUpdateProgress { phase: Started, .. }is emitted beforetxn.commit().await?, so a later error rolls back the DB work but leaves the counter incremented. Emit fromupdates_startedonly after the commit succeeds, matching the DPU path and keeping the series aligned with committed triggers.🤖 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/machine_update_manager/host_firmware.rs` around lines 70 - 115, The `FirmwareUpdateProgress` with phase `Started` is emitted too early in `host_firmware::check_for_updates`, before `txn.commit().await?`, which can leave progress counters out of sync if the transaction later fails. Move the `carbide_instrument::emit` call so it happens only after `txn.commit()` succeeds, and emit using the `updates_started` set after the batch is durably committed; keep `db::host_machine_update::trigger_host_reprovisioning_request`, `updates_started`, and the commit boundary aligned with the DPU path.
🤖 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/machine_update_manager/host_firmware.rs`:
- Around line 117-137: The `clear_completed_updates` path in
`MachineUpdateManager` emits `FirmwareUpdateProgress` with
`FirmwareUpdatePhase::Completed` before the surrounding transaction is
committed, so move those emits out of the transaction boundary. Collect the
affected machine IDs in `clear_completed_updates` while still calling
`remove_health_report` and `update_update_complete`, then have
`MachineUpdateManager::run` emit the `Completed` events only after
`txn.commit().await?` succeeds.
In `@docs/observability/core_metrics.md`:
- Line 44: Update the documentation row for carbide_firmware_updates_total in
the core metrics table to include the phase label alongside target. The current
description only mentions the update target, so clarify that the counter is
split by both target and phase, and note the started/completed series in the
metric description.
---
Outside diff comments:
In `@crates/api-core/src/machine_update_manager/host_firmware.rs`:
- Around line 70-115: The `FirmwareUpdateProgress` with phase `Started` is
emitted too early in `host_firmware::check_for_updates`, before
`txn.commit().await?`, which can leave progress counters out of sync if the
transaction later fails. Move the `carbide_instrument::emit` call so it happens
only after `txn.commit()` succeeds, and emit using the `updates_started` set
after the batch is durably committed; keep
`db::host_machine_update::trigger_host_reprovisioning_request`,
`updates_started`, and the commit boundary aligned with the DPU path.
🪄 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: 77a83aa7-0cb7-4e16-9f30-ca4a55aa12ac
📒 Files selected for processing (6)
crates/api-core/src/handlers/svpc.rscrates/api-core/src/machine_update_manager/dpu_nic_firmware.rscrates/api-core/src/machine_update_manager/host_firmware.rscrates/api-core/src/machine_update_manager/metrics.rscrates/api-core/src/tests/dpu_nic_firmware.rsdocs/observability/core_metrics.md
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/machine_update_manager/host_firmware.rs (1)
89-114: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winEmit
Startedafter the batch commit, not per-iteration inside it.
txn.commit().await?at line 113 covers the whole loop. If a latertrigger_host_reprovisioning_requestin the same batch fails and returnsErr, the transaction rolls back — undoing earlier machines' DB writes — but theirFirmwareUpdateProgress{Started}events already fired. Those machines get re-triggered (and re-counted) on the next pass, inflatingcarbide_firmware_updates_total{target=host,phase=started}beyond real distinct starts.dpu_nic_firmware.rs'sstart_updatesavoids this exact pitfall by committing before emitting; apply the same ordering here.🛡️ Proposed fix: commit before emitting
db::host_machine_update::trigger_host_reprovisioning_request( &mut txn, "Automated", machine_update, ) .await?; - // Counted after the trigger succeeds; the commit below spans the - // whole batch, so a later DB error can re-count machines on the - // next pass -- the same repeat-on-retry the old log line had. - carbide_instrument::emit(FirmwareUpdateProgress { - target: FirmwareUpdateTarget::Host, - phase: FirmwareUpdatePhase::Started, - machine_id: *machine_update, - detail: String::new(), - }); updates_started.insert(*machine_update); } txn.commit().await?; + for machine_update in &updates_started { + carbide_instrument::emit(FirmwareUpdateProgress { + target: FirmwareUpdateTarget::Host, + phase: FirmwareUpdatePhase::Started, + machine_id: *machine_update, + detail: String::new(), + }); + } Ok(updates_started)🤖 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/machine_update_manager/host_firmware.rs` around lines 89 - 114, Emitting FirmwareUpdateProgress::Started inside the loop in host_firmware::start_updates can overcount when the surrounding transaction rolls back; move the start-event emission to after txn.commit().await? succeeds, so only committed host reprovision requests are counted. Keep the trigger_host_reprovisioning_request loop and updates_started collection as-is, but defer carbide_instrument::emit for each machine until after the batch commit, following the same commit-before-emit ordering used in dpu_nic_firmware::start_updates.
🤖 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/machine_update_manager/host_firmware.rs`:
- Around line 89-114: Emitting FirmwareUpdateProgress::Started inside the loop
in host_firmware::start_updates can overcount when the surrounding transaction
rolls back; move the start-event emission to after txn.commit().await? succeeds,
so only committed host reprovision requests are counted. Keep the
trigger_host_reprovisioning_request loop and updates_started collection as-is,
but defer carbide_instrument::emit for each machine until after the batch
commit, following the same commit-before-emit ordering used in
dpu_nic_firmware::start_updates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0c1245b1-1e70-47dc-ac43-0cb7f7f534e3
📒 Files selected for processing (6)
crates/api-core/src/handlers/svpc.rscrates/api-core/src/machine_update_manager/dpu_nic_firmware.rscrates/api-core/src/machine_update_manager/host_firmware.rscrates/api-core/src/machine_update_manager/metrics.rscrates/api-core/src/tests/dpu_nic_firmware.rsdocs/observability/core_metrics.md
chet
force-pushed
the
gh-issue-3175-firmware
branch
from
8188e48 to
b22549d
Compare
|
@coderabbitai full_review, thanks! |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/api-core/src/machine_update_manager/metrics.rs (1)
113-127: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
Option<String>instead of empty-string sentinels for optional context.
unmatched_dpu_machine_id,firmware_version, anddetailall rely on an emptyStringto mean "not applicable for this cause/target." This is a workable convention documented in the field comments, but it's a magic value that a future caller could misuse (e.g., accidentally logging a genuinely empty version string as "n/a"). Since these are#[context]fields (log-only, not labels), anOption<String>would letcarbide_instrumentrenderNoneexplicitly and remove the ambiguity, at the cost of updating the three call sites that currently passString::new().♻️ Sketch of the alternative
- #[context] - pub unmatched_dpu_machine_id: String, + #[context] + pub unmatched_dpu_machine_id: Option<String>,Also applies to: 150-167
🤖 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/machine_update_manager/metrics.rs` around lines 113 - 127, The `FirmwareUpdateProgress` context fields are using empty strings as sentinel values for “not applicable,” which is ambiguous and easy to misuse. Update the `FirmwareUpdateProgress` struct fields (`machine_id`/`detail` and the related optional context fields used in the same module) to use `Option<String>` for log-only `#[context]` data, then adjust the call sites that currently pass `String::new()` to pass `None` when a value is not relevant. Keep the `FirmwareUpdateProgress` and `carbide_instrument` usage aligned so the logged context renders the absence explicitly.
🤖 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/machine_update_manager/metrics.rs`:
- Around line 113-127: The `FirmwareUpdateProgress` context fields are using
empty strings as sentinel values for “not applicable,” which is ambiguous and
easy to misuse. Update the `FirmwareUpdateProgress` struct fields
(`machine_id`/`detail` and the related optional context fields used in the same
module) to use `Option<String>` for log-only `#[context]` data, then adjust the
call sites that currently pass `String::new()` to pass `None` when a value is
not relevant. Keep the `FirmwareUpdateProgress` and `carbide_instrument` usage
aligned so the logged context renders the absence explicitly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 729a6855-20f3-4989-a465-b1f415714e55
📒 Files selected for processing (6)
crates/api-core/src/handlers/svpc.rscrates/api-core/src/machine_update_manager/dpu_nic_firmware.rscrates/api-core/src/machine_update_manager/host_firmware.rscrates/api-core/src/machine_update_manager/metrics.rscrates/api-core/src/tests/dpu_nic_firmware.rsdocs/observability/core_metrics.md
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
|
@coderabbitai full_review, thanks! (External Codex round: a persistent wrong-version condition now counts once per outcome instead of once per manager pass -- an in-memory reported-set, cleared when the DPU's markers clear so a fresh attempt re-reports; and operator-initiated host reprovisions emit the paired started, so the started-to-completed gap stays truthful for every initiator.) |
chet
force-pushed
the
gh-issue-3175-firmware
branch
from
b22549d to
f1b2160
Compare
|
(ᵔᴥᵔ) ✏️ Learnings added
✅ Action performedFull review finished. |
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/handlers/host_reprovisioning.rs (1)
76-92: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winEmit occurs before the transaction commits.
The
carbide_instrument::emitcall at Lines 85-92 fires immediately aftertrigger_host_reprovisioning_request, but the enclosing transaction is not committed until Line 100. Iftxn.commit()fails, aStartedevent and counter increment will have already been recorded for a reprovisioning request that was never actually persisted — undermining the started-to-completed gap accuracy that this PR is explicitly designed to preserve.Move the emit after the successful
txn.commit()(or only within theMode::Setbranch, after commit) so metrics only reflect durably-committed state.🐛 Proposed fix
match req.mode() { Mode::Set => { let initiator = req.initiator().as_str_name(); db::host_machine_update::trigger_host_reprovisioning_request( &mut txn, initiator, &machine_id, ) .await?; - // Manual initiations pair with the same completion emit the - // update manager's automatic path gets, keeping the - // started-to-completed gap truthful for every initiator. - carbide_instrument::emit( - crate::machine_update_manager::metrics::FirmwareUpdateProgress { - target: crate::machine_update_manager::metrics::FirmwareUpdateTarget::Host, - phase: crate::machine_update_manager::metrics::FirmwareUpdatePhase::Started, - machine_id, - detail: initiator.to_string(), - }, - ); } Mode::Clear => { db::host_machine_update::clear_host_reprovisioning_request(&mut txn, &machine_id) .await?; } } txn.commit().await?; + + if matches!(req.mode(), Mode::Set) { + carbide_instrument::emit( + crate::machine_update_manager::metrics::FirmwareUpdateProgress { + target: crate::machine_update_manager::metrics::FirmwareUpdateTarget::Host, + phase: crate::machine_update_manager::metrics::FirmwareUpdatePhase::Started, + machine_id, + detail: String::new(), + }, + ); + } Ok(Response::new(()))🤖 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/host_reprovisioning.rs` around lines 76 - 92, The Started metrics emit in host reprovisioning is happening before the transaction is durably committed, so a failed commit can still record a request that never persisted. Update host_reprovisioning.rs in the flow around trigger_host_reprovisioning_request and txn.commit() so carbide_instrument::emit runs only after a successful commit, or guard it so it executes only in the committed Mode::Set path. Keep the metrics emission aligned with the committed state by referencing the existing FirmwareUpdateProgress, FirmwareUpdateTarget::Host, and FirmwareUpdatePhase::Started 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.
Inline comments:
In `@crates/api-core/src/handlers/host_reprovisioning.rs`:
- Line 90: The `HostReprovisioning` handler is populating
`FirmwareUpdateProgress::detail` with `initiator.to_string()`, which conflicts
with the Host contract and existing Host test expectations. Update the
`host_reprovisioning` flow to keep `detail` empty for Host emissions, using the
`FirmwareUpdateProgress` construction in this handler; if initiator info must be
preserved, first update the documented Host schema and related tests to match
the new contract.
---
Outside diff comments:
In `@crates/api-core/src/handlers/host_reprovisioning.rs`:
- Around line 76-92: The Started metrics emit in host reprovisioning is
happening before the transaction is durably committed, so a failed commit can
still record a request that never persisted. Update host_reprovisioning.rs in
the flow around trigger_host_reprovisioning_request and txn.commit() so
carbide_instrument::emit runs only after a successful commit, or guard it so it
executes only in the committed Mode::Set path. Keep the metrics emission aligned
with the committed state by referencing the existing FirmwareUpdateProgress,
FirmwareUpdateTarget::Host, and FirmwareUpdatePhase::Started 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: f2b4cedb-6c86-46c3-a6da-dc8653885574
📒 Files selected for processing (7)
crates/api-core/src/handlers/host_reprovisioning.rscrates/api-core/src/handlers/svpc.rscrates/api-core/src/machine_update_manager/dpu_nic_firmware.rscrates/api-core/src/machine_update_manager/host_firmware.rscrates/api-core/src/machine_update_manager/metrics.rscrates/api-core/src/tests/dpu_nic_firmware.rsdocs/observability/core_metrics.md
✅ Files skipped from review due to trivial changes (1)
- docs/observability/core_metrics.md
🚧 Files skipped from review as they are similar to previous changes (5)
- crates/api-core/src/handlers/svpc.rs
- crates/api-core/src/tests/dpu_nic_firmware.rs
- crates/api-core/src/machine_update_manager/host_firmware.rs
- crates/api-core/src/machine_update_manager/metrics.rs
- crates/api-core/src/machine_update_manager/dpu_nic_firmware.rs
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/api-core/src/handlers/host_reprovisioning.rs (1)
85-92: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer a top-level
useover repeated fully-qualified paths.Other call sites (
host_firmware.rs,dpu_nic_firmware.rs,svpc.rs) importFirmwareUpdateProgress/FirmwareUpdateTarget/FirmwareUpdatePhaseat the top of the file; this handler instead spells out the full module path three times inline, which is more verbose than necessary.♻️ Proposed refactor
+use crate::machine_update_manager::metrics::{ + FirmwareUpdatePhase, FirmwareUpdateProgress, FirmwareUpdateTarget, +}; + pub(crate) async fn trigger_host_reprovisioning( ... - carbide_instrument::emit( - crate::machine_update_manager::metrics::FirmwareUpdateProgress { - target: crate::machine_update_manager::metrics::FirmwareUpdateTarget::Host, - phase: crate::machine_update_manager::metrics::FirmwareUpdatePhase::Started, - machine_id, - detail: initiator.to_string(), - }, - ); + carbide_instrument::emit(FirmwareUpdateProgress { + target: FirmwareUpdateTarget::Host, + phase: FirmwareUpdatePhase::Started, + machine_id, + detail: String::new(), + });🤖 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/host_reprovisioning.rs` around lines 85 - 92, The host reprovisioning handler is using repeated fully-qualified paths for firmware update metrics, making the `emit` call more verbose than the other firmware handlers. Add top-level `use` imports for `FirmwareUpdateProgress`, `FirmwareUpdateTarget`, and `FirmwareUpdatePhase` in `host_reprovisioning.rs`, then update the `carbide_instrument::emit` call to use those imported symbols consistently like the other call sites.crates/api-core/src/machine_update_manager/dpu_nic_firmware.rs (1)
262-282: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor:
Default::default()avoids repeating the mutex boilerplate.
reported_wrong_versions: std::sync::Mutex::new(HashSet::new())is spelled out identically here and in five test struct literals intests/dpu_nic_firmware.rs.Mutex<HashSet<T>>implementsDefault, soDefault::default()would be slightly more idiomatic and DRY.♻️ Optional tweak
Some(DpuNicFirmwareUpdate { - reported_wrong_versions: std::sync::Mutex::new(HashSet::new()), + reported_wrong_versions: Default::default(), metrics: Some(metrics), config, dpf, })🤖 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/machine_update_manager/dpu_nic_firmware.rs` around lines 262 - 282, The `DpuNicFirmwareUpdate::new` initializer repeats the mutex construction boilerplate for `reported_wrong_versions`, and the same pattern appears in the related test struct literals; switch this field to use `Default::default()` since `Mutex<HashSet<_>>` implements `Default`, keeping the constructor and the `tests/dpu_nic_firmware.rs` fixtures consistent and more idiomatic.
🤖 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/host_reprovisioning.rs`:
- Around line 85-92: The host reprovisioning handler is using repeated
fully-qualified paths for firmware update metrics, making the `emit` call more
verbose than the other firmware handlers. Add top-level `use` imports for
`FirmwareUpdateProgress`, `FirmwareUpdateTarget`, and `FirmwareUpdatePhase` in
`host_reprovisioning.rs`, then update the `carbide_instrument::emit` call to use
those imported symbols consistently like the other call sites.
In `@crates/api-core/src/machine_update_manager/dpu_nic_firmware.rs`:
- Around line 262-282: The `DpuNicFirmwareUpdate::new` initializer repeats the
mutex construction boilerplate for `reported_wrong_versions`, and the same
pattern appears in the related test struct literals; switch this field to use
`Default::default()` since `Mutex<HashSet<_>>` implements `Default`, keeping the
constructor and the `tests/dpu_nic_firmware.rs` fixtures consistent and more
idiomatic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: afb23cac-cb53-4ce3-9659-6cca1cb36892
📒 Files selected for processing (7)
crates/api-core/src/handlers/host_reprovisioning.rscrates/api-core/src/handlers/svpc.rscrates/api-core/src/machine_update_manager/dpu_nic_firmware.rscrates/api-core/src/machine_update_manager/host_firmware.rscrates/api-core/src/machine_update_manager/metrics.rscrates/api-core/src/tests/dpu_nic_firmware.rsdocs/observability/core_metrics.md
chet
force-pushed
the
gh-issue-3175-firmware
branch
from
f1b2160 to
9d4bc54
Compare
|
@coderabbitai full_review, thanks! |
|
(๑>ᴗ<๑) ✅ Action performedFull review finished. |
|
@coderabbitai full_review, thanks! (Rebased over the merged #3272 -- clean, no conflicts; integration regen re-verified the catalogue stable.) |
chet
force-pushed
the
gh-issue-3175-firmware
branch
from
9d4bc54 to
4f50ead
Compare
|
(๑>ᴗ<๑) ✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/handlers/host_reprovisioning.rs`:
- Around line 82-92: Move the Started FirmwareUpdateProgress emission from
before txn.commit().await? to immediately after the commit succeeds in the host
reprovisioning handler. Preserve the existing event fields and ensure no metric
is emitted when the transaction fails.
In `@crates/api-core/src/machine_update_manager/dpu_nic_firmware.rs`:
- Around line 184-203: Add a focused test alongside test_clear_completed_updates
that configures a DPU with an unexpected firmware version, invokes
clear_completed_updates twice, and verifies that WrongVersionAfterUpdate is
emitted only once. Also assert reported_wrong_versions contains the marker after
the first call and prevents duplicate emission on the second.
🪄 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: 476b9aed-621c-4169-9442-edbb3de8124a
📒 Files selected for processing (7)
crates/api-core/src/handlers/host_reprovisioning.rscrates/api-core/src/handlers/svpc.rscrates/api-core/src/machine_update_manager/dpu_nic_firmware.rscrates/api-core/src/machine_update_manager/host_firmware.rscrates/api-core/src/machine_update_manager/metrics.rscrates/api-core/src/tests/dpu_nic_firmware.rsdocs/observability/core_metrics.md
chet
force-pushed
the
gh-issue-3175-firmware
branch
from
4f50ead to
85d59ad
Compare
|
@coderabbitai full_review, thanks! |
|
(๑>ᴗ<๑) ✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/observability/core_metrics.md`:
- Line 45: Update the carbide_firmware_updates_total description in
core_metrics.md to document phase coverage by target: DpuNic and SuperNic emit
only started, while Host emits both started and completed.
🪄 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: 199cee19-b72f-49c7-b6ea-09b691d517bc
📒 Files selected for processing (7)
crates/api-core/src/handlers/host_reprovisioning.rscrates/api-core/src/handlers/svpc.rscrates/api-core/src/machine_update_manager/dpu_nic_firmware.rscrates/api-core/src/machine_update_manager/host_firmware.rscrates/api-core/src/machine_update_manager/metrics.rscrates/api-core/src/tests/dpu_nic_firmware.rsdocs/observability/core_metrics.md
Firmware updates run for three targets -- host BIOS/BMC, DPU NICs, and SuperNICs -- and none of their outcomes are countable today: starts, completions, and failures live only in log lines, including the "wrong version after attempted update" case that means an update silently didn't take. This makes all of it countable, with each event replacing its site's log line at the historical level (info for progress, warn for failures) and the same information as structured fields.
Two counters from `carbide-instrument`:
- `carbide_firmware_updates_total{target, phase}` -- started and completed, per target. For hosts both phases fire, so the started-to-completed gap is the in-flight backlog; the DPU-NIC and SuperNIC targets emit started only, with completion visible in their own state flows.
- `carbide_firmware_update_failures_total{target, cause}` -- `no_update_match` (a DPU left ready between snapshot and trigger) and `wrong_version_after_update` (the update ran and the firmware version still doesn't match -- the silent-failure case).
Notable details:
- Started counts only committed triggers: the DPU-NIC emit fires after `txn.commit()`, so an update whose trigger rolled back is a `no_update_match` failure, never a started update, and the started-minus-completed math stays truthful.
- Host completion emits once per machine as its update markers clear, replacing one Vec-debug log line with per-machine lines.
- A SuperNIC re-counts one started per scout pass while a device stays mismatched -- each pass issues a real apply command, and the sustained rate is the stuck-device signal.
Tests added!
This supports NVIDIA#3175
Signed-off-by: Chet Nichols III <chetn@nvidia.com>
chet
force-pushed
the
gh-issue-3175-firmware
branch
from
85d59ad to
1c7a0c3
Compare
|
@coderabbitai full_review, thanks! |
|
(๑>ᴗ<๑) ✅ Action performedFull review finished. ✅ Action performedFull review finished. |
2 participants
Firmware updates run for three targets -- host BIOS/BMC, DPU NICs, and SuperNICs -- and none of their outcomes are countable today: starts, completions, and failures live only in log lines, including the "wrong version after attempted update" case that means an update silently didn't take. This makes all of it countable, with each event replacing its site's log line at the historical level (info for progress, warn for failures) and the same information as structured fields.
Two counters from
carbide-instrument:carbide_firmware_updates_total{target, phase}-- started and completed, per target. For hosts both phases fire, so the started-to-completed gap is the in-flight backlog; the DPU-NIC and SuperNIC targets emit started only, with completion visible in their own state flows.carbide_firmware_update_failures_total{target, cause}--no_update_match(a DPU left ready between snapshot and trigger) andwrong_version_after_update(the update ran and the firmware version still doesn't match -- the silent-failure case).Notable details:
txn.commit(), so an update whose trigger rolled back is ano_update_matchfailure, never a started update, and the started-minus-completed math stays truthful.Tests added!
This supports #3175