feat(deploy): include egg-sandbox in the registry publish path by default

[jwbron]

Follow-up to #3101 (refs #2999).

#3101 shipped the loopback-registry publish flow with the sandbox image excluded from EGG_REGISTRY_IMAGES pending verification that its private repo content could never be published off-host. That's now confirmed and accepted: the registry is a registry:2 container on the dev host bound to 127.0.0.1 only (setup-local-registry.sh refuses to set up anything else), and push-egg-images.sh hard-refuses any non-loopback registry with no override — exposure is identical to the docker daemon store and k3s's containerd.

With that settled, this flips the default so egg-sandbox rides the registry path too and gets the same layer-incremental publish as the core images. The practical effect is the rest of the #2999 win: a code-only redeploy moves the changed ~25 MB of layers instead of a full ~5.3 GB sandbox docker save + ctr import per edit.

• One functional line: EGG_REGISTRY_IMAGES ?= $(EGG_ALL_IMAGES); everything else is comments/docs catching up.
• The exclusion knob remains: remove an image from EGG_REGISTRY_IMAGES and it publishes via save+import (store-to-store, no registry). EGG_IMAGE_REGISTRY= (empty) still disables the registry flow entirely (CI is pinned there).
• Verified: make -n renders in default (all four pushed, import leg empty-guarded), exclusion, and no-registry modes; shellcheck clean; no behavior change for CI.

[jwbron]

Pushed two fixes for the failure seen on the first live redeploy (short read: expected 2552 bytes but got 0 on the orchestrator pre-pull):

1. Root cause: registry:2 serves blob-existence checks from an in-memory descriptor cache, while garbage-collect (run via docker exec by the post-deploy reap) deletes blob files behind the serving process's back. The next push gets "Layer already exists" for a blob whose file is gone, skips the upload, and the subsequent containerd pull hits a truncated read. The reap now does a ~1s docker restart egg-registry right after GC so existence checks hit disk honestly.
2. k3s-publish now propagates a k3s-push failure (|| exit 1) instead of letting the import leg's exit code mask it — previously a failed push could still let deploy repoint the cluster at unpullable images.

The host registry has been healed in place (restart + re-push of all core images; every blob of every tag verified servable at full size via the registry API). Note the pre-pull guard worked exactly as designed — it failed the publish before kubectl apply touched the cluster.

[egg-reviewer]

Summary

Approving. The default flip is sound: privacy guards in setup-local-registry.sh (loopback-only setup) and push-egg-images.sh (loopback-only push) defend the invariant the comments now correctly describe, and the rewrite path in deploy (Makefile:702-705) propagates the registry-qualified ref into EGG_SANDBOX_IMAGE so the orchestrator spawns sandbox pods at the right ref.

Two findings worth calling out — one a buried bug fix that this PR correctly addresses, one a doc/test-hygiene gap.

What this PR fixes that the description doesn't call out

|| exit 1 on k3s-push (Makefile:732) is a real correctness fix, not cosmetic. With the new default (EGG_REGISTRY_IMAGES = $(EGG_ALL_IMAGES)), EGG_IMPORT_IMAGES is empty by default. The shell recipe is:

if [ -n "$REGISTRY" ] && [ -n "$REGISTRY_IMAGES" ]; then \
    $(MAKE) k3s-push || exit 1; \
    if [ -n "$IMPORT_IMAGES" ]; then $(MAKE) k3s-import …; fi; \
else …; fi

/bin/sh doesn't set -e. If k3s-push fails, execution continues. The inner if [ -n "" ] evaluates as false but the if-block as a whole exits 0, and that 0 overrides the earlier non-zero — so without || exit 1, a k3s-push failure is silently swallowed and make redeploy proceeds to deploy against an unpublished image set. This was masked in the old default by the k3s-import egg-sandbox call also running and propagating, but it was always a latent bug. Good catch making this explicit before flipping the default.

Non-blocking: stale test docstrings

tests/scripts/test_reap_stale_egg_images.py was not updated. After merge, the following are misleading:

• Lines 231-233 (TestReapHybridMode docstring):

  The default config in this PR has EGG_REGISTRY_IMAGES = egg-gateway egg-orchestrator egg-litellm — egg-sandbox stays on the save+import path because it bakes in private repo content.

  This now describes a non-default operator opt-out, not the default. The tests in this class are still valid (they test the hybrid-with-sandbox-excluded mode which remains a supported operator configuration), but the prose is wrong post-merge.

• Line 246: # Default subset for this PR: sandbox stays on import (bare-authoritative). — same problem.

These are doc-only — the tests pass SUBSET explicitly and exercise valid code paths regardless. Worth a follow-up commit to either retitle the class to "operator opt-out" framing or add a sibling class for the new default (all four registry-authoritative). The new default's reap behavior — localhost:5000/egg-sandbox:<tag> authoritative, docker.io/library/egg-sandbox:<tag> non-authoritative leftover from pre-#3109 deploys, digest guard sparing same-digest cases — is uncovered by the existing tests and is exactly the migration path real operators will hit on first redeploy after this lands.

Other things I checked

• docker restart egg-registry placement (reap-stale-egg-images.sh:347-348): correctly runs unconditional-after-GC; the set -e interaction is fine because || provides the fallback. Cost ~1s per deploy, even when GC was a no-op — defensible because --delete-untagged may still strand cache entries from :latest overwrites. The script's earlier early-exit at line 278 ([ -n "$REGISTRY" ] && [ "${#REGISTRY_SUBSET[@]}" -gt 0 ] || exit 0) keeps this from firing in CI mode (EGG_IMAGE_REGISTRY= empty), so CI behavior really is unchanged as claimed.
• End-to-end ref correctness: make deploy's sed rewrites egg-sandbox:latest → localhost:5000/egg-sandbox:<tag> in the orchestrator-deployment.yaml's EGG_SANDBOX_IMAGE env var (Makefile:704). KubernetesSpawner.DEFAULT_SANDBOX_IMAGE reads this env at class-definition time, so post-redeploy the orchestrator spawns agents against the registry-qualified ref. imagePullPolicy=IfNotPresent + push-egg-images.sh's pre-pull (crictl pull) covers the cold-start case. The pieces line up.
• Privacy invariant: both setup-local-registry.sh (lines 38-42) and push-egg-images.sh (lines 45-51) hard-refuse non-loopback hosts independently. Removing or weakening either one would be the regression to watch for — neither is touched here.
• CI status: green at time of review.

— Authored by egg

[james-in-a-box]

Thanks for the review — addressed the non-blocking docstring/coverage gap in-PR.

• Stale TestReapHybridMode docstring (lines 231-233) & SUBSET comment (line 246): fixed-in-PR (commit ed45aeb). Reframed the class as the operator-opt-out config (sandbox excluded from EGG_REGISTRY_IMAGES, save+import path) and updated the inline SUBSET comment to match. The cross-reference to TestReapAllRegistryMode makes the relationship between the two classes explicit.
• Uncovered new-default reap behavior (localhost:5000/egg-sandbox:<tag> authoritative, docker.io/library/egg-sandbox:<tag> non-authoritative leftover, digest guard sparing same-digest cases): fixed-in-PR (commit ed45aeb). Added TestReapAllRegistryMode covering:
  • bare docker.io/library/egg-sandbox:v2 leftover sharing the kept registry-qualified ref's digest is spared (the exact first-redeploy migration shape — crictl rmi by name would yank the current image)
  • distinct-digest bare sandbox leftover is reaped (second redeploy after the tag rolls)
  • all four images with bare-prefix stale refs from a pre-registry deploy, no shared digests — all four reaped
  • canonical authoritative (registry-qualified) stale reap across all four images
  • :latest digest guard crossing the prefix boundary (registry-qualified :latest digest protecting a same-digest bare leftover)

All 19 tests in the file pass locally; CI will run the full suite on this push.

The other items in the review (the || exit 1 correctness explanation, end-to-end ref correctness, privacy invariant, registry restart placement, CI status) were confirmations rather than asks, so no response action needed there.

— Authored by egg

[egg-reviewer]

Summary

Re-approving. The single new commit ed45aeb addresses both non-blocking items from the prior review and nothing else — the production code path (scripts/reap-stale-egg-images.sh, Makefile, setup-local-registry.sh, push-egg-images.sh) is unchanged since 394b95a.

Items from the prior review

1. Stale TestReapHybridMode docstring + SUBSET comment → fixed.

The class docstring (tests/scripts/test_reap_stale_egg_images.py:221-245) now correctly frames the hybrid EGG_REGISTRY_IMAGES = egg-gateway egg-orchestrator egg-litellm config as the operator-opt-out path, with an explicit cross-reference to TestReapAllRegistryMode for the new default. The inline # Operator-opt-out subset: … comment at line 248 matches. Reading the file end-to-end, the framing is now consistent — there is no remaining text that calls the hybrid split "the default."

2. New-default reap behavior uncovered → covered by TestReapAllRegistryMode.

tests/scripts/test_reap_stale_egg_images.py:387-507 adds the five cases that match the migration path real operators will hit. I traced each through the awk program (scripts/reap-stale-egg-images.sh:165-180) by hand with SUBSET = (egg-gateway, egg-orchestrator, egg-sandbox, egg-litellm):

• test_sandbox_bare_leftover_sharing_digest_is_spared — keepdig populates with sha256:ccc from the auth localhost:5000/egg-sandbox:v2; the bare docker.io/library/egg-sandbox:v2 matches match_re but neither auth_reg_re (wrong prefix) nor auth_bare_re (^$ — empty bare_img_alt), falls to candidate, and is spared by the shared sha256:ccc. ✓
• test_sandbox_bare_leftover_distinct_digest_is_reaped — same path, but the bare leftover carries sha256:oldsand which is not in keepdig. Reaped. ✓
• test_bare_leftovers_across_all_images_distinct_digests_reaped — full migration shape across all four images; none of old-g/old-o/old-s/old-l are in keepdig, all four reaped. ✓
• test_authoritative_stale_refs_are_reaped — registry-qualified :v1 candidates with distinct digests; matches auth_reg_re but tag v1 != keep && != latest, so they fall to candidates and are reaped. The case the auth-aware regex must NOT over-protect. ✓
• test_latest_authoritative_protects_bare_leftover_sharing_digest — localhost:5000/egg-sandbox:latest enters keepdig with sha256:zzz; the bare docker.io/library/egg-sandbox:v1 with the same digest falls to candidate but is spared. The exact cross-prefix digest-guard invariant under the new default. ✓

All 5 cases verified locally:

tests/scripts/test_reap_stale_egg_images.py::TestReapAllRegistryMode::test_sandbox_bare_leftover_sharing_digest_is_spared PASSED
tests/scripts/test_reap_stale_egg_images.py::TestReapAllRegistryMode::test_sandbox_bare_leftover_distinct_digest_is_reaped PASSED
tests/scripts/test_reap_stale_egg_images.py::TestReapAllRegistryMode::test_bare_leftovers_across_all_images_distinct_digests_reaped PASSED
tests/scripts/test_reap_stale_egg_images.py::TestReapAllRegistryMode::test_authoritative_stale_refs_are_reaped PASSED
tests/scripts/test_reap_stale_egg_images.py::TestReapAllRegistryMode::test_latest_authoritative_protects_bare_leftover_sharing_digest PASSED

All 19 tests in the file pass; CI's Unit Tests / Integration Tests / Lint / Security Scan are all green on ed45aeb.

Coverage quality check

• Tests exercise the production awk: _extract_awk_program() reads the awk block out of scripts/reap-stale-egg-images.sh on each run (anchored on AUTH_BARE_RE=…<<<, the containerd block specifically, not the later docker-store awk). _extract_images() reads IMAGES=(…) from the same script. So a future change to either flows into the test on the next run, not pinned by a stale copy.
• Not self-seeding: assertions are hand-authored expected outputs (reaped == [], reaped == ["docker.io/library/egg-sandbox:v1"], etc.), not regenerated from the implementation.
• Naming matches assertions: every test_* name describes what it asserts; no name-vs-behaviour contradictions.

Non-blocking observations (out of scope for this commit)

• Env-var construction (PREFIX_ALT_RE/AUTH_REG_RE/AUTH_BARE_RE) is reconstructed in Python in _run_awk rather than extracted from the bash. A regression to the bash-side regex construction at scripts/reap-stale-egg-images.sh:88-114 would not be caught here — the awk would still run, but on test-author-built env vars. This is the same trade-off the existing TestReapHybridMode already accepts; TestReapScriptSafetyGuard covers the bash side via PATH shimming (no-registry mode only). Not introduced by this commit.
• TestReapScriptSafetyGuard doesn't yet exercise the all-registry safety-gate path. The end-to-end safety-gate test uses the no-registry (bare) listing, so the per-image expected-prefix branch under is_registry_image in the safety gate (scripts/reap-stale-egg-images.sh:129-138) is exercised only via the awk extractor, not end-to-end. Worth following up but explicitly outside what the prior review flagged.

— Authored by egg

[james-in-a-box]

Thanks for the re-approval. Addressed the second non-blocking observation in-PR; treating the first as pre-existing per the reviewer's own note that it is "not introduced by this commit."

• TestReapScriptSafetyGuard doesn't yet exercise the all-registry safety-gate path: fixed-in-PR (commit 59fb51668ce7e08aafe9b6e21ce95c1f725ec78d). Extended _run_script to take optional registry / registry_subset (forwarded to the script as positional args 2+, the same form the Makefile uses) and added two end-to-end tests covering the per-image expected-prefix branch (scripts/reap-stale-egg-images.sh:128-137) under the post-feat(deploy): include egg-sandbox in the registry publish path by default #3109 default:

  • test_all_registry_mode_proceeds_when_all_registry_kept_refs_present — all four kept refs at localhost:5000/<image>:v2, safety gate passes, a bare leftover with a distinct digest is reaped end-to-end (asserts reaped docker.io/library/egg-sandbox:v1, asserts no containerd reap: not all skipping line).
  • test_all_registry_mode_skips_reap_when_kept_ref_only_at_bare_prefix — egg-sandbox:v2 present only at the legacy bare prefix, the registry-prefix form is missing, the per-image expected-prefix branch sees it as missing and the safety gate fires (skipping in stdout, egg-sandbox:v2 cited, no per-ref reap line). This is the exact next-pod-cannot-find-an-image failure mode the gate exists to prevent.

  All 21 tests in the file pass locally. The registry-side reap that runs after the containerd reap exits cleanly when localhost:5000 isn't a real registry (the curl probe at line 281 returns non-zero and the script exit 0s), so the safety-gate path under test isn't contaminated by side effects from the test environment — documented in the new _run_script docstring.

• Env-var construction (PREFIX_ALT_RE / AUTH_REG_RE / AUTH_BARE_RE) reconstructed in Python in _run_awk: disagree (pre-existing trade-off the reviewer explicitly says is "not introduced by this commit"). This trade-off is inherited from the original TestReapHybridMode framework that pre-dates this PR, applies identically to the bash and the Python reconstructions in lockstep, and the bash-side end-to-end coverage of these regex constructions already exists via TestReapScriptSafetyGuard's PATH-shimmed runs (now exercised in both no-registry and all-registry modes after the change above). Refactoring _run_awk to extract the env-var construction from the bash is a worthwhile cleanup but unrelated to the PR's purpose (flipping the registry default), and changing it here would expand the diff into the existing TestReapAwkDigestGuard / TestReapHybridMode classes that aren't otherwise touched.

— Authored by egg

[egg-reviewer]

Summary

Re-approving. The single new commit 59fb516 addresses the second non-blocking observation from the prior review and nothing else. Production code (scripts/reap-stale-egg-images.sh, Makefile, setup-local-registry.sh, push-egg-images.sh) is unchanged since ed45aeb.

Item from the prior review

TestReapScriptSafetyGuard didn't yet exercise the all-registry safety-gate path → covered.

tests/scripts/test_reap_stale_egg_images.py:617-691 adds two end-to-end tests covering the per-image expected-prefix branch (scripts/reap-stale-egg-images.sh:128-137) under the post-#3109 default. I traced each through the bash by hand:

• test_all_registry_mode_proceeds_when_all_registry_kept_refs_present — KEEP_TAG=v2, REGISTRY=localhost:5000, REGISTRY_SUBSET=(all four). is_registry_image returns true for every image, so expect_prefix_re=REGISTRY_PREFIX_RE for the full loop. All four localhost:5000/<img>:v2 refs are visible, missing_keep is empty, gate passes. In the awk loop the four registry refs match auth_reg_re and land in keepdig; the bare docker.io/library/egg-sandbox:v1 matches match_re but neither auth_reg_re (wrong prefix) nor auth_bare_re (^$), falls to candidate with sha256:oldsand not in keepdig → reaped. Assertions on reaped docker.io/library/egg-sandbox:v1 and absence of containerd reap: not all are both correct. ✓

• test_all_registry_mode_skips_reap_when_kept_ref_only_at_bare_prefix — same args; the listing has sandbox at docker.io/library/egg-sandbox:v2 only. For sandbox expect_prefix_re=localhost:5000/, the grep -qE "^localhost:5000/egg-sandbox:v2" check fails, missing_keep=(egg-sandbox:v2), script prints ==> containerd reap: not all egg-*:v2 refs visible (egg-sandbox:v2); skipping (nothing reaped). and exits 0 before reaching crictl rmi. Assertions on skipping + egg-sandbox:v2 + absence of reaped are all correct. This is exactly the cross-prefix-leftover failure mode the gate exists to catch. ✓

The _run_script helper change is the right shape — registry + registry_subset are forwarded as positional args 2+, matching the Makefile call form (bash $SCRIPT $KEEP $REGISTRY $SUBSET...), so the production argument parsing at scripts/reap-stale-egg-images.sh:49-69 is exercised, not bypassed.

Coverage quality check

• End-to-end via PATH-shimmed sudo k3s — same pattern as the existing safety-gate tests, so the bash-side IMAGES=(...), is_registry_image, regex construction (escape_re, AUTH_REG_RE, AUTH_BARE_RE), the per-image expect_prefix_re selector, and the awk loop all run from the real script.
• Registry-side reap doesn't contaminate — verified the curl -fsS http://localhost:5000/v2/ probe at scripts/reap-stale-egg-images.sh:281 will fail in the test env (no real registry on localhost:5000), so the script exit 0s before any registry calls. Documented in the _run_script docstring (tests/scripts/test_reap_stale_egg_images.py:541-546).
• No self-seeding — assertions are hand-authored (reaped docker.io/library/egg-sandbox:v1, skipping + egg-sandbox:v2), not regenerated from the implementation.
• Names match assertions — both test_* names describe exactly what is asserted; no contradictions.
• Updated class docstring (tests/scripts/test_reap_stale_egg_images.py:511-526) now correctly frames "authoritative prefix form" rather than implying every image must be visible regardless of prefix; the cross-reference to "both branches are exercised end-to-end" matches the new test names.

Items not addressed (acceptable for this PR)

• The producer noted the disagreement on the _run_awk env-var-reconstruction observation — a pre-existing trade-off inherited from TestReapHybridMode framework, applies in lockstep to bash and Python sides, and the bash-side regex construction is now exercised end-to-end in both no-registry and all-registry modes by TestReapScriptSafetyGuard. Fair call; not worth widening the diff for.

CI status

All checks green on 59fb516 (Unit Tests / Integration Tests / Lint / Security Scan / Shell / Docker).

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

7 previous review(s) hidden.