Repository navigation
fix(ci): pull the KWOK images once per run instead of once per job - #2497
Merged
Merged
Conversation
The KWOK lanes need two public images -- an in-cluster OCI registry and Gitea for the *-git deployers -- and every matrix job pulled them itself. In run 33350102144, 127 concurrent jobs pulled registry:3.1.1 at the same moment from the shared runner egress IPs; 125 succeeded and the one that was shed reddened main. That is a per-IP throttle, not an outage, so retrying harder cannot fix it: #2496 narrowed the window, it did not close it. Pull each image once, in a prime-images job, and carry it to the matrix as a tarball in actions/cache. Each lane loads from disk, and preload_image then finds the image already in the host Docker cache and never contacts the registry. On a warm cache not even the prime job pulls. Priming is a hard gate: if an image is genuinely unreachable, failing one job in a minute beats 127 jobs each burning a rollout timeout. Loading is best effort -- a miss falls back to preload_image's pull, and the kubelet pull behind that -- so a cold cache or a fork PR degrades to today's behavior rather than failing. A miss does emit a ::warning::, so a cache key that ever drifts from the prime job's cannot leave every lane pulling again, green, with the fix silently inert. Also fixes a false green in the summary job: a failed prime leaves every tier "skipped", and the tier checks deliberately tolerate "skipped", so without an explicit check the run would report success having validated nothing. The retry loop moves out of preload_image into preload_pull_retry so the prime job reuses the schedule tuned against the observed throttle rather than growing a second copy. preload_image's behavior is unchanged; its existing suite passes unmodified. Signed-off-by: Mark Chmarny <mark@chmarny.com>
mchmarny
added
the
theme/ci-dx
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughThe change adds a Bash image-cache library with bounded save and load operations, pinned-image settings, cache-key generation, and CLI commands. It extracts deadline-based pull retries from Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to This change moves KWOK image pulls into a dedicated CI priming step and reuses cached tarballs across matrix jobs; no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Coverage Report ✅
Coverage BadgeNo Go source files changed in this PR. |
mchmarny
marked this pull request as ready for review
August 31, 2026 17:48
This was referenced Aug 31, 2026
10 of 25 tasks
mchmarny
added a commit
that referenced
this pull request
Sep 28, 2026
The argocd-* KWOK lanes pull the Argo CD chart's Redis anonymously at job time from ECR Public, which sheds the request under the per-IP throttle the matrix already trips. Six lanes died on `429 Too Many Requests` for ecr-public.aws.com/docker/library/redis:8.2.3-alpine in run 36290149406, and every scheduled run since 2026-09-25 has been red the same way. #2497 removed the two images install-infra.sh preloads from the critical path; this covers the one the deployer stack pulls for itself, out of the same ECR Public namespace the registry image was already cached from. The chart's quay.io and ghcr.io images are left to the kubelet, neither having been observed shedding a pull. Follows the #2483 machinery: prime-images pulls once per run and caches the tarball, each argocd-* lane restores and loads it, and install_argocd side- loads it into the Kind node before Helm's 5m --wait. The pin carries no renovate annotation on purpose. testing_tools is renovate-managed, so an annotated entry would be bumped to the newest Redis independently of the chart, caching an image nothing pulls and restoring the 429. Nothing gates the pin against the chart, so a failed install now renders the chart and reports the mismatch by name instead of leaving the next reader to re-derive it from an ImagePullBackOff. Pinning the ref surfaces ecr-public.aws.com to tools/registry-inventory, which gates structured sources only. This is not new egress: the chart has always pulled that host in-cluster, but as a rendered chart image it sat outside the gate's scope. Allowlisted with that justification, and docs/contributor/registry-egress.md regenerated to match. Fixes: #2501 Signed-off-by: Mark Chmarny <mark@chmarny.com>
mchmarny
added a commit
that referenced
this pull request
Sep 28, 2026
The argocd-* KWOK lanes pull the Argo CD chart's Redis anonymously at job time from ECR Public, which sheds the request under the per-IP throttle the matrix already trips. Six lanes died on `429 Too Many Requests` for ecr-public.aws.com/docker/library/redis:8.2.3-alpine in run 36290149406, and every scheduled run since 2026-09-25 has been red the same way. #2497 removed the two images install-infra.sh preloads from the critical path; this covers the one the deployer stack pulls for itself, out of the same ECR Public namespace the registry image was already cached from. The chart's quay.io and ghcr.io images are left to the kubelet, neither having been observed shedding a pull. Follows the #2483 machinery: prime-images pulls once per run and caches the tarball, each argocd-* lane restores and loads it, and install_argocd side- loads it into the Kind node before Helm's 5m --wait. The chart renders one distinct Redis ref with redis-ha off, and the argocd-redis container is imagePullPolicy: IfNotPresent, so the side-load is what the kubelet uses. The pin carries no renovate annotation on purpose. testing_tools is renovate-managed, so an annotated entry would be bumped to the newest Redis independently of the chart, caching an image nothing pulls and restoring the 429. Nothing gates the pin against the chart, so a failed install renders the chart and reports the mismatch by name instead of leaving the next reader to re-derive it from an ImagePullBackOff. That render is bound by timeout: it re-fetches the chart the install just failed on, and an unbounded stall there would replace exit 30 with a job cancellation. Pinning the ref surfaces ecr-public.aws.com to tools/registry-inventory, which gates structured sources only. This is not new egress: the chart has always pulled that host in-cluster, but as a rendered chart image it sat outside the gate's scope. Allowlisted with that justification, and docs/contributor/registry-egress.md regenerated to match. kwok/README.md described the cache as covering two images; it now covers three, with the Redis pin's provenance and the per-lane restore gating. Fixes: #2501 Signed-off-by: Mark Chmarny <mark@chmarny.com>
11 of 25 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Pull the two public images the KWOK lanes need once per run, in a dedicated priming job, and carry them to the matrix as tarballs in
actions/cache— instead of having every one of ~127 matrix jobs pull them itself.Motivation / Context
It is a per-IP throttle, not an outage, and not something retrying can fix. In run 33350102144:
125 jobs pulled
public.ecr.aws/docker/library/registry:3.1.1successfully at the same moment, from the same runner pool, in the same run. That rules out an outage, a bad pin, and a broken image. What is left is anonymous per-IP rate limiting under a 127-job concurrent matrix — which also explains the 2026-08-30 streak where a different lane failed each run.#2496 fixed the retry schedule (it was giving up in ~17s while holding a 180s budget). That narrowed the window; it could not close it, because no retry schedule fixes a quota shared across the whole matrix. The fix is to stop making 127 requests.
So: one pull, one tarball, 127 loads from disk.
preload_imagealready checks the host Docker cache before every pull attempt, so an image loaded from the tarball means the lane never contacts the registry at all.Fixes: N/A
Related: #2483, #2496, #2480, #843
Deliberately
Related, not a closing reference. This meets the first two of #2483's acceptance criteria — no unauthenticated third-party pull at job time, and the pull failure reason in the log (that one landed in #2496). The third is "several consecutive green KWOK runs onmain", which can only be met by observation: with a 1-in-127 intermittent failure, one green run proves nothing. #2483 stays open until the runs accumulate.Type of Change
Component(s) Affected
cmd/aicr,pkg/cli)cmd/aicrd,pkg/server)pkg/recipe)pkg/bundler,pkg/component/*)pkg/collector,pkg/snapshotter)pkg/validator)pkg/errors,pkg/k8s)docs/,examples/)kwok/scripts/lib,.github/workflows/kwok-recipes.yaml,.github/actions/kwok-testImplementation Notes
kwok/scripts/lib/image-cache.sh(new). Verbskey/file/save/load/settings. Sourceable by its unit harness with stubbeddocker, executable by the workflow. Both workflow sites derive the cache key fromimage_cache_keyhere rather than from a value threaded through the reusable workflow — if the two could disagree, every restore would miss and the cache would silently do nothing while reporting success.saveis a gate,loadis best effort. The asymmetry is deliberate.saveruns once so the matrix never touches the registry, so a save that reports success without producing a usable tarball is worse than no cache at all — it sends all 127 jobs back to the pull with the gate lit green. Every failure path returns non-zero.loadkeepspreload_image's contract: a miss returns non-zero for the caller to shrug at, because the pull path is still intact behind it. A cold cache or a fork PR whose cache scope differs must degrade to today's behavior, not fail.A miss is annotated, not silent. The load step emits
::warning::on a miss. Without it, a cache key that ever drifts from the prime job's would leave every lane pulling again, green, with the fix inert and nothing to notice it by.Partial tarballs are never published.
savewrites to scratch and publishes with a rename. Adocker savekilled partway (cancelled job, reclaimed runner) leaves bytes on disk; writing straight to the final path would letactions/cacheupload that stump, every later run would take it as a hit, anddocker loadwould fail in all 127 jobs — with the prime job green, because it did pull the image.One prime job per image.
actions/cachesaves at the end of a job only when that job succeeds, so a single job priming both would discard a registry pull that had already worked if Gitea failed. Gitea is also restored only for the*-gitlanes, which are the only onesinstall-infra.shinstalls it for.Summary false-green fix. A failed prime leaves every tier
skipped, and the tier checks deliberately tolerateskipped(that is how a schedule-only run with no recipes passes). Without an explicit check on the prime result, the whole run would have reported green having validated nothing.preload_pull_retryextraction. The retry loop moves out ofpreload_imageso the prime job reuses the schedule tuned against the observed throttle in #2496 rather than growing a second copy.preload_image's behavior is unchanged — its existing suite passes unmodified, which is the regression guard.discovertimeout 5 → 10 min. The script-test block now runs ~90s locally before classification starts; those suites are deliberately wall-clock bound (they assert a hanging pull stays inside its deadline, which costs real seconds). A discovery job that times out would be exactly the self-inflicted red this PR exists to remove.Testing
All five KWOK script suites green (130 cases). The new suite is wired into the
discoverjob alongside the existing four.The new suite was mutation-tested. Four mutations of the implementation; three were caught immediately:
docker loadpresence verifyload-verifies-the-image-is-presentkey-changes-with-the-tag,key-changes-with-the-registrysavereturns 0 when the pull never landssave-pull-exhausted-fails,save-hanging-pull-failsThe fourth exposed a real gap: the partial-save case let
docker saveexit normally, so the function's ownrm -fcleaned up the stump and a non-atomic implementation passed anyway. Added a case that kills the process mid-save, where no cleanup runs — that mutation now fails two assertions. Both cases are kept, and the one that only bites under a kill says so in a comment.No Go source changed, so the coverage gate does not apply.
Risk Assessment
CI-only. Every new path degrades to current behavior on failure except the prime job itself, which is a deliberate gate. Reverting the two workflow files restores today's behavior with no other cleanup.
Not exercised locally: no Docker on the authoring machine, so the real
docker save/docker loadround-trip is covered by stubs only. The flags are standard, but the first CI run is the real proof — that is part of why this is a draft.Rollout notes: The first run after merge is a cold cache: the prime job pulls, and the matrix restores from the cache it just wrote. Local runs (
make kwok-test-all) are unaffected and still pull throughpreload_image.Checklist
make testwith-race)make lint)git commit -S)