feat: allow the ability to supress some DCGM error codes#1450
Merged
XRFXLP merged 3 commits intoJul 3, 2026
Conversation
Contributor
📝 WalkthroughWalkthroughAdds configurable DCGM error-code suppression to gpu-health-monitor: Helm values, config rendering, and docs define suppressedErrorCodes; the CLI passes it into DCGMWatcher, which suppresses matching incidents and records a new metric; tests cover the suppression paths. ChangesDCGM Incident Suppression
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant DCGMWatcher
participant Metrics
CLI->>DCGMWatcher: DCGMWatcher(suppressed_error_codes)
DCGMWatcher->>DCGMWatcher: _perform_health_check(incident)
DCGMWatcher->>DCGMWatcher: resolve error_code
alt error_code in suppressed_error_codes
DCGMWatcher->>Metrics: increment dcgm_health_check_suppressed_incidents
DCGMWatcher->>DCGMWatcher: remove matching failures
else not suppressed
DCGMWatcher->>DCGMWatcher: publish health details
end
Related issues: Suggested labels: enhancement, gpu-health-monitor Suggested reviewers: none specified 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
🌿 Fern Docs Preview: https://nvidia-preview-pull-request-1450.docs.buildwithfern.com/nvsentinel |
![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: 1
🧹 Nitpick comments (2)
health-monitors/gpu-health-monitor/gpu_health_monitor/cli.py (1)
168-176: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider normalizing/validating suppressed error codes against known DCGM codes.
suppressed_error_codesis compared downstream via exact-string match against canonical DCGM error code names (e.g.DCGM_FR_CLOCK_THROTTLE_POWER, uppercase). If an operator enters a code with different casing or a typo in the configmap, suppression silently no-ops with no warning — the exact failure mode this feature is meant to prevent (noisy, non-actionable events keep flowing). Consider validating entries against the known error-code set (dcgm_errors_info_dict, populated above from the CSV mapping) and logging a warning for unrecognized codes.♻️ Proposed enhancement
suppressed_error_codes = frozenset() if config.has_section("dcgmhealthcheck"): suppressed_error_codes_raw = config["dcgmhealthcheck"].get("SuppressedErrorCodes", fallback="") suppressed_error_codes = frozenset( code.strip() for code in suppressed_error_codes_raw.split(",") if code.strip() ) if suppressed_error_codes: log.info(f"DCGM error codes suppressed via config: {sorted(suppressed_error_codes)}") + known_error_codes = set(dcgm_errors_info_dict.values()) + unknown_codes = suppressed_error_codes - known_error_codes + if unknown_codes: + log.warning(f"SuppressedErrorCodes contains unrecognized DCGM error codes: {sorted(unknown_codes)}")🤖 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 `@health-monitors/gpu-health-monitor/gpu_health_monitor/cli.py` around lines 168 - 176, The SuppressedErrorCodes parsing in cli.py accepts raw strings but never validates them against the canonical DCGM codes, so typos or casing mismatches silently fail. Update the suppression handling around the suppressed_error_codes block to normalize entries as needed and check them against dcgm_errors_info_dict (or the same known-code set used downstream), then log a warning for any unrecognized codes while still preserving valid ones.health-monitors/gpu-health-monitor/gpu_health_monitor/dcgm_watcher/dcgm.py (1)
70-93: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider validating suppressed error codes against known DCGM codes.
self._suppressed_error_codesis stored and logged, but never cross-checked againstself._error_codes(built at Line 93). A typo or renamed error code in the Helm config would silently result in zero suppression, defeating the purpose of this feature (avoiding datastore noise) without any operator-visible signal.♻️ Proposed validation after error codes are loaded
self._error_codes = self._get_available_error_codes() log.debug(f"Got available error codes {self._error_codes}") + + if self._suppressed_error_codes: + unknown_suppressed = self._suppressed_error_codes - set(self._error_codes.values()) + if unknown_suppressed: + log.warning(f"Configured suppressed error codes not recognized by DCGM: {sorted(unknown_suppressed)}")🤖 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 `@health-monitors/gpu-health-monitor/gpu_health_monitor/dcgm_watcher/dcgm.py` around lines 70 - 93, Validate the configured suppressed DCGM error codes against the known codes loaded in DcgmWatcher._get_available_error_codes(). In the DcgmWatcher initialization flow, after self._error_codes is populated, compare self._suppressed_error_codes to the known set, log a warning for any unknown entries, and only retain valid codes for suppression so typos or renamed Helm values are surfaced instead of silently ignored.
🤖 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/configuration/gpu-health-monitor.md`:
- Around line 133-154: The markdown in the DCGM health check section skips a
heading level by going from the `DCGM Health Check Incident Suppression` section
heading directly to `####` subsections. Update the headings in this block so
they progress by only one level at a time, using the existing section title and
the `suppressedErrorCodes` / `Example: Suppress power-cap throttle flaps`
headings as the anchors to adjust.
---
Nitpick comments:
In `@health-monitors/gpu-health-monitor/gpu_health_monitor/cli.py`:
- Around line 168-176: The SuppressedErrorCodes parsing in cli.py accepts raw
strings but never validates them against the canonical DCGM codes, so typos or
casing mismatches silently fail. Update the suppression handling around the
suppressed_error_codes block to normalize entries as needed and check them
against dcgm_errors_info_dict (or the same known-code set used downstream), then
log a warning for any unrecognized codes while still preserving valid ones.
In `@health-monitors/gpu-health-monitor/gpu_health_monitor/dcgm_watcher/dcgm.py`:
- Around line 70-93: Validate the configured suppressed DCGM error codes against
the known codes loaded in DcgmWatcher._get_available_error_codes(). In the
DcgmWatcher initialization flow, after self._error_codes is populated, compare
self._suppressed_error_codes to the known set, log a warning for any unknown
entries, and only retain valid codes for suppression so typos or renamed Helm
values are surfaced instead of silently ignored.
🪄 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: 05e17b40-9419-460d-947b-90d2ff1e3cd2
📒 Files selected for processing (9)
distros/kubernetes/nvsentinel/charts/gpu-health-monitor/templates/configmap.yamldistros/kubernetes/nvsentinel/charts/gpu-health-monitor/values.yamldocs/configuration/gpu-health-monitor.mddocs/gpu-health-monitor.mdhealth-monitors/gpu-health-monitor/gpu_health_monitor/cli.pyhealth-monitors/gpu-health-monitor/gpu_health_monitor/dcgm_watcher/dcgm.pyhealth-monitors/gpu-health-monitor/gpu_health_monitor/dcgm_watcher/metrics.pyhealth-monitors/gpu-health-monitor/gpu_health_monitor/tests/conftest.pyhealth-monitors/gpu-health-monitor/gpu_health_monitor/tests/test_dcgm_watcher/test_dcgm.py
XRFXLP
reviewed
Jul 3, 2026
deesharma24
reviewed
Jul 3, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
health-monitors/gpu-health-monitor/gpu_health_monitor/tests/test_dcgm_watcher/test_dcgm.py (1)
386-507: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSuppression tests don't verify the new metric is recorded.
The suppression logic in
dcgm.py's_suppress_configured_error_codesincrementsmetrics.dcgm_health_check_suppressed_incidents.labels(error_code).inc()for each suppressed entity, per the upstream snippet. None of the four new suppression tests assert this metric fires (or is skipped in the no-op case). Since observability of suppressed incidents is a core objective of this PR (avoiding silent data loss), consider adding an assertion that mocks/patchesmetrics.dcgm_health_check_suppressed_incidentsand checks it's incremented with the expected error-code label in the positive cases, and not touched in the no-op case.🧪 Example addition for one test
+ `@patch`("gpu_health_monitor.dcgm_watcher.dcgm.metrics.dcgm_health_check_suppressed_incidents") + def test_suppress_configured_error_codes_clears_matching_dcgm_watch(self, mock_metric): ... watcher._suppress_configured_error_codes(health_status) expected_response = watcher._get_health_status_dict() assert health_status == expected_response + mock_metric.labels.assert_called_once_with("DCGM_FR_CLOCK_THROTTLE_POWER") + mock_metric.labels.return_value.inc.assert_called_once()🤖 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 `@health-monitors/gpu-health-monitor/gpu_health_monitor/tests/test_dcgm_watcher/test_dcgm.py` around lines 386 - 507, The new suppression tests in DCGMWatcher are only checking health_status changes and miss the metrics side effect. Update the tests around DCGMWatcher._suppress_configured_error_codes to mock or patch metrics.dcgm_health_check_suppressed_incidents and assert it is incremented with the suppressed error-code label in the matching suppression cases, while asserting no metric increment happens in test_suppress_configured_error_codes_noop_by_default. Use the existing test methods and the _suppress_configured_error_codes path to keep the assertions tied to the observed suppression behavior.
🤖 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
`@health-monitors/gpu-health-monitor/gpu_health_monitor/tests/test_dcgm_watcher/test_dcgm.py`:
- Around line 386-507: The new suppression tests in DCGMWatcher are only
checking health_status changes and miss the metrics side effect. Update the
tests around DCGMWatcher._suppress_configured_error_codes to mock or patch
metrics.dcgm_health_check_suppressed_incidents and assert it is incremented with
the suppressed error-code label in the matching suppression cases, while
asserting no metric increment happens in
test_suppress_configured_error_codes_noop_by_default. Use the existing test
methods and the _suppress_configured_error_codes path to keep the assertions
tied to the observed suppression behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 77a17d36-92a5-4aa6-af48-a7bdc793f16b
📒 Files selected for processing (3)
distros/kubernetes/nvsentinel/values-full.yamlhealth-monitors/gpu-health-monitor/gpu_health_monitor/dcgm_watcher/dcgm.pyhealth-monitors/gpu-health-monitor/gpu_health_monitor/tests/test_dcgm_watcher/test_dcgm.py
🚧 Files skipped from review as they are similar to previous changes (1)
- health-monitors/gpu-health-monitor/gpu_health_monitor/dcgm_watcher/dcgm.py
Contributor
|
LGTM!! |
XRFXLP
approved these changes
Jul 3, 2026
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
3 participants
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
Currently we don't have a way to supress specific DCGM error codes and hence all events including the ones that are non-fatal and may not be actionable will get persisted into the database. This creates noise in the datastore and in some cases may cause other unintended side effects
Testing
Validated by injecting a DCGM error before the change
and noticed that the power event was shown
to validate suppression, I updated the configmap to have:
and injected the same error again and I noticed that the event wasn't fired this time confirming that the error was supressed.
Type of Change
Component(s) Affected
Testing
Checklist
Closes #1447
Summary by CodeRabbit
dcgmHealthCheck.suppressedErrorCodesconfiguration option and added example values.PASSwhen all their failures are suppressed.