Repository navigation
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Drive-slot ordering, staging ownership during cancellation, and rejection of already-unmounted volumes remain unresolved.
3 open issues
What changed in this PR
This PR makes snapshot volume replacement release captured guest filesystems before rebinding Firecracker drives.
Changes:
- Stages captured disks until filesystem release is verified.
- Adds replacement lifecycle handling and HTTP 409 errors.
- Documents replacement behavior and adds regression coverage.
| File | Description |
|---|---|
| tests/integration/volume_replacement.rs | Tests replacement and busy-filesystem rejection. |
| tests/integration/snapshot.rs | Updates launch configuration fixtures. |
| tests/integration/posix_startup_pack.rs | Updates launch configuration fixture. |
| tests/integration/orchestrator.rs | Updates creation request fixtures. |
| tests/integration.rs | Registers replacement tests. |
| src/volume/mod.rs | Adds temporary backing materialization. |
| src/sandbox/mod.rs | Exposes replacement configuration and errors. |
| src/sandbox/mock.rs | Records launch configuration for tests. |
| src/sandbox/guest_volumes.rs | Unmounts and verifies filesystem release. |
| src/sandbox/firecracker/sandbox.rs | Stages and swaps snapshot drives. |
| src/sandbox/firecracker/resources.rs | Cleans up staging devices. |
| src/orchestrator/types.rs | Adds replacement drive request data. |
| src/orchestrator/tests.rs | Tests replacement configuration forwarding. |
| src/orchestrator/service.rs | Forwards replacement drives to launch. |
| src/api/openapi.yml | Documents replacement and conflict behavior. |
| src/api/impls/volumes.rs | Materializes captured volume staging configs. |
| src/api/impls/sandbox.rs | Stages replacements and maps busy errors. |
| src/api/impls/image_build/worker.rs | Updates builder request initialization. |
| src/api/generated/src/models.rs | Updates generated field documentation. |
| docs/src/concepts/volumes/sandbox-forks-and-snapshots.md | Explains replacement restrictions. |
| docs/src/concepts/sandboxes/starting.md | Documents replacement launch semantics. |
Review effort: Balanced
| .collect::<Result<Vec<_>, _>>()?; | ||
| // resolve_volume_mounts assigns reserved slots in mount-path order. Snapshot | ||
| // manifests have no slot field, so restore the same order before loading memory. | ||
| snapshots.sort_unstable_by(|(left, _), (right, _)| left.cmp(right)); |
| replaced_snapshot_drives: staged_snapshot_drives | ||
| .as_ref() | ||
| .map(|staged| staged.drives.clone()) | ||
| .unwrap_or_default(), |
| exit 1 | ||
| } | ||
| for entry in "$@"; do | ||
| /agentenv/bin/busybox umount "${entry#*:}" || fail 'non-lazy umount failed' |
|
🔍 OpenCodeReview found 2 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| replaced_snapshot_drives: staged_snapshot_drives | ||
| .as_ref() | ||
| .map(|staged| staged.drives.clone()) | ||
| .unwrap_or_default(), |
There was a problem hiding this comment.
StagedSnapshotDrives owns the only TempDir containing these image.json files, but only the drives (and their paths) are copied into SandboxLaunchConfig. The staged_snapshot_drives local is dropped when this API handler returns, while the sandbox can still use these paths for later startup_artifacts, snapshot/pause, resume, or runtime-artifact processing. Transfer the staging-directory ownership into the sandbox/resources (or otherwise retain it for the sandbox lifetime) rather than retaining only cloned path strings.
| let index = entry | ||
| .as_ref() | ||
| .and_then(|entry| entries.iter().position(|value| value == entry)) | ||
| .unwrap_or(0); |
There was a problem hiding this comment.
When the guest command fails without emitting the entry marker (for example an executor error or timeout while processing a later mount), this falls back to mounts[0]. The resulting SnapshotMountBusy and HTTP 409 can identify the wrong mount, misleading clients about which filesystem is busy. Preserve the failing mount identifier through all script/error paths, or use an explicitly unknown path rather than silently selecting the first entry.
|
Closing in favor of #405. After discussing with maintainers, we decided to reject explicit |
What
Safely replace snapshot volume mounts, including empty arrays, changed paths, and read-only/writable transitions.
Why
Rebinding a drive while the guest retains its old filesystem can reuse a live superblock and cached data against a different disk, risking corruption.
Related issue
Identified while implementing #398. This fix branches directly from
main.Design and behavior changes
Compatibility and operations
Explicit replacement now returns HTTP 409 when captured processes or mount aliases retain an old filesystem. Close files, move process working directories off volumes, and remove aliases before recapturing affected templates. The snapshot format is unchanged.
Validation
Run on Ubuntu 24.04 with non-root test processes and isolated state:
make agentenv-server,make fmt,make clippy.make test-unit: 1210 passed; CLI tests run serially.cargo test -p agentenv --lib: 1021 passed.