diff --git a/backend/tests/unit/onyx/server/features/build/sandbox/test_sandbox_image_prepuller.py b/backend/tests/external_dependency_unit/craft_helm/test_sandbox_image_prepuller.py similarity index 76% rename from backend/tests/unit/onyx/server/features/build/sandbox/test_sandbox_image_prepuller.py rename to backend/tests/external_dependency_unit/craft_helm/test_sandbox_image_prepuller.py index ba17274530..707cb2049e 100644 --- a/backend/tests/unit/onyx/server/features/build/sandbox/test_sandbox_image_prepuller.py +++ b/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: diff --git a/deployment/helm/charts/onyx/Chart.yaml b/deployment/helm/charts/onyx/Chart.yaml index 6a98584e23..1b684a1990 100644 --- a/deployment/helm/charts/onyx/Chart.yaml +++ b/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 diff --git a/deployment/helm/charts/onyx/templates/sandbox-image-prepuller.yaml b/deployment/helm/charts/onyx/templates/sandbox-image-prepuller.yaml index f0fb8d713c..a2c149e08f 100644 --- a/deployment/helm/charts/onyx/templates/sandbox-image-prepuller.yaml +++ b/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 diff --git a/deployment/helm/charts/onyx/values.yaml b/deployment/helm/charts/onyx/values.yaml index 770796458c..98c0412fad 100644 --- a/deployment/helm/charts/onyx/values.yaml +++ b/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. diff --git a/docs/craft/sandbox/image-and-spinup.md b/docs/craft/sandbox/image-and-spinup.md index c00fdd021e..1172cc9df8 100644 --- a/docs/craft/sandbox/image-and-spinup.md +++ b/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;