demos: standalone system-services-monitor demo - #1383
Conversation
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughThe PR adds a standalone system-services monitor. It checks Fabric Manager and NVIDIA services through host systemd and journal interfaces, exports Prometheus metrics, and provides container and Kubernetes deployment resources with alert rules. ChangesSystem Services Monitor
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🔵 Low · up to The demo adds a Fabric Manager flapping alert, but its restart counter is not yet produced by the dependent implementation, so the alert will remain silent until that implementation lands. This is a bounded integration risk requiring owner follow-up before merging the series. Sequence Diagram(s)sequenceDiagram
participant SystemServicesMonitor
participant ServiceChecker
participant HostServices
participant Prometheus
SystemServicesMonitor->>ServiceChecker: Run configured service checks
ServiceChecker->>HostServices: Query systemd and journal state
HostServices-->>ServiceChecker: Return service statuses and errors
ServiceChecker-->>SystemServicesMonitor: Return health results
SystemServicesMonitor->>Prometheus: Update monitoring metrics
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@dmvevents this PR has been inactive for 14 days. Do you need help finishing it, or should we close it for now? Feel free to reopen anytime. |
Lands the demo subtree (NVIDIA#891 split, 5 of 5). Standalone — depends on the implementation in <impl-pr-num> but doesn't gate any production functionality. Includes CR-ignored fixes from the umbrella PR: - daemonset.yaml: image pinned to SHA tag (not :0.1.0) - daemonset.yaml: livenessProbe httpGet validated - servicemonitor.yaml: FabricManagerFlapping alert spec - Dockerfile: python:3.13 base for CVE remediation Signed-off-by: Anton Alexander <dmvevents@users.noreply.github.com>
…validation The demo re-bundled CUDA context/memory validation as a polling daemon check — the exact model deesharma24 rejected on NVIDIA#891, where a long-running daemon contends for GPU memory with active workloads. That review is why CUDA validation re-landed as a preflight init-container (NVIDIA#1384), which runs once before workloads schedule. Remove the daemon check from this demo to avoid contradicting the accepted design: delete checks/cuda_validation.py and drop all references from monitor.py, config.py, metrics.py, the k8s ConfigMap/PrometheusRule, and the tests. README now points at preflight-checks/cuda-validation/ (NVIDIA#1384) as the supported form. Also note in the DaemonSet that the system-services-monitor:0.1.0 tag is a demo-local pin, independent of the Helm chart appVersion. Signed-off-by: Anton Alexander <dmvevents@users.noreply.github.com>
The system-services-monitor design doc is ADR-049; ADR-030 upstream is the unrelated grpc-tls-authentication design. Corrects the scope docstring only. Signed-off-by: Anton Alexander <dmvevents@users.noreply.github.com>
9d55deb to
28ed8ad
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
demos/system-services-monitor/tests/test_service_check.py (1)
25-160: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd type annotations to the test functions.
The helper, test methods, and nested
side_effectfunction lack parameter or return annotations. Add annotations to satisfy the Python code requirement.As per coding guidelines, “Include type hints for all functions in Python code.”
🤖 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 `@demos/system-services-monitor/tests/test_service_check.py` around lines 25 - 160, Add type annotations to _mock_systemctl_output, all test_* methods, and the nested side_effect function, including parameter and return types; use appropriate pytest/mock and subprocess-related types without changing test behavior.Source: Coding guidelines
🤖 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 `@demos/system-services-monitor/checks/service_check.py`:
- Around line 1-5: Add the repository-standard Apache 2.0 license header to
demos/system-services-monitor/checks/service_check.py lines 1-5 and
demos/system-services-monitor/tests/test_service_check.py lines 1-1 before their
module docstrings. Add the standard YAML comment header to
demos/system-services-monitor/k8s/daemonset.yaml lines 1-1 and rbac.yaml lines
1-1 before apiVersion, and to servicemonitor.yaml lines 1-1 before its existing
comment.
- Around line 46-55: Add the repository-standard Apache 2.0 license header at
the top of the module, then run Black on service_check.py so inline comments and
long calls use the formatter’s canonical layout while preserving the
ServiceStatus fields and behavior.
In `@demos/system-services-monitor/config.py`:
- Line 46: Validate the CHECK_INTERVAL environment value before constructing
MonitorConfig, rejecting zero or negative values with an appropriate error; only
pass a strictly positive integer to the check_interval field.
- Around line 29-37: Separate Fabric Manager handling from generic GPU-service
monitoring: in demos/system-services-monitor/config.py lines 29-37, remove
nvidia-fabricmanager from the default gpu_services list; in
demos/system-services-monitor/monitor.py lines 92-123, run the gpu_services
checks independently of enable_fabric_check so disabling Fabric Manager checks
does not disable nvidia-persistenced monitoring.
- Around line 1-5: Prepend the standard Apache 2.0 license header before the
module docstrings or manifest content in demos/system-services-monitor/config.py
lines 1-5, demos/system-services-monitor/metrics.py lines 1-5,
demos/system-services-monitor/monitor.py lines 1-10,
demos/system-services-monitor/tests/test_monitor.py lines 1-1, and
demos/system-services-monitor/k8s/configmap.yaml lines 1-10; leave the existing
content unchanged after each header.
- Around line 16-37: Run Black with a 120-character line length on both
demos/system-services-monitor/config.py (lines 16-37) and
demos/system-services-monitor/monitor.py (lines 59-60), formatting the aligned
comments, gpu_services declaration, logging calls, and manually wrapped calls
without changing behavior.
In `@demos/system-services-monitor/Dockerfile`:
- Line 1: Update the Dockerfile’s Python base image reference from the mutable
python:3.13-slim tag to the corresponding immutable image digest, preserving the
Python 3.13 slim base while ensuring reproducible rebuilds.
In `@demos/system-services-monitor/k8s/rbac.yaml`:
- Around line 20-23: Update the RBAC manifest to retain only the ServiceAccount,
removing the ClusterRole and ClusterRoleBinding along with the unused node-read
permissions. Add the required Apache 2.0 license header at the beginning of
rbac.yaml.
In `@demos/system-services-monitor/k8s/servicemonitor.yaml`:
- Around line 59-61: Update the FabricManagerFlapping alert expression
associated with alert name FabricManagerFlapping to trigger when
fabric_manager_restarts_total increases by at least three within 10 minutes,
matching ServiceChecker.is_flapping’s default threshold; alternatively, source
both thresholds from the same configuration.
In `@demos/system-services-monitor/metrics.py`:
- Around line 23-27: Update metrics.py’s fabric_manager_restarts_total handling
and monitor.py’s fm_status processing to retain the previous n_restarts value,
skip incrementing on the first observation, and increment the Counter only by
positive restart-count deltas; leave the Counter unchanged when the count
decreases.
In `@demos/system-services-monitor/monitor.py`:
- Around line 124-126: Update the exception handler in the service-check flow to
set overall_healthy to False when a check fails after the boot grace period,
while preserving the existing grace-period behavior. Use the surrounding
overall_healthy and boot-grace logic to ensure gpu_node_health_up reports
unhealthy only once grace has elapsed.
In `@demos/system-services-monitor/README.md`:
- Around line 59-63: Update the GPUServiceDown entry in the ServiceMonitor alert
list to document its severity as warning, matching the severity configured in
k8s/servicemonitor.yaml; leave the alert name and 3m duration unchanged.
- Around line 25-27: Align the image tag in the README build command with the
tag referenced by the DaemonSet in k8s/daemonset.yaml. Use one consistent tag,
preferably system-services-monitor:0.1.0, across the build instructions and
deployment manifest.
- Around line 40-41: Update the POD lookup command’s kubectl label selector to
use the DaemonSet label app.kubernetes.io/name=system-services-monitor instead
of app=system-services-monitor, while preserving the existing namespace, node,
and jsonpath filters.
In `@demos/system-services-monitor/tests/test_service_check.py`:
- Around line 37-41: Run Black on the entire test_service_check.py module so
every subprocess.CompletedProcess invocation, including mock_run.return_value,
uses Black’s required multiline argument formatting.
---
Outside diff comments:
In `@demos/system-services-monitor/tests/test_service_check.py`:
- Around line 25-160: Add type annotations to _mock_systemctl_output, all test_*
methods, and the nested side_effect function, including parameter and return
types; use appropriate pytest/mock and subprocess-related types without changing
test behavior.
🪄 Autofix
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: 8aac5a0b-36fc-4033-bfca-5d3c1287573d
📒 Files selected for processing (15)
demos/system-services-monitor/Dockerfiledemos/system-services-monitor/README.mddemos/system-services-monitor/checks/__init__.pydemos/system-services-monitor/checks/service_check.pydemos/system-services-monitor/config.pydemos/system-services-monitor/k8s/configmap.yamldemos/system-services-monitor/k8s/daemonset.yamldemos/system-services-monitor/k8s/rbac.yamldemos/system-services-monitor/k8s/servicemonitor.yamldemos/system-services-monitor/metrics.pydemos/system-services-monitor/monitor.pydemos/system-services-monitor/requirements.txtdemos/system-services-monitor/tests/__init__.pydemos/system-services-monitor/tests/test_monitor.pydemos/system-services-monitor/tests/test_service_check.py
| enable_fabric_check: bool = True | ||
|
|
||
| # Services to monitor (besides fabric manager). | ||
| # nv-hostengine is monitored by gpu-health-monitor via | ||
| # GpuDcgmConnectivityFailure; not duplicated here. | ||
| gpu_services: list = field(default_factory=lambda: [ | ||
| "nvidia-fabricmanager", | ||
| "nvidia-persistenced", | ||
| ]) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Separate Fabric Manager control from generic GPU-service checks.
gpu_services is documented as services besides Fabric Manager, but it includes nvidia-fabricmanager. The monitor also nests all GPU-service checks under enable_fabric_check. The default configuration checks Fabric Manager twice, and disabling Fabric Manager checks also disables nvidia-persistenced monitoring.
demos/system-services-monitor/config.py#L29-L37: removenvidia-fabricmanagerfrom the generic GPU-service default list, or revise the configuration contract.demos/system-services-monitor/monitor.py#L92-L123: run generic GPU-service checks independently fromenable_fabric_check.
📍 Affects 2 files
demos/system-services-monitor/config.py#L29-L37(this comment)demos/system-services-monitor/monitor.py#L92-L123
🤖 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 `@demos/system-services-monitor/config.py` around lines 29 - 37, Separate
Fabric Manager handling from generic GPU-service monitoring: in
demos/system-services-monitor/config.py lines 29-37, remove nvidia-fabricmanager
from the default gpu_services list; in demos/system-services-monitor/monitor.py
lines 92-123, run the gpu_services checks independently of enable_fabric_check
so disabling Fabric Manager checks does not disable nvidia-persistenced
monitoring.
There was a problem hiding this comment.
Confirmed both halves. gpu_services is commented as services besides fabric manager yet lists nvidia-fabricmanager first (config.py:34), so check_fabric_manager() and check_all_gpu_services() both probe it every cycle. And the whole block is nested under if self.config.enable_fabric_check (monitor.py:92), so ENABLE_FABRIC_CHECK=false silently stops monitoring nvidia-persistenced too — the config contract says nothing about that coupling. This is a real design bug, not a nit, so I am leaving it OPEN rather than patching the fork; it is also one of the things that disappears cleanly if the demo moves to the #1382 package. Flagging for the direction call in the PR comment.
There was a problem hiding this comment.
@dmvevents, agreed. The configuration contract and runtime behavior conflict.
The finding should remain open until the demo is removed or the monitor separates Fabric Manager checks from generic GPU-service checks. No code change is claimed in this thread.
🧠 Learnings used
Learnt from: dmvevents
Repo: NVIDIA/NVSentinel PR: 1383
File: demos/system-services-monitor/checks/service_check.py:1-5
Timestamp: 2026-08-22T10:44:42.920Z
Learning: If the standalone demos/system-services-monitor probe is retained, every Python file in this directory must include the repository-required Apache 2.0 license header. If the demo is deleted in favor of the shipped package, this guideline no longer applies.
You are interacting with an AI system.
| except Exception: | ||
| logger.exception("Service check failed") | ||
| health_check_errors_total.labels("services").inc() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Mark a failed service check as unhealthy after boot grace.
When this block catches an exception, overall_healthy remains True. After boot grace, gpu_node_health_up reports healthy even though the monitor could not inspect host services. Set it to unhealthy when the check fails outside the grace period.
🤖 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 `@demos/system-services-monitor/monitor.py` around lines 124 - 126, Update the
exception handler in the service-check flow to set overall_healthy to False when
a check fails after the boot grace period, while preserving the existing
grace-period behavior. Use the surrounding overall_healthy and boot-grace logic
to ensure gpu_node_health_up reports unhealthy only once grace has elapsed.
There was a problem hiding this comment.
Real, and it fails in the unsafe direction. The except Exception at monitor.py:126 logs and increments health_check_errors_total but leaves overall_healthy at True, so past boot grace the node publishes gpu_node_health_up=1 when the monitor could not inspect host services at all — a broken nsenter/dbus mount reads as a healthy node. Same class as the journal-probe thread: a failed probe must be UNKNOWN, never healthy. Leaving OPEN for the same reason as the other functional findings; it needs the probe-error plumbing, not a one-line flag flip, and #1382's ServiceStatus is the right place for that contract.
There was a problem hiding this comment.
@dmvevents, agreed. The finding remains open.
A service-check failure must produce an unknown or unhealthy result after boot grace. The current exception path can report a healthy node without completing host-service inspection.
The ServiceStatus contract in #1382 is the correct place to add the probe-error state. This demo should consume that contract instead of adding a local one-line override.
✏️ Learnings added
Learnt from: dmvevents
Repo: NVIDIA/NVSentinel PR: 1383
File: demos/system-services-monitor/monitor.py:124-126
Timestamp: 2026-08-22T10:52:55.229Z
Learning: For the system-services monitor, a failed host-service probe, such as a broken `nsenter` or host D-Bus mount, must not report the node as healthy after `boot_grace_period`. The probe-error or unknown state should be represented by `ServiceStatus` in implementation PR `#1382`, and the standalone demo should consume that contract.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@demos/system-services-monitor/checks/service_check.py`:
- Around line 94-97: Update the systemctl query in the service-check command to
request LoadState alongside the existing properties, then preserve and inspect
that value in the service status handling. When LoadState is not-found, return
the explicit not-applicable result and skip the unit without emitting
*_NOT_RUNNING or GPU_SERVICE_NOT_RUNNING events; add coverage for absent Fabric
Manager and GPU-support units.
- Around line 209-222: The journal parsing flow around the error-pattern scan
must distinguish probe failures from successful probes with no matches: update
the non-zero return-code/empty-output branch and the `except` handling to return
or raise an explicit probe error, then propagate that failure through
`check_fabric_manager` and `FabricManagerStatus` so the monitor records a check
error, treats the state as UNKNOWN, and suppresses `HealthEvent` emission.
- Line 68: Annotate every function in the affected files: update
ServiceChecker.__init__ in
demos/system-services-monitor/checks/service_check.py:68-68 with int parameters
and a None return type; in
demos/system-services-monitor/tests/test_service_check.py:25-25, type
_mock_systemctl_output’s parameters and return; at 36-36, 51-51, 64-64, 74-74,
119-120, and 144-144, type mock_run, nested side_effect where applicable, and
each test’s return; and at 86-86, 95-95, and 104-104, add None return types.
Preserve existing test behavior and use appropriate types for the mocked
subprocess interfaces and outputs.
🪄 Autofix
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: 3a9451ba-733a-4ab1-ae61-085423a96e95
📒 Files selected for processing (15)
demos/system-services-monitor/Dockerfiledemos/system-services-monitor/README.mddemos/system-services-monitor/checks/__init__.pydemos/system-services-monitor/checks/service_check.pydemos/system-services-monitor/config.pydemos/system-services-monitor/k8s/configmap.yamldemos/system-services-monitor/k8s/daemonset.yamldemos/system-services-monitor/k8s/rbac.yamldemos/system-services-monitor/k8s/servicemonitor.yamldemos/system-services-monitor/metrics.pydemos/system-services-monitor/monitor.pydemos/system-services-monitor/requirements.txtdemos/system-services-monitor/tests/__init__.pydemos/system-services-monitor/tests/test_monitor.pydemos/system-services-monitor/tests/test_service_check.py
🚧 Files skipped from review as they are similar to previous changes (11)
- demos/system-services-monitor/k8s/rbac.yaml
- demos/system-services-monitor/k8s/configmap.yaml
- demos/system-services-monitor/tests/init.py
- demos/system-services-monitor/config.py
- demos/system-services-monitor/metrics.py
- demos/system-services-monitor/checks/init.py
- demos/system-services-monitor/k8s/servicemonitor.yaml
- demos/system-services-monitor/tests/test_monitor.py
- demos/system-services-monitor/monitor.py
- demos/system-services-monitor/README.md
- demos/system-services-monitor/requirements.txt
| result = self._run_host_cmd([ | ||
| "systemctl", "show", service_name, | ||
| "--property=ActiveState,SubState,MainPID,ExecMainStartTimestamp", | ||
| ]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Query and preserve LoadState.
This command does not request LoadState. A missing unit then becomes active=False, which is indistinguishable from a loaded but stopped unit. Downstream code can emit a false *_NOT_RUNNING or GPU_SERVICE_NOT_RUNNING event.
Request LoadState with ActiveState. If LoadState=not-found, return an explicit not-applicable status and skip the unit. Add coverage for absent Fabric Manager and GPU-support units.
Based on learnings, “If LoadState=not-found, it must skip the unit as not applicable and must not emit *_NOT_RUNNING or GPU_SERVICE_NOT_RUNNING.”
🤖 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 `@demos/system-services-monitor/checks/service_check.py` around lines 94 - 97,
Update the systemctl query in the service-check command to request LoadState
alongside the existing properties, then preserve and inspect that value in the
service status handling. When LoadState is not-found, return the explicit
not-applicable result and skip the unit without emitting *_NOT_RUNNING or
GPU_SERVICE_NOT_RUNNING events; add coverage for absent Fabric Manager and
GPU-support units.
Source: Learnings
There was a problem hiding this comment.
Confirmed real, and it is the one genuine divergence from the implementation PR. #1382's service_check.py:133 requests --property=LoadState,ActiveState,SubState,MainPID,ExecMainStartTimestamp and keeps it at :161; this fork asks for the same set minus LoadState (checks/service_check.py:96) and has zero references to it, so not-found and masked are indistinguishable from inactive here. Leaving OPEN deliberately — it is the concrete evidence for the direction proposed in #1383 (comment) (import the shipped package instead of maintaining a second copy that has to re-earn this fix). Not resolving until that call is made.
There was a problem hiding this comment.
@dmvevents, confirmed. The finding remains valid.
The demo copy diverges from #1382 because it does not request or preserve LoadState. The current code cannot distinguish not-found or masked from inactive.
Keeping this thread open is appropriate until the project decides whether to import the shipped package or maintain this duplicate implementation with the required behavior.
✏️ Learnings added
Learnt from: dmvevents
Repo: NVIDIA/NVSentinel PR: 1383
File: demos/system-services-monitor/checks/service_check.py:94-97
Timestamp: 2026-08-22T10:45:10.217Z
Learning: For NVIDIA/NVSentinel PR `#1383`, `demos/system-services-monitor/checks/service_check.py` is a duplicate of the implementation in PR `#1382` and currently diverges by omitting `LoadState` from the `systemctl show` query and status handling. This makes `LoadState=not-found` and `LoadState=masked` indistinguishable from `ActiveState=inactive`. The preferred direction under discussion is to import the shipped package instead of maintaining a second copy that must independently receive behavior fixes.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: dmvevents
Repo: NVIDIA/NVSentinel PR: 1383
File: demos/system-services-monitor/checks/service_check.py:1-5
Timestamp: 2026-08-22T10:44:42.920Z
Learning: If the standalone demos/system-services-monitor probe is retained, every Python file in this directory must include the repository-required Apache 2.0 license header. If the demo is deleted in favor of the shipped package, this guideline no longer applies.
You are interacting with an AI system.
| if result.returncode != 0 or not result.stdout.strip(): | ||
| return [] | ||
|
|
||
| found: List[ErrorCategory] = [] | ||
| text = result.stdout.lower() | ||
| for category, patterns in _ERROR_PATTERNS.items(): | ||
| if any(p.lower() in text for p in patterns): | ||
| found.append(category) | ||
|
|
||
| return found | ||
|
|
||
| except (subprocess.TimeoutExpired, Exception) as e: | ||
| logger.warning("Journal parsing failed for %s: %s", service_name, e) | ||
| return [] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Preserve journal probe failures as UNKNOWN.
A non-zero journalctl result, timeout, or exception returns []. check_fabric_manager then receives a status that is indistinguishable from a successful journal probe with no errors.
Return or raise an explicit probe error. Propagate it through FabricManagerStatus so the monitor can record a check error and suppress HealthEvent emission.
Based on learnings, “A failed or timed-out probe is UNKNOWN state, not service-down.”
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 220-220: Do not catch blind exception: Exception
(BLE001)
🤖 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 `@demos/system-services-monitor/checks/service_check.py` around lines 209 -
222, The journal parsing flow around the error-pattern scan must distinguish
probe failures from successful probes with no matches: update the non-zero
return-code/empty-output branch and the `except` handling to return or raise an
explicit probe error, then propagate that failure through `check_fabric_manager`
and `FabricManagerStatus` so the monitor records a check error, treats the state
as UNKNOWN, and suppresses `HealthEvent` emission.
Source: Learnings
There was a problem hiding this comment.
The finding is right, with one correction to the framing: this is not fork drift. #1382 has the identical shape — _parse_journal_errors returns [] on result.returncode != 0 (:298) and swallows into except (subprocess.TimeoutExpired, Exception) → return [] (:309), so a failed or timed-out journal probe is indistinguishable from a clean one in the shipping package as well, not just in this demo. Worth fixing once against #1382's ServiceStatus/FabricManagerStatus (an explicit probe-error field the monitor can use to suppress HealthEvent emission) rather than patching the fork. Leaving OPEN and flagging it to @XRFXLP / @deesharma24 for #1382.
|
Acknowledging the open review round here rather than leaving it silent: the 08-12 CodeRabbit pass posted a full set of findings on this PR (missing Apache headers on most files, docstring coverage far under the gate, config/README/manifest inconsistencies) that I have not addressed, and on re-reading the demo against the implementation PR (#1382) I don't think patching them is the right spend. This demo forks the implementation's host-probe logic, and the fork has already drifted behind fixes #1382 carries (LoadState handling, journal-failure propagation, exception hygiene). Two diverging copies of that logic is a maintenance liability. Plan I'd propose: once #1382 lands, either rebuild this as a thin demo that imports the shipped package, or drop it if the Helm chart PR (#1385) covers the deployment story. Until that direction is settled I'm not pushing fixes to the forked copy. The branch is merged up to current |
|
Picking the open review round up rather than leaving it silent, and correcting my own read of it first: of the three divergences I was about to cite between this demo and the implementation PR (#1382), only one is actually a divergence. Real drift — Not drift — the journal and broad-except findings. #1382 carries the identical shape: On the license headers: 13 of the 15 files in this PR have no Apache header (only Proposal: once #1382 lands, either rebuild this as a thin demo that imports the shipped package (which deletes the forked probe and the header/format/type-hint findings with it), or drop it if the Helm chart in #1385 already covers the deployment story. Until that direction is settled I am not pushing fixes into the forked copy. The branch is currently 8 commits behind |
Four mechanical inconsistencies from the CodeRabbit review, all cases where two files in this demo state different values for the same contract: - README build tag was `:latest` while k8s/daemonset.yaml deploys `:0.1.0`, so following the Quick Start verbatim produced an ImagePullBackOff. - README port-forward selector used `app=` but the DaemonSet only sets `app.kubernetes.io/name=`, so the command returned no pod. - README documented GPUServiceDown as critical; servicemonitor.yaml sets severity: warning. - FabricManagerFlapping used `increase(...) > 3`, requiring four restarts, while ServiceChecker.is_flapping flags at three (`len(history) >= 3` with FLAP_THRESHOLD=3). Aligned to `>= 3` and noted the shared threshold. Scope note: this fixes only the self-contradictions, which hold regardless of whether the demo keeps its forked probe or is rebuilt over the NVIDIA#1382 package. The functional findings on that probe (LoadState, journal-probe UNKNOWN, fabric/GPU-service separation, CHECK_INTERVAL validation, restart-counter deltas, boot-grace health) are left open pending that direction; the flapping alert additionally cannot fire until fabric_manager_restarts_total is wired. Signed-off-by: Anton Alexander <dmvevents@gmail.com>
1 participant
Per #891 split (5 of 5). Marked draft — depends on PR landing first.
Series progress
What this contains
demos/system-services-monitor/standalone demo subtreeWhat this does NOT contain
CR-ignored findings addressed
cc @XRFXLP
Summary by CodeRabbit
New Features
Documentation
Tests