feat(monitor): system-services-monitor implementation + unit tests - #1382
Open
dmvevents wants to merge 9 commits into
Open
feat(monitor): system-services-monitor implementation + unit tests#1382dmvevents wants to merge 9 commits into
dmvevents wants to merge 9 commits 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:
📝 WalkthroughWalkthroughThis pull request adds ChangesSystem Services Monitor Implementation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant FabricManagerWatcher
participant ServiceChecker
participant FabricStateChecker
participant PlatformConnectorEventProcessor
participant PlatformConnector
FabricManagerWatcher->>ServiceChecker: poll systemd and journal state
FabricManagerWatcher->>FabricStateChecker: poll per-GPU fabric state
FabricManagerWatcher->>PlatformConnectorEventProcessor: dispatch CheckResult batch
PlatformConnectorEventProcessor->>PlatformConnector: publish changed HealthEvent messages over gRPC
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
This was referenced Jun 10, 2026
![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: 3
🧹 Nitpick comments (5)
health-monitors/system-services-monitor/Dockerfile (1)
39-56: 💤 Low valueConsider security posture: root user in runtime image.
The Dockerfile does not specify a non-root USER directive. While
nsentertypically requires elevated privileges (CAP_SYS_ADMIN or root) to enter host namespaces, consider whether the container can run with a non-root user and grant only the necessary Linux capabilities via the pod security context (e.g.,securityContext.capabilities.add: ["SYS_ADMIN"]). This follows least-privilege principles.If root access is truly required for operational reasons, document this requirement in the deployment configuration or README.
🤖 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/system-services-monitor/Dockerfile` around lines 39 - 56, The runtime image currently runs as root (no USER set) while installing util-linux for nsenter; either create and switch to a non-root user in the Dockerfile (e.g., add a dedicated user/group and a USER directive after pip install and before ENTRYPOINT) and ensure the `system_services_monitor` binary and any needed files are chown'd so the non-root user can execute them, or if host namespace access truly requires root, document that root/CAP_SYS_ADMIN is required in deployment manifests/README and remove expectations of non-root execution; in either case mention `nsenter`/util-linux, the ENTRYPOINT `system_services_monitor`, and the need to grant only the minimal capability (SYS_ADMIN) via pod securityContext rather than running the container as full root wherever possible.Source: Linters/SAST tools
health-monitors/system-services-monitor/system_services_monitor/cli.py (1)
69-69: ⚡ Quick win
exitidentifier shadows Python builtin in both files.Both
cli.py(variable assignment) andwatcher.py(function parameter) useexitas an identifier, which shadows the Python builtinexit(). Rename toexit_eventorshutdown_eventconsistently across both files for clarity and to satisfy linter warnings.🤖 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/system-services-monitor/system_services_monitor/cli.py` at line 69, The identifier exit shadows the Python builtin; rename the Event instance exit in cli.py and the corresponding function parameter named exit in watcher.py to a consistent name like exit_event or shutdown_event across both files (update all references where Event is set/checked) so cli.py's exit -> exit_event and watcher.py's parameter exit -> exit_event to satisfy the linter and avoid builtin shadowing.health-monitors/system-services-monitor/system_services_monitor/checkers/service_check.py (1)
174-188: ⚡ Quick winLog exceptions in
_get_restart_countfor debugging.The try-except silently returns 0 when parsing fails, making it difficult to distinguish between "NRestarts unsupported" (expected on older systemd) and "parsing failed unexpectedly" (potential bug). Add debug-level logging inside the except block to aid troubleshooting.
📝 Suggested improvement
try: result = self._run_host_cmd([ "systemctl", "show", service_name, "--property=NRestarts", ]) if result.returncode == 0 and result.stdout.strip(): _, _, val = result.stdout.strip().partition("=") return int(val) - except Exception: - pass + except Exception as e: + log.debug(f"Failed to get NRestarts for {service_name}: {e}") return 0🤖 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/system-services-monitor/system_services_monitor/checkers/service_check.py` around lines 174 - 188, The except block in _get_restart_count currently swallows all exceptions and returns 0; modify _get_restart_count to log the caught exception at debug level (including exception message and stacktrace) before returning 0 so parsing/runtime errors are visible for debugging. Use the module/class logger (or self._logger if available) and include context like the service_name and result (e.g., output from _run_host_cmd) when logging; keep the existing behavior of returning 0 for unsupported NRestarts. Ensure the logging call is placed inside the except Exception block in _get_restart_count.health-monitors/system-services-monitor/system_services_monitor/metrics.py (1)
38-48: 💤 Low valueConsider adding
_totalsuffix to Counter metric names for consistency.For clarity and Prometheus best practice, Counter metrics should end with
_total. Thecheck_errorscounter follows this convention (line 28:fabric_monitor_check_errors_total), butcallback_failuresandcallback_successdo not. Whileprometheus_clientautomatically appends_totalduring export, explicitly including it in the metric name improves readability and consistency.🤖 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/system-services-monitor/system_services_monitor/metrics.py` around lines 38 - 48, Rename the two Counter metrics to include the _total suffix for consistency with Prometheus conventions and the existing fabric_monitor_check_errors_total metric: update the declarations of callback_failures and callback_success to use names "fabric_monitor_callback_failures_total" and "fabric_monitor_callback_success_total" respectively, and then update any usages/references of the variables callback_failures and callback_success elsewhere in the module so they continue to increment/observe the same Counter objects.health-monitors/system-services-monitor/system_services_monitor/platform_connector/metrics.py (1)
25-33: 💤 Low valueConsider adding
_totalsuffix to Counter metric names for consistency.Similar to the counters in
system_services_monitor/metrics.py, these Counter metrics (events_sent_successandevents_sent_error) should include the_totalsuffix per Prometheus naming conventions. This would improve consistency across the codebase, especially sincecheck_errorsin the parent module follows this pattern (fabric_monitor_check_errors_total).🤖 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/system-services-monitor/system_services_monitor/platform_connector/metrics.py` around lines 25 - 33, Rename the Prometheus Counter metric names for consistency by adding the `_total` suffix: update the name strings used when constructing events_sent_success and events_sent_error in metrics.py from "fabric_monitor_events_sent_success" and "fabric_monitor_events_sent_error" to "fabric_monitor_events_sent_success_total" and "fabric_monitor_events_sent_error_total" respectively, leaving the variable names and descriptions unchanged so existing references to the Counter objects remain valid.
🤖 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
`@health-monitors/system-services-monitor/system_services_monitor/checkers/watcher.py`:
- Line 69: The attribute self._checkers is annotated with the builtin callable
instead of a typing hint; import Callable (and Tuple/Any if needed) from typing
and update the annotation to use typing types, e.g. change "self._checkers:
List[tuple[str, callable]] = []" to "self._checkers: List[Tuple[str,
Callable[..., Any]]] = []" (and add "from typing import List, Tuple, Callable,
Any" to the imports) to provide a proper type hint for the watcher._checkers
list.
In
`@health-monitors/system-services-monitor/system_services_monitor/platform_connector/event_processor.py`:
- Around line 174-184: The HealthEventOccurredV1 gRPC call can block
indefinitely; update the call to include a reasonable per-attempt timeout (e.g.,
10–30s) by passing the timeout argument to stub.HealthEventOccurredV1(...) (the
call that currently builds platformconnector_pb2.HealthEvents and invokes
HealthEventOccurredV1). Keep the existing retry loop (sleep, delay, MAX_DELAY,
metrics.events_sent_success and the except grpc.RpcError as e handler) intact so
timeouts surface as RpcError and are retried; choose a timeout constant, use it
in the call, and ensure logging still reports the exception.
In
`@health-monitors/system-services-monitor/system_services_monitor/protos/health_event_pb2.py`:
- Around line 1-6: Generated protobuf/gRPC bindings (health_event_pb2.py,
health_event_pb2.pyi, health_event_pb2_grpc.py) are missing the required
Apache-2.0 license header; update the protobuf generation pipeline to inject the
license during generation by configuring protoc or your wrapper: add a
license-header plugin or a generation wrapper/script (or adjust
Makefile/pyproject.toml build step) that prepends the Apache 2.0 header template
to all generated files (health_event_pb2*, health_event_pb2_grpc*) so the files
keep the "DO NOT EDIT" note but include the correct license on every regen.
---
Nitpick comments:
In `@health-monitors/system-services-monitor/Dockerfile`:
- Around line 39-56: The runtime image currently runs as root (no USER set)
while installing util-linux for nsenter; either create and switch to a non-root
user in the Dockerfile (e.g., add a dedicated user/group and a USER directive
after pip install and before ENTRYPOINT) and ensure the
`system_services_monitor` binary and any needed files are chown'd so the
non-root user can execute them, or if host namespace access truly requires root,
document that root/CAP_SYS_ADMIN is required in deployment manifests/README and
remove expectations of non-root execution; in either case mention
`nsenter`/util-linux, the ENTRYPOINT `system_services_monitor`, and the need to
grant only the minimal capability (SYS_ADMIN) via pod securityContext rather
than running the container as full root wherever possible.
In
`@health-monitors/system-services-monitor/system_services_monitor/checkers/service_check.py`:
- Around line 174-188: The except block in _get_restart_count currently swallows
all exceptions and returns 0; modify _get_restart_count to log the caught
exception at debug level (including exception message and stacktrace) before
returning 0 so parsing/runtime errors are visible for debugging. Use the
module/class logger (or self._logger if available) and include context like the
service_name and result (e.g., output from _run_host_cmd) when logging; keep the
existing behavior of returning 0 for unsupported NRestarts. Ensure the logging
call is placed inside the except Exception block in _get_restart_count.
In `@health-monitors/system-services-monitor/system_services_monitor/cli.py`:
- Line 69: The identifier exit shadows the Python builtin; rename the Event
instance exit in cli.py and the corresponding function parameter named exit in
watcher.py to a consistent name like exit_event or shutdown_event across both
files (update all references where Event is set/checked) so cli.py's exit ->
exit_event and watcher.py's parameter exit -> exit_event to satisfy the linter
and avoid builtin shadowing.
In `@health-monitors/system-services-monitor/system_services_monitor/metrics.py`:
- Around line 38-48: Rename the two Counter metrics to include the _total suffix
for consistency with Prometheus conventions and the existing
fabric_monitor_check_errors_total metric: update the declarations of
callback_failures and callback_success to use names
"fabric_monitor_callback_failures_total" and
"fabric_monitor_callback_success_total" respectively, and then update any
usages/references of the variables callback_failures and callback_success
elsewhere in the module so they continue to increment/observe the same Counter
objects.
In
`@health-monitors/system-services-monitor/system_services_monitor/platform_connector/metrics.py`:
- Around line 25-33: Rename the Prometheus Counter metric names for consistency
by adding the `_total` suffix: update the name strings used when constructing
events_sent_success and events_sent_error in metrics.py from
"fabric_monitor_events_sent_success" and "fabric_monitor_events_sent_error" to
"fabric_monitor_events_sent_success_total" and
"fabric_monitor_events_sent_error_total" respectively, leaving the variable
names and descriptions unchanged so existing references to the Counter objects
remain valid.
🪄 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: 9c597df0-6547-499b-94dd-0ea9b87cb1fa
📒 Files selected for processing (23)
.github/workflows/container-build-test.ymlhealth-monitors/system-services-monitor/Dockerfilehealth-monitors/system-services-monitor/Makefilehealth-monitors/system-services-monitor/README.mdhealth-monitors/system-services-monitor/pyproject.tomlhealth-monitors/system-services-monitor/system_services_monitor/__init__.pyhealth-monitors/system-services-monitor/system_services_monitor/checkers/__init__.pyhealth-monitors/system-services-monitor/system_services_monitor/checkers/fabric_state_check.pyhealth-monitors/system-services-monitor/system_services_monitor/checkers/service_check.pyhealth-monitors/system-services-monitor/system_services_monitor/checkers/tests/__init__.pyhealth-monitors/system-services-monitor/system_services_monitor/checkers/tests/test_service_check.pyhealth-monitors/system-services-monitor/system_services_monitor/checkers/types.pyhealth-monitors/system-services-monitor/system_services_monitor/checkers/watcher.pyhealth-monitors/system-services-monitor/system_services_monitor/cli.pyhealth-monitors/system-services-monitor/system_services_monitor/logger.pyhealth-monitors/system-services-monitor/system_services_monitor/metrics.pyhealth-monitors/system-services-monitor/system_services_monitor/platform_connector/__init__.pyhealth-monitors/system-services-monitor/system_services_monitor/platform_connector/event_processor.pyhealth-monitors/system-services-monitor/system_services_monitor/platform_connector/metrics.pyhealth-monitors/system-services-monitor/system_services_monitor/protos/__init__.pyhealth-monitors/system-services-monitor/system_services_monitor/protos/health_event_pb2.pyhealth-monitors/system-services-monitor/system_services_monitor/protos/health_event_pb2.pyihealth-monitors/system-services-monitor/system_services_monitor/protos/health_event_pb2_grpc.py
Contributor
|
@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. |
dmvevents
added a commit
to dmvevents/NVSentinel
that referenced
this pull request
Jul 24, 2026
Adds the design doc for system-services-monitor — a health-monitor for the host services behind fabric health (nvidia-fabricmanager, nvidia-persistenced, NVSwitch registration state) that sit in a layer neither gpu-health-monitor nor syslog-health-monitor observes today. Numbered 049 (next free slot on main; 030/042/043 taken). Detection mechanism, HealthEvent schema, checkName/errorCode taxonomy, and cached-state semantics are grounded in the system-services-monitor implementation (PR NVIDIA#1382). Signed-off-by: Anton Alexander <dmvevents@users.noreply.github.com>
Lands the CI wiring as a focused PR ahead of the implementation (NVIDIA#891 split, 2 of 5). Adds the system-services-monitor matrix entry in container-build-test.yml so the implementation PR's first CI run will be on the real matrix row rather than backfilled afterwards. Includes a minimal stub Makefile with no-op targets (lint-test, docker-build, docker-publish) so the matrix row passes; the real Makefile (Poetry, make/python.mk + make/docker.mk includes, etc.) lands together with the Python implementation in PR NVIDIA#3. The original umbrella branch also touches lint-test.yml and publish.yml, but those diffs only contain unrelated actions/checkout SHA reverts (no actual module registration), so they are not included in this PR. cleanup-untagged-images.yml is similarly unmodified on the umbrella branch. Signed-off-by: Anton Alexander <dmvevents@users.noreply.github.com>
Implements the system-services-monitor package per ADR-030 (NVIDIA#1380): - service_check.py for systemd service health - watcher.py + event_processor.py with thread-safe entity_cache - cli.py with --verbose flag and version handling - logger.py with explicit warning on unknown log levels - Dockerfile (python:3.13 base, apt cache mount per CR review) - Makefile with real lint-test / test / docker-build / docker-publish targets - Unit tests under tests/ covering service_check + event_processor Lands as NVIDIA#891 split (3 of 5) on top of NVIDIA#1380 (ADR) and the CI PR. Excludes cuda_validation.py — that checker is being moved to preflight-checks/cuda-validation/ in a follow-up PR per @XRFXLP review on the umbrella PR. The runtime GPU-allocation concern raised in his review is resolved by removing it from the daemon-poll path entirely. Signed-off-by: Anton Alexander <dmvevents@users.noreply.github.com>
- Makefile: set MODULE_NAME := $(PYTHON_PACKAGE_NAME). common.mk defaults MODULE_NAME to the hyphenated dir basename, so python.mk's `coverage run --source=$(MODULE_NAME)` never matched the importable package and reported 0% coverage. Matches preflight-checks/nccl-allreduce. - pyproject.toml: relax python floor ^3.13 -> ^3.10 to match siblings (nccl-allreduce, dcgm-diag). Package uses no 3.11+/3.13-only syntax or stdlib (verified: no tomllib, PEP 695 type params, StrEnum, datetime.UTC, ExceptionGroup, etc.). - Dockerfile: pin poetry==2.3.3 + poetry-plugin-export==1.10.0 per .versions.yaml (was poetry==1.8.2 with no export plugin while running `poetry export`). Export invocation already matches the poetry-2.x sibling Dockerfiles. - cli.py: rename `exit = Event()` -> `stop_event` to stop shadowing the builtin (CodeRabbit flagged this on the prior NVIDIA#891). - platform_connector/event_processor.py: guard the UDS dial with _is_platform_connector_socket_present() before/between retries, mirroring gpu-health-monitor; log-and-skip instead of noisy gRPC stack traces when the socket isn't present yet. Adds events_sent_skipped_pc_unavailable counter. Signed-off-by: Anton Alexander <dmvevents@users.noreply.github.com>
…ounter, tests Builds on the review-findings base commit; adds the fixes it did not cover. - ADR-030 -> ADR-049 sweep (4 refs): README.md scope-doc link, watcher.py module + class docstrings, cli.py comment. Upstream renamed the ADR to 049-system-services-monitor-scope.md (030 is now an unrelated gRPC-TLS ADR). - cli.py: add Click envvar= fallbacks so the Helm chart's configmap (which passes config via env only, no args) satisfies the required --platform-connector-socket and the other tunables. Fixes the container CrashLoop. Names match the chart configmap: PLATFORM_CONNECTOR_SOCKET, METRICS_PORT, CHECK_INTERVAL, BOOT_GRACE_PERIOD, FLAP_WINDOW, FLAP_THRESHOLD, ENABLE_FABRIC_CHECK. CLI args still take precedence over env. - metrics.py + watcher.py: add fabric_manager_restarts_total counter and increment it by the systemd NRestarts delta each poll cycle. Backs the chart's FabricManagerFlapping alert (increase(fabric_manager_restarts_total[10m]) > 3). - tests: add pytest coverage for watcher.py, fabric_state_check.py, event_processor.py (incl. the socket-presence skip path), and cli.py (env-only, arg-overrides-env, and startup-validation failure paths), following the existing test_service_check.py mocking style. Full suite: 44 passed. Signed-off-by: Anton Alexander <dmvevents@users.noreply.github.com>
dmvevents
force-pushed
the
impl-system-services-monitor
branch
from
July 24, 2026 17:28
b6be122 to
8bfc1b4
Compare
dmvevents
added a commit
to dmvevents/NVSentinel
that referenced
this pull request
Jul 24, 2026
Lands the Helm chart for system-services-monitor (NVIDIA#891 split, 4 of 5). Subchart under distros/kubernetes/nvsentinel/charts/ + values.yaml + Chart.yaml dependency registration in the parent chart. The chart is aligned to the actual runtime contract of the app on the implementation branch (cli.py / metrics.py), not an assumed one: DaemonSet - The app's entrypoint is a Click CLI whose --platform-connector-socket option is required=True with no env fallback. The container now passes it (plus --port/--poll-interval/--boot-grace-period/--flap-window/ --flap-threshold/--enable-fabric-check/--processing-strategy) via args:, and mounts the platform-connector Unix socket (hostPath /var/run/nvsentinel at /var/run, mirroring the slurm/nic/csp siblings). The app prepends unix:// itself, so the flag value is a bare path. - Dropped the envFrom configMapRef and the unused /var/run/dbus mount -- the app reads host systemd state via nsenter into PID 1, not dbus. - Keeps NODE_NAME (fieldRef) and LOG_LEVEL, the only env the app reads. Metrics / alerts - metricsPort now binds global.metricsPort (2112), matching siblings and the --port flag the app actually honors. - PrometheusRule alerts only on metrics the monitor exports: fabric_manager_up, fabric_state_healthy, nvidia_service_up, and fabric_manager_restarts_total (added in NVIDIA#1382). The flapping alert fires on increase(fabric_manager_restarts_total[10m]) > 3. Removed the CUDAValidationFailed alert -- cuda validation is an exit-code-only init container (NVIDIA#1384), no cuda_validation_passed metric. - Alert names follow the ADR-049 check taxonomy: FabricManagerServiceDown, FabricStateUnhealthy, GpuServiceDown. Config - Deleted the ConfigMap: its keys were either dead or are real CLI flags, now templated into args: from values.yaml. LOG_LEVEL is a plain env var. - ServiceMonitor + PrometheusRule default enabled: false (no health-monitor sibling ships them enabled) and ServiceMonitor's release label is now driven by .Values.serviceMonitor.labels (empty default) instead of a hardcoded release: prometheus. Mirrors the sibling pattern (nic-health-monitor) for .Values.global references; the parent chart supplies globals, so validate by rendering the parent chart (helm template distros/kubernetes/nvsentinel --set global.systemServicesMonitor.enabled=true), not standalone lint. Signed-off-by: Anton Alexander <dmvevents@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
health-monitors/system-services-monitor/system_services_monitor/platform_connector/event_processor.py (1)
204-214: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winMissing timeout on gRPC call — can block indefinitely (still unaddressed).
stub.HealthEventOccurredV1(...)has notimeout, so a slow/unresponsive platform-connector can hang this call forever, tying up aThreadPoolExecutorworker inwatcher.py's_fire_callback_funcsand eventually exhausting the pool. This was flagged in a prior review and remains unfixed.🔧 Proposed fix
try: - stub.HealthEventOccurredV1(platformconnector_pb2.HealthEvents(events=health_events, version=1)) + stub.HealthEventOccurredV1( + platformconnector_pb2.HealthEvents(events=health_events, version=1), + timeout=30.0, + )🤖 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/system-services-monitor/system_services_monitor/platform_connector/event_processor.py` around lines 204 - 214, Update the HealthEventOccurredV1 call in the event-sending retry loop to provide a finite timeout, using the existing timeout configuration or an appropriate bounded value. Preserve the current RpcError logging, backoff, retry, and success behavior when the call times out.
🧹 Nitpick comments (5)
health-monitors/system-services-monitor/system_services_monitor/platform_connector/tests/test_event_processor.py (1)
100-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded retry count instead of referencing the constant.
call_count == 5 # MAX_RETRIESduplicates the constant as a magic number; ifMAX_RETRIESinevent_processor.pychanges, this assertion silently drifts from the comment's intent.♻️ Suggested fix
-from system_services_monitor.platform_connector.event_processor import PlatformConnectorEventProcessor +from system_services_monitor.platform_connector.event_processor import MAX_RETRIES, PlatformConnectorEventProcessor ... - assert mock_stub.HealthEventOccurredV1.call_count == 5 # MAX_RETRIES + assert mock_stub.HealthEventOccurredV1.call_count == MAX_RETRIES🤖 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/system-services-monitor/system_services_monitor/platform_connector/tests/test_event_processor.py` around lines 100 - 131, Update test_rpc_error_exhausts_retries_and_returns_false to assert the RPC call count against the MAX_RETRIES constant imported from event_processor rather than the hardcoded value 5, keeping the existing retry-exhaustion behavior unchanged.health-monitors/system-services-monitor/system_services_monitor/checkers/tests/test_watcher.py (1)
30-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrivate test helper functions omit return type hints. As per coding guidelines,
**/*.pyfiles must "Include type hints for all functions in Python code"; both helpers below are missing return annotations — same root cause, add the missing hints.
health-monitors/system-services-monitor/system_services_monitor/checkers/tests/test_watcher.py#L30-L30: annotate_make_watcheras-> FabricManagerWatcher.health-monitors/system-services-monitor/system_services_monitor/tests/test_cli.py#L27-L27: annotate_mock_runtimeas-> Generator[tuple[MagicMock, MagicMock], None, None].🤖 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/system-services-monitor/system_services_monitor/checkers/tests/test_watcher.py` at line 30, Add the missing return type hints to both private test helpers: annotate _make_watcher in health-monitors/system-services-monitor/system_services_monitor/checkers/tests/test_watcher.py (line 30) as FabricManagerWatcher, and annotate _mock_runtime in health-monitors/system-services-monitor/system_services_monitor/tests/test_cli.py (line 27) as Generator[tuple[MagicMock, MagicMock], None, None].Source: Coding guidelines
health-monitors/system-services-monitor/system_services_monitor/checkers/service_check.py (1)
262-262: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant exception type in tuple.
except (subprocess.TimeoutExpired, Exception)—TimeoutExpiredis already a subclass ofException, so the tuple is redundant;except Exceptionalone is equivalent and clearer.🤖 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/system-services-monitor/system_services_monitor/checkers/service_check.py` at line 262, Update the exception handler in the service-checking flow to catch only Exception; remove the redundant subprocess.TimeoutExpired entry from the tuple while preserving the existing handler behavior and alias.health-monitors/system-services-monitor/Makefile (1)
62-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
poetry shellrequires a plugin on Poetry 2.x.The Dockerfile pins
poetry==2.3.3. Poetry removed theshellcommand from core in 2.0; it now requires installingpoetry-plugin-shell, otherwisepoetry shellerrors with "command does not exist." If contributors are expected to use the same Poetry major version, this target will fail as-is.♻️ Proposed fix
shell: `@echo` "Opening Poetry shell for $(MODULE_NAME)..." - poetry shell + poetry env activate || (poetry self add poetry-plugin-shell && poetry shell)🤖 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/system-services-monitor/Makefile` around lines 62 - 64, Update the Makefile target named shell to work with the pinned Poetry 2.x setup by replacing the unsupported poetry shell invocation with the supported environment-entry command, or ensure the required shell plugin is installed before invoking it. Keep the existing user-facing message and MODULE_NAME behavior unchanged.health-monitors/system-services-monitor/system_services_monitor/protos/health_event_pb2_grpc.py (1)
20-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRuff F541 on generated code — exclude generated files from lint instead of editing them.
This file is marked "DO NOT EDIT!" (grpc_tools codegen output); the
f-string-without-placeholder on line 22 is a byproduct of the generated template, and hand-fixing it will be reverted on the nextprotoc/grpc_toolsregeneration. Consider excluding*_pb2.py/*_pb2_grpc.pyfrom Ruff (extend-excludeinpyproject.toml) instead.🤖 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/system-services-monitor/system_services_monitor/protos/health_event_pb2_grpc.py` around lines 20 - 26, Exclude generated protobuf files matching *_pb2.py and *_pb2_grpc.py from Ruff via the project’s Ruff configuration, such as extend-exclude in pyproject.toml. Do not modify the generated RuntimeError code in health_event_pb2_grpc.py, preserving it for future protoc/grpc_tools regeneration.Source: Linters/SAST tools
🤖 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
`@health-monitors/system-services-monitor/system_services_monitor/checkers/service_check.py`:
- Around line 174-188: Update _get_restart_count to log caught exceptions at
debug level before returning the existing fallback value of 0. Preserve the
current behavior for unsupported NRestarts queries while retaining the exception
details for troubleshooting.
In
`@health-monitors/system-services-monitor/system_services_monitor/tests/test_cli.py`:
- Around line 88-98: Remove the unused watcher binding from the _mock_runtime()
tuple unpacking in test_cli_arg_overrides_env, while preserving the processor
binding and all existing assertions.
---
Duplicate comments:
In
`@health-monitors/system-services-monitor/system_services_monitor/platform_connector/event_processor.py`:
- Around line 204-214: Update the HealthEventOccurredV1 call in the
event-sending retry loop to provide a finite timeout, using the existing timeout
configuration or an appropriate bounded value. Preserve the current RpcError
logging, backoff, retry, and success behavior when the call times out.
---
Nitpick comments:
In `@health-monitors/system-services-monitor/Makefile`:
- Around line 62-64: Update the Makefile target named shell to work with the
pinned Poetry 2.x setup by replacing the unsupported poetry shell invocation
with the supported environment-entry command, or ensure the required shell
plugin is installed before invoking it. Keep the existing user-facing message
and MODULE_NAME behavior unchanged.
In
`@health-monitors/system-services-monitor/system_services_monitor/checkers/service_check.py`:
- Line 262: Update the exception handler in the service-checking flow to catch
only Exception; remove the redundant subprocess.TimeoutExpired entry from the
tuple while preserving the existing handler behavior and alias.
In
`@health-monitors/system-services-monitor/system_services_monitor/checkers/tests/test_watcher.py`:
- Line 30: Add the missing return type hints to both private test helpers:
annotate _make_watcher in
health-monitors/system-services-monitor/system_services_monitor/checkers/tests/test_watcher.py
(line 30) as FabricManagerWatcher, and annotate _mock_runtime in
health-monitors/system-services-monitor/system_services_monitor/tests/test_cli.py
(line 27) as Generator[tuple[MagicMock, MagicMock], None, None].
In
`@health-monitors/system-services-monitor/system_services_monitor/platform_connector/tests/test_event_processor.py`:
- Around line 100-131: Update test_rpc_error_exhausts_retries_and_returns_false
to assert the RPC call count against the MAX_RETRIES constant imported from
event_processor rather than the hardcoded value 5, keeping the existing
retry-exhaustion behavior unchanged.
In
`@health-monitors/system-services-monitor/system_services_monitor/protos/health_event_pb2_grpc.py`:
- Around line 20-26: Exclude generated protobuf files matching *_pb2.py and
*_pb2_grpc.py from Ruff via the project’s Ruff configuration, such as
extend-exclude in pyproject.toml. Do not modify the generated RuntimeError code
in health_event_pb2_grpc.py, preserving it for future protoc/grpc_tools
regeneration.
🪄 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: 051c7b06-0802-483c-9e4b-f7d4b275d3b6
📒 Files selected for processing (29)
.github/workflows/container-build-test.ymlhealth-monitors/system-services-monitor/Dockerfilehealth-monitors/system-services-monitor/Makefilehealth-monitors/system-services-monitor/README.mdhealth-monitors/system-services-monitor/pyproject.tomlhealth-monitors/system-services-monitor/system_services_monitor/__init__.pyhealth-monitors/system-services-monitor/system_services_monitor/checkers/__init__.pyhealth-monitors/system-services-monitor/system_services_monitor/checkers/fabric_state_check.pyhealth-monitors/system-services-monitor/system_services_monitor/checkers/service_check.pyhealth-monitors/system-services-monitor/system_services_monitor/checkers/tests/__init__.pyhealth-monitors/system-services-monitor/system_services_monitor/checkers/tests/test_fabric_state_check.pyhealth-monitors/system-services-monitor/system_services_monitor/checkers/tests/test_service_check.pyhealth-monitors/system-services-monitor/system_services_monitor/checkers/tests/test_watcher.pyhealth-monitors/system-services-monitor/system_services_monitor/checkers/types.pyhealth-monitors/system-services-monitor/system_services_monitor/checkers/watcher.pyhealth-monitors/system-services-monitor/system_services_monitor/cli.pyhealth-monitors/system-services-monitor/system_services_monitor/logger.pyhealth-monitors/system-services-monitor/system_services_monitor/metrics.pyhealth-monitors/system-services-monitor/system_services_monitor/platform_connector/__init__.pyhealth-monitors/system-services-monitor/system_services_monitor/platform_connector/event_processor.pyhealth-monitors/system-services-monitor/system_services_monitor/platform_connector/metrics.pyhealth-monitors/system-services-monitor/system_services_monitor/platform_connector/tests/__init__.pyhealth-monitors/system-services-monitor/system_services_monitor/platform_connector/tests/test_event_processor.pyhealth-monitors/system-services-monitor/system_services_monitor/protos/__init__.pyhealth-monitors/system-services-monitor/system_services_monitor/protos/health_event_pb2.pyhealth-monitors/system-services-monitor/system_services_monitor/protos/health_event_pb2.pyihealth-monitors/system-services-monitor/system_services_monitor/protos/health_event_pb2_grpc.pyhealth-monitors/system-services-monitor/system_services_monitor/tests/__init__.pyhealth-monitors/system-services-monitor/system_services_monitor/tests/test_cli.py
🚧 Files skipped from review as they are similar to previous changes (16)
- health-monitors/system-services-monitor/system_services_monitor/checkers/tests/init.py
- health-monitors/system-services-monitor/system_services_monitor/checkers/init.py
- health-monitors/system-services-monitor/system_services_monitor/protos/init.py
- health-monitors/system-services-monitor/system_services_monitor/checkers/types.py
- health-monitors/system-services-monitor/system_services_monitor/platform_connector/init.py
- health-monitors/system-services-monitor/system_services_monitor/platform_connector/metrics.py
- health-monitors/system-services-monitor/README.md
- health-monitors/system-services-monitor/system_services_monitor/logger.py
- health-monitors/system-services-monitor/pyproject.toml
- .github/workflows/container-build-test.yml
- health-monitors/system-services-monitor/system_services_monitor/protos/health_event_pb2.pyi
- health-monitors/system-services-monitor/system_services_monitor/init.py
- health-monitors/system-services-monitor/system_services_monitor/checkers/tests/test_service_check.py
- health-monitors/system-services-monitor/system_services_monitor/protos/health_event_pb2.py
- health-monitors/system-services-monitor/system_services_monitor/metrics.py
- health-monitors/system-services-monitor/system_services_monitor/cli.py
- add a per-attempt gRPC deadline (10s) to HealthEventOccurredV1 so a hung platform-connector surfaces as DEADLINE_EXCEEDED and is retried instead of pinning a ThreadPoolExecutor thread indefinitely - type the checker registry as Callable[[], List[CheckResult]] instead of the builtin callable - debug-log the NRestarts query fallback instead of silently swallowing the exception (Ruff S110/BLE001) - rename the unused watcher binding in test_cli (Ruff RUF059) Generated *_pb2* files intentionally carry no license header: the repo's license-headers-lint (addlicense) explicitly ignores '**/*pb2*', matching the checked-in gpu-health-monitor bindings on main. Full module test suite: 44 passed. Signed-off-by: Anton Alexander <dmvevents@gmail.com>
Contributor
|
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. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (10)
health-monitors/system-services-monitor/system_services_monitor/checkers/service_check.py (1)
264-266: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant exception in the tuple.
subprocess.TimeoutExpiredis a subclass ofException, soexcept (subprocess.TimeoutExpired, Exception)is equivalent toexcept Exception. The tuple suggests that the two cases are handled differently, which they are not.♻️ Proposed change
- except (subprocess.TimeoutExpired, Exception) as e: + except Exception as e: log.warning(f"Journal parsing failed for {service_name}: {e}") return []🤖 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/system-services-monitor/system_services_monitor/checkers/service_check.py` around lines 264 - 266, Update the exception handler in the journal parsing flow to catch only Exception, removing the redundant subprocess.TimeoutExpired entry while preserving the existing warning log and empty-list return behavior.health-monitors/system-services-monitor/system_services_monitor/platform_connector/tests/test_event_processor.py (3)
29-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the
tmp_pathfixture for the socket path.Line 32 hardcodes
/tmp/does-not-exist.sock, which Ruff flags as S108. A pytesttmp_pathvalue gives a per-test unique directory and removes the reliance on a shared world-writable path.♻️ Proposed change
`@pytest.fixture` -def processor() -> PlatformConnectorEventProcessor: +def processor(tmp_path) -> PlatformConnectorEventProcessor: return PlatformConnectorEventProcessor( - socket_path="/tmp/does-not-exist.sock", + socket_path=str(tmp_path / "does-not-exist.sock"), node_name=NODE, processing_strategy=pb.ProcessingStrategy.Value("EXECUTE_REMEDIATION"), )🤖 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/system-services-monitor/system_services_monitor/platform_connector/tests/test_event_processor.py` around lines 29 - 35, Update the processor fixture to accept pytest’s tmp_path fixture and build the nonexistent socket path from it instead of hardcoding /tmp/does-not-exist.sock. Keep the existing PlatformConnectorEventProcessor configuration unchanged.Source: Linters/SAST tools
64-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis assertion does not verify commit-on-success.
health_check_completedwrites the cache entry at line 150 ofevent_processor.py, before it callssend_health_event_with_retries. The assertionlen(processor.entity_cache) == 1therefore passes for both the reservation and the commit. The docstring claim about committing after a successful send is not tested.Assert the cached value instead, and assert that the reservation is present with the expected state.
♻️ Proposed change
send.assert_called_once() (events_arg,), _ = send.call_args assert len(events_arg) == 1 - # The reservation is committed in the cache after a successful send. - assert len(processor.entity_cache) == 1 + # The reservation survives a successful send with the observed state. + key = processor._build_cache_key("FabricManagerServiceDown", [{"entityType": "NODE", "entityValue": NODE}]) + assert processor.entity_cache[key].is_healthy is False + assert processor.entity_cache[key].is_fatal is True🤖 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/system-services-monitor/system_services_monitor/platform_connector/tests/test_event_processor.py` around lines 64 - 73, Update test_new_state_sends_event_and_updates_cache to inspect the cached entry rather than only asserting entity_cache length. Verify the reservation exists under the expected entity key and that its cached value contains the expected state after send_health_event_with_retries succeeds.
115-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport
MAX_RETRIESinstead of hardcoding 5, and cover the remaining send paths.Line 131 hardcodes
5with a comment naming the constant. IfMAX_RETRIESchanges, this test fails for an unrelated reason. Import the constant.Two behaviors in
send_health_event_with_retrieshave no coverage:
- The per-attempt deadline. No test asserts that
HealthEventOccurredV1receivestimeout=GRPC_SEND_TIMEOUT_SECS. That deadline prevents indefinite blocking, so a regression would be silent.- The mid-retry abort at lines 200-206 of
event_processor.py, where the socket disappears between attempts.♻️ Proposed change
-from system_services_monitor.platform_connector.event_processor import PlatformConnectorEventProcessor +from system_services_monitor.platform_connector.event_processor import ( + GRPC_SEND_TIMEOUT_SECS, + MAX_RETRIES, + PlatformConnectorEventProcessor, +)assert ok is False - assert mock_stub.HealthEventOccurredV1.call_count == 5 # MAX_RETRIES + assert mock_stub.HealthEventOccurredV1.call_count == MAX_RETRIESAdd a deadline assertion to the success test:
_, kwargs = mock_stub.HealthEventOccurredV1.call_args assert kwargs["timeout"] == GRPC_SEND_TIMEOUT_SECS🤖 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/system-services-monitor/system_services_monitor/platform_connector/tests/test_event_processor.py` around lines 115 - 131, Update the event processor tests to import and assert the retry count using MAX_RETRIES instead of hardcoded 5. Extend the successful send test for send_health_event_with_retries to verify HealthEventOccurredV1 receives timeout=GRPC_SEND_TIMEOUT_SECS, and add coverage for the mid-retry socket disappearance path where _is_platform_connector_socket_present becomes false, asserting the send aborts as expected.health-monitors/system-services-monitor/system_services_monitor/checkers/fabric_state_check.py (2)
56-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
GpuFabricState.erroris never set or read.
_parse_outputalways constructsGpuFabricStatewithouterror, and no consumer reads the field.check()returns an empty list on failure instead of populating a per-GPU error. Remove the field, or populate it so callers can distinguish a query failure from a healthy fabric.🤖 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/system-services-monitor/system_services_monitor/checkers/fabric_state_check.py` around lines 56 - 63, Remove the unused GpuFabricState.error field and keep the dataclass aligned with the values produced by _parse_output and consumed by check(). Do not add per-GPU error population unless callers are updated to read and distinguish it.
104-124: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
_parse_outputsilently drops rows with too few fields.Line 109 skips any line with fewer than three comma-separated fields without a log entry. A change in the
nvidia-smioutput format then produces an empty result set and no diagnostic. Add a debug or warning log for the skipped line, consistent with the parse-failure log at line 123.♻️ Proposed change
if len(parts) < 3: + log.warning(f"Unexpected fabric state line format, skipping: '{line}'") continue🤖 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/system-services-monitor/system_services_monitor/checkers/fabric_state_check.py` around lines 104 - 124, Update _parse_output to log a debug or warning message, including the original line, before continuing when len(parts) < 3; keep the existing parse-failure logging and valid-row parsing unchanged.health-monitors/system-services-monitor/system_services_monitor/platform_connector/event_processor.py (1)
197-221: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThe loop sleeps after the final attempt.
The
except grpc.RpcErrorbranch always callssleep(delay), including on the last iteration. The function then returnsFalseimmediately after that sleep. The caller's worker thread therefore blocks for up toMAX_DELAYseconds with no attempt left to make.Skip the sleep on the final attempt.
♻️ Proposed change
except grpc.RpcError as e: log.error(f"Failed to send health event to UDS: {e}") + if attempt == MAX_RETRIES - 1: + break sleep(delay) delay = min(delay * 1.5, MAX_DELAY) continue🤖 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/system-services-monitor/system_services_monitor/platform_connector/event_processor.py` around lines 197 - 221, Update the grpc.RpcError handler in the retry loop around PlatformConnectorStub.HealthEventOccurredV1 so sleep(delay) and delay backoff occur only when another retry remains; the final attempt must proceed directly to failure return without sleeping.health-monitors/system-services-monitor/Dockerfile (1)
39-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSet an explicit
WORKDIRin the runtime stage.The runtime stage does not declare
WORKDIR, soCOPY --from=build /app/dist/*.whl ./writes the wheel andconstraints.txtinto/. Thepip install ./system_services_monitor*.whlglob then resolves against/. This works, but it pollutes the filesystem root and depends on the implicit default directory.♻️ Proposed change
ENV PYTHONUNBUFFERED=1 +WORKDIR /app + COPY --from=build /app/dist/*.whl ./ COPY --from=build /app/constraints.txt ./ RUN --mount=type=cache,target=/root/.cache/pip \ pip install ./system_services_monitor*.whl --constraint constraints.txt🤖 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/system-services-monitor/Dockerfile` around lines 39 - 56, Set an explicit WORKDIR in the runtime stage before the wheel and constraints COPY instructions, using a dedicated application directory so the files and pip install glob resolve there instead of the filesystem root.health-monitors/system-services-monitor/system_services_monitor/checkers/watcher.py (1)
75-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
enable_fabric_checkalso disables the systemd service checks.The flag name refers to the fabric check, but the guard at line 75 also controls
ServiceCheckerconstruction and the"services"checker registration. If a caller passesenable_fabric_check=False, the watcher runs no checks at all, andself._service_checkerandself._fabric_state_checkerare never assigned.Split the flag, or rename it to match the behavior. Assign both attributes to
Noneunconditionally so a later refactor cannot introduce anAttributeError.🤖 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/system-services-monitor/system_services_monitor/checkers/watcher.py` around lines 75 - 84, Separate the `ServiceChecker` setup and `"services"` registration from the `enable_fabric_check` guard so systemd service checks remain enabled independently. Keep only `FabricStateChecker` initialization and `"fabric_state"` registration conditional on the fabric flag, and initialize both `self._service_checker` and `self._fabric_state_checker` to `None` unconditionally before the conditional setup.health-monitors/system-services-monitor/Makefile (1)
62-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace
poetry shellwith an interactive Poetry-compatible command. Poetry 2.3.3 does not includeshell, and this project installspoetry-plugin-export, notpoetry-plugin-shell, somake shellfails. Useexec poetry run "$${SHELL:-bash}"to preserve interactive-shell behavior, or documenteval "$(poetry env activate)"for direct execution in the caller’s shell. A standalonepoetry env activaterecipe only prints the activation command.🤖 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/system-services-monitor/Makefile` around lines 62 - 64, Update the Makefile shell target to replace poetry shell with exec poetry run "$${SHELL:-bash}", preserving interactive behavior under Poetry 2.3.3 without requiring the shell plugin.
🤖 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
`@health-monitors/system-services-monitor/system_services_monitor/checkers/fabric_state_check.py`:
- Around line 126-145: Update classify_failure to represent unknown fabric
state/status combinations separately from FM_FABRIC_ERROR, and ensure
to_check_results emits a non-fatal result for that classification rather than
recommending RESTART_BM. Preserve healthy handling for documented N/A and
Completed/Success values, and add tests covering unknown and unavailable values.
In
`@health-monitors/system-services-monitor/system_services_monitor/checkers/service_check.py`:
- Around line 192-211: The restart counters are treated as monotonically
increasing and never recover after systemd resets them. In service_check.py
lines 192-211, update _update_flap_tracking to detect current_restarts below
last_count, log the reset, and re-baseline _last_restart_count[service_name] so
subsequent restarts are tracked; in watcher.py lines 155-160, update
_run_service_checks to treat a negative restart_delta as a reset and set
_last_fm_restarts to fm.n_restarts so fabric_manager_restarts_total resumes
counting.
- Around line 124-129: Update the _run_host_cmd calls in the service-state check
and _get_restart_count so their sole list arguments use Black’s expanded
multiline formatting, matching the existing _parse_journal_errors call. Preserve
the command contents and behavior unchanged.
In
`@health-monitors/system-services-monitor/system_services_monitor/checkers/watcher.py`:
- Line 66: Change the ThreadPoolExecutor used by _callback_thread_pool to a
single-worker executor by setting max_workers=1, preserving submission order and
preventing callbacks from accumulating across concurrent workers.
- Around line 168-189: Handle unknown service states before reporting
service-down events in the Fabric Manager check flow: in the logic around the
Fabric Manager result and the per-service loop using status, inspect the
returned error field and emit a non-fatal check error instead of a service-down
fatal event when state retrieval fails. Preserve the existing down-service
handling only when the status is known, and apply the same guard consistently to
both paths.
---
Nitpick comments:
In `@health-monitors/system-services-monitor/Dockerfile`:
- Around line 39-56: Set an explicit WORKDIR in the runtime stage before the
wheel and constraints COPY instructions, using a dedicated application directory
so the files and pip install glob resolve there instead of the filesystem root.
In `@health-monitors/system-services-monitor/Makefile`:
- Around line 62-64: Update the Makefile shell target to replace poetry shell
with exec poetry run "$${SHELL:-bash}", preserving interactive behavior under
Poetry 2.3.3 without requiring the shell plugin.
In
`@health-monitors/system-services-monitor/system_services_monitor/checkers/fabric_state_check.py`:
- Around line 56-63: Remove the unused GpuFabricState.error field and keep the
dataclass aligned with the values produced by _parse_output and consumed by
check(). Do not add per-GPU error population unless callers are updated to read
and distinguish it.
- Around line 104-124: Update _parse_output to log a debug or warning message,
including the original line, before continuing when len(parts) < 3; keep the
existing parse-failure logging and valid-row parsing unchanged.
In
`@health-monitors/system-services-monitor/system_services_monitor/checkers/service_check.py`:
- Around line 264-266: Update the exception handler in the journal parsing flow
to catch only Exception, removing the redundant subprocess.TimeoutExpired entry
while preserving the existing warning log and empty-list return behavior.
In
`@health-monitors/system-services-monitor/system_services_monitor/checkers/watcher.py`:
- Around line 75-84: Separate the `ServiceChecker` setup and `"services"`
registration from the `enable_fabric_check` guard so systemd service checks
remain enabled independently. Keep only `FabricStateChecker` initialization and
`"fabric_state"` registration conditional on the fabric flag, and initialize
both `self._service_checker` and `self._fabric_state_checker` to `None`
unconditionally before the conditional setup.
In
`@health-monitors/system-services-monitor/system_services_monitor/platform_connector/event_processor.py`:
- Around line 197-221: Update the grpc.RpcError handler in the retry loop around
PlatformConnectorStub.HealthEventOccurredV1 so sleep(delay) and delay backoff
occur only when another retry remains; the final attempt must proceed directly
to failure return without sleeping.
In
`@health-monitors/system-services-monitor/system_services_monitor/platform_connector/tests/test_event_processor.py`:
- Around line 29-35: Update the processor fixture to accept pytest’s tmp_path
fixture and build the nonexistent socket path from it instead of hardcoding
/tmp/does-not-exist.sock. Keep the existing PlatformConnectorEventProcessor
configuration unchanged.
- Around line 64-73: Update test_new_state_sends_event_and_updates_cache to
inspect the cached entry rather than only asserting entity_cache length. Verify
the reservation exists under the expected entity key and that its cached value
contains the expected state after send_health_event_with_retries succeeds.
- Around line 115-131: Update the event processor tests to import and assert the
retry count using MAX_RETRIES instead of hardcoded 5. Extend the successful send
test for send_health_event_with_retries to verify HealthEventOccurredV1 receives
timeout=GRPC_SEND_TIMEOUT_SECS, and add coverage for the mid-retry socket
disappearance path where _is_platform_connector_socket_present becomes false,
asserting the send aborts as expected.
🪄 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: c4662925-3d42-4641-bb2a-b4fd9d46017e
📒 Files selected for processing (29)
.github/workflows/container-build-test.ymlhealth-monitors/system-services-monitor/Dockerfilehealth-monitors/system-services-monitor/Makefilehealth-monitors/system-services-monitor/README.mdhealth-monitors/system-services-monitor/pyproject.tomlhealth-monitors/system-services-monitor/system_services_monitor/__init__.pyhealth-monitors/system-services-monitor/system_services_monitor/checkers/__init__.pyhealth-monitors/system-services-monitor/system_services_monitor/checkers/fabric_state_check.pyhealth-monitors/system-services-monitor/system_services_monitor/checkers/service_check.pyhealth-monitors/system-services-monitor/system_services_monitor/checkers/tests/__init__.pyhealth-monitors/system-services-monitor/system_services_monitor/checkers/tests/test_fabric_state_check.pyhealth-monitors/system-services-monitor/system_services_monitor/checkers/tests/test_service_check.pyhealth-monitors/system-services-monitor/system_services_monitor/checkers/tests/test_watcher.pyhealth-monitors/system-services-monitor/system_services_monitor/checkers/types.pyhealth-monitors/system-services-monitor/system_services_monitor/checkers/watcher.pyhealth-monitors/system-services-monitor/system_services_monitor/cli.pyhealth-monitors/system-services-monitor/system_services_monitor/logger.pyhealth-monitors/system-services-monitor/system_services_monitor/metrics.pyhealth-monitors/system-services-monitor/system_services_monitor/platform_connector/__init__.pyhealth-monitors/system-services-monitor/system_services_monitor/platform_connector/event_processor.pyhealth-monitors/system-services-monitor/system_services_monitor/platform_connector/metrics.pyhealth-monitors/system-services-monitor/system_services_monitor/platform_connector/tests/__init__.pyhealth-monitors/system-services-monitor/system_services_monitor/platform_connector/tests/test_event_processor.pyhealth-monitors/system-services-monitor/system_services_monitor/protos/__init__.pyhealth-monitors/system-services-monitor/system_services_monitor/protos/health_event_pb2.pyhealth-monitors/system-services-monitor/system_services_monitor/protos/health_event_pb2.pyihealth-monitors/system-services-monitor/system_services_monitor/protos/health_event_pb2_grpc.pyhealth-monitors/system-services-monitor/system_services_monitor/tests/__init__.pyhealth-monitors/system-services-monitor/system_services_monitor/tests/test_cli.py
🚧 Files skipped from review as they are similar to previous changes (21)
- health-monitors/system-services-monitor/system_services_monitor/platform_connector/tests/init.py
- health-monitors/system-services-monitor/system_services_monitor/checkers/tests/init.py
- health-monitors/system-services-monitor/system_services_monitor/platform_connector/init.py
- health-monitors/system-services-monitor/system_services_monitor/tests/init.py
- health-monitors/system-services-monitor/system_services_monitor/checkers/types.py
- .github/workflows/container-build-test.yml
- health-monitors/system-services-monitor/system_services_monitor/protos/health_event_pb2.py
- health-monitors/system-services-monitor/system_services_monitor/checkers/init.py
- health-monitors/system-services-monitor/pyproject.toml
- health-monitors/system-services-monitor/system_services_monitor/platform_connector/metrics.py
- health-monitors/system-services-monitor/system_services_monitor/protos/init.py
- health-monitors/system-services-monitor/system_services_monitor/logger.py
- health-monitors/system-services-monitor/system_services_monitor/tests/test_cli.py
- health-monitors/system-services-monitor/system_services_monitor/protos/health_event_pb2.pyi
- health-monitors/system-services-monitor/system_services_monitor/metrics.py
- health-monitors/system-services-monitor/README.md
- health-monitors/system-services-monitor/system_services_monitor/checkers/tests/test_service_check.py
- health-monitors/system-services-monitor/system_services_monitor/init.py
- health-monitors/system-services-monitor/system_services_monitor/checkers/tests/test_watcher.py
- health-monitors/system-services-monitor/system_services_monitor/cli.py
- health-monitors/system-services-monitor/system_services_monitor/checkers/tests/test_fabric_state_check.py
…eview Four behavioral fixes raised by the ADR-049 review (PR NVIDIA#1380): - absent units are not failures: LoadState is now queried alongside ActiveState, and a unit with LoadState=not-found (e.g. a host without nvidia-fabricmanager installed) emits nothing instead of a false, fatal FABRIC_MANAGER_NOT_RUNNING / GPU_SERVICE_NOT_RUNNING - probe failure != service down: a systemctl/nsenter error or timeout means state UNKNOWN; the watcher now logs + counts it instead of emitting a fatal NOT_RUNNING event - FM_REGISTRATION_STUCK is now time-based: 'In Progress' only classifies as stuck after stuck_threshold consecutive polls (default 3, ~90s at the default 30s interval); the streak resets when registration completes. Boot-grace suppression now also applies to fabric-state results, matching the service checks - transition cache: errorCode is part of the cached identity (a code-only escalation like NOT_RUNNING -> NOT_RUNNING+FLAPPING now emits), and rollback is generation-safe (a failed older send no longer pops a newer reservation; object identity is the reservation token) Test suite: 53 passed (9 new tests covering each behavior). Signed-off-by: Anton Alexander <dmvevents@gmail.com>
dmvevents
added a commit
to dmvevents/NVSentinel
that referenced
this pull request
Aug 12, 2026
- specify per-probe timeouts (systemctl 10s, journalctl/nvidia-smi 15s) and the unknown-state rule: a failed/timed-out probe emits no event - add the applicability rule: LoadState=not-found units are skipped, never reported as *_NOT_RUNNING - define the FM_REGISTRATION_STUCK criterion: In Progress for stuck_threshold consecutive polls (default 3), streak resets on completion; boot grace applies to fabric-state checks too - document that boot-grace suppression happens at result generation and cannot poison the transition cache - document socket authorization: root-owned /var/run/nvsentinel hostPath, mounted only into NVSentinel DaemonSets; SO_PEERCRED as a shared platform-connector hardening item - cache identity now includes the normalized errorCode set; rollback is generation-safe (reservation-token semantics) - state explicitly that Phase 1 has no FM_UNRESPONSIVE code and what its future addition requires Matches implementation commit 12f6c07 on PR NVIDIA#1382. Signed-off-by: Anton Alexander <dmvevents@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
health-monitors/system-services-monitor/system_services_monitor/platform_connector/event_processor.py (2)
231-235: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSkip backoff after the final failed attempt.
The final
grpc.RpcErrorstill sleeps before the method returns. No retry remains at that point. This delays reservation rollback and the next health-check cycle.Proposed fix
except grpc.RpcError as e: log.error(f"Failed to send health event to UDS: {e}") - sleep(delay) - delay = min(delay * 1.5, MAX_DELAY) + if attempt < MAX_RETRIES - 1: + sleep(delay) + delay = min(delay * 1.5, MAX_DELAY) continue🤖 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/system-services-monitor/system_services_monitor/platform_connector/event_processor.py` around lines 231 - 235, Update the grpc.RpcError handling in the event-processing retry loop to sleep and increase delay only when another retry remains; on the final failed attempt, exit or return immediately after logging so reservation rollback and the next health-check cycle are not delayed.
154-158: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftSerialize delivery of state transitions.
The reservation prevents duplicate events. It does not preserve event order.
An older state can remain in flight while a newer state is sent on another gRPC channel. If platform-connector processes the newer event first, the older event can restore stale health and remediation state. The local cache still contains the newer state and suppresses correction.
Serialize state reservation and delivery through one ordered queue, or add a per-entity sequence that platform-connector uses to reject stale events. Add a concurrent test where an older send succeeds after a newer send.
🤖 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/system-services-monitor/system_services_monitor/platform_connector/event_processor.py` around lines 154 - 158, Update the event delivery flow around the entity_cache reservation so state transitions for each entity are serialized in order, using one ordered queue or an equivalent per-entity sequencing mechanism. Ensure platform-connector rejects stale sequences if ordering is enforced there, and add a concurrency test where an older send completes after a newer send while the final health and remediation state remains current.
🧹 Nitpick comments (3)
health-monitors/system-services-monitor/system_services_monitor/platform_connector/tests/test_event_processor.py (1)
100-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd type hints to
send_side_effect.The coding guidelines require type hints for all functions. The nested helper has neither a parameter nor a return annotation.
♻️ Proposed change
- def send_side_effect(events): + def send_side_effect(events: List[platformconnector_pb2.HealthEvent]) -> bool:Use whichever event type the module already imports; a simpler
events: listwith-> boolis also acceptable.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 `@health-monitors/system-services-monitor/system_services_monitor/platform_connector/tests/test_event_processor.py` around lines 100 - 108, Add parameter and return type annotations to the nested send_side_effect helper, using the module’s existing event type import for events (or list) and bool for the return value. Preserve its current callback and return behavior.Source: Coding guidelines
health-monitors/system-services-monitor/system_services_monitor/checkers/tests/test_watcher.py (1)
150-160: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the GPU service probe-error branch.
The new tests cover the absent GPU service path (
load_state == "not-found"). The sibling branch in_run_service_checksat lines 222-225 ofhealth-monitors/system-services-monitor/system_services_monitor/checkers/watcher.pyhandlesstatus.error is not Noneand is untested. That branch prevents a falseGpuServiceDownwhen the probe fails.💚 Proposed test
def test_gpu_service_down_is_non_fatal(self) -> None:Add before it:
def test_gpu_service_probe_error_yields_no_result(self) -> None: """A failed GPU service probe (state unknown) is not reported as down.""" w = _make_watcher(boot_grace_period=0) w._service_checker.check_fabric_manager.return_value = _fm_status(active=True) w._service_checker.check_all_gpu_services.return_value = { "nvidia-persistenced": ServiceStatus( name="nvidia-persistenced", active=False, error="systemctl show timed out", ), } results = w._run_service_checks() assert [r for r in results if r.check_name == "GpuServiceDown"] == []🤖 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/system-services-monitor/system_services_monitor/checkers/tests/test_watcher.py` around lines 150 - 160, Add a test beside test_absent_gpu_service_yields_no_result covering a ServiceStatus with error set and inactive state; configure the watcher and fabric-manager status identically, run _run_service_checks, and assert no GpuServiceDown result is emitted.health-monitors/system-services-monitor/system_services_monitor/checkers/fabric_state_check.py (1)
69-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the return type annotation to
__init__.The coding guidelines require type hints for all functions.
__init__has an annotated parameter but no return annotation.♻️ Proposed change
- def __init__(self, stuck_threshold: int = 3): + def __init__(self, stuck_threshold: int = 3) -> None: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 `@health-monitors/system-services-monitor/system_services_monitor/checkers/fabric_state_check.py` around lines 69 - 77, Add the `None` return type annotation to the `__init__` method of the relevant monitor checker, preserving its existing `stuck_threshold` parameter and initialization logic.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
`@health-monitors/system-services-monitor/system_services_monitor/checkers/fabric_state_check.py`:
- Around line 175-203: Update check() to prune _in_progress_streak entries for
GPU indices not observed in the current poll, including when statuses is empty
due to command failures or a missing binary. Perform this pruning before
processing current statuses, while preserving the existing consecutive-poll
counting for GPUs present in the poll.
In
`@health-monitors/system-services-monitor/system_services_monitor/checkers/watcher.py`:
- Around line 270-276: Update the watcher flow around
FabricStateChecker.to_check_results and _in_grace_period so in-progress streaks
are reset during boot grace, using a new
FabricStateChecker.reset_in_progress_streaks method that clears
_in_progress_streak. Ensure the reset occurs while grace is active so post-grace
polling starts the FM_REGISTRATION_STUCK threshold from a clean state.
---
Outside diff comments:
In
`@health-monitors/system-services-monitor/system_services_monitor/platform_connector/event_processor.py`:
- Around line 231-235: Update the grpc.RpcError handling in the event-processing
retry loop to sleep and increase delay only when another retry remains; on the
final failed attempt, exit or return immediately after logging so reservation
rollback and the next health-check cycle are not delayed.
- Around line 154-158: Update the event delivery flow around the entity_cache
reservation so state transitions for each entity are serialized in order, using
one ordered queue or an equivalent per-entity sequencing mechanism. Ensure
platform-connector rejects stale sequences if ordering is enforced there, and
add a concurrency test where an older send completes after a newer send while
the final health and remediation state remains current.
---
Nitpick comments:
In
`@health-monitors/system-services-monitor/system_services_monitor/checkers/fabric_state_check.py`:
- Around line 69-77: Add the `None` return type annotation to the `__init__`
method of the relevant monitor checker, preserving its existing
`stuck_threshold` parameter and initialization logic.
In
`@health-monitors/system-services-monitor/system_services_monitor/checkers/tests/test_watcher.py`:
- Around line 150-160: Add a test beside
test_absent_gpu_service_yields_no_result covering a ServiceStatus with error set
and inactive state; configure the watcher and fabric-manager status identically,
run _run_service_checks, and assert no GpuServiceDown result is emitted.
In
`@health-monitors/system-services-monitor/system_services_monitor/platform_connector/tests/test_event_processor.py`:
- Around line 100-108: Add parameter and return type annotations to the nested
send_side_effect helper, using the module’s existing event type import for
events (or list) and bool for the return value. Preserve its current callback
and return 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: 9ab6c88b-cd19-45c3-9cbf-a3a6f5e58f89
📒 Files selected for processing (7)
health-monitors/system-services-monitor/system_services_monitor/checkers/fabric_state_check.pyhealth-monitors/system-services-monitor/system_services_monitor/checkers/service_check.pyhealth-monitors/system-services-monitor/system_services_monitor/checkers/tests/test_fabric_state_check.pyhealth-monitors/system-services-monitor/system_services_monitor/checkers/tests/test_watcher.pyhealth-monitors/system-services-monitor/system_services_monitor/checkers/watcher.pyhealth-monitors/system-services-monitor/system_services_monitor/platform_connector/event_processor.pyhealth-monitors/system-services-monitor/system_services_monitor/platform_connector/tests/test_event_processor.py
🚧 Files skipped from review as they are similar to previous changes (1)
- health-monitors/system-services-monitor/system_services_monitor/checkers/service_check.py
Author
|
This PR stack (#1380–#1385) has been refreshed today:
Per copy-pr-bot, workflows have not yet run on NVIDIA's runners for any PR in this stack — only the DCO check has ever executed. Could a maintainer authorize CI validation for the stack (and take a first review pass on the ADR in #1380)? Happy to split, squash, or re-scope if that makes review easier. |
…king, streak hygiene Addresses the 08-12 CodeRabbit re-review round: - Callback dispatch is ordered: ThreadPoolExecutor(max_workers=1) turns the pool into a serial queue so a stale unhealthy result can never be applied after a newer healthy one, while callback latency stays out of the poll loop. - Flap tracking survives systemd NRestarts resets (reset-failed, unit re-creation, reboot): a decrease re-baselines and records one restart event instead of going quiet until the counter passes the old high-water mark. Probe failures now return None (unobserved) and leave the baseline untouched, distinct from a real 0. - The FM_REGISTRATION_STUCK streak is strictly consecutive: GPUs absent from a poll lose their streak, and boot grace evaluates without accumulating (advance_streak=False), so a normal slow startup cannot trip the alert on the first post-grace poll. - Black formatting applied; docstring coverage now 100% of defs (was 66.67% vs the 80% gate). ADR references updated 049 -> 050. - New tests: counter-reset re-baseline, unobserved-probe baseline, absent-GPU streak drop, grace-period non-accrual (57 passing). Signed-off-by: Anton Alexander <dmvevents@gmail.com>
Author
|
Status update across the series, correcting my 2026-08-12 comment above, which aged badly on two claims:
What changed today, per PR:
The |
dmvevents
added a commit
to dmvevents/NVSentinel
that referenced
this pull request
Aug 22, 2026
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>
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.
Per #891 split (3 of 5). Core Python package + unit tests.
Series progress
ci-system-services-monitor)What this contains
health-monitors/system-services-monitor/Python package (nocuda_validation.py)system_services_monitor/checkers/tests/What this does NOT contain
preflight-checks/cuda-validation/in follow-up PR per @XRFXLP reviewCR-ignored findings carried over from #891
This PR addresses these findings in scope:
cli.py--verbose flag handling (is_flag=True, default=False)cli.pytry/except aroundget_package_version(PackageNotFoundError fallback)Dockerfileapt cache mount (--mount=type=cache,target=/var/cache/apt,sharing=locked)logger.pywarning on unknown log levelsthreading.Lockinevent_processor.pyCR-ignored findings deferred to PR 5 (demo)
demos/.../servicemonitor.yamlalert specdemos/.../daemonset.yamlimage pin + livenessProbecc @XRFXLP
Summary by CodeRabbit
Release Notes
New Features
Documentation
Chores