feat(hw-health): out-of-band GPU count & SEL fault alerting#3368
Open
hatamzad-nv wants to merge 1 commit into
Open
feat(hw-health): out-of-band GPU count & SEL fault alerting#3368hatamzad-nv wants to merge 1 commit into
hatamzad-nv wants to merge 1 commit into
Conversation
Contributor
|
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:
Summary by CodeRabbit
WalkthroughAdds GPU inventory validation using live SKU and BMC data, GPU fault log processing, health-report types, configuration validation, collector discovery wiring, API access, tests, and RBAC permission support. ChangesGPU health monitoring
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant DiscoveryLoop
participant GpuInventoryCollector
participant ApiClientWrapper
participant BmcRedfish
participant HealthReportSink
DiscoveryLoop->>GpuInventoryCollector: start for enabled machine endpoint
GpuInventoryCollector->>ApiClientWrapper: resolve assigned SKU
GpuInventoryCollector->>BmcRedfish: count chassis and processor GPUs
GpuInventoryCollector->>HealthReportSink: emit shortage or success report
sequenceDiagram
participant CollectorEvent
participant GpuFaultEventProcessor
participant HealthReportSink
CollectorEvent->>GpuFaultEventProcessor: deliver log event
GpuFaultEventProcessor->>GpuFaultEventProcessor: filter severity and GPU keywords
GpuFaultEventProcessor->>HealthReportSink: emit GPU fault alert
🚥 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
crates/health/src/processor/gpu_events.rs (2)
92-106: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover both alert-target branches.
Current tests always use empty attributes, so the
entry_id→GPU/{id}mapping is never exercised, and theHostBMCfallback is not asserted. Add focused cases that verify both target values.🤖 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/health/src/processor/gpu_events.rs` around lines 92 - 106, The tests for GPU fault report targets only cover empty attributes and do not verify either mapping branch. Add focused tests around the GPU fault report processing logic that provide an entry_id and assert the alert target is formatted as GPU/{id}, and provide no entry_id and assert the fallback target is HostBMC.Source: Coding guidelines
168-179: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse repository table-driven test helpers.
These tests repeat
process(...)with multiple inputs and manually loop over cases. Prefervalue_scenarios!orcheck_valuesso each severity and keyword variant is labeled and reported independently.As per coding guidelines, prefer table-driven tests for operations with multiple input variants.
Also applies to: 191-206
🤖 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/health/src/processor/gpu_events.rs` around lines 168 - 179, The tests in ignores_non_actionable_severity and alerts_on_warning_severity should use the repository’s table-driven test helpers instead of repeated assertions. Refactor the severity variants into labeled value_scenarios! or check_values cases so each input is evaluated and reported independently, preserving the existing expected alert behavior.Source: Coding guidelines
crates/health/src/discovery/spawn.rs (1)
387-425: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing success log on collector start — inconsistent with sibling blocks.
Every other collector-start block in this function (
Discovery,Sensor,Metrics,Firmware,LeakDetector) logs atracing::info!on theOk(monitor)branch withendpoint_keyand a running total (e.g. lines 107-114, 150-158, 192-199, 368-376, 449-457). The new GPU inventory block silently inserts the collector without any success log, which weakens observability parity when diagnosing rollout/startup behavior for this feature.♻️ Suggested fix
Ok(monitor) => { ctx.collectors .insert(CollectorKind::GpuInventory, key.clone().into(), monitor); + tracing::info!( + endpoint_key = %key, + total_collectors = ctx.collectors.len(CollectorKind::GpuInventory), + "Started GPU inventory collection for BMC endpoint" + ); }🤖 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/health/src/discovery/spawn.rs` around lines 387 - 425, Add a success-level tracing::info! call in the Ok(monitor) branch of the GPU inventory Collector::start block, matching the logging pattern used by Discovery, Sensor, Metrics, Firmware, and LeakDetector, including the endpoint key and current running collector total after insertion.
🤖 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/health/src/collectors/gpu_inventory.rs`:
- Around line 247-269: Align the entity_count semantics in run_iteration: when
handling Expected::SkuMissing, do not report a measured GPU count because
Redfish was not queried. Return entity_count: None, matching the Expected::NoSku
and Expected::Count(0) branches while preserving the missing-SKU alert.
In `@crates/health/src/processor/gpu_events.rs`:
- Around line 29-35: The GPU fault keyword matching in GPU_FAULT_KEYWORDS misses
hyphenated “row-remap” messages. Normalize separators before matching or add
both “row remap” and “row-remap” keywords, then add a regression test covering
an “ECC row-remap” payload.
---
Nitpick comments:
In `@crates/health/src/discovery/spawn.rs`:
- Around line 387-425: Add a success-level tracing::info! call in the
Ok(monitor) branch of the GPU inventory Collector::start block, matching the
logging pattern used by Discovery, Sensor, Metrics, Firmware, and LeakDetector,
including the endpoint key and current running collector total after insertion.
In `@crates/health/src/processor/gpu_events.rs`:
- Around line 92-106: The tests for GPU fault report targets only cover empty
attributes and do not verify either mapping branch. Add focused tests around the
GPU fault report processing logic that provide an entry_id and assert the alert
target is formatted as GPU/{id}, and provide no entry_id and assert the fallback
target is HostBMC.
- Around line 168-179: The tests in ignores_non_actionable_severity and
alerts_on_warning_severity should use the repository’s table-driven test helpers
instead of repeated assertions. Refactor the severity variants into labeled
value_scenarios! or check_values cases so each input is evaluated and reported
independently, preserving the existing expected alert behavior.
🪄 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: a666367c-e6b2-4bc3-9fbd-aae405311c89
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
crates/api-core/src/auth/internal_rbac_rules.rscrates/health/Cargo.tomlcrates/health/example/config.example.tomlcrates/health/src/api_client.rscrates/health/src/collectors/gpu_inventory.rscrates/health/src/collectors/mod.rscrates/health/src/config.rscrates/health/src/discovery/context.rscrates/health/src/discovery/spawn.rscrates/health/src/lib.rscrates/health/src/processor/gpu_events.rscrates/health/src/processor/mod.rscrates/health/src/sink/events.rs
hatamzad-nv
force-pushed
the
feat/301-oob-gpu-detection
branch
2 times, most recently
from
d78ae6e to
89238f7
Compare
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
hatamzad-nv
force-pushed
the
feat/301-oob-gpu-detection
branch
from
89238f7 to
e4bfbdb
Compare
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3368.docs.buildwithfern.com/infra-controller |
Detect GPUs missing out-of-band (issue NVIDIA#301): a GPU-count-vs-SKU collector and a SEL GPU-fault processor in hw-health. GPU counting is vendor-agnostic: max(HGX_GPU_* chassis, ProcessorType=GPU processors) vs the SKU total. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Amir Hatamzad <ahatamzad@nvidia.com>
hatamzad-nv
force-pushed
the
feat/301-oob-gpu-detection
branch
from
e4bfbdb to
00f5a2e
Compare
Contributor
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/health/src/config.rs`:
- Around line 692-706: Update Config::validate() to reject a zero
GpuInventoryConfig.interval, including the corresponding externally configurable
interval fields noted by the comment, while preserving valid positive durations.
Add a regression test covering a 0s interval and asserting validation fails.
In `@crates/health/src/processor/gpu_events.rs`:
- Around line 46-53: The GpuFaultEventProcessor retains active GPU faults
indefinitely, causing resolved transient faults to remain in later reports.
Update the processing logic around GpuFaultEventProcessor and its event-handling
methods to reconcile or explicitly remove faults when non-actionable/OK events
indicate resolution, while preserving accumulation of still-active faults; add a
regression test verifying that a resolved fault is absent from subsequent
reports.
🪄 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: d0bfb0d9-6769-433a-bbd4-51b2b134d3d9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
crates/api-core/src/auth/internal_rbac_rules.rscrates/health/Cargo.tomlcrates/health/example/config.example.tomlcrates/health/src/api_client.rscrates/health/src/collectors/gpu_inventory.rscrates/health/src/collectors/mod.rscrates/health/src/config.rscrates/health/src/discovery/context.rscrates/health/src/discovery/spawn.rscrates/health/src/lib.rscrates/health/src/processor/gpu_events.rscrates/health/src/processor/mod.rscrates/health/src/sink/events.rs
✅ Files skipped from review due to trivial changes (1)
- crates/health/example/config.example.toml
🚧 Files skipped from review as they are similar to previous changes (9)
- crates/health/src/collectors/mod.rs
- crates/api-core/src/auth/internal_rbac_rules.rs
- crates/health/src/processor/mod.rs
- crates/health/Cargo.toml
- crates/health/src/api_client.rs
- crates/health/src/sink/events.rs
- crates/health/src/collectors/gpu_inventory.rs
- crates/health/src/discovery/spawn.rs
- crates/health/src/discovery/context.rs
Comment on lines
+692
to
+706
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| #[serde(default)] | ||
| pub struct GpuInventoryConfig { | ||
| #[serde(with = "humantime_serde")] | ||
| pub interval: Duration, | ||
| } | ||
|
|
||
| impl Default for GpuInventoryConfig { | ||
| fn default() -> Self { | ||
| Self { | ||
| interval: Duration::from_secs(300), | ||
| } | ||
| } | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Reject zero GPU inventory intervals.
interval is externally configurable, but Config::validate() does not reject 0s. Because this value drives periodic collection, zero can create a tight loop and flood the BMC/API. Add validation and a regression case.
Suggested validation
+ if let Configurable::Enabled(gpu_inventory) = &self.collectors.gpu_inventory
+ && gpu_inventory.interval.is_zero()
+ {
+ return Err(
+ "collectors.gpu_inventory.interval must be greater than 0".to_string(),
+ );
+ }Also applies to: 1473-1496, 1792-1811
🤖 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/health/src/config.rs` around lines 692 - 706, Update
Config::validate() to reject a zero GpuInventoryConfig.interval, including the
corresponding externally configurable interval fields noted by the comment,
while preserving valid positive durations. Add a regression test covering a 0s
interval and asserting validation fails.
Comment on lines
+46
to
+53
| /// Faults accumulate per machine: each event re-emits that machine's full active | ||
| /// fault set, so a burst of distinct faults is preserved in a single report | ||
| /// rather than each event overwriting the previous same-source report in the | ||
| /// sink. Clearing resolved faults is tracked as a follow-up. | ||
| #[derive(Default)] | ||
| pub struct GpuFaultEventProcessor { | ||
| active: Mutex<HashMap<String, BTreeMap<String, HealthReportAlert>>>, | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not retain GPU faults forever.
The processor only inserts or replaces entries in active; it never removes them. Since every alert is classified as PreventAllocations, a transient fault remains in every later report and permanently fences the machine. Non-actionable/OK events are ignored, so they cannot clear the state. Add explicit resolution, reconciliation, or TTL handling, plus a regression test for clearing faults.
Also applies to: 133-139, 267-285
🤖 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/health/src/processor/gpu_events.rs` around lines 46 - 53, The
GpuFaultEventProcessor retains active GPU faults indefinitely, causing resolved
transient faults to remain in later reports. Update the processing logic around
GpuFaultEventProcessor and its event-handling methods to reconcile or explicitly
remove faults when non-actionable/OK events indicate resolution, while
preserving accumulation of still-active faults; add a regression test verifying
that a resolved fault is absent from subsequent reports.
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
1 participant
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.
Summary
Implements out-of-band detection of missing GPUs, addressing #301.
Today SKU validation only runs in-band and at limited points in the host
lifecycle, so a GPU that drops off the BMC can go unnoticed. This adds two
complementary out-of-band detections to the
hw-healthservice.What's included
1.
GpuInventoryCollector— GPU count vs. assigned SKUPeriodically counts a host's GPUs out-of-band via Redfish and raises a health
alert when the count is below the machine's assigned SKU total.
Counting is vendor-agnostic — GPUs attach in two forms, so we count both and
take the max:
HGX_GPU_*chassis per GPU
ProcessorswithProcessorType=GPUmax(chassis, processors)(not sum) because some platforms (e.g. GB200) exposethe same GPUs both ways. There is no BMC-vendor table to maintain — it works
across Dell / Lenovo / Supermicro / NVIDIA regardless of GPU form.
2.
GpuFaultEventProcessor— SEL GPU-fault alertingWatches the Redfish SEL / log stream and raises alerts on GPU-related faults
(NVLink, ECC row-remap, XID, …). Complements the count check by catching GPUs
that are present but faulting.
Both reuse the existing collector → sink → API pipeline, and use distinct
report sources so inventory and fault alerts don't overwrite each other.
Safety / rollout
[collectors.gpu_inventory](documentedin
config.example.toml).health-report sink is rejected at startup.
Healthrole permission forFindSkusByIds(needed toresolve the expected count from the SKU).
Testing
cargo test -p carbide-health— 384 pass8 GPUs via
ProcessorType=GPU— confirms the processor path on real PCIe hardware.cargo fmt --all -- --checkclean.Known follow-ups (non-blocking)
deliberate decision (see below).
PCIeDevices(neither chassis nor processors) they'd count as 0. Not seen on any current
fleet platform (Lenovo L40 uses
ProcessorType=GPU, confirmed above); a PCIesource can be added if such firmware appears.
SEL samples.
Open decision for reviewers
Ships disabled by default for a safe rollout. Should it be enabled in
Helm/Kustomize now, or in a follow-up PR after a canary?
Checklist
cargo fmt --checkcleanmainSigned-off-by)clippy-flow— runs on CI (Linux only)cc @williampnvidia for review