feat: support PVM-backed deployment - #87
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| .version | ||
| .as_deref() | ||
| .unwrap_or(&manifest.firecracker.version); | ||
| .unwrap_or(&self.manifest_firecracker().version); |
There was a problem hiding this comment.
[bug · high]
The mode now selects the default artifact, but the resolved cache path remains deps/firecracker/<version>/... (and the kernel/helper paths follow the same mode-independent layout). If KVM and PVM use the same explicit version—or future manifest entries reuse a version label—switching modes makes ensure_* accept the existing nonempty artifact and run the binary/kernel from the other backend. Include virtualization_mode in all managed Firecracker/kernel/helper paths (for example, <dependency>/<mode>/<version>) so the compatibility domain is part of the cache key, and update the path tests accordingly.
| MockBackendFactory::new(), | ||
| persister.clone(), |
There was a problem hiding this comment.
[test · medium]
This does not verify the “without mutation” guarantee for backend/runtime side effects. Because the fixture sets paused_state: None and uses an unobserved factory, the resume path cannot build a backend anyway; a regression that performs backend work before checking the virtualization mode is therefore not covered. Give the metadata a MockSnapshot, construct the factory with a MockBehavior hook for MockOperation::BuildFromSnapshot (or a call counter), and assert that the hook is never invoked when the mismatch is returned.
Suggestion:
| MockBackendFactory::new(), | |
| persister.clone(), | |
| MockBackendFactory::with_behavior(Arc::clone(&behavior)), | |
| persister.clone(), |
| # the DEFAULT_BOOT_ARGS constant in src/sandbox/firecracker/config.rs. | ||
| # Keep the DAMON reclaim parameters in sync between both locations. | ||
| boot_args = "console=ttyS0 reboot=k panic=1 pci=off init=/init damon_reclaim.enabled=Y damon_reclaim.min_age=60000000 damon_reclaim.quota_ms=100 damon_reclaim.quota_sz=1073741824 damon_reclaim.quota_reset_interval_ms=1000 damon_reclaim.wmarks_high=900 damon_reclaim.wmarks_mid=700 damon_reclaim.wmarks_low=200 damon_reclaim.skip_anon=Y damon_reclaim.wmarks_interval=5000000" | ||
| boot_args = "console=ttyS0 reboot=k panic=1 pci=off mitigations=off init=/init damon_reclaim.enabled=Y damon_reclaim.min_age=60000000 damon_reclaim.quota_ms=100 damon_reclaim.quota_sz=1073741824 damon_reclaim.quota_reset_interval_ms=1000 damon_reclaim.wmarks_high=900 damon_reclaim.wmarks_mid=700 damon_reclaim.wmarks_low=200 damon_reclaim.skip_anon=Y damon_reclaim.wmarks_interval=5000000" |
There was a problem hiding this comment.
[security · medium]
Adding mitigations=off to the default guest command line disables kernel mitigations for CPU vulnerabilities (such as Spectre-class attacks) for every deployment, including the default KVM mode. This weakens isolation between processes running in a guest and is especially risky when guest workloads are untrusted. Keep mitigations enabled by default; if this is required for a specific backend or trusted environment, apply it conditionally or expose it as an explicit opt-in override rather than shipping it in the global default.
Suggestion:
| boot_args = "console=ttyS0 reboot=k panic=1 pci=off mitigations=off init=/init damon_reclaim.enabled=Y damon_reclaim.min_age=60000000 damon_reclaim.quota_ms=100 damon_reclaim.quota_sz=1073741824 damon_reclaim.quota_reset_interval_ms=1000 damon_reclaim.wmarks_high=900 damon_reclaim.wmarks_mid=700 damon_reclaim.wmarks_low=200 damon_reclaim.skip_anon=Y damon_reclaim.wmarks_interval=5000000" | |
| boot_args = "console=ttyS0 reboot=k panic=1 pci=off init=/init damon_reclaim.enabled=Y damon_reclaim.min_age=60000000 damon_reclaim.quota_ms=100 damon_reclaim.quota_sz=1073741824 damon_reclaim.quota_reset_interval_ms=1000 damon_reclaim.wmarks_high=900 damon_reclaim.wmarks_mid=700 damon_reclaim.wmarks_low=200 damon_reclaim.skip_anon=Y damon_reclaim.wmarks_interval=5000000" |
|
|
||
| [kernel.pvm] | ||
| version = "6.12.33-pvm" | ||
| url = "https://github.com/kvcache-ai/linux/releases/download/pvm-kernel-6.12.33/vmlinux-guest-6.12.33-pvm" |
There was a problem hiding this comment.
[bug · medium]
This PVM URL ignores {version}, unlike the manifest contract used by ensure_kernel. If kernel.version is overridden while kernel.url is left unset, setup stores this fixed 6.12.33 image under the overridden version's directory, so configuration and snapshot/dependency identity claim a different kernel than is actually installed. Make the URL derivable from version (adjusting the version/tag naming if necessary), or require a URL whenever a custom version is configured.
|
|
||
| [firecracker] | ||
| boot_args = "console=ttyS0 reboot=k panic=1 pci=off init=/init" | ||
| boot_args = "console=ttyS0 reboot=k panic=1 pci=off mitigations=off init=/init" |
There was a problem hiding this comment.
[security · medium]
mitigations=off disables the guest kernel's speculative-execution CPU vulnerability mitigations (for example, Spectre/Meltdown mitigations). This configuration can also be used for OSS validation outside a strictly isolated benchmark, so making this unconditional weakens guest security and may expose kernel or cross-process data to untrusted workloads. Please keep mitigations enabled by default and make this an explicit opt-in performance setting for trusted test environments, with the security trade-off documented.
| # Builds the server, starts it, creates a base template, then runs each suite. | ||
| # Requires: curl, jq, and a Linux host with /dev/kvm. | ||
| # Requires: curl, jq, and a Linux host whose /dev/kvm and modules match | ||
| # AENV_VIRTUALIZATION_MODE (kvm by default). |
There was a problem hiding this comment.
[documentation · low]
This newly referenced setting is missing from the environment-variable list directly below, so users are not told the supported values. Add an entry documenting that AENV_VIRTUALIZATION_MODE accepts kvm or pvm and defaults to kvm.
Suggestion:
| # AENV_VIRTUALIZATION_MODE (kvm by default). | |
| # AENV_VIRTUALIZATION_MODE - Virtualization backend: kvm or pvm (default: kvm). |
| VirtualizationModeMismatch { | ||
| resource: String, | ||
| resource_mode: VirtualizationMode, | ||
| node_mode: VirtualizationMode, | ||
| }, |
There was a problem hiding this comment.
[bug · medium]
This new expected compatibility error is not handled by From<OrchestratorError> for models::Error; it falls through to ApiImpl::internal_error and is returned as HTTP 500. For example, creating a sandbox from a snapshot produced in another virtualization mode is a client-visible incompatibility, not an internal server failure. Add an explicit API conversion (for example, 409 Conflict, or 400 for the create request) and an integration test for the response status.
| retained_artifacts.insert(sandbox_id); | ||
| sandboxes.push(record.into_metadata_without_runtime_state()); | ||
| continue; |
There was a problem hiding this comment.
[bug · high]
Stripping paused_state here preserves only the snapshot directory, not the sandbox's image-cache dependencies. During orchestrator startup, restored_paused includes only metadata with paused_state: Some(_), and reconcile_paused_at_startup passes those IDs to reconcile_paused; consequently, the durable cache protection for every foreign-mode record is treated as orphaned and removed. Image maintenance can then collect its rootfs/drive artifacts, so switching the node back to the record's mode may no longer allow this supposedly retained sandbox to resume. Preserve foreign sandbox IDs during protection reconciliation (and ideally persist enough mode-neutral RuntimeArtifactSet data to recreate missing protection) while still withholding resumable backend state.
| MockBackendFactory::new(), | ||
| persister.clone(), |
There was a problem hiding this comment.
[test · medium]
The test claims resume is rejected “without mutation,” but it drops the backend factory and only verifies metadata/persister activity. Consequently, it cannot detect backend construction or lifecycle calls if the compatibility check is later moved after launch begins. Also, paused_state: None prevents the resume path from reaching backend restoration independently of the mode check. Use a valid mock paused state and retain a MockBehavior with a BuildFromSnapshot hook/counter (or a failing action) so the test explicitly proves that no backend operation occurs.
Suggestion:
| MockBackendFactory::new(), | |
| persister.clone(), | |
| MockBackendFactory::with_behavior(Arc::clone(&behavior)), | |
| persister.clone(), |
| #[derive(Clone, Debug)] | ||
| pub struct SnapshotPublishMetadata { |
There was a problem hiding this comment.
[maintainability · medium]
Removing Serialize and Deserialize is an unnecessary breaking change to this exported type. VirtualizationMode already implements both traits, so the new field does not require dropping Serde support. Downstream users that serialize publication requests or deserialize stored/test fixtures will stop compiling; retain the existing derives unless this API break is explicitly intended.
Suggestion:
| #[derive(Clone, Debug)] | |
| pub struct SnapshotPublishMetadata { | |
| #[derive(Clone, Debug, Serialize, Deserialize)] | |
| pub struct SnapshotPublishMetadata { |
| #[serde(rename_all = "lowercase")] | ||
| pub enum VirtualizationMode { |
There was a problem hiding this comment.
[bug · medium]
Serde and FromStr accept different spellings for the same public type. FromStr trims whitespace and ignores case, but direct Serde consumers such as the new AppConfig.virtualization_mode TOML field accept only exact "kvm"/"pvm"; consequently AENV_VIRTUALIZATION_MODE=KVM (if routed through FromStr) and virtualization_mode = "KVM" can behave differently. Use a custom Deserialize implementation that delegates to FromStr (or make FromStr equally strict), and add tests covering both parsing paths.
Suggestion:
| #[serde(rename_all = "lowercase")] | |
| pub enum VirtualizationMode { | |
| pub enum VirtualizationMode { |
| if git diff --name-only "$before" "${{ github.sha }}" | grep -qE '^(docs/|src/api/openapi\.yml$|\.github/workflows/docs\.yml$)'; then | ||
| echo "should_build=true" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
[bug · medium]
With set -o pipefail, grep -q may exit after its first match and close the pipe while git diff is still writing. On a sufficiently large push, git diff receives SIGPIPE, the pipeline becomes nonzero, and a relevant docs build is incorrectly skipped. Avoid the early-closing pipeline, e.g. capture the name list before matching or use Git pathspecs and test whether git diff --quiet "$before" "${{ github.sha }}" -- docs src/api/openapi.yml .github/workflows/docs.yml reports changes.
| id: sandbox_id, | ||
| state: SandboxState::Paused, | ||
| virtualization_mode: sandbox_mode, | ||
| paused_state: None, |
There was a problem hiding this comment.
[test · medium]
This makes the non-mutation assertion below vacuous: a resume path that clears an existing paused-state payload before returning the mode mismatch would still pass. Initialize this fixture with Some(test_paused_state().clone()), retain the original Arc, and assert that the payload is still present (ideally with Arc::ptr_eq) after the failed resume so the test covers preservation of the data required for a later valid resume.
Suggestion:
| paused_state: None, | |
| paused_state: Some(test_paused_state().clone()), |
| return Ok(()); | ||
| } | ||
|
|
||
| let mode_manifest = manifest.firecracker.for_mode(config.virtualization_mode); |
There was a problem hiding this comment.
[bug · medium]
The selected mode affects the download URL, but not the cache identity, and the mode-agnostic fc_path is checked before this selection. If KVM and PVM are configured with the same explicit firecracker.version (but different URLs/artifacts), switching modes will treat the previously downloaded binary as valid and skip downloading the required one. Include virtualization_mode in the managed dependency path/cache key (and apply the same fix to the kernel path), or persist and validate artifact metadata before accepting the cache hit.
|
@yingdi-shan 47942e2 introduced a versioned documentation site with three coexisting paths under GitHub Pages, published to the gh-pages branch instead of the previous "Actions artifact" deploy source:
|
1 participant
What
Add an opt-in PVM virtualization mode alongside the existing KVM deployment path.
virtualization_mode = "kvm" | "pvm"withAENV_VIRTUALIZATION_MODEoverride./dev/kvmprerequisites at startup.Why
Cloud VMs often do not expose nested virtualization, which prevents the standard KVM deployment from running. PVM provides an alternative deployment path for those environments while keeping standard KVM as the default and recommended mode.
Related issue
Closes #7
Scope and non-goals
Included:
Not included:
Design and behavior changes
VirtualizationModeis a shared domain type used by configuration, setup, snapshots, and persistence.firecracker.{kvm,pvm}andkernel.{kvm,pvm}entries; only the active variant is provisioned.kvm_pvmmodule. PVM startup requires x86_64,kvm_pvm, and read/write access to/dev/kvm.--setup-onlyremains host-module independent so release artifacts can be assembled on ordinary CI runners.aenv-server-linux-x86_64.tar.gzfor KVMaenv-server-linux-x86_64-pvm.tar.gzfor PVMlatest/ version tags for KVM imageslatest-pvm/ version-pvmtags for PVM imagesCompatibility and operations
virtualization_mode; default remainskvm.virtualization_modeto committed snapshot metadata and paused-sandbox records. Legacy records default to KVM.CONFIG_KVM_PVM, plus an ABI-compatible guest kernel withCONFIG_PVM_GUEST. AgentENV does not install the host kernel automatically.Validation
make fmtmake clippymake test-unitmake -C services test(required whenservices/changes)maketargetCommands and results:
All completed successfully. Release, Compose, and Kubernetes YAML files were also parsed successfully, and local Markdown links were checked.
Skipped checks and reasons:
make -C services testwas not required because no Go service code changed.Risks and reviewer notes
kvcache-ai/linux.src/cfg.rsandsrc/setup/, compatibility checks in snapshot/persistence code, release packaging, and the deployment guide.Checklist