Fix DPU firmware update health alert never clearing on DPF hosts - #4236
Merged
wminckler merged 1 commit intoJul 28, 2026
Merged
Conversation
DPF-ingested hosts pick their DPU firmware update targets from the DPUDeployment's expected BFB — find_outdated_dpus skips DPF hosts outright and find_outdated_dpus_dpf selects them by BFB hash instead. The completion path, though, only removed the update markers when the DPU's reported NIC firmware appeared in dpu_nic_firmware_update_versions, a list that only ever holds NIC firmware strings. For a DPF host that comparison has no reason to match, so a successful reprovision left the HostUpdateInProgress alert pinned on the host forever. Because that alert carries prevent_allocations, the host stayed unallocatable, and its lingering alert also made is_available_for_updates return false, excluding the host from every later firmware update. Each manager pass additionally emitted a bogus WrongVersionAfterUpdate outcome. Carry the host's DPF-ingestion flag through DpuMachineUpdate so the completion path can skip the NIC-version comparison for DPF hosts. The rest of the completion gate is unchanged: the host must still hold the update marker, be Ready, and have every DPU's reprovisioning flag cleared. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
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
WalkthroughThe change adds DPF provenance to ChangesDPF firmware completion
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant DPFDiscovery
participant CompletedUpdates
participant ClearCompletedUpdates
participant UpdateHealth
participant FirmwareMetrics
DPFDiscovery->>CompletedUpdates: Set dpf_managed provenance
CompletedUpdates->>ClearCompletedUpdates: Return completed DpuMachineUpdate
ClearCompletedUpdates->>UpdateHealth: Remove update marker
ClearCompletedUpdates->>FirmwareMetrics: Skip wrong-version failure metric
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
abvarshney-nv
approved these changes
Jul 28, 2026
7 tasks
nv-dmendoza
pushed a commit
that referenced
this pull request
Jul 29, 2026
…4312) Backport of #4236 to `release/v2.0` (upstream commit 7ba222e). DPF-ingested hosts pick their DPU firmware update targets from the DPUDeployment's expected BFB: `find_outdated_dpus` skips DPF hosts outright, and `find_outdated_dpus_dpf` selects them by BFB hash instead. The completion path, however, only removed the update markers when the DPU's reported NIC firmware appeared in `dpu_nic_firmware_update_versions` — a config list that only ever holds NIC firmware strings. For a DPF host that comparison has no reason to match, so a successful reprovision left the `HostUpdateInProgress` alert pinned on the host forever. That is not a cosmetic problem. The alert carries `prevent_allocations`, so the host stayed unallocatable indefinitely, and its lingering alert also made `is_available_for_updates` return false, excluding the host from every later firmware update. Each manager pass additionally emitted a bogus `WrongVersionAfterUpdate` outcome. The fix carries the host's DPF-ingestion flag through `DpuMachineUpdate` so the completion path can skip the NIC-version comparison for DPF hosts. The rest of the completion gate is unchanged: the host must still hold the update marker, be Ready, and have every DPU's reprovisioning flag cleared. A DPF-driven reprovision finishing is itself the completion signal. ## Related issues None. ## Type of Change - [ ] **Add** - New feature or capability - [ ] **Change** - Changes in existing functionality - [x] **Fix** - Bug fixes - [ ] **Remove** - Removed features or deprecated functionality - [ ] **Internal** - Internal changes (refactoring, tests, docs, etc.) ## Breaking Changes - [ ] **This PR contains breaking changes** ## Testing - [x] Unit tests added/updated Adds `test_clear_completed_updates_dpf_host_off_list_version`, which marks a host DPF-ingested, lands its DPU on a firmware version deliberately outside the configured allowlist, and asserts the update marker and its alert are both cleared. The test was confirmed non-vacuous: neutralizing the `dpf_managed` branch makes it fail with the exact production symptom (`Incorrect firmware version after attempted update`, marker still pinned). Full runs against a local Postgres, all green: `tests::dpu_nic_firmware` (6), `tests::dpu_machine_update` and `tests::machine_update_manager` (13), `tests::instance::test_instance_creation_when_reprovision_is_triggered_parallel`, and the complete `carbide-api-db` and `carbide-api-model` suites (575). ## Additional Notes Two adaptations were needed relative to the upstream commit; the production logic change itself is identical. 1. `crates/api-db/src/dpu_machine_update.rs` — upstream reads the flag as `host_snapshot.config.dpf.used_for_ingestion`. On v2.0 `Machine` has no `config` field, so this is `host_snapshot.dpf.used_for_ingestion`, matching the existing usage in `crates/api-model/src/dpu_machine_update.rs`. 2. `crates/api-core/src/tests/dpu_nic_firmware.rs` — the new test drops two things absent from v2.0: the `reported_wrong_versions` field on `DpuNicFirmwareUpdate`, and a `MetricsCapture`-based assertion that no `wrong_version_after_update` failure is counted. v2.0 has no wrong-version metric on this path at all. The health-report and alert assertions, which are the actual regression coverage, are unchanged. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DPF-ingested hosts never had their
HostUpdateInProgresshealth alert removed after an automatic DPU firmware update finished. The machine would reprovision successfully, come backReady, and then sit permanently with anAutomaticDpuFirmwareUpdatealert pinned to it. Non-DPF hosts were unaffected.The cause is an asymmetry between how an update is started and how it is completed. DPF hosts are deliberately excluded from the version-based staleness check —
find_outdated_dpusreturns early for any host withdpf.used_for_ingestion, andfind_outdated_dpus_dpfselects those hosts instead by comparing against the expected BFB from theirDPUDeployment. The completion path had no such split: it only removed the update markers when the DPU's reported NIC firmware appeared indpu_nic_firmware_update_versions, a config list that only ever contains NIC firmware version strings. Nothing in the DPF path constrains a DPU to land on a version from that list, so for a DPF host the comparison had no reason to ever match, and the marker was never removed.The consequences went beyond a stale alert. The alert carries
prevent_allocations, so affected hosts stayed unallocatable indefinitely. The lingering alert also makesis_available_for_updatesreturn false, which excluded those hosts from every subsequent firmware update. And each manager pass emitted a spuriousWrongVersionAfterUpdatefailure metric for a host that had in fact updated correctly.The fix carries the host's DPF-ingestion flag through
DpuMachineUpdateso the completion path can mirror the split the start path already has, skipping the NIC-version comparison for DPF hosts. The rest of the completion gate is untouched: the host must still be holding the update marker, be inManagedHostState::Ready, and have every one of its DPUs'reprovisioning_requestedflags cleared before anything is removed.Related issues
None.
Type of Change
Breaking Changes
Testing
Added
test_clear_completed_updates_dpf_host_off_list_version, which marks a host DPF-ingested, lands its DPU on a firmware version asserted to be absent fromdpu_nic_firmware_update_versions, clears the reprovisioning flag, and leaves the update marker in place. It asserts the health report is removed, aggregate health is clean, and nowrong_version_after_updatefailure is counted. The test fails against the previous behaviour.Ran the
dpu_nic_firmware(7 tests),dpu_machine_update, andmachine_update_manager(18 tests) suites — all passing. Not yet exercised against a live DPF-managed cluster.Additional Notes
DpuMachineUpdate::dpf_managedis marked#[sqlx(default)]because the struct is also loaded viaFromRowinget_reprovisioning_machines, whose query does not select the column; that path only consumeshost_machine_id, so defaulting tofalsethere is inert.Worth a reviewer's opinion: when a DPF host's reprovision completes but the DPU is still mismatched against a
DPUDeploymentthat changed mid-update, this clears the marker rather than retaining it. That is deliberate —run_single_iterationcallsclear_completed_updatesand thenstart_updatesin the same pass, so the host is re-detected byfind_outdated_dpus_dpfand the update is re-triggered immediately, with the marker re-applied. Retaining the marker instead would makeis_available_for_updatesfalse forever and recreate exactly the permanent-pin failure this PR fixes.🤖 Generated with Claude Code