Skip to content

Honor GPUCluster priority classes in monitoring and validation - #2922

Open
ajavanma wants to merge 2 commits into
NVIDIA:mainfrom
ajavanma:fix/gpucluster-priority-class
Open

ajavanma wants to merge 2 commits into
NVIDIA:mainfrom
ajavanma:fix/gpucluster-priority-class

Conversation

@ajavanma

@ajavanma ajavanma commented Sep 18, 2026

Copy link
Copy Markdown

Description

Setting GPUCluster.spec.daemonsets.priorityClassName left DCGM, DCGM exporter, and DRA validator at system-node-critical, although their render data already carried the requested value. Honor the configured name in these three DaemonSets, preserving system-node-critical when unset.

Quote the resolved value so names such as on and null remain strings through YAML decoding. Cover ordinary custom names, an unset value, a YAML boolean keyword, and a YAML null keyword through the real renderer for all three operands (12 cases). Existing golden manifests remain unchanged.

Checklist

  • No secrets, sensitive information, or unrelated changes
  • Lint checks passing (make lint)
  • Generated assets in-sync (make validate-generated-assets)
  • Go mod artifacts in-sync (make validate-modules)
  • Test cases are added for new code paths

Testing

  • Original custom-name regression: all three custom cases failed on the upstream base and passed with the initial fix.
  • Quoting regression: before quoting, all six on/null cases failed while the six existing custom/default cases passed. With quoting, all 12 cases pass through real rendering and typed DaemonSet conversion.
  • make fmt license-check: passed.
  • make coverage: passed, including build and the full unit suite (21 tested packages); existing golden manifests unchanged.
  • make lint: passed with zero issues.
  • make validate-modules: passed for the source module, vendor tree, and tools module.
  • make validate-generated-assets: passed; no generated-file drift.
  • make check-third-party-notices: passed for all 123 packages on Linux AMD64 and ARM64; notices unchanged.
  • Current local environment: Linux ARM64, Go 1.27.1 and golangci-lint 2.13.1 matching CI pins.
  • No live Kubernetes/GPU or end-to-end scheduling validation performed.

Signed-off-by: Arash Javanmardi <arash@javanmardi.info>
@copy-pr-bot

copy-pr-bot Bot commented Sep 18, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Sep 18, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

📝 Walkthrough

Walkthrough

The DCGM, DCGM exporter, and DRA validator DaemonSet templates now use .Daemonsets.PriorityClassName. They default to system-node-critical when the value is unset. A table-driven test verifies custom and default values across all three renderers.

Priority: ➖ Normal

Merge Risk: 🔵 Low · up to aa35f

Clusters using YAML-keyword priority-class names can fail to render the affected DaemonSets correctly. Quote the configured value and add the regression case before merging.


Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.

🟡 Other comments (1)
manifests/state-dcgm/0500_daemonset.yaml-39-39 (1)

39-39: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Quote the defaulted PriorityClassName in all three templates. DaemonsetsSpec.PriorityClassName is a string, and values such as "on", "off", "true", and "null" are valid string values. The renderer decodes these templates through Kubernetes YAML-to-JSON conversion. Unquoted on, off, and true become boolean values, while null becomes nil. Typed DaemonSet conversion then rejects the boolean values and loses the configured "null" value.

Apply the same fix at all three sites:

  • manifests/state-dcgm/0500_daemonset.yaml#L39
  • manifests/state-dcgm-exporter/0700_daemonset.yaml#L42
  • manifests/state-dra-validation/0500_daemonset.yaml#L59
priorityClassName: {{ .Daemonsets.PriorityClassName | default "system-node-critical" | quote }}

Add an "on" case to TestGPUClusterDaemonsetsPriorityClass. The test exercises all three renderers and converts the rendered object to a typed DaemonSet, so it will fail without the quote and assert that the value remains "on" with the fix.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: QUIET

Plan: Enterprise

Run ID: 8c6b81a7-7df7-4de8-954b-112d646718c9

📥 Commits

Reviewing files that changed from the base of the PR and between 93c0ff5 and aa35f6e.

📒 Files selected for processing (4)
  • internal/state/gpucluster_render_test.go
  • manifests/state-dcgm-exporter/0700_daemonset.yaml
  • manifests/state-dcgm/0500_daemonset.yaml
  • manifests/state-dra-validation/0500_daemonset.yaml

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.

Signed-off-by: Arash Javanmardi <arash@javanmardi.info>
@myeolenv

Copy link
Copy Markdown

Thanks for the contribution. We will review this PR soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants