feat: implement SnapStart compatibility layer with gRPC northbound API - #257
Open
lyuyun wants to merge 2 commits into
Open
feat: implement SnapStart compatibility layer with gRPC northbound API#257lyuyun wants to merge 2 commits into
lyuyun wants to merge 2 commits into
Conversation
Introduces a gRPC-based compatibility layer that bridges upper-layer orchestrators with Kuasar's existing snapshot/restore ecosystem. Signed-off-by: lyuyun <lyuyun068@gmail.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements a SnapStart compatibility layer for Kuasar, introducing a dedicated gRPC service for sandbox instance and snapshot artifact lifecycles, a reverse index for fast pod-to-sandbox lookups, and a resolver chain for CRI-triggered restores. Feedback on the changes highlights several critical issues, including a bug where deleting a snapshot could accidentally target the root store directory, a leak in the reverse index when destroying sandboxes, silent parsing of malformed generation parameters, unpropagated deletion errors in the gRPC service, and unnecessary warnings logged during unmount operations.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+272
to
274
| // Best-effort: remove the empty pod-UID parent left by the two-level layout. | ||
| let _ = tokio::fs::remove_dir(dir.parent().unwrap_or(&self.store_dir)).await; | ||
| Ok(true) |
Contributor
There was a problem hiding this comment.
If key does not contain a slash (e.g., when it is a user-supplied snapshot_name or fallback sandbox_id), dir.parent() will resolve to self.store_dir. Attempting to delete it via remove_dir will try to delete the root directory of the continuation store, which is a critical bug. We should ensure we only delete the parent directory if it is not the root store directory.
// Best-effort: remove the empty pod-UID parent left by the two-level layout.
if let Some(parent) = dir.parent() {
if parent != self.store_dir {
let _ = tokio::fs::remove_dir(parent).await;
}
}
Ok(true)
Comment on lines
+199
to
+201
| handle.sandboxes.write().await.remove(sandbox_id); | ||
| info!("service:destroyed sandbox {}", sandbox_id); | ||
| Ok(()) |
Contributor
There was a problem hiding this comment.
When a sandbox is destroyed via the gRPC service (destroy_sandbox), it is removed from handle.sandboxes but its entry in handle.pod_uid_index is not cleaned up. This leaves stale mappings in the reverse index. We should clean up the index entry during destruction.
handle.sandboxes.write().await.remove(sandbox_id);
handle.pod_uid_index.write().await.retain(|_, sid| sid != sandbox_id);
info!("service:destroyed sandbox {}", sandbox_id);
Ok(())
Comment on lines
+386
to
+390
| let generation = req | ||
| .parameters | ||
| .get("generation") | ||
| .and_then(|g| g.parse::<u64>().ok()) | ||
| .unwrap_or(0); |
Contributor
There was a problem hiding this comment.
If the generation parameter is present but malformed (e.g., non-numeric), parsing it silently defaults to 0. This can lead to silent state mismatches or matching the wrong snapshot generation. We should return an invalid argument error if the parameter is present but cannot be parsed.
let generation = match req.parameters.get("generation") {
Some(g) => g.parse::<u64>().map_err(|_| {
Status::invalid_argument(format!("invalid generation parameter: {}", g))
})?,
None => 0,
};
Comment on lines
+441
to
+460
| if let Some(pool) = &handle.inner.pool { | ||
| let found = pool | ||
| .list_templates() | ||
| .await | ||
| .into_iter() | ||
| .find(|t| t.key.key == req.snapshot_name); | ||
| if let Some(tmpl) = found { | ||
| let _ = pool.remove_by_id(&tmpl.id, &SnapshotType::WarmFork).await; | ||
| } | ||
| } | ||
| if let Some(cs) = &handle.inner.continuation_store { | ||
| let found = cs | ||
| .list() | ||
| .await | ||
| .into_iter() | ||
| .find(|t| t.key.key == req.snapshot_name); | ||
| if let Some(tmpl) = found { | ||
| let _ = cs.delete_by_template_id(&tmpl.id).await; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Errors returned by remove_by_id and delete_by_template_id are silently ignored using let _ = .... If the deletion fails (e.g., due to I/O errors or permission issues), the gRPC call will still return success. We should propagate these errors to the client.
Suggested change
| if let Some(pool) = &handle.inner.pool { | |
| let found = pool | |
| .list_templates() | |
| .await | |
| .into_iter() | |
| .find(|t| t.key.key == req.snapshot_name); | |
| if let Some(tmpl) = found { | |
| let _ = pool.remove_by_id(&tmpl.id, &SnapshotType::WarmFork).await; | |
| } | |
| } | |
| if let Some(cs) = &handle.inner.continuation_store { | |
| let found = cs | |
| .list() | |
| .await | |
| .into_iter() | |
| .find(|t| t.key.key == req.snapshot_name); | |
| if let Some(tmpl) = found { | |
| let _ = cs.delete_by_template_id(&tmpl.id).await; | |
| } | |
| } | |
| if let Some(pool) = &handle.inner.pool { | |
| let found = pool | |
| .list_templates() | |
| .await | |
| .into_iter() | |
| .find(|t| t.key.key == req.snapshot_name); | |
| if let Some(tmpl) = found { | |
| pool.remove_by_id(&tmpl.id, &SnapshotType::WarmFork).await.map_err(grpc_err)?; | |
| } | |
| } | |
| if let Some(cs) = &handle.inner.continuation_store { | |
| let found = cs | |
| .list() | |
| .await | |
| .into_iter() | |
| .find(|t| t.key.key == req.snapshot_name); | |
| if let Some(tmpl) = found { | |
| cs.delete_by_template_id(&tmpl.id).await.map_err(grpc_err)?; | |
| } | |
| } |
Comment on lines
+428
to
+439
| fn unmount_preserved_netns(dir: &Path) { | ||
| let preserved = dir.join("preserved_netns"); | ||
| if let Some(path_str) = preserved.to_str() { | ||
| if let Err(e) = vmm_common::mount::unmount(path_str, MNT_DETACH) { | ||
| log::warn!( | ||
| "continuation store: unmount preserved_netns at {}: {}", | ||
| preserved.display(), | ||
| e | ||
| ); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
The unmount_preserved_netns function is called for all deleted continuation templates. However, non-Continuation snapshots do not have a preserved_netns file, causing unmount to fail and log a warning. We should check if the file exists before attempting to unmount it to avoid spamming the logs.
Suggested change
| fn unmount_preserved_netns(dir: &Path) { | |
| let preserved = dir.join("preserved_netns"); | |
| if let Some(path_str) = preserved.to_str() { | |
| if let Err(e) = vmm_common::mount::unmount(path_str, MNT_DETACH) { | |
| log::warn!( | |
| "continuation store: unmount preserved_netns at {}: {}", | |
| preserved.display(), | |
| e | |
| ); | |
| } | |
| } | |
| } | |
| fn unmount_preserved_netns(dir: &Path) { | |
| let preserved = dir.join("preserved_netns"); | |
| if preserved.exists() { | |
| if let Some(path_str) = preserved.to_str() { | |
| if let Err(e) = vmm_common::mount::unmount(path_str, MNT_DETACH) { | |
| log::warn!( | |
| "continuation store: unmount preserved_netns at {}: {}", | |
| preserved.display(), | |
| e | |
| ); | |
| } | |
| } | |
| } | |
| } |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a gRPC-based “northbound” compatibility layer for Kuasar snapshot/restore, enabling orchestrators to manage sandbox snapshots (create/list/delete) and sandbox pause/resume via typed gRPC APIs over a Unix socket, while preserving the existing admin JSON socket for operational/pool controls. It also refactors service code into focused modules and extends snapshot/restore internals (network handling, continuation store layout, CH restore plumbing) to support WarmFork and Continuation reliably across the new flows.
Changes:
- Add gRPC services (
SandboxController,SandboxSnapshotController/SSI) plus newvmm-apicrate and gRPC clients/CLI plumbing. - Refactor sandboxer service code into modules (
service/{admin,grpc,sandbox,snapshot}) and add annotation-based restore intent resolver chain. - Enhance snapshot/restore internals: WarmFork MAC/IP handling + GARP, Continuation store layout/metadata, and Cloud Hypervisor restore (config/state patching + net_fds support).
Reviewed changes
Copilot reviewed 39 out of 40 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| vmm/task/src/netlink.rs | WarmFork restore networking fixes: name-based fallback, MAC update, explicit UP, gratuitous ARP. |
| vmm/scripts/image/common.sh | Install protobuf-compiler for proto generation. |
| vmm/sandbox/src/vm.rs | Extend VM trait with pause/resume and set_netns defaults. |
| vmm/sandbox/src/version.rs | Add version_string() helper using built git version. |
| vmm/sandbox/src/template/types.rs | Adjust WarmFork network hotplug requirement; change workload generation to u64; update TemplateKey layout; add netns to template metadata. |
| vmm/sandbox/src/template/pool.rs | Update tests for new TemplateKey format. |
| vmm/sandbox/src/template/continuation.rs | Support mixed-depth store layout, key-based acquire/list/delete, and unmount preserved netns on cleanup. |
| vmm/sandbox/src/storage/guest_file.rs | Handle empty-content injection without blocking on cat. |
| vmm/sandbox/src/service/snapshot.rs | New snapshot-from-sandbox implementation (WarmFork + Continuation), including checks and netns preservation. |
| vmm/sandbox/src/service/sandbox.rs | New sandbox slot creation/destruction and restore flows (WarmFork/Continuation + paused resume). |
| vmm/sandbox/src/service/grpc.rs | New tonic gRPC server implementing SandboxController + SSI snapshot controller. |
| vmm/sandbox/src/service/admin.rs | New admin JSON socket server for pool/template diagnostics/maintenance. |
| vmm/sandbox/src/sandbox.rs | Integrate resolver chain, pod_uid index, pause status handling, continuation restore networking changes, tap reopen support. |
| vmm/sandbox/src/restore.rs | Remove restore phase for post-restore network hotplug. |
| vmm/sandbox/src/resolver.rs | New restore-intent resolver trait + native/mapping resolver implementations. |
| vmm/sandbox/src/network/link.rs | Add tap reopen helper for IFF_PERSIST continuation restores. |
| vmm/sandbox/src/lib.rs | Export new resolver module. |
| vmm/sandbox/src/cloud_hypervisor/snapshot.rs | Add config/state patching for restore, read net device IDs/configs, and network-device handling rules per snapshot type. |
| vmm/sandbox/src/cloud_hypervisor/mod.rs | Implement pause/resume and set_netns, clear stale net state on stop, improve restore flow and pre-resume hotplug, pass net_fds. |
| vmm/sandbox/src/cloud_hypervisor/devices/block.rs | Add disk image_type support to avoid CH autodetect pitfalls. |
| vmm/sandbox/src/cloud_hypervisor/client.rs | Support disk image_type and extend vm_restore to send net_fds via SCM_RIGHTS. |
| vmm/sandbox/src/bin/cloud_hypervisor/main.rs | Start gRPC server alongside legacy admin socket. |
| vmm/sandbox/src/args.rs | Add --grpc-listen option. |
| vmm/sandbox/Cargo.toml | Add tonic/tower/tokio-stream deps and vmm-api dependency. |
| vmm/common/Cargo.toml | Add tonic dependency; remove tonic-build from build-deps. |
| vmm/client/src/template.rs | Align admin-socket template client with new admin API scope (remove create/extra kind fields). |
| vmm/client/src/snapshot.rs | New gRPC SSI snapshot client. |
| vmm/client/src/sandbox.rs | Switch sandbox client to gRPC and update returned fields. |
| vmm/client/src/lib.rs | Export new snapshot module; update examples. |
| vmm/client/Cargo.toml | Add tonic/tower and depend on vmm-api/vmm-common. |
| vmm/api/src/protos/ssi.proto | New SSI proto definition for snapshot lifecycle + plugin introspection. |
| vmm/api/src/protos/sandbox.proto | New proto definition for sandbox pause/resume/list/get. |
| vmm/api/src/lib.rs | New generated-code wrapper modules. |
| vmm/api/Cargo.toml | New vmm-api crate with tonic/prost build. |
| vmm/api/build.rs | Generate Rust code from protos via tonic-build. |
| tools/kuasar-ctl/src/main.rs | Add snapshot subcommand and switch sandbox operations to gRPC; update pool/template commands. |
| docs/proposals/snapstart_compat_layer.md | New design proposal describing the gRPC layer and restore flows. |
| Cargo.toml | Add vmm/api to workspace members. |
| Cargo.lock | Lockfile updates for new crates/deps. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+425
to
+427
| Ok(Response::new(CreateSandboxSnapshotResponse { | ||
| snapshot: Some(template_to_proto(&tmpl, &req.snapshot_name, &req.pod_uid)), | ||
| })) |
Comment on lines
+291
to
+294
| let snapshot_mode = match sb.restore.template_snapshot_type.as_ref() { | ||
| Some(SnapshotType::Continuation) => SnapshotMode::Continuation as i32, | ||
| _ => SnapshotMode::WarmFork as i32, | ||
| }; |
Comment on lines
+367
to
+371
| if req.snapshot_name.contains('/') { | ||
| return Err(Status::invalid_argument( | ||
| "snapshot_name must not contain '/'; that separator is reserved for auto-generated continuation keys", | ||
| )); | ||
| } |
Comment on lines
+566
to
+570
| let _ = std::fs::remove_file(sock_path); | ||
|
|
||
| let uds = UnixListener::bind(sock_path)?; | ||
| let uds_stream = UnixListenerStream::new(uds); | ||
|
|
Comment on lines
+362
to
+367
| let kind_str = match req["kind"].as_str() { | ||
| Some(k) => k.to_string(), | ||
| None => { | ||
| return Ok(json!({"ok": false, "error": "missing required field 'kind'"})); | ||
| } | ||
| }; |
Comment on lines
+224
to
234
| let mut sub = match tokio::fs::read_dir(&top_path).await { | ||
| Ok(d) => d, | ||
| Err(_) => continue, | ||
| }; | ||
| while let Ok(Some(sub_entry)) = sub.next_entry().await { | ||
| let sub_path = sub_entry.path(); | ||
| if sub_path.is_dir() { | ||
| result.push(sub_path); | ||
| } | ||
| } | ||
| } |
Comment on lines
+19
to
+21
| // PauseSandbox: pause the VM vCPUs of a running sandbox. | ||
| // The CH process stays alive; the sandbox netns, tap, and TC rules are untouched. | ||
| message PauseSandboxRequest { string sandbox_id = 1; } |
Comment on lines
+255
to
+257
| let info = SnapshotApi::new(&grpc_sock) | ||
| .create(name.as_deref().unwrap_or(""), &pod_uid, &mode, generation) | ||
| .await?; |
Comment on lines
+191
to
+193
| /// Pause the VM vCPUs of a running sandbox. | ||
| /// The CH process stays alive; network (tap, TC rules, netns) is untouched. | ||
| Pause { |
Comment on lines
+47
to
+49
| /// Pause the VM vCPUs of a running sandbox. | ||
| /// The CH process stays alive; network (tap, TC rules) is untouched. | ||
| pub async fn pause(&self, sandbox_id: &str) -> Result<()> { |
Security fixes:
- continuation.rs: guard against deleting store root when key has no slash
- continuation.rs: validate key consistency in acquire_from_dir to reject corrupted entries
- continuation.rs: require pooled_template.json for two-level sub-directories in entry_dirs()
- grpc.rs: block path traversal components ('/', '..', '.') in snapshot_name
- grpc.rs: restrict gRPC Unix socket permissions to 0o600
Bug fixes:
- sandbox.rs: clean up pod_uid_index on destroy_sandbox to prevent stale index entries
- grpc.rs: return InvalidArgument for malformed 'generation' parameter instead of silently defaulting to 0
- grpc.rs: propagate pool/continuation-store deletion errors to gRPC caller
- grpc.rs: return the derived key (not the empty request field) in CreateSandboxSnapshot response
- grpc.rs: fix get_sandbox snapshot_mode to return Unspecified for cold-boot/environment restores, matching list_sandboxes
- admin.rs: remove hard-required 'kind' field from pool-gc
- snapshot.rs: replace blocking PathBuf::exists() with tokio::fs::try_exists() in async context
- kuasar-ctl: add client-side validation for missing --name in warm_fork mode
Documentation fixes:
- sandbox.proto: correct PauseSandbox comment to reflect actual semantics (checkpoint+stop, not vCPU pause)
- kuasar-ctl: update Pause help text to match implementation
- vmm-client: update pause() docstring to match implementation
Signed-off-by: lyuyun <lyuyun068@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Introduces a gRPC-based compatibility layer that bridges upper-layer orchestrators with Kuasar's existing snapshot/restore ecosystem. This enables orchestrators
(e.g., AgentCube) to discover, create, and restore snapshots through a unified gRPC interface without coupling to Kuasar's internal structures.
Problem Statement
Kuasar's snapshot capabilities (WarmFork and Continuation) were previously exposed only through:
This limited orchestrators' ability to programmatically manage snapshots at scale. This PR introduces a proper northbound API.
Key Changes
1. New gRPC Services
SandboxSnapshotController: Snapshot lifecycle management
CreateSandboxSnapshot/DeleteSandboxSnapshot/ListSandboxSnapshotssnapshot_nameto internal template artifactsSandboxController: Pause/Resume API for upper-layer orchestrators
PauseSandbox/ResumeSandbox2. Service Architecture Refactoring
service/mod.rsinto focused modules:service/sandbox.rs: Core sandbox lifecycle (RunPodSandbox, StopPodSandbox)service/snapshot.rs: Snapshot operationsservice/admin.rs: Operational endpoints (inspect, pool management)service/grpc.rs: New gRPC server implementation3. Restore Path Extensibility
RestoreIntentResolvertrait andKuasarNativeResolverimplementation4. Template System Enhancement
start()flow to handle template-based restore operations5. Client Updates
kuasar-ctl snapshotsubcommand for snapshot managementDesign Rationale
Testing Strategy
Compatibility