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
|
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 (13)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (10)
Summary by CodeRabbit
WalkthroughAdds GPU inventory validation from 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
from
b638905 to
d78ae6e
Compare
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
d78ae6e to
89238f7
Compare
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