[WIP] feat(minvmd,minimald): host gvproxy + per-PTask vsock shuttle for DM1/3/4 (#572) - #577
Closed
norrietaylor wants to merge 2 commits into
Closed
[WIP] feat(minvmd,minimald): host gvproxy + per-PTask vsock shuttle for DM1/3/4 (#572)#577norrietaylor wants to merge 2 commits into
norrietaylor wants to merge 2 commits into
Conversation
On DM1/3/4 (a libkrun VM) gvproxy must run on the host, owned by minvmd, not inside the guest where it has no host uplink (issue #572). Wire up the previously-dead host-side switch: - HostGvproxy: an RAII supervisor that spawns the host gvproxy switch (reusing GvproxyConfig::spawn) on a dedicated current-thread tokio runtime, so minvmd's synchronous boot/run supervisor can own an async switch lifecycle and tear it down on drop. Started with an empty static-lease table — the guest configures each PTask's switch IP statically, so minvmd need not own the per-PTask address book. - run --foreground spawns + supervises the host gvproxy for an own-IP VM before booting the VMM child, so the -listen switch socket exists when libkrun dials it; the handle is held for the VM's lifetime. - VmConfig::apply registers the per-PTask shuttle vsock bridge for an own-IP VM: add_vsock_port2(VSOCK_GVPROXY_SHUTTLE_PORT, switch_sock, listen = false) — the guest shuttle connects to AF_VSOCK CID 2 and libkrun splices it to the host gvproxy -listen socket, carrying raw L2 frames (no second TCP/IP stack). - net::shuttle: the shuttle vsock port + host switch-socket path resolver (placed beside the minimald bridge socket). - image::resolve_gvproxy_path reads MINVMD_GVPROXY_BIN (fallback to the fixed install path); cmd::own_ip_requested reads MINVMD_VM_OWN_IP so the parent's gvproxy-spawn decision and the VMM child's VM network mode stay in lock-step. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
In a libkrun VM (DM1/3/4) minimald-in-guest must NOT spawn gvproxy locally — gvproxy runs on the host (owned by minvmd) and the guest only relays raw L2 frames to it (issue #572). Previously the DM2 spawn-local path ran verbatim in the guest, so an OwnIp PTask got a 100.64 switch IP but no egress (gvproxy NAT'd into a dead-end netns). - net::SwitchTransport enum (LocalSpawn vs HostShuttle { cid, port }) makes "spawn locally" and "relay to the host switch" mutually exclusive in the type system. GvproxySwitch::with_transport selects it; in HostShuttle mode attach() skips the local gvproxy spawn/config and only tracks the attach count. - switch::attach_to_switch_vsock relays a PTask tap to the host gvproxy over AF_VSOCK (CID 2 : shuttle port); it shares a transport-agnostic spawn_relay with the DM2 UDS attach_to_switch — same HyperKit-framed raw-L2 relay, so exactly one gVisor stack stays in the path. - session_host::attach_own_ip branches on the switch transport: DM2 uses the local control socket, DM1/3/4 the vsock shuttle. Static ingress on a VM own-IP PTask is skipped with a warning (gvproxy's port-forward API is host-side and unreachable over the frame-only shuttle); egress, the #572 focus, needs no such call. - DM signal: the `vsock` listen-arg is exactly the libkrun-VM boundary (DM1/3/4) vs the UDS daemon (DM2); threaded through Config::in_microvm into the transport selection. DM2 behaviour is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Contributor
|
This pull request has no accompanying spec. Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
crates/minimald/src/net/mod.rs (1)
61-65: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftSingle-source the shuttle port contract.
minimaldhardcodes the shuttle port here, whileminvmdregisters its own copy on the host side. A one-sided edit will still compile and only fail at runtime when VM own-IP attach tries to connect. Please move this port definition into shared code or re-export a single source of truth.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/minimald/src/net/mod.rs` around lines 61 - 65, The shuttle port contract is duplicated between minimald and minvmd, so update the VSOCK_GVPROXY_SHUTTLE_PORT definition to come from a single shared location instead of hardcoding it only in net::mod. Move or re-export the constant through a common module used by both sides, and adjust the references in VSOCK_GVPROXY_SHUTTLE_PORT so both guest and host code read the same source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/minimald/src/session_host.rs`:
- Around line 720-731: The HostShuttle branch in session_host::attach currently
logs a warning and returns an empty ingress list when ingress.port_mappings are
requested, which silently accepts an unsupported VM ingress setup. Update this
path to fail the attach/request explicitly instead of returning Vec::new(), so
callers of the own-IP PTask are told the ingress contract is unsupported until
the gvproxy host-side forwarding path exists. Use the ingress handling in
session_host::attach and the SwitchTransport::HostShuttle match arm as the
locating symbols.
In `@crates/minvmd/src/net.rs`:
- Around line 949-950: The readiness signal in the gvproxy startup path is sent
too early: `HostGvproxy::spawn()` and the surrounding `switch` setup in
`crates/minvmd/src/net.rs` should not report `Ok(pid)` immediately after
`Command::spawn()`. Change the readiness handoff so it waits until the gvproxy
switch socket is actually created and connectable before calling
`ready_tx.send(...)`, ensuring the pre-boot barrier in
`crates/minvmd/src/cmd/run.rs` only unblocks once the `-listen` socket is
usable; apply the same fix in the related readiness path around the other
referenced block.
- Around line 1405-1448: The HostGvproxy tests are using `sleep` as a fake
gvproxy binary, but `HostGvproxy::spawn()` always appends
`GvproxyConfig::argv()`, so `sleep` can exit immediately on invalid arguments
and make the tests race startup failure instead of supervision behavior. Replace
the stand-in with a temp helper script or binary that ignores extra args and
blocks, then keep the assertions in `host_gvproxy_spawns_supervises_and_stops`
and `host_gvproxy_drop_stops_the_switch` focused on steady-state lifecycle and
stop behavior. Leave `host_gvproxy_spawn_reports_launch_failure` unchanged since
it validates the missing-binary error path.
- Around line 961-968: The unexpected-exit handling in net.rs still drops
GvproxySwitch without marking it as stopping, so GvproxySwitch::Drop may send a
signal after the process has already exited. Update the exit.recv() branch in
the host gvproxy switch loop to set the switch into a stopping/consumed state
before drop(switch), or route it through a no-signal consume path, so the Drop
implementation does not attempt to signal a recycled PID.
In `@crates/minvmd/src/net/shuttle.rs`:
- Around line 73-78: The test around resolve_switch_sock currently mutates
XDG_RUNTIME_DIR globally and removes any pre-existing value, which can affect
other tests or callers. Update the shuttle test to guard the environment change
the same way as the pattern used in sock.rs: serialize access with the shared
lock, capture the original XDG_RUNTIME_DIR value before calling
std::env::set_var, and restore it afterward instead of unconditionally removing
it. If possible, avoid mutating the environment in this test altogether while
keeping the resolve_switch_sock behavior deterministic.
---
Nitpick comments:
In `@crates/minimald/src/net/mod.rs`:
- Around line 61-65: The shuttle port contract is duplicated between minimald
and minvmd, so update the VSOCK_GVPROXY_SHUTTLE_PORT definition to come from a
single shared location instead of hardcoding it only in net::mod. Move or
re-export the constant through a common module used by both sides, and adjust
the references in VSOCK_GVPROXY_SHUTTLE_PORT so both guest and host code read
the same source of truth.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 85bb3288-2c81-4170-8cc5-12a508e7e361
📒 Files selected for processing (13)
crates/minimald/src/main.rscrates/minimald/src/net/mod.rscrates/minimald/src/net/switch.rscrates/minimald/src/server.rscrates/minimald/src/session_host.rscrates/minimald/src/test_harness.rscrates/minvmd/src/cmd/mod.rscrates/minvmd/src/cmd/run.rscrates/minvmd/src/cmd/vmm_child.rscrates/minvmd/src/image.rscrates/minvmd/src/net.rscrates/minvmd/src/net/shuttle.rscrates/minvmd/src/vm.rs
Comment on lines
720
to
+731
| let exposed = match ingress { | ||
| Some(ingress) | ||
| if !ingress.port_mappings.is_empty() | ||
| && matches!(transport, SwitchTransport::HostShuttle { .. }) => | ||
| { | ||
| tracing::warn!( | ||
| ip = %lease.ip, | ||
| "static ingress on a VM (DM1/3/4) own-IP PTask is not yet wired to the \ | ||
| host gvproxy port-forward API; egress works, ingress is skipped (issue #572)" | ||
| ); | ||
| Vec::new() | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don't silently accept unsupported VM ingress.
When port_mappings are present in HostShuttle mode, this still returns a successful attachment and just drops the requested forwards. That leaves callers with a running own-IP PTask whose ingress contract is broken. Fail the attach/request explicitly until the host-side gvproxy API path exists.
Suggested direction
Some(ingress)
if !ingress.port_mappings.is_empty()
&& matches!(transport, SwitchTransport::HostShuttle { .. }) =>
{
- tracing::warn!(
- ip = %lease.ip,
- "static ingress on a VM (DM1/3/4) own-IP PTask is not yet wired to the \
- host gvproxy port-forward API; egress works, ingress is skipped (issue `#572`)"
- );
- Vec::new()
+ return Err(io::Error::new(
+ io::ErrorKind::Unsupported,
+ "static ingress for own-IP PTasks is not supported in microvm mode yet",
+ ));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let exposed = match ingress { | |
| Some(ingress) | |
| if !ingress.port_mappings.is_empty() | |
| && matches!(transport, SwitchTransport::HostShuttle { .. }) => | |
| { | |
| tracing::warn!( | |
| ip = %lease.ip, | |
| "static ingress on a VM (DM1/3/4) own-IP PTask is not yet wired to the \ | |
| host gvproxy port-forward API; egress works, ingress is skipped (issue #572)" | |
| ); | |
| Vec::new() | |
| } | |
| let exposed = match ingress { | |
| Some(ingress) | |
| if !ingress.port_mappings.is_empty() | |
| && matches!(transport, SwitchTransport::HostShuttle { .. }) => | |
| { | |
| return Err(io::Error::new( | |
| io::ErrorKind::Unsupported, | |
| "static ingress for own-IP PTasks is not supported in microvm mode yet", | |
| )); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/minimald/src/session_host.rs` around lines 720 - 731, The HostShuttle
branch in session_host::attach currently logs a warning and returns an empty
ingress list when ingress.port_mappings are requested, which silently accepts an
unsupported VM ingress setup. Update this path to fail the attach/request
explicitly instead of returning Vec::new(), so callers of the own-IP PTask are
told the ingress contract is unsupported until the gvproxy host-side forwarding
path exists. Use the ingress handling in session_host::attach and the
SwitchTransport::HostShuttle match arm as the locating symbols.
Comment on lines
+949
to
+950
| let pid = switch.pid(); | ||
| if ready_tx.send(Ok(pid)).is_err() { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Don't report gvproxy as ready before its switch socket is usable.
This sends Ok(pid) immediately after Command::spawn(), but crates/minvmd/src/cmd/run.rs uses HostGvproxy::spawn() as the pre-boot barrier that should guarantee the -listen socket exists before libkrun connects. A slow bind or early startup failure turns VM boot into a race. Wait for the switch socket to be created/connectable before notifying readiness.
Also applies to: 974-979
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/minvmd/src/net.rs` around lines 949 - 950, The readiness signal in the
gvproxy startup path is sent too early: `HostGvproxy::spawn()` and the
surrounding `switch` setup in `crates/minvmd/src/net.rs` should not report
`Ok(pid)` immediately after `Command::spawn()`. Change the readiness handoff so
it waits until the gvproxy switch socket is actually created and connectable
before calling `ready_tx.send(...)`, ensuring the pre-boot barrier in
`crates/minvmd/src/cmd/run.rs` only unblocks once the `-listen` socket is
usable; apply the same fix in the related readiness path around the other
referenced block.
Comment on lines
+961
to
+968
| status = exit.recv() => { | ||
| tracing::error!( | ||
| pid, | ||
| code = status.and_then(|s| s.code()), | ||
| "host gvproxy switch exited unexpectedly", | ||
| ); | ||
| // gvproxy is already gone; drop the handle (no signal). | ||
| drop(switch); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Avoid signaling after the unexpected-exit path already won.
By the time this branch runs, the exit-notify path has already observed gvproxy's death. Dropping switch with stopping == false falls into GvproxySwitch::Drop; on non-Linux that sends SIGKILL by numeric PID and can hit a recycled process. Mark the switch as stopping, or add a consume-without-signal path, before dropping it.
Suggested fix
status = exit.recv() => {
tracing::error!(
pid,
code = status.and_then(|s| s.code()),
"host gvproxy switch exited unexpectedly",
);
// gvproxy is already gone; drop the handle (no signal).
+ switch.stopping.store(true, Ordering::Release);
drop(switch);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| status = exit.recv() => { | |
| tracing::error!( | |
| pid, | |
| code = status.and_then(|s| s.code()), | |
| "host gvproxy switch exited unexpectedly", | |
| ); | |
| // gvproxy is already gone; drop the handle (no signal). | |
| drop(switch); | |
| status = exit.recv() => { | |
| tracing::error!( | |
| pid, | |
| code = status.and_then(|s| s.code()), | |
| "host gvproxy switch exited unexpectedly", | |
| ); | |
| // gvproxy is already gone; drop the handle (no signal). | |
| switch.stopping.store(true, Ordering::Release); | |
| drop(switch); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/minvmd/src/net.rs` around lines 961 - 968, The unexpected-exit
handling in net.rs still drops GvproxySwitch without marking it as stopping, so
GvproxySwitch::Drop may send a signal after the process has already exited.
Update the exit.recv() branch in the host gvproxy switch loop to set the switch
into a stopping/consumed state before drop(switch), or route it through a
no-signal consume path, so the Drop implementation does not attempt to signal a
recycled PID.
Comment on lines
+1405
to
+1448
| #[test] | ||
| fn host_gvproxy_spawns_supervises_and_stops() { | ||
| // `sleep` stands in for gvproxy: HostGvproxy::spawn only needs a binary | ||
| // it can launch and read a PID from; the socket is dialed by libkrun in | ||
| // production, not by this supervisor. Use a temp socket path so no real | ||
| // file is needed (gvproxy would bind it; sleep ignores its argv). | ||
| let dir = tempfile::TempDir::new().expect("tempdir"); | ||
| let sock = dir.path().join("gvproxy-switch.sock"); | ||
| let gvproxy = HostGvproxy::spawn(PathBuf::from("sleep"), sock).expect("spawn host gvproxy"); | ||
| let pid = gvproxy.pid(); | ||
| assert!( | ||
| pid_is_alive(pid), | ||
| "host gvproxy should be alive after spawn" | ||
| ); | ||
| gvproxy.stop(); | ||
| // stop() signals SIGTERM and joins the supervising runtime thread, which | ||
| // only returns once gvproxy has been reaped. | ||
| assert!( | ||
| !pid_is_alive(pid), | ||
| "host gvproxy must be stopped after stop()" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn host_gvproxy_drop_stops_the_switch() { | ||
| let dir = tempfile::TempDir::new().expect("tempdir"); | ||
| let sock = dir.path().join("gvproxy-switch.sock"); | ||
| let gvproxy = HostGvproxy::spawn(PathBuf::from("sleep"), sock).expect("spawn host gvproxy"); | ||
| let pid = gvproxy.pid(); | ||
| assert!(pid_is_alive(pid)); | ||
| drop(gvproxy); | ||
| assert!( | ||
| !pid_is_alive(pid), | ||
| "dropping HostGvproxy must stop the switch" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn host_gvproxy_spawn_reports_launch_failure() { | ||
| let dir = tempfile::TempDir::new().expect("tempdir"); | ||
| let sock = dir.path().join("gvproxy-switch.sock"); | ||
| let err = HostGvproxy::spawn(PathBuf::from("/nonexistent/definitely/not/gvproxy"), sock) | ||
| .expect_err("spawning a missing binary must fail"); | ||
| assert_eq!(err.kind(), io::ErrorKind::NotFound); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
sleep is not a stable gvproxy stand-in here.
HostGvproxy::spawn() always appends GvproxyConfig::argv() (-config, -listen, -ssh-port), so sleep does not ignore the argv; it exits immediately on invalid options on GNU/BSD systems. These tests race that startup failure instead of validating steady-state supervision. Use a temp script/binary that ignores extra args and blocks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/minvmd/src/net.rs` around lines 1405 - 1448, The HostGvproxy tests are
using `sleep` as a fake gvproxy binary, but `HostGvproxy::spawn()` always
appends `GvproxyConfig::argv()`, so `sleep` can exit immediately on invalid
arguments and make the tests race startup failure instead of supervision
behavior. Replace the stand-in with a temp helper script or binary that ignores
extra args and blocks, then keep the assertions in
`host_gvproxy_spawns_supervises_and_stops` and
`host_gvproxy_drop_stops_the_switch` focused on steady-state lifecycle and stop
behavior. Leave `host_gvproxy_spawn_reports_launch_failure` unchanged since it
validates the missing-binary error path.
Comment on lines
+73
to
+78
| // Pin XDG_RUNTIME_DIR so the path is deterministic; the switch socket | ||
| // shares the minimald bridge's parent dir. | ||
| // SAFETY: single-threaded test; restored before returning. | ||
| unsafe { std::env::set_var("XDG_RUNTIME_DIR", "/run/user/1000") }; | ||
| let sock = resolve_switch_sock().unwrap(); | ||
| unsafe { std::env::remove_var("XDG_RUNTIME_DIR") }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and nearby lines.
git ls-files crates/minvmd/src/net/shuttle.rs
wc -l crates/minvmd/src/net/shuttle.rs
sed -n '1,180p' crates/minvmd/src/net/shuttle.rsRepository: gominimal/minimal
Length of output: 4023
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and nearby lines.
git ls-files crates/minvmd/src/net/shuttle.rs
wc -l crates/minvmd/src/net/shuttle.rs
sed -n '1,180p' crates/minvmd/src/net/shuttle.rsRepository: gominimal/minimal
Length of output: 4023
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and nearby lines.
git ls-files crates/minvmd/src/net/shuttle.rs
wc -l crates/minvmd/src/net/shuttle.rs
sed -n '1,180p' crates/minvmd/src/net/shuttle.rsRepository: gominimal/minimal
Length of output: 4023
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "set_var\\(|remove_var\\(" crates tests . -g '!target'Repository: gominimal/minimal
Length of output: 4363
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "set_var\\(|remove_var\\(" crates tests . -g '!target'Repository: gominimal/minimal
Length of output: 4363
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "set_var\\(|remove_var\\(" crates tests . -g '!target'Repository: gominimal/minimal
Length of output: 4363
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "set_var\\(|remove_var\\(" crates tests . -g '!target'Repository: gominimal/minimal
Length of output: 4363
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '110,180p' crates/minvmd/src/sock.rsRepository: gominimal/minimal
Length of output: 2948
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '110,180p' crates/minvmd/src/sock.rsRepository: gominimal/minimal
Length of output: 2948
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --hidden --glob '!.git' --glob '!target' --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' -- "--test-threads|serial_test|RUST_TEST_THREADS" .Repository: gominimal/minimal
Length of output: 1117
Guard this XDG_RUNTIME_DIR mutation.
std::env::set_var/remove_var is process-global, and this test drops any pre-existing value. Use a shared lock and restore the original value (as in crates/minvmd/src/sock.rs), or avoid mutating the environment here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/minvmd/src/net/shuttle.rs` around lines 73 - 78, The test around
resolve_switch_sock currently mutates XDG_RUNTIME_DIR globally and removes any
pre-existing value, which can affect other tests or callers. Update the shuttle
test to guard the environment change the same way as the pattern used in
sock.rs: serialize access with the shared lock, capture the original
XDG_RUNTIME_DIR value before calling std::env::set_var, and restore it afterward
instead of unconditionally removing it. If possible, avoid mutating the
environment in this test altogether while keeping the resolve_switch_sock
behavior deterministic.
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
1 participant
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.
What & why
Wherever
minimaldruns inside aminvmdlibkrun VM (DM1 macOS/HVF, DM3/DM4 Linux/KVM), gvproxy was being spawned inside the guest, which has no host uplink — so anOwnIpPTask got a100.64.x.xswitch IP but no real egress. Per the spec (docs/specs/03-spec-networking/{networking-with-diagrams.md,architecture.md}, decisionsshared-gvproxy-per-host,minvmd-owns-vm-gvproxy,ownip-ptask-fd-pass), gvproxy must run on the host (minvmd-owned) with a per-PTask vsock shuttle in the guest relaying raw L2 frames. DM2 (native Linux, no VM) is correct as-is and is left untouched.Design
Invariant preserved: exactly one gVisor TCP/IP stack in the path — the host gvproxy. The guest shuttle is a pure L2 frame relay (the same HyperKit
/connectframing the DM2 native relay uses), not a second gvproxy.The three pieces
minvmd spawns + supervises the host gvproxy (
crates/minvmd). Newnet::HostGvproxyRAII supervisor reuses the existingGvproxyConfig::spawn/GvproxySwitchon a dedicated current-thread tokio runtime (minvmd'srun/bootsupervisor is synchronous; the switch lifecycle is async).run --foregroundspawns it before the VMM child boots — so the-listenswitch socket exists when libkrun dials it — and holds the handle for the VM's lifetime (stop-on-drop). Started with an empty static-lease table: the guest configures each PTask's switch IP statically, so minvmd need not own the per-PTask address book.Per-PTask vsock shuttle (
crates/minvmd/src/vm.rs+crates/minimald/src/net/switch.rs).VmConfig::applyregistersadd_vsock_port2(VSOCK_GVPROXY_SHUTTLE_PORT=1024, switch_sock, listen=false)for an own-IP VM —listen=falsemeans the guest initiates the vsock connection and libkrun dials the host gvproxy-listensocket, splicing the two (the same direction as the READY-marker port). In the guest, the switch relay was generalized:attach_to_switch(DM2, UDS) and the newattach_to_switch_vsock(DM1/3/4) share a transport-agnosticspawn_relay; the bidirectional frame loops already work over anyAsyncRead/AsyncWrite.minimald deployment-model branch (
crates/minimald). Newnet::SwitchTransportenum (LocalSpawnvsHostShuttle { cid, port }) makes "spawn locally" and "shuttle to host" mutually exclusive in the type system.GvproxySwitch::with_transportselects it; inHostShuttlemodeattach()skips the local gvproxy spawn/config andattach_own_iprelays the tap over vsock. DM-detection signal: thevsocklisten-arg already cleanly distinguishes the libkrun-VM path (DM1/3/4) from the UDS path (DM2) inmain.rs— it is the VM boundary. It is threaded throughConfig::in_microvminto the transport selection. (is_minimal_microvm()/ argv0==/initis the same population, butvsockis the precise per-listener signal already at hand.) DM2 (in_microvm=false) keeps the local-spawn + tap-relay path verbatim.Verified
cargo fmt— clean.cargo build -p minvmd,cargo clippy -p minvmd --all-targets -- -D warnings— clean (libkrun build on the macOS host).cargo test -p minvmd— 90 passed, incl. 3 newHostGvproxytests (spawn/supervise/stop, drop-stops, launch-failure surfacesNotFound).cross test ... --lib net::(QEMU): 39 passed, 0 failed — incl.transport_defaults_to_local_spawn,with_transport_selects_host_shuttle, andhost_shuttle_attach_allocates_without_spawning_gvproxy(aHostShuttleswitch attaches without spawning the binary).cross build -p minimald --profile initramfs --target aarch64-unknown-linux-musl --features networking-proxy,networking-wg— succeeded (Finished initramfs profile). Note: in this worktree the cross build initially failed incrates/minimald/build.rs(git rev-parsereturns empty inside the container because the worktree's.gitis a file pointing outside the mounted dir — a pre-existing build-script fragility, unrelated to this change); building with the parent.gitbind-mounted into the container confirms the crate itself compiles clean.NOT verified (pending)
MINVMD_VM_OWN_IPenv var (read consistently by both the supervisor and the VMM child). NominvmdCLI flag is added — the VMM child still defaults toHostNetunless the env is set.Coordination
PR #575 (branch
feat/exec-in-session-ptask, not merged) makesattach_own_ippub(crate)and factorsbuild_session_envinsession_host.rs. This branch is based on origin/main (which lacks #575), so it carries the private versions. Thesession_host.rsattach_own_ipseam will need a trivial rebase against #575 whenever both land (per #575's note); the changes here are confined to the body ofattach_own_ipand do not touch its signature beyond what #575 already changes.Refs #572
Summary by CodeRabbit
New Features
Bug Fixes