orchestratord: allow setting a PriorityClass on environmentd and clusterd pods - #38457
Conversation
…terd pods The operator sets no priority class on environmentd or clusterd pods, so they run at priority 0. Any higher-priority pod can evict them when a node fills up. A clusterd replica preempted mid-hydration aborts the deploy, and the chart has no way to raise these pods. Add `environmentd.priorityClassName` and `clusterd.priorityClassName`, next to the per-component node selectors, affinities and tolerations both components already have. Each becomes an orchestratord argument. `environmentd.priorityClassName` sets `priorityClassName` on the environmentd StatefulSet's pod template. `clusterd.priorityClassName` is forwarded to environmentd as `--orchestrator-kubernetes-priority-class-name` and applied there to clusterd pod specs. Both default to unset, which renders no argument and leaves every pod spec unchanged. The forwarded argument is gated on `meets_minimum_version(&V26_40_0)`, matching how `clusterd.affinity` and `clusterd.tolerations` are gated on `V144`. environmentd exits on an unrecognized argument, so an instance pinned to an older `environmentdImageRef` would otherwise fail to start once the value was set. balancerd and console are left out. Both default to two replicas and neither holds state, so an eviction costs a connection rather than a rehydration. The negative chart assertions use `notMatchRegex`. `notContains` compares whole list elements, so it cannot fail against a flag rendered as `--flag=value`. The existing `schedulerName` assertion has the same defect and is fixed here.
|
All contributors have signed the CLA. |
|
I have read the Contributor License Agreement (CLA) and I hereby sign the CLA. |
| # -- PriorityClass to use for environmentd pods spawned by the operator. The | ||
| # PriorityClass must already exist. Kubernetes rejects a pod that names one | ||
| # it cannot resolve, so a typo here stops these pods being created at all. | ||
| priorityClassName: |
There was a problem hiding this comment.
Most helm charts default priorityClassName to "" rather than null (having an empty yaml value isn't a great yaml practice). Since you are using an if ...priorityClassName in your helm template, nothing changes on that side.
8dd73ea to
170c3da
Compare
doy-materialize
left a comment
doy-materialize
left a comment
There was a problem hiding this comment.
this looks fine to me, i've fixed up the couple of test failures and review comments
QA LLM Review (Post Merge)@jazz-mo — an automated review of commit 1. HIGH -- clusterd version gate is set below the release that actually has the flag, so v26.40/v26.41 environmentd gets an argument it rejects
The DetailsVerified against the tags: The failure mode is the one the gate exists to prevent, and the file says so at The precedent for the convention is #37260, which added Fix: -static V26_40_0: LazyLock<Version> = LazyLock::new(|| Version {
+static V26_42_0: LazyLock<Version> = LazyLock::new(|| Version {
major: 26,
- minor: 40,
+ minor: 42,
patch: 0,
|
|
(that issue was resolved in #38756 ) |
Restores the commits that [`auto_cut_release.py`](https://github.com/MaterializeInc/materialize/blob/main/misc/python/materialize/release/auto_cut_release.py) produced but could not push when the v26.42 cut ran on 2026-09-11, plus the doc file dropped by the v26.41 cut on 2026-09-04. Same situation as #38567. ## What happened [`Cut release` run 34553906086](https://github.com/MaterializeInc/release/actions/runs/34553906086) tagged and pushed `v26.42.0-rc.1` successfully, then failed on its last step: ``` remote: error: GH006: Protected branch update failed for refs/heads/main. remote: - Changes must be made through a pull request. remote: - 2 of 2 required status checks are expected. ``` `auto_cut_release.py` pushes the post-tag bump straight to `main`, which branch protection now refuses. The run made both commits on the runner and lost them. The same thing happened on 2026-09-04 and on 2026-08-28; only the 08-28 one was restored (#38567). ## What this PR contains - **`release: bump to version v26.43.0-dev.0`** - produced by running `bin/bump-version v26.43.0-dev.0`, not by hand. 15 files, +29/-29, matching the diffstat the failed run reported. - **`release: create doc file for v26.42`** - dropped by the 2026-09-04 cut and never restored. `v26.42.md` is missing from `main` today even though v26.42 is the release currently being qualified. - **`release: create doc file for v26.43`** - dropped by this week's cut. ## Two lines worth a second look `bin/bump-version` regenerates `doc/user/data/self_managed/materialize_operator_chart_parameter.yml` and rewrites `misc/helm-charts/operator/tests/deployment_test.yaml` through ruamel, and both pick up changes that are not version bumps: - The `priorityClass` `description` added by #38457 is emptied, because `value_descriptions.yml.gotmpl` does not emit a description for that field. If that text should survive regeneration it needs to come from the template. - `statementLoggingMaxSampleRate: null` becomes `statementLoggingMaxSampleRate:`, a ruamel round-trip artifact. Semantically identical YAML. These are kept rather than stripped so this PR is exactly what the workflow would have pushed; any future bump reproduces them. `check-helm-docs.sh` only covers `misc/helm-charts/operator`, and the regenerated `README.md` matches it. ## Follow-up This will recur every Friday until https://linear.app/materializeinc/issue/SEC-725/replace-gh-token-for-the-release-automation-currently-a-personal-pat is implemented. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01XzQZdcie5q75t4NJjUQDeB
4 participants
Why
The operator sets no priority class on environmentd or clusterd pods. Higher priority pods evict them when a node fills up. One eviction aborted a deploy mid-hydration.
What
Users set
environmentd.priorityClassNameandclusterd.priorityClassNameto apply priority classes to those pods. Both values default to unset, which renders no argument and changes no pod spec. The clusterd argument requires environmentd v26.40 or newer. This matches howclusterd.affinityandclusterd.tolerationsare already gated. Older environmentd versions exit on unrecognized arguments. We exclude balancerd and console because they hold no state and run multiple replicas.Before you set it
system-cluster-criticalis 2000000000, so these pods remain vulnerable to critical preemptors.preemptionPolicy: Neverto prevent new pods from evicting serving pods during the rollout.Testing
Helm unit tests use
notMatchRegexfor negative assertions. ThenotContainsfunction compares whole list elements, so it never fails against a flag rendered as--flag=value. The pre-existingschedulerNameassertion has the same defect and is fixed here. An mzcompose test asserts the resolved integer priority to prove Kubernetes admitted the class.Open: I omitted the Chart.yaml version bump because a script manages it and PR #38406 omitted it, but which rule is live?