release-train: develop -> staging - #952
Merged
Merged
Conversation
…ackend#2870) (#944) * feat(tests): track and ratchet the control-plane request footprint (backend#2870) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(tests): inline the footprint summer with a PyYAML preflight, fail closed on a failed render (backend#2870) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(tests): parse the full k8s memory grammar and fail closed on the unparseable (backend#2870) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(footprint): make the DaemonSet x1 arm falsifiable (backend#2870) The fixture DaemonSet had no replicas, so the special case and the missing-replicas fallback both yielded 1 and deleting the arm changed nothing. A stray replicas: 3 makes the two disagree. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(tests): mktemp the footprint render stderr, not a PID-predictable path (backend#2870) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(footprint): the scheduler max()es init containers, it does not add them (backend#2870) @aptracebloc nit 1, and it is more than a nit. A pod effective request per resource is max( sum(app containers), max(init containers) ): init containers run to completion ONE AT A TIME before the app containers start, so they are never resident alongside them. This summed both, which CONTRADICTED the guard own "STEADY STATE ONLY" note and inflated the figure the ceiling is compared against. Conservative is not harmless here -- this number is the input to backend#2460/#2461, so an inflated one sends that work after MiB that no scheduler ever reserves. PER-RESOURCE, not per-pod: k8s takes the max independently for memory and cpu. THE NUMBER DOES NOT MOVE: 3136 MiB / 900 m either way, because this chart init containers carry no requests. So the "64 MiB OVER" finding STANDS -- the method was wrong, the conclusion was not. Worth saying rather than leaving the doubt open. WHICH MEANS THE FIX IS INERT ON THE REAL RENDER, and a green run proves nothing about the formula. So the new fixture is built where the two answers disagree: app 300Mi/200m, init max 1000Mi/500m -> scheduler 1000 MiB / 500 m against sum-everything 1350 MiB / 750 m. BOTH numbers are asserted rather than "under the ceiling", which would pass on either formula. The 1350/750 figure was MEASURED by running the previous code in place, not computed by hand. Mutation-proved: max -> + reddens the fixture test. Only the formula, rebased onto ab756ea. My own mktemp fix for the same Bugbot Low is DISCARDED -- the co-driver got there first and theirs is better, using mktemp "${TMPDIR:-/tmp}/cp-footprint-helm.XXXXXX" rather than a bare mktemp. control-plane-footprint.bats 9 passing. gen-manifest --check clean, shellcheck -S warning -x clean, bash -n clean. The ~1600-test client bats suite is CI job. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
… it safe (backend#2626) (#911) * feat(chart): fullnameOverride, with the completeness guard that makes it safe (backend#2626) backend#2621 built this helper, proved the default render byte-identical, and REVERTED IT. The helper is the easy half. The release name appears ~174 times across these templates and is at least six different things, only two of which may follow an override -- and a partial routing produces `prod-auto-upgrade` beside `myrel-jobs-manager`, which is harder to reason about than a release that is merely badly named. So the guard is the deliverable and the helper rides along. WHAT MOVED (39 sites, 15 files): names of resources this chart creates, and two things that REFERENCE those names -- * `image-refresh-cronjob.yaml` DEPLOYMENT_NAME. It names a Deployment this chart creates; leaving it behind points `kubectl set image` at a workload that no longer exists. Not in the ticket's list. * `telemetry-collector-configmap.yaml`'s filelog glob, `{ns}_{release}-*/{container}/*.log`. It matches POD DIRECTORIES, and pods are named after the DaemonSet. Route the DaemonSet without the glob and the Collector runs, reports healthy, and collects nothing -- the exact failure that file already warns about. Also not in the ticket's list. WHAT STAYED: `app.kubernetes.io/instance` (Helm convention), the 27 `meta.helm.sh/release-name` annotations (Helm's bookkeeping), the 3 RELEASE_NAME/RELEASE envs (a HELM IDENTITY -- `helm status`, `helm rollback`), and 4 on-disk paths (a LOCATION: renaming orphans a tenant's data). THE TICKET PREDICTED THE BUG AND I WROTE IT ANYWAY. "A `sed` over `.Release.Name` catches it on the first pass; mine did." My first routing pass anchored the env-var exception with `$` against a CONCATENATED context string, so it never matched and the RELEASE_NAME env was routed -- backend#2620 re-introduced by the fix for backend#2621, exactly as written. Two more followed: `$.Release.Name` substituted as a plain string clipped `$.Release.Namespace` to `...$)space`, and routing ran over the helper's own body so `tracebloc.fullname` called itself until helm died. The third is now structural rather than guarded: the helper is inserted AFTER routing, so its body is never a candidate. The guard, `scripts/tests/fullname-override-completeness.sh` plus its assertions module, over EVERY platform profile in `client/ci/*-values.yaml`: 1. NO-OP override == release name renders identically to unset 1b. VERBATIM with the override unset, resource names carry the release name WHOLE 2. MOVED with a distinctive override, no resource name still carries the release name -- misses reported BY NAME 3. STAYED every exception still carries it, checked with the right predicate per class Assertions 2 and 3 are a pair: without 3, assertion 2 is satisfied by renaming Helm's own bookkeeping. PROFILES MATTER, and a single render hid it: `bm-values.yaml` sets `hostPath.enabled`, and the hostPath PVs and dataset directory exist in no other profile. One ad-hoc render checked ONE of the four release-scoped paths and called itself satisfied. ASSERTION 1b EXISTS BECAUSE A MUTATION SURVIVED. Assertion 1 diffs two renders that both pass through the helper, so `| trunc N` cancels out on both sides and is structurally invisible to it. The mutation was also inert for a three-letter release name. The release name used by the guard is now 38 characters, and 1b reads the default render directly. Non-determinism is MEASURED, not listed: secrets.yaml mints credentials with `randAlphaNum`, so the guard renders twice with identical inputs and excludes whatever differs. A hand-kept key list goes stale, and the template's variable names do not even map to the rendered keys (`$podTokenSecret` renders as `POD_TOKEN_SIGNING_SECRET`). Wired into `DRIFT_GUARDS`, which the REQUIRED `Source-of-truth drift` job runs -- a guard in a non-required job is advice. Evidence -------- helm unittest ./client (helm 3.15.4, CI's pin) -> 36 suites, 631 tests, OK 18 name-sensitive shell gates -> all pass helm lint / shellcheck -S warning -x -> clean the guard, 4 profiles -> 32 assertions OK mutations, each anchor asserted to have applied: route the RELEASE_NAME env -> caught (helm-identity) un-route one resource name -> caught (by name) route the dataset directory -> caught (on-disk path) trunc 20 on the default -> caught (verbatim) upper on the default -> caught (verbatim) restored -> green Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(chart): the guard walks every scalar, NOTES is routed and checked (backend#2626) All four review points, and the first two were the same defect. THE GUARD DID NOT COVER THE SITES THIS PR SINGLED OUT. MOVED read doc-root `metadata.name` only, while STAYED enumerated the exception classes separately - so the two halves disagreed about what the exceptions were and every name-REFERENCE site fell through the gap. Un-routing DEPLOYMENT_NAME (which `kubectl set image` targets) or the Collector filelog glob (which globs pod directories) rendered both broken and left the guard green on all four profiles. Now ONE classifier, TWO callers: `classify()` walks every string scalar and labels each release-name hit with the exception that licenses it, or None. MOVED is "nothing unlicensed"; STAYED is "every class non-empty and correct". Adding a class cannot weaken MOVED without adding an obligation to STAYED. 1339-1422 scalars per profile, against a handful of metadata.name before. NOTES.txt: L6, L24 and L27 routed. It was half-routed seven lines apart - L6 printed the release name while L13 printed the override, in the first thing an operator reads. Assertion 4 now checks it, and needed its own render: `helm template` omits NOTES, `--show-only templates/NOTES.txt` answers "could not find template", and BOTH `--dry-run` and `--dry-run=client` need a cluster on 3.15.4 - with a reachable one, ownership validation against real objects made the verdict depend on whose kubeconfig ran it. So it renders a chart copy in which NOTES is an ordinary template, with --debug (helm refuses to emit output it cannot parse as YAML, and NOTES carries ANSI escapes) and KUBECONFIG=/dev/null. PyYAML: a named refusal with EXIT 2, distinct from the 1 that means "the chart is incomplete" - a missing dependency used to print a traceback and then "[ERROR] fullnameOverride is incomplete in 4 profile check(s)", sending the reader hunting un-routed names that do not exist. And `.py` is now in pyyaml-preflight.bats`s filter: `extract_python` finds python embedded in shell, so the first sidecar in this tree escaped the class rule entirely and every future one would have too. values.schema.json: DNS-1123 pattern + maxLength 53, the same constraints Helm enforces on the value this replaces. Verified - a bad value now fails at template time by name instead of object-by-object at the API server mid-install. VERIFIED: make drift 35/35 guards green on all four profiles; CI=true bats scripts/tests/*.bats 1568 passing, 0 failures; shellcheck -S warning -x clean. Mutation-proved eight ways, each anchor asserted applied - and two mutations exposed weaknesses in my own fix: a broadened path allowlist swallowed unrouted sites out of MOVED (the path class was the only one a non-path could satisfy by accident - it now asserts its members are paths), and deleting assertion 4`s "no NOTES supplied" refusal changed nothing because the loop always supplies it (there is now a self-check that invokes the assertions without it and requires a refusal). Part of tracebloc/backend#2626 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(chart): refuse a fullnameOverride rename of a LIVE release (backend#2626) Bugbot High, and it is a data-loss-shaped bug rather than a naming one. fullnameOverride now routes tracebloc.secretName. That is correct for an INSTALL and unsafe for a RENAME: setting the override on a release that already exists moves the Secret name, so the lookup at the top of secrets.yaml misses and four of the six credentials fall to their tier-3 randAlphaNum and are MINTED FRESH. The MySQL PVC is keep-ed and still holds the old ones: helm upgrade -> STATUS: deployed mysql -> ERROR 1045 (28000): Access denied A successful upgrade that leaves the database unopenable, with no warning at any layer. The completeness guard this PR adds could not see it -- every rendered NAME follows the override exactly as designed; what did not follow was the DATA. secrets.yaml now looks for the Secret under the name it would have had WITHOUT the override. Present, plus an override that differs, means a live release is being renamed, and it fails with the migrate-deliberately remedy. A fresh install with an override, and a release that always had one, both see no old-name Secret and are untouched. MEASURED against a live cluster, not reasoned about -- k3d + `--dry-run=server`, which unlike `helm template` actually performs lookup: override + pre-existing un-overridden Secret -> REFUSED, by the named message override, no such Secret (fresh install) -> silent no override at all -> silent the dangerous case re-run after the controls -> still REFUSED The assertions go in client-credentials-have-a-secret-tier.sh rather than the chart suite, for that file own structural reason: this is a lookup, so helm-unittest renders it away. Three assertions -- the refusal exists, it derives the UN-overridden name from .Release.Name, and it gates on the two names differing. The middle one is the correctness of the whole thing: keyed on the overridden name it would compare a name against itself, never fire, and still read as a guard. Collapsed-run floor raised 13 -> 16. 3 mutations, all reddening, every anchor asserted applied. 631 chart tests pass, drift 35/35, completeness guard green, shellcheck clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(chart): key the re-mint refusal on the persisted data, not on a name (backend#2626) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(chart): hold the CLASS behind the Secret-lookup finding, not just the instance (backend#2626) fae5441 fixed the instance, and better than my own attempt twice over. I had un-routed `tracebloc.secretName` so a rename could not lose the credentials - UNTESTED, because `lookup` is inert under every client-side renderer I have. ea6568d refused the dangerous case and MEASURED it with `--dry-run=server`, the only way to exercise a lookup; fae5441 then re-keyed that refusal on the persisted DATA after Arturo showed name-keying caught one of three rename directions and missed reinstall-over-a-kept-PVC entirely. That work is on the branch. Mine is discarded rather than layered on: with the Secret un-routed, the refusal would compare a name against itself and fire on every override-set upgrade. What nothing covers is the CLASS. A Secret `lookup` keyed on a name that follows the override misses on a rename, and a missed lookup is not an error - it silently takes the last resolution tier. This chart has TWO such sites: secrets.yaml a `fail` reached by the miss (fae5441, held by client-credentials-have-a-secret-tier.sh) tracebloc.telemetryTokenPresent safe ONLY because it ORs a lookup on the legacy fixed name - and nothing asserted that So the second member was one edit from the same silent shape with no check. Assertion 5 requires every routed Secret lookup to carry one of the two mitigations in its own file. Which mitigation a site needs is not this assertion`s call - it is a text-level check and could not have established what `--dry-run=server` did. It only requires that one is still there. THE REFUSAL IS DETECTED ON THE INVARIANT, NOT THE ARITHMETIC, and that is the lesson from writing this against a moving target. My first detector matched `printf "%s-secrets" .Release.Name` - the shape the refusal had while it keyed on the un-overridden name. fae5441 improved the refusal and my detector would have reported it as NO refusal. A detector that breaks when the thing it guards gets better is worse than none: it pushes back toward the shape it was written against. It now keys on the property that must hold however the other half is computed - a miss on THAT lookup reaches a `fail`. Which names follow the override is DERIVED transitively, closing `include` over the helper definitions, so a helper added tomorrow that wraps `fullname` is covered without anyone remembering. Fails closed three ways: a define parser that matches nothing, zero routed sites in a chart known to have two, and zero Secret lookups at all. Mutation-proved four ways, each anchor asserted applied: the data-keyed refusal removed -> flagged the refusal no longer negates that lookup -> flagged telemetry legacy-name fallback removed -> flagged (the unguarded member) routed-var detection stops resolving -> fail closed VERIFIED on helm v3.15.4 (the CI pin): make drift 35/35; helm unittest 631/631 in 36 suites; client-credentials-have-a-secret-tier 17 assertions OK; shellcheck clean. No chart files touched, so no version bump. Part of tracebloc/backend#2626 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(installer): read the client Secret under fullnameOverride, and parse helper bodies whole (backend#2626) Two Bugbot findings on a043e52, both Medium, both real. 1. THE INSTALLER MISSES AN OVERRIDDEN SECRET NAME. `tracebloc.secretName` follows `fullnameOverride`, so on a release installed with one the Secret is `<override>-secrets`. `detect_installed_client` and `Get-InstalledClientInfo` read `<release>-secrets`, find nothing, and a live client whose id lives only in the Secret (the backend#2571 shape the chart now recommends) reads as UNIDENTIFIABLE - so `diagnose` and `upgrade` treat it as having no id. The override is already in the values both callers have open, so the effective prefix costs one more read of the same file rather than a second API call. Absent -> the release name, which is the chart`s own `default .Release.Name .Values.fullnameOverride`. Four tests, two per language, and the CONTROL is the load-bearing half: without it the fix is satisfied by always using the override key, which would break every ordinary release. Mutation-proved in both directions, in both languages - reverting to the release name fails the override case, always using the override fails the control. 2. MY DEFINE PARSER STOPPED AT THE FIRST `end`. `{{- define "x" -}}(.*?){{- end -}}` is non-greedy, so a helper containing an inner `if`/`range` closed at the INNER end and its tail was silently dropped. Measured: 28 of 55 helper bodies truncated. IT CHANGED NO ANSWER TODAY - no helper`s `tracebloc.fullname` reference happens to sit in a dropped tail, so the routed-helper closure came out at 21 either way. That is why it was worth fixing rather than noting: the guard`s coverage depended on WHERE in a helper an include happened to sit, and one edit moving an include below an `if` would silently un-route it - after which a routed Secret lookup reads as unrouted and assertion 5 stops requiring a mitigation. Replaced with a balanced parse over the `{{ }}` ACTIONS, so an `end` inside a string or comment cannot close a block early. A permanent self-test feeds it the exact shape the regex dropped - a helper whose `fullname` reference sits after an inner `if`/`end` - because the chart contains no such helper today and a check only exercisable by a bug already present arrives too late. Both halves asserted: the routed probe must be seen, the unrouted probe must not. VERIFIED on helm v3.15.4 (the CI pin): make drift 35/35 (manifest.sha256 regenerated - the two installer files are hashed there); helm unittest 631/631; Pester 776 passed / 0 failed / 15 skipped; the two new bats cases green; shellcheck -S warning -x clean. Part of tracebloc/backend#2626 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(chart): assert the SPECIFIC refusal, gate the interpreter, and correct the prose the refusal falsified (backend#2626) Asad`s five remaining items, and two of them were defects in my own guard. 1. THE SELF-CHECK WAS SATISFIED BY ANY NON-ZERO EXIT. Written as `if …; then error; else OK; fi`, so with python3 absent (rc 127) the sidecar never ran and it printed [OK]. The one assertion whose stated purpose is "an unreachable refusal is one nobody notices has stopped refusing" reported green for a reason unrelated to what it checks - the bare `assertRaises(Exception)` shape, in shell (CLAUDE.md rule 10). It now requires EXIT 1 specifically, and treats any other code as a cannot-tell finding. 2. NO INTERPRETER GATE, where 23 siblings have one. python3 absent gave rc 127, missed the exit-2 branch, and ended on "fullnameOverride is incomplete in 4 profile check(s)" - a missing tool reported as a chart defect, the exact misdiagnosis the module half was fixed for. Added in the siblings` `fail_closed` idiom, for python3 AND helm. Measured: absent python3 now exits 2 with "THIS IS A MISSING TOOL, NOT A VERDICT ON THE CHART". And that work exposed a third: my PyYAML guard caught `ModuleNotFoundError`, which is a SUBCLASS of `ImportError` - so it missed the parent, and a plain `ImportError` is exactly what `pyyaml-preflight.bats` injects to simulate an absent PyYAML. Under the repo`s own simulation the guard gave a TRACEBACK and rc 1 while the bats class rule stayed green, because it reads the AST and accepts either name. Widened to `ImportError`, which also covers the real-world broken-install case. Verified: rc 2 with the named refusal. 3. `fullnameOverride: ""` was schema-INVALID. The pattern required at least one character, so the chart`s own commented example at values.yaml would have been rejected if uncommented verbatim, while the template treats "" as unset (Arturo). Pattern now accepts the empty string; Bad_Name and a 54-char value still fail at template time. 4. values.yaml said changing it on an existing install is "a migration, not a config tweak". It is REFUSED outright. That was the most misleading line in the file - it invited planning a migration the chart will not start. 5. values.yaml said the chart "ALWAYS emits `<release>-secrets`", with pre-create commands using that name. False under an override, and the consequence after the PVC re-key is a tier-3 hard fail on clientId for an operator who followed the doc correctly. It now says the name follows the override and gives a command to read it off the render rather than guess. 6. THE REFUSAL BLOCKED A DOCUMENTED RECOVERY PATH. docs/MIGRATIONS.md Option C is uninstall -> clear claimRef -> re-create PVCs -> install. Only the PVCs carry `resource-policy: keep`, so uninstall deletes the Secret and the re-install renders PVC-present + Secret-absent -> refused, with a primary remedy ("copy the credentials") that has nothing left to copy from. Option C now saves the Secret in a step 0 and restores it under the new effective name in a step 4, and the refusal message names `kubectl delete pvc mysql-pvc` as the accept-data-loss path so the failure is escapable from the failure itself. NOT CLAIMED: the `--dry-run=server` matrix was run against ea6568d, before the PVC re-key. Asad flagged that and he is right - it is not measured for this version and is not recorded as such. VERIFIED on helm v3.15.4: make drift 36/36; helm unittest 640/640; client-credentials-have-a-secret-tier 17 assertions; pyyaml-preflight 3/3; chart-pull-secret green; shellcheck clean; manifest --check clean. Part of tracebloc/backend#2626 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(chart): accept a renamed release`s token Secret, and scope the path-class check to the chart (backend#2626) Three Bugbot findings on f585de1. One demoted with evidence, two fixed. 1. HIGH, DEMOTED: "Path class fails on cloud profiles". It does not, at head. Measured on all four CI profiles: aks 1, bm 4, eks 1, oc 1 - the cloud profiles each render ONE release-scoped path, the Collector`s queue directory `/var/lib/tracebloc/<release>/telemetry`, because `telemetryCollector` is on by chart DEFAULT and no client/ci profile sets it. The required drift guard is green and does not refuse a complete chart. The HAZARD is real though, and the finding named the right one with the wrong scope: the gate is tri-state since backend#1906, so a profile that disabled the Collector AND hostPath would have no release-scoped path, and a PER-PROFILE emptiness check would then refuse a complete chart. So the emptiness assertion moved from per-profile to ACROSS profiles - a profile with none is a `[note]`, a CHART with none anywhere is still an error, because "no path followed the override" is equally true of a chart that stopped scoping paths by release. Proved both ways: a 5th profile disabling both keeps the guard green (the false positive that would otherwise have fired), and neutering the path class reddens it. 2. MEDIUM, FIXED: the token lookup missed a renamed release`s Secret. `telemetryTokenPresent` accepted the override-following name and the legacy FIXED name, but not `<release>-telemetry-token` - so on a renamed release `telemetryCollectorState` hard-FAILED for an operator who had explicitly enabled the Collector, naming two names that were never going to match while the token sat there under a third. Accepted now, and named in the refusal. Accepted rather than refused, which is deliberately the opposite call from the credentials Secret: there a name miss means silently minting a password against a datadir holding the old one. The token is server-side and re-derivable (jobs-manager writes it, backend#2274), so finding the existing one is safe and is what the operator meant. Nothing pinned the accepted-name set, so nothing would have caught it going away again. `telemetry-token-agreement.sh` now asserts an AGREEMENT rather than a list: every name the lookup accepts must be reported by the refusal. Placeholders are COUNTED against arguments, because the first cut checked that the name appeared on the line - and the format string and its args share one line, so deleting a `%q` left the substring matching while printf silently dropped the argument. Caught by mutation-proving; the counting lives in a `.py` sidecar, which is inside pyyaml-preflight`s class rule now that it covers `.py`. 3. MEDIUM, ALREADY FILED: the installer still builds `{namespace}-jobs-manager` in six places. That is backend#2888, filed with the measurement - six reconstructing sites, two that already DISCOVER by pattern and are override-safe, and the structural fix (select on `app.kubernetes.io/instance`). It is a change to the installer`s naming model, not a patch, and it should land before fullnameOverride is recommended to operators. VERIFIED on helm v3.15.4: make drift 36/36; helm unittest 640/640; telemetry-token-agreement green and mutation-proved 4 ways; pyyaml-preflight 3/3; shellcheck clean; manifest --check clean. Part of tracebloc/backend#2626 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(chart): bump to 1.9.91 — develop reached 1.9.89 while this PR was open * fix(chart): make the token refusal escapable after an override-to-override rename (backend#2626) Bugbot, Medium: `telemetryTokenPreOverrideName` covers `<release>-telemetry-token`, so unset->A and mid-migration are handled, but A->B leaves the token under A`s name and the render hard-fails naming three names none of which will match. Correct, and the fix is NOT a fourth candidate name. WHY NOT ENUMERATE. Measured across the three consumers: the Collector`s volume (telemetry-collector-daemonset.yaml:197), the RBAC`s resourceNames (telemetry-token-rbac.yaml:102) and jobs-manager`s writer env (jobs-manager-deployment.yaml:351) ALL resolve `tracebloc.telemetryTokenSecretName` and nothing else. So a token discovered under any other name is one nothing is permitted to read - enumerating the namespace would turn a loud refusal into a green render with a Collector that cannot mount its token. That is strictly worse than the hard fail. What makes the hard fail acceptable is that it is ESCAPABLE, and it was not. The message now names the rename as the likely cause and gives the two ways out, neither of which is a reinstall: * copy the Secret to the name this render wants (the command is in the message) * leave telemetryCollector.enabled unset for one upgrade and let jobs-manager re-mint under the new name on its next re-authentication The second is safe because the Collector`s mount is `optional: true` and the daemonset says why in terms - "buffers until the token arrives" rather than "CrashLoopBackOff on every node" - so the window costs buffered telemetry, not node health. NO NEW PLACEHOLDERS, so telemetry-token-agreement.sh`s placeholder/argument arity check still holds; verified green. One design question left for the reviewer rather than decided here: whether the gate should skip the refusal entirely on a LIVE release, keying on the persisted MySQL PVC the way the credentials refusal now does. That covers every rename direction with no name arithmetic, and it costs the fresh-install protection on live releases. It changes the gate`s invariant, so it is the reviewer`s call and the evidence is on the thread. VERIFIED on helm v3.15.4: make drift 38/38; helm unittest 641/641; telemetry-token-agreement green; manifest --check clean. Part of tracebloc/backend#2626 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(guard): make the fallback mitigation the property, not the arity (backend#2626) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(manifest): regenerate after the guard-mitigation change (backend#2626) `make drift` was red on `gen-manifest.sh --check`: 9eb3c7e changed scripts/lib/summary.sh and scripts/install-k8s.ps1 without regenerating scripts/manifest.sha256, so the R8 static-analysis gate saw two stale digests. Manifest-only. Verified: make drift 39/39, helm unittest 641/641, bats 69/69 over the manifest/hygiene/style suites. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(chart): the rename remedy is incomplete on hostPath, and said otherwise (backend#2626) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(guard): a sidecar exit outside {0,1} is could-not-run, not a chart defect (backend#2626) The last half of @saadqbal`s interpreter finding. 9eb3c7e/head fixed the fabricated path-class claim - assertion 6 now reports NOT CHECKED rather than "NO profile rendered a release-scoped on-disk path" - but the final verdict still blamed the chart: $ PATH=<python3 that exits 127> bash scripts/tests/fullname-override-completeness.sh [ERROR] fullnameOverride is incomplete in 5 profile check(s) # rc 1 `if [ "$rc" -eq 2 ]` handled exactly 2, and `[ "$rc" -eq 0 ] || failures++` swept every other code into "the chart is incomplete" - 127 from a stale pyenv shim or a dangling symlink, 126 from a non-executable interpreter, 137 from an OOM kill. Now classified: 0 clean, 1 a finding, ANYTHING ELSE exits 2 with the named refusal. $ PATH=<python3 that exits 127> … [ERROR] the assertions exited 127, which is not a verdict … NOTHING ABOUT fullnameOverride WAS CHECKED. # rc 2 AND THE PREFLIGHT I ADDED CANNOT COVER THIS, which the comment now says because it read as though it could: `command -v python3` answers "is there something on PATH by that name", not "does it run". A shim that execs a deleted interpreter passes it and exits 127 here. Mutation-proved: folding 127/126 back into `failures` restores the false "incomplete in N profile check(s)" claim and rc 1; the classification restores rc 2 and zero such claims. VERIFIED at head on helm v3.15.4: make drift 39/39; helm unittest 641/641; shellcheck -S warning -x clean; manifest --check clean. Part of tracebloc/backend#2626 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(guard): "not checked" is not a pass, and the fallback detector gets a fixture (backend#2626) Two from @saadqbal, and they are the same sentence twice: an absence is not an answer. 1. The pathclass_missing branch printed NOT CHECKED and fell through to exit 0. The two causes it enumerates both raise `failures` elsewhere, so the branch looked safe -- but a third does not: rename the PATHCLASS marker in the sidecar, a pure refactor with every assertion intact, and assertion 6 stops asserting while DRIFT_GUARDS reports a pass. Now `could_not_run` -> exit 2, the vocabulary the interpreter loop already owns; a confirmed finding still outranks it, so a real defect is reported as one and not as "cannot tell". Mutation-proved with the anchor asserted: PATHCLASS -> PATH_CLASS took the guard from rc=0 to rc=2. 2. _fallback_on_line was pinned by nothing -- regress it to the file-wide `count("lookup ") >= 2` and the real chart still renders green, because the chart does not contain the shape that separates the two. Written-down fixture, same reasoning as _SELFTEST_TEMPLATE: two lookups both routed (the half the old form gets wrong), a literal-keyed real fallback, a single routed lookup, and the $var indirection. Mutation-proved: the old form reddens the first and the $var case. `make drift`: all 39 guards green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(chart): a path is not an object, and the PV caveat outlived the refusal (backend#2626) Two Bugbot findings on the pvCaveat added in 91e95f4. High: the caveat rendered ONLY inside the credential refusal, which is skipped once a Secret exists under the new name -- so an operator on their second attempt read the copy-the-Secret remedy with the caveat missing. values.yaml made it worse by saying "the hostPath PVs keep the RELEASE name": true of the on-disk PATH, false of the PersistentVolume OBJECTS, which are named through tracebloc.fullname and carry no helm.sh/resource-policy: keep. Following that on bare metal deletes the PVs and leaves the retained PVCs Bound to them. Separated path from object in values.yaml, and NOTES.txt now carries the caveat on every hostPath render -- the path the refusal cannot reach. Gated on hostPath alone, not on fullnameOverride being set, because the A->unset rename is the same hazard and would have gone quiet. Medium: .Values.hostPath.enabled read without the (default dict ...) guard the rest of the chart uses. Guarded. Note the stated consequence does not hold as written: with hostPath nil the render already dies in shared-images-pvc.yaml:3, before secrets.yaml, both before and after this commit. Nine other unguarded sites remain and are filed separately -- one of ten does not fix the nil case. make drift: all 39 guards green. NOTES verified rendered under --set hostPath.enabled=true and absent when false. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(chart): the PV rename hazard gets a tripwire, and a refusal to go with the notice (backend#2626) Builds on 2cd99cd rather than replacing it. That commit fixed both Bugbot findings by correcting the values.yaml conflation and repeating the caveat in NOTES.txt; this adds the two things it left implicit. A REFUSAL, alongside the notice. NOTES.txt and this guard cover different halves and neither substitutes for the other. NOTES.txt renders on every hostPath install including dry-runs, so it is the half that is always visible -- but it prints after a successful render and cannot stop the upgrade that deletes the PVs. The new guard in secrets.yaml blocks that upgrade: it compares the retained claim's spec.volumeName against the volume this render names, and refuses when they disagree. It reads the claim rather than doing a PV lookup because spec.volumeName is written by the binding controller, needs no cluster-scoped RBAC, and cannot come back empty-and-look-renamed the way a forbidden lookup would. Scoped to hostPath because off hostPath the provisioner names volumes pvc-<uuid>, which would make it refuse every storage-class install. It names the observed pair and both causes -- a rename, or hostPath switched on over a dynamically provisioned claim -- rather than asserting which. A TRIPWIRE UNDER THE PROSE. Nothing tested any of this. hostpath_pv_rename_test pins the four facts both comments now assert, from render output rather than from a restatement: PV names follow the override, claim names do not, claims carry helm.sh/resource-policy keep, and the PVs do NOT. That last pair is the tripwire -- annotating the PVs keep is the real fix for this hazard, and when someone does it those cases go red and force the values.yaml paragraph and this guard to be revisited in the same PR. Each fact is asserted with the override both set and unset, since the set-only form would pass on a chart that named everything renamed-* unconditionally. Two things measured along the way. The claim's spec.volumeName is set by no template, so the chart test asserts the binding from the PV's claimRef end and says so; and on helm-unittest 0.5.2 a documentIndex is silently ignored when a suite scopes several templates and a test narrows with template:, evaluating against document 0 while reporting DocumentIndex 0 -- all six claim tests here ran against the PersistentVolume before switching to documentSelector. Also: a scoped regression test for the nil-guard, which had none. It is provable only template-scoped, because a whole-chart render under hostPath: null still dies at shared-images-pvc.yaml:3 either way. And 2cd99cd's new comment said the guard was "like every other hostPath read in this chart" -- measured 8 guarded, 9 bare on this tree, so the sentence is corrected and points at backend#2910. Filed a duplicate of that ticket (#2911) and closed it, moving its three additional measurements onto #2910. helm unittest 654/654 (was 641). make drift 39/39. Every claim mutation-proved: PV gains keep -> 1 red; PV name stops following -> 1 red; claim name starts following -> 3 red; host path starts following -> 1 red; nil-guard reverted to the bare read -> the secrets case reddens with the nil pointer. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(guard): the identity-env class also has to name the right identity (backend#2626) Layered on 7dc86c2's `identity_env_sites`, which landed the same Bugbot Medium concurrently. Both of us reproduced the fail-open and fixed it differently; this keeps that commit's fix as the base and adds the two cases it cannot see, plus the chart-level backstop. WHAT 7dc86c2 ALREADY DOES, kept unchanged: derives the site set from the render with the override UNSET and requires every site to still be there with it set, so a routed site is a DISAPPEARANCE rather than an absence. That is the better half of the two approaches -- it catches a site vanishing for any reason, not only a routed value -- and replacing it with a same-shaped enumeration of my own would have been a discard, not a merge. WHAT IT CANNOT SEE, and both are now checked: * `identity_env_sites` accepts a value in (rel, ns) for ANY of the three names, so swapping RELEASE_NAME with RELEASE_NAMESPACE keys the same triple on both sides and passes -- while a consumer reading RELEASE_NAME gets the namespace. Each name is now checked against its own meaning. Assertable only because this profile deliberately makes rel and ns differ; under the installer's one-string-for-both convention (backend#2621) it would not be. * A `valueFrom` env has no literal to read, so it drops silently out of BOTH site sets and reads as agreement. It is now reported as a site the check cannot see, which is the finding rather than a pass (rule 3). AND PER-PROFILE EMPTINESS IS NOT A CHART FINDING. 7dc86c2 failed the guard when the default render named zero identity envs, which would refuse a complete chart on a profile rendering neither CronJob nor the storage-assertions Job -- the mistake the PATH class already made and had demoted after measuring. Replaced with the reviewed PATHCLASS shape: an `ENVCLASS` count out of the sidecar, asserted once ACROSS profiles by the shell (new assertion 7), where a missing marker exits 2 (could-not-run) instead of 0. Measured at head: aks 5, bm 5, eks 5, oc 5. Also took 7dc86c2's NOTES wording over mine -- same conclusion, tighter -- and added one clause it lacked: that the destructive upgrade is now refused at template time, so the notice tells the operator they are protected rather than only that a hazard exists. Mutation-proved, and the split is the point: routing one RELEASE_NAME -> exit 1 (both layers fire); swapping RELEASE_NAMESPACE -> exit 1 with ONLY the new per-name check firing, the site comparison silent; renaming the ENVCLASS marker -> exit 2 with NOT CHECKED; stripping the envs from all three templates -> exit 1 naming the chart-level cause. helm unittest 656/656. make drift 39/39. shellcheck clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(chart): force egress-enforcement and registry-Secret into the completeness denominator (backend#2626) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(chart): bump to 1.9.92 for the completeness denominator change (backend#2626) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(completeness): demote CLS_ENV emptiness, and pin the refusal detector (backend#2626) Both findings from @saadqbal review, plus the base merge. 1. CLS_ENV EMPTINESS WAS HALF-DEMOTED (Bugbot Medium; he reproduced it). The identity-env block already treated per-profile emptiness as legitimate and printed `ENVCLASS 0` -- but the STAYED empty-class arm set `fail = True` first, so that half could never rescue the run. Reproduced here on aks with autoUpgrade, imageRefresh and sealCheck.storageAssertions.enabled all off: 38 documents, and the ORIGINAL prints "[ERROR] found NO RELEASE_NAME ..." and "ENVCLASS 0" in ONE output. A required drift guard refusing a complete chart. AND THE ONE-LINE FIX WOULD HAVE BROKEN THE CROSS-PROFILE ASSERTION. Extending the demotion is right, but it must NOT also print `ENVCLASS 0` the way CLS_PATH prints `PATHCLASS 0`: the shell parses `grep -E ^ENVCLASS | head -1` and the identity-env block below prints the REAL count unconditionally, so a zero here would be read first and the true value discarded -- turning a cross-profile assertion into one that always sees 0. CLS_PATH can print its count because its two branches are mutually exclusive; CLS_ENV count is owned by the block below. Stated in the code. STILL LATENT, and I am not claiming a test covers it: all four CI profiles name 5 identity envs, so disabling the demotion does NOT redden the suite. The evidence is the live reproduction above, not a guard. Pinning it needs either a fifth profile or a specimen driving assert_stayed empty arm -- happy to add the latter. 2. THE REFUSAL DETECTOR HAD NO SPECIMENS AT ALL, which is the asymmetry that made his mutation possible: `_fallback_on_line` has three pinning lines, `_mitigations` had none. `return {"refusal"}` unconditionally left assertion 5 printing "all 2 routed Secret lookup(s) of 2 carry a mitigation" and exiting 0, both existing selftests green -- the detector deciding whether a routed credential Secret is mitigated could be DELETED and nothing in the tree would say so. Four specimens now, written down rather than hunted for in the chart: a real refusal, a negation that merely branches, a `fail` reached by negating an UNRELATED variable (the routed_vars discrimination), and a `fail` below the 4-line window. Mutations, all reddening: his exact `return {"refusal"}` (3 of 4 halves fire); widening the window 4 -> 40; dropping the per-var discrimination. A fourth came back INERT and its anchor assertion said so rather than reporting a pass. 3. BASE MERGED, both conflicts as he specified. Makefile: develop block plus one line, verified by comparing guard sets programmatically -- 39 on develop, 40 here, the single delta being fullname-override-completeness.sh -- rather than by eye, because `--ours` would delete #933 empty-entry refusal and `--theirs` would drop this guard from the list entirely. manifest.sha256 REGENERATED from the merged tree. Guard at head: rc=0, 60 OK, 0 ERROR, aks=5 bm=5 eks=5 oc=5 -- the 56/0 baseline he measured, plus the four new selftest lines. drift-list-integrity 9/9, gen-manifest --check clean. black is NOT run by this repo CI and the file was already non-black-clean (14 hunks before and after, none in the added code), so it is left alone rather than reformatted into an unrelated diff. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(completeness): the preflight does NOT make 127 unreachable (backend#2626) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <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>
…ulling them (backend#2935) (#948) * fix(chart): pin CronJob memory requests==limits so a node OOM stops culling them (backend#2935) The ap-workspace-auto-upgrade CronJob was exit-137 (SIGKILL) killed on every hourly tick on a live edge, and image-refresh failed in lock-step (six paired failures) — together, which points at ONE shared cause, not two independent container-limit overruns. Diagnosis (measured, not assumed): - Both CronJobs already declare memory limits: auto-upgrade 256Mi, image-refresh 128Mi. - Neither workload's own working set approaches its limit. helm's peak on this chart is ~50Mi (repo update + search + `helm template` render); kubectl's is ~40-80Mi (one `get deployment -o json` + `rollout status`; the curl HEADs return headers only; jq ~2Mi). So the limit was never the binding constraint, and there is no leak or unbounded read (both scripts read bounded JSON and stream through awk). - What they share is QoS: both are Burstable with requests BELOW real usage (128Mi<256Mi, 64Mi<128Mi). A Burstable pod running above its memory request draws a high oom_score_adj, so under NODE memory pressure the kernel OOM-killer targets it first. The telemetry reason is "Error, exit code 137" (not OOMKilled), consistent with a node-level kill rather than a container hitting its own cgroup limit. auto-upgrade at :23 and image-refresh at :22 overlap, so a single pressure event culls both. Fix — the repo's established remedy for exactly this exit-137 signature: pin requests.memory == limits.memory on both CronJobs (auto-upgrade 256Mi, image-refresh 128Mi). This reserves the memory and lowers oom_score_adj (the pods stay Burstable) so the node OOM-killer no longer prefers them — identical to jobs-manager/pods-monitor after their exit-137 mass-restart OOM (#642, backend#1144), the GPU device plugins (#919), and wait-for-mysql (backend#2913). Limits are UNCHANGED — this is not a "move the cliff" bump; the measured footprints show >3x headroom already. CPU stays burstable (a share weight, throttled not killed — backend#2418). Guard: auto_upgrade_test.yaml and image_refresh_test.yaml now assert requests.memory == limits.memory on each CronJob pod, so the gap that made them easy victims cannot silently reopen. Chart version bumped 1.9.91 -> 1.9.92. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(chart): make the two CronJobs Guaranteed QoS, not just memory-pinned (backend#2935) Addresses @saadqbal's review on client#948. The memory-only pin was the wrong mechanism for the claim it carried: a Burstable pod's oom_score_adj is 1000 - 1000*memRequest/nodeCapacity, fixed at container start and independent of usage, so raising the request 128Mi->256Mi moves auto-upgrade ~992->984 on a 16Gi node and image-refresh 999->998 on a large node — it does NOT "stop the OOM-killer preferring the pod". Only Guaranteed QoS (oom_score_adj -997) does that. These two CronJobs are the only chart pods one line from Guaranteed: zero initContainers, memory already equal, cpu the sole blocker. (#642 pinned jobs-manager's memory but could not reach Guaranteed there — it renders an unresourced init container; here we can.) So pin cpu too: - auto-upgrade: cpu request 50m->500m (== limit); memory already 256Mi==256Mi. - image-refresh: cpu request 20m->200m (== limit); memory already 128Mi==128Mi. Limits never moved — only requests rose to meet them. The two levers now do two honest things: the memory RESERVATION reduces the node overcommit that let pressure build (the structural half — #2935's cause), and Guaranteed's -997 keeps the pod off the OOM-killer's shortlist under pressure (the mechanism). Reservation-vs-throttle tradeoff stated in values.yaml; helm/kubectl are I/O-bound so the cpu cap does not slow real work; Forbid + startingDeadlineSeconds bound the Pending case. Guards now assert cpu AND memory request==limit on both pods (the in-manifest proxy for Guaranteed given no initContainers; client#922's checker will assert the class directly). Comments corrected to credit each lever with what it actually delivers. Chart 1.9.92 -> 1.9.93 (develop advanced to 1.9.92 under this branch). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
bugbot run |
tracebloc-release-train
Bot
added
the
gate-nudge
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 526f4ad. Configure here.
…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>
* fix(chart): collect the upgrade path own logs (backend#2935) When a customer edge stops upgrading we could see THAT it failed and never WHY: helm and refresh were not in classAContainers. The agreement guard could not see them either -- it enumerated three kinds and missed CronJob. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(tests): preflight PyYAML in the class-A count heredoc (backend#2935) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(telemetry): pin that the upgrade containers are collected, not just agreed with (backend#2961) @saadqbal removed `- helm` and `- refresh` from `classAContainers` and nothing went red: the agreement guard passed at 4 globs and all 61 telemetry unittests still passed. The fix was removable without one leg noticing. The cause is that the guard checks AGREEMENT -- every glob targets a container the chart really deploys -- which is a different property from COVERAGE. Deriving the expected count from the declaration was right in itself, and it also means the count moves WITH the list, so a shrink is invisible to it by construction. Coverage has to be pinned by name. Two matchRegex assertions in the existing Class A path test, and two more in the partial-map test so a fleet install that sets some Collector keys but not `classAContainers` is covered too. Mutation-proved against the exact removal that motivated this: dropping either name, or both, now fails 2 tests; restored, 657 pass. Chart 1.9.94 -> 1.9.95: #922 also wrote 1.9.94 against a 1.9.93 base, and because both are literally equal git will not conflict and the version guard -- which only asserts `version:` CHANGED -- passes on each. With `strict: false` fleet-wide the second merge never re-evaluates, so both would ship different chart content under one published 1.9.94. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…#2849) (#931) * fix(installer): bound every remaining unbounded wait on the Windows path (backend#2849) An unbounded external call against a SICK dependency does not fail, it BLOCKS — and this journey has now spent four runs learning that the hard way, twice (backend#2675's reboot prompt, backend#2849's empty exit code). Both were "the caller spent its whole budget and reported a timeout that named nothing". Auditing the never-executed Windows path for the same shape turned up three more, all on the route the e2e leg is about to walk for the first time. This file already has the rule — Invoke-BoundedProcess, "installer external-call timeout rule" — so each of these is a site that predates it, not a new policy. 1. THE ENGINE-UP PROBE (Install-DockerDesktop). The wait that exists to survive a bad Docker start read `docker info` natively, so a wedged daemon — a half-open \\.\pipe\docker_engine, which is what Docker Desktop leaves when it starts and gives up — blocked the probe itself. New Test-DockerEngineUp goes through Invoke-DockerCli at 15s. A timeout reads as "not up", never as up: mistaking a wedged daemon for a healthy one would send the install into Step 3 on an engine that is not there. 2. THAT WAIT'S DEADLINE WAS NOT A DEADLINE. `$maxWait = $waitMin * 20` assumed each pass costs exactly its 3s sleep, which stops being true the moment a probe blocks — so the 10-minute cap the code believed it had was an iteration count. Now wall-clock, checked in the loop guard. 3. THE STEP-3 PREFLIGHT READERS (Get-PfRuntimeMemGb/MemMib/Cpu) are the first thing to touch Docker after the engine wait, and had the same bare call. An install that got past Step 2 on a sick engine would hang at the ENTRY to Step 3, before anything printed, instead of falling back to the host reader as their own $null contract promises. 4. THE CLI INSTALLER CHILD had the only `WaitForExit()` in ~7400 lines with no bound. It runs `irm <cli install.ps1> | iex` — a network fetch we then execute — so a stalled handshake or a slow CDN parked the whole install with no output and nothing to kill. 10-minute deadline, then killed; a timeout is non-fatal because a failed CLI install already is (Step 4 falls back to the legacy credential flow). Scope, deliberately: this does NOT try to make Docker Desktop supported on Windows Server. It makes the answer ARRIVE — bounded and named — instead of arriving as a 30-minute silence. The field test (backend#1232) reached a working environment on Windows 11, so Steps 3-6 do work; what the e2e host adds is an unsupported SKU, and one run should be able to say so. The two Get-Pf* tests that mocked `docker` natively now mock Invoke-DockerCli — same assertions, seam moved out one layer. Nothing removed; a timed-out-probe case is added, which was previously unreachable. 958 Pester assertions pass on BOTH 5.5.0 and 6.0.1 (was 948). Part of tracebloc/backend#2849. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(installer): a reboot-pending Docker install (exit 3010) is a success, not a failure (backend#2849) Finding 1 of backend#2849: on the Windows Server 2022 journey host Docker Desktop installed cleanly (full tree on disk, no error log) yet the run aborted. Docker's installer returns 3010 (ERROR_SUCCESS_REBOOT_REQUIRED) whenever the WSL2 backend adds Windows features -- our exact --backend=wsl-2 path, and expected success per Docker's enterprise-deployment docs -- but Invoke-TrackedInstall judged success as `$p.ExitCode -eq 0`, so a completed install was misfiled 'failed'. That is the client#611 idiom one layer up. Add an opt-in -SuccessExitCodes to the shared Invoke-TrackedInstall (default @(0)) and pass the documented reboot-pending SUCCESS codes at the three installer call sites: Win32 3010/1641 (a direct installer, e.g. Docker) and winget 0x8A150109/0x8A15010B as Int32 (-1978334967/-1978334965). winget's 0x8A15010A (REBOOT_REQUIRED_FOR_INSTALL, a real failure) is deliberately excluded. The real code is preserved in the return so a reboot-pending 'ok' stays visible in the log. Follows #913 (finding 2, the empty exit-code slot), which made the code readable in the first place. The WSL-update helper keeps its bare -eq 0 on purpose: its symptom was the null code (fixed by #913), it is non-fatal, and a reboot code there is speculative. Tests: reboot codes accepted end-to-end (incl. the negative winget HRESULTs), the winget failure reboot code stays 'failed', the default set stays @(0) so non-installer callers don't start tolerating 3010, and both Docker paths opt in. Full Pester suite 801/0/15; PSScriptAnalyzer 0 errors on install-k8s.ps1; scripts/manifest.sha256 regenerated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(installer): distinguish reboot-INITIATED from reboot-required in the Docker install log (backend#2849) Review follow-up (shujaatTracebloc): the post-install log line claimed "continuing to bring up the engine" for BOTH reboot-pending families, but 1641 / winget 0x8A15010B (INSTALL_REBOOT_INITIATED) mean the installer has already started restarting the machine -- so on those codes that line is the last thing written before the box goes down, reading as a script that carried on when it didn't. Given this PR exists because a log said the wrong thing, the message should not. Add $script:INSTALLER_REBOOT_INITIATED_CODES = @(1641, -1978334965) (a subset of the OK codes) and branch the Docker log: an INITIATED code says the machine is restarting and the install resumes via the reboot handoff; a merely-required reboot (3010 / 0x8A150109, box still up) keeps "continuing to bring up the engine". Behavior is unchanged -- both remain 'ok' and press on; only the log text differs. Test: the INITIATED subset is exactly @(1641, -1978334965), is a subset of the OK codes, excludes 3010/0x8A150109, and the Docker handler branches on it. Pester 802/0/15; PSScriptAnalyzer 0 errors; scripts/manifest.sha256 regenerated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(installer): an initiated reboot arms a fresh resume + stops, on both Docker paths (backend#2849) Bugbot (Medium): the reboot-INITIATED log line claimed "resumes via the reboot handoff", but Register-ResumeAfterReboot's RunOnce is armed only in Step 1 and is already spent by the Step-2 Docker install — so no handoff was actually armed. The winget path (tried first, the one that can return the winget HRESULT) never logged or stopped either, so an initiated reboot could drop the box mid-install and leave the run looking interrupted / the engine wait Err-ing. Replace the inline log with Invoke-PostInstallReboot, called from BOTH Docker install paths so the handling can't depend on which ran: - REQUIRED (3010 / winget 0x8A150109, box still up): log and continue, as before. - INITIATED (1641 / winget 0x8A15010B, already restarting): arm a FRESH resume-after-reboot and stop with the declared exit 2 — the same handoff Step 1 uses — so the install genuinely resumes instead of racing the reboot. The message no longer promises a handoff that isn't armed. Our flags never allow a reboot (--quiet; no winget --allow-reboot), so INITIATED stays the unexpected-but-safe branch; this makes it correct rather than merely quiet. Tests: the initiated branch arms a resume + exit 2 and both paths route through the handler (source, since exit 2 would end Pester); the REQUIRED/no-op branches never arm or exit (behavioral, Should -Invoke ... -Times 0). Pester 804/0/15; PSScriptAnalyzer 0 errors; manifest regenerated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(installer): the bootstrap must not close the user's window (#577, backend#2849) Reported from a real Windows machine testing from develop: "it just closed the PowerShell". #577 -- "the PowerShell installer must never terminate ungracefully, always show the user a clean what-happened" -- was fixed in install-k8s.ps1, which runs as a CHILD process where `exit` costs nothing. install.ps1 was missed, and it is the one that runs INSIDE the user's console via the documented `irm ... | iex`. A top-level `exit` there ends THEIR session: the window closes and takes the outcome with it, which is exactly the symptom #577 closed. Not only a failure path. `exit $LASTEXITCODE` after the child returns fires on EVERY run, so a perfectly successful install also slammed the window shut over its own summary. All three exit sites are affected (the child's code, the platform gate, the top-level catch). The exit CODE still propagates untouched. Swapping `exit` for `return` would have been the obvious fix and is wrong: `powershell.exe -Command` callers would then read 0 for every outcome, and the e2e harness reads this code to tell install-k8s.ps1's declared `exit 2` reboot handoff from a real failure -- a failed install would book as a pass. So Complete-Bootstrap holds the window open long enough to be read, then exits exactly as before. The hold is BOUNDED (60s) and gated on the same Test-CanPrompt predicate install-k8s.ps1 uses, so CI, a service and any piped/redirected stdin take the no-hold path -- an unbounded hold is the very class this ticket has spent its life removing. A host that cannot report keystrokes (ISE, a redirected console) throws on KeyAvailable and is caught. 977 Pester assertions pass on BOTH 5.5.0 and 6.0.1. Part of tracebloc/backend#2849. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(installer): a branch ref may contain '/', because every branch does (client#917) Measured on a real Windows box, testing this very branch: Installation stopped: Ref 'fix/2849-bound-the-docker-probes' contains a path separator or '..' -- refusing to build a fetch URL from it. The belt-and-suspenders guard refused '/' outright, which broke the ONLY flow it exists for. Every real development branch is `fix/1234-thing` or `feat/...`, so the documented developer override could fetch `develop`, `staging` and `main` and nothing else -- while the whole point of the escape hatch is testing UNRELEASED code, which lives on feature branches. '..' is the actual traversal lever and is still refused on EVERY path, opt-in or not. The R8 property never rested on '/': a TAG still cannot carry one, because the vX.Y.Z shape check rejects it -- so the case that comment names, 'v1.2.3-../../heads/main', is refused twice over. A '/' is accepted only on the path that has already announced itself as an unverified branch install and printed the four-line warning. Multi-segment refs are held to exactly the shape a single segment is: validated segment by segment, so a leading, trailing or doubled slash (an empty segment) and a bare '.' are all refused. 985 Pester assertions pass on BOTH 5.5.0 and 6.0.1. Part of tracebloc/backend#2849. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(installer): bound the docker CLASS, drain the console, stop the off-Windows stall Addresses @LukasWodka's change-request and @aptracebloc's independent review on client#917, plus both open Bugbot findings. Each fix is mutation-proven below. 1. THE GUARD DEFENDED ONE VERB, THE TITLE CLAIMED THE CLASS (blocking) `Should -Not -Match '\(docker info'` has an input domain of one needle, so a new unbounded native call walked past it -- Lukas injected `docker ps -a` and watched all 38 tests stay green. Seven native calls were live at head, and not in a corner: `Set-ClusterAutostart` (MAIN install path), three cluster-REUSE inspects, and the two `-Diagnose` calls, i.e. the bundle a user collects BECAUSE Docker is wedged -- bare there hangs the one tool meant to explain the hang. All seven now go through `Invoke-DockerCli` with a 20s bound. Every timeout degrades in the safe direction: autostart logs and skips (k3d already sets unless-stopped), the advisories stay silent rather than inverting into a false warning, and the dataset-mount precondition is SKIPPED rather than fired -- inferring "no mount" from a wedged daemon would abort a correct cluster. The guard is now the class, asserted on the PowerShell AST rather than text: no native `docker` invocation is unbounded, where bounded means `Invoke-DockerCli` or a `Start-Job` reaped by `Wait-JobWithProgress`. AST is load-bearing -- ~10 Log/return strings in this file contain "docker run" / "docker build", so every text version either drowns in false positives or gets narrowed back into an instance guard. A second test requires the job sites actually be reaped on a deadline, closing "bounded-looking unbounded". Both fail closed on a parse error and refuse to pass vacuously. 2. A STALE KEYSTROKE COLLAPSED THE HOLD TO ZERO (install.ps1) `[Console]::KeyAvailable` reports what is QUEUED, not a press since the hold began, and nothing drained it. install-k8s.ps1 runs as a child on the same console with 11 Read-Host sites, so one extra Enter -- or the trailing newline of `irm | iex` -- made the wait false on its first evaluation and closed the window over the summary: the exact #577 failure this function exists to stop, under a line that had just promised to wait. Drained before the deadline wait, inside the same try. The test pins the ORDER, since draining after the wait would eat the user's real keypress and fix nothing. 3. THE WRONG-PLATFORM BAIL STALLED 60s WHERE NO WINDOW EXISTS `[Environment]::UserInteractive` is hardcoded $true on non-Windows .NET, so the predicate reduced to "stdin not redirected" and an instant, zero-cost bail-out held for a minute -- on the one branch that only runs off Windows, where `exit` from a child pwsh closes nothing. Now a bare `exit 1`. The `$bare.Count | Should -Be 1` assertion was a count standing in for a rule, which made this fix look like a regression. Replaced with the actual property, derived: no `exit` outside Complete-Bootstrap or the platform gate. 4. THE ELAPSED LABEL WAS THE LEAST HONEST THING ON SCREEN (Bugbot, both reviews) The deadline became wall-clock; the label still divided the iteration counter by 20. With the probe capped at 15s a wedged pass costs ~18s, so the loop exited at ~10 REAL minutes still reading "1 min elapsed", immediately before "didn't come up within 10 minutes". Derived from `$dockerStart`, the same clock as the deadline. `$maxWait` is gone entirely -- with the bound expressed as a deadline it had no job left, and keeping it invites the slip back in. 5. THE PROMPT PREDICATE WAS A COPY CHECKED AGAINST A COPY The old assertion hand-wrote the rule and tested install.ps1 only, so if Test-CanPrompt gained a condition the two would diverge silently and it would stay green. Now parsed out of BOTH files and required to agree, failing closed if either is unreadable. The doc comment no longer asserts the e2e journey takes the no-hold path -- an interactive desktop session satisfies both conjuncts, and that assumption was already measured false once on this journey. Correctness rests on the bound, not the predicate. Also: the two bare `Should -Throw` now pin `-ExpectedMessage` (the tag-shaped traversal case was satisfied by any of three refusals), and Set-ClusterAutostart's tests move their seam to `Invoke-DockerCli` -- same assertions, plus timeout and blank-line cases that were unreachable while the calls were bare. The Helm winget gap Lukas raised is fixed in develop: #921's merged version carried it, and develop's own invariant test (accepts == routes) now covers it. MUTATION-PROVEN, each reverted in isolation: - Lukas's injected `docker ps -a` -> both class guards redden (was green) - unbound the -Diagnose `docker ps -a` -> both class guards redden - remove the input-buffer drain -> drain-order test reddens - platform gate back to Complete-Bootstrap -> gate test reddens - diverge the two prompt predicates -> drift test reddens (was green) - elapsed label back to `$i / 20` -> same-clock test reddens 889 Pester assertions pass on BOTH 5.5.0 and 6.0.1, 0 failures. Both .ps1 parse clean, check-style.sh clean, manifest regenerated. Part of tracebloc/backend#2849. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(installer): the dashboard link follows CLIENT_ENV (backend#2849) Reported from a real dev install on Windows: a `CLIENT_ENV=dev` run told the operator to create a client and fetch credentials at https://ai.tracebloc.io/clients -- the PRODUCTION dashboard -- and credentials from there are then rejected by dev-api. The operator is sent to the wrong place at exactly the moment they need the right one. Hardcoded at THIRTEEN sites, while Get-BackendUrl sitting right above them was correctly env-aware. Both installers had it; this is the PowerShell half. The hosts are the BACKEND'S OWN per-environment settings, not a guess -- DEVICE_VERIFICATION_URI / RESET_PASSWORD_URL in xraybackend/settings/{dev,stg,prod}.py: dev -> https://dev.tracebloc.io stg -> https://stg.tracebloc.io prod -> https://ai.tracebloc.io Get-TraceblocDashboardUrl uses the SAME vocabulary and the same unknown->prod fallback as Get-BackendUrl, so the two can never disagree about which environment an install belongs to -- and a test pairs them per environment rather than asserting each alone, because the defect WAS precisely those two disagreeing. The guard that keeps it fixed: each of the three hosts must appear exactly once in the file (as its switch arm), and no live link may carry a path -- a hardcoded link always has one (/clients, /my-use-cases), while the bare host is only ever the mapping. Caught in review of my own change: the bare-host substitution rewrote the helper's OWN default arm into a call to itself, so the suite hung on infinite recursion instead of failing. Fixed, and the "appears exactly once" guard would now catch that shape too. 1019 Pester assertions pass on BOTH 5.5.0 and 6.0.1 (was 1013). Part of tracebloc/backend#2849. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(installer): bind each docker job's reap to THAT job, not to its function @LukasWodka's follow-up on the reap guard, and he proved it rather than asserted it: the check matched `Wait-JobWithProgress -Job $x -TimeoutSec N` anywhere in the enclosing FUNCTION, so one compliant job vouched for its neighbours. He added a second, unreaped docker job to a function that already had a good one and watched all 41 tests stay green. The guard now walks from each native docker call out to its `Start-Job`, finds the variable that job is assigned to, and requires a reap naming THAT variable. A job with no assignment at all fails too -- nothing can reap what nothing names. Mutation-proven with his exact probe (an unreaped second job inside Test-ExistingClusterKubeletConfig, which already reaps $job correctly): [-] the Start-Job docker sites are actually reaped on a deadline, not merely in a job Test-ExistingClusterKubeletConfig starts docker job 'sneaky' but never reaps THAT job on a deadline Was Passed=41 Failed=0. The failure now names the offending job variable. 902 Pester assertions pass on both 5.5.0 and 6.0.1, 0 failures. check-style.sh clean, manifest regenerated. Part of tracebloc/backend#2849. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(installer): prove every guard still catches its own bug (backend#2849) A fix is not fixed until a guard has been SEEN to fail. Both of this week's Windows defects were "already fixed", and nothing in CI could say otherwise: * #577 shipped a graceful-exit boundary and PR #588 DID touch install.ps1 -- but only its message. Every `exit` was untouched, so the bootstrap kept closing the user's console for another month. Reported from a real machine as "it just closed the PowerShell". * install-k8s.ps1's own comment promised WaitForExit() "guarantees ExitCode is populated for every caller". It did not, so every failure in Invoke-TrackedInstall rendered `exited ` with an empty slot -- and a Docker install that SUCCEEDED with 3010 was filed as a failure. Both were found by a human running the installer. The structural reason nothing else could: most installer guards read a property off the SOURCE TEXT, and such a guard passes forever once the string drifts -- "green" and "no longer looking" are the same colour. And every exit path sits behind `if (-not $env:TB_PESTER)` while every suite sets TB_PESTER=1, so NO test in this repo has ever executed an `exit`. mutation-check.ps1 reintroduces each fixed defect into a COPY of the tree and requires the claiming suite to go red. 11 registered, 11 caught. `-Dry` resolves markers only and says in as many words that it is not evidence anything bites. Four things it refuses to do, each because it has gone wrong somewhere: never mutates the working tree (nothing to restore, nothing a SIGKILL strands); never trusts a marker matching more than one line (a duplicated line becomes a loud STALE, and an `After` anchor disambiguates the two byte-identical Test-CanPrompt guards); never reports a catch without a green baseline; never lets a mutation be a no-op. A FRESH PROCESS PER RUN, found the hard way: the first version ran every suite in one pwsh session and reported two baseline failures that do not reproduce when the file is run alone -- Pester state survives between Invoke-Pester calls, so run N is not run 1. A harness whose verdicts depend on how many times it has already run cannot say whether a guard bit. It also now NAMES a failing baseline instead of printing a count, which is the same diagnostic gap these tickets are about. Wired into installer-tests.yaml, and .cursor/BUGBOT.md gains the six review rules this week produced -- fix-in-one-twin-only, guards behind TB_PESTER, comments asserting guarantees, source-text assertions with no mutation, numbers changed without their consumers, and preferring a pure function to a source grep. Part of tracebloc/backend#2849. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(installer): cover the CLI wait's handle and flush, which nothing did (Bugbot on #931) Bugbot: the registry tracked the handle-cache / empty-ExitCode class only on Wait-ProcessWithDeadline, and the CLI wait only as a missing deadline -- so Install-TraceblocCli's own `$null = $p.Handle` and parameterless `WaitForExit()` flush were source-text assertions with nothing behind them. Correct, and the hole is exactly where this class keeps recurring: Bugbot found the missing flush on #917, INSIDE the fix for the same class. Worse than reported. The existing fakes give WaitForExit a no-arg ScriptMethod returning NOTHING, so `if ($p.WaitForExit($cliWaitMs))` is falsy and every one of them takes the TIMEOUT branch. "reports success only when the installer exits 0" was passing through the kill path -- the right verdict for the wrong reason -- and the success branch, where the flush lives, was never executed by any test. So the new fake models the REAL contract: WaitForExit(ms) returns $true, and ExitCode stays $null until the PARAMETERLESS overload has drained the streams. Two directions, so neither can pass by accident: with the flush present the install reports success and the flush is asserted to have happened; with ExitCode never readable it must WARN rather than credit an install whose verdict it cannot read (the client#611 shape -- Step 4 warns, Step 5 falls back to the legacy credential prompt). Both lines now carry registered mutations, disambiguated by `After` anchors since neither is unique in the file. 13/13 mutations caught. 1027 Pester assertions pass on BOTH 5.5.0 and 6.0.1. Part of tracebloc/backend#2849. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(installer): the harness must catch via the CLAIMING guard, not any failure (@saadqbal on #931) Asad was right, and the finding is the one this PR exists to prevent, inside this PR: the harness could not fail for 11 of its 13 entries. The sandbox copied only `scripts/`, but install-k8s.Tests.ps1 reads six paths above it (../../docker/k3s-cuda/*, ../../client/templates/resource-monitor-daemonset.yaml). Unmutated in a scripts-only sandbox that suite is FAIL=7 -- I reproduced it -- and the predicate was `if ($fails -gt 0) { $caught++ }`, so every install-k8s.ps1 entry cleared on inherited failures alone. My own output said so and I did not read it: install-k8s entries all reported >=8 failing while the two install.ps1 entries reported 2 and 3. Asad proved the end state -- dashboard guard made vacuous AND the link reintroduced, run still 13/13. Five fixes, his order: 1. THE SANDBOX IS THE WHOLE TREE (minus .git, which is 106MB of 122 and which no suite reads). Both baselines are now 0 in the sandbox, not just in the repo. 2. ATTRIBUTION, which he called the one that matters. Each entry names the guard it EXPECTS to fail, only that guard's failure counts, and a mutation that reddens the suite via some OTHER test is MISATTRIBUTED -- reported as a registry failure, as loud as SURVIVED, never a catch. An entry with no `Expect` is refused at marker resolution, because without one it could only be scored on a bare count. Every catch now prints WHICH test bit. It found a real one immediately: removing the CLI handle cache is a semantic NO-OP under the mocks (`$p.StartTime` on a pscustomobject returns $null silently), so no behavioural test could see it and the mutation was reddening the suite through two unrelated flaky tests. Added the source-level guard the Wait-ProcessWithDeadline site already has, for the same reason: handle reaping is .NET behaviour no fake can reproduce. 3. `After` IS A SCOPE, NOT A LOWER BOUND. Uniqueness is enforced with an anchor too (it was skipped entirely), and the search is bounded to a `Within` window -- otherwise `After 'function Read-RebootChoice {'` still saw Read-ClientName's byte-identical guard 3900 lines later. That stricter check caught this the moment it was added. 4. THE CHILD'S STDERR IS KEPT. `2>$null` discarded why a run produced no RESULT line, and under $ErrorActionPreference='Stop' it killed the harness on Asad's machine with fourteen bytes of red and no cause. 5. Pester pinned to the 5 major; `-MinimumVersion` took the newest installed and 6 is API-incompatible. Still open, and I could not settle it either: whether mutation-check is a REQUIRED check. The protection API 404s on my token too, so that is an absence rather than a pass -- needs someone with admin scope. 13/13 caught BY the claiming guard. 1028 Pester assertions pass on both 5.5.0 and 6.0.1. Part of tracebloc/backend#2849. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(installer): a crashed child is not a survived mutation, and the Pester pins agree (Bugbot on #931) Two findings, both mine, both the same class this harness exists to close. 1. A CRASHED CHILD WAS SCORED "SURVIVED". Invoke-Suite returns FailedCount -1 when the child dies before printing RESULT, and -1 is not `-gt 0`, so it fell through to SURVIVED -- reporting "this guard does not bite" for a run that never reached the guard, and discarding the very stderr the previous commit added to capture it. Now its own third outcome, printed with the cause and exiting non-zero: a crash means the entry proved NOTHING either way, and scoring it as anything softer would be the harness lying again. Proved both halves rather than reading the branch. A child that throws before RESULT during the BASELINE now reports the actual exception text instead of a bare count. Injected a sandbox-only death (baseline left green) and the per-mutation path reports CRASHED with its cause, where it previously said SURVIVED. 2. THE INSTALL AND THE IMPORT PINS DISAGREED. The workflow installed Pester with only a minimum, so the gallery hands the runner 6.x, while every suite child imports with a 5.x ceiling -- the child then cannot load Pester, prints no RESULT, and the job dies at the baseline having proven nothing. @saadqbal measured exactly that on his machine, twice. Both now carry the same ceiling, and the comment says they move together or not at all. @saadqbal's earlier items were already in ff7d479 (whole-tree sandbox, per-entry attribution, `After` as a bounded scope, kept stderr) -- his latest pass predates that push. His independent reproduction matches mine exactly: scripts-only sandbox is FAIL=7 for install-k8s.Tests.ps1 and FAIL=0 for install.Tests.ps1, and the control/treatment pair on the dashboard entry showed `caught` printed with the guard absent from the failure list. Still open, and NOT fixable from here: whether mutation-check is a REQUIRED check. The protection API 404s on both our tokens. 13/13 caught by the claiming guard, no SURVIVED/MISATTRIBUTED/CRASHED. 1064 Pester assertions pass. Part of tracebloc/backend#2849. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(mutation-check): say what the harness does NOT yet guarantee (@aptracebloc on #931) Arturo settled the question @saadqbal and I both 404'd on: `mutation-check` is NOT a required status check on `develop`. The required set is Unit tests, Lint, quality/*, version-bump-gate, Source-of-truth drift, chart-version bump, Helm unit tests. So a guard-death reddens THIS job and still passes the gate the merge button reads -- and the header claimed "every guard must still catch the bug it was written for" without that qualifier. A file overstating its own coverage is precisely the class this harness exists to catch, so it should not be the first offender. The header now says it plainly: read the claim as "verified by CI-as-run", not "by CI-as-enforced", until the context is added. Adding it is not possible from a PR. Protection is not managed as code here -- I checked `client` for a settings/ruleset file and `.github`'s `repo-inventory.yml`, which carries only visibility and release_train for this repo -- so the required set lives in the API/UI and needs admin scope. The alternative, folding the tier into the required `Unit tests` (standard-checks.yml:149), is left to Arturo's call rather than taken: it gates by construction but roughly triples a required check every PR pays, including ones touching no installer code. And the obvious escape from that cost -- a `paths:` filter on a required context -- is a known trap: a skipped required check never reports, which is registered as a mutation in e2e-test-agent because it has bitten before. Comment-only; 13/13 markers still resolve, style clean. Part of tracebloc/backend#2849. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(installer): measure the floor where the mutations run (@saadqbal on #931) Asad measured it rather than argued it, and he was right again. The whole-tree copy fixed seven drift guards and never touched a second floor: the sandbox lived under GetTempPath(), and Get-ElevationCommand branches on `$ScriptPath -notlike "$temp*"` (install-k8s.ps1:81) -- so two tests fail there that pass at the repo root. Reproduced: 890/FAIL=0 at the root, 888/FAIL=2 under temp, and the two are exactly `Get-ResumeCommand … durable script path` and `Get-ElevationCommand … QUOTED -File path`. I HAD SEEN THOSE TWO FAIL AND CALLED THEM FLAKY. They are deterministic, and the cause was the harness's own choice of location. Writing them off as flakiness is the same rationalising that let the first floor through, and attribution only MASKED this one -- when the claiming guard does fail, it is credited regardless of the two inherited failures. Asad's probe proves the false green was still constructible: a comment-only reword with `Expect = 'durable script path'` printed `caught / 1/1 / exit 0`. It now prints SURVIVED / 0/1, which is the point. The structural cause was in plain sight in my own script: the baseline ran `-Root $RepoRoot` while mutations ran `-Root $sandbox`. A floor measured in a different tree than the mutations run in is not the floor. Fixes: 1. ONE `New-Sandbox`, used by the baseline AND every mutation, and it lives beside the repo rather than in temp. The baseline now reports "green in the sandbox" -- 890 and 57, FAIL=0 where the mutations actually run. 2. A LOCAL `ErrorActionPreference = 'Continue'` around the child call. Under the file-level `Stop`, the first stderr ErrorRecord kills the parent before `if (-not $line)` can quote the cause -- so the kept-stderr fix was dead code, and so was the CRASHED branch added for Bugbot's finding, in exactly the case Bugbot described. One scope change fixes both. (Worth noting: on pwsh 7.6 here `2>&1` yields strings rather than ErrorRecords, so I could not reproduce the parent dying; the local Continue makes it robust either way rather than depending on that.) 3. The `by:` line prints the full path. `-replace '^.*?\.'` cut at the FIRST period, mangling the five-plus Describes that contain one -- on the harness's main human-readable output. 4. Refuse below pwsh 7.3, which strips the child's double quotes: CI is on 7.4+ so only a local run is affected, which is the worst shape for it. 5. The `Pester` job is pinned to the 5 major, and 6.x moved to a NEW `Pester next-major` context. Unpinned, it had drifted onto 6.x while the mutation tier certifies under 5.x -- and BUGBOT.md rule 6, which this PR adds, exists because guards bind differently across those majors. A new context rather than a matrix dimension: renaming `Pester (<os>)` would break it if it is required, and branch protection is unreadable from any of our three tokens. 13/13 caught by the claiming guard, baseline FAIL=0 in the sandbox, style clean, actionlint clean. Part of tracebloc/backend#2849. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Arturo Peroni <arturo@tracebloc.io>
* feat(199): ship deny-by-default training egress (RFC-0003 D6) Flip the chart defaults so new/upgraded installs are locked down out of the box: egressProxy.routeWorkloads=true + networkPolicy.training.allowExternalHttps =false. Training egress now flows through the squid allowlist and the direct 0.0.0.0/0:443 rule is dropped by default; set either back to opt a fleet out. Bumps chart 1.9.90 -> 1.9.91. Updates helm-unittest suites (egress_proxy, egress_enforcement_check) and the e2e-auto-upgrade path-2 assertions to the new default (path-1 --reuse-values replays old computed values, unaffected). SECURITY §8.2 status + changelog updated. All 643 helm-unittest cases pass. Enforcement still depends on a policy-enforcing CNI and the §8.2 per-fleet gated rollout; non-enforcing CNIs render the rule without blocking. Part of client-runtime#199 · epic tracebloc/backend#1151 * docs(security): correct stale §5.1 CNI note — both EKS fleets now enforce The 2026-06 note said tb-client-dev-templates ran a self-managed VPC CNI with NetworkPolicy disabled (lockdown cosmetic). Both tracebloc EKS fleets (tb-client-dev-templates dev/staging, tracebloc-clients-prod prod) have since moved to the managed vpc-cni add-on with enableNetworkPolicy=true, verified via aws eks describe-addon. Egress NetworkPolicy is now enforced on both, so the D6 deny-by-default lockdown is real there, not cosmetic. Part of tracebloc/client-runtime#199 * test(199): derive e2e-auto-upgrade path-1 egress asserts from the baseline Path 1 hardcoded the permissive-baseline posture (external 443 present, no EGRESS_PROXY_URL). Once this chart is the published baseline, --reuse-values replays deny-by-default and those asserts would false-fail. Capture the baseline egress posture at install (like BASELINE_PROD_DIGEST) and assert --reuse-values replays it verbatim, correct in both eras. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(199): record the egress-enforcement seal-check probe in the QoS goldens Flipping allowExternalHttps=false to the chart DEFAULT makes the egress-enforcement-check hook render by default, so its training-labelled probe pod (t-egress-enforcement-check) now appears in every default render. Record it: Burstable in the pod-qos class goldens (hostpath/csi/gpu-*), and blocked/probe in the qos-reachability golden (a probe Job, never Guaranteed- reachable). Bump chart 1.9.94 -> 1.9.95 (develop moved to 1.9.94). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(199): restore opt-out coverage + fix SECURITY.md shipping version Three items from review: - SECURITY.md: the lockdown ships in 1.9.95, not 1.9.91 (1.9.91-1.9.94 released permissive); correct A2/8.2/changelog and the < boundary (Arturo). - egress_enforcement_check_test.yaml: add the assertion that the seal-check hook does NOT render when an operator opts back out (allowExternalHttps=true) — the coverage the inverted default-render case dropped (Bugbot). - e2e-auto-upgrade.sh path 3/4: set allowExternalHttps=true (the opt-out, opposite of the new default) so path 4 genuinely tests that --reset-then-reuse-values preserves an operator opt-out instead of the next hourly auto-upgrade silently re-locking the fleet (Bugbot). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(199): flip values.schema.json defaults + correct stale deny-by-default comments Addresses Asad review: - values.schema.json: allowExternalHttps.default true->false, routeWorkloads .default false->true (were inverted vs values.yaml; helm ignores schema defaults but it is the same two-places drift the gate flagged), and reword the descriptions for deny-by-default. - network-policy-training.yaml: the shipped default is now false; the dig TRUE is the legacy absent-key fallback only. - network_policy_test.yaml: true is the opt-out now, not the default; both egressProxy.enabled and routeWorkloads default true. - values.yaml: routeWorkloads no longer ships inert. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(199): re-bump chart 1.9.95 -> 1.9.96 after develop collision develop advanced to 1.9.95 (backend#951), colliding with this branch. Bump to 1.9.96 (Chart version+appVersion) and sync the SECURITY.md shipping version refs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(199): opt out of routeWorkloads too, so path 4 covers it (Bugbot) The prior opt-out fix inverted only allowExternalHttps and dropped the EGRESS_PROXY_URL assertion, leaving routeWorkloads at the new default and untested — an auto-upgrade that dropped or forced it would stay green. Path 3 now also sets routeWorkloads=false (a genuine override, opposite the default) and asserts EGRESS_PROXY_URL is absent; path 4 asserts that opt-out survives the auto-upgrade alongside allowExternalHttps=true and the canary. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(199): correct runbook + comments for deny-by-default renders on install Review follow-ups (Asad + Arturo): - SEAL-CHECK.md: the egress-enforcement caveat + EKS/AKS table cells said the hook only renders after a per-fleet flip; on 1.9.96 it renders by default, so a fresh install is sealed and helm test runs it. Reword for deny-by-default (opt-out is allowExternalHttps=true). - jobs-manager-deployment.yaml: routeWorkloads defaults true now, not inert. - egress_proxy_test.yaml: fix the inverted test title (Arturo) — the case renders the 443 rule AND keeps the gateway path when opted back to true. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(199): update SEAL-CHECK does-not-render example for deny-by-default The example of a check that does not render used "the egress-enforcement probe before the lockdown is flipped" — but it renders by default on 1.9.96. Use the current accurate example: it un-renders when an operator opts a fleet back out with allowExternalHttps=true (Asad nit). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Syed Saqlain <syedsaqlain@MacBook-Pro.local> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
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 c23646b. Configure here.
tracebloc-release-train
Bot
added
gate-nudge
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Automated promotion by the release train (RFC-0008 D14). Head is the train-managed
release-train/to-stagingbranch (a mirror ofdevelop), so it never collides with a human PR. Merged only when the fr-gate is green.Note
High Risk
Changes affect live-upgrade safety (credential/PV refusal gates), default network egress (locked-down training unless opted out), and auto-upgrade rollback logic—any regression impacts fleet security, data access, or upgrade automation.
Overview
Release train promotion bundles chart 1.9.92 → 1.9.96 with installer/CI hardening, Helm naming safety, deny-by-default egress, and several production-adjacent fixes.
fullnameOverride(backend#2626) addstracebloc.fullnameand routes created resource names through it while keeping Helm release identity, instance labels, and on-disk paths on.Release.Name. Live-cluster template-time refusals insecrets.yamlblock credential re-mint when retained MySQL data exists without a matching Secret, and block hostPath PV name drift when a bound PV no longer matches the rendered name. NOTES.txt, values/schema docs, drift guardfullname-override-completeness.sh, andhostpath_pv_rename_test.yamldocument and lock the PVC/PV asymmetry. Installers now resolve secrets using the override prefix; bash/PowerShell workload name lists still ignore override (backend#2888).RFC-0003 D6 deny-by-default egress: defaults flip to
networkPolicy.training.allowExternalHttps: falseandegressProxy.routeWorkloads: true, with helm tests and docs (SECURITY, SEAL-CHECK) updated for opt-out behavior.Auto-upgrade / ops:
pending_age_secondsgains RFC3339 range and year/offset validation (#2908) so bogus timestamps cannot force wedge rollbacks; Guaranteed QoS on auto-upgrade and image-refresh CronJobs (#2935). Telemetry collector Class A lists addhelmandrefresh(#2961).CI / quality: Installer workflow pins Pester 5.x, adds a Pester 6.x job and
mutation-check; Makefile drift suite adds control-plane footprint, fullname override completeness, and QoS reachability guards. BUGBOT.md adds review rules for mutation testing and cross-platform installer twins.Docs / misc:
MIGRATIONS.mdOption C now requires backing up the credentials Secret before uninstall; extensive comment corrections on training-job QoS claims (#2872) across values, schema, and templates.Reviewed by Cursor Bugbot for commit c23646b. Bugbot is set up for automated code reviews on this repo. Configure here.