fix(chart): wait for MySQL before jobs-manager starts (backend#2913) - #942
Conversation
jobs-manager exits on its first failed DB connection, so a MySQL a few seconds behind it crashloops the container. initContainers was gated on hostPath.enabled, so an edge without it had no init containers at all; the gate now covers only init-writable-data. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…at gates them (backend#2913) Two Bugbot findings, both real, and both about position rather than about the change itself. 1. HIGH -- THE REQUIRED HELM SUITE ASSERTED ORDINAL SLOTS. `wait-for-mysql` becoming `initContainers[0]` moved `init-writable-data` to `[1]`, so the #611 and #672 cases were inspecting the WRONG CONTAINER while still passing their own names, and the CSI case asserted `notExists: initContainers` -- true of the chart until this PR, false after it. Measured before fixing: 3 failed / 640 passed. NOT renumbered to `[1]`, because that re-couples the test to ordinal position and is what broke. 23 paths now select by name -- `initContainers[?(@.name=="init-writable-data")]` -- the filter form this repo already uses for `volumes` and `env`. The `[0].name == init-writable-data` equality became `exists:` on the filter, since under a name filter the old assertion was tautological. The CSI case now states the property its TITLE always claimed -- "skips the privileged init", not "has no inits": no `init-writable-data`, nothing running as `runAsUser: 0`, and -- for non-vacuity -- `wait-for-mysql` present, so both refusals are about a populated list rather than an empty one. 2. MEDIUM -- THE NEW GUARD WAS NOT ON THE LIST THE GATE RUNS. `jobs-manager-waits-for-mysql.sh` existed and passed, but `DRIFT_GUARDS` did not name it, so `make drift` and the required Source-of-truth drift job never invoked it: advice, not a gate. Now armed, 38 -> 39 guards. ARMED WHILE GREEN: the guard was run standalone first (rc 0) before being added, rather than landing a red gate. Mutation-proved with the regression the finding names -- folding `wait-for-mysql` back under `hostPath.enabled`, which leaves a VALID chart -- and `make drift` now fails with `FAIL hostPath.enabled=false: first initContainer is 'api', want 'wait-for-mysql'`. My first attempt deleted the container outright, broke the template, and produced a render error instead of the guard's refusal; that proved nothing about the guard and is not what is recorded here. helm unittest 643/643, drift 39/39, check-facts 14/14, shellcheck + bash -n clean. Chart already one patch above develop (1.9.91 vs 1.9.90); manifest unchanged, since no installer script moved. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
bugbot run |
OpenShift restricted assigns a uid from the project range; one pinned container fails SCC admission for the whole pod. runAsNonRoot is inherited from the pod, so nothing is loosened. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
bugbot run |
1 similar comment
|
bugbot run |
|
Your OpenShift analysis on The measurement. Verified on your head, all three preconditions together: So on plain Kubernetes — including the k3d installer path, the primary install — the kubelet should refuse the container with What I could not do: run it. The local k3d cluster is down ( Two remedies, and I now prefer the second:
Not pushing anything: your guard is a design decision and this needs your call, not my override. Happy to implement (2) if you confirm the jobs-manager image runs non-root. For the record, the rest of my pass on this PR did land earlier ( |
…nywhere (backend#2913) Bugbot High, and it independently confirms the plain-Kubernetes regression I raised on this PR last round: dropping the pin fixed OpenShift and broke every other platform. THE MECHANISM. `runAsNonRoot: true` only FORBIDS uid 0 -- it assigns nothing -- so with no `runAsUser` anywhere (which the OpenShift arbitrary-UID path requires) the kubelet reads the effective user from the IMAGE. `library/busybox` declares `Config.User` EMPTY (measured: `[]`), i.e. root, so AKS, EKS, k3d and bare metal all fail the init with "container has runAsNonRoot and image will run as root". THE FIX IS THE IMAGE, NOT A VALUES SEAM. `Dockerfile.jobs_manager:26` declares `USER 1001` -- which is exactly why `api` and `pods-monitor-container` need no pin either. Reusing it means NO uid is pinned anywhere in this pod: OpenShift assigns from the project range, plain Kubernetes gets 1001 from the image, and the arbitrary-UID invariant this branch added holds with no operator action and no new values key. I had a `pinRunAsUser` seam built and this is strictly better -- it needs nothing from the operator and keeps the guard exactly as written. It also removes an image: busybox was a second pull on every edge for a TCP connect the app image can already do. (`library/busybox` is still used by `mysql-deployment.yaml` and by `init-writable-data`, so the values key stays live.) THE PROBE IS PYTHON NOW, because the app image carries no netcat. `socket.create_connection` is the same test with a real per-attempt timeout, which `nc -z` did not have. Every message is byte-identical -- the diagnosis was already right. Driven against a real socket, not just rendered: rc=0 with a listener, rc=1 without, and the deadline is checked AFTER an attempt so a wait starting inside the last two seconds still gets one try. Verified on the render: pod `runAsNonRoot=true`, and `runAsUser=<none>` on all three containers. Two mutations, both with the anchor asserted on disk first: * busybox restored -> the new helm-unittest case fails * a uid re-pinned -> the case fails AND the drift guard fires helm unittest 644/644, drift 39/39. Chart already one patch above develop; manifest regenerated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The version-bump gate needs one patch above develop, and develop moved to 1.9.91 while this branch sat at 1.9.91 too. Caught on the push I had just made, not by CI. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 4a31686. Configure here.
aptracebloc
left a comment
There was a problem hiding this comment.
Reviewed the mechanism against the rendered chart, not just the description — it holds up well.
The wait is genuinely a readiness gate, not a bare port check: wait-for-mysql connects to the mysql-client Service, which is a plain ClusterIP (no headless, no publishNotReadyAddresses), so it only routes to Ready endpoints — and MySQL's readiness is a real mysqladmin ping. The TCP connect therefore succeeds only once MySQL actually responds, which is exactly the condition jobs-manager needs. Bounded at 300s, deadline checked after each attempt, exits non-zero with a clear diagnostic on expiry (loud failure, not a silent Init hang). Image is byte-identical to the api container, so no extra pull, and the no-runAsUser-anywhere posture keeps OpenShift arbitrary-UID SCC admission intact while plain Kubernetes gets UID 1001 from the image. Making initContainers: unconditional rather than gated on hostPath.enabled is the right depth for the fix — an edge without hostPath previously rendered no init containers at all. All four Bugbot findings were addressed in code.
One scope note, not a blocker: this guards cold start only — an init container doesn't re-run if MySQL restarts mid-run — but that matches the ticket, and runtime reconnection is the app's concern.
Not approving yet: the PR is CONFLICTING with develop. develop reworked DRIFT_GUARDS in the Makefile from a single line into a |\-continuation multiline block, and this branch appended its guard to the old single-line form; there's also a Chart.yaml version-line collision (develop is now 1.9.91). Please merge develop in and re-resolve — re-add jobs-manager-waits-for-mysql.sh in develop's new multiline DRIFT_GUARDS format, and re-bump the chart version above 1.9.91 — then it's good to land.
— drafted with Claude Code
Three conflicts, and two of them would have silently reverted work: * Makefile DRIFT_GUARDS -- develop moved to one-guard-per-line (#933) while this branch added `jobs-manager-waits-for-mysql.sh` to the old single-line form. UNION merged: develop format plus our guard as its own line. Taking either side alone drops a REQUIRED guard -- ours would have lost `hostpath-reads-guarded.sh`, theirs ours. Verified both are present: 40 green. * jobs-manager-deployment.yaml -- ours is this PR (unconditional initContainers), theirs is the nil-guarded hostPath read from #939/backend#2910. Adopted their guard inside our block; taking ours verbatim would have reverted a fix that keeps `--reuse-values` rendering when the hostPath key is absent entirely. Verified: 3 live hostPath reads, 0 unguarded, and the chart renders with hostPath omitted. * Chart.yaml -- 1.9.92, one patch above develop 1.9.91. Also reworded one comment of mine: `hostpath-reads-guarded.sh` reads RAW text, so my line describing the old gate tripped it by quoting the bare read. Prose breaking a check rather than satisfying one -- the guard cannot tell a comment from code, which is worth fixing in the guard rather than in every comment that needs to name the anti-pattern. Not doing that here: it is develop s guard and belongs in its own change. helm unittest 644/644, drift 40/40. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Merged develop (
One thing I worked around rather than fixed, and filed instead — backend#2946: No live read on that line. I reworded the comment to get past it, which is the wrong direction — the guard makes the anti-pattern unmentionable, so the next person documenting this migration either rewords around it or deletes the explanation. The fix is to strip comments before matching, as |
aptracebloc
left a comment
There was a problem hiding this comment.
Re-reviewed at head 7861fb2 after the develop merge. The fix survived intact and the two dangerous conflicts were resolved the right way.
Verified against git diff origin/develop...HEAD:
wait-for-mysqlinit container — still present and now unconditional (moved out from under thehostPath.enabledgate), still connecting to themysql-clientClusterIP Service (a real readiness gate, Ready-only endpoints), still bounded at 300s with the deadline checked after each attempt, still failing loudly with the connection-refused diagnosis. UID posture is right: it reuses the jobs-manager app image with norunAsUser, inheriting pod-levelrunAsNonRootexactly likeapiandpods-monitor-container— so admission passes on both OpenShift (project range) and plain Kubernetes (USER 1001 from the image).MakefileDRIFT_GUARDS — union-merged correctly: develop's one-guard-per-line|\format (#933) is preserved andjobs-manager-waits-for-mysql.shis added as its own line, withhostpath-reads-guarded.shkept. Neither side's guard was dropped. Drift gate green.jobs-manager-deployment.yaml— develop'shostPathnil-guard (#939/backend#2910) was adopted inside the unconditionalinitContainersblock rather than reverted.Chart.yaml— 1.9.92, one patch above develop's 1.9.91.
No conflict markers, no dropped hunks. All four prior Bugbot threads resolved; bugbot run on this head reports no new issues; CI is fully green. Approving.
— drafted with Claude Code
develop reached 1.9.92 when #942 merged, so this branch no longer sat above it and `chart content => Chart.yaml version bump` failed. The gate compares against the develop tip (BASE_SHA), not the fork point, so a bump goes stale whenever develop moves under a long-lived branch. Caught by re-running the check rather than reading it: the PR still showed a green pass from before #942 landed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…(backend#2872) Three things, all from @saadqbal review of client#922. 1. #942 FALSIFIED TWO CLAIMS THIS PR ADDS. Its `wait-for-mysql` is unconditional and unequal on BOTH dimensions (cpu 10m/100m, memory 16Mi/64Mi) with no values key, so `values.yaml` and `jobs-manager-deployment.yaml` were both wrong to say equalising cpu buys Guaranteed for jobs-manager on a CSI cluster. Measured on the merged tree with hostPath off and every resources.* knob equalised: t-jobs-manager Burstable wait-for-mysql:cpu(req=10m,lim=100m); wait-for-mysql:memory(req=16Mi,lim=64Mi) Guaranteed is now unreachable through values on EVERY cluster. Both comments corrected. 2. AND THE GUARD STAYED GREEN THROUGH IT -- this PR subject one turn deeper. The goldens pin the CLASS and the unresourced-init SET; `wait-for-mysql` is resourced, merely unequal, and jobs-manager was already Burstable, so nothing moved. `scripts/tests/qos-reachability.sh` asserts reachability: it equalises every resources.* knob derived from values.schema.json, classifies through pod-qos-class.py (no second copy of ComputePodQOS), and compares per-pod verdicts to a golden by set equality both ways. Proved rather than argued -- adding an unconditional unequal init container to an ALREADY-Burstable pod (the exact #942 shape) leaves all 19 existing QoS tests green and reddens this guard, naming the container. The golden records blocking container NAMES, not quantities, so a resource bump does not churn it. 3. BOTH INSTALLER HEADERS called the training pod BURSTABLE flat. On a GPU edge it is BestEffort: the GPU path requests only nvidia.com/gpu / amd.com/gpu plus ephemeral-storage, and client-runtime `_get_gpu_resources` never reads RESOURCE_REQUESTS / RESOURCE_LIMITS there. backend#2871 raised both GPU BestEffort workloads; client#919 fixed the device-plugin half and the issue was CLOSED with the training half unfixed, so the record is written into the two files rather than left as a reference. Chart 1.9.94. drift 43/43, helm unittest 657/657, QoS 19/19, Pester 887/887. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…instant can't force a rollback (backend#2908) (#949) Closes tracebloc/backend#2908 `pending_age_seconds` gated `last_deployed` on the SHAPE of its RFC3339 fields, not their RANGES: match(ts, /^[0-9][0-9][0-9][0-9]-[0-9][0-9]-[0-9][0-9]T.../) so `2026-00-01`, `2026-08-00`, or `...T40:99:99` passed, fed the civil→days math a nonsensical date, and yielded a finite-but-garbage epoch. A month/day that rolls the instant into the deep past lands a LARGE POSITIVE age, and the downstream sanitiser only rejects empty / non-numeric / NEGATIVE: case "$AGE" in ''|*[!0-9-]*) AGE="" ;; esac so the bogus-but-positive age sailed through, cleared WEDGE_MIN_AGE_SECONDS, and the #554/#2877 recovery path rolled back a release that was never wedged — discarding an operator's in-flight upgrade values. Pre-existing since #923. Fix (the class, not just the instance) — three sibling gates so a non-instant yields NO age (empty), which the caller already treats as "too recent, never clobber", the safe side: - Range-check the date/time fields in the awk body before trusting the derived age: month 1-12, day 1-31, hour 0-23, minute 0-59, second 0-60 (leap second). - The zone-offset parse is SHAPE-only in the same way: `+40:00`/`+99:99` are accepted and shift the epoch by up to ~4 days — enough to age a genuinely RECENT (in-flight) upgrade past the threshold and roll it back. Bound it to a real RFC3339 numoffset (hour 0-23, minute 0-59) too. - The year is the one field the RFC3339 ranges can't bound (any 4 digits are in-range), so `0000`/`1970`/`1999` still fabricate a huge positive age. Add a fail-safe domain floor: a pre-2000 stamp is "cannot tell" (skip). Worst case if a release were somehow genuinely that old: we skip a rollback, never the reverse. In-range corruption to another plausible date is undetectable from the stamp alone and out of scope. A real recent in-flight timestamp and a genuine aged wedge are unaffected. Tests: - scripts/tests/auto-upgrade-inflight-vs-wedge.sh gains non-instant unit assertions (every out-of-range field / offset / pre-2000 year -> empty age, asserted on empty-vs-non-empty so they are independent of wall-clock now) and decision-level cases (month `00`, day `00`, offset `+40:00`, year `1970` -> skip, not rollback). Mutation-proved: reverting any one gate reddens both layers — the unit cases return fabricated ages and the decision cases ROLL BACK. A leap-second counter-guard pins the ss<=60 bound so the gate is not over-tight. - client/tests/auto_upgrade_test.yaml gains a render-level guard that all three gates are present and the field-range gate sits before the epoch math. Chart.yaml version 1.9.91 -> 1.9.93 (chart-version-guard: template changed; 1.9.92 already taken on develop by #942). Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
|
/fr-pass Functional review on staging — passed, with direct evidence. Journey (tier A), install via the real installer → client components healthy → CLI installed from its signed release and signed in → dataset ingested for every task type → use case published → model trained and the leaderboard read. The train leg, which is the one that matters: This repo's change is on the path that run exercised, so this is functional evidence rather than an inference from code review. Two things stated rather than glossed:
|
…ss for real (backend#2872) (#922) * docs(qos): correct the false Guaranteed-QoS claims and assert the class for real (backend#2872) Six claims in this repo said a workload had Guaranteed QoS. None did, and two files contradicted themselves internally -- install-client-helm.sh:38 said "requests == limits (Guaranteed QoS)" while :324 said "Guaranteed QoS is lost by design", and install-k8s.ps1 carried the same pair. The training-envelope claims went stale when backend#2418 made CPU a request-only share weight; nobody noticed because no test tier could assert a QoS class. Corrected: values.yaml, values.schema.json (which SHIPS to users), jobs-manager-deployment.yaml, mysql-deployment.yaml, install-client-helm.sh, install-k8s.ps1. Deleted the false history at values.yaml:964, which called jobs-manager "BestEffort QoS in older releases". It was Burstable from day one: before #66 introduced the resources block the template hardcoded requests cpu 100m / memory 256Mi and limits cpu 500m / memory 512Mi (git show 2d9d013^:client/templates/jobs-manager-deployment.yaml). Requests AND limits set, so never BestEffort. The pod was under-resourced, which is the real cause; the class was never the difference. Amended the ACCURATE comments too. They attributed Burstable to cpu req != lim alone, which is true but incomplete and would send the next fix attempt down a dead end: ComputePodQOS requires requests == limits in every container INCLUDING init containers, and jobs-manager renders an unresourced init-writable-data whenever hostPath.enabled=true while mysql renders an unresourced mysql-format-guard UNCONDITIONALLY. Measured on the rendered chart: equalising cpu buys Guaranteed for jobs-manager only on a CSI cluster, and for mysql nowhere. New scripts/tests/pod-qos-class.{py,bats} DERIVES each pod class with the kubelet rule instead of asserting the values believed to imply it -- the defect shape that let all of this survive. Asserted per hostPath mode, because the mode changes the answer. Six mutations proven to redden; two of them (an ignored memory dimension, a workload silently becoming BestEffort) exposed real gaps in the first version of the suite. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(qos-check): only cpu and memory count toward the class (backend#2871) The first version of pod-qos-class.py treated ANY resource key as qos-relevant. ComputePodQOS skips everything isSupportedQoSComputeResource rejects, so a container whose only requests are nvidia.com/gpu and ephemeral-storage -- exactly what client-runtime._get_gpu_resources produces for every GPU training pod -- has EMPTY qos-relevant maps and the pod is BestEffort, not Burstable. So the checker would have reported those pods as Burstable and quietly agreed they were fine: the same defect shape it exists to catch. Found while verifying backend#2871 against the real code rather than trusting the ticket. Two tests added (gpu-only -> BestEffort; extended resources alongside equal cpu/memory -> still Guaranteed, so the rule is not "ignore unknown keys"), and the mutation back to the old behaviour reddens. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(chart): bump to 1.9.88 — develop reached 1.9.87 while this PR was open * docs(qos): the class follows DERIVE_JOB_ENVELOPE, not the template (backend#2872) Review on rfcs#63 (@saadqbal) caught this sweep swapping one wrong QoS claim for another -- the one thing it cannot afford to do. Three of my own claims here had the same defect: they read as though the derive path's Burstable envelope were the DEFAULT. It is not. DERIVE_JOB_ENVELOPE is off by default (client-runtime jobs_manager.py:2664), and the fallback assigns the literal cpu=1,memory=2Gi to requests AND limits, so those pods are GUARANTEED -- client-runtime says so outright at :2412: "the literal (and a symmetric env-override) has request == limit, so those pods are Guaranteed ... The derive path is deliberately Burstable on CPU instead." There is even a test named test_derivation_disabled_by_default_uses_literal. So values.yaml, values.schema.json (which ships) and the template comment now split by the flag and state BOTH classes, rather than naming one. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(chart): bump to 1.9.90 — develop reached 1.9.89 while this PR was open * test(qos): derive the expected class set from the render, not a hand-list (backend#2872) Bugbot, and it is right about the guard I built to close this exact shape. The expected table restated SIX workload names in the bats file. The chart renders TEN: t-auto-upgrade, t-image-refresh, t-egress-reachability-check and t-storage-assertions-check were classified by the checker and then ignored by the assertion, so a silent Burstable<->Guaranteed change on any of them stayed green. And the init-container check tested that two names APPEARED -- a membership test where a set comparison was needed -- so init-mysql-data could vanish with nothing reddening. A restated list inside a guard against restated claims. CLAUDE.md rule 1, in the file that cites it. Now `pod-qos-class.py --expect <file>` compares the render against a declared expectation by SET EQUALITY IN BOTH DIRECTIONS: a workload the chart starts rendering fails until someone classifies it, and a row naming a workload the chart no longer renders fails rather than being satisfied by nothing. The unresourced-init set is asserted exactly, because that set is what decides whether Guaranteed is reachable at all. Three new tests prove the check can fail: a dropped row, a stale row, and a vanished init container each redden. Five mutations proven, including a REAL chart mutation -- stripping resources from image-refresh-cronjob.yaml, one of the four workloads that was previously invisible -- which now reddens both mode assertions. Verified: bats 53/53, helm unittest 641/641, drift 37/37, version guard ✓. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(qos): render the GPU device plugins — the chart's only Guaranteed pods (backend#2872) Bugbot, and it is the sharpest gap yet: `_render` never enabled gpu.devicePlugin, so neither nvidia-device-plugin-daemonset nor amdgpu-device-plugin-daemonset was ever classified. Measured: both are GUARANTEED -- which is exactly what client#919 bought. Stripping their resources would return them to BestEffort, leave every GPU training pod Pending, and this guard would have stayed green. So the one place Guaranteed actually exists in this chart was the one place the QoS suite could not see. _render now takes extra --set pairs, and the suite asserts a class table PER VENDOR -- the chart renders one plugin keyed on gpu.devicePlugin.vendor, so asserting only nvidia would leave the amd template unclassified, the same partial-coverage mistake one level down. Side benefit worth naming: the checker's Guaranteed path is now exercised against REAL chart output rather than only synthetic manifests. A classifier that could never produce "Guaranteed" from the chart would previously have passed every assertion in this file. Proven with a real chart mutation, anchor asserted: renaming both `resources:` blocks in gpu-device-plugin.yaml reddens the two new tests. My first attempt used the wrong indentation, matched 0 lines, and reported "gap NOT closed" -- which is why the anchor assertion is there. Verified: bats 55/55, helm unittest 641/641, drift 37/37. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(qos): the "only route" claim was false, and the preflight skipped .py guards (backend#2872) Review on client#922, all verified on this head rather than taken on trust. - "Pod-level resources are the only route" was the same false QoS claim in the PR whose job is deleting false QoS claims. Resourcing every init container reaches Guaranteed too: verified by patching the render so every container including the init containers carries requests == limits, at which point pod-qos-class.py returns Guaranteed for BOTH t-jobs-manager and mysql-client. Rejected on cost (the reservation is held for the pod's whole life), not capability. - KEP-2837 is beta in 1.34, not 1.36. Checked rather than trusted, as asked; the "measured on a real 1.36 cluster" mentions are about our own pin and stand. - pyyaml-preflight enumerated .sh and .bats only, so its "the denominator is the tree" header was false and pod-qos-class.py was invisible to it. Now 23 guards, with .py treated as whole-file python -- extract_python finds only embedded blocks, so without that a python guard would fail closed while correctly wrapped. Mutation-proved: a bare `import yaml` reddens test 1 naming pod-qos-class.py. - Deleted _class_of: dead after the --expect rewrite, and its unanchored `$1 ~ w` would have matched a sibling workload and asserted a different pod's class. - Six claims, not ten, in both headers. CLAUDE.md rule numbers replaced with the rules themselves -- a reader in this repo cannot resolve a number that lives in a workspace file. - _norm's zero-quantity exception stated: requests == limits == 0 derives Guaranteed here and is BestEffort on a cluster. helm unittest 641/641, drift 38/38, check-facts, qos 17/17, preflight 3/3. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(qos): classify the lockdown Job, and fail closed on any unreached template (backend#2872) Bugbot Medium on client#922, verified before fixing: `egress-enforcement-check` renders only with `networkPolicy.training.enabled=true`, `allowExternalHttps=false` AND a probe host set -- three values no mode in this suite set, so the Job was never classified. Stripping its resources demoted it to BestEffort with the suite green. TWO FIXES, because one more mode closes the instance and not the class. - A lockdown render mode plus `pod-qos-expect.lockdown.txt`, compared by set equality in both directions like every other mode. Mutation: removing the Job's `resources` block reddens test 18. - A CROSS-MODE COVERAGE ASSERTION, both sides derived. The chart side greps the templates for a pod-bearing `kind`; the reached side is the union of a new `--sources` mode over all five renders. It reads helm's `# Source:` comment from the raw text, because `yaml.safe_load_all` discards it and a workload NAME cannot say which file produced it -- a template that renders nothing contributes no name to notice the absence of. Empty on either side fails closed. Mutation: a new conditionally-rendered Deployment no mode reaches reddens test 19 by name. The coverage guard earned itself immediately: it caught my own first draft passing `gpu.enabled` / `gpu.vendor` instead of `gpu.devicePlugin.*`, so the two GPU modes were rendering no device plugin at all. That is exactly the failure it exists to report, found on the way in. Merged develop; the manifest conflict is a hash, so the only correct resolution is the recomputed value, not either side. helm unittest 641/641, drift 38/38, qos 19/19, check-facts 14/14, preflight 3/3. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(qos): the coverage guard held its own copy of the kind list (backend#2872) Bugbot on client#922, and it is my own restatement one commit old: the coverage assertion grepped for a hand-written `Deployment|StatefulSet|DaemonSet|Job|CronJob` while `POD_KINDS` also carries `Pod`. A raw Pod template was therefore absent from the denominator, so if no mode rendered it, coverage stayed green and `--expect` never saw it -- a BestEffort demotion on that pod would not fail anything. Fixed by removing the copy, not by adding `Pod` to it: a new `--kinds` mode prints POD_KINDS and the bats side builds its grep pattern from that, so the two cannot disagree again. Empty output fails closed -- an empty alternation would make the grep match every template or none, and either way the set comparison stops meaning anything. Mutation, Bugbot's exact scenario: a `kind: Pod` template gated behind a value no mode sets. With the derived list, test 19 fails naming `zz-raw-pod.yaml`; with the old hand-written list re-pasted in its place, the same template is invisible and test 19 passes. Both measured. helm unittest 641/641, drift 38/38, qos 19/19, preflight 3/3, shellcheck clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(qos): the class follows the flag AND whether the pod asks for a GPU (backend#2872) Asad, review of #922. The correction overshot in the three places that ship: values.schema.json, values.yaml and the jobs-manager comment all said the class follows the flag, full stop. On a GPU cluster it does not -- client-runtime resolves a GPU envelope to nvidia.com/gpu and ephemeral-storage only, neither is QoS-relevant, so the container sets no cpu/memory at all and the pod is BestEffort under EITHER setting. That matters because the schema description ships to operators, and the BestEffort/oom_score_adj argument used to justify Guaranteed elsewhere is exactly what a GPU operator would conclude does not apply to them -- then flip DERIVE_JOB_ENVELOPE on to escape Burstable and change nothing on their cluster. All three now carry the same sentence: cpu-only + off -> Guaranteed, cpu-only + on -> Burstable, GPU -> BestEffort either way. pod-qos-class.py already encoded the carve-out (345ab1d in this PR); the prose asserted the opposite of the chart' own checker. Dropped the verbatim client-runtime quote that propped up the Guaranteed claim. A cross-repo prose quote is a dependency nothing checks -- client-runtime#457 exists to delete one in the other direction -- and it pinned the very sentence being corrected here. Cite the behaviour and where it lives, do not copy it. Declared the checker' third divergence: a limits-only container reads Burstable here and is Guaranteed on a cluster, because the API server defaults requests from limits before the class is computed and this reads the rendered manifest. Safe direction, no chart pod does it -- written down because the other two are, and an undeclared divergence is paid for by whoever meets a red CI on correct code with nothing to read. helm unittest 641/641 (the body said 631 -- fixed there too). bats pod-qos-class: 19/19. make drift: all 38 guards green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(chart): bump to 1.9.94 (backend#2872) develop reached 1.9.92 when #942 merged, so this branch no longer sat above it and `chart content => Chart.yaml version bump` failed. The gate compares against the develop tip (BASE_SHA), not the fork point, so a bump goes stale whenever develop moves under a long-lived branch. Caught by re-running the check rather than reading it: the PR still showed a green pass from before #942 landed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(qos): correct the claims #942 falsified, and assert REACHABILITY (backend#2872) Three things, all from @saadqbal review of client#922. 1. #942 FALSIFIED TWO CLAIMS THIS PR ADDS. Its `wait-for-mysql` is unconditional and unequal on BOTH dimensions (cpu 10m/100m, memory 16Mi/64Mi) with no values key, so `values.yaml` and `jobs-manager-deployment.yaml` were both wrong to say equalising cpu buys Guaranteed for jobs-manager on a CSI cluster. Measured on the merged tree with hostPath off and every resources.* knob equalised: t-jobs-manager Burstable wait-for-mysql:cpu(req=10m,lim=100m); wait-for-mysql:memory(req=16Mi,lim=64Mi) Guaranteed is now unreachable through values on EVERY cluster. Both comments corrected. 2. AND THE GUARD STAYED GREEN THROUGH IT -- this PR subject one turn deeper. The goldens pin the CLASS and the unresourced-init SET; `wait-for-mysql` is resourced, merely unequal, and jobs-manager was already Burstable, so nothing moved. `scripts/tests/qos-reachability.sh` asserts reachability: it equalises every resources.* knob derived from values.schema.json, classifies through pod-qos-class.py (no second copy of ComputePodQOS), and compares per-pod verdicts to a golden by set equality both ways. Proved rather than argued -- adding an unconditional unequal init container to an ALREADY-Burstable pod (the exact #942 shape) leaves all 19 existing QoS tests green and reddens this guard, naming the container. The golden records blocking container NAMES, not quantities, so a resource bump does not churn it. 3. BOTH INSTALLER HEADERS called the training pod BURSTABLE flat. On a GPU edge it is BestEffort: the GPU path requests only nvidia.com/gpu / amd.com/gpu plus ephemeral-storage, and client-runtime `_get_gpu_resources` never reads RESOURCE_REQUESTS / RESOURCE_LIMITS there. backend#2871 raised both GPU BestEffort workloads; client#919 fixed the device-plugin half and the issue was CLOSED with the training half unfixed, so the record is written into the two files rather than left as a reference. Chart 1.9.94. drift 43/43, helm unittest 657/657, QoS 19/19, Pester 887/887. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(qos): walk EVERY resources node, not just the top-level one (backend#2872) autoUpgrade, imageRefresh and egressProxy each expose requests and limits under their own parent, so they were never equalised and their pods read blocked while an operator could already reach Guaranteed through values. The guard was agreeing with its own incomplete domain. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(qos): probe the free-form resources node instead of recording it blocked (backend#2872) Builds on 691841e rather than replacing it -- the recursive walk and the fatal/informational note split are that commit, and they are right. One verdict was still wrong: `telemetryCollector.resources` is `{"type": "object"}` with no declared keys, so it fell into #NOTAPAIR and t-telemetry-collector was recorded `blocked`. But the schema does not forbid keys either, and the DaemonSet renders the node with `toYaml $tc.resources` -- a wholesale passthrough -- so a requests/limits pair set there reaches the pod. Measured, with ONLY that key equalised: t-telemetry-collector Guaranteed every container has requests == limits otel-collector {requests: {cpu: 1000m, memory: 1Gi}, limits: {same}} So `blocked` was a verdict reached by never having looked -- the one thing this guard exists to refuse. Free-form nodes are now PROBED with the canonical pair and the run reports that the verdict came from a probe rather than a declaration. `gpu.devicePlugin.*` stays #NOTAPAIR: its flat one-pair shape is applied to both sides by construction (client#919 makes the split form unexpressible), so there is genuinely nothing to equalise. Golden: two rows corrected, with the reason recorded beside them. Mutation-proved: disabling the probe branch returns both rows to `blocked` and reddens the guard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(qos): the pod-level branch needs the cpu/memory carve-out too (backend#2872) KEP-2837 pod-level resources short-circuit above the container walk, so an envelope of extended resources only returned Burstable where the kubelet says BestEffort. The container path already carried this guard. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Revert "fix(qos): the pod-level branch needs the cpu/memory carve-out too (backend#2872)" This reverts commit 51681eb. * test(qos): report the checker output when an expectation fails (backend#2872) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(qos): record that #948 moved both CronJobs to Guaranteed (backend#2872) The four QoS failures were real and were NOT visible on the branch head. CI evaluates the pull_request MERGE of head into base, and #948 (526f4ad) landed on develop after this branch last merged it: pinning CronJob memory to requests == limits moves t-auto-upgrade and t-image-refresh from Burstable to Guaranteed. On the head alone every case passes, which is why it first read as an environment difference; reproduced by merging develop locally. Measured under CI pinned helm v3.15.4 on the merge -- both CronJobs Guaranteed on every profile -- and all five expectation files updated. Nothing in the chart prose called the CronJobs Burstable, so no comment needed correcting. Rows re-derived from measurement rather than restored from a pre-revert copy: doing the latter would have reintroduced the TEST that 2d25d1a reverted without its code. That revert and b0e4500 diagnostic both stand untouched. Separately worth recording: local helm here is v4.1.1 while CI pins v3.15.4, so chart renders verified locally were not on the CI configuration. Everything above ran under 3.15.4. drift 43/43, helm unittest 660/660, full bats 1647/1647 -- on the merge, v3.15.4. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(qos): harden the coverage loop renders, and say what the finding did NOT reproduce (backend#2872) Bugbot Medium: the coverage loop called `_render` in five case arms with no `|| return 1` -- the only `_render` calls in the file without it -- so a failed render fell through and whatever it left behind was measured for coverage. THE MECHANISM AS DESCRIBED DOES NOT REPRODUCE, and that belongs on the record rather than being implied away. The finding said `helm template` streams, so a failed render can still emit earlier pod documents. Measured on the pinned v3.15.4 AND on v4.1.1, across three failure modes -- a late template `fail`, a values-schema violation, and invalid YAML in the rendered output -- helm buffers the whole manifest and writes 0 bytes every time. With an empty file the classifier already refuses ("no pod-bearing template in this render") and the `--sources` call already carried `|| return 1`. Fixed anyway, because "safe" was resting on two accidents this test asserts neither of: helm buffering, and the classifier refusal. A helm that ever did stream would reopen it in silence, and one word per arm is cheaper than that dependency. Mutation-proved: a late `fail` in requests-proxy-service.yaml now reddens "coverage: every pod-bearing template is classified by at least one mode". bats-hygiene 14/14 accepts the form. drift 43/43, helm unittest 660/660, full bats 1647/1647 under v3.15.4 on the merge. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(qos): re-land the pod-level cpu/memory carve-out, verified under helm 3.15.4 (backend#2962) KEP-2837 pod-level resources short-circuit above the per-container walk, so a pod-level envelope of extended resources only (nvidia.com/gpu + ephemeral-storage and nothing else) returned Burstable where ComputePodQOS says BestEffort: with pod-level resources set the kubelet derives the class from them alone, filtered to cpu/memory, and does not fall back to the containers. The per-container path already carried this guard (backend#2871); this is its missing pod-level half. The first attempt (51681eb) was reverted after CI reddened four --expect tests, blamed on a helm-3.15.4-vs-4 render difference. Re-verified under the pinned helm v3.15.4 (identical build to CI, v3.15.4+gfa9efb0): no chart workload renders pod-level resources on any hostPath mode under 3.15.4 OR 4, so the carve-out is inert on every chart render and cannot flip an --expect result. The reverted red was not reproducible under the pinned helm in isolation, in a matched Linux amd64 container, or in the full bats suite; its diagnostics were swallowed by the pre-_ok assertion, so it was reverted without evidence. The suite is now 21/21 green under helm 3.15.4, and the _ok helper (added since) means a re-land cannot go blind again. The new pod-level BestEffort case is built so it cannot pass for the wrong reason: its containers are themselves Guaranteed, so a fall-through would read Guaranteed and the reverted no-carve-out branch read Burstable -- the case rejects both and only the carve-out yields BestEffort. Closes tracebloc/backend#2962 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Revert "fix(qos): re-land the pod-level cpu/memory carve-out, verified under helm 3.15.4 (backend#2962)" This reverts commit e777910. The carve-out encoded the wrong kubelet rule. Verified against the Kubernetes source (pkg/apis/core/v1/helper/qos/qos.go + component-helpers/resource/helpers.go): ComputePodQOS gates the pod-level branch on IsPodLevelResourcesSet, whose supported set is {cpu, memory, hugepages} only -- an extended-resources-only envelope (nvidia.com/gpu + ephemeral-storage) makes it FALSE, so with the PodLevelResourcesFixKubeletQOSClass gate (Beta ~1.36) the kubelet FALLS THROUGH to the per-container walk and returns Guaranteed when the containers are Guaranteed -- NOT BestEffort. The reverted commit returned BestEffort and asserted it with a test built on Guaranteed containers, which is the pre-fix behavior and is wrong for the 1.34-1.36+ clusters this chart targets. Only latent (no chart renders pod-level resources) kept it from being caught. backend#2962's premise and acceptance criteria rest on that pre-fix behavior; the finding is written up on the issue for re-scoping. Removing the wrong claim from this PR rather than papering over it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Arturo Peroni <arturo@tracebloc.io>
Closes tracebloc/backend#2913. Found by the e2e journey, on a real run.
The defect
jobs-manager opens its database connection during initialisation and exits when it fails, so a MySQL a few seconds behind it crashloops the container:
Errno 111 is connection refused — nothing is listening yet. It is none of the four causes that error message helpfully lists, which is why it cost a real investigation to separate from backend#2492.
It recovers on its own, and that is what makes it expensive. By the time anyone looks the pod is healthy and the reason survives only in the previous container's log. On the journey the leg had already failed.
The fix, and the thing it uncovered
An init container that waits for
mysql-client:3306. An init container rather than a retry in the app: the ordering is a property of the pod spec, it needs no image change, and it reaches every edge on the next chart upgrade rather than the next jobs-manager release.And
initContainers:was gated onhostPath.enabled. An edge without hostPath rendered no init containers at all, so a wait added naively inside that block would have been absent exactly where it was needed. The gate now covers onlyinit-writable-data, which is what it was ever for;initContainers:is unconditional, as MySQL is.Rendered both ways:
Bounded, and it fails loudly. An unbounded wait converts a crashloop into a pod that hangs in
Initforever — quieter, and strictly worse, because a crashloop at least announces itself. On expiry it exits non-zero naming what it waited for, so the pod's events saywait-for-mysqlrather than a driver error three layers down.Verification
The guard renders rather than grepping the template, because the bug is not "the text is missing" but "the container does not run" — a text search would have found the block while an edge without hostPath ran nothing.
make checkgreen;automount-token-explicitgreen (14 pod specs, 13 templates);chart-version-guardgreen. Chart bumped1.9.90 -> 1.9.91, version and appVersion equal.One trap worth passing on
The guard's first version used
printf '%s' "$out" | grep -q PATTERNand reported failure on a successful match.grep -qexits the moment it matches,printfis killed by SIGPIPE (141), andset -o pipefailreturns that. The check failed exactly when the thing it looked for was present. It is acasenow, and the reason is written down beside it — this pattern is common enough in this repo's scripts to be worth a wider look.Note
Medium Risk
Changes jobs-manager pod startup ordering on every install/upgrade; misconfiguration could delay or block rollout for up to 300s, though behavior is bounded and heavily guarded by rendered-manifest tests.
Overview
Fixes a startup race where jobs-manager could crashloop on first boot when MySQL was not listening yet (connection refused on
mysql-client:3306), then look healthy by the time someone investigated.The chart adds an unconditional
wait-for-mysqlinit container that runs before the main containers. It probesmysql-client:3306with a 300s bounded Pythonsocket.create_connectionloop and fails loudly on timeout.initContainers:is no longer tied tohostPath.enabled—onlyinit-writable-datastays behind that gate—so CSI / non–hostPath installs get the wait too (they previously rendered no inits at all).The wait uses the
tracebloc/jobs-managerimage (non-rootUSERin the image) instead of busybox, with norunAsUserpins anywhere in the pod, preserving OpenShift arbitrary-UID admission alongsiderunAsNonRoot.Verification: helm unit tests now assert inits by name (not index
[0]); a new drift guardjobs-manager-waits-for-mysql.shrenders bothhostPathmodes and checks ordering, expiry messaging, and no UID pins when hostPath is off. Chart version 1.9.91 → 1.9.92.Reviewed by Cursor Bugbot for commit 7861fb2. Bugbot is set up for automated code reviews on this repo. Configure here.