feat(sandbox): cross-node golden dependency cache (L2 archive) - #5351
Conversation
Validated on a real cluster — it works, and it turned up two thingsRancher Desktop (k3s v1.25.16), image built from this branch's Dockerfile. The image change is realRound trip in the real image (two containers, shared store)4000 files / ~32 MB of incompressible content, driving the actual Round trip on the cluster (two pods, one RWX PVC)Verified the restorer really was the tenant shape, not an accident: This settles the open risk in the PR description: a tenant can restore through a read-only mount. The security posture doesn't cost us the feature. Same archive key derived in both environments ( Finding 1 — a fresh volume breaks publish, silently (fixed in b2e8f80)First cluster run died on
Finding 2 — the fd-to-fd fix had no test (fixed in cbfc84e)
Added a 1500-file / ~9 MB round trip asserting the exact member count and the contents of the last file — the tail is what a truncated stream loses, and the observed failure delivered 532 of 1069 members with both children exiting 0. Verified as a guard rather than just an addition:
Still not provenSingle node, so this is cross-cache, not cross-node: pod B had an empty local cache and restored from the shared volume alone, which is the mechanism — but NFS metadata latency and a cold-node herd hitting one EFS filesystem at once are untested and need the real infra.
|
b2e8f80 to
921c525
Compare
|
Conflicts cleared, and the PR now ships the metrics that judge it. RebasedOnto current L2 was shipping blind — fixedGood catch.
Guarded, because a mislabel is worse than no metric — it credits the node-local cache with work it never did. The new orchestrator test publishes a real archive from a separate root with L1 switched off, so a hit can only be L2; flipping the emitted label to Cross-node on Rancher: not possible, and it wouldn't prove anythingTwo separate reasons, and the second is the one that matters. Mechanically: Rancher Desktop is one VM. I stood up an in-cluster NFS server and an NFS-backed RWX PV to get the real thing, and kubelet failed with Torn back down; the cluster is clean. Substantively: the L2 code has zero node awareness — it reads a path. What a second node would change is (a) the node-local cache is cold and (b) the store is network-backed. (a) is already covered — the restorer pod had an empty local cache and restored from the shared volume alone, which is the entire mechanism. (b) is the real untested risk, and it isn't about node count: it's NFS metadata latency and a cold-node herd hitting one filesystem at once. A second kubelet on the same VM and the same disk reproduces neither. So the honest gap after this PR is unchanged and specific: NFS behaviour under load, provable only on real EFS. Everything the code can distinguish locally is tested.
|
nicacioliveira
left a comment
nicacioliveira
left a comment
There was a problem hiding this comment.
LGTM
just some points that I noticed:
the L2 store has no pruning like L1 does, so as written it grows forever, one archive per repo+lockfile, and it'll need a prune or an EFS lifecycle policy before it becomes writable
...also, since the whole volume is mounted and repoHash is derived from the clone url, any sandbox can read any repos files, that's fine if we treat node_modules as non-sensitive, but private registry deps would be readable across tenants, so its worth deciding on purpose.
Last one that does matter for this PR: version 0.9.38 is behind mains 0.9.39 (the publish skips lowr versions) and app version regresses from 1.25.3 to 1.25.1, so rebase and bump both above main before procede
36f7a5f to
fca1b6c
Compare
|
Thanks — all three handled, and the version one was worse than you saw: Versions (the one that blocked)Rebased onto current No pruning — fixedYou're right, and it was worse than asymmetric: L1 has had TTL + per-repo cap since it shipped, so L2 was the one tier that grew forever. Two things it does differently from
On your "before it becomes writable" framing: prune hangs off a successful publish, so it runs exactly where writes do and nowhere else. Which means the honest statement is that nothing prunes in the deployed config — the tenant mount is read-only, so until the trusted writer exists a filesystem lifecycle policy is the only reaper. That's now written at the Tests: the rule (TTL reap, cap, temp files spared) and separately the wiring, because the rule being correct doesn't stop the store growing if publish never calls it. Commenting out the one call: 6 pass / 1 fail, only the wiring test. Cross-repo reads — decided on purpose, in writingThis is the one I'd been sloppy about. The spec claimed a cross-repo read "leaks only a dependency set, not source or secrets" — and that's wrong for a private registry, where Also worth being precise about the mechanism: it isn't just that the whole volume is mounted, it's that the key is Decision, now at the knob where someone enabling it reads it: accepted for public-registry dependencies — no first-party source, no secrets, and the read-only mount means nothing is tamperable. And the condition that flips it is named rather than left to be rediscovered: a repo installing from a private registry must not be enabled on this store without per-repo enforcement, which means an EFS access point per repo — and that needs the repo known at mount time, i.e. it costs the warm pool. That trade belongs to whoever has a private-registry repo, not to this PR. No enforcement added, deliberately: the only mechanism that would work is the access-point-per-repo one, and paying for it before a private-registry repo exists buys nothing.
Unchanged gap, same as before: NFS behaviour under a cold-node herd, provable only on real EFS. |
| cpu: "4" | ||
| memory: 16Gi | ||
| ephemeral-storage: 40Gi | ||
| emptyDirSizeLimits: |
There was a problem hiding this comment.
those tiers/defaultTier/emptyDirSizeLimits are ghost, no template here reads it, so it does nothing. drop it or use it
There was a problem hiding this comment.
Dropped — 91 lines gone.
You're right, and it's worse than dead config: defaultTier, tiers and emptyDirSizeLimits describe a multi-SandboxTemplate feature that doesn't exist in this chart. Every key is declared, documented at length, and then ignored at render time — so the block reads as configurable and isn't. Orphan config that renders nothing is worse than no config.
It also had nothing to do with L2. It rode in on a values.yaml conflict resolution during an earlier rebase; main never carried it (git log -S'defaultTier: small' origin/main → empty), which is why it looked like context rather than something this PR added. Thanks for catching it — I'd stopped reading that region as mine.
Deleting rather than implementing, deliberately: a tiers feature is a real change to how claims pick a template, and it isn't this PR.
values.yaml vs main is now depsCache.remote only — no new top-level keys. Verified there's no other reference to any of the three anywhere in the chart, helm lint clean, and the render is byte-identical off / on / still-fails-on-missing-pvcName.
Also rebased onto current main (52c2457), so the branch is no longer behind. Chart stays 0.9.44, above main's 0.9.43.
There was a problem hiding this comment.
Correction to what I wrote above — I mischaracterized this, and the difference matters if you're reviewing #5361 next.
Those values are not a feature that doesn't exist. They're the values half of #5361 (feat(sandbox): per-(org, repo) size tiers), which is open. The 91-line block I deleted here is byte-identical to that branch's, and over there it is read — _helpers.tpl references the three keys 14 times, plus a rewritten sandbox-template.yaml and the warm-pool template.
So the accurate statement is narrower than mine: the values leaked into this PR without the templates that consume them, which is what made them inert here. Your read stands unchanged — ghost in #5351, does nothing, drop it — and deleting was still right, because #5351 shouldn't carry half of #5361. But I shouldn't have implied the work was junk. Nothing was lost: #5361 still has all of it.
Two things this turned up that do need attention on #5361, not here:
- It's
CONFLICTINGagainstmainright now. - It has the same version problem you caught on this PR:
appVersion1.24.2, behind main's1.26.1. Chart0.10.0is fine (above 0.9.43), but the appVersion regresses. Same fix.
And for whoever merges second: #5351 and #5361 conflict on Chart.yaml only (both bump version + add a changelog entry at the top). values.yaml and sandbox-template.yaml merge clean between them, so it's a one-file resolution, not a rebase fight.
The golden node_modules cache is NODE-LOCAL: it only hits when the pod lands on a node already warm for its repo. The sandbox pool churns ~170 nodes/day across three AZs and ~80% of ensures are fresh pods, so a large share of boots land somewhere cold and pay a full install — and cross-zone a node-local cache cannot help at all. Add a second tier keyed identically to the first: a per-(repo, pm, lockfile) tar.zst on a shared RWX volume, restorable on any node in any zone. Restore order becomes L1 reflink (~1s) → L2 extract → install. An L2 hit also seeds this node's L1, so the next pod here gets the reflink. Store a TARBALL, never the tree. EFS/NFS charges per-operation metadata latency: fatal across ~100k small files, a non-issue for one large sequential blob. The per-file cost is paid by the local extract. Transport is an explicit `zstd -dc | tar -xf -` pipe rather than tar's own compressor flags, which are not portable and fail differently per flavor: the image ships GNU tar, but a macOS host runner has bsdtar, where `-I` means --include (so `-I zstd` looks for a file named "zstd") and --use-compress-program is rejected on read. Both verified before choosing. Spawned and streamed throughout, no *Sync — a 2 GB tree through the daemon's single-threaded loop would stall the health probe and get the sandbox torn down. Extraction stages into a temp dir and renames into place, so a tar interrupted mid-write can never leave a partial node_modules that later code reads as complete (the boot would skip install and fail somewhere unrelated). Publish writes a temp file and renames, so concurrent publishers on different nodes cannot produce a truncated archive. Publish is gated on the same healthy-boot transition as L1, which matters more here: a broken install published to a shared store would poison every node in the fleet, not just this one. Chart mounts the volume READ-ONLY into the sandbox container. That is the load-bearing control, not a hint — sandboxes run untrusted code and bun installs cached content as-is, so a writable shared store would widen the existing node-local RCE surface to a fleet-wide one. Publishing from tenant pods stays off; a trusted writer comes with the infra work. Ships dark: no GOLDEN_CACHE_REMOTE means the daemon is L1-only, exactly today's behavior. Deploying it additionally needs the EFS CSI driver, which is not installed — that is infra, tracked separately.
…cating Restore never worked on a real-sized tree. `runPiped` relayed the archive through this process (`p.stdout.pipe(c.stdin)`), and once the consumer applies backpressure — `tar -x` writing many small files is slower than `zstd -dc` produces — the tail of the stream was dropped while BOTH children still exited 0. Verified in the sandbox image on a real daemon boot: restore of a byte-verified-good 2.3 MB archive failed 2/2 with `tar: Unexpected EOF in archive`; isolated, 5 of 12 runs failed at 2 MB and 12 of 12 pass wired fd-to-fd. It never fails below ~1 MB, which is why the ~1 KB test fixture passes. The same loss hit publish once, and there it is worse: a valid zstd frame containing a truncated tar (532 of 1069 members) published as a success. Since publish no-ops when the key exists, that archive is permanent — every node in the fleet keeps failing restore and paying a full install with nothing to repair it. Publish now reads the archive back before the rename. Also: - Surface the failing side's stderr. "tar exit 2" alone is unactionable; it took a hand-run of the pipe to learn it meant a truncated archive rather than a permissions or disk problem. - Render GOLDEN_CACHE_REMOTE whenever `remote.enabled` instead of nesting it under `golden`. The volume was already mounted at that scope, so with golden off the chart mounted an RWX PVC into every sandbox pod that the daemon never read. Bun.spawn's direct fd wiring also delivers what the old comment claimed: a multi-GB tree no longer passes through the daemon's single event loop during a boot.
The fd-to-fd fix shipped without a test, and the existing fixtures cannot catch what it fixed: the relayed pipe never dropped anything below ~1 MB and every case here used ~1 KB. A revert to `p.stdout.pipe(c.stdin)` stayed green on all four. Round-trip a 1500-file / ~9 MB tree of incompressible content, so tar's many small writes actually fall behind zstd and the transfer has to survive backpressure. Assert the exact member count and the contents of the LAST file — the observed failure delivered 532 of 1069 members with both children exiting 0, so a partial extract looks fine unless you count. Verified as a guard, not just an addition: reverted to the relayed pipe and this test fails 3/3 (and only this test), passes with the fix.
…nally Found by running the two-pod shape on a bare RWX volume: publish dies with `EACCES: mkdir '/golden-cache/golden'`. A fresh volume is root-owned, and this chart has no way to fix that — the tenant mount is read-only by design and fsGroup does not apply to it. Every restore then misses silently, forever, which reads as "the cache just never hits". Say so where someone setting pvcName will read it, and name the EFS spelling (an access point with RootDirectory CreationInfo for uid/gid 1000, not the filesystem root).
L2 shipped blind. `sandbox.deps.restore` only knew `l1` and `miss`, so a
boot the shared archive rescued was indistinguishable from one the
node-local golden served — which makes the one question the tier exists to
answer ("was it worth the shared store?") unanswerable from the data.
Widen the source union with `l2` and emit it at the archive-restore site.
The counter split is then the hit rate per tier, and `duration_ms` on `l2`
against `miss` is the per-boot saving.
Guarded, because a mislabel is worse than no metric — it credits the
node-local cache with work it never did. The new orchestrator test drives a
real archive published from a separate root with L1 switched OFF, so a hit
can only have come from L2; changing the emitted label to `l1` fails it.
The shared store grew forever: one archive per (repo, lockfile), no reaper, while L1 has had a TTL + per-repo cap since it shipped. `pruneRemoteGoldens` applies the same rule to the remote store (untouched >7d, then newest 5 per repo) after a successful publish, reusing L1's constants so the two tiers can't drift apart. Async throughout, unlike L1's `pruneGoldens` — the store is NFS, where a readdir is a round trip per entry, and parking the daemon's event loop stalls its health probe (CONTRIBUTING rule #1). `.tmp.<pid>` entries are skipped: they are another node's publish in flight. Prune runs only where publish does, so a read-only tenant mount never prunes anything — that's now stated at the values.yaml knob, since it means a filesystem lifecycle policy is the only reaper until the trusted writer lands. Also written down where someone enabling this will read it: any sandbox can read any repo's archive. The volume is mounted whole (warm-pool pods are bound before they know their repo) and the key is sha256 of the clone URL, so read isolation is guessability, not enforcement. Accepted for public-registry dependencies — no first-party source, no secrets, nothing writable. Named the condition that flips it: a private registry puts proprietary package source in node_modules, which this store would expose fleet-wide. Chart 0.9.44, above main's 0.9.43 (the publish workflow skips lower versions); appVersion stays at main's 1.26.1 rather than regressing to the pre-rebase tag.
…ver used 91 lines of values that no template reads. `defaultTier`, `tiers` and `emptyDirSizeLimits` describe a multi-SandboxTemplate feature that does not exist in this chart — every key is declared, documented, and then ignored at render time, so the whole block is a lie about what the chart does. It also has nothing to do with the L2 cache. It rode in on a values.yaml conflict resolution during an earlier rebase; main never had it. Deleting rather than implementing: a tiers feature is not this PR, and orphan config that renders nothing is worse than absent config — it reads as configurable. values.yaml vs main is now depsCache.remote only. Chart renders identically off, on, and still fails on a missing pvcName.
f9b6da6 to
af6469e
Compare
… EPIPE CI (Linux) failed the 9 MB round-trip: `EPIPE: broken pipe, write` thrown from inside Bun's stream pump, asynchronously, where the try/catch around the spawn can't see it. It took out the whole test file and left publish with no archive. Passing the producer's `stdout` as the consumer's `stdin` reads as an fd handoff, but Bun relays it through this process once the stream outgrows the pipe buffer, and then the relay's write races the consumer. That is the same handoff that previously truncated the stream's tail with both children exiting 0 — two failure modes, one cause. The 1 KB fixtures never reach the buffer's edge, which is why only the big case caught either. So hand the pipe to `bash -c` and let the kernel move the bytes: nothing traverses this process, both failure modes go away, and a multi-GB tree still stays off the daemon's event loop. `set -o pipefail` keeps the producer's exit code visible — without it a failed `tar -c` feeding a happy `zstd` would look like a successful publish. Verified both directions (producer-fails → exit 1, both-ok → exit 0). Arguments are shell-quoted, since the mount root and install root come from operator config and a path with a space would otherwise split. Also fixes why the CI log was so hard to read: `soleArchive` destructured an empty readdir and threw ERR_INVALID_ARG_TYPE, which reads as a broken helper rather than "publish produced nothing", and the test passed no `log` so publish's own diagnostic was discarded. Both now assert with the real reason.
PR: #5351 feat(sandbox): cross-node golden dependency cache (L2 archive) Bump type: minor - @decocms/sandbox (packages/sandbox/package.json): 1.26.1 -> 1.27.0 - deploy/helm/sandbox-env (chart 0.9.45) (deploy/helm/sandbox-env/values.yaml deploy/helm/sandbox-env/Chart.yaml): image.tag/appVersion -> 1.27.0 Deploy-Scope: both
2 participants
The P1b transport from the EFS spec. Buildable and testable today — only deploying it needs the EFS CSI driver.
Why
The golden cache is node-local. A hit needs the pod to land on a node already warm for its repo, and the pool churns ~170 nodes/day across three AZs with ~80% fresh pods — so a large share of boots land cold and pay a full install, and cross-zone a node-local cache can't help at all.
What
A second tier, keyed identically to L1 (same credential-stripped repo hash, same lockfile hash — the two tiers can never disagree about what a key means, and per-repo isolation is the same boundary L1 already relies on):
Restore order is now L1 reflink (~1s) → L2 extract → install. An L2 hit also seeds this node's L1 via the existing
pendingGoldenpath, so the next pod on this node gets the reflink instead of another extract.Tarball, never the tree. EFS/NFS charges per-operation metadata latency — fatal across ~100k small files, a non-issue for one large sequential blob. The per-file cost is paid by the local extract, on local disk. Mounting
node_modulesover NFS would be slower than no cache at all.Portability — the part that needed checking
Transport is an explicit
zstd -dc | tar -xf -pipe, not tar's own compressor flags. Those are not portable and fail differently per flavor. The image ships GNU tar, but a macOS host runner has bsdtar, where:-I zstdmeans--include, so it looks for a file named "zstd" — and exits 0, which would have looked like a successful restore of nothing;--use-compress-program=zstdis rejected on read with "Unrecognized archive format".Both verified on bsdtar 3.5.3 before choosing. The explicit pipe behaves identically everywhere.
Spawned and streamed throughout, no
*Sync— a 2 GB tree through the daemon's single-threaded loop would stall the health probe, which Studio reads as a dead sandbox (CONTRIBUTING rule #1).Failing closed
node_modulesthat later code reads as complete — the boot skips install and fails somewhere unrelated. Covered by a test that corrupts the archive and asserts nothing is left behind.Security
The chart mounts the volume read-only into the sandbox container. That's the load-bearing control, not a hint — sandboxes run untrusted code and bun installs cached content as-is, so a writable shared store widens the existing node-local RCE surface to a fleet-wide one (one tenant poisoning another repo's archive). This is exactly the case the existing
depsCachecomment warns about ("Do NOT widen to a shared cache without making it read-only to tenant pods").Publishing from tenant pods therefore does nothing in the deployed config; a trusted writer comes with the infra work. The daemon-side publish path exists and is tested, but only a writable mount would exercise it.
Ships dark
No
GOLDEN_CACHE_REMOTE→ the daemon is L1-only, exactly today's behavior. Chart defaultsdepsCache.remote.enabled: false; enabling without apvcNamefails the render rather than silently mounting nothing. Rollback is unsetting the value.Deploying additionally needs the EFS CSI driver, which is not installed in prod (only
ebs-csi) — that's terraform, tracked separately. Also note prod pins chart0.9.5while this bumps0.9.34, so it needs atargetRevisionbump indeco-apps-cdto land (see the note on #5340).Also
zstdadded to the sandbox image — the base shipstar/gzip/xzbut notzstd, and the L1 path never needed a compressor.Testing
remote-golden.test.ts— real files, realtar/zstd, no mocks:node_modulesgets a complete tree, contents verified. That's the whole point of the tier.GOLDEN_CACHE_REMOTEunset, the identical case restores nothing — without this the first test would pass even if the code ignored the env.bun test packages/sandbox/daemon/setup/→ 90 pass.daemon.e2e.test.ts→ 103 pass.helm templateverified off / on / missing-PVC.helm lint,bun run fmt,bun run lint(0 errors),knipclean,tscclean apart from a pre-existing zod error inpackages/shared.Not validated on a real pod — that needs the EFS infra. Per repo norm, "done" means a tailed pod showing an L2 hit; this is not that yet.
Summary by cubic
Adds an opt-in L2 cross-node golden cache that restores
node_modulesfrom a shared.tar.zston an RWX PVC so cold boots across nodes/zones skip installs. Updates the transport to a shell-ownedzstd -dc | tar -xf -pipeline withpipefailto fix EPIPE/truncation on large trees, addsl2metrics, and ships via Helm chart0.9.44with a read-only mount.New Features
bash -cwithset -o pipefail, stderr surfaced, args shell-quoted) avoids relayed pipes that caused EPIPE/silent truncation under backpressure; excludes pod-local caches (.vite,.cache). Sandbox image includeszstd.source="l2"added tosandbox.deps.restore; guarded by an orchestrator test.depsCache.remote.*(off by default) mounts an RWX PVC read-only and setsGOLDEN_CACHE_REMOTE; chart0.9.44. Removed unuseddefaultTier/tiers/emptyDirSizeLimits. Values docs call out provisioning the volume root for uid/gid 1000.Migration
depsCache.enabled: true,depsCache.remote.enabled: true, providedepsCache.remote.pvcName, and bump to chart0.9.44.Written for commit 51bb9a4. Summary will update on new commits.