chart: system-services-monitor subchart + parent registration - #1385
Open
dmvevents wants to merge 2 commits into
Open
chart: system-services-monitor subchart + parent registration#1385dmvevents wants to merge 2 commits into
dmvevents wants to merge 2 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a disabled-by-default ChangesSystem services monitor
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Monitor as system-services-monitor
participant Service as Headless Service
participant Prometheus
participant Rules as PrometheusRule
Monitor->>Service: Expose metrics port
Prometheus->>Service: Scrape /metrics through ServiceMonitor
Prometheus->>Rules: Evaluate gpu-node-health rules
Rules-->>Prometheus: Emit node health alerts
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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. |
Contributor
|
@dmvevents this PR now has merge conflicts with |
6 tasks
dmvevents
force-pushed
the
helm-system-services-monitor
branch
from
July 24, 2026 17:16
9440838 to
ebf37fa
Compare
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>
dmvevents
force-pushed
the
helm-system-services-monitor
branch
from
July 24, 2026 17:28
ebf37fa to
2083bfa
Compare
dmvevents
marked this pull request as ready for review
August 12, 2026 15:07
![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
🤖 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
`@distros/kubernetes/nvsentinel/charts/system-services-monitor/templates/daemonset.yaml`:
- Around line 102-118: Update the DaemonSet template to provide separate,
non-overlapping regular and Kata variants selected by nodeAffinity on the
kata.enabled label. Ensure the regular variant mounts /var/log, while the Kata
variant mounts both /run/log/journal and /var/log/journal, and document the
variant-selection and required mounts in the chart values documentation.
In
`@distros/kubernetes/nvsentinel/charts/system-services-monitor/templates/prometheusrule.yaml`:
- Around line 44-48: Update the FabricManagerFlapping expression in the
PrometheusRule template to use the configured flapWindow and flapThreshold
values rather than hardcoded 10m and 3, matching the values passed by the
DaemonSet to the monitor.
In `@distros/kubernetes/nvsentinel/charts/system-services-monitor/values.yaml`:
- Around line 15-83: Document every value in
distros/kubernetes/nvsentinel/charts/system-services-monitor/values.yaml,
including image, podAnnotations, resources, affinity, and logLevel, with
examples where settings are non-obvious; explicitly require YAML booleans for
enabled fields rather than quoted strings, and describe how regular versus Kata
DaemonSet variants are selected via kata.enabled and their differing host log
mounts. Also document global.systemServicesMonitor.enabled in
distros/kubernetes/nvsentinel/values.yaml lines 193-194, including that it must
be a YAML boolean.
🪄 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: 6829b459-0e2c-407a-bac9-e10bf1588e0e
📒 Files selected for processing (13)
distros/kubernetes/nvsentinel/Chart.yamldistros/kubernetes/nvsentinel/charts/system-services-monitor/.helmignoredistros/kubernetes/nvsentinel/charts/system-services-monitor/Chart.yamldistros/kubernetes/nvsentinel/charts/system-services-monitor/templates/_helpers.tpldistros/kubernetes/nvsentinel/charts/system-services-monitor/templates/clusterrole.yamldistros/kubernetes/nvsentinel/charts/system-services-monitor/templates/clusterrolebinding.yamldistros/kubernetes/nvsentinel/charts/system-services-monitor/templates/daemonset.yamldistros/kubernetes/nvsentinel/charts/system-services-monitor/templates/prometheusrule.yamldistros/kubernetes/nvsentinel/charts/system-services-monitor/templates/service.yamldistros/kubernetes/nvsentinel/charts/system-services-monitor/templates/serviceaccount.yamldistros/kubernetes/nvsentinel/charts/system-services-monitor/templates/servicemonitor.yamldistros/kubernetes/nvsentinel/charts/system-services-monitor/values.yamldistros/kubernetes/nvsentinel/values.yaml
Comment on lines
+102
to
+118
| volumes: | ||
| - name: var-run-vol | ||
| hostPath: | ||
| path: /var/run/nvsentinel | ||
| type: DirectoryOrCreate | ||
| nodeSelector: | ||
| {{- with (.Values.global.nodeSelector | default .Values.nodeSelector) }} | ||
| {{- toYaml . | nindent 8 }} | ||
| {{- end }} | ||
| {{- with (.Values.global.affinity | default .Values.affinity) }} | ||
| affinity: | ||
| {{- toYaml . | nindent 8 }} | ||
| {{- end }} | ||
| {{- with (.Values.global.tolerations | default .Values.tolerations) }} | ||
| tolerations: | ||
| {{- toYaml . | nindent 8 }} | ||
| {{- end }} |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Create separate Kata and regular DaemonSet variants.
Line 102 defines no required host log mount. Lines 107-118 do not select Kata nodes with nodeAffinity based on the kata.enabled label.
Create non-overlapping DaemonSet variants. Mount /var/log on regular nodes. Mount /run/log/journal and /var/log/journal on Kata nodes. Document the variant selection logic in the chart values documentation.
As per coding guidelines, “Use separate DaemonSet variants for kata vs regular nodes selected via nodeAffinity based on kata.enabled label” and require the distinct regular and Kata log mounts.
🤖 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
`@distros/kubernetes/nvsentinel/charts/system-services-monitor/templates/daemonset.yaml`
around lines 102 - 118, Update the DaemonSet template to provide separate,
non-overlapping regular and Kata variants selected by nodeAffinity on the
kata.enabled label. Ensure the regular variant mounts /var/log, while the Kata
variant mounts both /run/log/journal and /var/log/journal, and document the
variant-selection and required mounts in the chart values documentation.
Source: Coding guidelines
Comment on lines
+44
to
+48
| # Backed by the fabric_manager_restarts_total counter the app | ||
| # increments from the systemd NRestarts delta observed between poll | ||
| # cycles: more than 3 restarts within 10 minutes flags flapping. | ||
| - alert: FabricManagerFlapping | ||
| expr: increase(fabric_manager_restarts_total[10m]) > 3 |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the configured flap values in the alert expression.
Line 48 ignores flapWindow and flapThreshold. The DaemonSet passes both values to the monitor. A user override then makes the alert use different flap criteria than the application.
Proposed fix
- expr: increase(fabric_manager_restarts_total[10m]) > 3
+ expr: increase(fabric_manager_restarts_total[{{ .Values.flapWindow }}s]) > {{ .Values.flapThreshold }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Backed by the fabric_manager_restarts_total counter the app | |
| # increments from the systemd NRestarts delta observed between poll | |
| # cycles: more than 3 restarts within 10 minutes flags flapping. | |
| - alert: FabricManagerFlapping | |
| expr: increase(fabric_manager_restarts_total[10m]) > 3 | |
| # Backed by the fabric_manager_restarts_total counter the app | |
| # increments from the systemd NRestarts delta observed between poll | |
| # cycles: more than 3 restarts within 10 minutes flags flapping. | |
| - alert: FabricManagerFlapping | |
| expr: increase(fabric_manager_restarts_total[{{ .Values.flapWindow }}s]) > {{ .Values.flapThreshold }} |
🤖 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
`@distros/kubernetes/nvsentinel/charts/system-services-monitor/templates/prometheusrule.yaml`
around lines 44 - 48, Update the FabricManagerFlapping expression in the
PrometheusRule template to use the configured flapWindow and flapThreshold
values rather than hardcoded 10m and 3, matching the values passed by the
DaemonSet to the monitor.
Comment on lines
+15
to
+83
| image: | ||
| repository: ghcr.io/nvidia/nvsentinel/system-services-monitor | ||
| pullPolicy: IfNotPresent | ||
| tag: "" | ||
|
|
||
| podAnnotations: {} | ||
|
|
||
| resources: | ||
| limits: | ||
| cpu: 100m | ||
| memory: 128Mi | ||
| requests: | ||
| cpu: 50m | ||
| memory: 64Mi | ||
|
|
||
| # Polling interval (seconds) between service health check cycles. | ||
| # Passed as --poll-interval. | ||
| pollInterval: "30" | ||
|
|
||
| # Boot grace period (seconds) — unhealthy results within this window after | ||
| # startup are suppressed. Passed as --boot-grace-period. | ||
| bootGracePeriod: "300" | ||
|
|
||
| # Flap detection window (seconds) over which flapThreshold restarts are counted. | ||
| # Passed as --flap-window. | ||
| flapWindow: "600" | ||
|
|
||
| # Restart-count threshold within flapWindow that flags Fabric Manager as | ||
| # flapping. Passed as --flap-threshold. | ||
| flapThreshold: "3" | ||
|
|
||
| # Enable the Fabric Manager + GPU systemd service checks. When false the app | ||
| # is launched with --disable-fabric-check. | ||
| enableFabricCheck: true | ||
|
|
||
| # Processing strategy for emitted health events. | ||
| # valid values: EXECUTE_REMEDIATION, STORE_ONLY | ||
| processingStrategy: EXECUTE_REMEDIATION | ||
|
|
||
| # ServiceMonitor (Prometheus Operator) for auto-discovery. Disabled by default | ||
| # to match the sibling health-monitor charts, which do not ship one enabled. | ||
| serviceMonitor: | ||
| enabled: false | ||
| interval: 30s | ||
| scrapeTimeout: 10s | ||
| # Extra labels required by the cluster's Prometheus operator selector. | ||
| # Most operator installs expect `release: <prometheus-release>`; set it here | ||
| # when enabling, e.g. labels: { release: prometheus }. | ||
| labels: {} | ||
|
|
||
| # PrometheusRule with FabricManager / GPU service alert definitions. Disabled by | ||
| # default to match the sibling health-monitor charts. | ||
| prometheusRule: | ||
| enabled: false | ||
| labels: {} | ||
|
|
||
| # Scheduling. Default selector targets nodes that expose NVIDIA PCI devices | ||
| # (label set by the GPU Feature Discovery operator). | ||
| nodeSelector: | ||
| feature.node.kubernetes.io/pci-10de.present: "true" | ||
|
|
||
| tolerations: | ||
| - key: nvidia.com/gpu | ||
| operator: Exists | ||
| effect: NoSchedule | ||
|
|
||
| affinity: {} | ||
|
|
||
| logLevel: INFO |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the chart values and DaemonSet selection behavior.
Add inline comments for undocumented values such as image, podAnnotations, resources, affinity, and logLevel. Document that enabled fields require YAML boolean values, not quoted strings. Explain how regular-node and Kata-node DaemonSet variants use kata.enabled node affinity and different host log mounts.
distros/kubernetes/nvsentinel/charts/system-services-monitor/values.yaml#L15-L83: document every value, include examples for non-obvious settings, document literal boolean requirements, and describe Kata versus regular DaemonSet selection.distros/kubernetes/nvsentinel/values.yaml#L193-L194: documentglobal.systemServicesMonitor.enabledand state that it requires a YAML boolean value.
As per coding guidelines, values.yaml files must document all values, document truthy requirements where applicable, and explain DaemonSet variant selection logic.
📍 Affects 2 files
distros/kubernetes/nvsentinel/charts/system-services-monitor/values.yaml#L15-L83(this comment)distros/kubernetes/nvsentinel/values.yaml#L193-L194
🤖 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 `@distros/kubernetes/nvsentinel/charts/system-services-monitor/values.yaml`
around lines 15 - 83, Document every value in
distros/kubernetes/nvsentinel/charts/system-services-monitor/values.yaml,
including image, podAnnotations, resources, affinity, and logLevel, with
examples where settings are non-obvious; explicitly require YAML booleans for
enabled fields rather than quoted strings, and describe how regular versus Kata
DaemonSet variants are selected via kata.enabled and their differing host log
mounts. Also document global.systemServicesMonitor.enabled in
distros/kubernetes/nvsentinel/values.yaml lines 193-194, including that it must
be a YAML boolean.
Source: Coding guidelines
5 tasks
This was referenced Aug 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 (4 of 5). Marked draft — depends on the implementation PR (#1382) landing first.
Series progress
What this contains
distros/kubernetes/nvsentinel/charts/system-services-monitor/Helm subchart (10 files: Chart.yaml, values.yaml, .helmignore, 7 templates including DaemonSet, ServiceMonitor, ClusterRole/Binding, ServiceAccount, Service, PrometheusRule, _helpers.tpl)distros/kubernetes/nvsentinel/Chart.yaml— subchart dependency registrationdistros/kubernetes/nvsentinel/values.yaml— defaults wiringWhat this does NOT contain
Notes
The subchart's
.Values.global.*references (e.g.imagePullSecrets,metricsPort) are supplied by the parent chart at deploy time. Standalonehelm lintreports nil-pointer errors on these — that's expected and matches the same pattern in the merged sibling subchart (nic-health-monitor/templates/daemonset.yaml). Validated viahelm templateagainst the parent chart context.cc @XRFXLP
Summary by CodeRabbit