feat(orch): upgrade envd in filesystem-only snapshots via offline rootfs swap - #3467
feat(orch): upgrade envd in filesystem-only snapshots via offline rootfs swap#3467kalyazin wants to merge 5 commits into
Conversation
PR SummaryHigh Risk Overview Reviewed by Cursor Bugbot for commit 94673f3. Bugbot is set up for automated code reviews on this repo. Configure here. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d220dccdb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| out, err := runDebugfsSwapSandboxed(ctx, devicePath, stage, scriptPath) | ||
| span.SetAttributes(attribute.Int("debugfs_output_bytes", len(out))) | ||
| if err != nil { | ||
| return fmt.Errorf("debugfs envd swap: %w (output: %q)", err, string(out)) |
There was a problem hiding this comment.
When the offline flag is enabled and the debugfs process accepts the script but an individual command fails, this check still treats the swap as successful; I verified with debugfs 1.47.0 that failing commands in a -f script can exit 0. If rm /usr/bin/envd succeeds and the subsequent write fails because the new binary needs more space than the old one freed, the rootfs is left without envd, yet the caller records success and cold-boots it instead of falling back to the original envd. Please verify the resulting file and executable mode after the script, or use a transactional replacement that preserves the old binary on failure.
AGENTS.md reference: AGENTS.md:L22-L26
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed — the swap is now transactional instead of trusting the debugfs exit code. SwapEnvdBinary now:
dumps the original/usr/bin/envdout to a host file first (and refuses to proceed if it can't — nothing to roll back to);- does the
rm+write+sif; - verifies by reading
/usr/bin/envdback out and comparing its sha256 to the intended binary — thedebugfs -fprocess exit is no longer trusted, since it's 0 even when a scripted command fails; - on any mismatch, restores the original from the host backup and returns an error, so the guest boots its original envd rather than a rootfs with no envd.
So the exact scenario (rm succeeds, write fails → envd gone, exit 0) is now caught by step 3 and undone by step 4. packages/orchestrator/pkg/sandbox/rootfs/envd_swap_linux.go.
Note: there's no CI coverage of the real debugfs path yet, so this is pending a dev-cluster re-verify (including a full-rootfs fault-injection case to exercise the rollback) before merge.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| func runDebugfsSwapSandboxed(ctx context.Context, devicePath, stageDir, scriptPath string) ([]byte, error) { | ||
| if !nbdDevicePath.MatchString(devicePath) { | ||
| return nil, fmt.Errorf("refusing to run debugfs on unexpected device path %q", devicePath) | ||
| } | ||
|
|
||
| unit := "envd-swap-" + filepath.Base(devicePath) | ||
|
|
||
| args := []string{ | ||
| "--wait", "--pipe", "--collect", "--quiet", | ||
| "--unit=" + unit, | ||
| fmt.Sprintf("--property=RuntimeMaxSec=%d", int(EnvdSwapTimeout.Seconds())), | ||
| "--property=KillSignal=SIGKILL", | ||
| "--property=TimeoutStopSec=10s", | ||
| "--property=DynamicUser=yes", | ||
| "--property=SupplementaryGroups=disk", | ||
| "--property=ProtectProc=invisible", | ||
| "--property=ProcSubset=pid", | ||
| "--property=PrivateNetwork=yes", | ||
| "--property=PrivateIPC=yes", | ||
| "--property=ProtectHome=yes", | ||
| "--property=NoNewPrivileges=yes", | ||
| "--property=CapabilityBoundingSet=", | ||
| "--property=AmbientCapabilities=", | ||
| "--property=RestrictNamespaces=yes", | ||
| "--property=SystemCallFilter=@system-service", | ||
| "--property=SystemCallArchitectures=native", | ||
| "--property=RestrictAddressFamilies=AF_UNIX", | ||
| "--property=LockPersonality=yes", | ||
| "--property=ProtectClock=yes", |
There was a problem hiding this comment.
🔴 debugfs runs in batch mode (-w -f script device) and per-command failures (rm/write/sif) — or even a failed device open — are only reported via stderr, never through the process exit code, so cmd.Run() returns nil on real failures too. SwapEnvdBinary/runDebugfsSwapSandboxed never scan the captured output on the success path, so a genuine failure (e.g. rootfs missing /usr/bin, ENOSPC) is recorded as offline_upgrade.attempts{result=success} and logged as swapped, while the guest actually cold-boots the old envd — and a rm-succeeds/write-fails split can even leave the rootfs with no envd binary at all, still reported as success.
Extended reasoning...
The bug: debugfs -w -f <script> <device> is a well-documented case where the process exit code does not reflect the success or failure of the scripted commands. In batch (-f) mode, debugfs reads each line of the script and executes it in a request loop; if a command fails (e.g. rm/write/sif hit "File not found by ext2_lookup" because the parent directory is missing, or write fails with ENOSPC) the error is printed to stderr via com_err, but the loop continues to the next command and the process still exits 0 at the end of the script. Even a failed filesystem open (a corrupt or unexpected device) only prints an error and still returns exit code 0. systemd-run --wait simply propagates that same exit status.
Where it breaks: runDebugfsSwapSandboxed (envd_swap_linux.go:117-145) returns cmd.Run()'s error directly as its result. SwapEnvdBinary treats a nil error as proof the rewrite happened. Back in reboot.go's envdOfflineUpgradePreBoot, err == nil drives the result := "success" branch: it logs "swapped envd binary before reboot" and records offline_upgrade.attempts{result="success", ...} via the counter. Nothing about this path is contingent on the debugfs script actually having succeeded — only on the wrapping process having exited 0, which it almost always does.
Why nothing else catches it: the out buffer captured from stdout/stderr (cappedBuffer) is inspected only inside SwapEnvdBinary's error branch, to decorate the fmt.Errorf message (\"debugfs envd swap: %w (output: %q)\"). On the success path, out is discarded entirely (only len(out) is recorded as a span attribute). There is also no post-swap verification step — no re-open/re-stat of /usr/bin/envd on the rewritten device to confirm the write landed — so nothing else in the pipeline would notice a silently-failed rewrite.
Concrete walkthrough: Suppose a filesystem-only snapshot's rootfs unexpectedly lacks a /usr/bin directory (a malformed/atypical template, or a bit of drift from what the resolver assumed). envdOfflineUpgradePreBoot resolves an upgrade target and fsQuiesced is true, so decideOfflineSwap returns swap: true. The PreBootFn calls rootfs.SwapEnvdBinary, which stages the new binary, writes the debugfs script (rm /usr/bin/envd, write ... /usr/bin/envd, sif ...), and calls runDebugfsSwapSandboxed. Inside the jail, debugfs -w -f script /dev/nbdX opens the device fine, but rm /usr/bin/envd fails with "File not found by ext2_lookup" (parent dir absent), prints that to stderr, and moves on to write, which also fails the same way, then sif, same. debugfs finishes the script and exits 0. cmd.Run() returns nil. runDebugfsSwapSandboxed returns (out, nil). SwapEnvdBinary sees err == nil and returns nil. Back in the PreBootFn, err == nil → result = \"success\"; the orchestrator logs "swapped envd binary before reboot" and emits offline_upgrade.attempts{result=\"success\", from_version=0.6.5, to_version=0.6.12}. The VM then cold-boots on the untouched, original /usr/bin/envd (still 0.6.5) while every observable signal — logs and metrics — says the upgrade succeeded.
A worse variant: if /usr/bin/envd does exist so rm succeeds, but the subsequent write fails (e.g. ENOSPC, or a transient jail/bind-mount hiccup), the rootfs is left with no /usr/bin/envd at all — an unbootable/broken sandbox — while the same success telemetry is still recorded. This directly contradicts the PR's stated "best-effort, never fail the resume" safety property, since a failure here isn't merely a no-op back to the original envd, it's actively destructive, and it's invisible to the very metric (result=swap_failed) the rollout plan says operators should watch.
Impact: This defeats the core observability the PR's own dev-cluster validation leans on (checking orchestrator_envd_offline_upgrade_attempts_total{result=\"success\"} in Mimir) to gauge whether the ramp is working — a real failure rate of, say, 5% would be entirely absorbed into the success bucket. Since the feature is default-off (envd-offline-upgrade-target=off), there's no current production exposure, but as soon as it's ramped, this makes swap_failed an unreliable/blind signal and creates a path to silently un-resumable sandboxes with no alerting.
Fix direction: After runDebugfsSwapSandboxed returns with err == nil, scan the captured out buffer for debugfs's known failure markers (e.g. lines containing "File not found", "not open", or the general "debugfs: " error prefix that com_err emits) and treat their presence as a failure. Better still, verify the swap actually happened — e.g. re-run a debugfs stat /usr/bin/envd (or show_inode_info) after the write and compare size/mtime against the staged binary, or check debugfs's own request-count vs error-count if exposed — rather than trusting either the exit code or output-string matching alone.
There was a problem hiding this comment.
Fixed at HEAD. The swap no longer trusts the debugfs exit code: it backs the original out to the host, does the swap, verifies by reading /usr/bin/envd back and comparing its sha256 to the intended binary, and rolls the original back on any mismatch. Cluster-verified both directions — happy path 0.6.5 → 0.6.12, and a forced rm-succeeds/write-fails run where the verify caught the empty result and the rollback restored a bootable original envd.
| logger.WithBuildID(buildID.String()), | ||
| zap.String("built_with", from), | ||
| zap.String("target", toVersion), | ||
| ) | ||
| envdOfflineUpgradeAttempts.Add(ctx, 1, metric.WithAttributes( | ||
| attribute.String("result", dec.countResult), | ||
| attribute.String("from_version", from), | ||
| attribute.String("to_version", toVersion), | ||
| )) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 In the not_quiesced skip-log branch of envdOfflineUpgradePreBoot (reboot.go:265-278), zap.String("target", toVersion) logs the resolved envd version string (e.g. 0.6.12) under the target field, whereas the sibling swap-success/failure logs a few lines below use target for the binary path and carry the version separately as to_version. This line also omits to_version entirely. A log query or dashboard that filters/joins on target expecting a path will silently mismatch on this skip case.
Extended reasoning...
What the bug is: envdOfflineUpgradePreBoot in packages/orchestrator/pkg/sandbox/reboot.go emits three structured log lines for the offline envd-upgrade feature: a misconfig-skip log, a not_quiesced-skip log, and the swap success/failure logs. The swap success log (line ~294) and the swap failure log (line ~302) both use zap.String("target", path) — path is the resolved binary path such as /fc-envd/envd.<sha> — and separately carry zap.String("to_version", toVersion). The not_quiesced skip log (lines 265-278) breaks this convention: it emits zap.String("target", toVersion), putting the version string (e.g. "0.6.12") into the field everywhere else means "binary path," and it drops the to_version field altogether.
Code path that triggers it: decideOfflineSwap returns countResult: "not_quiesced" only when the resolver already found a valid upgrade path (resolverPath != "") but the snapshot's rootfs wasn't frozen at pause (!fsQuiesced). At that call site in envdOfflineUpgradePreBoot, the real binary path is sitting right there in the path variable returned by featureflags.ResolveEnvdOfflineUpgrade — it just isn't the value logged. So this isn't a case where a path is unavailable; the wrong variable was logged under the wrong field name.
Why nothing else catches it: This is unit-tested at the decideOfflineSwap pure-function level (reboot_offline_upgrade_test.go), which only asserts on the swap/countResult/logMisconfig struct fields — it never inspects the log line's field names or values, so the mismatch has no test coverage. Code review of the diff also doesn't immediately surface it, because the log statement is syntactically valid and toVersion reads plausibly close to target in context.
Impact: Purely an observability/log-schema inconsistency — no functional or behavioral effect on the swap decision, the swap itself, or the emitted metric (envdOfflineUpgradeAttempts correctly uses to_version=toVersion for this same skip case, so metric-based dashboards are unaffected). The impact is confined to structured log consumers: an operator or dashboard that filters/joins the target field across the three offline-upgrade log lines expecting a filesystem path (to e.g. correlate "which binary would have been swapped in") will get a version string instead for not_quiesced events, and will find no to_version field to fall back to on those lines.
Step-by-step proof:
- A filesystem-only snapshot resumes via
RebootSandbox→envdOfflineUpgradePreBootis invoked withfsQuiesced=false(the pause wasn't frozen) and the offline-upgrade flag resolves an upgrade, e.g.path="/fc-envd/envd.vb",toVersion="0.6.12",reason="". decideOfflineSwap("/fc-envd/envd.vb", "", false)hits the!fsQuiescedbranch and returnsofflineSwapDecision{countResult: "not_quiesced"}(swap=false).- Back in
envdOfflineUpgradePreBoot, sincedec.countResult != "", it logs:zap.String("built_with", from),zap.String("target", toVersion)→ this emitstarget="0.6.12". - Compare to the swap-success log a resume later actually executes (when
fsQuiesced=true):zap.String("target", path),zap.String("to_version", toVersion),zap.String("built_with", from)→ this emitstarget="/fc-envd/envd.vb",to_version="0.6.12". - A dashboard built to filter
target="/fc-envd/envd.vb"(a path) across all three offline-upgrade log lines to see "who was gated vs who succeeded on this binary" will match the success/failure lines but silently miss everynot_quiescedline, because theretargetholds"0.6.12"instead.
How to fix: In the not_quiesced log block, log zap.String("target", path) instead of toVersion, and add zap.String("to_version", toVersion) alongside it, matching the two sibling log statements in the same function.
There was a problem hiding this comment.
Good catch — fixed. The not_quiesced skip log now uses target for the binary path and carries the version as to_version, matching the swap-success/failure logs (reboot.go):
zap.String("built_with", from),
zap.String("target", path),
zap.String("to_version", toVersion),
so target means the same thing (a path) across all three log sites, and the skip log no longer omits to_version.
|
|
||
| // The jail runs debugfs as a transient DynamicUser that must traverse this | ||
| // directory to read the staged binary and command file; MkdirTemp creates it | ||
| // 0700 (owner-only), so widen it to 0755. The files inside are world-readable. | ||
| if err := os.Chmod(stage, 0o755); err != nil { | ||
| return fmt.Errorf("chmod swap stage dir: %w", err) | ||
| } | ||
|
|
||
| stagedBin := filepath.Join(stage, "envd") | ||
| if err := copyFile(srcPath, stagedBin, 0o755); err != nil { | ||
| return fmt.Errorf("stage target envd %q: %w", srcPath, err) | ||
| } | ||
|
|
||
| // debugfs request script: replace /usr/bin/envd (rm then write, since write | ||
| // refuses an existing path) and restore an executable regular-file mode | ||
| // (0100755 = S_IFREG | 0755) so systemd's ExecStart=/usr/bin/envd can run it. | ||
| script := fmt.Sprintf("rm /usr/bin/envd\nwrite %s /usr/bin/envd\nsif /usr/bin/envd mode 0100755\n", stagedBin) | ||
| scriptPath := filepath.Join(stage, "cmds") | ||
| if err := os.WriteFile(scriptPath, []byte(script), 0o644); err != nil { | ||
| return fmt.Errorf("write debugfs script: %w", err) | ||
| } | ||
|
|
||
| out, err := runDebugfsSwapSandboxed(ctx, devicePath, stage, scriptPath) |
There was a problem hiding this comment.
🟡 The staged envd binary/script in SwapEnvdBinary are created via os.OpenFile/os.WriteFile with an explicit mode, but that mode is still subject to the orchestrator process' umask, and there is no follow-up chmod to correct it. Under a restrictive umask (e.g. 0077) the staged binary lands 0700 root-owned, and the jailed debugfs runs as an unprivileged DynamicUser (empty CapabilityBoundingSet, no CAP_DAC_OVERRIDE) that can only read it via the world bits — so it fails to open the source file and the offline swap silently fails on that node (result=swap_failed).
Extended reasoning...
SwapEnvdBinary in packages/orchestrator/pkg/sandbox/rootfs/envd_swap_linux.go stages the new envd binary and a debugfs command script into a private MkdirTemp directory that gets bind-mounted read-only into a systemd-run jail. The jail runs debugfs as a DynamicUser — an unprivileged, dynamically-allocated uid/gid with SupplementaryGroups=disk, CapabilityBoundingSet= and AmbientCapabilities= both empty (no CAP_DAC_OVERRIDE/CAP_DAC_READ_SEARCH). Since the staged files are owned by root and the DynamicUser's only relevant group membership is disk (not the file's owning group), the jailed process can only read them via the "other" permission bits.
copyFile(srcPath, stagedBin, 0o755) creates the staged binary with os.OpenFile(dst, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, mode). Per POSIX, the kernel ANDs that mode with ^umask before applying it — Go's os.OpenFile does not correct for this. The debugfs command script is written the same way via os.WriteFile(scriptPath, []byte(script), 0o644). Neither file gets a follow-up chmod. Under the default umask 0022 (root daemons typically inherit this, and nothing in the repo overrides it for the orchestrator's Nomad raw_exec job) the result is 0755/0644 as intended and the DynamicUser can read both. But under a more restrictive umask commonly used to harden root daemons — 0077 — the binary becomes 0700 and the script becomes 0600: both owner-only, unreadable by the jail's DynamicUser.
The code is aware of this exact pitfall for the directory: right after MkdirTemp (which always creates directories 0700), it does os.Chmod(stage, 0o755) specifically to defeat that default and make the directory traversable by the DynamicUser, with a comment asserting "The files inside are world-readable." That assumption is not actually enforced for the files themselves — only the directory chmod is umask-immune (chmod doesn't apply umask); the files' initial OpenFile/WriteFile modes are not re-applied via chmod and so remain umask-dependent. This is precisely the pattern already known and worked around elsewhere in this codebase: packages/orchestrator/pkg/volumes/file_create.go explicitly re-Chmods a written file after creation with the comment "we do this again to avoid the process' umask from automatically 'fixing' our requests" (verified at file_create.go:105-107).
Step-by-step manifestation:
- The orchestrator process is started with
umask 0077(a plausible hardening default for a root-owned daemon, though not the current one used anywhere in this repo). - A filesystem-only snapshot resumes via
RebootSandbox,decideOfflineSwapresolves an upgrade target and the rootfs isfs_quiesced, soenvdOfflineUpgradePreBoot'sPreBootFnruns. SwapEnvdBinarycallscopyFile(srcPath, stagedBin, 0o755)→os.OpenFile(..., 0o755)→ kernel applies0o755 &^ 0o077 = 0o700.stagedBinis now mode 0700, owned by root.runDebugfsSwapSandboxedlaunchessystemd-runwithDynamicUser=yes,SupplementaryGroups=disk, emptyCapabilityBoundingSet/AmbientCapabilities, and bind-mountsstageDirread-only.- Inside the jail,
debugfs -w -f <script> <device>executeswrite <stagedBin> /usr/bin/envd. To openstagedBin, the kernel checks the DynamicUser's uid/gid against the file's owner (root, no match) and group (root's primary group, notdisk, no match), falling through to the "other" bits — which are 0 under mode 0700.open()fails withEACCES. debugfsreturns a non-zero exit status;runDebugfsSwapSandboxedand thenSwapEnvdBinaryreturn an error;envdOfflineUpgradePreBoot'sPreBootFnlogs "offline envd swap before reboot failed; booting original envd", recordsenvdOfflineUpgradeAttempts{result="swap_failed"}, and swallows the error so the boot proceeds — the sandbox boots successfully, just still on the oldenvd.- Every cold-boot resume on that node repeats this failure indefinitely (since the gate re-fires idempotently per the PR's documented convergence behavior), so the whole offline-upgrade feature is silently inert on any node with a restrictive umask, with no crash or user-visible symptom — only a
swap_failedmetric an operator would have to notice.
Why existing code doesn't prevent it: the directory's os.Chmod(stage, 0o755) only fixes traversal into stage/; it does nothing for the mode of the files created inside it afterward. The stagedBin/scriptPath writes still rely on the initial OpenFile/WriteFile mode argument surviving the umask mask unmodified, which POSIX does not guarantee.
Fix: add an explicit os.Chmod(stagedBin, 0o755) after copyFile (or after os.WriteFile for scriptPath), mirroring the existing file_create.go pattern — or set stage's files up before setting the process umask to 0 for this section. A one-line fix per staged file.
Severity: This is real but not blocking. The orchestrator's Nomad raw_exec job doesn't set an explicit umask anywhere in this repo, so it inherits the shell/service default (typically 0022), under which the code works exactly as intended — this is why the PR's dev-cluster e2e passed. The feature itself is also default-off (envd-offline-upgrade-target flag) and explicitly best-effort: a swap_failed result boots the original envd and never aborts or breaks the resume. So merging as-is causes no user-visible failure under any currently-deployed configuration; it's a latent robustness gap that would only manifest if this orchestrator is later run under a hardened umask, and the fix is trivial and low-risk.
There was a problem hiding this comment.
Fixed. The staged binary and the debugfs command file now get an explicit chmod (0755 / 0644) after creation, so a restrictive orchestrator umask can't strip the world-read bits the jailed DynamicUser relies on. Cluster-verified: the swap reads the staged binary and completes (0.6.5 → 0.6.12).
2d220dc to
f6ab5f0
Compare
a37adfb to
ee3622f
Compare
ee3622f to
5883e00
Compare
5883e00 to
5aaef5e
Compare
5aaef5e to
3836430
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3836430. Configure here.
3836430 to
67a6464
Compare
Add the shared building blocks the offline (cold-boot) envd upgrade
needs, reusing the merged live-upgrade decision so both paths present a
single upgrade decision with two delivery mechanisms:
- EnvdOfflineUpgradeTargetFlag, a sibling of envd-upgrade-target with the
same value grammar ("off"/"promoted"/"<sha>") and env-overridable dev
fallback, so the newer offline mechanism ramps independently.
- ResolveEnvdOfflineUpgrade, forwarding to the existing pure
resolveEnvdUpgradePath (built-with vs target, upgrade-only, path-safe),
but keyed on the snapshot's built-with version — there is no running
envd at cold-boot swap time to key on.
- orchestrator.envd.offline_upgrade.{attempts,duration} meter entries.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Nikita Kalyazin <nikita.kalyazin@e2b.dev>
Rewrite /usr/bin/envd inside the filesystem-only snapshot's rootfs before the cold boot, so an envd too old to self-upgrade (< 0.6.12) comes up on a newer envd without any guest cooperation. - SwapEnvdBinary rewrites the unmounted ext4 rootfs entirely in userspace via debugfs (libext2fs) under a systemd-run seccomp jail (empty root, DynamicUser, no network) — never a host-kernel mount of the tenant image. The jailed debugfs is constrained to a bare /dev/nbd* device. - RebootSandbox resolves the target through ResolveEnvdOfflineUpgrade; decideOfflineSwap then gates it: swap only when an upgrade is resolved AND the snapshot's rootfs was frozen at pause (fs_quiesced) — a legacy/ sync-fallback snapshot stays on its envd until a future freezing pause, and a misconfigured/unstaged target is logged rather than swapped. The swap runs in the PreBootFn, fully best-effort: any failure boots the original envd. Because the swap keys on the built-with version, which it does not update, it re-fires idempotently on every cold-boot resume until a re-pause re-bakes the version (visible via the offline_upgrade.attempts metric). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Nikita Kalyazin <nikita.kalyazin@e2b.dev>
…eters Pin the resolver and metric plumbing: ResolveEnvdOfflineUpgrade reads the sibling flag and is independent of the live-path flag (so the two ramp separately); the convergence behavior — the swap keys on the built-with version, which it never advances, so a re-resumed already-upgraded snapshot resolves an upgrade again on every cold boot (idempotent re-fire), and only a re-pause that re-bakes the version makes the resolver no-op (same_version); and a guard that the offline_upgrade counter + duration histogram both carry description and unit map entries (a missing one silently emits an undocumented series). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Nikita Kalyazin <nikita.kalyazin@e2b.dev>
Pin the safety-critical wiring: decideOfflineSwap swaps only when an upgrade is resolved AND the rootfs was frozen at pause (fs_quiesced), counts the not_quiesced gated skip, and stays silent on benign resolver no-ops while logging misconfigured targets — the frozen gate is what keeps a torn-journal rootfs from being rewritten. Also pin the swap primitive's device allowlist (jailed debugfs refuses anything but a bare /dev/nbd*, verified without root or debugfs) and the output cap. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Nikita Kalyazin <nikita.kalyazin@e2b.dev>
Add the cold-boot offline-upgrade sibling to the resume-flow notes in ARCHITECTURE.md, next to the existing live-upgrade bullet: the jailed debugfs rewrite of the rootfs envd before RebootSandbox, its envd-offline-upgrade-target flag and fs_quiesced gate, and the built-with-keyed idempotent re-fire. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Nikita Kalyazin <nikita.kalyazin@e2b.dev>
67a6464 to
94673f3
Compare
1 participant
Why
The same-PID live self-upgrade (envd's
/upgradehandover) moves a sandbox'senvdforwardonly for
envdthat already carries the upgrade machinery (MinEnvdVersionForUpgrade = 0.6.12).Everything older — including the pre-fsfreeze
< 0.6.6cohort — is stuck on whateverenvditwas built with for the sandbox's entire life: there is no way to deliver an
envdfix, feature,or security patch to it, and a sandbox whose old
envdis slow enough to time out on resumebecomes effectively un-resumable.
This reaches that cohort with a version-agnostic path: the old
envdnever has to cooperate.When a filesystem-only snapshot (memory dropped, resume = cold boot) comes up, we rewrite
/usr/bin/envdin its rootfs before the VM boots, so it cold-boots on the newenvd. The cost isthe in-memory state a filesystem-only pause already discards — nothing more.
What
Offline swap primitive (
pkg/sandbox/rootfs/envd_swap_linux.go).SwapEnvdBinaryreplaces/usr/bin/envdinside the unmounted ext4 rootfs entirely in userspace viadebugfs(libext2fs) —
rm+write+sifto restore an executable mode — never a host-kernelmountof the tenant image. It runs under a
systemd-runseccomp jail (empty root,DynamicUser, nocapabilities, no network,
@system-serviceminus@privileged/@resources,MemoryDenyWriteExecute,kernel-logs/cgroups/hostname/realtime protections), so parsing a hostile filesystem is contained, not a
host compromise —
systemd-analyze securityscores the unit 0.4 (SAFE), its residual exposure beingonly the
diskgroup and the one/dev/nbdXnode the rewrite fundamentally needs. It operates on therootfs, which the cold-boot path exposes as an NBD block device
(
/dev/nbdX) —debugfsedits its ext4 in place, and the jail is granted exactly that one devicenode (
DeviceAllow/BindPaths) with the staged binary bind-mounted read-only. A^/dev/nbd[0-9]+$check guards that grant so the raw-block rewrite can't be pointed at a real disk, a partition, or a
traversal to a co-tenant device.
The swap is transactional against the never-brick guarantee.
debugfs -fruns a script ofcommands and exits 0 even if an individual command fails, so a naive
rm-then-writecould deletethe old envd, fail the
write(e.g. ENOSPC), and still report success — a bricked rootfs. SoSwapEnvdBinary(a)dumps the original out to the host first, refusing to proceed if it can't;(b) does the
rm+write+sif; (c) verifies by reading/usr/bin/envdback and checking both itscontent sha256 against the intended binary and that it is an executable regular file (
debugfs stat) — the process exit is not trusted, and a content-only check would miss a silentsiffailure that left it non-executable; and (d) decides purely from the read-back state, never the
debugfsexit code (a non-zero exit — timeout, kill, jail/device failure — says nothing reliableabout what is on disk): new binary → success; the intact original (swap never touched it) → boot
it with no rollback; damaged/gone (partial write, or removed then not rewritten, e.g. a kill
after
rm) → rollback from the host backup; and unreadable rootfs (persistent jail failure)→
ErrOfflineSwapUnrecoverable, on which the reboot hook fails the boot soCreateSandboxdiscardsthe overlay rather than booting an unknown rootfs. This single state-driven decision avoids both
bricking (never boot a damaged/unknown envd) and breaking an intact rootfs (never run the rollback
rmwhen the original is already in place); a rollback that itself fails is also unrecoverable.Wired into cold-boot resume (
pkg/sandbox/reboot.go).RebootSandboxbuilds aPreBootFnthat runs the swap against the NBD-attached rootfs before Firecracker boots. A pure
decideOfflineSwapgates it on two conditions: the resolver returns an upgrade target, andthe snapshot's rootfs was captured frozen (
meta.IsFsQuiesced(), from #3445) so it iscrash-consistent. A misconfigured/unstaged target is logged (not swapped); a resolved-but-unfrozen
snapshot is skipped and counted (
not_quiesced). When the swap primitive returnsErrOfflineSwapUnrecoverable(it couldn't confirm a bootable envd — see above), the hook fails theboot so
CreateSandboxdiscards the dirty overlay; every other failure is best-effort — it logs,counts
swap_failed, and boots the original envd. The swap is time-bounded on a cancel-free contextso a request cancellation can't kill
debugfsmid-write.Reuses the merged upgrade decision (
packages/shared/pkg/featureflags/flags.go). A newResolveEnvdOfflineUpgradeforwards to the same pureresolveEnvdUpgradePaththe live path uses(built-with vs target version compare, upgrade-only, path-traversal-safe) — one decision, two
delivery mechanisms. It is keyed on a sibling flag
envd-offline-upgrade-target(sameoff/promoted/<sha>grammar asenvd-upgrade-target) so the newer offline mechanism rampsindependently of the vetted live path, and threads the sandbox/template LD context for
%-ramptargeting.
Metrics (
packages/shared/pkg/telemetry/meters.go):orchestrator.envd.offline_upgrade.attempts{result,from_version,to_version}(result ∈success/swap_failed/not_quiesced) and.duration{result}.envd-offline-upgrade-targetoffConvergence (by design). The gate keys on the snapshot's built-with version (there is no
running
envdat cold-boot swap time), which the swap does not advance and pause does notoverwrite with the live version. So a re-resumed, already-upgraded snapshot resolves the upgrade
again and re-fires the swap on every cold boot — harmlessly (the on-disk binary is already the
target;
debugfsreplaces same-with-same). This matches the already-shipped live path, whichre-upgrades on every memory-resume. True cross-pause convergence (persist the running version at
pause) is a deliberately deferred, orthogonal follow-up.
Validation
-race,golangci-lintclean):ResolveEnvdOfflineUpgradereads the sibling flag and is independent of thelive-path flag (the two ramp separately);
same_versionno-op;decideOfflineSwapswaps only when an upgrade is resolved and the rootfs isfrozen, counts the
not_quiescedskip, stays silent on benign no-ops, logs misconfiguredtargets — the frozen gate is what keeps a torn-journal rootfs from being rewritten;
debugfsdevice allowlist refuses anything but a bare/dev/nbd*(verified without root/debugfs) and the output cap holds;
systemd-analyze securityon a live unit with the jail's exactproperty set scores 0.4 (SAFE); a fixture ext4 driven through the real
write+sif+stat+dumpsequence under that hardened jail on a dev node produced the expected inode (regular, mode
0755,matching content sha), confirming the tightened syscall/mapping restrictions don't break
debugfs.decideOfflineSwapfactor-out;identical results). One sandbox from an
envd-0.6.5 template,envd-offline-upgrade-target=vb→
/fc-envd/envd.vb(0.6.12), twopause(memory:false)→ resume cycles:sha256(/usr/bin/envd)/ version:a0810802…/0.6.5 →8e05066c…/0.6.12 →8e05066c…/0.6.12 (swapped on cycle 1, unchanged on cycle 2);swapped envd binary before reboot, bothbuilt_with=0.6.5 → envd.vb/0.6.12;orchestrator_envd_offline_upgrade_attempts_total{result="success",from_version="0.6.5", to_version="0.6.12"} = 2.debugfsin CI — so this was validated on a dev node):
jailed
debugfswrites its backup + verifydumps successfully and the content-hash verifypasses) — confirmed by the in-guest hash flip + the
swapped envd binary before rebootlog;rm-succeeds/write-fails injection, the verify caught theemptied binary (
got sha e3b0c442…[empty] vs the 0.6.12 target) and the rollback restored theoriginal — the guest booted and stayed responsive on 0.6.5, never a rootfs without envd;
sif 0100644(content correct, mode wrong), theverify failed with
/usr/bin/envd is not an executable regular file after writeand therollback again restored a bootable 0.6.5 — proving the mode-check catches a silent
siffailure a content-only check would pass;
read-back matched the original, so it logged
swap did not apply; original left in placeandbooted 0.6.5 with no rollback — proving an intact rootfs is never rm'd, and (with the
rollback passes above) that the swap decision is driven by the read-back state, not the exit code.
Key decisions
debugfsin a seccomp jail, not a host mount — mounting a customer-controlledext4 image on the host kernel exposes the node and co-tenants to the ext4 parser's CVE surface;
the jailed userspace rewrite contains a crash.
fs_quiesced(feat(orch): persist an fs-quiesced flag on filesystem-only pause, with telemetry #3445), no repair — only rewrite rootfs known to be frozen at pause(crash-consistent). Legacy/sync-fallback snapshots are left on their
envdand become eligibleafter their next freezing pause; there is deliberately no host-side journal repair.
live path, so it shares
resolveEnvdUpgradePath; a separate flag keeps the offline mechanism'srollout blast radius independent.
debugfs -fexits 0 even when a scripted commandfails, so the primitive backs the original out to the host, verifies the result by content sha
after the swap, and rolls the original back on any mismatch; a swap failure boots the original
envd, so the feature can only make a sandbox newer, never unbootable. (An atomic in-placerename via
debugfs lnwas rejected — itsln/unlinkdon't maintain inode link counts andwould leave an fsck-dirty rootfs.)
cross-pause convergence is deferred rather than bundled here.
debugfsrewrite (fixture ext4 → assertbytes/mode/no-other-inode-changed) — it needs
debugfs+ a loop image, so it is left as abuild-tagged follow-up; the rewrite is currently covered by the dev-cluster e2e.
off; no customer-facing API change; noenvd/proto/schema changes.🤖 Generated with Claude Code