feat(chart): route JOB_IMAGE_HOST through tracebloc.tbRegistry so training pods follow the tracebloc registry default (ghcr.io) - #1041
Conversation
…ining pods follow the tracebloc registry default (ghcr.io)
What: both JOB_IMAGE_HOST render sites in jobs-manager-deployment.yaml (the api
container and pods-monitor) now render `{{ include "tracebloc.tbRegistry" . }}/`
instead of `<global.imageRegistry|docker.io>/`. At the chart default the
jobs-manager stamps `ghcr.io/` onto every training image it spawns
(tracebloc/client-<task>-<arch>:<CLIENT_ENV>); every one of those images is
published on GHCR at the same digest as its Docker Hub copy (the GHCR
migration), so this changes where the training pods pull from, not which bytes
run. No new egress: ghcr.io is already required for the control-plane images
(1.9.113) and the ingestor.
Why: the control plane moved to ghcr.io in 1.9.113 while the training host
stayed on a docker.io literal, so one install pulled the two halves of the
product from two registries and a rollback of one did not move the other.
ONE precedence chain, the same helper the control-plane images, the
image-refresh CronJob and NOTES.txt already read: `global.imageRegistry` (a
mirror re-homes everything) wins, else `images.traceblocRegistry`, else the
chart default "ghcr.io". The helper is nil-guarded and `| default`-chained, so
an empty or absent knob renders `ghcr.io/`, never a bare "/".
Rollback is the SAME knob as 1.9.113 and moves the control plane and the
training host together:
helm upgrade <release> tracebloc/client -n <ns> \
--reset-then-reuse-values --set images.traceblocRegistry=docker.io
Mirrors are unaffected: `global.imageRegistry` still wins for JOB_IMAGE_HOST
exactly as before. The client-runtime's own fallback for an UNSET
JOB_IMAGE_HOST changes separately in that project; the chart always sets the
variable, so only installs predating the key ever see that default.
Tests (helm-unittest 0.5.2, one template per test):
jobs_manager_test: default -> "ghcr.io/" on both containers
global_image_registry_test: mirror still wins on both containers; no mirror
-> the tracebloc registry default, not docker.io
tracebloc_registry_test: the "does NOT move ... JOB_IMAGE_HOST" case is
split: the ingestor repository still does not
follow the knob; JOB_IMAGE_HOST now does
(docker.io -> "docker.io/", quay.example ->
"quay.example/"), an EMPTY knob renders the
default (dig-empty trap), and mirror + knob ->
mirror wins. NOTES now says "control-plane +
training images".
Contract text updated to match: _helpers.tpl doc block, values.yaml comment,
values.schema.json description, docs/INSTALL.md (egress table + registry
paragraph), a 1.9.114 MIGRATION.md entry, and scripts/list-images.sh's
derivation note (the task NAMES still come from Docker Hub's repository list,
which GHCR has no anonymous equivalent of; the host prefix is the rendered
JOB_IMAGE_HOST -- dual-published, so the names agree).
Chart 1.9.113 -> 1.9.114 (template change).
Suite counts: helm unittest 733/733 (41 suites; was 729, +5 new, -1
replaced); helm lint --strict clean on aks/bm/eks/oc; helm template x4 renders
"ghcr.io/" at both sites with every control-plane image: line byte-identical to
the base; make drift 48/48 guards; chart-env-vocabulary 177/177.
Mutations (helm unittest client; anchor asserted applied, restored byte-for-byte,
733/733 green after each):
M1 first JOB_IMAGE_HOST site back to the mirror-or-docker.io dig
-> 4 failed / 733 (default on both containers x2 suites, quay.example knob,
EMPTY-knob trap)
M2 both sites hardcoded to "ghcr.io/" (helper ignored)
-> 4 failed / 733 (mirror re-home, docker.io rollback knob, quay.example
knob, mirror-wins-over-knob)
bats: every scripts/tests/*.bats that reads values/templates/helm template
(23 files, 1057 tests) green on the final tree.
|
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 bd48a7d. Configure here.
saqlainsyed007
left a comment
There was a problem hiding this comment.
Code LGTM — reviewed at bd48a7dd, together with its runtime half client-runtime#555. Not approving yet only because CI is still pending (Unit tests, mutation-check, and both Bugbot checks); I'll flip to approve once it's green and Bugbot is clean.
The contract matches its runtime half exactly. Both jobs-manager containers now render JOB_IMAGE_HOST from printf "%s/" (include "tracebloc.tbRegistry" .), so the training-image host rides the same precedence chain as the control-plane images — global.imageRegistry mirror wins, then images.traceblocRegistry, then the ghcr.io chart default — rendering "ghcr.io/", which is byte-for-byte the runtime's DEFAULT_JOB_IMAGE_HOST. Same host, no scheme, same trailing slash; the schema pattern forbids a scheme or trailing slash in the knob, so no double-slash is reachable.
The helm-unittest coverage is thorough and pins the traps that matter: both containers asserted (not just [0]), the empty-knob → default ("ghcr.io/", not a bare /) dig-empty trap, mirror-wins-over-knob, and the docker.io/quay.example rollback values — each with the trailing slash. MIGRATION.md, values.yaml, values.schema.json, INSTALL.md and NOTES.txt are all updated consistently (control-plane + training move together; rollback is the one images.traceblocRegistry=docker.io flag), and the egress story is a no-op since ghcr.io is already required. Chart version bumped 1.9.113 → 1.9.114.
Will re-check on the next pass and approve when green.
saqlainsyed007
left a comment
There was a problem hiding this comment.
Approve — re-reviewed at bd48a7dd, now green. CI is fully passing (Unit tests, mutation-check, both Bugbot checks), no conflict, no open Bugbot/cursor thread, and no standing change-request.
Re-confirmed the contract on green: both jobs-manager containers render JOB_IMAGE_HOST as printf "%s/" (include "tracebloc.tbRegistry" .), which at the chart default is ghcr.io/ — byte-for-byte the runtime's DEFAULT_JOB_IMAGE_HOST in client-runtime#555 (same host, no scheme, same trailing slash; the knob's schema forbids a scheme/trailing slash so no double-slash is reachable). The helm-unittest coverage pins both containers, the empty-knob→default (not bare /), mirror-wins, and the docker.io/quay.example rollback values; MIGRATION.md / values.yaml / values.schema.json / INSTALL.md / NOTES.txt are consistent and the chart is bumped to 1.9.114.
Paired with client-runtime#555 (already approved) this completes the JOB_IMAGE_HOST move to ghcr.io with a single rollback knob. No findings.
saqlainsyed007
left a comment
There was a problem hiding this comment.
Approving — the only thing I was holding on (CI still pending at my last pass) is now green, and Bugbot is clean.
Reviewed at bd48a7dd (unchanged since). The chart now renders JOB_IMAGE_HOST from printf "%s/" (include "tracebloc.tbRegistry" .) on both jobs-manager containers, so the training-image host rides the same precedence chain as the control-plane images (global.imageRegistry mirror → images.traceblocRegistry → the ghcr.io chart default), rendering "ghcr.io/" — byte-for-byte the runtime's DEFAULT_JOB_IMAGE_HOST in its paired half client-runtime#555. Same host, no scheme, matching trailing slash; the schema pattern forbids a scheme/trailing-slash in the knob so no double-slash is reachable. helm-unittest pins the traps that matter (both containers, empty-knob→default, mirror-wins, docker.io/quay rollback), docs/values/schema all move together, version bumped 1.9.114. CI green incl. both Bugbot checks, no open threads, no conflict. Good to land.
saadqbal
left a comment
There was a problem hiding this comment.
Approving. This is the step #1028 deliberately deferred, and it lands the way that PR said it would.
Both JOB_IMAGE_HOST sites are routed, which is the thing I'd have blocked on if only one were: the api container and the second jobs-manager container now both render printf "%s/" (include "tracebloc.tbRegistry" .). Two containers in one Deployment stamping different registry prefixes onto spawned training pods would be a genuinely confusing failure, and the tests pin the symmetry rather than the mechanism — should set JOB_IMAGE_HOST ... on both containers and images.traceblocRegistry moves JOB_IMAGE_HOST on both containers.
The helper's guard is byte-for-byte what I checked on #1028 — both reads still dig-with-"" and the | default chain intact, only the final default moved to ghcr.io. Worth noting the two sites you replaced were themselves the anti-pattern: dig "imageRegistry" "docker.io" (...) used a non-empty dig fallback, so absent and empty behaved differently there. Routing them through the helper fixes that as a side effect of the consolidation.
The test I'd single out is an EMPTY knob renders the default registry into JOB_IMAGE_HOST, not a bare "/" (the dig-empty trap). That's the same trap as on the control plane, but the consequence is different here — not an empty registry prefix, a malformed image reference (/tracebloc/client-<task>:<env>), which fails at pull rather than pulling the wrong thing. Recognising that the same root cause needs its own assertion at this call site, because what it breaks is different, is the part that generalises.
Two more things worth keeping. The "NOT routed through here" list had — for now — the training-image host JOB_IMAGE_HOST, which moves in its own step removed in the same PR that made it false, and the ingestor added with its reason (named by full repository, follows only the mirror). That's a carve-out list staying true rather than aging into a lie — the failure I've hit three times elsewhere today. And the trailing slash is handled at the call site with the reason stated ("the runtime concatenates <host><repo>:<tag> verbatim"), so the helper keeps returning a bare host and there's no // hazard.
54 checks passing, nothing pending, no unresolved threads, CLEAN. One knob now rolls the control plane and the training host back together, which is what the per-edge rollback needed to mean.
Summary
Routes the training-image host
JOB_IMAGE_HOSTthrough the existingtracebloc.tbRegistryhelper, on both jobs-manager containers. The jobs-manager stamps this prefix onto every training image it spawns (tracebloc/client-<task>-<cpu|gpu>:<CLIENT_ENV>); until now the two render sites had their own chain (global.imageRegistry, elsedocker.io/). They now share the control-plane images' chain: aglobal.imageRegistrymirror wins, thenimages.traceblocRegistry, then the chart default —ghcr.iosince #1038. Third and last chart step of the GHCR migration: the control plane (1.9.113) and the training pods (this, 1.9.114) pull from the same registry and cannot be pointed at different ones.Why. Every training image is published to GHCR at the same digests as its Docker Hub copy, so this changes where the training pods pull from, not which bytes run. One knob now rolls the control plane and the training host back together (
--set images.traceblocRegistry=docker.io), which is what a per-edge rollback should do.Unchanged:
tracebloc/mysql-client, busybox and the other third-party images; the ingestor (named by full repository, already onghcr.io, follows only the mirror). The runtime's own fallback for an unsetJOB_IMAGE_HOSTchanges separately in client-runtime; the chart always sets the variable, so only pre-key installs see that default.Docs. New "Upgrading to 1.9.114" entry in
MIGRATION.md(no new egress, one jobs-manager rollout then one pull per task, digest-pinned spawn path unaffected, mirrors unaffected, rollback + verification one-liners);INSTALL.mdegress table and registry paragraph; the helper's doc block,values.yamlcomment,values.schema.jsondescription, NOTES line, andscripts/list-images.sh's derivation note (task names still come from Docker Hub's repository list, which GHCR has no anonymous equivalent of; the host prefix is the renderedJOB_IMAGE_HOST).Test plan
helm unittest client: 41 suites / 733 tests (was 729: +5 new, −1 replaced).helm lint --stricton aks/bm/eks/oc + ingestor green.helm template× 4 platforms:JOB_IMAGE_HOST="ghcr.io/"on both containers, every control-plane image line byte-identical to the base.make drift48/48; the 23scripts/tests/*.batsfiles that read values/templates/render, 1057 tests, green;chart-env-vocabulary.sh177/177;chart-version-guard.shbumped 1.9.113 → 1.9.114.images.traceblocRegistry: docker.io→docker.io/(the rollback) andquay.example→quay.example/; an EMPTY knob renders the default, not a bare/(the dig-empty trap); mirror + knob → mirror wins; the ingestor repository still does not follow the knob (that case was split off the old combined test).JOB_IMAGE_HOSTsite back to the mirror-or-docker.iodigquay.exampleknob, EMPTY-knob trap"ghcr.io/", helper ignoreddocker.iorollback knob,quay.exampleknob, mirror-wins-over-knobChecklist
Note
Medium Risk
Changes where every new training job pulls its image after upgrade (jobs-manager rollout + first pull per task); mirrors and digest-pinned spawns are unchanged, but misconfigured egress or expecting Docker Hub-only training pulls without rollback could break experiments.
Overview
Chart 1.9.114 completes the GHCR migration for training workloads by wiring
JOB_IMAGE_HOSTon both jobs-manager containers throughtracebloc.tbRegistryinstead of a separateglobal.imageRegistry→docker.io/chain. Training pods therefore default toghcr.io/(with the same precedence as control-plane images: mirror, thenimages.traceblocRegistry, then chart default), and control plane plus training host cannot diverge on registry.Docs and operator surfaces are updated to match: new MIGRATION.md section for 1.9.114 (egress, rollout, rollback), INSTALL.md egress/registry text,
images.traceblocRegistrydescriptions in values/schema, NOTES.txt wording, andlist-images.shnotes that task repo names still come from Docker Hub while the host prefix follows the renderedJOB_IMAGE_HOST. Helm unit tests assert defaults, rollback knob, mirror precedence, and both containers.Reviewed by Cursor Bugbot for commit bd48a7d. Bugbot is set up for automated code reviews on this repo. Configure here.