Add node-local OCI image layer cache: overlay-composed actor rootfs from shared unpacked layers (#463 Phase 1)#467
Conversation
…llcache Content-addressed pool of unpacked image layers, shared by every actor on the node; actor rootfs becomes an overlayfs mount (cached layers as read- only lowers, bundle-local upper) instead of a full re-untar per run. atelet (no capabilities) pulls and unpacks; the privileged ateoms finalize whiteouts and mount. Tag refs resolve via one HEAD and become cacheable; pull memory is O(stream buffers); the cache survives restarts. Phase 1 of agent-substrate#463. Fixes agent-substrate#437, agent-substrate#166, agent-substrate#228. Validated: kind + GKE counter demos (gvisor and microvm), suspend/resume (oci_unpack ~3ms vs ~15-20s), 411 SWE-bench-scale images pulled and unpacked with 0 failures, root-gated unit tests for the privileged paths. Known gaps: no GC yet (Phase 2, see internal/imagecache/README.md); upgrade ordering — deploy new ateoms before/with the new atelet.
9450dcf to
8933927
Compare
Benjamin Elder (BenTheElder)
left a comment
Benjamin Elder (BenTheElder)
left a comment
There was a problem hiding this comment.
Quick AI review ... (continuing to refine that experiment on coworker PRs for now, intend to add a skill)
Findings inline: two 🟡 (overlayfs mount-option page-size cap → images beyond ~34 layers can't mount; phantom parent dirs shadowing lower-layer dir metadata in the merged view) and two 🟢 (failure-path unmount asymmetry; record temp files not swept).
Checked and found no issues: unpack confinement (os.Root, tar-name validation, hardlink/symlink replacement) matches the old hardened untar; whiteout capture→materialize split is correct and FinalizeLayer is safely idempotent under concurrent ateoms; lowerdir dedup (topmost wins) is the right fix for the duplicate-layer ELOOP; the tag→digest HEAD resolution only affects non-pinned refs (actor images are digest-pinned by CRD validation); microvm covers both run and restore via buildActorContainers; FIFO/device tar entries still hard-fail but did in the old untar too (pre-existing, not a regression); GOOS=linux build and the unprivileged imagecache tests pass locally.
Drafted with the assistance of an automated review agent; reviewed by a human before submitting.
| if strings.ContainsAny(upper, ":,") || strings.ContainsAny(work, ":,") { | ||
| return "", fmt.Errorf("bundle path %q contains overlay option separators", upper) | ||
| } | ||
| return "lowerdir=" + strings.Join(lowers, ":") + ",upperdir=" + upper + ",workdir=" + work, nil |
There was a problem hiding this comment.
🟡 🤖 No guard on the assembled option-string length. mount(2) copies options via a single page, so the whole string is capped at ~4096 bytes. Layer paths here are ~114 bytes each (/var/lib/ateom-gvisor/image-cache/layers/sha256/<64-hex>/fs:), which caps images at roughly 34 layers — Docker permits 127, and heavily-layered CI/base-chain images in the 30–60 range exist in the wild. The validation corpus topped out at 19 layers, so this wasn't hit; when it is, the failure is an opaque EINVAL from the mount.
This is exactly why containerd's overlay snapshotter uses short integer snapshot IDs. Options: a preflight length check with a clear error (minimum), short per-layer symlinks/IDs in the pool, or the new mount API (fsconfig + lowerdir+ appends, kernel ≥ 6.5) which avoids the single-page limit.
| // lower layers' directories at compose time. | ||
| if parent := filepath.Dir(name); parent != "." { | ||
| if err := root.MkdirAll(parent, 0o755); err != nil { | ||
| return nil, fmt.Errorf("while creating parent directories for %q: %w", name, err) |
There was a problem hiding this comment.
🟡 🤖 Phantom parent dirs shadow lower-layer directory metadata in the merged view. In overlayfs, the top-most layer containing a directory supplies its attributes. A parent auto-created here (root:root 0755) in an upper layer hides the real metadata a lower layer declared — e.g. a layer shipping root/.bashrc without a root/ entry turns /root from 0700 into 0755; a file added under /tmp without a tmp/ entry drops the 1777 sticky bit. The old flattened extract didn't have this (parents declared in any layer kept their modes); the PR notes the omitted-parent case was hit on real GKE images, so this will occur. mknodWhiteout's MkdirAll in bundle_linux.go has the same property.
Not easily fixable without lower-layer context at unpack time (containerd sidesteps it by applying through the mounted parent chain, so copy-up preserves metadata). At minimum worth documenting in the README as a known deviation; a fuller fix could consult the already-cached lower layers of the same image when creating phantom parents.
| // (deleting a bundle out from under a live mount in this namespace would | ||
| // leave the mount orphaned until the pod restarts). Best-effort, same as | ||
| // the container cleanup above. | ||
| if err := imagecache.UnmountAllUnder(ateompath.OCIBundleDir(req.GetActorUid())); err != nil { |
There was a problem hiding this comment.
🟢 🤖 Teardown asymmetry: this detaches bundle overlays after checkpoint, but the Run-failure defer (line ~197) and Restore-failure defer (~390) only clean up the network — overlays mounted by a partially-successful SetupBundleRootfs linger in this namespace until atelet's later bundle wipe lazily detaches them. Adding UnmountAllUnder to those failure defers would make cleanup symmetric.
| // sweepTempDirs removes unpack temp dirs orphaned by a crash. A layer dir | ||
| // without the temp prefix is always complete (it was moved into place with a | ||
| // single rename), so this is the only recovery the pool needs. | ||
| func (s *Store) sweepTempDirs() error { |
There was a problem hiding this comment.
🟢 🤖 This sweeps only orphaned layer temp dirs; writeRecord's temp files (.<digest>.json.tmp-* under manifests/sha256/) orphaned by a crash are never cleaned. Tiny files, but the sweep exists for exactly this class — worth including the manifests dir.
The fixed 30-minute idle guard made nothing evictable while a fast corpus filled a small disk (32-VM hicard sweep: local SSDs filled in ~12 minutes, then every remaining image failed ENOSPC). Expose it as --evict-idle.
2 participants
Summary
Phase 1 of #463: replaces atelet's in-memory flattened-tarball pull cache (
memorypullcache) with a node-local, on-disk, content-addressed pool of unpacked image layers (internal/imagecache), shared by every actor on the node. An actor's rootfs becomes an overlayfs mount — cached layers as read-only lowerdirs, a bundle-local upper for the actor's private writes — instead of a full image re-untar on every run/resume.Fixes #437, fixes #166, fixes #228.
What changes
internal/imagecache(new): layer pool (layers/sha256/<diffid>/fs), digest-keyed image records, tag→digest resolution via oneremote.Head(tag refs become cacheable), parallel streaming layer download+unpack (bounded, singleflight-collapsed per layer and per image), atomic tmpdir+rename with startup recovery, layout version marker. Seeinternal/imagecache/README.mdfor the full design.prepareOCIDirectorycallsEnsureImageand writes a per-bundlerootfs-overlay.json(+ emptyrootfs/,upper/,work/) instead of fetch+untar. New--image-cache-dirflag (default/var/lib/ateom-gvisor/image-cache, on the shared hostPath).memorypullcacheis deleted.SetupBundleRootfs) right beforerunsc create/restore/ virtio-fs lower staging, and detach the mounts at teardown (UnmountAllUnder). Bundles without a spec are left untouched (compatibility with pre-imagecache bundles).tools/validate-image-cache(new): batch-validates that arbitrary registry images can be pulled, parsed, and unpacked by the store half; used to sweep 400+ SWE-bench-scale images during development.run-testsre-runs the imagecache package undersudoso the root-gated tests (mknod whiteouts, trusted xattrs, overlay mount round trip) execute instead of skipping.Deviations from the #463 design — please review deliberately
manifests/ate-install/atelet.yaml). So the module is split along that existing boundary: atelet pulls/unpacks (pure file I/O) and records whiteouts in per-layer metadata; the privileged ateoms materialize whiteouts (mknod+trusted.overlay.opaque) once per layer and mount, in their own mount namespace — which is where runsc's gofer / virtiofsd resolve paths, so no mount-propagation configuration is needed anywhere.containerd/archive.Apply. containerd's applier assumes capabilities atelet doesn't have (e.g. writing children into image-defined read-only dirs relies onCAP_DAC_OVERRIDE). The existing hardened untar already solves that (dir-mode juggling for plain root,os.Rootconfinement, later-entry-wins); it's extended with whiteout capture and missing-parent-dir creation rather than replaced.Behavior preserved
resetActorDirsbetween runs, same contract as the old full re-untar — just at mount cost (ms) instead of extraction cost (tens of seconds).Validation
runsc create/start/restoreall on overlay rootfs;oci_unpack2.9–4.8 ms on resume (was ~15–20 s per Avoid re-untarring actor image rootfs on every restore #166); durable state preserved across suspend/resume.oci_unpackon both runtimes; cache survives atelet restarts.tools/validate-image-cache. Two real-image bugs found on GKE during development are fixed with regression tests: layer tars omitting parent-dir entries, and images listing the same layer twice (overlayfs rejects duplicate lowerdirs withELOOP; compose dedups to the topmost occurrence).Known gaps / notes for reviewers
internal/imagecache/README.mdwith operator guidance.untartracing span no longer exists (untar is gone);prepareOCIDirectoryand RPC spans remain. AnEnsureImagespan with hit/miss attributes is natural Phase 2 material alongside cache metrics.🤖 Generated with Claude Code