feat: count preingestion's firmware, BFB copy, and power operations#3386
feat: count preingestion's firmware, BFB copy, and power operations#3386chet wants to merge 1 commit into
Conversation
|
@coderabbitai full_review, thanks! |
|
ᕦ(ò_óˇ)ᕤ ✅ Action performedFull review finished. |
|
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 ignored due to path filters (1)
📒 Files selected for processing (4)
Summary by CodeRabbit
WalkthroughPreingestion-manager adds metrics for firmware uploads, upgrade tasks, BFB copy operations, and power controls. Update initiation now reports wait-task persistence, BFB completion handling resolves races atomically, and observability tests and documentation are expanded. ChangesPreingestion observability
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PreingestionManager
participant RedfishClient
participant WaitTaskStore
participant Metrics
PreingestionManager->>RedfishClient: initiate firmware upload or update
RedfishClient-->>PreingestionManager: upload result or task state
PreingestionManager->>WaitTaskStore: store Redfish wait task when started
PreingestionManager->>Metrics: emit firmware upload or upgrade task event
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-3386.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: 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 `@crates/preingestion-manager/src/lib.rs`:
- Around line 2600-2653: Eliminate the TOCTOU race by adding an atomic lock-held
BfbCopyManager operation, such as take_result_or_expire(&address, expire), that
returns any completed result or removes the entry only when expired. Update
in_bfb_copy_in_progress and the corresponding logic around the additional
occurrence to compute elapsed_mins/expiration first, then use this single
operation instead of separate state() and clear() calls; preserve existing
success, failure, and timeout handling based on its result.
🪄 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: c3f707a5-f167-4bd5-aaf2-1ae264bf8e0e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
crates/preingestion-manager/Cargo.tomlcrates/preingestion-manager/src/lib.rscrates/preingestion-manager/src/metrics.rsdocs/observability/core_metrics.md
| PowerCycle, | ||
| BmcReset, | ||
| ChassisReset, | ||
| } |
There was a problem hiding this comment.
how many of these enums do we have now? 😆
There was a problem hiding this comment.
Honest count: this PR adds four (BfbCopyOutcome, PowerOperation, UpgradeTaskFinalState, and the FirmwareComponentLabel newtype), and the initiative overall is somewhere north of twenty-five. It's deliberate, for what it's worth -- a bounded label enum is the framework's whole cardinality mechanism: every variant is a compile-time-known Prometheus series, so nothing caller-supplied can ever mint unbounded label values. The enums ARE the guardrail. (But yes, lol.)
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
chet
force-pushed
the
gh-issue-3174-preingestion
branch
from
41f0c7d to
bdf3f91
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 `@crates/preingestion-manager/src/metrics.rs`:
- Around line 312-322: Update the host power-reset branches in
in_reset_for_new_firmware to handle RedfishError::UnnecessaryOperation
explicitly after count_power_op records it as successful, allowing the reset
flow to continue without returning through the generic Err(e) path.
Alternatively, only classify it as ok when the caller can continue; align this
behavior with PowerOperation::treats_unnecessary_as_ok and ensure the reset
outcome is consistently reflected in carbide_preingestion_power_control_total.
🪄 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: 734b3587-dfae-46cd-8b59-3bc27eaae89c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
crates/preingestion-manager/Cargo.tomlcrates/preingestion-manager/src/lib.rscrates/preingestion-manager/src/metrics.rsdocs/observability/core_metrics.md
The preingestion manager runs the longest and most failure-prone hardware operations in the system -- multi-decade-minute BFB copies, Redfish firmware uploads and upgrade tasks, power and reset sequences -- and none of it is countable: the code literally says "Someday, we should generate metrics on how often this happens". Someday is now.
- `carbide_preingestion_bfb_copy_duration_seconds{outcome}` -- a histogram over the ~30-minute copy, with `ok`, `error`, and `timeout` outcomes; the event owns the historical completion lines (INFO on success, ERROR otherwise). A copy that outlives the 35-minute give-up counts exactly once: a ready result is consumed before the deadline check, and a timed-out copy's late result drops uncounted -- timeout stays its only record.
- `carbide_preingestion_firmware_upgrade_tasks_total{firmware, final_state, outcome}` -- one count per Redfish upgrade task reaching a terminal state, emitted only after the transition commits; a chain step stuck behind a failing next-artifact upload no longer re-counts every manager pass, and Dell's vanished-task-but-version-matches path now counts as the completion it is.
- `carbide_preingestion_firmware_upload_total{method, outcome}` -- uploads by `simple_update`, `multipart`, and `http_push`, counting real wire attempts (a multipart NotSupported followed by the http-push fallback is two truthful counts).
- `carbide_preingestion_power_control_total{operation, outcome}` -- all fifteen power, BMC-reset, and chassis-reset call sites through one wrapper. An HTTP 409 counts as `ok` only for operations that target a power state (already in the requested state); for restarts, powercycles, and resets -- which have no state to already be in -- it counts as the refusal it is.
Notable details:
- Every site's own log line stays; the metric-only counters (`log = off`) sit beside untouched error/warn lines, and the two dynamic-level events own their historical lines at the historical levels.
- The exposition rides carbide-api's existing meter (the manager is a hosted library) -- no new endpoint or wiring.
Tests added!
This supports NVIDIA#3174
Signed-off-by: Chet Nichols III <chetn@nvidia.com>
chet
force-pushed
the
gh-issue-3174-preingestion
branch
from
bdf3f91 to
1480c12
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/preingestion-manager/src/lib.rs (1)
3081-3133: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider whether a successful Multipart→HttpPush fallback should surface as an
Outcome::Errorseries. When a BMC does not support multipart, theMultipart/Errorcounter increments even though the overall upload subsequently succeeds viaHttpPush. Any dashboard or alert trackingcarbide_preingestion_firmware_upload_total{outcome="error"}will therefore treat a benign protocol negotiation as a failure.This appears intentional (the semantics are exercised in
metrics.rs), so no change may be warranted — but please confirm the alerting story accounts for expected fallback traffic on multipart-incapable vendors, or consider a distinct outcome/label to distinguish "fell back" from "genuinely failed".🤖 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/preingestion-manager/src/lib.rs` around lines 3081 - 3133, The successful Multipart-to-HttpPush fallback currently emits Multipart with Outcome::Error, potentially inflating failure metrics. Review the event semantics in the firmware upload flow around initiate_update and the metric definitions/tests in metrics.rs; confirm alerting explicitly accounts for expected fallback traffic, or introduce a distinct outcome/label for fallback so genuine upload failures remain distinguishable.
🤖 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/preingestion-manager/src/lib.rs`:
- Around line 3081-3133: The successful Multipart-to-HttpPush fallback currently
emits Multipart with Outcome::Error, potentially inflating failure metrics.
Review the event semantics in the firmware upload flow around initiate_update
and the metric definitions/tests in metrics.rs; confirm alerting explicitly
accounts for expected fallback traffic, or introduce a distinct outcome/label
for fallback so genuine upload failures remain distinguishable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 39544575-c255-4c64-a73a-43046e3cd9b4
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
crates/preingestion-manager/Cargo.tomlcrates/preingestion-manager/src/lib.rscrates/preingestion-manager/src/metrics.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 (2)
- crates/preingestion-manager/Cargo.toml
- crates/preingestion-manager/src/metrics.rs
|
@coderabbitai full_review, thanks! |
|
(ᵔᴥᵔ) ✅ Action performedFull review finished. |
2 participants
The preingestion manager runs the longest and most failure-prone hardware operations in the system -- multi-decade-minute BFB copies, Redfish firmware uploads and upgrade tasks, power and reset sequences -- and none of it is countable: the code literally says "Someday, we should generate metrics on how often this happens". Someday is now.
carbide_preingestion_bfb_copy_duration_seconds{outcome}-- a histogram over the ~30-minute copy, withok,error, andtimeoutoutcomes; the event owns the historical completion lines (INFO on success, ERROR otherwise). A copy that outlives the 35-minute give-up counts exactly once: a ready result is consumed before the deadline check, and a timed-out copy's late result drops uncounted -- timeout stays its only record.carbide_preingestion_firmware_upgrade_tasks_total{firmware, final_state, outcome}-- one count per Redfish upgrade task reaching a terminal state, emitted only after the transition commits; a chain step stuck behind a failing next-artifact upload no longer re-counts every manager pass, and Dell's vanished-task-but-version-matches path now counts as the completion it is.carbide_preingestion_firmware_upload_total{method, outcome}-- uploads bysimple_update,multipart, andhttp_push, counting real wire attempts (a multipart NotSupported followed by the http-push fallback is two truthful counts).carbide_preingestion_power_control_total{operation, outcome}-- all fifteen power, BMC-reset, and chassis-reset call sites through one wrapper. An HTTP 409 counts asokonly for operations that target a power state (already in the requested state); for restarts, powercycles, and resets -- which have no state to already be in -- it counts as the refusal it is.Notable details:
log = off) sit beside untouched error/warn lines, and the two dynamic-level events own their historical lines at the historical levels.Tests added!
This supports #3174