Uh oh!
There was an error while loading. Please reload this page.
release-train: develop -> staging - #815
Merged
Merged
Conversation
…ocess (backend#2350) (#811) `E2E auth-proxy (squid)` failed 2 of the last 30 develop runs, both times on the same named assertion: ✖ App pod WITH the ingestion proxy env did NOT tunnel through the squid The probe already carried retries meant to cover exactly the cluster-DNS startup window, under a comment asserting that `--retry-all-errors` covered the "Could not resolve proxy" case. It does not. curl caches a FAILED name resolution for the life of the process, so a single curl's `--retry` re-uses the failure instead of re-querying the resolver. Both failing runs show it — nine attempts, eight of them answered from the cache: * Could not resolve proxy: tb-egress-squid.default.svc.cluster.local * Negative DNS entry curl: (5) Could not resolve proxy: tb-egress-squid... So exactly ONE resolver query was ever issued, about a second after the pod started, and the guard turned entirely on whether CoreDNS happened to be serving at that instant. Confirmed A/B in curlimages/curl:latest — the image the pod runs — with the proxy name made resolvable 4s into the run: the one-process form failed all 9 attempts on the stale negative entry, while the fresh-process loop re-resolved on the very next attempt. 8.20.0 behaves the same, so this was never a floating-tag regression; the claim was wrong from the start. The probe now loops fresh curl PROCESSES, which cannot inherit the poisoned cache, and reports attempt number, exit code and elapsed seconds so a future red says whether it waited — the old failure could not distinguish "did not tunnel" from "had not tunnelled yet". Not a blind retry: only exit 5/6/7 (unresolvable proxy, unresolvable host, refused connection) are retried. Every other outcome, success included, ends the probe, so a real #119 regression — proxy env ignored, so the call dials direct and succeeds with no CONNECT — still fails on attempt 1 rather than being retried into a slow green. A squid that is genuinely down exhausts the deadline and fails. The probe is emitted from e2e-common.sh rather than written inline in the pod manifest so the new bats file executes that same text (backend#1729 rule 9), and it runs in `Unit tests`, which is required on develop. Six tests, each mutation-proved with the anchor asserted: reverting to the in-process retry, retrying every exit code, dropping the fail-closed host guard, removing the give-up report and drifting the manifest indent each redden the specific test named for them. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…e scratch mounts tmpfs (backend#2223) (#812) Two of backend#2223 remaining items, both in this repo. 1. THE GRAMMAR FIX IS HERE, NOT IN client-runtime. The ticket blames "client-runtime/jobs_manager.py:2014" for admitting only cpu= and memory=. That is not true of that repo: its parser is a bare `key, value = pair.split("=")` with NO allow-list, and it already accepted ephemeral-storage. The restriction was always THIS file (client/values.schema.json) -- so ephemeral-storage was unexpressible via the chart even though the runtime would have honoured it. before ^(cpu=\\S+,memory=\\S+)?$ after ^((cpu|memory|ephemeral-storage)=[^,=\\s]+(,(...)=[^,=\\s]+)*)?$ Now: any subset, any order, over a CLOSED set of three dimensions. The list stays closed deliberately. Because client-runtime has no allow-list, an unrecognised key does not error there -- it lands in the pod spec as a silently wrong envelope. The schema is the ONLY place a typo can be caught, so `memroy=8Gi` must fail at install time. FOUND WHILE TESTING: the old pattern was leakier than it looked. `\\S+` matches commas, so `cpu=2,memory=8Gi,gpu=1` VALIDATED against the supposedly-closed grammar. Tightening the value to [^,=\\s]+ closes that; a Kubernetes quantity never contains a comma, an equals or a space. 2. THE "tmpfs-backed" CLAIM IN 4.4 WAS WRONG ABOUT THE CODE. The three scratch mounts are `emptyDir` with no `medium` -- node disk. Corrected, with the reason it must STAY disk-backed stated at the site: `medium: Memory` would charge every file against the pod MEMORY limit, so an X3 resume checkpoint (~3x model size, ~3.6 GiB for BERT-large) would OOM the very run the checkpoint exists to save. Fixed the prose, not the code, and said so -- the ticket explicitly warns against "correcting" the code to match the old doc. Also records the sizeLimits from client-runtime#380 and the backend#2053 eviction-reported-as-CPU-Overload that motivated them. TESTING NOTE worth keeping. The accept/reject cases go through a VALUES FILE, not `--set`. `--set` splits on commas itself, so `--set env.RESOURCE_REQUESTS=cpu=2,` reaches the schema as `cpu=2` and the trailing-comma case passes for entirely the wrong reason -- it briefly showed as a false "accepted" while I was developing this. Recorded at the helper so nobody re-introduces it. 20 new checks (10 per key) live in chart-env-vocabulary.sh rather than helm-unittest, because that plugin treats a schema violation as a plugin-level ERROR and not a template failure, so `failedTemplate` cannot see it -- the same reason CLIENT_ENV vocabulary is asserted from outside the plugin. Chart 1.9.67 -> 1.9.68: values.schema.json is packaged chart content. Evidence: bash scripts/tests/chart-env-vocabulary.sh all 48 checks passed (was 28) make drift all 17 guards green make helm-lint 5/5 values files clean helm unittest ./client -f jobs_manager 59 passed scripts/check-style.sh clean gen-manifest.sh --check / check-facts.sh clean shellcheck -S warning -x (edited script) clean Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
LukasWodka
commented
Aug 24, 2026
ContributorAuthor
bugbot run |
Uh oh!
There was an error while loading. Please reload this page.
…ackend#2246) (#816) Bugbot flagged `try { Write($Stdin); Close() } catch { }` in Invoke-BoundedProcess: a throw from Write() skips Close(), so a child waiting for EOF hangs to the bound and returns 124 instead of its real exit. The mechanism as stated does not fire, and the measurement says why: the only exception seen here is a broken pipe, and a broken pipe means the child has already STOPPED reading -- so it is not waiting for EOF. Constructed it: /usr/bin/true with a 200 KB payload returns the child's Code=0 in 0.2s with the unfixed code, skipped Close() and all. But the four flagged lines are genuinely defective, for a worse reason. The stdout/stderr readers started AFTER the stdin write, so a child that both reads stdin and writes output deadlocks once the payload passes the ~64 KiB pipe buffer: it fills its stdout pipe, stops reading stdin, our Write() blocks, and WaitForExit() is never reached. Not 124 -- no return at all, the hard timeout this function exists to provide silently gone. Measured: /bin/cat with a 200 KB payload and -TimeoutSec 20 outlived a 60s outer watchdog. With the readers started first it returns Code=0 in 0.2s with all 200000 bytes back. Two changes, both inside those lines: * the ReadToEndAsync() drains move ahead of the stdin write * Close() moves into a `finally`, so it runs even when Write() throws Neither is reachable from today's two call sites -- `docker login --password-stdin` and `docker exec -i <node> sh` -- whose payloads are a credential and a ~304-byte prep script, both far under the pipe buffer. This hardens a latent contract violation in a general-purpose bounded-exec helper; it is not a live incident fix. Also replaces the old source guard, which pinned `Write(...); Close()` as one literal blob and so spoke for two independent properties at once, with three narrower checks: the write guard, the finally placement, and a drain-before-write ordering check derived by index position from the real function body (failing closed when either anchor is missing, so "cannot tell" is a finding). Plus the behavioural deadlock case, which is the one that actually reddens. scripts/manifest.sha256 regenerated -- install-k8s.ps1 is in the supply-chain integrity manifest, and `make drift` caught the stale digest. Verification on macOS 26.5.2, pwsh 7.5.2 / Pester 6.0.1: install-k8s.Tests.ps1 749 total, 736 passed, 0 failed, 13 skipped (origin/develop baseline 746/733/0/13 -- exactly +3) installer-parity 3/3 install.Tests.ps1 41/41 telemetry.Tests.ps1 114/114 make lint green (54 parsed, 61 shellchecked) make drift all 18 guards green Parser::ParseFile clean on both changed files Mutation-proved, anchor counts asserted == 1 before each replacement: M1 readers moved back after the write -> ordering guard RED (drain 3976 > write 3851) and deadlock case RED ("outlived a 60s watchdog despite -TimeoutSec 20"); restored byte-identical (cmp). M2 Close() chained back into the write's try -> finally guard RED and write guard RED; restored byte-identical (cmp). The deadlock case is gated on `Test-Path /bin/cat` -- the real precondition -- not on $IsWindows, which does not exist under the Windows PowerShell 5.1 that install.ps1 pins and would have turned a missing binary into a false regression. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…backend#2446) (#814) The last three floating image tags in scripts/tests/e2e-proxy.sh: ubuntu/squid:latest (host container + in-cluster Deployment) and nginx:alpine. E2E auth-proxy (squid) is a candidate required status check. Under a floating tag an external registry push can redden it on an unrelated PR, with nothing in the diff to explain why. Squid especially: the hand-written squid.conf names a path inside the image (/usr/lib/squid/basic_ncsa_auth) and the access-log assertions parse squid's log format, so an image rebuild can break either. Pinned by digest rather than by tag. Canonical publishes ubuntu/squid only under channel suffixes (_beta / _edge) — there is no immutable plain 6.6-24.04 — and those channel tags move: 6.6-24.04_edge carries a different digest, re-pushed months after _beta. A bare tag would only narrow the hole the pin exists to close. The readable tag is kept alongside the digest. Both digests are what the floating tags already resolved to when the pin was taken, so this is behaviour-preserving by construction and the e2e run proves the pin rather than a version migration. Squid is now declared once (SQUID_IMAGE) and consumed by both use-sites, so the host container and the in-cluster Deployment cannot drift apart. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…backend#2350) (#813) * chore(tests): pin the e2e-proxy probe image, like the one next door (backend#2350) `scripts/tests/e2e-proxy.sh` ran its egress-app probe on `curlimages/curl:latest` while `e2e_egress_positive_control` in `scripts/tests/lib/e2e-common.sh` pinned `curlimages/curl:8.20.0` for the same job. Same version now, so the two probes cannot drift. This is hygiene, not a regression fix. The unpinned tag was NOT the cause of the backend#2350 flake — both `latest` and `8.20.0` were measured there and behave identically with respect to curl's negative DNS caching. The reason to pin anyway: `E2E auth-proxy (squid)` is a candidate required status check. Under a floating tag, an external registry's next push would be able to block a merge in this repo, with nothing in the diff to explain it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(tests): say what the pin does NOT close, and refresh the stale image claim (backend#2350) Review feedback from @saadqbal and @aptracebloc, both making the same point: the comment read as though the required-check exposure was handled, while squid and nginx in the same file and the same job still floated. Neither asked to widen the scope, so the code is unchanged and only the claim moves. The comment now says the exposure is a property of the whole job rather than of one line, and points at client#814 (backend#2446), which digest-pinned squid and nginx. With those merged and this, no image the job pulls floats. Also refreshes e2e-proxy-probe.bats, which landed on develop meanwhile and asserted "curlimages/curl:latest (the image the pod runs)" -- true before this PR, false after it. It now names the pinned tag and records that the A/B was measured on both, so the finding still holds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
… seal row (client-runtime#199) (#809) Pre-verify the dev fleet's SEAL-CHECK substrate row (the runbook on client-runtime#199 permits recording this without flipping). Read-only cluster inspection only — no chart/config change. - EKS cluster tb-client-dev-templates enforces NetworkPolicy egress: VPC CNI aws-network-policy-agent v1.2.7, --enable-network-policy=true, NETWORK_POLICY_ENFORCING_MODE=standard. Substrate VERIFIED; full-chart egress-enforcement probe not recorded because the per-fleet flip is HELD. - The flip is held: the only dev fleet (tracebloc/tracebloc-templates) is a mixed template-validation fleet that still runtime-fetches HuggingFace for NLP templates (no TRANSFORMERS_OFFLINE/HF_HUB_OFFLINE set), so it is out of the RFC-0003 D6 CV/non-NLP scope until HF runtime-fetch support is removed. - Also notes the fleet currently disables the training NetworkPolicy (networkPolicy.training.enabled=false), so a future flip needs that too. Co-authored-by: Syed Saqlain <syedsaqlain@MacBook-Pro.local> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…data dir (backend#2422) (#817) * feat(installer): refuse to install when the nodes can't see the host data dir (backend#2422) In hostpath mode every chart PV is a hostPath onto /tracebloc/<release>/..., and /tracebloc is the k3d bind mount of HOST_DATA_DIR. When that mount is not in effect, NOTHING FAILS: kubelet's DirectoryOrCreate fabricates the directory inside the node's own filesystem, the PVC Binds, the pod Runs, MySQL initialises a brand-new empty datadir and the dataset dir reads as zero rows. No event, no warning, no failed probe -- the operator sees a healthy install that has quietly stopped using their data, and on the next `cluster delete` it goes with the node. Found during the backend#2422 PV-rebinding rehearsal. It is not hypothetical on the laptops this epic targets: a HOST_DATA_DIR outside Docker Desktop's shared paths produces exactly this, as does a cluster recreated by hand without -v. WHY NOT THE OBVIOUS CHART FIX. Flipping the two data PVs to `type: Directory` so kubelet refuses does not work: spec.persistentvolumesource is IMMUTABLE after creation, so it is rejected on any release that already has PVs. Measured on a real v1.36.3+k3s1 cluster -- `helm upgrade` fails with "spec.persistentvolumesource is immutable after creation" and leaves the release in `failed`, which is then what the fleet auto-upgrade CronJob retries. That would break the next upgrade of every existing hostpath install to close a silent-data bug. So the check goes in the installer, before helm runs, where being wrong costs an error message instead of a broken upgrade. The probe writes a token under HOST_DATA_DIR and reads it back from inside every node container. Content, not presence: a mount pointed at the WRONG directory still shows a file of that name from an earlier run. Fails CLOSED. An unreadable marker, a node that cannot be exec'd, and a node list we cannot obtain all block the install -- "cannot tell" is a finding, since proceeding anyway is the exact behaviour this exists to end. Skipped in node-local mode (RFC-0003 Option C), which deliberately has no host mount. EVERY node, not just the server: AGENTS defaults to 1 and agents run kubelets, so a training pod can land on an agent -- the same @all-vs-@server trap as the cgroup v1 flag in #806. k3d's -serverlb is excluded; it is a proxy, not a kubelet, and probing it would fail every install. Both installers, because a guard in one language leaves the other half of the fleet with the silent mode -- and Windows/Docker Desktop is where the unshared- path cause is MOST likely. A twin-presence test asserts both exist AND are wired in, defined-but-never-called being the likeliest regression. Mutation-proved, 10 anchors, each reddening only the test that owns it: never refusing; presence instead of token match; empty node list failing open; server-only probe; head -1 (first node only); node-local not skipped; probe file left behind; and the three PowerShell equivalents. Two of the bats tests were found VACUOUS during this -- a bad mock made every case take the "cannot list nodes" branch, which is also non-zero, so the refusal tests passed while exercising the wrong refusal. They now assert their own message, and the mock uses globals (bash captures no closure, so the locals were unset by call time). Verified: cluster.bats 126/126, Pester install-k8s 739 passed / 0 failed (develop baseline 733/0, +6 here), assess.bats 71/71, hostpath-prep / bats-hygiene / check-style / check-facts / gen-manifest / copy-catalog / check-drift all green, shellcheck -S warning -x at develop's baseline count (2), bash -n clean, manifest regenerated. The first placement of the PowerShell half was wrong and the suite caught it: in Install-ClientHelm it fired inside unit tests that mock docker away, and one Err exit cascaded into 583 failures. It now sits at the end of New-K3dCluster, which is also the correct parity with the bash twin's cluster path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * style(installer): say "secure environment", not "workspace", in the mount-probe refusal (backend#2422) The style guard bans "workspace" in user-facing text (STYLE.md) and it caught both twins. Manifest regenerated for the copy change; `make drift` is now 18/18. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(installer): select probe nodes by k3d label, bound both docker calls, drop the culture-sensitive mint (backend#2422) Addresses every finding on #817 -- two from Bugbot, one from @saqlainsyed007. 1. NODE SELECTION BY LABEL, NOT NAME (@saqlainsyed007). `name=k3d-<cluster>-` is an unanchored SUBSTRING match, so it also lists a same-prefixed sibling cluster's nodes (k3d-tracebloc-dev-server-0). If that sibling was created against a different HOST_DATA_DIR its nodes cannot see this token, so the probe would refuse THIS install while naming a node that is not ours -- a false refusal, the one failure mode a fail-closed guard most has to avoid. Fixed one step further than the review suggested: rather than anchoring a name REGEX, select on k3d's own labels. `label=k3d.cluster=<name>` is an exact value match (verified: `--filter label=k3d.cluster=tb` returns nothing for cluster `tb-copyreview`), and `k3d.role` says what each container IS -- so the load balancer is excluded because it is a `loadbalancer`, not because its name happens to end in `-serverlb`. Node names are no longer parsed at all. 2. BOUNDED DOCKER CALLS (Bugbot, Medium, reported twice). A WEDGED as opposed to stopped daemon never returns from a bare `docker`, which would freeze a headless install right at this probe with no further output -- the exact failure the guard exists to replace with a clear refusal. Now `_bounded` in bash and `Invoke-DockerCli` in PowerShell, the house patterns, both at 10s. 3. CULTURE-SENSITIVE TOKEN MINT (Bugbot, High). Replaced `[int][double]::Parse((Get-Date -UFormat %s))` with `[DateTimeOffset]::UtcNow.ToUnixTimeSeconds()` -- an integer, so nothing is parsed and no culture is involved. Worth recording precisely, because the finding does NOT reproduce on the machine I tested on: under PowerShell 7 `%s` emits a bare integer ("1787575411"), which [double]::Parse accepts in en-US, de-DE and fr-FR alike -- measured all three. But this installer declares `#Requires -Version 5.1` and is invoked via powershell.exe (see its own note at install-k8s.ps1:1896), and Windows PowerShell 5.1 emits %s WITH a fractional part. In de-DE "." is the GROUP separator, so that string either throws FormatException or parses to a wildly wrong number. Bugbot is right about the platform that matters. TESTS. The de-DE round-trip test I wrote first was VACUOUS -- it passed with the bug still in place, exactly because pwsh 7 emits no decimal. It is now a source guard asserting the mint does no culture-sensitive parsing, which is the property, is checkable here, and reddens under the mutation the round-trip could not see. That guard in turn had to strip comment lines, or it tripped on the comment that EXPLAINS the ban (it names both banned constructs) -- same reason k3s-components-agreement.sh reads the installer with comments removed. Two further test defects fixed while doing this: * The Pester mocks returned Output as an ARRAY. Invoke-BoundedProcess always builds it as ONE string ($outTask.Result + $errTask.Result), so the mocks were exercising a shape production never produces -- testing a copy of the code instead of the code. They now use the real single-string form. * Two new bats tests recorded into VARIABLES from inside `$(docker ps …)`, i.e. a command-substitution subshell, so the parent never saw them. They record into files now, like the suite's own `record` helper. Mutation-proved, 7 new anchors on top of the existing 10: name-substring filter restored (2 tests redden), `docker ps` unbounded, `docker exec` unbounded, role filter dropped so the lb is probed, and the three PowerShell equivalents (culture mint, substring filter, bare docker). Every one reddens only the tests that own it. Verified: cluster.bats 129/129, Pester install-k8s 747 passed / 0 failed / 13 skipped (develop baseline 733/0/13, +14 here), `make drift` 18/18 including check-style and gen-manifest --check, shellcheck -S warning -x back at develop's baseline count of 2 (the label refactor left `role` unused -- removed), bash -n clean, manifest regenerated. Also merges origin/develop: the only conflict was scripts/manifest.sha256, which is regenerated rather than hand-resolved. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(installer): isolate stdout in the mount probe, so docker stderr can't forge a miss (backend#2422) @saadqbal's blocker on #817, and it is real. Invoke-BoundedProcess returns `Output = ($outTask.Result + $errTask.Result)` -- a plain string concatenation -- so any client-side docker warning lands in the same string the probe compares against the mount token. The result is a FALSE REFUSAL: the install aborts with "cannot see your data directory" on a machine where the mount is fine, after the cluster is already up. That is the single worst outcome for this guard. WHY THE OBVIOUS FIX DOES NOT WORK. The marker is written -NoNewline, so `cat` emits the token with NO trailing newline and stderr glues onto it INSIDE THE SAME LINE. "Take the first non-empty line" therefore still yields `<token>WARNING: ...`. Saqlain called this out explicitly; the three options he listed were isolating stderr in the helper, comparing a prefix, or giving the marker a trailing newline. Took the first, opt-in: Invoke-BoundedProcess and Invoke-DockerCli gain a `-StdoutOnly` switch, and both probe calls use it. Opt-in is the design, not laziness -- the merged Output is LOAD-BEARING for most callers (Get-GpuBuildFailureReason classifies a docker build by matching stderr text), so isolating globally would break the diagnosis those callers exist to produce. Only the success path is isolated; the failure/timeout paths keep their merged or synthetic text, which is pure diagnostics, and every caller checks .Code before reading .Output for a value. Same root cause, second-order, on the `docker ps` parse above it: with no separator inserted, a stdout lacking its trailing newline would glue a warning onto the LAST node's role field ("serverWARNING: ..."), dropping that node from the list so a single-node cluster falls into "Couldn't list the nodes". docker's --format does terminate its output, so it was latent -- and it goes away with the same switch. TESTED AGAINST A REAL PROCESS, not a mock of the call whose output shape IS the bug: a child writes to both streams, and the test asserts the default still merges them with no separator (`tokWARNING:chatter` -- the precise mechanism) while -StdoutOnly returns `tok` alone. A second test captures what Assert-NodesSeeHostData actually requests, so the wiring cannot silently regress to the merged form. Mutation-proved three ways, each reddening only what it should: reverting -StdoutOnly on the exec call (the original bug), neutering the switch inside the helper, and isolating ALWAYS -- that last one reddens a pre-existing GPU test, which is the proof that the opt-in design is protected rather than merely intended. BASH IS IMMUNE BY CONSTRUCTION, and the test that says so had to be corrected. `$( )` captures stdout only, so the twin's `2>/dev/null` merely keeps the terminal quiet. My first bats test claimed to guard that redirect and was VACUOUS -- removing it changed nothing capturable and the test stayed green. It now documents the real mechanism and pins the regression that CAN break this side: someone adding `2>&1` to capture diagnostics into the variable. Mutation-checked -- with `2>&1` it fails with the exact false refusal. Non-blocking review point also addressed: install-k8s.ps1's enumeration of the places that are correct only because Windows is hostpath-only now names Assert-NodesSeeHostData as the third, since that comment is what someone adding a Windows node-local path will read. Verified: cluster.bats 130/130, Pester install-k8s 749 passed / 0 failed / 13 skipped, `make drift` 18/18, shellcheck -S warning -x at develop's baseline count (2), manifest regenerated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(installer): query nodes per role so no argument carries a quote, and count call sites not mentions (backend#2422) Two findings from @saadqbal's review, both confirmed and both real. He corrected Bugbot's mechanism on the first in a way that changed the fix, and owned the second. 1. HIGH -- WINDOWS ARGV ATE THE FORMAT STRING. Invoke-BoundedProcess joins the args into one command line and quotes any whitespace-bearing value as '"' + $_ + '"' with NO escaping of inner quotes. The single query --format "{{.Names}} {{.Label `"k3d.role`"}}" has both a space AND quotes, so it went out with its own quotes intact and CommandLineToArgvW toggled in and out of quoting to hand docker ONE token with the inner quotes CONSUMED: `{{.Names}} {{.Label k3d.role}}`. text/template then cannot parse k3d.role as an identifier, docker exits non-zero, $nodes stays empty, and the probe throws "Couldn't list the nodes" -- a FALSE REFUSAL on every Windows hostpath install, after the cluster is already up. Bugbot said it splits into several argv tokens; Asad measured that it does not, and that detail is the fix: one intact argument with broken quoting, not fragments. Fixed by removing the need for quotes at all: ONE QUERY PER ROLE, letting docker AND two label filters. `{{.Names}}` has no space and `label=k3d.role=server` has neither, so no argument reaches the quoting branch. It also drops the awk/PowerShell role parsing, and the load balancer is now excluded BY CONSTRUCTION -- its role is `loadbalancer`, which is simply never queried. Applied to BOTH twins even though bash was never exposed (it passes an array and never re-joins). Keeping both halves on the shape the constrained one requires is what keeps them diffable; a divergence here is a twin gap nobody notices until Windows breaks. Asad's general alternative -- escape inner quotes in the shared quoting branch -- would cover every other caller too, and is the better long-term fix. Deliberately NOT done here: it changes command-line semantics for every caller in a signed installer bootstrap, on a platform I cannot test from this machine. Filed separately rather than smuggled into a data-loss guard. Test: the quoting lives BELOW the Invoke-DockerCli mock, so no Pester case could ever reach it -- which is exactly why it shipped. The property is asserted at the mock boundary instead: no argument may contain a quote or whitespace, and both roles plus the exact cluster label must be queried. Mutation-proved by restoring the old combined format (7 tests redden). 2. MEDIUM -- THE WIRING GUARD WAS VACUOUS ON ITS OWN REGRESSION. It counted MENTIONS (`grep -c … -ge 2`), and comments naming the function keep the count up. Measured: deleting the real call left THREE mentions in install-k8s.ps1 -- definition plus two comments -- so it stayed green with the wiring gone. The mutation output in this change shows both numbers side by side: 3 mentions (old check passes) vs 1 code mention (new check fails). The bash half was sound only by luck at 2 occurrences, and would have gone vacuous the moment anyone wrote a comment naming the function -- precisely what happened on the ps1 side. So a threshold bump would paper over it; the count has to be of CALL SITES. Both halves now strip comment lines before counting, the technique k3s-components-agreement.sh already uses. Mutation-proved on both. A third gap surfaced while mutation-testing my own fix, and it was mine, not the review's: the new PER-ROLE fail-closed branch was untested. A fail-open mutation of it stayed green, because an errored query and an empty one both reach the same final error. The distinguishing case is one role answering while the other ERRORS -- we cannot tell whether there are agents to probe, so refusing is the only safe answer. Added on both sides, plus its opposite (an EMPTY agent list is legitimate on AGENTS=0 and must not be refused), which pins the branch to exit status rather than emptiness from both directions. Both mutation-proved. Verified: cluster.bats 132/132, Pester install-k8s 752 passed / 0 failed / 13 skipped (this base's develop baseline is 736/0/13; +16 here), `make drift` 18/18, check-style clean, shellcheck -S warning -x at develop's baseline count (2), bash -n clean, manifest regenerated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(installer): keep the per-role fail-closed branch reachable under set -e (backend#2422) @saadqbal's finding, and he is right on both the mechanism and on why my test could not see it. install-k8s.sh runs under `set -euo pipefail` and shell options are global to the sourcing shell. A bare `out=$(docker ps …)` is a simple command whose status is the substitution's, so when docker errors set -e exits AT THE ASSIGNMENT -- and everything below it is dead code: the fail-closed branch AND the `rm -f` of the probe marker. The previous shape survived only because it ended in `| awk … || true`, which is exactly what kept set -e off it; my per-role rewrite dropped that without replacing it. What the operator would have got: the ERR trap's generic `_record_err` naming `docker ps` instead of the curated refusal, and the probe marker left behind in HOST_DATA_DIR. Precisely the opaque failure this guard exists to replace. Fixed with `st=0` then `out=$(…) || st=$?` -- the `||` context suppresses set -e and preserves the status. WHY THE EXISTING TESTS WERE BLIND, which is the part worth recording: `run` captures the status, and that SUPPRESSES set -e. Production calls this function BARE (create_cluster -> install-k8s.sh:272), so `run`-based tests exercise a different shape than production and pass either way. Measured against Asad's exact pre-fix shape (bare assignment + `st=$?` on the next line): the two per-role tests stay GREEN while the new test reddens. So the note I had left on those tests -- "a fail-OPEN mutation stays green (measured)" -- was covering the logic axis and claiming the reachability one. Corrected in place to say what each test does and does not cover. The new test reproduces production instead of `run`: a subshell that sets the same options and calls the function bare, with the outer `|| st=$?` on the substitution rather than inside it. It asserts three things, because the bug breaks all three -- non-zero status, the curated message actually reached, and the marker cleaned up. Checked the whole function for the same class rather than just the reported line, and pinned the result: the `printf … || error`, the `$( … || true )` exec capture, the `[[ -n "$out" ]] && nodes+=…` append (an AND-list failure does NOT trip set -e -- verified empirically, so this one is safe as written) and the `rm -f … || true` are all fine; only the per-role assignment was not. Two extra tests keep the SUCCESS path and the node-local early return honest under set -e too, since an abort on either would fail every install rather than merely skip a guard. This class was already known in this suite -- there is a #424 Bugbot test for errexit-safety on the CA resolve capture. I should have applied that existing pattern when I introduced a new command substitution; the fix is the same shape it uses. The PowerShell twin is unaffected: `throw` is not conditional on shell options. Verified: cluster.bats 135/135, Pester install-k8s 752 passed / 0 failed / 13 skipped (unchanged -- ps1 untouched this round), `make drift` 18/18, check-style clean, shellcheck -S warning -x at develop's baseline count (2), bash -n clean, manifest regenerated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…8) (#819) * feat(installer): move the Kubernetes pin to v1.36.3-k3s1 (backend#2448) We pinned v1.29.4-k3s1, ~18 months past EOL (1.29 EOL 2025-02-28). Only 1.33-1.36 are supported today. And the fleet was already drifting past the pin on its own -- install-k8s.ps1 records a customer running k3s v1.35.5 against it -- so this is not "hold the line", it is "the line was already gone and untested". Lifecycle, not features. Do not read this as unlocking in-place pod resize: that has no actor today (training pods have automountServiceAccountToken: false, the NetworkPolicy blocks the apiserver, and jobs-manager has no pods/resize RBAC), and Kueue conflicts with it upstream (#5257). TARGET 1.36.3 DIRECTLY, no 1.34 stepping stone: a freshly created cluster is exempt from the no-skip-minor rule, so two rebuilds buy nothing. Spike backend#2422 answered all six questions on a real cluster -- k3d v5.9.0 creates a working v1.36.3+k3s1 cluster (node Ready, CoreDNS up, pod schedulable), the containerd 2.x config path holds, and k3d's registries.yaml translation still works. No newer k3d needed; this was never "wait for upstream". THE TAG IS v1.36.3-k3s1 WITH A HYPHEN. The k3s RELEASE is v1.36.3+k3s1, but Docker tags cannot contain `+`, so rancher publishes the hyphen form. Verified against registry-1.docker.io: `v1.36.3-k3s1` -> 200, `v1.36.3+k3s1` -> 404. The ticket text quotes the release string; the pin has to be the image tag. DERIVED, NOT RESTATED. scripts/spec/facts.env is the single source of truth and `check-facts.sh --write` stamps it into every consumer, so the version itself is a one-line change reaching 7 consumers. What needed work was the places that were NOT derived: * The HELP TEXT default is now its own check-facts consumer row in both installers. It was a restate the script could not see: the pin would move in code while `--help` went on advertising the old version, with only a golden-file diff to notice. Two new rows (common.sh:K8S_VERSION-help, install-k8s.ps1:K8S_VERSION-help), mutation-proved by reverting one of them. * check-facts.bats seeded its fixture consumers from LITERALS -- a second copy of every pin. This bump turned the whole suite red because the fixture disagreed with the spec it had just copied. It now derives the seed values from that copied spec, and is proven pin-agnostic: the suite stays green with the pin set to 1.29.4, 1.33.9 and 9.9.9. * cluster.bats had a test named "NOT passed on the currently pinned k3s (1.29.4 predates the flag)" that hardcoded 1.29.4 -- so it sailed through this migration still green while NOTHING asserted the behaviour of the version we actually ship. Renamed to what it really covers (a pre-1.31 version), and a new test DERIVES the pin from facts.env and asserts the cgroup v1 override is emitted for it. That test reddens when the pin is rolled back below 1.31 and fails closed when facts.env is unreadable. THE DRIFT WARNING WOULD HAVE LIED TO EVERYONE. k3s's version is fixed at cluster create, so every pre-existing cluster is now "drifted" -- the warning fires for the whole installed base. Its text blamed "an older/unpinned installer or K8S_VERSION=latest", which is not what happened to those operators: their cluster simply predates the pin move. Reworded in both twins to name that cause first, with a test that reads the pin from facts.env, simulates a 1.29.4 cluster, and asserts the message does not misattribute it. It WARNS and does not refuse, so re-runs on an older cluster keep working. Also regenerated/kept in step: the 00-install golden (one line -- which is the proof the help text is the operator-visible surface), the base64-embedded copy of the GPU Dockerfile inside install-k8s.ps1 (the drift guard caught it; the decoded delta is exactly the one ARG line, asserted in the re-embed), docker/k3s-cuda README examples, both installer header comments, and the manifest. README gains an operator note: a new install gets the pinned version, an EXISTING cluster keeps the version it was born with, and moving it means recreating the cluster -- with the data behaviour of each storage mode spelled out. Two things deliberately NOT changed: * The `1.31.0` literal in the cgroup gate. That is the release that ADDED --fail-cgroupv1, not our pin, and it must not track the pin. * The v1.35.5 drift-incident narrative comments that mention 1.29.4. They describe a past incident accurately; rewriting them would falsify history. The Windows GPU path does NOT depend on a prebuilt image for the new tag: the default path BUILDS the node image locally (install-k8s.ps1:6817 -> Build-GpuNodeImage) with --build-arg K3S_TAG=$K8S_VERSION, and only reaches for a prebuilt one when TRACEBLOC_K3S_CUDA_IMAGE or TRACEBLOC_IMAGE_REGISTRY is set. So GHCR does not gate this. Air-gapped/mirror customers do need the new tag in their mirror, and publishing ghcr.io/tracebloc/k3s-cuda:v1.36.3-k3s1-cuda-12.4.1-base-ubuntu22.04 (the workflow default is already bumped here) is worth doing as hygiene. Verified: cluster.bats 119/119, check-facts.bats 14/14, Pester install-k8s 736 passed / 0 failed / 13 skipped -- byte-identical to this base's develop baseline, measured by stashing -- plus copy-catalog, check-drift, check-style, gen-manifest, assess, common, hostpath-prep, bats-hygiene, chart-version-guard, gpu-embed-drift and check-digest-drift all green, `make drift` 18/18, bash -n clean on all four touched shell files, shellcheck -S warning -x at develop's baseline count. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(readme): release the secure environment BEFORE deleting the cluster (backend#2448) Bugbot's finding, and it is the most consequential thing on this PR. My pin-move note told operators to recreate with a bare `k3d cluster delete`. Both installers explicitly warn against exactly that: "Release this machine's secure environment BEFORE deleting the cluster -- it is anchored to the cluster's identity, so deleting the cluster first strands it on your dashboard for good" and print `tracebloc delete --keep-data` first. Verified in both twins (_recreate_cluster_hint / Write-RecreateClusterHint) rather than taking the finding on trust. WHY IT MATTERS MORE HERE THAN ANYWHERE ELSE. k3s's version is fixed at cluster create, so this pin move is precisely what forces a rebuild across the whole installed base -- which makes this README paragraph the most-followed recreate instruction in the repo the moment it ships. Following it as written would have stranded the backend secure-environment record for every one of those customers. That is a worse outcome than the EOL pin it exists to fix. Now the same two-step the installer prints, as a copyable block, with the reason stated rather than implied, and the "nothing installed yet? just the k3d line" carve-out kept. GUARDED, DERIVED, not restated. A new test reads the release command OUT OF _recreate_cluster_hint and requires the README to carry it -- and to carry it BEFORE the k3d delete, since the ordering is the entire protection. A guard that hardcoded "tracebloc delete --keep-data" would have agreed with itself while the hint moved on. Mutation-proved three ways, each reddening for its own reason: * README reverted to the k3d-only line -> "omits the release step the installer prints" * README lists the k3d delete first -> "puts the k3d delete (line 76) before the release (line 77)" * the HINT's command renamed in cluster.sh -> guard follows the hint and fails, proving it is derived rather than matching a literal Verified: cluster.bats 138/138, copy-catalog current, `make drift` 18/18, check-style clean, shellcheck -S warning -x at baseline (2), bash -n clean, manifest regenerated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(readme): don't assert which storage mode is the default (backend#2448) @saadqbal's sequencing flag on #819. He raised it as a note rather than an ask, but it is cheaper to remove the hazard than to coordinate around it. The paragraph said "In **hostpath** mode (the installer default)". client#808 (RFC-0003 D15) flips that default to node-local on the bash path -- so whichever of the two lands second silently makes the other's parenthetical false, and this one is in the README rather than a comment, i.e. the copy customers read. Reworded to describe both modes' data behaviour and name the knob that selects them (TB_STORAGE_MODE) without claiming which way it points. True under either merge order, so #808 needs no coordination with this PR at all. Deliberately did NOT write "the installer prints the mode": it only `log`s it to the install log (cluster.sh:681), so that would have been a new false claim in place of the old one. Verified: cluster.bats 138/138 (including the derived README-vs-hint guard), drift 18/18, check-style clean, manifest regenerated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
LukasWodka
commented
Aug 24, 2026
ContributorAuthor
bugbot run |
Uh oh!
There was an error while loading. Please reload this page.
…808) * feat(installer): default TB_STORAGE_MODE to node-local (client#456) RFC-0003 D15: flip the local (k3s) installer default from hostpath to node-local so a fresh install gets the "delete means gone" storage model — datasets on k3s local-path inside the node, no ~/.tracebloc host dirs, wiped on `cluster delete`. Scoped to the Linux/macOS bash core; the Windows installer (install-k8s.ps1) stays hostpath (no node-local path yet — Linux/Windows scope is @saadqbal's D15 call, see the PR). - common.sh: canonical default -> node-local (still forces AGENTS=0/SERVERS=1) - cluster.sh, summary.sh, install-client-helm.sh: fallback defaults -> node-local - install-k8s.sh + docs/INSTALL.md: help/docs; hostpath is now the opt-out - manifest.sha256: regenerated for the changed installer scripts - tests: pin hostpath where hostpath-only features are exercised; add default->node-local assertions (C1 clamp, cluster create, values render) Closes#456 Epic: tracebloc/backend#1151 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(installer): mismatch guard offers hostpath opt-out under the D15 default (client#456) Addresses the Bugbot High + @saadqbal review on #808. After the node-local default flip, _check_existing_cluster_storage_mode fires on an unmodified re-run of every pre-existing hostpath install, not just operators who chose node-local. The old text offered only "recreate as node-local" and phrased the mode as the operator's setting. - common.sh: record TB_STORAGE_MODE_SOURCE (explicit vs default). - cluster.sh: the node-local-onto-hostpath branch now leads with the keep-your-cluster remedy (re-run with TB_STORAGE_MODE=hostpath, no recreate), keeps the recreate-for-node-local path, and names the source — "node-local is the default now" vs "TB_STORAGE_MODE=node-local". - install-k8s.ps1: node-local is no longer a "prototype" — it's the Linux/k3s default since D15; the "no Windows path" half stays true (per the repo rule to fix statements a change makes false). - cluster.bats: assert the hostpath opt-out is offered + the default-source wording. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(installer): common.sh comment must not claim the D15 training-run gate is closed (client#456) @aptracebloc review: the storage-model comment stated "the green node-local dev training run closed the gate", but that run hasn't happened — Gate 1 is still open in the PR. Soften to describe node-local as the D15 default superseding the #367 prototype, and point the training-run sign-off at client#456 rather than asserting it here. 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>
LukasWodka
commented
Aug 25, 2026
ContributorAuthor
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
2 issues from previous reviews remain unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit bd2966d. Configure here.
…or the other (backend#2418) (#821) * fix(chart): emit only the resource var that is set, never a literal for the other (backend#2418) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(chart): pin hasKey rather than one value, and correct the L0.2 claim (backend#2418) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…backend#2418) (#820) * feat(installer): CPU is a share weight, so limits carry memory only (backend#2418) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(installer): regenerate the bootstrap manifest and harden the new bats assertions (backend#2418) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(installer): carry the envelope from RESOURCE_REQUESTS, not the memory-only limits (backend#2418) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(installer): match cpu= case-insensitively, and cover the write-read round trip (backend#2418) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…d so (backend#2458) (#824) * fix(image-refresh): a stale digest pin froze an image and nothing said so (backend#2458) imageRefresh honours an explicit images.<name>.digest pin -- an operator pin outranks auto-refresh, and that is correct. It skipped BEFORE resolving the tag, so nothing ever learned the pin had gone stale. A pin set once for a good reason silently became a freeze, and the only symptom was a fix that appeared to ship and did not. Measured on tb-client-dev-templates 2026-08-25: jobsManager was pinned to a build predating the code that writes the edge Collector ingest token, so the token was never written and the Collector could not be enabled. Every version signal read current -- chart label client-1.9.67, deployment spec current, pod minutes old with 0 restarts -- while the container was days behind. It cost most of a day, after the chart, the Terraform, the promotion and the RBAC had each been verified correct. Compare anyway, and report. One registry call -- the same one every unpinned image already makes -- and no behaviour changes: `continue` still ends the tick for this image either way. A stale pin now WARNs with both digests, the consequence (FROZEN) and the fix, and leaves a tracebloc.io/stale-pin-<name> annotation so it outlives the log. Fails closed on both unknowns: an unresolvable tag and a pin the tick cannot see each say so rather than reporting agreement. Two unread values compare equal in exactly the wrong way. NOT a duplicate of check-digest-drift.sh (backend#1853). That watcher reads client/values.yaml -- the CHART defaults, where jobsManager.digest is "". A pin set in an INSTALL\047s values is invisible to it. This branch runs in the cluster against the effective values, which is the only place that pin exists. Tests exercise the branch extracted from the RENDERED chart, not the template source, so they run the shell that actually ships. 9 cases: CURRENT, STALE, both digests named, FROZEN stated, annotation present, no annotation when current, and both fail-closed paths. Mutation-proved: restoring the blind skip reddens all 9 -- setup fails with "no rendered script containing the stale-pin branch". Anchor asserted. bats-hygiene caught 18 assertions that were advisory rather than enforcing ([[ ]] does not propagate under errexit on bash 3.2); all hardened. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(image-refresh): the stale-pin signal must clear, and must not cry wolf (backend#2458) Two findings from @saqlainsyed007 and Bugbot on client#824, both correct. 1. The annotation was WRITE-ONLY. Nothing removed tracebloc.io/stale-pin-* once the finding stopped being true, in either direction: a pin that became current again kept the old annotation, and clearing the pin -- the exact remediation the WARN prescribes -- stopped the branch running at all, orphaning it forever. A durable signal that outlives its problem is a permanent false finding, the opposite of the point. Now cleared on the CURRENT path and on the unpinned path, and only when one is actually present, so a healthy tick makes no extra write. An UNRESOLVABLE tag deliberately leaves it alone: that is the last KNOWN state, and overwriting it would assert an agreement we did not observe. 2. resource-monitor reports PINNED=1 when `resourceMonitor: false` disables the DaemonSet, with no pin value -- so the new empty-pin branch logged "cannot compare, which is a finding" on every healthy edge that turns the DaemonSet off, every tick. Training operators to ignore the warning is the worst possible outcome for a signal that exists to be noticed. PINNED=1 with an empty pin is that case by construction -- the only other reason for PINNED=1 is a values digest, which is non-empty -- so it is now a quiet skip with nothing to compare. Also fixed a defect in the TEST harness, not the product: it stripped every `continue` from the extracted branch so it could run outside a loop, which silently changed control flow once a second `continue` existed -- the new early skip fell through into the comparison. It now wraps the branch in a one-iteration loop and runs the real thing. A harness that quietly rewrites the code under test is worse than no harness. 12 cases, mutation-proved, anchors asserted: * remove the empty-pin skip -> 1/12 pass * remove the CURRENT-path clear -> 11/12 pass 566 chart tests, 19 drift guards, bats-hygiene, helm lint all green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(image-refresh): the third exit needs the same annotation clear as the other two (backend#2458) The empty-pin skip -- the `resourceMonitor: false` path -- continued without clearing tracebloc.io/stale-pin-*. The CURRENT and unpinned exits clear it; this one did not. A stale pin can be remediated INTO that state: drop the digest AND disable the DaemonSet. The finding then outlives the problem exactly as it did before the clear paths existed. Fixing two of three exits leaves the same write-only annotation, just harder to reach -- which is worse than not having fixed it, because the two working paths make it look handled. Mutation-proved: removing the clear drops the suite to 13/14. Anchor asserted. 14 cases, 564 chart tests, 18 drift guards, bats-hygiene all green. Found by Bugbot on client#824. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
LukasWodka
commented
Aug 25, 2026
ContributorAuthor
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
2 issues from previous reviews remain unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 31bdc52. Configure here.
…out cluster-scope RBAC (backend#2469) (#823) * fix(chart): the metrics-server preflight locked out any operator without cluster-scope RBAC (backend#2469) `lookup` has THREE outcomes, not two. The comment above this guard said it "returns empty during helm template", which is true, and then treated connected-and-permitted as the only other case. The third is connected and FORBIDDEN, which RAISES -- and helm cannot catch it. APIService is cluster-scoped and Kubernetes\047 built-in `admin` ClusterRole excludes it. So an operator whose EKS access entry is AmazonEKSAdminPolicy rather than AmazonEKSClusterAdminPolicy could not `helm upgrade` this chart AT ALL -- the whole release, not just this DaemonSet -- and the error named resource-monitor and metrics-server, so the obvious first reading was "metrics-server is broken" when metrics-server was fine. Hit while enabling the edge Collector on tb-client-dev-templates; measured afterwards with `kubectl auth can-i get apiservices` -> no. Ask something the caller can definitely read FIRST: metrics-server is a namespaced Deployment and any namespace admin can get it. When it is there the API is registered and the privileged call never happens. Only when the cheap probe comes up empty do we fall back to the authoritative one -- and by then there is a real problem worth a privileged answer. The fail message now also names the RBAC cause, since that is the reading it will most often be. nodeAgents.metricsServerPreflight: false covers the residual case. It does NOT skip silently: tracebloc.io/metrics-server-preflight on the DaemonSet records which path decided, so a skipped check cannot be mistaken for a passed one. Mutation-proved, anchors asserted: * drop the annotation -> 3 chart tests redden * remove the Deployment probe (the exact bug) -> the new source check ERRORS That second mutation is why the source check exists: helm-unittest renders OFFLINE, where every lookup is empty, so it never reaches the privileged call and cannot tell a guarded lookup from an unguarded one -- all 566 chart tests stay green under it. Claiming that coverage would have been vacuous. Registered in DRIFT_GUARDS, which runs in `Source-of-truth drift` -- a REQUIRED check. A guard in a non-required job is advice, not a gate. 566 chart tests + 19 drift guards green. shellcheck -S warning clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(chart): the cheap probe must be readable by the SA that renders every tick (backend#2469) Three findings from @saadqbal and Bugbot on client#823, all correct. 1. HIGH -- the new metrics-server Deployment probe was a 403 for the auto-upgrade ServiceAccount. auto-upgrade-rbac.yaml grants apiservices (backend#953, added specifically so the old lookup resolved) and no deployments anywhere. After the cluster-admin cutover that SA is what renders the chart on every hourly tick, so the probe would raise, helm would abort, and --atomic would roll the tick back. That trades a lockout a human hits once and can diagnose for one that hits the UNATTENDED path on every deployed client -- strictly worse than the bug being fixed. The scoped role now grants deployments get/list/watch. 2. MEDIUM -- sprig `default` treats boolean FALSE as empty, so `default true false` returns true: metricsServerPreflight: false never reached the skip branch and the opt-out was inert while looking present. Now hasKey + an explicit false test. 3. The comment beside the apiservices grant claimed a 403 makes `lookup` "return empty, which the template\047s fail misreads as metrics-server missing". It does not -- it RAISES, and the release dies before reaching the fail. Corrected, with the measured error text, since this PR is what makes that claim load-bearing. The RBAC coupling is asserted in preflight-not-privilege-gated.sh rather than helm-unittest: the auto-upgrade template needs values the resource-monitor suite does not set, and the guard already runs in the REQUIRED drift job alongside the rest of this preflight\047s assertions. Mutation-proved: removing the deployments grant reddens the guard, naming the unattended-tick consequence. Anchor asserted. Also fixed a defect in my own guard: backticks inside a double-quoted fail string were command-substituted, so the message printed "does not grant ," plus a stray "deployments: command not found". shellcheck -S warning did not catch it; the mutation run did. Chart rebumped to 1.9.70 -- develop moved to 1.9.69 while this was open. 566 chart tests, 19 drift guards, helm lint, shellcheck all green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(rbac): the preflight probe needs a NAMED get, not cluster-wide deployment read (backend#2469) The grant added to unblock the auto-upgrade path was get/list/watch on deployments in every namespace. The preflight reads exactly ONE object -- the metrics-server Deployment -- so that reopened the workload-read blast radius backend#953 closed, to buy nothing. Narrowed to resourceNames: ["metrics-server"] with verbs: ["get"]. NOT a namespaced Role in kube-system, which would be narrower still: the scoped SA can only create Roles there via the gpu-device-plugin block, which is gated on $dp.enabled. An unconditional kube-system Role would 403 on upgrade for every install with the device plugin off -- trading this grant for a new lockout on the unattended path, which is the exact mistake this PR already made once. The trade is stated in the template rather than left for the next reader to rediscover. The guard now checks the narrowing, not just the presence, and separately refuses list/watch: Kubernetes IGNORES resourceNames for collection verbs, so a resourceNames rule carrying `list` would grant enumeration of every Deployment in the cluster while looking restricted. Mutation-proved: widening back to get/list/watch reddens the guard. Anchor asserted. 568 chart tests, 19 drift guards, shellcheck all green. Found by Bugbot on client#823. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(test): the guard piped into early-closing readers under pipefail (backend#2469) My own guard tripped `quality / pipefail early-close`, which is exactly what that check exists for: grep -n ... | head -1 | cut -d: -f1 (x2) grep -A3 ... | grep -qE ... Under this file\047s `set -euo pipefail` the reader closes first, the writer takes SIGPIPE, and the guard aborts -- so a check written to fail closed would instead die on its own plumbing. Capture-then-slice for the line lookups, matching the house idiom in node-jsonpath-agreement.sh (${all%$\047\\n\047*}), and a here-string for the verbs test so there is no pipe at all. `printf | grep -q` was NOT enough -- it has the same early-closing reader, just with a different writer. Re-mutated after the rewrite to confirm the check still bites: widening the grant back to get/list/watch still reddens it. A plumbing fix that quietly disarmed the assertion would be worse than the original failure. 568 chart tests, 19 drift guards, shellcheck -S warning, bash -n all green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(chart): the cheap probe was a fleet-wide lockout; the flag is the whole fix (backend#2469) Two findings from Bugbot on client#823, and the first invalidates the approach rather than refining it. HIGH -- probing a metrics-server Deployment first, with a matching grant on the auto-upgrade ClusterRole, is a PERMANENT fleet-wide lockout. THE TICK THAT WOULD APPLY THE GRANT IS THE TICK THAT NEEDS IT: on the first auto-upgrade onto such a chart the scoped SA renders before the ClusterRole lands, the new lookup 403s, helm aborts, --atomic rolls back, and every later hourly tick fails identically. The grant can never arrive. The general rule, now written into the template: ANY new `lookup` of a resource the CURRENT ServiceAccount cannot already read locks the unattended path permanently. So the probe is gone and the grant is reverted; apiservices (already granted by backend#953) stays the only privileged call, gated by nodeAgents.metricsServerPreflight. The flag is the whole escape hatch, which is what the ticket originally proposed before I got clever. MEDIUM -- my own guard was vacuous. It grepped the template for "metricsServerPreflight" ANYWHERE, and that string also appears in the explanatory comment, so deleting the real `if` still went green. It now strips comment blocks first and asserts on code. A guard a comment can satisfy is not a guard. The guard also gained the invariant that matters more than any single assertion: it ENUMERATES the preflight\047s lookups and fails on any it does not recognise as already-granted -- so the next person to add a cheap-looking probe is stopped rather than trusted. Also removed the chart test that set metricsServerPreflight: false. Offline `lookup` is empty, so $probe is false and the branch is unreachable -- the assertion passed on the offline default and proved nothing. Bugbot caught it. The reason is recorded in its place. Mutation-proved, all five assertions, anchors asserted: delete the real if (comment left) -> reddens reintroduce the Deployment probe -> reddens re-add the deployments grant -> reddens drop the annotation -> reddens remove the apiservices grant -> reddens 567 chart tests, 19 drift guards, helm lint, shellcheck all green. Chart 1.9.72. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(test): the guard could be satisfied by a comment and by the lockout itself (backend#2469) Two more from Bugbot, both on the guard rather than the chart, and the second is the one that mattered. 1. The comment stripper matched only `{{/*` / `*/}}`. This template uses BOTH forms -- `{{- /*` opens twice and `*/ -}}` closes once -- so the stripper either leaked prose into the greps or, on a closer it could not see, swallowed the rest of the file and passed on an empty parse. Both directions are silent. It now matches the trimmed pair too, blanks comment lines rather than deleting them (the ordering checks need original line numbers), and REFUSES on an unterminated block or an all-blank result -- an over-eager strip leaves an empty parse that compares equal to anything. 2. Check 2 asserted only that SOME `if` mentions metricsServerPreflight. Hoisting the lookup ABOVE that `if` -- the original lockout, unchanged -- still passed. It now pins the ordering: gate < else < lookup, all on code lines, so the privileged call is reachable only through the gate\047s else branch. Presence was never the property; reachability was. Mutation-proved, anchors asserted: hoist the lookup above the gate -> reddens (previously GREEN) delete the real if, keep a {{- /* comment naming the flag -> reddens unterminated comment block -> reddens rather than passing empty 567 chart tests, 19 drift guards, helm lint, shellcheck all green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(test): the guard could report a pass for a check it did not perform (backend#2469) Two of the three line lookups were followed by [ -n ... ] || fail; the APIService one was not. With that empty, [ 98 -ge "" ] prints "integer expression expected" and returns 2 -- but it is the condition of an `if`, so errexit is suppressed by design, the branch is simply not taken, `fail` is never reached, and the script runs on to print GREEN. A required guard reporting a pass for a check it did not perform, on the one job whose whole purpose is noticing exactly that. It contradicted this file\047s own header. Fixed structurally rather than by adding the missing line: `require_line` refuses on its own behalf, so there is no way to call it and forget the check. The asymmetry that caused this cannot recur. The more important half is @saadqbal\047s point that this is the THIRD fail-open hole in this one script -- the untrimmed match, the pipefail early-close, and now this -- and that every one was the GUARD failing while the guarded thing was fine, found by reading rather than running. So the mutations now run in CI: preflight-guard-fails-closed.bats breaks the template or the RBAC nine ways and asserts the guard exits NON-ZERO, on a throwaway copy so a mutation cannot escape into the tree. Case 1 asserts the unmutated tree passes, or every case below it would be vacuous. Mutation-proved against itself: removing the new refusal reddens case 2. Two test-authoring defects found by RUNNING it, both mine, neither in the product: rewording the lookup trips the unrecognised-lookup check first and exercises a different branch (it now deletes the line outright, which is the real scenario), and the expected phrase spans a line break in the real output so asserting it would have failed on a CORRECT guard. 567 chart tests, 19 drift guards, bats-hygiene, helm lint, shellcheck green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…e baked literal) (#822) * sec(#947): chart-manage the mysql-client root password (rotate off the baked literal) backend#947 / backend#1528 Phase 0: enable rotating the mysql-client root password off the image's baked literal. New rotateMysqlRoot gate (default off, ByEnv-resolved like serviceDbAccounts / bootstrapDbReparent): when on, the chart generates a random root password into the Secret (3-tier: pin -> existing Secret -> randAlphaNum, mirroring credmgr) and injects it as MYSQL_ROOT_PASSWORD on the mysql-client deployment. Scope — this rotates a FRESH datadir only. The mysql entrypoint reads MYSQL_ROOT_PASSWORD at init and ignores it thereafter, so an existing edge keeps its current root password until the one-time rollout DDL runs (ALTER USER 'root'@'%'/'@localhost' IDENTIFIED BY '<the generated Secret value>', via kubectl exec as root@localhost). The chart deliberately does not run that ALTER: it would need to authenticate as root with the current literal (re-introducing it) or hit a chicken/egg once rotated — so, like the rest of the epic, the live-fleet DDL stays an ops step. Wiring this Secret into #785's bootstrapDbPassword is a deliberate follow-up (one self-contained change per PR; don't churn just-merged code). - values.yaml: rotateMysqlRoot + rotateMysqlRootByEnv{dev,stg,prod: false} + mysqlRootPassword pin. Heavy comment on the fresh-vs-existing distinction and the ALTER rollout step. - _helpers.tpl: tracebloc.rotateMysqlRoot (mirrors serviceDbAccounts resolution). - secrets.yaml: MYSQL_ROOT_PASSWORD 3-tier generate-and-persist, gated. - mysql-deployment.yaml: MYSQL_ROOT_PASSWORD env from the Secret, gated; probes use `mysqladmin ping` (no auth) so they are undisturbed. - values.schema.json: three keys. Chart.yaml 1.9.68 -> 1.9.69 (+ appVersion). - tests: +7 helm-unittest cases (secrets off/on/pin/byEnv/placeholder; deployment env off/on). Additive and default-off: helm unittest 569 passed, lint clean, default render byte-identical (0 MYSQL_ROOT_PASSWORD refs), gate-on injects a 48-char random password (not the literal). No literal in the diff. Refs: backend#947 (rotate + remove hardcoded creds), backend#1528 (Phase 0), client#785 (re-parent this rotation feeds). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(#947): teach migrate-tenant.sh where root lives once the chart rotates it migrate-tenant.sh already requires MYSQL_ROOT_PW (no hardcode); this just tells the operator that on a fleet with rotateMysqlRoot on, that value is the mysql-client Secret MYSQL_ROOT_PASSWORD, not the image-baked legacy password. Comment-only, lands with the rotation feature it describes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(#947): add the per-fleet mysql root rotation runbook The operational half of rotateMysqlRoot: read the generated Secret value, run the one-time ALTER USER root (both hosts) via kubectl exec, verify old-pw-dead / new-pw-works / heartbeat-full-count, roll back by re-ALTERing. Documents the consumer inventory that must move to the Secret first and the prod exec-access gap. Lands with the feature it operationalizes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(#947): keep the rotation runbook root password out of argv (Bugbot) The runbook interpolated NEWPW into the sh -c string, so the ALTER USER text showed in the node ps output — the exact leak the runbook warned against. Rewritten so every secret travels only over the exec stdin stream: auth via a mode-600 --defaults-extra-file written+deleted in the pod, the new password via mysql stdin. Nothing in any process argv. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(#947): fail the rotation runbook loudly on a failed ALTER (Bugbot) The remote sh -s body had no set -e and ended in rm -f, so a failed ALTER USER still exited 0 — kubectl exec reported success while root stayed on the old password and the Secret diverged. Now: set -e + an EXIT trap that always wipes the password file; the rotate and new-pw-verify blocks exit non-zero on failure, and the old-pw-verify inverts explicitly (OK only when the old pw is rejected). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(#947): runbook mysql over TCP + assert 1045 on the old-pw check (Bugbot) Two related High findings on the runbook snippets: - every mysql call used the default unix socket, but this image puts the socket at /var/lib/mysql/mysql.sock (the reason the probes are pinned to -h 127.0.0.1) so the ALTER/verifies could not reach mysqld. Added host=127.0.0.1 to every --defaults-extra-file. - the old-password verify treated ANY non-zero mysql exit as \"rejected\", so a connection error (mysqld restarting, 2003) read as success. It now requires ERROR 1045 (access denied) to pass and flags anything else INCONCLUSIVE. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(#947): refuse an empty root password in the rotation runbook (Bugbot) An empty NEWPW (wrong NS/REL, or rotateMysqlRoot not actually enabled so the Secret lacks MYSQL_ROOT_PASSWORD) would ALTER root to an EMPTY password while the runbook printed success. Guard both CURPW and NEWPW non-empty before building any ALTER, using ${var:?} so it fails fast but is safe to paste interactively — same fail-fast intent as migrate-tenant.sh MYSQL_ROOT_PW guard. Preserves the earlier fixes (argv-safe stdin, set -e/trap, 1045-specific verify, TCP host). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * sec(#947): constrain mysqlRootPassword to alphanumeric (Bugbot) The chart documented mysqlRootPassword as \"never interpolated into DDL\", but the rotation runbook this PR adds embeds it in ALTER USER ... IDENTIFIED BY '...'. A pin with a quote or metacharacter (which the chart allowed) would break the one-time ALTER; generated randAlphaNum happened to be safe. Reconciled by enforcing [A-Za-z0-9]+ on the pin, exactly like the credmgr/tb_meta/tb_ingest pins (which are constrained for the same CREATE USER DDL reason). bootstrapDbPassword stays unconstrained — it is only ever a connection parameter, never DDL. Updated values.yaml + schema wording and added a reject test. --------- Co-authored-by: Syed Saqlain <syedsaqlain@MacBook-Pro.local> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
LukasWodka
commented
Aug 25, 2026
ContributorAuthor
bugbot run |
…end#2223) (#826) Found while verifying #2223 before closing it. The Disk row still read "not bounded at all ... the resource grammar cannot express one", which is precisely what #2223 fixed: client-runtime#380 added the pod-level ephemeral-storage request/limit and a sizeLimit per scratch volume, client#812 opened the grammar, backend#2477 gave an eviction its own disk failure class. Carries the same version qualifier as the CPU row, for the same reason: the bound is applied by the runtime image, not by anything this chart renders, so a fleet on an older image has no disk bound however this file reads. Not chart content (*.md is excluded from the version guard), so no Chart.yaml bump. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
1 issue from previous review remains unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 09bb86c. Configure here.
…lient#828) (#829) * fix(installer): keep merged stderr on a non-zero exit under -StdoutOnly (client#828) Invoke-BoundedProcess documents -StdoutOnly as isolating stdout ONLY on the success path, keeping merged stderr on failures for diagnostics. But the switch was applied on ANY exit, so a child that exited non-zero returned stdout only and its stderr was silently discarded -- contract and behaviour disagreed. Gate the isolation on exit code 0: on a non-zero exit return the merged stdout+stderr, matching the documented contract. The timeout arm (Code 124, synthetic text) is unchanged. Blast radius today is none -- both current -StdoutOnly callers (Assert-NodesSeeHostData) check .Code before reading .Output -- but a future caller reading .Output on failure would have lost the stderr the contract promises. Pin it with a Pester case that runs a child exiting non-zero with output on stderr and asserts .Output keeps the merged stderr even under -StdoutOnly. Verified: the new test fails against the pre-fix code and passes after; all 17 Assert-NodesSeeHostData cases green (pwsh 7.6.5 / Pester). Closes#828 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(installer): regenerate manifest.sha256 for the install-k8s.ps1 change (client#828) install-k8s.ps1 is an R8 signed sub-script whose digest is pinned in scripts/manifest.sha256; editing it makes the pinned hash stale, which reds `gen-manifest.sh --check` (Source-of-truth drift) and the `a clean tree passes --check` bats case (Unit tests / bats). Regenerate the manifest so the pin matches the file. Digests-only, no signing -- the release workflow re-signs on promotion. Only the install-k8s.ps1 line changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…#830) The manifest-landing test parses the rendered pod YAML with `python3 -c 'import yaml'` but never checked the tooling first, so a box without PyYAML failed with an opaque `ModuleNotFoundError: No module named 'yaml'` instead of a clean skip that names the missing tool. Add a require_yaml_tooling guard called at the top of that one test. The five curl-probe tests use only POSIX sh/awk/sed/grep, so guarding there rather than in setup() keeps them running when only PyYAML is absent — the faithful analogue of chart-pull-secret.bats's require_tool (there every test renders a chart, so it guards in setup(); here only one test needs the tool). CI=true (set by GitHub Actions) turns a missing tool into a hard failure so a required gate can't go green on a skip. Verified with bats 1.14: - without PyYAML: the YAML test skips cleanly naming PyYAML, curl tests pass - with PyYAML: all six tests pass - CI=true without PyYAML: the test fails with an ::error:: annotation Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
LukasWodka
commented
Aug 25, 2026
ContributorAuthor
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 2fa178c. Configure here.
Uh oh!
There was an error while loading. Please reload this page.
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 freeto 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
Touches cluster version pins, default storage topology, job resource envelopes, MySQL credentials, and Helm render-time RBAC lookups — mis-ordering with runtime images or flipping flags on live fleets can break installs, scheduling, or upgrades.
Overview
Release-train promotion bundling chart 1.9.71, a k3s pin bump to v1.36.3-k3s1 (installers, GPU image,
check-facts), and several operator-facing behavior changes.Installers & storage. Linux/k3s now defaults
TB_STORAGE_MODE=node-local(hostpath is opt-in); messaging for leftover data and storage-topology mismatches is updated accordingly. Hostpath installs gain a fail-closed mount probe (bash + Windows) so nodes must seeHOST_DATA_DIRat/traceblocbefore Helm runs. Windows training values use memory-onlyRESOURCE_LIMITS(Get-TrainingLimits, L0.2) and carryRESOURCE_REQUESTSon reinstall;Invoke-BoundedProcessfixes stdin/pipe deadlock and adds optional stdout-only docker output for the probe.Helm chart. Optional
rotateMysqlRoot/mysqlRootPasswordchart-manages MySQL root on fresh datadirs (Secret + env); runbook added.RESOURCE_REQUESTS/RESOURCE_LIMITSare emitted only when set (no injectedcpu=2,memory=8Gion the other side), with tighter values schema and tests — requires client-runtime#388. Image-refresh still skips pinned images but compares digest pins and sets/clearstracebloc.io/stale-pin-*annotations. Resource-monitor metrics-server preflight is skippable vianodeAgents.metricsServerPreflight: falseand records outcome on the DaemonSet; drift guardpreflight-not-privilege-gated.shadded.Docs & ops. README explains k3s pin vs existing clusters; SECURITY/SEAL-CHECK updates; migration tools note rotated root password.
Reviewed by Cursor Bugbot for commit 2fa178c. Bugbot is set up for automated code reviews on this repo. Configure here.