Expose priorityClassName for alpha and zero pods - #139
Conversation
- Add an optional priorityClassName to the alpha and zero StatefulSets so pods can reference an existing PriorityClass; defaults to unset - Deliberately do not expose a pod-level preemptionPolicy: Kubernetes forbids a pod whose preemptionPolicy differs from its PriorityClass, so it belongs on the PriorityClass itself (matches the Bitnami, Consul, and prometheus charts) - Add the chart's first helm-unittest suites covering the rendered field - Add a default-priority helmfile env and example values for manual live-cluster testing (v0-style, not run in CI) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR adds optional priorityClassName values for Dgraph Alpha and Zero pods so operators can assign an existing Kubernetes PriorityClass (and thereby control preemption behavior via the PriorityClass itself).
Changes:
- Exposes
alpha.priorityClassNameandzero.priorityClassNamein chart values and wires them into the Alpha/Zero StatefulSet pod specs. - Adds initial
helm-unittestcoverage verifying the field is omitted by default and rendered when set. - Adds a
default-priorityhelmfile environment and example values file for manual cluster testing; bumps chart version.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
helmfiles/tests/helmfile.yaml |
Adds default-priority test environment entry. |
charts/dgraph/values.yaml |
Documents new optional alpha/zero.priorityClassName values. |
charts/dgraph/tests/zero_priority_test.yaml |
Helm-unittest coverage for zero priorityClassName rendering behavior. |
charts/dgraph/tests/alpha_priority_test.yaml |
Helm-unittest coverage for alpha priorityClassName rendering behavior. |
charts/dgraph/templates/zero/statefulset.yaml |
Renders spec.template.spec.priorityClassName for zero when set. |
charts/dgraph/templates/alpha/statefulset.yaml |
Renders spec.template.spec.priorityClassName for alpha when set. |
charts/dgraph/README.md |
Documents new values in the chart parameters table. |
charts/dgraph/example_values/default-priority-config.yaml |
Provides example values to exercise the new fields in a live cluster. |
charts/dgraph/Chart.yaml |
Bumps chart version to 25.3.1-preview3. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| schedulerName: {{ .Values.zero.schedulerName }} | ||
| {{- end }} | ||
| {{- if .Values.zero.priorityClassName }} | ||
| priorityClassName: {{ .Values.zero.priorityClassName }} |
| schedulerName: {{ .Values.alpha.schedulerName }} | ||
| {{- end }} | ||
| {{- if .Values.alpha.priorityClassName }} | ||
| priorityClassName: {{ .Values.alpha.priorityClassName }} |
| @@ -0,0 +1,15 @@ | |||
| suite: alpha priorityClassName | |||
There was a problem hiding this comment.
Good to see unit tests land here, but two things before this ships.
tests/ isn't in .helmignore, which currently only excludes example_values/ and scripts/, so both suites get packaged into the released tarball. Confirmed against this branch:
$ helm package charts/dgraph && tar tzf dgraph-25.3.1-preview3.tgz | grep -v templates/
dgraph/Chart.yaml
dgraph/values.yaml
dgraph/.helmignore
dgraph/README.md
dgraph/tests/alpha_priority_test.yaml
dgraph/tests/zero_priority_test.yaml
One line in .helmignore fixes it, and since this PR establishes the tests/ convention it should land here.
Second, nothing runs these. cd-charts.yml is the only workflow and it just runs chart-releaser on push to main, so there's no CI job, Makefile target, or doc line that invokes helm unittest. A test framework with no runner rots. Can you add a minimal PR workflow that does helm lint plus helm unittest charts/dgraph?
| alpha-tls: | ||
| default-json: | ||
| default-yaml: | ||
| default-priority: |
There was a problem hiding this comment.
This env isn't reachable through the documented workflow. helmfiles/tests/README.md has four TESTS="..." lists (lines 50, 58, 66, and the cleanup list at 112) and default-priority isn't in any of them, so nobody following the README will run it, and its namespace gets skipped at teardown.
It would also fail if they did run it. The values file notes that dgraph-test-high has to exist first, but nothing in the repo creates it, so helmfile --environment default-priority apply gets rejected by the priority admission controller. Compare the TLS envs: those get scripts/make_tls_secrets.sh plus a setup section in the README. This needs the equivalent, even if it's just a PriorityClass manifest and a kubectl apply line.
Minor: the env list is otherwise alphabetical, so this belongs above default-json.
| ## Exercises the priorityClassName field on the alpha and zero StatefulSets. | ||
| ## The referenced PriorityClass (dgraph-test-high) must exist on the cluster | ||
| ## before deploying, otherwise the priority admission controller rejects the pods. | ||
| alpha: |
There was a problem hiding this comment.
This is a verbatim copy of default-yaml-config.yaml with two lines added. Those ~45 lines of config now have to be kept in sync by hand, and they won't be. helmfile.yaml already lists multiple values files per release, so layering a two-key overlay on top of default-yaml-config.yaml gets the same coverage without the drift.
| | `zero.podLabels` | Specify additional labels for template metadata | `{}` | | ||
| | `zero.updateStrategy` | Strategy for upgrading zero nodes | `RollingUpdate` | | ||
| | `zero.schedulerName` | Configure an explicit scheduler | `nil` | | ||
| | `zero.priorityClassName` | PriorityClass to protect zero pods from preemption | `nil` | |
There was a problem hiding this comment.
"protect zero pods from preemption" is half of what the field does. Assigning a priority also lets these pods preempt other workloads, which is the part an operator wants to know before turning it on. Suggest something like "Name of an existing PriorityClass to assign to zero pods", and the same for the alpha.priorityClassName row.
| {{- if .Values.alpha.schedulerName }} | ||
| schedulerName: {{ .Values.alpha.schedulerName }} | ||
| {{- end }} | ||
| {{- if .Values.alpha.priorityClassName }} |
There was a problem hiding this comment.
ratel and backups both expose schedulerName alongside alpha and zero. Was leaving priorityClassName off them a deliberate scope call since #129 only names alpha and zero, or a follow-up?
Closes #129.
Problem
The alpha and zero StatefulSets have no way to reference a PriorityClass, so their pods can't be protected from preemption on contended clusters.
Solution
Expose an optional
alpha.priorityClassNameandzero.priorityClassName(default unset). Pod-levelpreemptionPolicyis intentionally not exposed: Kubernetes rejects a pod whosepreemptionPolicydiffers from its PriorityClass, so it belongs on the PriorityClass itself — matching the convention of the Bitnami, Consul, and prometheus charts.The details
Adds the chart's first
helm-unittestsuites (charts/dgraph/tests/) covering the rendered field, plus adefault-priorityhelmfile env + example values for manual live-cluster testing.🤖 Generated with Claude Code