feat: add helm chart for metadata-collector by XRFXLP · Pull Request #281 · NVIDIA/NVSentinel

XRFXLP · 2025-11-06T10:52:01Z

Summary

$ helm template nvsentinel distros/kubernetes/nvsentinel/ \
  --show-only charts/metadata-collector/templates/daemonset.yaml
---
# Source: nvsentinel/charts/metadata-collector/templates/daemonset.yaml
# Copyright (c) 2025, NVIDIA CORPORATION.  All rights reserved.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
#     http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

apiVersion: apps/v1
kind: DaemonSet
metadata:
  name: metadata-collector
  labels:
    helm.sh/chart: metadata-collector-0.1.0
    app.kubernetes.io/name: metadata-collector
    app.kubernetes.io/instance: nvsentinel
    app.kubernetes.io/version: "1.0.0"
    app.kubernetes.io/managed-by: Helm
spec:
  updateStrategy:
    type: RollingUpdate
    rollingUpdate:
      maxUnavailable: 1
  selector:
    matchLabels:
      app.kubernetes.io/name: metadata-collector
      app.kubernetes.io/instance: nvsentinel
  template:
    metadata:
      labels:
        app.kubernetes.io/name: metadata-collector
        app.kubernetes.io/instance: nvsentinel
    spec:
      hostNetwork: true
      hostPID: true
      initContainers:
        - name: metadata-collector
          securityContext:
            runAsUser: 0
            runAsGroup: 0
            privileged: true
            capabilities:
              add:
                - SYS_ADMIN
          image: "ghcr.io/nvidia/nvsentinel/metadata-collector:main"
          imagePullPolicy: IfNotPresent
          args:
            - --output-path=/var/lib/nvsentinel/gpu_metadata.json
          env:
            - name: NODE_NAME
              valueFrom:
                fieldRef:
                  apiVersion: v1
                  fieldPath: spec.nodeName
            - name: NVIDIA_VISIBLE_DEVICES
              value: all
            - name: NVIDIA_DRIVER_CAPABILITIES
              value: utility
          resources:
            limits:
              cpu: 500m
              memory: 256Mi
            requests:
              cpu: 100m
              memory: 128Mi
          volumeMounts:
            - name: output
              mountPath: /var/lib/nvsentinel
            - name: sys
              mountPath: /sys
              readOnly: true
      containers:
        - name: pause
          image: "gcr.io/google-containers/pause:3.0"
          imagePullPolicy: IfNotPresent
      volumes:
        - name: output
          hostPath:
            path: /var/lib/nvsentinel
            type: DirectoryOrCreate
        - name: sys
          hostPath:
            path: /sys
            type: Directory
      nodeSelector:
        nvidia.com/gpu.present: "true"
        nvsentinel.dgxc.nvidia.com/driver.installed: "true"

Type of Change

Component(s) Affected

Testing

Tests pass locally
Manual testing completed
No breaking changes (or documented)

Checklist

Self-review completed
Documentation updated (if needed)
Ready for review

Summary by CodeRabbit

New Features
- Added GPU Metadata Collector component running as a DaemonSet on GPU nodes; collects and stores GPU metadata in JSON.
- Enabled by default with configurable image, pause image, resource requests/limits, and output path.
- Supports optional node selectors, affinity, tolerations, annotations, and image pull settings for fine-grained deployment control.
- Helm chart and dependency entry added for the new component.

Signed-off-by: Ajay Mishra <[email protected]>

coderabbitai · 2025-11-06T10:52:10Z

Walkthrough

Adds a new Helm subchart "metadata-collector" to nvsentinel, including chart metadata, helpers, values, and a DaemonSet template; registers it as a dependency in the parent Chart.yaml and enables it via global.metadataCollector.enabled.

Changes

Cohort / File(s)	Summary
Parent Chart / Defaults `distros/kubernetes/nvsentinel/Chart.yaml`, `distros/kubernetes/nvsentinel/values.yaml`, `distros/kubernetes/nvsentinel/values-tilt.yaml`	Registered `metadata-collector` as a chart dependency (version `0.1.0`) with condition `global.metadataCollector.enabled`; added `global.metadataCollector.enabled` (true in values.yaml, false in values-tilt.yaml).
Subchart Metadata `distros/kubernetes/nvsentinel/charts/metadata-collector/Chart.yaml`	New Helm Chart declaration for `metadata-collector` (apiVersion: v2, version `0.1.0`, appVersion `1.0.0`, type `application`).
Template Helpers `distros/kubernetes/nvsentinel/charts/metadata-collector/templates/_helpers.tpl`	Added helper templates: `metadata-collector.name`, `metadata-collector.fullname`, `metadata-collector.chart`, `metadata-collector.labels`, and `metadata-collector.selectorLabels`.
DaemonSet Template `distros/kubernetes/nvsentinel/charts/metadata-collector/templates/daemonset.yaml`	New DaemonSet Helm template with hostNetwork/hostPID, privileged initContainer (SYS_ADMIN), hostPath volumes, `/sys` mount, GPU/driver node selectors, and conditional annotations, imagePullSecrets, affinity, and tolerations driven by values.
Subchart Values `distros/kubernetes/nvsentinel/charts/metadata-collector/values.yaml`	New default values: image repo `ghcr.io/nvidia/nvsentinel/metadata-collector`, pause image, resource requests/limits, `outputPath: /var/lib/nvsentinel/gpu_metadata.json`, and related pod configuration.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Parent as Parent Chart (nvsentinel)
  participant Sub as metadata-collector (subchart)
  participant Kube as Kubernetes API
  Parent->>Sub: dependency declared (condition: global.metadataCollector.enabled)
  Parent->>Sub: render templates with merged values
  Sub->>Kube: apply DaemonSet manifest
  Kube-->>Sub: schedule DaemonSet pods on nodes matching GPU + driver selectors
  note right of Kube `#DFF2E1`: InitContainer runs privileged\nMain container writes to hostPath (outputPath)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Review focus:
- Privileged initContainer and SYS_ADMIN capability in DaemonSet.
- Node selector logic for GPU/driver detection and any merging with .Values.
- Conditional templating for annotations, imagePullSecrets, affinity, and tolerations.
- Image tag fallback and resource defaults in values.

Poem

🐰 A rabbit scurries, clipboard held tight,
Charts stitched together beneath moonlight,
DaemonSet hums where GPUs gleam,
Helpers trim names, values shape the dream,
Metadata gathered — a tidy delight.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title 'feat: add helm chart for metadata-collector' directly and accurately describes the main change: adding a Helm chart for the metadata-collector component.

✨ Finishing touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Signed-off-by: Ajay Mishra <[email protected]>

coderabbitai

Actionable comments posted: 2

🧹 Nitpick comments (1)

distros/kubernetes/nvsentinel/charts/metadata-collector/templates/daemonset.yaml (1)
102-102: Remove trailing blank line.

Apply this diff to remove the extra blank line:
       {{- end }}
-

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 19b0c91 and f3dce55.

📒 Files selected for processing (6)

distros/kubernetes/nvsentinel/Chart.yaml (1 hunks)
distros/kubernetes/nvsentinel/charts/metadata-collector/Chart.yaml (1 hunks)
distros/kubernetes/nvsentinel/charts/metadata-collector/templates/_helpers.tpl (1 hunks)
distros/kubernetes/nvsentinel/charts/metadata-collector/templates/daemonset.yaml (1 hunks)
distros/kubernetes/nvsentinel/charts/metadata-collector/values.yaml (1 hunks)
distros/kubernetes/nvsentinel/values.yaml (1 hunks)

🧰 Additional context used

🪛 YAMLlint (1.37.1)

distros/kubernetes/nvsentinel/charts/metadata-collector/templates/daemonset.yaml

[warning] 102-102: too many blank lines (1 > 0)

(empty-lines)

[error] 20-20: syntax error: expected the node content, but found '-'

(syntax)

🔇 Additional comments (8)

distros/kubernetes/nvsentinel/values.yaml (1)

46-47: LGTM!

The flag follows the established pattern for component enablement and is properly namespaced under global.

distros/kubernetes/nvsentinel/Chart.yaml (1)

55-57: LGTM!

The dependency declaration is consistent with the existing pattern and correctly references the new global.metadataCollector.enabled condition.

distros/kubernetes/nvsentinel/charts/metadata-collector/Chart.yaml (1)

14-22: LGTM!

Standard Helm v3 chart definition with all required fields properly configured.

distros/kubernetes/nvsentinel/charts/metadata-collector/values.yaml (1)

30-36: LGTM!

Resource limits and requests are appropriately defined for a metadata collection workload.

distros/kubernetes/nvsentinel/charts/metadata-collector/templates/daemonset.yaml (3)

42-52: Verify the necessity of elevated privileges.

The DaemonSet runs with extremely high privileges:

hostNetwork: true and hostPID: true

privileged: true

Root user (UID/GID 0)

SYS_ADMIN capability

While GPU metadata collection may require elevated access, these combined privileges grant nearly unrestricted host access and significantly expand the attack surface.

Please verify:

Is hostNetwork actually required for GPU metadata collection, or could the container use the pod network?

Is full privileged mode necessary, or would specific capabilities (e.g., just SYS_ADMIN) suffice?

Is hostPID access required for this workload?

If these privileges are required, consider documenting the specific operations that necessitate them in the chart's README or comments.

53-53: LGTM!

The image tag fallback logic correctly prioritizes local values, then global values, then falls back to the chart's AppVersion.

88-93: LGTM!

The node selector correctly targets GPU nodes with installed NVIDIA drivers, and properly merges additional selectors from values.

distros/kubernetes/nvsentinel/charts/metadata-collector/templates/_helpers.tpl (1)

1-40: LGTM!

The helper templates follow standard Helm patterns for generating names, labels, and selectors. The implementations correctly handle truncation and include all recommended Kubernetes labels.

Signed-off-by: Ajay Mishra <[email protected]>

coderabbitai

Actionable comments posted: 1

♻️ Duplicate comments (1)

distros/kubernetes/nvsentinel/charts/metadata-collector/templates/daemonset.yaml (1)
71-71: Fix imagePullPolicy reference for pause container.

Line 71 references .Values.image.pullPolicy for the pause container, but the pause container uses .Values.pauseImage.* configuration (see line 70). This causes the pause container's image pull policy to be controlled by the main container's image settings, which is inconsistent.

Apply this diff to use the correct pull policy for the pause container:
       - name: pause
         image: "{{ .Values.pauseImage.repository }}:{{ .Values.pauseImage.tag }}"
-        imagePullPolicy: {{ .Values.image.pullPolicy }}
+        imagePullPolicy: IfNotPresent
Alternatively, add a dedicated pullPolicy field under .Values.pauseImage in values.yaml and reference it as .Values.pauseImage.pullPolicy here.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 07bee90 and ad9779e.

📒 Files selected for processing (3)

distros/kubernetes/nvsentinel/charts/metadata-collector/templates/daemonset.yaml (1 hunks)
distros/kubernetes/nvsentinel/charts/metadata-collector/values.yaml (1 hunks)
distros/kubernetes/nvsentinel/values-tilt.yaml (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

distros/kubernetes/nvsentinel/charts/metadata-collector/values.yaml

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-11-06T10:59:26.666Z

Learnt from: XRFXLP
Repo: NVIDIA/NVSentinel PR: 281
File: distros/kubernetes/nvsentinel/charts/metadata-collector/values.yaml:20-0
Timestamp: 2025-11-06T10:59:26.666Z
Learning: The pause container image registry.k8s.io/pause:3.9+ requires eBPF and cgroup v2 support (kernel 4.15+, CONFIG_BPF, CONFIG_CGROUP_BPF, /sys/fs/bpf mounted). Older pause:3.0 images from gcr.io/google-containers/pause work on systems without full BPF/cgroup v2 support. When suggesting pause image updates, verify the target environment has necessary kernel features to avoid BPF_PROG_ATTACH errors.

Applied to files:

distros/kubernetes/nvsentinel/charts/metadata-collector/templates/daemonset.yaml

🪛 YAMLlint (1.37.1)

distros/kubernetes/nvsentinel/charts/metadata-collector/templates/daemonset.yaml

[warning] 95-95: too many blank lines (1 > 0)

(empty-lines)

[error] 20-20: syntax error: expected the node content, but found '-'

(syntax)

🔇 Additional comments (3)

distros/kubernetes/nvsentinel/values-tilt.yaml (1)

67-68: LGTM!

The addition of metadataCollector.enabled: false follows the established pattern for feature flags and is appropriately disabled in the Tilt environment.
distros/kubernetes/nvsentinel/charts/metadata-collector/templates/daemonset.yaml (2)
46-51: Clarify whether explicit SYS_ADMIN capability is needed.

The initContainer is already marked as privileged: true (line 49), which grants all Linux capabilities including SYS_ADMIN. Adding an explicit capabilities.add: [SYS_ADMIN] block would be redundant but may improve clarity about the container's intent.

Verify with the metadata-collector team whether an explicit capability declaration is required for your security/compliance policies or if the privileged context is sufficient. If explicit declaration is preferred, add it as shown below.

If an explicit capability declaration is needed, apply this diff:
       - name: metadata-collector
         securityContext:
           runAsUser: 0
           runAsGroup: 0
           privileged: true
+          capabilities:
+            add:
+            - SYS_ADMIN
54-59: Verify NVIDIA environment variables.

The PR objectives mention that the initContainer should set NVIDIA_VISIBLE_DEVICES=all and NVIDIA_DRIVER_CAPABILITIES=utility, but these are not present in the template. Confirm whether:

These variables should be explicitly set in the container's env section (as shown in lines 55–59).

They are instead configured via values overrides in .Values.env or similar.

They are intentionally omitted and handled by the NVIDIA driver runtime directly.

If they should be part of the template, add them to the env section.

Signed-off-by: Ajay Mishra <[email protected]>

github-actions · 2026-05-26T07:20:19Z

This pull request has been automatically locked since it has been closed for 90 days with no further activity. Please open a new pull request for related changes.

feat: add helm chart for metadata-collector

f3dce55

Signed-off-by: Ajay Mishra <[email protected]>

chore: remove runtime class ref

07bee90

Signed-off-by: Ajay Mishra <[email protected]>

coderabbitai Bot reviewed Nov 6, 2025

View reviewed changes

Comment thread distros/kubernetes/nvsentinel/charts/metadata-collector/templates/daemonset.yaml

Comment thread distros/kubernetes/nvsentinel/charts/metadata-collector/values.yaml Outdated

lalitadithya reviewed Nov 6, 2025

View reviewed changes

Comment thread distros/kubernetes/nvsentinel/charts/metadata-collector/templates/daemonset.yaml Outdated

Comment thread distros/kubernetes/nvsentinel/charts/metadata-collector/templates/daemonset.yaml Outdated

lalitadithya reviewed Nov 6, 2025

View reviewed changes

Comment thread distros/kubernetes/nvsentinel/values.yaml

chore: address review comments

ad9779e

Signed-off-by: Ajay Mishra <[email protected]>

lalitadithya approved these changes Nov 6, 2025

View reviewed changes

lalitadithya added this to the v0.3.0 milestone Nov 6, 2025

lalitadithya linked an issue Nov 6, 2025 that may be closed by this pull request

[Feature]: Add GPU ID tracking to syslog health monitor #120

Closed

1 task

lalitadithya enabled auto-merge (squash) November 6, 2025 11:19

coderabbitai Bot reviewed Nov 6, 2025

View reviewed changes

Comment thread distros/kubernetes/nvsentinel/charts/metadata-collector/templates/daemonset.yaml

lalitadithya merged commit f565613 into NVIDIA:main Nov 6, 2025
33 of 34 checks passed

dims pushed a commit to dims/NVSentinel that referenced this pull request Nov 7, 2025

feat: add helm chart for metadata-collector (NVIDIA#281)

645ff2d

Signed-off-by: Ajay Mishra <[email protected]>

github-actions Bot locked as resolved and limited conversation to collaborators May 26, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: add helm chart for metadata-collector#281

feat: add helm chart for metadata-collector#281
lalitadithya merged 3 commits into
NVIDIA:mainfrom
XRFXLP:120-helm

XRFXLP commented Nov 6, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Nov 6, 2025 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented May 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

XRFXLP commented Nov 6, 2025 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Type of Change

Component(s) Affected

Testing

Checklist

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented May 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

XRFXLP commented Nov 6, 2025 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Nov 6, 2025 •

edited

Loading