[PR #291] [MERGED] Decouple firecracker #322

New issue

Closed

opened 2026-10-29 14:22:11 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-29 14:22:11 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/kvcache-ai/AgentENV/pull/291
Author: @RuoqingHe
Created: 9/15/2026
Status: ✅ Merged
Merged: 9/16/2026
Merged by: @yingdi-shan

Base: main ← Head: decouple-firecracker

📝 Commits (3)

0b6f25a refactor(sandbox): decouple snapshot manifest from Firecracker
0860af5 refactor(sandbox): carry manifest on captured snapshot
01db5da refactor(template): build templates on any sandbox backend

📊 Changes

21 files changed (+254 additions, -164 deletions)

View changed files

📝 crates/e2e-tests/tests/snapshot_oss_e2e_test.rs (+3 -3)
📝 src/orchestrator/tests.rs (+2 -2)
📝 src/sandbox/backend.rs (+29 -15)
📝 src/sandbox/firecracker/config.rs (+9 -0)
📝 src/sandbox/firecracker/mod.rs (+1 -3)
📝 src/sandbox/firecracker/sandbox.rs (+13 -38)
📝 src/sandbox/manifest.rs (+60 -27)
📝 src/sandbox/mock.rs (+15 -1)
📝 src/sandbox/mod.rs (+5 -3)
📝 src/snapshot/manager.rs (+5 -12)
📝 src/snapshot/mock.rs (+5 -4)
📝 src/snapshot/repository/backends/oss/repository.rs (+6 -6)
📝 src/snapshot/repository/backends/oss/resolver.rs (+1 -1)
📝 src/snapshot/repository/backends/posixfs/artifacts.rs (+3 -3)
📝 src/snapshot/repository/backends/posixfs/backend.rs (+8 -7)
📝 src/snapshot/repository/backends/posixfs/runtime.rs (+1 -1)
📝 src/snapshot/repository/interfaces.rs (+2 -2)
📝 src/snapshot/runtime_support.rs (+5 -5)
📝 src/snapshot/types/snapshot.rs (+5 -5)
📝 src/snapshot/types/version.rs (+14 -9)

...and 1 more files

📄 Description

What

Move the snapshot manifest out of the firecracker module and make the capture
path generic over the sandbox backend. Three commits: the manifest and its types
lose the Firecracker prefix and gain a backend field; CapturedSandboxSnapshot
carries that manifest instead of an opaque payload the snapshot layer had to
downcast; SnapshotRuntimeVersions::probe and run_template_build stop naming a
concrete VMM type.

No behavior changes for Firecracker beyond one new guard: a snapshot whose
manifest names another backend is now refused at config time instead of being
misread.

Why

Three places in the snapshot and template layers were written against
FirecrackerSandbox and FirecrackerSnapshotManifest even though nothing about
them is Firecracker-specific. SnapshotManager::publish_captured downcast the
capture to a Firecracker type and returned RepositoryError::Unsupported for
anything else, so a second backend could run a sandbox but never publish a
snapshot from it. SnapshotRuntimeVersions::probe reached into
FirecrackerSandbox for the binary to version and the tools drive it was given,
and run_template_build constructed that type itself.

This is the groundwork for a second VMM backend. Landing it on its own keeps that
change reviewable and keeps this one free of any new backend's code.

Scope and non-goals

Included — the manifest move and rename, the backend field, the capture
payload split, and the two generalizations in probe / run_template_build.

Excluded — any second backend. No new VMM, no config keys, no factory
selection, no changes under src/bin/server.rs or src/cfg.rs. LINGCORE_BACKEND
and the widened visibilities a second backend needs are deliberately left out and
belong with that backend.

Unrelated refactor, called out — run_template_build took eight arguments
after the generalization, which clippy rejects. The five that describe the build
now travel as TemplateBuildInputs rather than an #[allow].

Design and behavior changes

SandboxSnapshotManifest gains backend: String, carrying the name of the VMM
that wrote the capture. FirecrackerSnapshotConfig::from_snapshot refuses a
manifest naming a different backend; previously such a manifest would have been
read as Firecracker's own.

CapturedSandboxSnapshot splits into two parts: the manifest, which every backend
produces and SnapshotManager reads directly, and the artifacts a backend must
keep alive until publication finishes, which stay Box<dyn Any>. The
RepositoryError::Unsupported branch in publish_captured disappears — there is
no longer a downcast that can fail. FirecrackerCapturedSnapshot is deleted.

SandboxBackend gains capture_to_dir(&mut self, at: &Path), which captures into
a directory and leaves the guest held. The template build takes its snapshot
through it, so the build no longer needs the concrete type. Firecracker implements
it in terms of the existing pause_to_dir, so the capture and recovery paths are
unchanged.

Compatibility and operations

Public API or generated protocol: Rust API only. FirecrackerSnapshotManifest
→ SandboxSnapshotManifest, the four Firecracker*Artifacts → Snapshot*Artifacts,
and agentenv::sandbox::FirecrackerCapturedSnapshot is removed. The only
in-tree consumer outside src/ is crates/e2e-tests, updated in the first
commit. No OpenAPI, protobuf or generated client changes.
Configuration or defaults: N/A — no config key added, removed or redefined.
Snapshot manifest, artifact layout, or storage format: MANIFEST_FORMAT_VERSION
stays 1 and the on-disk name stays firecracker-manifest.json. The one wire
change is the added backend field, which is #[serde(default)] to
"firecracker", so a manifest written before this change reads as a Firecracker
capture. Artifact layout is untouched.
Upgrade and rollback: Both directions are safe. Upgrading, existing published
snapshots deserialize with backend = "firecracker" and restore unchanged.
Rolling back, no struct in the snapshot layer sets deny_unknown_fields, so an
older binary ignores the extra key and reads the manifest as it always did.
Host requirements, permissions, ports, or dependencies: N/A — no new
dependency, binary, port or capability.

Validation

make fmt
make clippy
make test-unit
Relevant Rust integration tests
make -C services test (required when services/ changes)
Generated clients/server regenerated with the documented make target
Documentation updated
Benchmarks or performance comparison completed

Commands and results:

$ make fmt
cargo fmt --all -- --check
exit 0

$ make clippy
cargo clippy --workspace --all-targets --all-features -- -D warnings
exit 0

$ make test-unit
test result: ok. 885 passed; 0 failed; 4 ignored     (agentenv --lib)
test result: ok. 3 passed; 0 failed; 0 ignored       (linux-cap --lib)
test result: ok. 55 passed; 0 failed; 0 ignored      (aenv --bin aenv)
test result: ok. 4 passed; 0 failed; 885 filtered    (agentenv --lib -- --ignored)
test result: ok. 8 passed; 0 failed; 0 ignored       (uvm-ublk --lib)
test result: ok. 48 passed; 0 failed; 0 ignored      (uvm-ublk-daemon --lib)
exit 0

Skipped checks and reasons:

Rust integration tests — make test-agent-integration boots real microVMs
and needs host state this change does not touch. The paths it covers are
exercised by the unit suite above; a maintainer with a VM host should run it
before merge.
make -C services test — nothing under services/ changes.
Generated clients/server — no OpenAPI or protobuf source changes.
Documentation — no user-facing behavior changes; the affected types are
internal Rust API.
Benchmarks — the change moves types and adds one trait method. No hot path
gains or loses work.

Risks and reviewer notes

The riskiest commit is the second: SnapshotManager::publish_captured previously
downcast the capture and returned Unsupported when the downcast failed, and now
reads the manifest off the capture directly. Worth confirming the deleted error
branch was genuinely unreachable for Firecracker.

Read in commit order. refactor(sandbox): decouple snapshot manifest from Firecracker is large but almost entirely a rename — the substance is
src/sandbox/manifest.rs (the backend field) and the guard added in
src/sandbox/firecracker/config.rs; the ten files under src/snapshot/repository/
are the rename rippling outward.

Two naming inconsistencies are left deliberately: the persisted file is still
firecracker-manifest.json and SNAPSHOT_ARTIFACT_LAYOUT.firecracker_manifest
still says so. Renaming either would change the on-disk layout, which this PR
avoids. Worth a follow-up once a second backend exists.

capture_to_dir is added to SandboxBackend with no default, so any out-of-tree
implementor of the trait must add it.

Checklist

The PR contains one coherent change and no unrelated formatting or refactoring.
New behavior is covered by tests, or I explained why testing is impractical.
Logs and examples contain no credentials, tokens, or private registry information.
I did not manually edit generated code without updating its source and regenerating it.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/kvcache-ai/AgentENV/pull/291 **Author:** [@RuoqingHe](https://github.com/RuoqingHe) **Created:** 9/15/2026 **Status:** ✅ Merged **Merged:** 9/16/2026 **Merged by:** [@yingdi-shan](https://github.com/yingdi-shan) **Base:** `main` ← **Head:** `decouple-firecracker` --- ### 📝 Commits (3) - [`0b6f25a`](https://github.com/kvcache-ai/AgentENV/commit/0b6f25acc5316cd674fbf8bd6d096b4cda367f1b) refactor(sandbox): decouple snapshot manifest from Firecracker - [`0860af5`](https://github.com/kvcache-ai/AgentENV/commit/0860af52cf66eda4c8bd5885ae633011edf213cd) refactor(sandbox): carry manifest on captured snapshot - [`01db5da`](https://github.com/kvcache-ai/AgentENV/commit/01db5dac926bf7cc5feb7a6e87c28990cfcfc011) refactor(template): build templates on any sandbox backend ### 📊 Changes **21 files changed** (+254 additions, -164 deletions) <details> <summary>View changed files</summary> 📝 `crates/e2e-tests/tests/snapshot_oss_e2e_test.rs` (+3 -3) 📝 `src/orchestrator/tests.rs` (+2 -2) 📝 `src/sandbox/backend.rs` (+29 -15) 📝 `src/sandbox/firecracker/config.rs` (+9 -0) 📝 `src/sandbox/firecracker/mod.rs` (+1 -3) 📝 `src/sandbox/firecracker/sandbox.rs` (+13 -38) 📝 `src/sandbox/manifest.rs` (+60 -27) 📝 `src/sandbox/mock.rs` (+15 -1) 📝 `src/sandbox/mod.rs` (+5 -3) 📝 `src/snapshot/manager.rs` (+5 -12) 📝 `src/snapshot/mock.rs` (+5 -4) 📝 `src/snapshot/repository/backends/oss/repository.rs` (+6 -6) 📝 `src/snapshot/repository/backends/oss/resolver.rs` (+1 -1) 📝 `src/snapshot/repository/backends/posixfs/artifacts.rs` (+3 -3) 📝 `src/snapshot/repository/backends/posixfs/backend.rs` (+8 -7) 📝 `src/snapshot/repository/backends/posixfs/runtime.rs` (+1 -1) 📝 `src/snapshot/repository/interfaces.rs` (+2 -2) 📝 `src/snapshot/runtime_support.rs` (+5 -5) 📝 `src/snapshot/types/snapshot.rs` (+5 -5) 📝 `src/snapshot/types/version.rs` (+14 -9) _...and 1 more files_ </details> ### 📄 Description ## What Move the snapshot manifest out of the `firecracker` module and make the capture path generic over the sandbox backend. Three commits: the manifest and its types lose the `Firecracker` prefix and gain a `backend` field; `CapturedSandboxSnapshot` carries that manifest instead of an opaque payload the snapshot layer had to downcast; `SnapshotRuntimeVersions::probe` and `run_template_build` stop naming a concrete VMM type. No behavior changes for Firecracker beyond one new guard: a snapshot whose manifest names another backend is now refused at config time instead of being misread. ## Why Three places in the snapshot and template layers were written against `FirecrackerSandbox` and `FirecrackerSnapshotManifest` even though nothing about them is Firecracker-specific. `SnapshotManager::publish_captured` downcast the capture to a Firecracker type and returned `RepositoryError::Unsupported` for anything else, so a second backend could run a sandbox but never publish a snapshot from it. `SnapshotRuntimeVersions::probe` reached into `FirecrackerSandbox` for the binary to version and the tools drive it was given, and `run_template_build` constructed that type itself. This is the groundwork for a second VMM backend. Landing it on its own keeps that change reviewable and keeps this one free of any new backend's code. ## Scope and non-goals **Included** — the manifest move and rename, the `backend` field, the capture payload split, and the two generalizations in `probe` / `run_template_build`. **Excluded** — any second backend. No new VMM, no config keys, no factory selection, no changes under `src/bin/server.rs` or `src/cfg.rs`. `LINGCORE_BACKEND` and the widened visibilities a second backend needs are deliberately left out and belong with that backend. **Unrelated refactor, called out** — `run_template_build` took eight arguments after the generalization, which clippy rejects. The five that describe the build now travel as `TemplateBuildInputs` rather than an `#[allow]`. ## Design and behavior changes `SandboxSnapshotManifest` gains `backend: String`, carrying the name of the VMM that wrote the capture. `FirecrackerSnapshotConfig::from_snapshot` refuses a manifest naming a different backend; previously such a manifest would have been read as Firecracker's own. `CapturedSandboxSnapshot` splits into two parts: the manifest, which every backend produces and `SnapshotManager` reads directly, and the artifacts a backend must keep alive until publication finishes, which stay `Box<dyn Any>`. The `RepositoryError::Unsupported` branch in `publish_captured` disappears — there is no longer a downcast that can fail. `FirecrackerCapturedSnapshot` is deleted. `SandboxBackend` gains `capture_to_dir(&mut self, at: &Path)`, which captures into a directory and leaves the guest held. The template build takes its snapshot through it, so the build no longer needs the concrete type. Firecracker implements it in terms of the existing `pause_to_dir`, so the capture and recovery paths are unchanged. ## Compatibility and operations - **Public API or generated protocol:** Rust API only. `FirecrackerSnapshotManifest` → `SandboxSnapshotManifest`, the four `Firecracker*Artifacts` → `Snapshot*Artifacts`, and `agentenv::sandbox::FirecrackerCapturedSnapshot` is removed. The only in-tree consumer outside `src/` is `crates/e2e-tests`, updated in the first commit. No OpenAPI, protobuf or generated client changes. - **Configuration or defaults:** N/A — no config key added, removed or redefined. - **Snapshot manifest, artifact layout, or storage format:** `MANIFEST_FORMAT_VERSION` stays `1` and the on-disk name stays `firecracker-manifest.json`. The one wire change is the added `backend` field, which is `#[serde(default)]` to `"firecracker"`, so a manifest written before this change reads as a Firecracker capture. Artifact layout is untouched. - **Upgrade and rollback:** Both directions are safe. Upgrading, existing published snapshots deserialize with `backend = "firecracker"` and restore unchanged. Rolling back, no struct in the snapshot layer sets `deny_unknown_fields`, so an older binary ignores the extra key and reads the manifest as it always did. - **Host requirements, permissions, ports, or dependencies:** N/A — no new dependency, binary, port or capability. ## Validation - [x] `make fmt` - [x] `make clippy` - [x] `make test-unit` - [ ] Relevant Rust integration tests - [ ] `make -C services test` (required when `services/` changes) - [ ] Generated clients/server regenerated with the documented `make` target - [ ] Documentation updated - [ ] Benchmarks or performance comparison completed Commands and results: ```text $ make fmt cargo fmt --all -- --check exit 0 $ make clippy cargo clippy --workspace --all-targets --all-features -- -D warnings exit 0 $ make test-unit test result: ok. 885 passed; 0 failed; 4 ignored (agentenv --lib) test result: ok. 3 passed; 0 failed; 0 ignored (linux-cap --lib) test result: ok. 55 passed; 0 failed; 0 ignored (aenv --bin aenv) test result: ok. 4 passed; 0 failed; 885 filtered (agentenv --lib -- --ignored) test result: ok. 8 passed; 0 failed; 0 ignored (uvm-ublk --lib) test result: ok. 48 passed; 0 failed; 0 ignored (uvm-ublk-daemon --lib) exit 0 ``` Skipped checks and reasons: - **Rust integration tests** — `make test-agent-integration` boots real microVMs and needs host state this change does not touch. The paths it covers are exercised by the unit suite above; a maintainer with a VM host should run it before merge. - **`make -C services test`** — nothing under `services/` changes. - **Generated clients/server** — no OpenAPI or protobuf source changes. - **Documentation** — no user-facing behavior changes; the affected types are internal Rust API. - **Benchmarks** — the change moves types and adds one trait method. No hot path gains or loses work. ## Risks and reviewer notes The riskiest commit is the second: `SnapshotManager::publish_captured` previously downcast the capture and returned `Unsupported` when the downcast failed, and now reads the manifest off the capture directly. Worth confirming the deleted error branch was genuinely unreachable for Firecracker. Read in commit order. `refactor(sandbox): decouple snapshot manifest from Firecracker` is large but almost entirely a rename — the substance is `src/sandbox/manifest.rs` (the `backend` field) and the guard added in `src/sandbox/firecracker/config.rs`; the ten files under `src/snapshot/repository/` are the rename rippling outward. Two naming inconsistencies are left deliberately: the persisted file is still `firecracker-manifest.json` and `SNAPSHOT_ARTIFACT_LAYOUT.firecracker_manifest` still says so. Renaming either would change the on-disk layout, which this PR avoids. Worth a follow-up once a second backend exists. `capture_to_dir` is added to `SandboxBackend` with no default, so any out-of-tree implementor of the trait must add it. ## Checklist - [x] The PR contains one coherent change and no unrelated formatting or refactoring. - [x] New behavior is covered by tests, or I explained why testing is impractical. - [x] Logs and examples contain no credentials, tokens, or private registry information. - [x] I did not manually edit generated code without updating its source and regenerating it. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-29 14:22:11 +00:00