fix(craft): drop the prepuller PriorityClass (#13562)

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

backend/tests/external_dependency_unit/craft_helm/test_sandbox_image_prepuller.py

@@ -20,10 +20,10 @@ the original latency. The only signal is provisioning being slow, which is what
 the feature was meant to fix. All four are fixed at chart-render time, so a
 render test catches them before deploy.
 
-Rendering needs the ``helm`` binary and the chart's subchart tarballs, which are
-gitignored, so these skip unless you have run ``helm dependency build`` on the
-chart. No CI lane currently supplies both, so treat them as a local guard for
-now: run them when you touch the sandbox chart templates.
+Lives beside ``test_pod_spec.py`` in the ``craft_helm`` shard, the lane that
+provides ``helm`` and runs ``helm dependency build`` (see
+``pr-external-dependency-unit-tests.yml``). Skips locally when those are
+unavailable; in CI those conditions fail instead of masking the suite.
 
 Only a missing-dependency error is treated as "can't run here". Any other helm
 failure is the chart being broken and fails the test — a render test that
@@ -32,17 +32,18 @@ reports a broken chart as a skip is worse than no test.
 
 from __future__ import annotations
 
+import os
 import re
 import shutil
 import subprocess
-from pathlib import Path
 from typing import NoReturn
 
 import pytest
 import yaml
 
-# backend/tests/unit/onyx/server/features/build/sandbox/ -> repo root
-_REPO_ROOT = Path(__file__).resolve().parents[8]
+from tests.common.paths import find_ancestor_containing
+
+_REPO_ROOT = find_ancestor_containing("deployment/helm/charts/onyx")
 _CHART_DIR = _REPO_ROOT / "deployment" / "helm" / "charts" / "onyx"
 _PREPULLER_TEMPLATE = "templates/sandbox-image-prepuller.yaml"
 _PODTEMPLATE_TEMPLATE = "templates/sandbox-podtemplate.yaml"
@@ -58,11 +59,19 @@ _MISSING_DEPS_MARKERS = (
 )
 
 
+def _skip_or_fail(reason: str) -> NoReturn:
+    """Skip locally, but fail in CI — a shard that renders nothing must not
+    report green."""
+    if os.environ.get("CI"):
+        pytest.fail(reason)
+    pytest.skip(reason)
+
+
 def _handle_render_failure(stderr: str) -> NoReturn:
     """A missing-deps error means we can't run here; anything else is a real
     chart defect and must not be laundered into a skip."""
     if any(marker in stderr for marker in _MISSING_DEPS_MARKERS):
-        pytest.skip(f"chart dependencies not built: {stderr.strip()}")
+        _skip_or_fail(f"chart dependencies not built: {stderr.strip()}")
     pytest.fail(f"helm template failed: {stderr.strip()}")
 
 
@@ -71,7 +80,7 @@ def _run_helm(
 ) -> subprocess.CompletedProcess[str]:
     helm = shutil.which("helm")
     if helm is None:
-        pytest.skip("helm binary not available")
+        _skip_or_fail("helm binary not available")
     cmd = [
         helm,
         "template",
@@ -115,7 +124,7 @@ def _renders_nothing(extra_args: list[str]) -> bool:
 
 
 def _prepuller(extra_args: list[str] | None = None) -> dict:
-    """The DaemonSet. The template also emits a PriorityClass, so select by kind
+    """The DaemonSet. The template also emits a NetworkPolicy, so select by kind
     rather than assuming a single document."""
     docs = yaml.safe_load_all(_render(_PREPULLER_TEMPLATE, extra_args))
     return next(d for d in docs if d and d["kind"] == "DaemonSet")
@@ -205,61 +214,66 @@ def test_prepuller_requests_ephemeral_storage() -> None:
     assert resources["requests"] == resources["limits"]
 
 
-def test_prepuller_yields_to_pending_sandbox_pods() -> None:
-    """A do-nothing pod must never be the reason a sandbox fails to schedule and
-    triggers a scale-up. Negative priority lets a pending sandbox preempt it;
-    preemptionPolicy Never means the prepuller evicts nothing itself."""
-    docs = list(yaml.safe_load_all(_render(_PREPULLER_TEMPLATE)))
-    pc = next(d for d in docs if d and d["kind"] == "PriorityClass")
-    ds = next(d for d in docs if d and d["kind"] == "DaemonSet")
-
-    # Strictly below -10, cluster-autoscaler's default expendable-pods cutoff:
-    # at exactly -10 the prepuller is non-expendable and a pending one can be
-    # read as demand for a new node.
-    assert pc["value"] < -10
-    assert pc["globalDefault"] is False
-    assert pc["preemptionPolicy"] == "Never"
-    assert ds["spec"]["template"]["spec"]["priorityClassName"] == pc["metadata"]["name"]
-
-
-def _rendered_kinds(extra_args: list[str]) -> list[str]:
+def _rendered_kinds(extra_args: list[str] | None = None) -> list[str]:
     docs = yaml.safe_load_all(_render(_PREPULLER_TEMPLATE, extra_args))
     return [d["kind"] for d in docs if d]
 
 
-def test_existing_priority_class_skips_creation() -> None:
-    """An operator supplying their own class must not also get ours."""
+# Objects with no `metadata.namespace`. A cluster-scoped object from this
+# template is what broke the deploy described in
+# test_prepuller_ships_no_cluster_scoped_objects.
+_CLUSTER_SCOPED_KINDS = {
+    "PriorityClass",
+    "ClusterRole",
+    "ClusterRoleBinding",
+    "StorageClass",
+    "Namespace",
+    "CustomResourceDefinition",
+    "PersistentVolume",
+}
+
+
+def test_prepuller_ships_no_cluster_scoped_objects() -> None:
+    """Regression for a broken `helm upgrade` (#13490).
+
+    The prepuller originally created its own PriorityClass at value -10. A
+    follow-up changed the default to -11 while keeping the object's name, and
+    every existing install failed to upgrade:
+
+        Error: UPGRADE FAILED: cannot patch
+        "onyx-onyx-sandbox-image-prepuller" with kind PriorityClass: ...
+        value: Forbidden: may not be changed in an update.
+
+    `PriorityClass.value` is immutable and `helm upgrade` patches, so that field
+    could never be changed in place — and because Helm aborts the whole release
+    on one rejected patch, a latency optimisation took down the nightly deploy
+    of everything else.
+
+    The class was removed rather than worked around: it bought very little (see
+    docs/craft/sandbox/image-and-spinup.md), and an operator who wants one
+    points `priorityClassName` at their own. Keep this template namespaced —
+    cluster-scoped objects also collide between releases sharing a name in
+    different namespaces, which is why the removed class needed a
+    namespace-hashed name in the first place.
+    """
+    assert not _CLUSTER_SCOPED_KINDS.intersection(_rendered_kinds())
+
+
+def test_prepuller_has_no_priority_class_name_by_default() -> None:
+    """The chart creates no class, so it must not name one either — a
+    `priorityClassName` pointing at a nonexistent class fails pod admission."""
+    assert "priorityClassName" not in _prepuller_pod_spec()
+
+
+def test_prepuller_can_run_under_an_operator_supplied_priority_class() -> None:
+    """The opt-in for anyone who does want the prepuller preemptible: name an
+    existing class, and it is used verbatim without the chart creating one."""
     args = ["--set", "sandboxImagePrepull.priorityClassName=system-node-critical"]
 
     assert "PriorityClass" not in _rendered_kinds(args)
     assert _prepuller_pod_spec(args)["priorityClassName"] == "system-node-critical"
 
 
-def test_priority_class_creation_can_be_declined() -> None:
-    """Regression: sprig's ``merge`` treats ``false`` as empty and overwrites it
-    with the default, so a merge-based defaults block silently ignored
-    ``create: false``. Falsy values must survive."""
-    args = ["--set", "sandboxImagePrepull.priorityClass.create=false"]
-
-    assert "PriorityClass" not in _rendered_kinds(args)
-    assert "priorityClassName" not in _prepuller_pod_spec(args)
-
-
-def test_priority_class_value_zero_survives() -> None:
-    """Same trap for ints: ``value: 0`` must not silently become -10."""
-    pc = next(
-        d
-        for d in yaml.safe_load_all(
-            _render(
-                _PREPULLER_TEMPLATE,
-                ["--set", "sandboxImagePrepull.priorityClass.value=0"],
-            )
-        )
-        if d and d["kind"] == "PriorityClass"
-    )
-    assert pc["value"] == 0
-
-
 def test_repull_fanout_is_tunable_for_large_pools() -> None:
     """N nodes pulling ~1GB at once is a thundering herd against the registry."""
     ds = next(
@@ -309,32 +323,22 @@ def test_prepuller_service_account_follows_the_configured_name() -> None:
     assert spec["serviceAccountName"] == "custom-sandbox-sa"
 
 
-def test_priority_class_name_is_namespace_scoped() -> None:
-    """PriorityClass is cluster-scoped. Two same-named releases in different
-    namespaces must not render the same object — the second install would fail
-    Helm's ownership check, and either release's upgrade could disrupt the
-    other."""
-
-    def pc_name(namespace: str) -> str:
-        docs = yaml.safe_load_all(
-            _render(_PREPULLER_TEMPLATE, ["--namespace", namespace])
+def test_prepuller_object_names_stay_within_k8s_limits() -> None:
+    """A long release/namespace pair must still emit valid names. Namespaced
+    objects don't need the namespace hashed into them the way the removed
+    PriorityClass did, but they do still have to be DNS-1123."""
+    docs = [
+        d
+        for d in yaml.safe_load_all(
+            _render(_PREPULLER_TEMPLATE, ["--namespace", "a" * 60])
         )
-        pc = next(d for d in docs if d and d["kind"] == "PriorityClass")
-        return pc["metadata"]["name"]
-
-    assert pc_name("onyx") != pc_name("onyx-staging")
-
-
-def test_priority_class_name_stays_within_k8s_limits() -> None:
-    """Long release/namespace pairs must hash down rather than emit an invalid
-    name, matching the trunc+sha shape used for the RBAC names."""
-    long_ns = "a" * 60
-    docs = yaml.safe_load_all(_render(_PREPULLER_TEMPLATE, ["--namespace", long_ns]))
-    name = next(d for d in docs if d and d["kind"] == "PriorityClass")["metadata"][
-        "name"
+        if d
     ]
-    assert len(name) <= 253
-    assert re.fullmatch(r"[a-z0-9]([-a-z0-9]*[a-z0-9])?", name), name
+    assert docs
+    for doc in docs:
+        name = doc["metadata"]["name"]
+        assert len(name) <= 253, name
+        assert re.fullmatch(r"[a-z0-9]([-a-z0-9]*[a-z0-9])?", name), name
 
 
 def test_prepuller_is_excluded_from_sandbox_network_policies() -> None:

deployment/helm/charts/onyx/Chart.yaml

@@ -5,7 +5,7 @@ home: https://www.onyx.app/
 sources:
   - "https://github.com/onyx-dot-app/onyx"
 type: application
-version: 0.8.6
+version: 0.8.7
 appVersion: latest
 annotations:
   category: Productivity
@@ -16,12 +16,15 @@ annotations:
     - name: background
       image: docker.io/onyxdotapp/onyx-backend:latest
   artifacthub.io/changes: |
-    - kind: added
-      description: Pre-pull the Craft sandbox image onto the sandbox node pool with a
-        DaemonSet, so a user's first sandbox on a node doesn't pay the ~31s image pull.
-        Rendered only when configMap.ENABLE_CRAFT is true; disable with
-        sandboxImagePrepull.enabled=false. Costs ~3.3 GB of unreclaimable disk per
-        sandbox node.
+    - kind: fixed
+      description: Fixed "helm upgrade" failing with 'PriorityClass ... value: Forbidden;
+        may not be changed in an update' after the sandbox image prepuller landed in
+        0.8.6. PriorityClass.value is immutable and helm upgrade patches, so the
+        prepuller no longer ships a PriorityClass at all; it was buying very little.
+        Upgrading from 0.8.6 deletes it, and the prepuller runs at the cluster default
+        priority — set sandboxImagePrepull.priorityClassName to an existing class to
+        keep it preemptible. Running pods are unaffected, because pod priority is
+        resolved at admission. The chart now renders no cluster-scoped objects.
 dependencies:
   # Helm uses the first condition path that exists: `postgresqlOperator.enabled`
   # is unset by default, so this falls through to `postgresql.enabled`. Set it

deployment/helm/charts/onyx/templates/sandbox-image-prepuller.yaml

@@ -2,8 +2,8 @@
 Pre-pulls the sandbox image onto every node in the sandbox pool so a user's
 first sandbox there doesn't pay the pull. See docs/craft/sandbox/image-and-spinup.md.
 
-Defaults are resolved field by field, NOT with `merge`: sprig's merge treats
-`false` and `0` as empty, so `create: false` and `value: 0` would be overwritten.
+This template renders NAMESPACED objects only. It deliberately no longer ships a
+PriorityClass; see the docs above before adding one back.
 */}}
 {{- $prepull := .Values.sandboxImagePrepull | default dict }}
 {{- $res := $prepull.resources | default dict }}
@@ -11,11 +11,6 @@ Defaults are resolved field by field, NOT with `merge`: sprig's merge treats
 {{- $mem := $res.memory | default "32Mi" }}
 {{- $eph := $res.ephemeralStorage | default "64Mi" }}
 {{- $maxUnavailable := $prepull.updateMaxUnavailable | default "25%" }}
-{{- $pc := $prepull.priorityClass | default dict }}
-{{- $pcCreate := true }}
-{{- if hasKey $pc "create" }}{{- $pcCreate = $pc.create }}{{- end }}
-{{- $pcValue := -11 }}
-{{- if hasKey $pc "value" }}{{- $pcValue = $pc.value }}{{- end }}
 {{- if and (eq (include "onyx.craftEnabled" .) "true") $prepull.enabled }}
 {{- $cm := .Values.configMap }}
 {{- $sandboxNs := $cm.SANDBOX_NAMESPACE | default "onyx-sandboxes" }}
@@ -23,29 +18,6 @@ Defaults are resolved field by field, NOT with `merge`: sprig's merge treats
 {{- $image := include "onyx.sandboxImage" . }}
 {{- $pullPolicy := include "onyx.sandboxImagePullPolicy" . }}
 {{- $sandboxPod := .Values.sandboxPod | default dict }}
-{{- /* PriorityClass is cluster-scoped, so the name includes the release
-namespace. Same trunc+hash shape as the RBAC names in sandbox-rbac.yaml. */}}
-{{- $pcBase := printf "%s-%s" (include "onyx.fullname" .) .Release.Namespace }}
-{{- if gt (len $pcBase) 47 }}
-{{- $pcBase = printf "%s-%s" ($pcBase | trunc 38 | trimSuffix "-") (sha256sum $pcBase | trunc 8) }}
-{{- end }}
-{{- $pcName := $prepull.priorityClassName | default (ternary (printf "%s-sandbox-image-prepuller" $pcBase) "" $pcCreate) }}
-{{- if and $pcCreate (not $prepull.priorityClassName) }}
-apiVersion: scheduling.k8s.io/v1
-kind: PriorityClass
-metadata:
-  name: {{ $pcName }}
-  labels:
-    {{- include "onyx.labels" . | nindent 4 }}
-    app.kubernetes.io/component: sandbox-image-prepuller
-value: {{ $pcValue }}
-globalDefault: false
-description: >-
-  Sandbox image prepuller. Below cluster-autoscaler's expendable cutoff (-10) so
-  it never justifies a scale-up, and below a sandbox pod so one can preempt it.
-preemptionPolicy: Never
----
-{{- end }}
 apiVersion: apps/v1
 kind: DaemonSet
 metadata:
@@ -77,7 +49,9 @@ spec:
         {{- (index $sandboxPod "nodeSelector" | default (dict "onyx.app/workload" "sandbox")) | toYaml | nindent 8 }}
       tolerations:
         {{- (index $sandboxPod "tolerations" | default (list (dict "key" "workload" "operator" "Equal" "value" "sandbox" "effect" "NoSchedule"))) | toYaml | nindent 8 }}
-      {{- with $pcName }}
+      {{- /* Opt-in only, and it must be a class that already exists: the chart
+      creates none. Unset means the cluster default priority. */}}
+      {{- with $prepull.priorityClassName }}
       priorityClassName: {{ . }}
       {{- end }}
       {{- /* Pull credentials come from the sandbox SA only, same as the

deployment/helm/charts/onyx/values.yaml

@@ -1669,14 +1669,10 @@ sandboxImagePrepull:
     cpu: 10m
     memory: 32Mi
     ephemeralStorage: 64Mi
-  # Below cluster-autoscaler's expendable-pods cutoff (-10), so a pending
-  # prepuller is never itself a reason to scale up.
-  priorityClass:
-    create: true
-    value: -11
-  # Set to use an existing PriorityClass instead; skips creating the one above.
-  # create: false with this left empty is a valid opt-out, but drops the pod to
-  # the cluster default priority, where it competes with sandbox pods.
+  # Name of an EXISTING PriorityClass to run the prepuller under. The chart
+  # creates none, so an unset value means the cluster default priority. Point
+  # this at a low/negative class if you want a pending sandbox pod to be able to
+  # preempt the prepuller.
   priorityClassName: ""
   # Nodes that repull at once on a tag bump. Raise to warm a large pool faster,
   # at the cost of a thundering herd against the registry.

docs/craft/sandbox/image-and-spinup.md

@@ -156,13 +156,15 @@ Each of these fails *silently* if changed — the DaemonSet reports Ready
 while sandboxes go on cold-pulling, and the only symptom is the slow
 provisioning the feature was meant to remove. There are render-time
 tests for all of them in
-`backend/tests/unit/onyx/server/features/build/sandbox/test_sandbox_image_prepuller.py`.
+`backend/tests/external_dependency_unit/craft_helm/test_sandbox_image_prepuller.py`.
 
-Those tests need `helm` and the chart's subchart tarballs, which are
-gitignored, so they skip in the `backend/tests/unit` lane and no CI lane
-currently supplies both. Until one does they are a local guard — run
-them (after `helm dependency build deployment/helm/charts/onyx`) when
-you touch these templates, and don't assume CI caught a drift for you.
+They live in the `craft_helm` shard, next to `test_pod_spec.py`, because
+that is the lane which installs `helm` and runs `helm dependency build`
+(see `pr-external-dependency-unit-tests.yml`, triggered on
+`deployment/helm/**`). Chart-render tests belong there and nowhere else:
+under `backend/tests/unit` they have no helm, and a directory named
+`build` is additionally skipped by pytest's default `norecursedirs`, so
+they would never even be collected.
 
 - **A DaemonSet, not a one-shot pre-pull Job.** The kubelet only exempts
   images referenced by a *running* pod from image GC. A Job pulls and
@@ -184,23 +186,10 @@ you touch these templates, and don't assume CI caught a drift for you.
   same single route the sandbox PodTemplate uses. See "Private
   registries" below for why the chart-wide `.Values.imagePullSecrets`
   is not an option here.
-- **Priority `-11`, `preemptionPolicy: Never`.** Strictly below
-  cluster-autoscaler's default `--expendable-pods-priority-cutoff` of
-  -10, so a pending prepuller is never itself read as demand for a new
-  node — at exactly -10 it is *not* expendable. It also sits below a
-  sandbox pod, so one can preempt it; a sandbox running on the node pins
-  the image itself, making the prepuller redundant exactly when it would
-  compete. Don't over-read the preemption half: the prepuller holds 10m
-  CPU / 32Mi, so evicting it will not unblock a sandbox that wants 1000m
-  / 2Gi. It mainly guarantees the prepuller is never the *reason* for a
-  scale-up. This does *not* reorder disk-pressure eviction — kubelet
-  ranks "exceeds ephemeral-storage request" ahead of priority, and
-  evicting the prepuller frees ~0 bytes anyway. Setting
-  `priorityClass.create: false` without also setting `priorityClassName`
-  is a supported opt-out (for clusters where PriorityClasses are managed
-  centrally) but forfeits all of the above: the pod falls to the cluster
-  default priority and competes with sandbox pods. Point
-  `priorityClassName` at an existing low-priority class instead.
+- **No PriorityClass, and no cluster-scoped objects at all.** See
+  "Why there is no PriorityClass" below before adding one back.
+  `sandboxImagePrepull.priorityClassName` names an *existing* class for
+  operators who want the prepuller preemptible; the chart creates none.
 - **A portable idle loop**, not `sleep infinity` — a GNU coreutils
   extension that dies on busybox/alpine. A CrashLooping prepuller unpins
   the image.
@@ -221,6 +210,59 @@ used to let the node drift back to a fresh `latest` no longer runs on
 it. Under `pullPolicy: Always` the restart does re-resolve, which is why
 the policy is shared rather than hardcoded.
 
+### Why there is no PriorityClass
+
+The prepuller originally created one, at value -10, so a pending sandbox
+pod could preempt it rather than fail to schedule behind a do-nothing
+pod. **It broke deploys and has been removed. Don't add it back without
+reading this.**
+
+A follow-up changed the default to -11 — on the argument that -10 is
+exactly cluster-autoscaler's default `--expendable-pods-priority-cutoff`
+and the cutoff is exclusive — while keeping the object's name. Every
+cluster already holding the class then failed to upgrade:
+
+```
+Error: UPGRADE FAILED: cannot patch "onyx-onyx-sandbox-image-prepuller"
+with kind PriorityClass: ... value: Forbidden: may not be changed in an
+update.
+```
+
+Two Kubernetes facts collide there. `PriorityClass.value` is immutable —
+priority is resolved once at pod admission and copied into
+`pod.spec.priority`, so the class's value must not drift afterwards. And
+`helm upgrade` *patches* existing objects rather than replacing them. So
+that field could never be changed in place, by anyone, ever. Worse, Helm
+aborts the entire release on one rejected patch, so a latency
+optimisation took down the nightly deploy of the whole application.
+
+It was removed rather than worked around, because it was not earning its
+keep:
+
+- **Preemption freed nothing useful.** The prepuller requests 10m CPU /
+  32Mi. Evicting it cannot unblock a sandbox pod that wants 1000m / 2Gi.
+- **The autoscaler argument barely applies to a DaemonSet.**
+  cluster-autoscaler does not scale up *because* a DaemonSet pod is
+  pending; DS pods only factor into simulating a new node's capacity.
+- **It did not affect disk-pressure eviction**, the one form of eviction
+  the prepuller actually invites: kubelet ranks "exceeds
+  ephemeral-storage request" ahead of priority.
+- **Cluster-scoped objects are expensive in a Helm chart.** Two releases
+  of the same name in different namespaces render the same object, so the
+  name needed the namespace hashed into it, which is where the
+  trunc+sha32 logic and several tests came from.
+
+If you want the prepuller preemptible, point
+`sandboxImagePrepull.priorityClassName` at a low-priority class you
+manage yourself. A render test
+(`test_prepuller_ships_no_cluster_scoped_objects`) fails if this template
+starts emitting cluster-scoped objects again.
+
+Note also that **no CI lane would have caught this**: `ct install` only
+does a fresh install into an empty kind cluster, never an upgrade from
+the previously released chart, so the patch path where immutable-field
+violations live is untested. `ct install --upgrade` would cover it.
+
 ### Private registries
 
 Relevant only if you mirror the sandbox image into your own registry;