Repository navigation
feat: integrate nested KVM and support legacy v9 snapshot recovery - #358
Hygge-Gezelligheid wants to merge 4 commits into
Conversation
|
🔍 OpenCodeReview found 8 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| template = api( | ||
| "POST", | ||
| "/v3/templates", | ||
| {"name": f"nestedvirt-smoke-{time.time_ns()}", "cpuCount": 2, "memoryMB": 512}, | ||
| ) | ||
| tid, bid = template["templateID"], template["buildID"] |
There was a problem hiding this comment.
The smoke run creates a uniquely named template and its build, but there is no corresponding template deletion. This leaves every successful run—and any run that fails after creation—with persistent templates and build artifacts, so repeated hardware smoke tests accumulate service state. Wrap the template/build and sandbox exercise in an outer try/finally and delete /templates/{tid} in that cleanup (tolerating cleanup failures as appropriate).
There was a problem hiding this comment.
Fixed in 8bdb77e. An outer cleanup scope owns the template, and each iteration owns all returned sandboxes. Every delete is attempted; cleanup errors are attached to the original failure, or fail an otherwise successful run. Independent real-API validation deleted both fork children to force two cleanup 404s, confirmed that the source and template were still removed, and verified nonzero exit with no successful results file. The full nine-case API matrix also passed.
| root = work / "oci-root" | ||
| shutil.copytree(work / "probe-root", root, dirs_exist_ok=True) |
There was a problem hiding this comment.
dirs_exist_ok=True merges the new fixture into any existing oci-root; it does not remove files that disappeared from probe-root. If the same work directory is reused, the tarball can therefore contain stale files from an earlier run, making the image non-reproducible and potentially changing the fixture's behavior. Remove/recreate root (or otherwise guarantee and enforce a clean work directory) before copying.
There was a problem hiding this comment.
Fixed in 8bdb77e. The dedicated oci-root is removed before copying the current probe-root. A real HTTP fixture regression builds twice in the same work directory and verifies that a removed source file disappears, changed content is updated, and the expected init is included. This regression passed independently and is now included in the integration workflow.
Hygge-Gezelligheid
changed the title
| nestedvirt-tools: | ||
| runs-on: ubuntu-latest | ||
| steps: |
There was a problem hiding this comment.
This job has no timeout-minutes, so a hung fixture subprocess or server can consume the runner until GitHub's much longer default timeout. Please set an explicit bounded timeout appropriate for these syntax/fixture checks.
Suggestion:
| nestedvirt-tools: | |
| runs-on: ubuntu-latest | |
| steps: | |
| nestedvirt-tools: | |
| runs-on: ubuntu-latest | |
| timeout-minutes: 10 | |
| steps: |
There was a problem hiding this comment.
Added timeout-minutes: 10 to nestedvirt-tools in a436054. I checked the actual prior job: it succeeded in 7 seconds, and no hang was observed. The 10-minute limit is a bounded resource-protection measure with ample allowance for checkout and Python setup, replacing the 360-minute default for this lightweight job only. The existing per-request/readiness/process timeouts remain unchanged.
| assert response.status == 200 | ||
| manifest = json.loads(response.read()) | ||
| break |
There was a problem hiding this comment.
These runtime checks use assert, so running this verifier with python -O removes them and can report PASS without validating the HTTP response or rebuilt archive. Use explicit conditional checks that raise an exception (and likewise replace the archive-content assertions below) so the fixture test cannot be optimized into a no-op.
There was a problem hiding this comment.
Confirmed with a real regression reproduction, then fixed in a436054. An isolated fixture server using the historical merge-copy behavior produced an archive containing a deleted file: the old verifier exited 1 normally but incorrectly printed PASS and exited 0 under -O. All five checks now use explicit conditions and errors. Independent real HTTP/tar tests passed for valid archives and rejected stale archives under normal Python, -O and PYTHONOPTIMIZE=1; CI now also exercises -O. The previous workflow used normal Python, so its existing PASS is not invalidated by this issue.
| - name: Verify real fixture rebuild | ||
| run: | | ||
| python3 scripts/tests/test-nestedvirt-fixture.py "$RUNNER_TEMP" | ||
| python3 -O scripts/tests/test-nestedvirt-fixture.py "$RUNNER_TEMP" |
There was a problem hiding this comment.
This job only compiles nestedvirt-api-smoke.py; it never runs the smoke test or provisions the nested-KVM artifacts/configuration it reads. Consequently, regressions in the API flow, artifact paths, boot arguments, fork/pause-resume behavior, and nested KVM execution can pass CI despite this being the only new validation job. Add a runner/harness invocation for the smoke test (or explicitly gate and document the required hardware job) rather than treating syntax plus fixture rebuild as coverage of the API test.
There was a problem hiding this comment.
The hosted job is intentionally limited to syntax and the real fixture regression; it is not evidence that nested KVM or the normal API lifecycle passed. The deployment guide already requires dedicated-host raw/API validation with matching assets and retained source/binary identities: docs/src/configuration/nested-kvm.md, “Validate snapshots and the normal API”.
That hardware acceptance was completed separately for this delivery: Firecracker/helper were rebuilt from runtime source matching the requested c7e1cce branch, Linux 6.18 was built from the checksum-verified official archive with built-in KVM, and the new assets passed nine normal API cases. Independent review verified source/config/build/output identities, observed the running Firecracker executable and kernel path, and repeated all three API phases with fork, pause/connect, increasing inner counters and cleanup. The PR description records artifact hashes and coverage boundaries.
I am retaining that explicit dedicated-host requirement and evidence, rather than adding an unprovisioned nested-KVM invocation to an ordinary hosted runner. No claim is made that the syntax/fixture job exercises the hardware API smoke.
| self.assertTrue( | ||
| all(build[0].poll() is None for _, build in builds), | ||
| "workers finished before heartbeat overlap was observed", | ||
| ) |
There was a problem hiding this comment.
This assertion makes the polling race-sensitive: node_counts() can observe two active heartbeats after one of the subprocesses has already exited (the heartbeat is asynchronous and can lag process completion). In that valid case the assertion aborts the test instead of accepting the overlap, producing intermittent Compose failures. Treat the heartbeat observation as sufficient evidence of overlap, or only use the process state as a diagnostic after the wait succeeds.
There was a problem hiding this comment.
Confirmed with real BuildKit processes and heartbeat data, then fixed in 6753de6. After the original overlap check and a real heartbeat showing one worker on each node, a controlled 40-second pause of the checking process allowed both CLI processes to finish successfully. The old post-response assertion then failed. This demonstrates the scheduling boundary; it does not establish the frequency of natural CI failures.
The check now samples CLI liveness before requesting the heartbeat and retains the original RUN-output/overlap proof, worker totals, explicit two-node requirement, output, cache-seed and cleanup checks. Exit codes after the response are diagnostic. Independent validation passed the same delayed case with both CLIs exiting 0, rejected a real sequential-build counterexample, and passed both unmodified repository concurrency tests in 118.121 seconds.
| with socket.socket() as reservation: | ||
| reservation.bind(("127.0.0.1", 0)) | ||
| port = reservation.getsockname()[1] | ||
| with (work / f"server-{iteration}.log").open("w") as log: |
There was a problem hiding this comment.
The port is only reserved while this socket exists, then the socket is closed before the child server binds. Another process can claim the port in that interval, causing ThreadingHTTPServer to exit and making this test intermittently fail. Keep the reservation socket open and pass/inherit it to the server, or have the server bind to port 0 and communicate its selected port back to the parent.
Suggestion:
| with socket.socket() as reservation: | |
| reservation.bind(("127.0.0.1", 0)) | |
| port = reservation.getsockname()[1] | |
| with (work / f"server-{iteration}.log").open("w") as log: | |
| with (work / f"server-{iteration}.log").open("w") as log: |
There was a problem hiding this comment.
Confirmed with a real competing process: it claimed the released reservation port before the fixture bound, and the original server exited with EADDRINUSE. Fixed in 6753de6 by letting the fixture bind port 0 itself and atomically publish its actual port only after binding. The verifier waits for that file in its unique test directory; readiness/exit deadlines and all content checks remain intact. Fixed-port callers keep their existing behavior.
Independent validation held 64 real ports while starting four fixture servers concurrently: all used distinct ports, served valid HTTP manifests and rejected a competing bind to the advertised port. Real valid/stale archive cases also remained correctly accepted/rejected under normal Python, -O and PYTHONOPTIMIZE=1.
Hygge-Gezelligheid
changed the title
Hygge-Gezelligheid
changed the title
2c714b4 to
d224474
Compare
2dc6b48 to
22a6912
Compare
| enum LiveOverlaybdSnapshotState { | ||
| ReadOnly, | ||
| Restacked(PathBuf), | ||
| Restacked { live: PathBuf, staged: PathBuf }, |
There was a problem hiding this comment.
What is the relationship between nested virt with overlaybd snapshot? Why change this source file?
| pub slot: Slot, | ||
| pub fc_instance: FirecrackerInstance, | ||
| pub work_dir: TempDir, | ||
| binary_identity: (u64, u64), |
There was a problem hiding this comment.
Why is this needed? After upgrading the agentenv server with this PR, shouldn’t all FC instances in the pool be running the same version?
| /// Legacy executable digest retained for reading existing metadata. New captures omit it. | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub firecracker_binary_sha256: Option<String>, | ||
| /// Actual snapshot wire format, independent of the software version string. | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub firecracker_snapshot_version: Option<String>, |
There was a problem hiding this comment.
I’m not sure what these two newly added fields are for.
22a6912 to
7c8b1e3
Compare
Hygge-Gezelligheid
changed the title
| // x86_64 KVM restores use the current runtime's legacy snapshot loader. | ||
| // Captured guest assets remain unchanged. | ||
| snapshot.common.firecracker_binary = select_resume_binary( | ||
| ConfigManager::global_config(), | ||
| &snapshot.common.firecracker_binary, | ||
| ); |
There was a problem hiding this comment.
A Firecracker VM snapshot is tied to the VMM build/ABI that produced its vm_state.bin, but this unconditionally substitutes the configured current executable without recording or checking the captured executable's version/compatibility. If the configured binary is missing, changed, or incompatible with the snapshot state, the restore will only fail later during spawn/load (and the original usable path is discarded). Please validate the selected binary and its snapshot compatibility, or retain enough snapshot metadata to make this selection safe.
| #[cfg(target_arch = "x86_64")] | ||
| let config = ConfigManager::global_config(); | ||
| #[cfg(target_arch = "x86_64")] | ||
| if config.virtualization_mode == crate::virtualization::VirtualizationMode::Kvm { | ||
| assert_eq!( | ||
| restored.firecracker_binary, | ||
| config.resolved_firecracker_binary_path() | ||
| ); | ||
| } else { |
There was a problem hiding this comment.
This production-path assertion is conditional on the process-global virtualization mode, so it does not require the KVM branch to run; on PVM-configured or non-x86_64 CI the key behavior is only covered by the helper test, not by from_snapshot_config. Configure an isolated KVM config for this test (or test the selection through an injected config) so the restore-path regression is deterministic.
| // These processes share the configured host network rules. | ||
| static ORCHESTRATOR_TEST_LOCK: tokio::sync::Mutex<()> = tokio::sync::Mutex::const_new(()); |
There was a problem hiding this comment.
This mutex only coordinates tests in the orchestrator_integration process. make test-agent-integration invokes both this binary and the integration binary in the same Cargo command, and Cargo can run test binaries concurrently; the latter includes fc tests that install/update host network policy and start Firecracker VMs. The subprocesses spawned below also do not share this mutex, so the comment's process-wide protection is not provided and these tests can still interfere with one another. Use an inter-process/external lock (or serialize the binaries) around the shared host resources.
| std::fs::write( | ||
| root.join("restart-witness.json"), | ||
| serde_json::to_vec(&(sandbox_id, access_token.expose()))?, | ||
| )?; |
There was a problem hiding this comment.
This writes the sandbox's bearer access token to a JSON file on disk. Although the directory is temporary, it can remain after a timeout/panic or be retained in test artifacts, unnecessarily exposing a credential. Keep the witness limited to the sandbox ID and transfer/validate the token through a protected process-to-process channel (or avoid persisting it if the resumed state can provide the needed assertion).
7c8b1e3 to
40867d8
Compare
| done < <(compgen -e) | ||
| preserve_env="$(IFS=,; printf '%s' "${env_names[*]}")" | ||
| # sudo's secure_path can override PATH even with --preserve-env. | ||
| exec sudo --preserve-env="$preserve_env" /usr/bin/env "PATH=$PATH" "$0" "$@" |
There was a problem hiding this comment.
This reintroduces the caller-controlled PATH before executing a script whose shebang is #!/usr/bin/env bash. A non-root caller can put a malicious bash earlier in PATH; after sudo, /usr/bin/env will resolve the shebang through that path and execute the malicious binary as root. Invoke a fixed absolute Bash (or use a trusted fixed PATH) for the privileged re-entry instead of executing $0 through its env shebang.
Suggestion:
| exec sudo --preserve-env="$preserve_env" /usr/bin/env "PATH=$PATH" "$0" "$@" | |
| exec sudo --preserve-env="$preserve_env" /usr/bin/env "PATH=$PATH" /usr/bin/bash "$0" "$@" |
| connection = http.client.HTTPConnection( | ||
| endpoint.hostname, endpoint.port, timeout=180 | ||
| ) |
There was a problem hiding this comment.
urlsplit retains any path prefix in endpoint.path, but this request sends only the bare path argument and never joins that prefix. As a result, deployments exposed below a gateway prefix (for example, http://host/agentenv) will receive /v3/templates at the wrong route even though the supplied endpoint is reachable. Preserve and prepend the endpoint base path (and normalize it) for every direct API request, or explicitly reject non-root endpoint paths.
| elif self.path.startswith("/v2/nested/manifests/"): | ||
| body, kind = manifest, manifest_type |
There was a problem hiding this comment.
This accepts every manifest reference under nested, including misspelled repositories and tags/digests that the smoke test did not intend to use. A bad fromImage URL or reference can therefore still pull the same fixture and make the test pass. Validate the requested repository/reference against the fixture image (or configure the expected reference and return 404 for anything else) so the test also verifies image-reference construction.
| finally: | ||
| process.terminate() | ||
| process.wait(timeout=10) |
There was a problem hiding this comment.
If the server does not exit within the timeout, wait raises and leaves the child running; because this is in finally, it can also mask the original fixture validation failure. Add a kill-and-wait fallback (and preserve the original exception) so a hung server cannot leak into subsequent CI work or obscure the actual failure.
| &db.get(&key)? | ||
| .ok_or_else(|| anyhow::anyhow!("missing paused record"))?, | ||
| )?; | ||
| assert_snapshot_format(¤t_binary, &record["state"], "v9.0.0").await?; |
There was a problem hiding this comment.
This test hard-codes the snapshot-format strings for both installed Firecracker releases and reaches into the private persisted JSON/RocksDB record to rewrite common.firecracker_binary. Any change to the snapshot metadata schema or the CLI's version output will make the test fail before exercising resume behavior. Prefer deriving the expected versions from the configured binaries (or a supported typed persistence helper) and isolating the binary override behind a test-only API.
Suggestion:
| assert_snapshot_format(¤t_binary, &record["state"], "v9.0.0").await?; | |
| let old_version = old.config().firecracker.version.as_deref().unwrap_or_default(); | |
| assert_snapshot_format(¤t_binary, &record["state"], old_version).await?; |
52721ee to
bc46e7b
Compare
| connection.request( | ||
| method, | ||
| path, | ||
| None if body is None else json.dumps(body), | ||
| {"Content-Type": "application/json", "X-API-Key": key}, | ||
| ) | ||
| response = connection.getresponse() | ||
| data = response.read() | ||
| connection.close() |
There was a problem hiding this comment.
HTTPConnection.close() is reached only after request(), getresponse(), and read() all succeed. If any of those operations raises (for example, a timeout or dropped connection), this socket is never closed; the smoke test performs many API calls across rounds, so repeated failures can leak connections and mask the original error. Wrap the connection in a with block or close it from a finally block.
| // x86_64 KVM restores use the current runtime's legacy snapshot loader. | ||
| // Captured guest assets remain unchanged. | ||
| snapshot.common.firecracker_binary = select_resume_binary( | ||
| ConfigManager::global_config(), | ||
| &snapshot.common.firecracker_binary, | ||
| ); |
There was a problem hiding this comment.
This only rewrites the binary for from_snapshot_config_with_override, which is the paused-state/fork path. Published snapshot launches go through FirecrackerSandbox::from_snapshot and construct LaunchMode::Resume(snapshot_config) directly, so they still use the binary captured in FirecrackerSnapshotConfig (and can fail when that old KVM Firecracker is unavailable or incompatible). Apply the same selection when building the resume config in from_snapshot (or centralize it in build/the resume launch path) so all x86_64 KVM snapshot restores use the current runtime.
|
|
||
| const TEST_TIMEOUT: Duration = Duration::from_secs(120); | ||
| // These processes share the configured host network rules. | ||
| static ORCHESTRATOR_TEST_LOCK: tokio::sync::Mutex<()> = tokio::sync::Mutex::const_new(()); |
There was a problem hiding this comment.
This mutex only synchronizes tests within this one test process; it is not shared with the separately spawned child processes or with other integration-test binaries (for example fc). Those tests can still run concurrently and manipulate the same host networking/Firecracker resources, so the new lifecycle can remain flaky despite the comment claiming the processes are serialized. Use a cross-process lock (or configure the test runner/CI to serialize these resource-sensitive targets) if isolation is required.
Suggestion:
| static ORCHESTRATOR_TEST_LOCK: tokio::sync::Mutex<()> = tokio::sync::Mutex::const_new(()); | |
| // Use a cross-process lock for resource-sensitive integration tests. |
d60ce69 to
261d7f4
Compare
|
|
||
| const TEST_TIMEOUT: Duration = Duration::from_secs(120); | ||
| // These processes share the configured host network rules. | ||
| static ORCHESTRATOR_TEST_LOCK: tokio::sync::Mutex<()> = tokio::sync::Mutex::const_new(()); |
There was a problem hiding this comment.
This mutex only coordinates tests running in this one orchestrator_integration process. The lifecycle phases are executed in child processes, and Cargo can run the separate integration binary (whose Firecracker tests also change host networking/KVM state) concurrently; those processes have independent copies of this mutex. Consequently the comment's host-wide serialization guarantee does not hold and these tests can race with the other integration suite. Use an inter-process lock (for example, a lock file held across the parent and its children) or serialize the test binaries in the test runner.
Use the nestedvirt Firecracker release and its matching helper on x86_64 and aarch64. Consolidate image preparation and nested KVM lifecycle validation in AgentENV, with setup documentation and lightweight fixture checks. Signed-off-by: Yicheng Lin <linyc25@mails.tsinghua.edu.cn>
Restore snapshots with the node's configured Firecracker while retaining captured guest assets. Exercise persisted sandbox recovery across separate test processes and verify old v9 recovery followed by v1000 output. Signed-off-by: Yicheng Lin <linyc25@mails.tsinghua.edu.cn>
261d7f4 to
f782f90
Compare
| - name: Verify real fixture rebuild | ||
| run: | | ||
| python3 scripts/tests/nestedvirt-smoke.py check-fixture | ||
| python3 -O scripts/tests/nestedvirt-smoke.py check-fixture |
There was a problem hiding this comment.
This new CI job only compiles the scripts and runs check-fixture; there is no workflow step invoking the run subcommand (and no other caller exists in the changed workflow tree). Consequently the nested-KVM API matrix—KVM availability, QEMU startup, fork, pause/resume, and snapshot restore—is never exercised in CI. Please add a hardware-capable job that prepares the kernel/config/API key/node and runs the end-to-end command, or avoid presenting this as nested-KVM integration coverage.
Suggestion:
| - name: Verify real fixture rebuild | |
| run: | | |
| python3 scripts/tests/nestedvirt-smoke.py check-fixture | |
| python3 -O scripts/tests/nestedvirt-smoke.py check-fixture | |
| - name: Run nested KVM smoke matrix | |
| run: | | |
| python3 scripts/tests/nestedvirt-smoke.py run ... |
| source = api("POST", "/v2/sandboxes", {"templateID": tid, "timeout": 600}) | ||
| sandboxes.append(source) | ||
| sandbox_paths.append(f"/sandboxes/{source['sandboxID']}") |
There was a problem hiding this comment.
The cleanup list is populated only after the API call returns and the response is fully parsed. If sandbox creation succeeds but the connection drops/JSON decoding raises, source is never registered and the sandbox is leaked; the same applies to the template and fork children before sandbox_paths.extend. This is especially problematic in a multi-round smoke test because leaked VMs can exhaust the node and make later rounds hang. Register resources using an idempotent creation/cleanup strategy (or query/clean up the created resource on failure) as soon as the server has accepted the request.
Suggestion:
| source = api("POST", "/v2/sandboxes", {"templateID": tid, "timeout": 600}) | |
| sandboxes.append(source) | |
| sandbox_paths.append(f"/sandboxes/{source['sandboxID']}") | |
| source = api("POST", "/v2/sandboxes", {"templateID": tid, "timeout": 600}) | |
| sandboxes.append(source) | |
| sandbox_paths.append(f"/sandboxes/{source['sandboxID']}") |
| let paused_store = &root.join("paused-sandboxes"); | ||
| let persister = host_file_persister(paused_store.clone()); |
There was a problem hiding this comment.
paused_store is declared as &PathBuf, so calling .clone() clones the reference (&PathBuf) rather than producing a PathBuf; host_file_persister requires ownership of a PathBuf. This makes the integration test fail to compile. Keep the PathBuf by removing the borrow (e.g. let paused_store = root.join(...)) or pass paused_store.to_path_buf().
f782f90 to
2d0f138
Compare
| - name: Verify real fixture rebuild | ||
| run: | | ||
| python3 scripts/tests/nestedvirt-smoke.py check-fixture | ||
| python3 -O scripts/tests/nestedvirt-smoke.py check-fixture |
There was a problem hiding this comment.
This job only exercises check-fixture; there is no invocation of the run subcommand anywhere in the workflow, so the newly added template build, nested-KVM boot, fork, pause/resume, and Firecracker paths are not covered by CI. A syntax/fixture-only pass can therefore leave the main smoke test broken. Please either add the required KVM/kernel/API setup and run the full command here, or wire it into a separate CI job.
| [kernel.kvm_x86_64] | ||
| version = "vmlinux-6.18-agentenv-nestedvirt" | ||
| url = "https://github.com/kvcache-ai/firecracker/releases/download/v1.15.1-nestedvirt-v1/{version}-{arch}" |
There was a problem hiding this comment.
On x86_64 this new manifest entry makes for_mode(Kvm) select the architecture-specific version, but the existing managed_dependency_paths_select_the_active_mode_versions test still asserts manifest.firecracker.kvm.version for the KVM Firecracker path. That assertion will fail on the supported x86_64 target; update the test to use for_mode(VirtualizationMode::Kvm) (and add coverage for the x86_64 override) before merging.
| def copy_binary(path, root): | ||
| """Copy a host executable and its shared libraries into the fixture.""" | ||
| result = subprocess.run(["ldd", path], text=True, capture_output=True, check=True) |
There was a problem hiding this comment.
The full run path assumes that ldd, /bin/busybox, cpio, /usr/bin/qemu-system-x86_64, and the selected kernel exist and are usable, but does not validate these prerequisites before starting the registry/API workflow. On a clean or differently provisioned runner this fails as an opaque FileNotFoundError or creates an unusable fixture that fails much later during template boot. Please perform explicit prerequisite/architecture checks (or install the required host packages) before building the fixture.
| async fn orchestrator_v9_snapshot_upgrade() -> Result<()> { | ||
| let _guard = ORCHESTRATOR_TEST_LOCK.lock().await; |
There was a problem hiding this comment.
The guard is held while this test launches two child test processes and waits for the complete pause/resume VM lifecycle. The other orchestrator tests acquire the same process-global lock, so a slow or timed-out child serializes unrelated tests for up to the full timeout. Consider scoping the lock only around the host-network-sensitive lifecycle sections, or using test-level serialization without holding it across the parent’s child-process waits.
| async fn orchestrator_lifecycle() -> Result<()> { | ||
| let _guard = ORCHESTRATOR_TEST_LOCK.lock().await; |
There was a problem hiding this comment.
This lock is also held by the parent while it waits for both subprocess phases (and by the capture test while it waits for its child). Since the lock is process-local, it does not coordinate the child processes, but it does block other tests in the parent for the entire external VM run. Narrowing the guard scope would avoid unnecessary suite-wide serialization while retaining synchronization for the actual shared host-network setup.
Select the tested prebuilt Linux 6.18 release asset for x86_64 KVM in both dependency downloads and kernel path resolution. Preserve the aarch64 KVM and PVM defaults and explicit configuration overrides, and document managed kernel downloads. Signed-off-by: Yicheng Lin <linyc25@mails.tsinghua.edu.cn>
2d0f138 to
10c2493
Compare
| - name: Verify real fixture rebuild | ||
| run: | | ||
| python3 scripts/tests/nestedvirt-smoke.py check-fixture | ||
| python3 -O scripts/tests/nestedvirt-smoke.py check-fixture |
There was a problem hiding this comment.
This job never invokes the run subcommand, so the newly added runtime path is not exercised in CI: fixture boot contents, QEMU/KVM startup, fork/restore, pause/resume, and cleanup can all regress while these checks remain green. Add a hardware-capable job/step with the required node endpoint, API key, inner kernel, and runtime prerequisites, or explicitly wire this smoke test into the existing integration job.
Suggestion:
| - name: Verify real fixture rebuild | |
| run: | | |
| python3 scripts/tests/nestedvirt-smoke.py check-fixture | |
| python3 -O scripts/tests/nestedvirt-smoke.py check-fixture | |
| - name: Run nested KVM smoke | |
| run: | | |
| python3 scripts/tests/nestedvirt-smoke.py run "$AENV_ENDPOINT" "$AENV_API_KEY_FILE" "$RUNNER_TEMP/nestedvirt" --inner-kernel "$INNER_KERNEL" --kernel-release "$KERNEL_RELEASE" |
| connection = http.client.HTTPConnection( | ||
| endpoint.hostname, endpoint.port, timeout=180 | ||
| ) |
There was a problem hiding this comment.
urlsplit() permits an HTTP URL without an explicit port, but endpoint.port is then None; HTTPConnection requires an integer port (or a host string with its default port) and raises before the API smoke starts. Either require an explicit port during argument validation or use endpoint.port or 80 (and similarly normalize the endpoint passed to the SDK).
Suggestion:
| connection = http.client.HTTPConnection( | |
| endpoint.hostname, endpoint.port, timeout=180 | |
| ) | |
| connection = http.client.HTTPConnection( | |
| endpoint.hostname, endpoint.port or 80, timeout=180 | |
| ) |
| let status = timeout(TEST_TIMEOUT, child.status()) | ||
| .await | ||
| .map_err(|_| anyhow::anyhow!("{phase} test process timed out"))??; |
There was a problem hiding this comment.
The parent applies the same 120-second timeout to child.status() that the child applies to its entire lifecycle body. That outer budget also includes spawning the test binary, runtime initialization, and child shutdown, so a child that legitimately uses nearly TEST_TIMEOUT is killed by the parent before it can finish (and may leave the persisted pause state incomplete). Give the parent a larger budget or use a separate timeout with startup/cleanup margin.
Suggestion:
| let status = timeout(TEST_TIMEOUT, child.status()) | |
| .await | |
| .map_err(|_| anyhow::anyhow!("{phase} test process timed out"))??; | |
| let status = timeout(TEST_TIMEOUT + Duration::from_secs(30), child.status()) | |
| .await | |
| .map_err(|_| anyhow::anyhow!("{phase} test process timed out"))??; |
| let paused_store = &root.join("paused-sandboxes"); | ||
| let persister = host_file_persister(paused_store.clone()); |
There was a problem hiding this comment.
This changes paused_store to &Path, so paused_store.clone() clones the reference rather than producing the PathBuf required by host_file_persister(PathBuf). The integration test will fail to compile with a type mismatch. Keep the PathBuf owned (remove &) or explicitly convert with to_path_buf().
Suggestion:
| let paused_store = &root.join("paused-sandboxes"); | |
| let persister = host_file_persister(paused_store.clone()); | |
| let paused_store = root.join("paused-sandboxes"); | |
| let persister = host_file_persister(paused_store.clone()); |
| let status = timeout(TEST_TIMEOUT, child.status()) | ||
| .await | ||
| .map_err(|_| anyhow::anyhow!("capture test process timed out"))??; |
There was a problem hiding this comment.
The parent timeout is identical to the timeout used inside the spawned capture-test process, but it also includes child startup and shutdown. A child completing near 120 seconds can therefore be terminated by the parent first, making this process-isolation wrapper flaky. Use a larger parent timeout or reserve explicit margin.
Suggestion:
| let status = timeout(TEST_TIMEOUT, child.status()) | |
| .await | |
| .map_err(|_| anyhow::anyhow!("capture test process timed out"))??; | |
| let status = timeout(TEST_TIMEOUT + Duration::from_secs(30), child.status()) | |
| .await | |
| .map_err(|_| anyhow::anyhow!("capture test process timed out"))??; |
| - name: Verify real fixture rebuild | ||
| run: | | ||
| python3 scripts/tests/nestedvirt-smoke.py check-fixture | ||
| python3 -O scripts/tests/nestedvirt-smoke.py check-fixture |
There was a problem hiding this comment.
This job only runs syntax checks and check-fixture; it never invokes the run command, so the API/template-build, nested-KVM, fork, and pause/resume paths added here are not exercised by CI. Please add this smoke to a KVM-capable job (or explicitly wire it into the existing E2E setup) so those regressions cannot merge unnoticed.
| with http.server.ThreadingHTTPServer(("127.0.0.1", port), Handler) as server: | ||
| thread = threading.Thread(target=server.serve_forever) | ||
| thread.start() | ||
| try: | ||
| yield f"127.0.0.1:{server.server_port}/nested:latest" |
There was a problem hiding this comment.
The image reference advertises a registry on loopback, but the template builder addressed by args.endpoint may run in a different VM, container, or network namespace. In that deployment the builder cannot reach this server and the build fails when pulling fromImage; bind/publish an address reachable from the builder (or enforce and validate that the builder shares the runner's network namespace).
| // These processes share the configured host network rules. | ||
| static ORCHESTRATOR_TEST_LOCK: tokio::sync::Mutex<()> = tokio::sync::Mutex::const_new(()); |
There was a problem hiding this comment.
This Tokio mutex only serializes tests within this one test-binary process. The lifecycle test then spawns separate test processes, and Cargo can also run other integration-test binaries concurrently, so those processes can still manipulate the shared host network/Firecracker resources concurrently. Use an inter-process lock (for example, a filesystem lock) around the spawned lifecycle and other resource-sharing tests, or configure the test runner to serialize the relevant binaries.
| .args([ | ||
| "--exact", | ||
| "orchestrator::orchestrator_lifecycle", | ||
| "--nocapture", | ||
| ]) |
There was a problem hiding this comment.
orchestrator.rs is the integration-test crate root, so the test harness name for this top-level function is orchestrator_lifecycle, not orchestrator::orchestrator_lifecycle. With --exact, each spawned child will run zero tests and exit successfully; the parent then fails because restart-witness.json/resume-complete was never produced. Use the actual harness name (and make the same correction in the other child invocations below).
Suggestion:
| .args([ | |
| "--exact", | |
| "orchestrator::orchestrator_lifecycle", | |
| "--nocapture", | |
| ]) | |
| .args([ | |
| "--exact", | |
| "orchestrator_lifecycle", | |
| "--nocapture", | |
| ]) |
| .args([ | ||
| "--exact", | ||
| "orchestrator::orchestrator_lifecycle", | ||
| "--nocapture", | ||
| ]) |
There was a problem hiding this comment.
Correction to the preceding review note: this file is included as mod orchestrator by tests/orchestrator_integration.rs, so the orchestrator::orchestrator_lifecycle exact filter is valid here. No change is needed for this invocation.
Suggestion:
| .args([ | |
| "--exact", | |
| "orchestrator::orchestrator_lifecycle", | |
| "--nocapture", | |
| ]) | |
| .args([ | |
| "--exact", | |
| "orchestrator::orchestrator_lifecycle", | |
| "--nocapture", | |
| ]) |
| &db.get(&key)? | ||
| .ok_or_else(|| anyhow::anyhow!("missing paused record"))?, | ||
| )?; | ||
| assert_snapshot_format(¤t_binary, &record["state"], "v9.0.0").await?; |
There was a problem hiding this comment.
This opt-in test hard-codes both the old and new Firecracker version strings and requires the first output line of --describe-snapshot to equal them exactly. A runtime rebuild or harmless CLI output/version-format change will fail the test before it tests restore compatibility. Derive the expected versions from the configured binaries (or use the version API/parser already used by the project) and validate the snapshot format without depending on an exact presentation line.
| .args([ | ||
| "--exact", | ||
| "orchestrator::orchestrator_capture_snapshot_can_be_published_and_relaunched", | ||
| "--nocapture", | ||
| ]) |
There was a problem hiding this comment.
This integration-test function is also defined at the crate root, so its exact harness name does not include the orchestrator:: module prefix. The child therefore executes zero tests and the parent fails the completion-file check instead of running the capture test. Use orchestrator_capture_snapshot_can_be_published_and_relaunched (and the corresponding lifecycle child name above).
Suggestion:
| .args([ | |
| "--exact", | |
| "orchestrator::orchestrator_capture_snapshot_can_be_published_and_relaunched", | |
| "--nocapture", | |
| ]) | |
| .args([ | |
| "--exact", | |
| "orchestrator_capture_snapshot_can_be_published_and_relaunched", | |
| "--nocapture", | |
| ]) |
| .args([ | ||
| "--exact", | ||
| "orchestrator::orchestrator_capture_snapshot_can_be_published_and_relaunched", | ||
| "--nocapture", | ||
| ]) |
There was a problem hiding this comment.
Correction to the preceding review note: this file is included as mod orchestrator by tests/orchestrator_integration.rs, so the orchestrator::orchestrator_capture_snapshot_can_be_published_and_relaunched exact filter is valid here. No change is needed for this invocation.
Suggestion:
| .args([ | |
| "--exact", | |
| "orchestrator::orchestrator_capture_snapshot_can_be_published_and_relaunched", | |
| "--nocapture", | |
| ]) | |
| .args([ | |
| "--exact", | |
| "orchestrator::orchestrator_capture_snapshot_can_be_published_and_relaunched", | |
| "--nocapture", | |
| ]) |
What
Integrate the nested-KVM runtime and restore legacy v9 snapshots with the current configured Firecracker.
Why
An upgrade can replace the old runtime directory. Restored sandboxes should use the current runtime, matching the prewarmed process pool.
Related issue
Companion runtime: Firecracker #27.
Scope and non-goals
Runtime selection applies to the shared restore path. Hardware validation covers Intel and AMD nested KVM and ordinary ARM64 KVM. The kernel default changes only for x86_64 KVM.
Design and behavior changes
1.15.1-nestedvirt-v1and its matching helper for x86_64 and aarch64.Compatibility and operations
Restart AgentENV after upgrading. The new runtime reads v9 and writes v1000; old runtimes cannot read v1000. Existing snapshots retain their captured kernel and CPU state. Public APIs and snapshot artifact layouts are unchanged. The x86_64 Firecracker release archive was refreshed with AMD GIF preservation under the same version; existing cached runtimes need explicit replacement.
Validation
Risks and reviewer notes
Three commits: runtime/test integration; legacy snapshot recovery; x86_64 KVM kernel default. Review runtime selection in
src/sandbox/firecracker/sandbox.rsand dependency selection insrc/cfg.rs/src/deps/mod.rs.Checklist