orchestratord: allow setting a PriorityClass on environmentd and clusterd pods

[jazz-mo]

Problem

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. We
lost a clusterd replica mid-hydration to a cluster-critical DaemonSet rollout,
which aborted the deploy. The chart offers no way to raise these pods.

Solution

Two new chart values, environmentd.priorityClassName and
clusterd.priorityClassName. They sit next to the per-component nodeSelector,
affinity and tolerations that both components already have. Each becomes an
orchestratord argument. environmentd.priorityClassName sets
priorityClassName on the environmentd StatefulSet's pod template.
clusterd.priorityClassName reaches clusterd through environmentd, forwarded as
--orchestrator-kubernetes-priority-class-name.

Both default to unset, which renders no argument and leaves every pod spec
byte-identical.

balancerd and console are left out on purpose. Both default to two replicas and
neither holds state, so an eviction costs a connection rather than a
rehydration. The operator's own pod is out for the same reason. It runs two
replicas behind leader election and a PodDisruptionBudget.

The forwarded clusterd argument is gated on
meets_minimum_version(&V26_40_0), so clusterd.priorityClassName needs an
environmentd at v26.40 or newer. clusterd.affinity and clusterd.tolerations
are gated on V144 three lines above it. environmentd exits on an unknown
argument. Without the gate, an instance pinned to an older
environmentdImageRef would fail to start once the value was set.

Testing

helm unittest misc/helm-charts/operator covers both values, absent when unset
and present when set. The negative cases use notMatchRegex over the argument
list. notContains cannot fail here. It compares whole elements, and the
rendered element always carries =<value>.

test/orchestratord/mzcompose.py gains a PriorityClassName modification. It
installs the chart against a real PriorityClass, requests a rollout, and waits
for it to complete. It then asserts that environmentd and clusterd pods carry
both spec.priorityClassName and the resolved spec.priority. The resolved
integer is the assertion that matters. It proves the API server admitted the
class. The string alone only proves the operator copied it. On an older
environmentdImageRef it asserts the split instead: environmentd carries the
class, clusterd does not, and environmentd is running.

For the reviewer

• Neither value reaches a running pod until the next rollout. Setting either
  one changes the environmentd StatefulSet spec. Every instance then flips to
  UpToDate=False with reason WaitingForApproval, and waits for
  spec.requestRollout to change. That is the operator's standing behaviour for
  any generation-affecting change. Worth knowing before reaching for this value
  during an incident. Nothing moves until you roll.
• A user-defined PriorityClass cannot outrank a cluster-critical one.
  Kubernetes caps user-created classes at 1000000000.
  system-cluster-critical sits at 2000000000. Moving these pods off 0 takes
  them out of the victim queue ahead of ordinary workloads, which covers most of
  a cluster. It does not make them immune to the DaemonSet in the Problem
  section when they are the only pods on the node worth evicting. Surviving that
  needs capacity the DaemonSet does not have to take, so a dedicated node pool,
  not a priority class.
• Prefer a class with preemptionPolicy: Never. During the rollout that
  applies the value, the new generation's pods carry the class. The generation
  still serving traffic does not. On a full node the new pods can evict the old
  ones. Never stops that. The cost is that the new pods wait for capacity
  instead of taking it. A rollout on a full cluster then stays Pending until a
  node appears. preemptionPolicy has to live on the PriorityClass. The API
  server rejects it in a pod spec when the pod names a class.
• Create the class before setting the value. Kubernetes refuses a pod that
  names a class it cannot resolve. A StatefulSet surfaces that only as a
  FailedCreate event.
• The chart's default operator.image.tag trails the chart version between
  releases. Install from a git checkout with either value set, and the argument
  goes to an orchestratord that does not accept it. Released charts pin a
  matching image.
• No Rust test. generation.rs has no test module, so the orchestratord suite is
  the only automated cover for the gate.
• I also fixed the schedulerName negative chart test, which carries the same
  notContains defect. This change edits the block directly beneath it.
• V154_DEV0 is built with an empty prerelease under an
  .expect("dev.0 is valid prerelease") message, so the pod-annotations gate
  wants a final 0.154.0. Left alone, since changing it moves a gate this change
  does not touch.
• Three things for you to decide: whether the schedulerName fix should go
  separately, whether Chart.yaml needs the version bump its CONTRIBUTING asks
  for (the version looks release-managed by bin/helm-chart-version-bump, and
  CNS-136: expose statement logging sample rate in the operator chart #38406 did not bump it), and whether to open an A-docs issue for the
  scheduling guidance in the bullets above, since no self-managed page covers it
  today.

[github-actions]

Thank you for your submission! We really appreciate it. Like many source-available projects, we require that you sign our Contributor License Agreement (CLA) before we can accept your contribution.

You can sign the CLA by posting a comment with the message below.

---

I have read the Contributor License Agreement (CLA) and I hereby sign the CLA.

---

_{You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.}

[antiguru]

Review of the change. One blocking item, two suggestions, two nits.

Blocking

The new environmentd argument is not version-gated

src/orchestratord/src/controller/materialize/generation.rs, in create_environmentd_statefulset_object.

The function's own header comment states the rule:

IMPORTANT: Only pass secrets via environment variables. All other parameters should be passed as command line arguments, possibly gated with a meets_minimum_version call. This ensures typos cause environmentd to fail to start, rather than silently ignoring the configuration.

--orchestrator-kubernetes-priority-class-name is pushed unconditionally. The operator manages environmentd images older than itself: mz.spec.environmentdImageRef is user-controlled, meets_minimum_version reads that ref, and there is no minimum-supported-version floor anywhere in orchestratord or crd/materialize.rs. The existing gates document the supported range directly, for example the V26_1_0 check carries the comment "0.165 should work with anything greater than 0.164 including v26 and v25", and there is a bounded V143/V153 window further down.

environmentd's Args derives Parser with no ignore_errors or allow_external_subcommands, so clap 4 treats an unrecognized argument as a hard error. The failure mode is therefore: a user upgrades the operator, sets priorityClassName, has not yet upgraded environmentdImageRef, and the next rollout puts environmentd into a crash loop with an unknown-argument error. That is the fail-fast behavior the comment intends for typos, triggered here by a valid configuration.

The neighbouring ungated scheduler_name block is not precedent for skipping the gate. git log -L shows it landed in 088a115bd0 (2024-10-10), the initial orchestratord commit, so it predates every environmentd version the operator can manage.

Suggested fix: add a version constant for the release this lands in (main is currently 26.40.0-dev.0) alongside the existing LazyLock<Version> constants, and wrap the args.push in a meets_minimum_version check, matching the V154_DEV0 block a few lines below.

Suggestions

The negative Helm test cannot fail

misc/helm-charts/operator/tests/deployment_test.yaml:

- it: should not pass the priority class when not configured
  asserts:
  - notContains:
      path: spec.template.spec.containers[0].args
      content: "--priority-class-name"

helm-unittest's contains/notContains compares list elements for equality, not substrings. In v1.0.2 (the version pinned in ci/builder/Dockerfile) the match is reflect.DeepEqual(ele, v.Content) in pkg/unittest/validators/contains_validator.go. The template renders --priority-class-name=<value>, which is never equal to the bare --priority-class-name, so the assertion holds regardless of what the template does.

Verified empirically with helm 4.2.3 and helm-unittest 1.0.2 against a scratch copy of the chart carrying this change. Rewriting the template to emit the flag unconditionally, ignoring the value:

- "--priority-class-name={{ .Values.priorityClassName | default "my-priority-class" }}"

leaves the suite fully green at 35/35, should not pass the priority class when not configured included.

This assertion form works, failing on the broken template and passing on the one in this PR:

  - notMatchRegex:
      path: spec.template.spec.containers[0].args[*]
      pattern: "--priority-class-name"

The existing --scheduler-name negative test on line 277 has the same defect, so this is not a regression introduced here. Worth fixing in this change, and the scheduler one separately.

Nothing tests the pod specs or the argument forwarding

Every assertion added is on the operator's own Deployment arguments. None of the four places where the value takes effect is covered: the environmentd StatefulSet pod spec and its forwarded argument, balancerd, console, and clusterd. src/orchestratord has no test module for generation.rs, so there is no existing harness to extend, and the same gap already applies to scheduler_name.

Raising it because the version gate above is exactly what such a test would pin. An assertion that the argument is present for a recent image ref and absent for an older one would be cheap once the gate exists.

Nits

Regenerate the user-facing chart parameter table

doc/user/data/self_managed/materialize_operator_chart_parameter.yml lists schedulerName but not priorityClassName. Per bin/bump-version this file is regenerated from value_descriptions.yml.gotmpl at release time, so it self-heals, and ci/test/lint-main/checks/check-helm-docs.sh only diffs what helm-docs regenerates, which is README.md. Regenerating now just means the published table is correct before the next version bump. The README.md row itself is in the slot helm-docs would produce.

Consider documenting the blast radius of a bad value

A typo in schedulerName leaves pods Pending. A typo here is harsher: the Priority admission controller rejects the pod outright, since the Kubernetes documentation states "If the priority class is not found, the Pod is rejected." Because this is a single global value applied to every pod, one bad name blocks creation of all environmentd, clusterd, balancerd, and console pods. One clause in the values.yaml comment noting that the PriorityClass must already exist would earn its keep.

Looks correct

• The plumbing is complete. KubernetesOrchestratorConfig and each controller Config have exactly one construction site each, and all are updated.
• PodSpec.priority_class_name exists in the pinned k8s-openapi 0.27.0, confirmed against the vendored source.
• The README.md entry sits where helm-docs would generate it, so the docs lint check should pass.
• One semantic change, one CODEOWNERS area, 45 lines added. Comfortably within the size guidance in doc/developer/guide-changes.md, and the description states the user-visible effect in user-facing terms, so release note generation will pick it up.

Net: this improves codebase health and follows the scheduler_name pattern faithfully. The version gate should land before merge. Everything else is non-blocking.

---

Posted by Claude Code on behalf of @antiguru.

[jazz-mo]

Thanks, this was a good review. All five items are addressed, and the scope changed on the back of the second and third.

Blocking: version gate

Added. V26_40_0 sits with the other LazyLock<Version> constants and carries a comment saying what it gates and why. The args.push is wrapped in meets_minimum_version(&V26_40_0).

Your reading of the precedent matches what I found: git blame puts the ungated scheduler_name push in the initial orchestratord commit, before the gating mechanism existed. The closer precedent is three lines above it, where clusterd.affinity and clusterd.tolerations are gated on V144 and forwarded to environmentd exactly the way this flag is.

Negative Helm test

Fixed, and thank you for naming the pinned version. I had validated notMatchRegex against helmunittest/helm-unittest:latest, which is 1.1.2. CI pins 1.0.2 at ci/builder/Dockerfile:358, so I re-ran on helmunittest/helm-unittest:3.19.0-1.0.2:

• the chart as it now stands: 47/47 pass
• three separate mutants, each removing one {{- if }} guard so the flag always renders: each fails exactly one test, with asserts[0] notMatchRegex fail

The third mutant is the schedulerName one, which passed silently before. I fixed that assertion in this change, since this PR edits the block directly beneath it. You asked for it separately, so say the word and I will split it out.

I used ^--priority-class-name rather than your unanchored form, so the pattern cannot match a longer flag that merely contains the string.

Nothing tests the pod specs or the forwarding

Added a PriorityClassName modification to test/orchestratord/mzcompose.py, plus a test/orchestratord/priorityclass.yaml fixture applied beside storageclass.yaml.

It asserts spec.priority, not only spec.priorityClassName. The integer is written by the API server from the named class, so it separates a class Kubernetes actually resolved from a string the operator copied into the spec. It also asserts the pods exist, because an unresolvable class produces no pod at all and an assertion over an empty list would otherwise pass.

Two things that make it work, which were not obvious:

• it has to request a rollout. environmentd and clusterd take generation-affecting changes only on spec.requestRollout, so without that the pods keep the spec they were created with and the assertions check the previous generation.
• it must not skip on older image refs, which is the case the gate exists for. On an older EnvironmentdImageRef it asserts the split instead: environmentd carries the class, clusterd does not, and environmentd is running.

Nit: chart parameter table

Regenerated, via the two helm-docs invocations bin/bump-version runs. check-helm-docs.sh is clean.

Nit: blast radius of a bad value

Covered in the description. Its scope shrank, because of the next section: a bad name now blocks environmentd and clusterd, not balancerd and console.

Scope change

The feature now covers environmentd and clusterd only, as two per-component values next to the existing per-component scheduling knobs, rather than one global value over all four pod types.

balancerd and console default to two replicas and hold no state, so an eviction there costs a connection rather than a rehydration. environmentd is a single pod and clusterd replicas hold hydrated state. Narrowing it also means reaching for system-cluster-critical no longer puts the web console in the cluster-critical class.

Three things worth knowing that are not in the diff

Measured against a real API server rather than inferred:

• Kubernetes caps a user-defined PriorityClass at 1000000000, and system-cluster-critical is 2000000000. So this raises these pods above ordinary workloads, but does not make them immune to the cluster-critical DaemonSet in the motivation when they are the only pods on the node worth evicting.
• During the rollout that applies the value, the new generation's pods carry the class and the generation still serving traffic does not. On a full node the new pods can evict the old ones. A class with preemptionPolicy: Never prevents that, at the cost of the new pods waiting for capacity instead of taking it. preemptionPolicy cannot be set on the pod: the API server rejects it when the pod names a class.
• Neither value reaches a running pod until a rollout is requested, so instances sit at UpToDate=False / WaitingForApproval until then.

The first two are user-facing and no self-managed page covers pod scheduling today. Happy to open an A-docs issue rather than add a page here.

Still open

Chart.yaml version bump. The chart's CONTRIBUTING asks for one on every change, but the version looks release-managed by bin/helm-chart-version-bump and #38406 did not bump it. Tell me which rule is live and I will follow it.

Not verified locally: cargo clippy. Running it now in bin/ci-builder; I will report the result rather than assume it.

[jazz-mo]

Correction on the last nit. You asked for the prerequisite clause in the values.yaml comment, and I had put it only in the PR description, on the mistaken grounds that these comments are one-liners. They are not: operator.replicas carries a four-line one, and the statement-logging values from #38406 carry longer.

Both comments now read:

  # -- 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:

That flows through helm-docs into the README rows and the published parameter table, both regenerated. check-helm-docs.sh is clean and helm unittest is 47/47 on the pinned 1.0.2.

cargo check -p mz-orchestratord -p mz-orchestrator-kubernetes -p mz-environmentd passes in bin/ci-builder, zero errors and zero warnings. cargo clippy --all-targets -- -D warnings over the same three crates is still running; I will post the result either way.

[jazz-mo]

Local gate results, run in bin/ci-builder so the toolchain matches CI.

gate	result
cargo check -p mz-orchestratord -p mz-orchestrator-kubernetes -p mz-environmentd	pass, 0 errors, 0 warnings
cargo clippy over the same three crates, --all-targets -- -D warnings	pass, 0 errors, 0 warnings
cargo fmt -- --check	pass
helm unittest misc/helm-charts/operator on the pinned 1.0.2	47/47
check-helm-docs.sh equivalent	clean

Clippy ran before the last commit, which touched only values.yaml, README.md and the generated parameter table, so the Rust it checked is what is pushed.

Buildkite still cannot run this: the branch is on a personal fork, and ci/README.md says webhook builds from a fork outside the build-authorized team are refused in the agent env hook. The one red job is the bootstrap pipeline step, exit 1 after a second, before anything compiles. A rebuild from the Buildkite UI would be needed to get real CI on it.

The orchestratord suite that covers the new test is Nightly only, and a fork PR cannot carry the ci-nightly label either, so that test is unrun. It is written to the shape of the existing modifications in that file, but I have not executed it.

[jazz-mo]

Superseded by #38457, which carries this change as a single commit with your review addressed.

What changed since your review, in short:

• The forwarded clusterd argument is now version gated, your blocking item.
• Scope narrowed to environmentd and clusterd. balancerd and console default to two replicas and hold no state, so eviction there costs a connection rather than a rehydration. That also keeps the web console out of any cluster-critical class.
• Two per-component values, environmentd.priorityClassName and clusterd.priorityClassName, instead of one global one.
• Negative chart assertions rewritten with notMatchRegex, including the pre-existing schedulerName one. Verified against the 1.0.2 you pointed me at, not the latest tag I had been using.
• A PriorityClassName case in test/orchestratord/mzcompose.py covering the pod specs and the forwarded argument.
• Chart parameter table regenerated, and the values.yaml comments now carry the prerequisite clause you asked for.

Closing here rather than force pushing again, so the new PR has one clean commit and a description that is not three rounds of edits deep. Sorry for the extra notification.