diff --git a/docs/bridge-networking.md b/docs/bridge-networking.md index 6bf3e6d5e..43216d086 100644 --- a/docs/bridge-networking.md +++ b/docs/bridge-networking.md @@ -143,35 +143,37 @@ mode = "bridge" bridge = "dstack-br0" ``` -### QEMU bridge helper setup (needed unless every bridge NIC goes through netd) +### netd is required -The bridge helper allows QEMU to create and attach TAP devices without VMM needing root privileges. -It is used only on the single-queue bridge paths; a NIC that `netd` builds never touches it, so a -node that runs `netd` for all of its bridge VMs does not need it at all. - -The VMM probes `/usr/lib/qemu/qemu-bridge-helper`, `/usr/libexec/qemu-bridge-helper` and -`/usr/local/libexec/qemu-bridge-helper`. Set `cvm.qemu_bridge_helper` in `vmm.toml` for a path -outside that list. +Bridge networking needs `netd`, the privileged helper that owns every host +interface a bridge or macvtap NIC uses. It is the same binary: ```bash -# Allow QEMU to use the bridge -sudo mkdir -p /etc/qemu -echo "allow virbr0" | sudo tee /etc/qemu/bridge.conf -# Or for manual bridge: echo "allow dstack-br0" | sudo tee /etc/qemu/bridge.conf - -# Set setuid on bridge helper -sudo chmod u+s /usr/lib/qemu/qemu-bridge-helper +sudo dstack-vmm --config vmm.toml netd ``` +Nothing else on the node needs `CAP_NET_ADMIN`: the VMM itself still runs +unprivileged, and `netd` holds the privilege behind a Unix socket whose +filesystem permissions authorize callers. + +This used to be conditional — `netd` built the TAP when libvirt filtering was on +or when the NIC wanted more than one queue pair, and otherwise QEMU's setuid +`qemu-bridge-helper` did. Two owners meant two answers to the same questions: +which netdev QEMU gets, whether vhost is really on, and what a bridge NIC's TAP +is built with. So a bridge NIC's host interface has one owner now, on every +node. + +`qemu-bridge-helper` is no longer used, and `/etc/qemu/bridge.conf` no longer +needs an `allow` line for the bridge. + ## How it works -- With more than one queue pair, or with libvirt filtering on, `netd` creates the TAP and the VMM passes `-netdev tap,id=net0,ifname=,...` — this is the usual case on a node running `netd` with multi-vCPU VMs, since queue pairs default to the VM's vCPU count. Without `netd`, a bridge NIC that took that default drops back to one queue pair and takes a helper path below -- Otherwise the VMM passes `-netdev tap,id=net0,br=,helper=,vhost=on`, or `-netdev bridge,id=net0,br=` when vhost is off or no helper is found -- QEMU's bridge helper (setuid) creates a TAP device and attaches it to the bridge on the two helper paths +- `netd` creates a persistent TAP, attaches it to the bridge, binds the nwfilter if the node filters, and the VMM passes `-netdev tap,id=net0,ifname=,...` - Guest MAC address is derived from SHA256 of the VM ID, with an optional configurable prefix (stable across restarts for DHCP IP consistency) - The host DHCP server (dnsmasq) assigns an IP to the VM -- On the two bridge-helper paths the TAP disappears when QEMU exits; a `netd`-created TAP is persistent and is deleted when the VMM tears the VM's networking down -- The VMM process itself needs neither root nor `CAP_NET_ADMIN` on any path; the `netd` path moves that privilege into a separate root service instead +- The TAP outlives QEMU and is deleted when the VMM tears the VM's networking down, so a VM that crashes does not leave its filter rules attached to a name the next VM could take +- Every interface `netd` creates records which VM of which VMM instance it belongs to, in the kernel's interface alias — see [Who owns an interface](#who-owns-an-interface) +- The VMM process needs neither root nor `CAP_NET_ADMIN`; `netd` holds that privilege in a separate service ### MAC address prefix @@ -199,6 +201,101 @@ The remaining bytes are derived from the VM ID hash. The prefix applies to all n - Docker's nftables chains (`DOCKER-FORWARD`) run before libvirt's but do not block virbr0 traffic - Use `setup-bridge.sh check --bridge ` to diagnose missing rules +### Which NIC a port mapping uses + +A port mapping says which NIC its traffic enters through: + +```bash +vmm-cli.py deploy ... --port udp:0.0.0.0:7483:51820@0 --port tcp:127.0.0.1:7484:8001@0 +``` + +Leave `@` off and the VMM picks the first user-mode NIC — where QEMU's +`hostfwd=` entries have always gone — and failing that the first bridge NIC. A +single-NIC VM never needs it. + +With several NICs the choice used to be made silently, and not always the way an +operator would have. A bridge NIC for external traffic beside a user-mode NIC for +management — the topology multi-NIC was added for — put every published port on +the *management* NIC: the traffic reached the guest, but over slirp, bypassing +whatever the bridge NIC's nwfilter was there to enforce and hiding the client's +address behind the slirp gateway. A second user-mode NIC could never publish +anything at all, because only the first was ever selected. + +A mapping resolves to exactly one NIC, and that NIC's backend decides the +mechanism: `hostfwd=` for user mode, `netd` for a bridge. Nothing can be claimed +by both. + +### Which ports a bridge NIC can publish + +QEMU publishes a port with `hostfwd=` on a user-mode NIC, and that is the only +mechanism this host has. **The `netd` in this repository builds interfaces; it +does not forward host ports**, so a bridge NIC cannot carry a port mapping. + +`--port …@` therefore only ever names a user-mode NIC. Pinning to a bridge, +macvtap or custom NIC is refused at deployment, where the caller is there to be +told. An unpinned mapping goes to the first user-mode NIC; a VM that has none is +not refused — it may have been deployed before this — but every mapping it +strands is named in the launch log. + +## Who owns an interface + +`netd` names an interface `dt<12 hex>`, a digest of (VMM instance, VM, NIC +index). That answers "where is this VM's interface" but not "whose is this +interface" — and the second question is the one a leaked interface poses. So +`netd` also records the identity on the interface itself: + +```console +$ ip -d link show dtc41d9e0b7a52 | grep alias + alias dstack1:0:path-3f9a1c8e7d2b4a60:0a1b2c3d4e5f6071 +``` + +The kernel holds that for exactly the interface's lifetime, so unlike a file on +disk it cannot be written late, lost, or left behind. It is a hint, never an +authority: a record is believed only when re-deriving the interface name from +it reproduces the name it is written on, so a forged, truncated or ambiguous +record reads the same as no record at all. + +Teardown does not need it — a sweep derives the names it deletes. What needs it +is an operator, and a host running several VMM instances, where it is the only +thing that tells one instance's interfaces from another's. + +```bash +# What netd holds on this host +sudo dstack-vmm netd list + +# Everything one VM holds, for a VM whose VMM will never ask again +sudo dstack-vmm netd remove-vm --instance path-3f9a1c8e7d2b4a60 --vm 0a1b2c3d4e5f6071 +``` + +### When a release does not land + +Every stop and every removal asks `netd` to sweep that VM's interfaces, by +deriving each of the 256 names its identity could produce. That needs no +record, and it reaches what a per-NIC teardown cannot: an interface a crash +left behind before anything on disk pointed at it, or one whose NIC the +manifest has since dropped. + +A removal deletes the VM's directory, and that directory — with its `.removing` +marker — is the only thing left that says to try again. So it is deleted only +once the sweep has landed. If `netd` refused, or was not there to ask, the +directory stays and the next VMM start resumes the removal; `remove_all` is +idempotent, so the retry costs one round trip. A VM that never asked `netd` for +an interface is unaffected: there is nothing for `netd` to be holding. + +What no VMM will retry is an interface whose VM directory an operator deleted +by hand, or one recorded under an instance ID no VMM uses any more. `netd list` +shows both, with the instance and VM they are recorded under: + +```bash +sudo dstack-vmm netd list +sudo dstack-vmm netd remove-vm --instance --vm +sudo dstack-vmm netd remove-interface dtc41d9e0b7a52 +``` + +Changing `cvm.instance_id` — or `run_path`, which it is derived from — strands +interfaces the same way. Running VMs keep working until they stop, and +`netd list` still shows the old instance ID, which is what `remove-vm` needs. + ### Mixing networking modes Bridge and user-mode VMs can coexist. Set the global default in `vmm.toml` and override per-VM as needed: diff --git a/docs/libvirt-network-filter.md b/docs/libvirt-network-filter.md index 6cc2724cf..3a1313aef 100644 --- a/docs/libvirt-network-filter.md +++ b/docs/libvirt-network-filter.md @@ -14,13 +14,17 @@ host mechanism. The measurable acceptance criteria are: -- `network_filter = "none"` installs no nwfilter binding. It still uses `netd` - for any NIC with more than one queue pair, and a `tap` netdev behind - `qemu-bridge-helper` whenever vhost is on; only a single-queue, non-vhost - bridge NIC keeps the historical `-netdev bridge` path with no `netd` or - libvirt dependency. +- `network_filter = "none"` installs no nwfilter binding. It does not remove + the `netd` dependency: `netd` creates the TAP for every bridge NIC either + way, and the VMM uses `-netdev tap` either way. What changes is only whether + that TAP carries a binding. - `network_filter = "libvirt"` creates the TAP and filter binding before QEMU is submitted to Supervisor, and uses QEMU `-netdev tap`. +- An nwfilter binding outlives the TAP it was bound to, so a teardown clears + the binding at every name that VM could have used, whether or not the + interface is still there. `dstack-vmm netd list` shows a binding whose + interface is already gone as a `binding` row; remove one with + `dstack-vmm netd remove-interface `. - A failed TAP or filter setup prevents QEMU from starting and rolls back all interfaces prepared for that VM. - Normal stop and removal delete the filter binding and TAP. @@ -105,6 +109,26 @@ arguments. It never accepts a command, executable path, TAP name, or raw XML from a client. Filter XML is generated internally with XML escaping and is validated by libvirt. +Teardown by identity only reaches the NIC indices its caller still has a record +of, and that record is written *after* the interface exists — a VMM killed in +between leaves a TAP nothing on disk points at, and a manifest that lost a NIC +leaves the same thing behind. `remove_all` names a VM instead of an interface +and derives every name that VM could occupy, so neither has to be recorded for +teardown to work. The VMM sweeps before preparing a launch as well as on stop, +which makes a launch self-healing regardless of what the record says. + +A bridge prepare also carries two things `netd` does not need to build the TAP. +`workdir` names the VM's directory on the host: untrusted, never read for a +decision, and present only so an operator reading `netd`'s log can get from an +opaque TAP name back to the VM. `ingress` states the host ports that NIC should make +reachable at its guest, which the VMM cannot arrange itself — it runs without +`CAP_NET_ADMIN` by design, and QEMU's `hostfwd=` entries need a user-mode netdev +that a bridge NIC does not have. The `netd` in this repository builds interfaces +and does not forward ports; it says so by leaving `ingress` out of its response, +the same reading `queues` gets, so a caller can tell "this netd does not do that" +from "nothing was asked for" instead of assuming ports were forwarded because a +TAP came back. + ## Deployment modes Production should run one shared service. `netd` reads the `[netd]` section, @@ -194,11 +218,11 @@ sudo dstack-vmm --config ./vmm.toml \ --netd-socket /run/dstack-dev/netd.sock ``` -User networking never asks `netd` to build an interface; the VMM still opens a -short liveness-probe connection to the netd socket on every launch and when -describing a stopped VM. Libvirt mode fails closed if `netd` -is unavailable. Bridge networking with `mode = "none"` connects only when it -needs more than one queue pair, as described below. +User networking and a caller-supplied netdev never ask `netd` to build an +interface. The VMM still contacts the socket for such a VM -- every launch and +every stop releases whatever the VM held, before it decides whether it needs +anything built -- but nothing about the VM depends on the answer. Bridge and +macvtap do ask, and fail closed if `netd` is unavailable. Filtered TAP netdevs follow the node's `vhost` and `queues` settings like any other TAP-backed NIC (see [network-data-plane.md](network-data-plane.md)). The @@ -207,10 +231,11 @@ whether they were written by QEMU or by a vhost worker; filtering is unaffected by the data plane choice. Enabling vhost does require the QEMU user to be able to open `/dev/vhost-net`. -`netd` also creates the TAP for unfiltered bridge NICs that ask for more than -one queue pair, because `qemu-bridge-helper` returns a single descriptor and -cannot create a `multi_queue` device. Those TAPs carry no nwfilter binding, so -a multiqueue bridge node needs `netd` even when `network_filter.mode = "none"`. +`netd` creates the TAP for unfiltered bridge NICs too. Those TAPs carry no +nwfilter binding, so a bridge node needs `netd` even when +`network_filter.mode = "none"` — see +[bridge-networking.md](bridge-networking.md) for why the host interface has a +single owner. An empty filter name is what selects that unfiltered TAP, so `mode = "libvirt"` with an empty `filter` is rejected at config load rather than quietly producing diff --git a/docs/network-data-plane.md b/docs/network-data-plane.md index 57f0134a1..c80af880f 100644 --- a/docs/network-data-plane.md +++ b/docs/network-data-plane.md @@ -64,8 +64,8 @@ refuses a request for four queue pairs should not hand out sixteen by itself. The hard ceiling from any source is 64. Without vhost the default is a single queue pair. The QEMU main loop drains -every queue on one thread, so extra queues buy little while still costing a -netd interface, more MSI-X vectors, and a changed guest device. An explicit +every queue on one thread, so extra queues buy little while still costing more +MSI-X vectors and a changed guest device. An explicit queue count is still honoured without vhost, since that combination is a deliberate request rather than a default. The two defaults travelling together also means a node that never sets `vhost` keeps building the device its VMs @@ -150,9 +150,9 @@ measured, so attestation and app identity are unaffected. Before flipping it: restarting. The VMM warns at startup when its own access fails, but it cannot refuse on that basis — QEMU need not share its credentials. -2. **Restart `netd` before or together with the VMM.** Multiqueue bridge NICs - are prepared by `netd`, and the VMM checks that `netd` echoes the queue - count it built. An older `netd` fails that check; the launch is rolled back +2. **Restart `netd` before or together with the VMM.** Every bridge and + macvtap NIC is prepared by `netd`, and the VMM checks that `netd` echoes the + queue count it built. An older `netd` fails that check; the launch is rolled back and fails with the reason in the VMM log, but the VM does not start until `netd` is upgraded. @@ -165,35 +165,22 @@ measured, so attestation and app identity are unaffected. Before flipping it: | Mode | netdev | vhost | queues > 1 | |---|---|---|---| | `user` | `user,...` | no backend | not supported | -| `bridge` | `tap,ifname=` via netd, else `tap,br=,helper=`, else `bridge,br=` | yes | yes, through netd | -| `bridge` with libvirt filtering | `tap,ifname=` | yes | yes, through netd | +| `bridge` | `tap,ifname=` via netd | yes | yes | | `macvtap` | `tap,fd=` / `tap,fds=` | yes | yes | | `custom` | operator's own string | operator's own string | no, not settable | -QEMU's `bridge` netdev accepts neither `vhost=` nor `queues=`, so enabling -vhost switches bridge mode to a `tap` netdev driven by the same setuid -`qemu-bridge-helper`. The VMM still needs no network privileges. The helper has -no compiled-in default path for the `tap` netdev, so the VMM probes the known -distribution locations; set `cvm.qemu_bridge_helper` if yours is elsewhere. If -no helper is found the NIC falls back to the non-vhost `bridge` netdev with a -warning, because a node-wide setting must not stop a node from booting VMs -that never asked for it. - -The helper returns exactly one descriptor, which is why more than one queue -pair in bridge mode is created by `netd` instead: it adds a persistent -`multi_queue` TAP that QEMU then opens once per queue. `netd` requires the -`virsh` binary to be installed even when nothing is filtered, though it does -not require a reachable `libvirtd`. That applies whether or -not libvirt filtering is on, so a bridge node needs `netd` to get the default -queue count (see [libvirt-network-filter.md](libvirt-network-filter.md)). -Without it, bridge NICs fall back to a single queue pair with a warning rather -than failing to launch; a VM that asked for a queue count explicitly still -fails, so the caller learns their request was not met. `netd` is probed by -connecting, not by looking for its socket file, because a `netd` that died -leaves the socket behind. One-shot `dstack-vmm run` has no netd lifecycle at -all and behaves like a node without it. `netd` reports back the -queue count it created, and the VMM refuses to launch on a mismatch — a `netd` -deployed separately as a root service can be older than the VMM asking it for +QEMU's `bridge` netdev accepts neither `vhost=` nor `queues=`, and the setuid +`qemu-bridge-helper` behind its `tap` netdev returns exactly one descriptor. So +bridge mode runs on a TAP that `netd` creates: persistent, `multi_queue` when +asked for, and opened once per queue by QEMU. That is true of every bridge NIC, +filtered or not, single-queue or not — see +[bridge-networking.md](bridge-networking.md) for why the host interface has one +owner. `netd` requires the `virsh` binary to be installed even when nothing is +filtered, though it does not require a reachable `libvirtd`. One-shot +`dstack-vmm run` does not manage netd interface lifecycle, so it refuses bridge +and macvtap NICs outside `--dry-run`. `netd` reports back the queue count it +created, and the VMM refuses to launch on a mismatch — a `netd` deployed +separately as a root service can be older than the VMM asking it for multiqueue, and QEMU would otherwise reject the interface from inside the per-VM launcher. @@ -253,7 +240,7 @@ multiqueue line rate with zero `swiotlb buffer is full` events. `vectors` is derived, never configured: `2N + 2`, one vector per queue direction plus config and control. One queue pair emits no `mq=on` or `vectors=` at all, leaving the guest device line byte for byte identical to the -one before this feature. The `-netdev` half does change wherever vhost is on, +one before this feature. The `-netdev` half carries `vhost=on|off` either way, since that is what selects the backend. ## Requirements diff --git a/docs/onboarding.md b/docs/onboarding.md index 0d4488bab..28b216ff2 100644 --- a/docs/onboarding.md +++ b/docs/onboarding.md @@ -149,7 +149,7 @@ The deploy command: Pass `--vcpu`, `--memory`, or `--disk` to change the app resources before you deploy. -The `--port 8080:80` mapping means `host_port:vm_port` and uses TCP on `127.0.0.1`. The full accepted forms are `vm`, `host:vm`, `proto:host:vm`, and `proto:addr:host:vm`. Use `tcp` or `udp` for `proto`. Fixed host and VM ports must be between 1 and 65535. If you omit the host port, or use `auto` or `0`, `dstack` picks a free localhost port and prints the selected mapping after deploy. +The `--port 8080:80` mapping means `host_port:vm_port` and uses TCP on `127.0.0.1`. The full accepted forms are `vm`, `host:vm`, `proto:host:vm`, and `proto:addr:host:vm`, each optionally suffixed with `@` to name which NIC the traffic enters through (a single-NIC VM never needs it). Use `tcp` or `udp` for `proto`. Fixed host and VM ports must be between 1 and 65535. If you omit the host port, or use `auto` or `0`, `dstack` picks a free localhost port and prints the selected mapping after deploy. Open the app from the host: diff --git a/docs/vmm-cli-user-guide.md b/docs/vmm-cli-user-guide.md index 6a669ca4e..1f803e31c 100644 --- a/docs/vmm-cli-user-guide.md +++ b/docs/vmm-cli-user-guide.md @@ -290,6 +290,11 @@ Expose services running in your VM: # Multiple ports --port tcp:8080:80 --port tcp:8443:443 + +# Pin a mapping to one NIC: protocol[:host_address]:host_port:vm_port@ +# Without @ the mapping enters through the first user-mode NIC, or the +# first bridge NIC when the VM has no user-mode one. +--port tcp:0.0.0.0:8443:443@0 ``` #### GPU Assignment diff --git a/dstack/crates/dstack-cli-core/src/ports.rs b/dstack/crates/dstack-cli-core/src/ports.rs index ae662442a..142be1b16 100644 --- a/dstack/crates/dstack-cli-core/src/ports.rs +++ b/dstack/crates/dstack-cli-core/src/ports.rs @@ -33,7 +33,25 @@ pub fn tcp_port_free(addr: &str, port: u16) -> bool { /// * `:` — tcp, 127.0.0.1 /// * `::` /// * `:::` +/// +/// Any of them may carry a trailing `@` naming which NIC the traffic +/// enters through, as `vmm-cli.py --port` does. Without it the VMM picks: the +/// first user-mode NIC, else the first bridge NIC. A single-NIC VM never needs +/// it. pub fn parse_port(spec: &str) -> Result { + let (spec, nic_index) = match spec.rsplit_once('@') { + // Only digits. `parse()` would take " 1" and "+1" as 1, and a NIC index + // is a position in a list the caller wrote. + Some((rest, nic)) if nic.bytes().all(|byte| byte.is_ascii_digit()) && !nic.is_empty() => ( + rest, + Some( + nic.parse::() + .with_context(|| format!("invalid NIC index in --port '{spec}'"))?, + ), + ), + Some((_, nic)) => bail!("invalid NIC index in --port '{spec}': '{nic}' is not a number"), + None => (spec, None), + }; let parts: Vec<&str> = spec.split(':').collect(); let (proto, addr, host, vm) = match parts.as_slice() { [vm] => ("tcp", "127.0.0.1", "auto", *vm), @@ -58,6 +76,7 @@ pub fn parse_port(spec: &str) -> Result { host_address: addr.to_string(), host_port, vm_port, + nic_index, }) } @@ -90,4 +109,23 @@ mod tests { assert!(parse_port("70000:80").is_err()); assert!(parse_port("8080:0").is_err()); } + + #[test] + fn a_mapping_can_name_the_nic_it_enters_through() { + assert_eq!(parse_port("8080:80").unwrap().nic_index, None); + + let pinned = parse_port("udp:0.0.0.0:7483:51820@1").unwrap(); + assert_eq!(pinned.nic_index, Some(1)); + assert_eq!(pinned.protocol, "udp"); + assert_eq!(pinned.host_address, "0.0.0.0"); + assert_eq!(pinned.host_port, 7483); + assert_eq!(pinned.vm_port, 51820); + + // Unpinned must stay unpinned rather than default to NIC 0: the VMM + // resolves it to the first user-mode NIC, which need not be the first. + assert_eq!(parse_port("8080:80@0").unwrap().nic_index, Some(0)); + for bad in ["8080:80@", "8080:80@ 1", "8080:80@+1", "8080:80@a"] { + assert!(parse_port(bad).is_err(), "{bad} must be refused"); + } + } } diff --git a/dstack/crates/dstack-cli/src/main.rs b/dstack/crates/dstack-cli/src/main.rs index 9624c3a27..8a70141e9 100644 --- a/dstack/crates/dstack-cli/src/main.rs +++ b/dstack/crates/dstack-cli/src/main.rs @@ -102,7 +102,8 @@ enum Command { /// disk size in GB. #[arg(long, default_value_t = 20)] disk: u32, - /// expose a port: `vm` | `host:vm` | `proto:host:vm` | `proto:addr:host:vm` + /// expose a port: `vm` | `host:vm` | `proto:host:vm` | `proto:addr:host:vm`, + /// each optionally suffixed `@` to name the NIC it enters through /// (host omitted/`auto`/`0` ⇒ a free host port is picked). Repeatable. #[arg(long = "port", value_name = "SPEC")] ports: Vec, diff --git a/dstack/crates/dstackup/src/install.rs b/dstack/crates/dstackup/src/install.rs index 6599bc3cb..b32f9fea0 100644 --- a/dstack/crates/dstackup/src/install.rs +++ b/dstack/crates/dstackup/src/install.rs @@ -294,6 +294,9 @@ pub(crate) async fn cmd_install(mut o: InstallOpts, release_api_base_url: &str) host_address: "127.0.0.1".into(), host_port: kms_port as u32, vm_port: 8000, + // Unpinned: this deploys the node default topology, which + // is one NIC, and the VMM resolves that itself. + nic_index: None, }], ..Default::default() }; diff --git a/dstack/scripts/setup-bridge.sh b/dstack/scripts/setup-bridge.sh index edcbe35a7..ecc3d2901 100755 --- a/dstack/scripts/setup-bridge.sh +++ b/dstack/scripts/setup-bridge.sh @@ -44,83 +44,6 @@ run_cmd() { fi } -# --- Detect qemu-bridge-helper path --- - -find_bridge_helper() { - local paths=( - /usr/lib/qemu/qemu-bridge-helper - /usr/libexec/qemu-bridge-helper - /usr/local/lib/qemu/qemu-bridge-helper - /usr/local/libexec/qemu-bridge-helper - ) - for p in "${paths[@]}"; do - if [[ -f "$p" ]]; then - echo "$p" - return 0 - fi - done - return 1 -} - -# --- Detect current bridge provider --- - -# Returns "libvirt:" if bridge is managed by a libvirt network, -# "standalone" otherwise. -detect_bridge_provider() { - if command -v virsh &>/dev/null; then - local name br - while read -r name; do - [[ -z "$name" ]] && continue - br=$(virsh net-dumpxml "$name" 2>/dev/null | grep -oP "/dev/null) - fi - echo "standalone" -} - -# --- Check functions --- - -check_bridge_helper() { - echo - bold "qemu-bridge-helper" - local helper - if ! helper=$(find_bridge_helper); then - check_fail "qemu-bridge-helper not found" - check_info "Install QEMU: sudo apt install qemu-system-x86" - return - fi - check_pass "found at $helper" - - if [[ -u "$helper" ]]; then - check_pass "setuid bit is set" - else - check_fail "setuid bit not set" - check_info "Fix: sudo chmod u+s $helper" - fi -} - -check_bridge_conf() { - echo - bold "/etc/qemu/bridge.conf" - local conf="/etc/qemu/bridge.conf" - if [[ ! -f "$conf" ]]; then - check_fail "$conf does not exist" - check_info "Fix: sudo mkdir -p /etc/qemu && echo 'allow $BRIDGE' | sudo tee $conf" - return - fi - check_pass "$conf exists" - - if grep -qE "^allow[[:space:]]+($BRIDGE|all)[[:space:]]*$" "$conf" 2>/dev/null; then - check_pass "bridge '$BRIDGE' is allowed" - else - check_fail "bridge '$BRIDGE' not found in $conf" - check_info "Fix: echo 'allow $BRIDGE' | sudo tee -a $conf" - fi -} - check_bridge_interface() { echo bold "bridge interface: $BRIDGE" @@ -326,33 +249,6 @@ check_forward_rules() { # --- Setup: common --- -setup_bridge_conf() { - echo - bold "Setting up /etc/qemu/bridge.conf" - run_cmd sudo mkdir -p /etc/qemu - if [[ -f /etc/qemu/bridge.conf ]] && grep -qE "^allow[[:space:]]+($BRIDGE|all)" /etc/qemu/bridge.conf 2>/dev/null; then - echo " already configured" - else - run_cmd bash -c "echo 'allow $BRIDGE' | sudo tee -a /etc/qemu/bridge.conf" - fi -} - -setup_bridge_helper() { - echo - bold "Setting up qemu-bridge-helper" - local helper - if ! helper=$(find_bridge_helper); then - echo " $(red 'ERROR'): qemu-bridge-helper not found. Install QEMU first." - return 1 - fi - if [[ -u "$helper" ]]; then - echo " setuid already set on $helper" - else - run_cmd sudo chmod u+s "$helper" - echo " setuid set on $helper" - fi -} - setup_ip_forward() { echo bold "Enabling IP forwarding" @@ -681,8 +577,6 @@ cmd_check() { echo "provider: $(bold 'standalone')" fi - check_bridge_helper - check_bridge_conf check_bridge_interface check_dhcp check_dhcp_firewall @@ -728,8 +622,6 @@ cmd_setup() { $DRY_RUN && echo "dry-run: $(yellow 'yes')" # Common setup - setup_bridge_conf - setup_bridge_helper setup_ip_forward # Mode-specific setup @@ -812,7 +704,9 @@ cmd_destroy() { fi fi - # Remove bridge.conf entry + # Remove the bridge.conf entry an older setup added. netd owns every host + # interface now, so qemu-bridge-helper is not used and the `allow` line is + # a standing grant to attach any local user's TAP to the bridge. local conf="/etc/qemu/bridge.conf" if [[ -f "$conf" ]] && grep -qE "^allow[[:space:]]+${BRIDGE}[[:space:]]*$" "$conf" 2>/dev/null; then echo diff --git a/dstack/vmm/rpc/proto/vmm_rpc.proto b/dstack/vmm/rpc/proto/vmm_rpc.proto index 31fef51f4..62bf381be 100644 --- a/dstack/vmm/rpc/proto/vmm_rpc.proto +++ b/dstack/vmm/rpc/proto/vmm_rpc.proto @@ -182,6 +182,16 @@ message PortMapping { uint32 vm_port = 3; // Host address string host_address = 4; + // Which NIC this mapping's traffic enters through, as an index into + // `networks`. Unset picks the first user-mode NIC, which is where QEMU's + // hostfwd entries have always gone and the only place a host port is + // published from. + // + // A VM with one NIC never needs it. With several there is a choice, and it + // used to be made silently: a bridge NIC for external traffic beside a + // user-mode NIC for management -- the topology multi-NIC was added for -- + // put every published port on the management NIC. + optional uint32 nic_index = 5; } // Partial configuration used when mutating an existing VM. diff --git a/dstack/vmm/src/app.rs b/dstack/vmm/src/app.rs index e9205b07d..f6deaa8ec 100644 --- a/dstack/vmm/src/app.rs +++ b/dstack/vmm/src/app.rs @@ -3,10 +3,7 @@ // SPDX-License-Identifier: Apache-2.0 use crate::{ - config::{ - Config, NetdInterface, Networking, NetworkingMode, NicNetworking, ProcessAnnotation, - Protocol, - }, + config::{Config, Networking, NetworkingMode, NicNetworking, ProcessAnnotation, Protocol}, logrotate, netd::{ self, InterfaceIdentity, PrepareBridgeRequest, PrepareMacvtapRequest, @@ -35,7 +32,6 @@ use rand::seq::SliceRandom; use serde::{Deserialize, Serialize}; use serde_json::json; use sha2::{Digest, Sha256}; -use std::cell::OnceCell; use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet, VecDeque}; use std::net::IpAddr; use std::path::{Path, PathBuf}; @@ -46,8 +42,8 @@ use tracing::{debug, error, info, warn}; pub use image::{Image, ImageInfo}; pub(crate) use network::{ - clamp_queues_without_netd, filters_bridge_traffic, needs_netd_interface, netd_available, - netd_teardown, resolve_networking, resolved_networks, settle_vhost, validate_resolved_network, + filters_bridge_traffic, mode_carries_ingress, needs_netd_interface, resolve_networking, + resolved_networks, settle_vhost, stranded_ingress, validate_resolved_network, validate_resolved_networks, }; pub use qemu::VmConfig; @@ -61,7 +57,7 @@ mod host_share; mod id_pool; mod image; mod mr_config; -mod network; +pub(crate) mod network; mod qemu; pub(crate) mod registry; mod vm_info; @@ -97,6 +93,10 @@ pub struct PortMapping { pub protocol: Protocol, pub from: u16, pub to: u16, + /// Which NIC carries this mapping. `None` resolves by the node's rule; see + /// [`crate::app::network::ingress_nic`]. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub nic_index: Option, } /// An extra disk attached to the VM (e.g. a pre-baked verity volume). `source` @@ -310,6 +310,9 @@ pub struct App { state: Arc>, /// Pull status for registry images: tag → status. pub(crate) pull_status: Arc>>, + /// One lock per VM, held across a launch or a teardown. See + /// [`App::launch_lock`]. + launch_locks: Arc>>>>, } const GUEST_AGENT_RPC_TIMEOUT: Duration = Duration::from_secs(30); @@ -337,12 +340,42 @@ impl App { state: Arc::new(Mutex::new(AppState { cid_pool, vms: HashMap::new(), + removing: HashSet::new(), })), config: Arc::new(config), pull_status: Arc::new(Mutex::new(std::collections::HashMap::new())), + launch_locks: Arc::new(Mutex::new(HashMap::new())), } } + /// Serializes everything that creates or deletes one VM's host interfaces. + /// + /// A launch reads whether QEMU is already up and then spends many awaits -- + /// a GPU reset, a whole netd conversation, building the QEMU arguments -- + /// before it launches anything. Nothing used to cover that window. Two + /// `StartVm` calls, or one racing the auto-restart timer, could both read + /// "not running", and the loser's sweep would delete the TAPs the winner's + /// QEMU was already holding open: a live VM silently loses its networking, + /// and the loser's error path then clears the winner's record of it. The + /// authoritative rejection lives in the supervisor, which is reached long + /// after the damage is done. + /// + /// A tokio mutex, because it is held across awaits. Per VM, because a slow + /// start must not stall unrelated ones. Taken by stop and by removal as + /// well as by start: those delete the same interfaces from the other side. + pub(crate) async fn launch_lock(&self, id: &str) -> tokio::sync::OwnedMutexGuard<()> { + self.launch_lock_handle(id).lock_owned().await + } + + fn launch_lock_handle(&self, id: &str) -> Arc> { + let mut locks = self.launch_locks.lock().or_panic("mutex poisoned"); + // A VM that is neither starting nor stopping leaves the map holding + // the only reference, so the map stays the size of what is in flight + // rather than of every VM this process has ever touched. + locks.retain(|_, lock| Arc::strong_count(lock) > 1); + locks.entry(id.to_string()).or_default().clone() + } + pub async fn load_vm( &self, work_dir: impl AsRef, @@ -406,6 +439,19 @@ impl App { Ok(()) } + /// Refuses an operation on a VM that is being removed. + /// + /// Cheap, and taken before the launch lock as well as under it. Removal + /// holds that lock until the VM has exited -- hours, by its own estimate -- + /// so anything that only asked afterwards would wait the removal out in + /// order to be told no. + pub(crate) fn refuse_if_removing(&self, id: &str) -> Result<()> { + if self.lock().is_removing(id) { + bail!("VM is being removed"); + } + Ok(()) + } + pub async fn start_vm(&self, id: &str) -> Result<()> { self.start_vm_with_restart_policy(id, true).await } @@ -420,13 +466,24 @@ impl App { vm.state.auto_restart.reset(); } } - { - let state = self.lock(); - if let Some(vm) = state.get(id) { - if vm.state.removing { - bail!("VM is being removed"); - } - } + // Before the lock as well as after it. Removal holds the lock until the + // VM has exited, so a launch that only asked afterwards would wait that + // out -- hours, by removal's own estimate -- to be told no. Asking + // first is not sufficient on its own, because the marker can be set + // while this waits; asking again under the lock is what makes it + // authoritative. + self.refuse_if_removing(id)?; + // Everything below reads whether this VM is running and acts on the + // answer for as long as the launch takes. See [`App::launch_lock`]. + let _launch = self.launch_lock(id).await; + self.refuse_if_removing(id)?; + // A restart decided before a stop must not outlive it. The decision + // read `started` from disk; `stop_vm` writes it false under this lock, + // so re-reading it here is what makes the stop stick. An explicit start + // sets the flag itself and has nothing to re-read. + if !reset_restart_policy && !self.work_dir(id)?.started().unwrap_or(false) { + debug!(id, "skipping automatic restart: the VM was stopped"); + return Ok(()); } self.sync_dynamic_config(id)?; let is_running = self @@ -488,16 +545,12 @@ impl App { ) { Ok(processes) => processes, Err(error) => { - let _ = self - .remove_filtered_networks(&vm_config.manifest.id, &runtime_networks) - .await; + self.release_vm_interfaces(&vm_config.manifest.id).await; return Err(error); } }; if let Err(error) = work_dir.set_runtime_networks(&runtime_networks) { - let _ = self - .remove_filtered_networks(&vm_config.manifest.id, &runtime_networks) - .await; + self.release_vm_interfaces(&vm_config.manifest.id).await; return Err(error); } { @@ -507,12 +560,7 @@ impl App { } for process in processes { if let Err(err) = self.supervisor.deploy(&process).await { - if let Err(cleanup_error) = self - .remove_filtered_networks(&vm_config.manifest.id, &runtime_networks) - .await - { - warn!(id, %cleanup_error, "failed to roll back filtered networking"); - } + self.release_vm_interfaces(&vm_config.manifest.id).await; if let Err(clear_err) = work_dir.clear_runtime_networks() { warn!( id, @@ -542,13 +590,25 @@ impl App { } pub async fn stop_vm(&self, id: &str) -> Result<()> { + // Removal stops the VM itself and holds the launch lock while it does, + // so this would otherwise wait hours to do again what is already being + // done. + self.refuse_if_removing(id)?; if let Some(vm) = self.lock().get_mut(id) { vm.state.auto_restart.reset(); } + // Teardown deletes the same interfaces a launch creates, and derives + // their names rather than reading a record, so it must not overlap one. + let _launch = self.launch_lock(id).await; self.set_started(id, false)?; self.stop_vm_process(id).await?; - let networks = self.work_dir(id)?.runtime_networks(); - self.remove_filtered_networks(id, &networks).await?; + // Not fallible: a VM that has been asked to stop is stopped whether or + // not netd could be reached. What is left behind is reclaimed by this + // VM's next launch, which releases before it prepares, or by its + // removal, which will not finish until the release lands. A VM that is + // never started or removed again keeps its interfaces, and its + // directory is still there to say whose they are. + self.release_vm_interfaces(id).await; Ok(()) } @@ -557,16 +617,51 @@ impl App { vm: &VmConfig, networks: &mut [Networking], ) -> Result<()> { - if !networks - .iter() - .any(|network| needs_netd_interface(network, &self.config.cvm)) - { + // Before the early return, because a mapping with nowhere to go is a + // property of the resolved topology and not of whether netd is in it. + // Deployment refuses every way of asking for one, so reaching this + // means an edit removed the NIC out from under a mapping that named it. + for mapping in stranded_ingress(&vm.manifest.port_map, networks) { + warn!( + vm_id = %vm.manifest.id, + "port mapping {} {}:{} names NIC {:?}, which this VM no longer has a backend \ + for; it will not be published", + mapping.protocol.as_str(), + mapping.address, + mapping.from, + mapping.nic_index, + ); + } + // Whatever an earlier boot left behind: a crash between creating an + // interface and recording it, a NIC this VM no longer has, or a whole + // backend it no longer uses. Prepare replaces the names it is about to + // use, but only those, so an index nothing will claim again is only + // reachable from here. + // + // Before the early return, and not inside it. A VM that has moved from + // a bridge to user-mode networking needs this release precisely because + // it no longer wants an interface, and gating it on wanting one is how + // the interfaces it left behind would become unreachable to every + // later launch. + self.release_vm_interfaces(&vm.manifest.id).await; + if !networks.iter().any(needs_netd_interface) { return Ok(()); } let qemu_uid = Uid::effective().as_raw(); + // Only ever read back out of a log line: netd is told where the VM + // lives so an operator holding an opaque TAP name can reach the VM + // without going through the VMM first. + let workdir = self + .work_dir(&vm.manifest.id) + .map(|dir| dir.path().display().to_string()) + .unwrap_or_default(); + // `port_map` is implemented as QEMU `hostfwd=` entries on a user-mode + // netdev, so a bridge NIC drops every one of them. The VMM cannot + // forward them itself -- it runs without CAP_NET_ADMIN by design -- so + // it states the requirement and lets the node's netd answer it. let mut prepared = Vec::new(); for (nic_index, network) in networks.iter_mut().enumerate() { - if !needs_netd_interface(network, &self.config.cvm) { + if !needs_netd_interface(network) { continue; } let identity = InterfaceIdentity { @@ -593,6 +688,7 @@ impl App { // libvirt at all. filtered, queues, + workdir: workdir.clone(), }), NetworkingMode::Macvtap => NetdRequest::PrepareMacvtap(PrepareMacvtapRequest { identity: identity.clone(), @@ -601,20 +697,22 @@ impl App { qemu_uid, mode: network.macvtap_mode.clone(), queues, + workdir: workdir.clone(), }), NetworkingMode::User | NetworkingMode::Custom => continue, }; let response = match netd::request(&self.config.netd.socket, &request).await { Ok(response) => response, Err(error) => { - // The client may have timed out while netd was still finishing - // this Prepare. Remove the in-flight identity first; netd's - // serialized accept loop processes it after Prepare completes. + // The client may have timed out while netd was still + // finishing this Prepare. Remove the in-flight identity + // first: the operation lock makes netd run that removal + // after the Prepare it is undoing, whatever order the two + // connections arrived in. if let Err(cleanup_error) = netd::request( &self.config.netd.socket, &NetdRequest::Remove { identity: identity.clone(), - filtered, }, ) .await @@ -622,16 +720,17 @@ impl App { warn!(%cleanup_error, "failed to roll back in-flight filtered network"); } self.roll_back_prepared_networks(prepared).await; - // netd's own message is about a TAP, not about queues, so - // a caller who asked for multiqueue would not see their - // request named anywhere in the failure. + // netd's own message is about a socket or a TAP, so + // neither the NIC that asked nor what a node has to install + // to satisfy it appears anywhere in the failure. let unreachable = netd::is_unreachable(&error); + let mode = network.nic.mode.as_str(); let error = Err(error).context("failed to prepare netd-managed networking"); - return if queues > 1 && unreachable { + return if unreachable { error.with_context(|| { format!( - "interface {nic_index} asked for {queues} queue pairs, which needs \ - a netd running on this host" + "interface {nic_index} is {mode}, whose host interface only netd \ + can build; run dstack-vmm netd on this host" ) }) } else if queues > 1 { @@ -639,19 +738,11 @@ impl App { format!("interface {nic_index} asked for {queues} queue pairs") }) } else { - error + error.with_context(|| format!("interface {nic_index} is {mode}")) }; } }; - prepared.push((identity.clone(), filtered)); - // netd built this one. Record it now, before anything else can - // fail, so teardown never has to re-derive it from a node - // configuration the operator may since have changed. - network.netd_interface = if filtered { - NetdInterface::Filtered - } else { - NetdInterface::Unfiltered - }; + prepared.push(identity.clone()); // Everything below runs after netd already built a host interface, // so a failure has to unwind the same way a failed Prepare does. let accepted = (|| { @@ -692,45 +783,18 @@ impl App { /// down the snapshot describes a boot that is over: the node configuration /// and the VM's own manifest can both have changed since, so reporting it /// would answer a question about the past with the grammar of the present. - /// Predict instead, the same way the next launch will -- including the - /// drop to a single queue pair on a node with no netd. - /// - /// `netd_reachable` is shared across a request rather than probed here: - /// the probe is a blocking connect that netd's serialized accept loop has - /// to service, and one status query covers many VMs. - fn effective_networks( - &self, - info: &vm_info::VmInfo, - netd_reachable: &OnceCell, - ) -> Vec { + /// Predict instead, the same way the next launch will. + fn effective_networks(&self, info: &vm_info::VmInfo) -> Vec { if info.running && !info.runtime_networks.is_empty() { return info.runtime_networks.clone(); } - let available = *netd_reachable.get_or_init(|| netd_available(&self.config.netd.socket)); - self.merge_networks(&info.manifest, available).0 + self.merge_networks(&info.manifest) } - /// Launch-time view of a VM's NICs: node defaults merged in, the - /// vCPU-scaled queue count made concrete, and multiqueue dropped when this - /// node has no netd to build the interface. + /// Launch-time view of a VM's NICs: node defaults merged in and the + /// vCPU-scaled queue count made concrete. pub(crate) fn runtime_networks(&self, manifest: &Manifest) -> Vec { - let available = netd_available(&self.config.netd.socket); - let (networks, clamped, vhost_denied) = self.merge_networks(manifest, available); - if clamped > 0 { - warn!( - id = %manifest.id, - "netd is not available, so {clamped} bridge interface(s) fall back to a single \ - queue pair; run dstack-vmm netd to let queue pairs scale with vCPUs" - ); - } - if vhost_denied > 0 { - warn!( - id = %manifest.id, - "no qemu-bridge-helper found, so {vhost_denied} bridge interface(s) fall back to \ - the non-vhost bridge netdev; set cvm.qemu_bridge_helper to enable vhost" - ); - } - networks + self.merge_networks(manifest) } /// A running VM whose snapshot is missing, because a VMM that predates the @@ -747,93 +811,80 @@ impl App { /// queue pair and no vhost. Asking `runtime_networks` would apply today's /// defaults to a launch that predates them, and the guess is persisted, so /// it would keep describing that VM wrongly for the life of its boot. - /// - /// `merge_networks` rather than `runtime_networks` for the same reason: the - /// latter probes netd and warns about a multiqueue fallback, which says - /// nothing about a VM that is already up. fn inferred_runtime_networks(&self, manifest: &Manifest) -> Vec { - let mut networks = self.merge_networks(manifest, false).0; + let mut networks = self.merge_networks(manifest); for network in &mut networks { network.nic.vhost = Some(false); network.nic.queues = Some(1); } - for network in &mut networks { - network.netd_interface = match netd_teardown(network, &self.config.cvm) { - Some(true) => NetdInterface::Filtered, - Some(false) => NetdInterface::Unfiltered, - None => NetdInterface::None, - }; - } networks } - /// The merge itself, without the launch-time logging, plus how many NICs - /// lost multiqueue for want of netd. - fn merge_networks( - &self, - manifest: &Manifest, - netd_reachable: bool, - ) -> (Vec, usize, usize) { - let requested = if manifest.networks.is_empty() { - vec![self.config.cvm.networking.nic.clone()] - } else { - manifest.networks.clone() - }; + /// The merge itself: node defaults applied, then the data plane settled so + /// that every later stage reads one answer instead of recomputing it. + fn merge_networks(&self, manifest: &Manifest) -> Vec { let mut resolved = resolved_networks(manifest, &self.config.cvm); - let clamped = - clamp_queues_without_netd(&requested, &mut resolved, &self.config.cvm, netd_reachable); - let vhost_denied = settle_vhost(&mut resolved, &self.config.cvm); - (resolved, clamped, vhost_denied) + settle_vhost(&mut resolved); + resolved } /// Removes interfaces netd already built for a launch that then failed. - async fn roll_back_prepared_networks(&self, prepared: Vec<(InterfaceIdentity, bool)>) { - for (identity, filtered) in prepared.into_iter().rev() { - if let Err(cleanup_error) = netd::request( - &self.config.netd.socket, - &NetdRequest::Remove { identity, filtered }, - ) - .await + async fn roll_back_prepared_networks(&self, prepared: Vec) { + for identity in prepared.into_iter().rev() { + if let Err(cleanup_error) = + netd::request(&self.config.netd.socket, &NetdRequest::Remove { identity }).await { warn!(%cleanup_error, "failed to roll back prepared network interface"); } } } - pub(crate) async fn remove_filtered_networks( - &self, - vm_id: &str, - networks: &[Networking], - ) -> Result<()> { - if networks - .iter() - .all(|network| netd_teardown(network, &self.config.cvm).is_none()) + /// Releases every host interface netd holds for this VM. + /// + /// Unconditional and non-fatal, which is one decision made twice. The + /// release path must not be gated on a predicate that can change under it: + /// `needs_netd_interface` reads the VM's *current* backend, and a VM whose + /// NIC was a bridge when its TAP was built and is a user-mode NIC now would + /// skip the release for interfaces that exist. And a VM must be able to + /// stop when the daemon holding its interfaces cannot be reached, or a + /// netd outage becomes a fleet that cannot be stopped. + /// + /// Returns whether netd is known to hold nothing for this VM any more. + /// A removal reads that to decide whether it may delete the workdir: the + /// directory is what says to try again, and deleting it over a failed + /// release is what strands an interface with nothing left to reach it. + pub(crate) async fn release_vm_interfaces(&self, vm_id: &str) -> bool { + // Ask for the release, rather than asking whether it can be asked for. + // A probe first would put a second round trip in front of every stop + // and -- worse -- would make a *busy* netd look like an absent one and + // skip the release entirely. The operation itself cannot be misread + // that way: it succeeds, or it says netd is not there, or netd answers + // with a refusal, and only the last of those has a fallback. + match netd::remove_all( + &self.config.netd.socket, + &self.config.cvm.instance_id, + vm_id, + ) + .await { - return Ok(()); - } - let mut first_error = None; - for (nic_index, network) in networks.iter().enumerate().rev() { - let Some(filtered) = netd_teardown(network, &self.config.cvm) else { - continue; - }; - let identity = InterfaceIdentity { - instance_id: self.config.cvm.instance_id.clone(), - vm_id: vm_id.to_string(), - nic_index, - }; - if let Err(error) = netd::request( - &self.config.netd.socket, - &NetdRequest::Remove { identity, filtered }, - ) - .await - { - first_error.get_or_insert(error); + Ok(removed) => { + if removed > 0 { + info!(vm_id, removed, "released netd-managed interfaces"); + } + true + } + Err(error) if netd::is_unreachable(&error) => { + debug!(vm_id, %error, "no netd to release interfaces from"); + false + } + Err(error) => { + warn!( + vm_id, + "failed to release netd-managed interfaces: {error:#}" + ); + false } } - if let Some(error) = first_error { - return Err(error).context("failed to remove netd-managed networking"); - } - Ok(()) } pub(crate) async fn stop_vm_process(&self, id: &str) -> Result<()> { @@ -873,12 +924,11 @@ impl App { pub async fn remove_vm(&self, id: &str) -> Result<()> { { let mut state = self.lock(); - let vm = state.get_mut(id).context("VM not found")?; - if vm.state.removing { + state.get(id).context("VM not found")?; + if !state.start_removing(id) { // Already being removed — idempotent return Ok(()); } - vm.state.removing = true; } // Persist the removing marker so crash recovery can resume @@ -904,13 +954,31 @@ impl App { /// /// `delete_workdir`: true for user-initiated removal, false for orphan cleanup. async fn finish_remove_vm(&self, id: &str, delete_workdir: bool) -> Result<()> { + // Every exit from here clears the mark, including the `?`s below and a + // panic in this task, which `tokio::spawn` would otherwise swallow. + let _mark = RemovalMark { + app: self.clone(), + id: id.to_string(), + }; + // Held across the stop, the wait and the release, not just the release. + // `removing` turns launches away, but a launch that passed that check + // before the marker was set is already inside the lock: it has not + // deployed yet, so the wait below sees nothing running and returns at + // once, and the release then deletes the interfaces of the QEMU that + // launch went on to start. Taking the lock first means the launch + // finishes before removal decides anything, and removal then stops what + // it actually started. + let _launch = self.launch_lock(id).await; // Stop the supervisor process (idempotent if already stopped) if let Err(err) = self.stop_vm_process(id).await { debug!("graceful VM stop during removal failed: {err:?}"); } - // Poll until the process is no longer running, then remove it. - // Some VMs take a long time to stop (e.g. 2+ hours), so we wait indefinitely. + // Poll until the process is no longer running, then remove it. The + // stop above is a SIGKILL, so this is however long the kernel takes to + // tear the VM down -- seconds for a large TD, unbounded for one wedged + // in a device reset. Waiting is still right: what follows deletes the + // interfaces and the workdir it is using. let mut poll_count: u64 = 0; loop { match self.supervisor.info(id).await { @@ -942,25 +1010,37 @@ impl App { } } - let runtime_networks = self.work_dir(id)?.runtime_networks(); - if let Err(error) = self.remove_filtered_networks(id, &runtime_networks).await { - warn!(id, %error, "failed to remove filtered networking during VM removal"); - } + let vm_path = self.work_dir(id)?; + // Read before the release, because the release is what makes it stale. + // A VM that never asked netd for an interface -- user mode, a custom + // netdev, or one that never launched -- has nothing for netd to be + // holding, so an absent netd is not a reason to keep its directory. + let held_interfaces = vm_path.runtime_networks().iter().any(needs_netd_interface); + let released = self.release_vm_interfaces(id).await; // Only delete the workdir for user-initiated removal or if .removing marker exists. // Orphaned supervisor processes without the marker keep their data intact. - let vm_path = self.work_dir(id)?; - if delete_workdir || vm_path.is_removing() { + if !(delete_workdir || vm_path.is_removing()) { if vm_path.path().exists() { - if let Err(err) = fs::remove_dir_all(&vm_path) { - error!("failed to remove VM directory for {id}: {err:?}"); - } + info!( + "VM {id} workdir preserved (orphan cleanup): {}", + vm_path.path().display() + ); } - } else if vm_path.path().exists() { - info!( - "VM {id} workdir preserved (orphan cleanup): {}", - vm_path.path().display() + } else if held_interfaces && !released { + // The `.removing` marker and the directory are what a later boot + // reads to retry this, and `remove_all` is idempotent, so keeping + // them costs one retry and losing them strands every interface + // netd still holds: nothing else on the host can name them. + warn!( + "VM {id} keeps its directory because netd did not release its interfaces; \ + the removal resumes at the next VMM start" ); + return Ok(()); + } else if vm_path.path().exists() { + if let Err(err) = fs::remove_dir_all(&vm_path) { + error!("failed to remove VM directory for {id}: {err:?}"); + } } // Free CID and remove from memory (last step) @@ -980,16 +1060,13 @@ impl App { /// Returns false if a cleanup task is already running for this VM. fn spawn_finish_remove(&self, id: &str) -> bool { { - let mut state = self.lock(); - if let Some(vm) = state.get_mut(id) { - if vm.state.removing { - // Already being cleaned up — skip - return false; - } - vm.state.removing = true; + // Claimed in the set rather than in the entry: an orphaned + // supervisor process has no entry, and that is exactly the case + // where the launch lock is held with nothing turning waiters away. + if !self.lock().start_removing(id) { + // Already being cleaned up — skip + return false; } - // If VM is not in memory (e.g. orphaned supervisor process), no entry to guard - // but we still need to clean up the supervisor process. } let app = self.clone(); let id = id.to_string(); @@ -1325,14 +1402,11 @@ impl App { }); let total = infos.len() as u32; - // One probe for the whole page, and none at all when every VM is - // running and has its own snapshot to report. - let netd_reachable = OnceCell::new(); let vms = paginate(infos, request.page, request.page_size) .map(|vm| { let work_dir = self.work_dir(&vm.config.manifest.id)?; let info = vm.merged_info(vms.get(&vm.config.manifest.id), &work_dir); - let networks = self.effective_networks(&info, &netd_reachable); + let networks = self.effective_networks(&info); Ok(info.to_pb(&self.config.gateway, request.brief, &networks)) }) .collect::>>()?; @@ -1357,9 +1431,9 @@ impl App { pub async fn vm_info(&self, id: &str) -> Result> { let proc_state = self.supervisor.info(id).await?; - // Snapshot under the lock, then release it: describing the VM can - // probe netd, and that is a blocking connect the global state lock has - // no business being held across. + // Snapshot under the lock, then release it: the global state lock is + // held by every other VM's operations, and describing one VM has no + // business keeping it across the work that follows. let info = { let state = self.lock(); let Some(vm_state) = state.get(id) else { @@ -1367,8 +1441,7 @@ impl App { }; vm_state.merged_info(proc_state.as_ref(), &self.work_dir(id)?) }; - let netd_reachable = OnceCell::new(); - let networks = self.effective_networks(&info, &netd_reachable); + let networks = self.effective_networks(&info); Ok(Some(info.to_pb(&self.config.gateway, false, &networks))) } @@ -2064,6 +2137,205 @@ mod tests { use super::mr_config::{mr_config_version, MrConfigVersion}; use super::*; + fn test_app() -> App { + use rocket::figment::providers::Format as _; + let config: Config = rocket::figment::Figment::from( + rocket::figment::providers::Toml::string(crate::config::DEFAULT_CONFIG), + ) + .extract() + .unwrap(); + App::new(config, SupervisorClient::new("http://127.0.0.1:0")) + } + + fn test_config(netd_socket: &Path, run_path: &Path) -> Config { + use rocket::figment::providers::Format as _; + let mut config: Config = rocket::figment::Figment::from( + rocket::figment::providers::Toml::string(crate::config::DEFAULT_CONFIG), + ) + .extract() + .unwrap(); + config.netd.socket = netd_socket.to_path_buf(); + config.cvm.instance_id = "test-instance".to_string(); + config.run_path = run_path.to_path_buf(); + config + } + + fn app_talking_to(netd_socket: &Path) -> App { + let run_path = netd_socket.parent().unwrap_or(Path::new("/nonexistent")); + App::new( + test_config(netd_socket, run_path), + SupervisorClient::new("http://127.0.0.1:0"), + ) + } + + /// A netd outage must not become a fleet that cannot be stopped. + #[tokio::test] + async fn a_stop_survives_a_netd_that_is_not_there() { + let app = app_talking_to(Path::new("/nonexistent/dstack-netd.sock")); + // Returns rather than propagating: there is no error type here on + // purpose, because there is no caller that should act on one. + app.release_vm_interfaces("vm-1").await; + } + + /// A removal deletes the workdir, and the workdir is the only thing left + /// that says to retry. So the release has to say whether it landed: an + /// answer means netd holds nothing, and anything else -- a refusal, or a + /// netd that is not there to ask -- means it may still. + #[tokio::test] + async fn a_release_says_whether_netd_still_holds_anything() { + let netd = + netd::testing::FakeNetd::spawn(netd::testing::Behavior::handling(&["remove_all"])); + let app = app_talking_to(netd.socket()); + assert!(app.release_vm_interfaces("vm-1").await); + + // Refused: netd is up and still holding whatever it had. + let refusing = netd::testing::FakeNetd::spawn(netd::testing::Behavior::Legacy); + let app = app_talking_to(refusing.socket()); + assert!(!app.release_vm_interfaces("vm-1").await); + + // Not there to ask. A VM that never asked netd for an interface is + // unaffected -- the removal checks that separately -- but one that did + // must keep its directory so a later start can try again. + let app = app_talking_to(Path::new("/nonexistent/dstack-netd.sock")); + assert!(!app.release_vm_interfaces("vm-1").await); + } + + /// A netd too old for the sweep refuses it, and a refusal is not a reason + /// to fail the stop. What it holds stays until this VM launches again or + /// until an operator names it. The stop still succeeds: a netd outage must + /// not become a fleet that cannot be stopped. + #[tokio::test] + async fn a_netd_that_refuses_the_sweep_does_not_fail_the_stop() { + let netd = netd::testing::FakeNetd::spawn(netd::testing::Behavior::Legacy); + let app = app_talking_to(netd.socket()); + app.release_vm_interfaces("vm-1").await; + assert_eq!( + netd.operations(), + vec!["remove_all"], + "asked once, and not asked about afterwards" + ); + } + + /// One request, no question in front of it. A probe on the hot path is a + /// second round trip whose failure mode is silence: a netd too slow to + /// answer it reads as an absent one, and an absent netd's release is + /// skipped. + #[tokio::test] + async fn a_stop_asks_netd_to_sweep_without_a_question_first() { + let netd = + netd::testing::FakeNetd::spawn(netd::testing::Behavior::handling(&["remove_all"])); + let app = app_talking_to(netd.socket()); + app.release_vm_interfaces("vm-1").await; + + assert_eq!(netd.operations(), vec!["remove_all"]); + let sweep = &netd.seen()[0]; + assert_eq!(sweep["vm_id"], "vm-1"); + assert_eq!(sweep["instance_id"], "test-instance"); + // A sweep names no NIC: reaching the indices the caller can no longer + // name is the entire point. + assert!(sweep.get("nic_index").is_none()); + } + + /// The orphan cleanup runs for an ID that never loaded, so a guard living + /// in the VM entry is not there when it holds the launch lock across the + /// whole teardown. Without the mark, `StartVm` on that ID waits the + /// teardown out with no error and no log. + #[tokio::test] + async fn a_removal_with_no_vm_entry_still_turns_operations_away() { + let app = test_app(); + assert!(app.refuse_if_removing("orphan").is_ok()); + assert!(app.lock().start_removing("orphan")); + assert!( + app.refuse_if_removing("orphan").is_err(), + "an orphan being removed is still a VM being removed" + ); + // And the same removal cannot be started twice. + assert!(!app.lock().start_removing("orphan")); + } + + /// `finish_remove_vm` returns early on more than its happy path. A mark it + /// left behind is not a stale flag: every later operation on that VM, + /// including the removal that would retry, answers "being removed". + #[tokio::test] + async fn a_removal_that_gives_up_early_does_not_leave_the_vm_marked() { + let app = test_app(); + { + let _mark = RemovalMark { + app: app.clone(), + id: "vm-1".to_string(), + }; + assert!(app.lock().start_removing("vm-1")); + assert!(app.refuse_if_removing("vm-1").is_err()); + } + assert!( + app.refuse_if_removing("vm-1").is_ok(), + "the mark is cleared however the removal ends" + ); + } + + /// A restart decided before a stop must not outlive it. The restart task + /// reads the started flag off disk and only then queues a launch, which + /// waits for the lock the stop is holding; without a re-read under that + /// lock the launch resurrects a VM the operator was told was stopped. + #[tokio::test] + async fn an_automatic_restart_does_not_outlive_the_stop_it_raced() { + let dir = tempfile::tempdir().unwrap(); + let app = App::new( + test_config(Path::new("/nonexistent/netd.sock"), dir.path()), + SupervisorClient::new("http://127.0.0.1:0"), + ); + let work_dir = app.work_dir("vm-1").unwrap(); + std::fs::create_dir_all(work_dir.path()).unwrap(); + + work_dir.set_started(false).unwrap(); + app.start_vm_with_restart_policy("vm-1", false) + .await + .expect("an automatic restart of a stopped VM does nothing"); + + // The flag is the whole difference: with it set, the same call goes on + // to do the work, and fails here for want of a VM to launch. + work_dir.set_started(true).unwrap(); + assert!(app + .start_vm_with_restart_policy("vm-1", false) + .await + .is_err()); + + // An explicit start sets the flag itself and has nothing to re-read, + // so it is never turned away by one. + work_dir.set_started(false).unwrap(); + assert!(app.start_vm("vm-1").await.is_err()); + } + + /// The window a launch spends between reading "not running" and actually + /// starting QEMU is long -- a GPU reset, a netd conversation -- and the + /// sweep inside it deletes interfaces by deriving their names. Two entrants + /// in that window meant the loser deleting the winner's live TAPs. + #[tokio::test] + async fn one_vm_launches_at_a_time_and_the_lock_map_stays_small() { + let app = test_app(); + let held = app.launch_lock("vm-1").await; + + // A different VM is never blocked by it: a slow start must not stall + // every other launch on the node. + let other = tokio::time::timeout(Duration::from_millis(50), app.launch_lock("vm-2")).await; + assert!(other.is_ok(), "an unrelated VM must not wait"); + + // The same VM is. + let same = tokio::time::timeout(Duration::from_millis(50), app.launch_lock("vm-1")).await; + assert!(same.is_err(), "a second entrant must wait for the first"); + + drop(held); + drop(other); + tokio::time::timeout(Duration::from_millis(50), app.launch_lock("vm-1")) + .await + .expect("the lock is released"); + + // Nothing is in flight now, so the map holds nothing either. + assert!(app.launch_locks.lock().unwrap().len() <= 1); + let _ = app.launch_lock("vm-3").await; + assert!(app.launch_locks.lock().unwrap().len() <= 2); + } + #[test] fn accepts_server_generated_ids() { validate_vm_id(&uuid::Uuid::new_v4().to_string()).unwrap(); @@ -3158,6 +3430,14 @@ impl VmState { pub(crate) struct AppState { cid_pool: IdPool, vms: HashMap, + /// The VMs a removal is currently working on. + /// + /// Separate from `VmState::removing` because the set has to outlive the + /// entry. Orphan cleanup runs for IDs that never loaded into `vms`, and + /// `finish_remove_vm` holds the launch lock across the whole teardown, so + /// a guard that lives in the entry cannot turn away the operation that + /// would otherwise wait that teardown out. + removing: HashSet, } impl AppState { @@ -3180,6 +3460,38 @@ impl AppState { pub fn iter_vms(&self) -> impl Iterator { self.vms.values() } + + /// Claims `id` for a removal. False when one already has it. + fn start_removing(&mut self, id: &str) -> bool { + if let Some(vm) = self.vms.get_mut(id) { + vm.state.removing = true; + } + self.removing.insert(id.to_string()) + } + + fn is_removing(&self, id: &str) -> bool { + self.removing.contains(id) + } +} + +/// Clears the in-flight removal mark however the removal ends. +/// +/// `finish_remove_vm` returns early on more than its happy path, and a mark +/// left behind is not a stale flag: every operation on that VM answers "being +/// removed" from then on, including the removal that would retry. +struct RemovalMark { + app: App, + id: String, +} + +impl Drop for RemovalMark { + fn drop(&mut self) { + let mut state = self.app.lock(); + state.removing.remove(&self.id); + if let Some(vm) = state.vms.get_mut(&self.id) { + vm.state.removing = false; + } + } } /// Reject VM ids that would escape `run_path` once joined into a filesystem diff --git a/dstack/vmm/src/app/network.rs b/dstack/vmm/src/app/network.rs index 4017e89ff..b946e3ca1 100644 --- a/dstack/vmm/src/app/network.rs +++ b/dstack/vmm/src/app/network.rs @@ -9,10 +9,9 @@ use std::path::Path; use anyhow::{bail, Result}; use sha2::{Digest, Sha256}; -use super::Manifest; +use super::{Manifest, PortMapping}; use crate::config::{ - CvmConfig, NetdInterface, NetworkFilterMode, Networking, NetworkingMode, NicNetworking, - MAX_NET_QUEUES, + CvmConfig, NetworkFilterMode, Networking, NetworkingMode, NicNetworking, MAX_NET_QUEUES, }; /// Node configuration merged with what one NIC pins. @@ -39,7 +38,6 @@ pub(crate) fn resolve_networking( // Runtime state, never inherited from configuration or from a previous // launch. Interface preparation sets both for the NICs it builds, and a // node configuration that names either is rejected at startup. - resolved.netd_interface = crate::config::NetdInterface::None; resolved.device.clear(); if !networking.bridge.is_empty() { resolved.nic.bridge = networking.bridge.clone(); @@ -85,17 +83,22 @@ pub(crate) fn resolved_networks(manifest: &Manifest, cfg: &CvmConfig) -> Vec bool { - match networking.nic.mode { - NetworkingMode::Macvtap => true, - NetworkingMode::Bridge => { - cfg.network_filter.mode == NetworkFilterMode::Libvirt || networking.queue_pairs() > 1 - } - NetworkingMode::User | NetworkingMode::Custom => false, - } +/// Every macvtap and every bridge NIC. The alternative was a set of conditions +/// -- libvirt filtering, multiqueue -- under which netd was consulted and +/// outside of which the VMM built the interface some other way. Each of those +/// paths had to answer the same questions again and answer them differently: +/// which netdev QEMU gets, whether vhost is really on, and, once port mappings +/// grew a NIC, where a bridge NIC's host ports go. A host interface has one +/// owner now, and `port_map` on a bridge reaches netd on every node rather than +/// only on the ones that happened to filter or to have scaled their queues. +/// +/// The cost is stated plainly: bridge and macvtap need a netd on the host. User +/// mode and a caller-supplied netdev still need nothing. +pub(crate) fn needs_netd_interface(networking: &Networking) -> bool { + matches!( + networking.nic.mode, + NetworkingMode::Macvtap | NetworkingMode::Bridge + ) } /// Whether this NIC's host interface carries a libvirt nwfilter binding. @@ -105,146 +108,16 @@ pub(crate) fn filters_bridge_traffic(networking: &Networking, cfg: &CvmConfig) - && cfg.network_filter.mode == NetworkFilterMode::Libvirt } -/// Whether netd built this NIC's host interface, and if so whether it carries -/// an nwfilter binding. -/// -/// Interface preparation records this, because it is not derivable afterwards: -/// an operator can change `network_filter.mode` or `max_net_queues` while a VM -/// runs, and teardown has to undo what was built rather than what would be -/// built now. -pub(crate) fn netd_teardown(networking: &Networking, cfg: &CvmConfig) -> Option { - match networking.netd_interface { - NetdInterface::Filtered => Some(true), - NetdInterface::Unfiltered => Some(false), - // Either nothing was built, or this entry was persisted before - // preparation recorded the fact. Fall back to the derivation such an - // entry was created by; a Remove for an interface that does not exist - // is a no-op. - NetdInterface::None if needs_netd_interface(networking, cfg) => { - Some(filters_bridge_traffic(networking, cfg)) - } - NetdInterface::None => None, - } -} - -/// Drops a NIC back to one queue pair when multiqueue would need a netd -/// interface this node cannot provide. -/// -/// Queue pairs are a default now, not something the operator asked for, so a -/// node that has never deployed netd must keep launching bridge VMs. An -/// explicit per-VM request is left alone: the caller asked for it, and failing -/// at prepare tells them why far better than silently halving their throughput. -/// Returns how many NICs it dropped, so a launch can say so and a status -/// query, which runs the same calculation to describe a stopped VM, stays -/// silent. -pub(crate) fn clamp_queues_without_netd( - requested: &[NicNetworking], - resolved: &mut [Networking], - cfg: &CvmConfig, - netd_available: bool, -) -> usize { - if netd_available { - return 0; - } - let mut clamped = 0; - for (networking, asked) in resolved.iter_mut().zip(requested) { - // Macvtap has nothing to fall back to: netd is the only thing that can - // create the device, so clamping one would describe a VM that cannot - // start either way. - if networking.nic.mode != NetworkingMode::Bridge - || asked.queues.is_some() - || !needs_netd_interface(networking, cfg) - // Filtering needs netd whatever the queue count, so dropping this - // NIC to one queue pair would not make it launchable. It would only - // describe it as something no launch can produce, and warn about a - // fallback that is not happening. - || filters_bridge_traffic(networking, cfg) - { - continue; - } - networking.nic.queues = Some(1); - clamped += 1; - } - clamped -} - -/// Locations distributions install `qemu-bridge-helper` in. The helper is -/// setuid root and attaches an unprivileged TAP to a whitelisted bridge, which -/// is how bridge mode avoids giving the VMM `CAP_NET_ADMIN`. -const BRIDGE_HELPER_CANDIDATES: [&str; 3] = [ - "/usr/lib/qemu/qemu-bridge-helper", - "/usr/libexec/qemu-bridge-helper", - "/usr/local/libexec/qemu-bridge-helper", -]; - -/// Absolute path of `qemu-bridge-helper`, which QEMU's `tap` netdev, unlike its -/// `bridge` netdev, has no compiled-in default for. -/// -/// A configured path is passed through unchecked: the operator is naming a -/// binary for QEMU to exec, and QEMU need not see this filesystem. -pub(crate) fn find_bridge_helper<'a>( - configured: &'a str, - candidates: &[&'a str], -) -> Option<&'a str> { - let configured = configured.trim(); - if !configured.is_empty() { - return Some(configured); - } - candidates - .iter() - .copied() - .find(|candidate| Path::new(candidate).exists()) -} - -pub(crate) fn bridge_helper(cfg: &CvmConfig) -> Option<&str> { - find_bridge_helper(&cfg.qemu_bridge_helper, &BRIDGE_HELPER_CANDIDATES) -} - -/// Whether this NIC will actually run on the vhost-net data plane. -/// -/// A bridge NIC that neither needs a netd interface nor can find -/// `qemu-bridge-helper` falls back to QEMU's `bridge` netdev, which has no -/// vhost support. Both the QEMU arguments and the reported status read this, -/// so a VM is never described as using a data plane it did not get. -pub(crate) fn effective_vhost(networking: &Networking, cfg: &CvmConfig) -> bool { - if !networking.vhost_enabled() { - return false; - } - networking.nic.mode != NetworkingMode::Bridge - || needs_netd_interface(networking, cfg) - || bridge_helper(cfg).is_some() -} - -/// Makes the effective data plane concrete on a launch-time NIC list, and -/// returns how many interfaces asked for vhost and did not get it. +/// Makes the data plane concrete on a launch-time NIC list. /// /// `vhost` on a freshly resolved entry is still a *request*: `None` means -/// inherit, and a bridge NIC that cannot reach `qemu-bridge-helper` runs on the -/// non-vhost netdev whatever it asked for. Settling it once, here, is what lets -/// the QEMU arguments and the reported status read the same value -- and keeps -/// them reading it after the operator moves the helper out from under a VM that -/// is already running. -pub(crate) fn settle_vhost(networks: &mut [Networking], cfg: &CvmConfig) -> usize { - let mut denied = 0; +/// inherit from the node, which can change under a VM that is already running. +/// Settling it once, here, is what lets the QEMU arguments and the reported +/// status read the same value for the life of a boot. +pub(crate) fn settle_vhost(networks: &mut [Networking]) { for networking in networks.iter_mut() { - let effective = effective_vhost(networking, cfg); - if networking.vhost_enabled() && !effective { - denied += 1; - } - networking.nic.vhost = Some(effective); + networking.nic.vhost = Some(networking.vhost_enabled()); } - denied -} - -/// Whether netd is reachable. A netd that died leaves its socket behind, so -/// existence alone would report a node as capable and fail every launch. -/// -/// A connect and nothing more, deliberately: netd serves connections serially, -/// so anything that waits for an answer reads a *busy* netd as a missing one -/// and silently drops the VM to a single queue pair. Accepting the connection -/// is the one signal that does not depend on what netd is doing right now. -pub(crate) fn netd_available(socket: &Path) -> bool { - std::os::unix::net::UnixStream::connect(socket).is_ok() } pub(crate) fn validate_resolved_network(networking: &Networking) -> Result<()> { @@ -329,6 +202,57 @@ pub(crate) fn warn_if_vhost_net_missing(networks: &[Networking]) { } } +/// Which NIC an unpinned port mapping's traffic enters through. +/// +/// The first user-mode NIC, which is where QEMU's `hostfwd=` entries have +/// always gone. There is no second choice: nothing else on this host publishes +/// a port, so a VM without one has nowhere to put a mapping and the launch +/// says so. +pub(crate) fn default_ingress_nic(networks: &[Networking]) -> Option { + networks + .iter() + .position(|network| network.nic.mode == NetworkingMode::User) +} + +/// Whether a NIC of this mode has a mechanism to publish a host port at all. +/// +/// QEMU's `hostfwd=`, and nothing else. A bridge TAP is built by netd, and the +/// netd in this repository does not forward host ports; macvtap bypasses the +/// host bridge; a custom netdev is a string the VMM does not interpret. +pub(crate) fn mode_carries_ingress(mode: NetworkingMode) -> bool { + matches!(mode, NetworkingMode::User) +} + +/// Which NIC a port mapping's traffic enters through. +/// +/// One mapping resolves to at most one NIC, and only a user-mode NIC has a +/// mechanism to carry it, so a pin to any other kind resolves to nothing +/// rather than to a NIC with no path into the guest. +/// +/// `None` is a mapping with nowhere to go: a VM with no user-mode NIC, or one +/// whose NICs changed under a mapping that named one. The launch warns about +/// each of those rather than dropping it in silence. +pub(crate) fn ingress_nic(mapping: &PortMapping, networks: &[Networking]) -> Option { + mapping + .nic_index + .or_else(|| default_ingress_nic(networks)) + .filter(|index| { + networks + .get(*index) + .is_some_and(|network| mode_carries_ingress(network.nic.mode)) + }) +} + +/// Names the mappings that resolve to no NIC, for a launch to warn about. +pub(crate) fn stranded_ingress<'a>( + port_map: &'a [PortMapping], + networks: &'a [Networking], +) -> impl Iterator { + port_map + .iter() + .filter(|mapping| ingress_nic(mapping, networks).is_none()) +} + /// Derives a deterministic, locally administered unicast MAC address. /// /// Index zero preserves the legacy single-NIC derivation. Later interfaces @@ -356,10 +280,11 @@ pub(crate) fn mac_address_for_vm_index(vm_id: &str, prefix: &[u8], index: usize) #[cfg(test)] mod tests { use super::{ - clamp_queues_without_netd, effective_vhost, mac_address_for_vm_index, needs_netd_interface, - netd_teardown, resolve_networking, resolved_networks, settle_vhost, - validate_resolved_networks, + default_ingress_nic, ingress_nic, mac_address_for_vm_index, needs_netd_interface, + resolved_networks, settle_vhost, stranded_ingress, validate_resolved_networks, }; + use crate::app::PortMapping; + use crate::config::Protocol; use crate::config::{Networking, NetworkingMode, NicNetworking}; fn macvtap_network() -> NicNetworking { @@ -450,7 +375,6 @@ mod tests { let resolved = resolved_networks(&manifest_with(16, vec![]), &cvm); assert!(!resolved[0].vhost_enabled()); assert_eq!(resolved[0].queue_pairs(), 1); - assert!(!needs_netd_interface(&resolved[0], &cvm)); } } @@ -461,7 +385,6 @@ mod tests { let resolved = resolved_networks(&manifest_with(8, vec![]), &cvm); assert!(!resolved[0].vhost_enabled()); assert_eq!(resolved[0].queue_pairs(), 1); - assert!(!needs_netd_interface(&resolved[0], &cvm)); // Per-VM opt-out does the same thing. let cvm = node_config(NetworkingMode::Bridge); @@ -492,31 +415,37 @@ mod tests { assert_eq!(resolved[0].queue_pairs(), 16); } + /// One owner for a host interface, with no condition attached. What used + /// to decide this -- libvirt filtering, a scaled queue count -- decided it + /// per node, so the same VM definition got a netd-built TAP on one host and + /// a `qemu-bridge-helper` TAP on the next. #[test] - fn status_never_claims_a_data_plane_the_nic_did_not_get() { + fn every_bridge_and_macvtap_nic_is_netds_to_build() { let mut cvm = node_config(NetworkingMode::Bridge); - // No helper on this filesystem and no netd interface needed, so the - // NIC falls back to QEMU's `bridge` netdev, which has no vhost. - cvm.qemu_bridge_helper = String::new(); let mut single = cvm.networking.nic.clone(); single.queues = Some(1); + let resolved = resolved_networks(&manifest_with(8, vec![single.clone()]), &cvm); + assert!(needs_netd_interface(&resolved[0])); + + // Unfiltered, single queue, no vhost -- the shape that used to need no + // netd at all -- is netd's too. + cvm.networking.nic.vhost = Some(false); + single.vhost = Some(false); let resolved = resolved_networks(&manifest_with(8, vec![single]), &cvm); - assert!(resolved[0].vhost_enabled()); - let fell_back = !effective_vhost(&resolved[0], &cvm); - assert_eq!(fell_back, super::bridge_helper(&cvm).is_none()); + assert!(!resolved[0].vhost_enabled()); + assert_eq!(resolved[0].queue_pairs(), 1); + assert!(needs_netd_interface(&resolved[0])); - // A configured helper is taken at its word, so vhost is real. - cvm.qemu_bridge_helper = "/opt/qemu-bridge-helper".into(); + let cvm = node_config(NetworkingMode::Macvtap); let resolved = resolved_networks(&manifest_with(8, vec![]), &cvm); - assert!(effective_vhost(&resolved[0], &cvm)); + assert!(needs_netd_interface(&resolved[0])); - // Multiqueue goes through netd, which needs no helper at all. - let mut mq = cvm.networking.nic.clone(); - mq.queues = Some(4); - cvm.qemu_bridge_helper = String::new(); - let resolved = resolved_networks(&manifest_with(8, vec![mq]), &cvm); - assert!(needs_netd_interface(&resolved[0], &cvm)); - assert!(effective_vhost(&resolved[0], &cvm)); + // The two backends the VMM builds itself still need nothing. + for mode in [NetworkingMode::User, NetworkingMode::Custom] { + let cvm = node_config(mode); + let resolved = resolved_networks(&manifest_with(8, vec![]), &cvm); + assert!(!needs_netd_interface(&resolved[0])); + } } #[test] @@ -536,35 +465,6 @@ mod tests { assert_eq!(resolved[0].queue_pairs(), 1); } - #[test] - fn without_netd_a_defaulted_bridge_drops_to_one_queue_but_a_request_does_not() { - let cvm = node_config(NetworkingMode::Bridge); - - // The default is ours to lower: a node that never deployed netd must - // keep launching bridge VMs. - let requested = vec![cvm.networking.nic.clone()]; - let mut resolved = resolved_networks(&manifest_with(8, vec![]), &cvm); - assert_eq!(resolved[0].queue_pairs(), 8); - clamp_queues_without_netd(&requested, &mut resolved, &cvm, false); - assert_eq!(resolved[0].queue_pairs(), 1); - assert!(!needs_netd_interface(&resolved[0], &cvm)); - - // An explicit request is left alone, so prepare fails where the caller - // can see why instead of silently halving their throughput. - let mut asked = cvm.networking.nic.clone(); - asked.queues = Some(4); - let requested = vec![asked.clone()]; - let mut resolved = resolved_networks(&manifest_with(8, vec![asked]), &cvm); - clamp_queues_without_netd(&requested, &mut resolved, &cvm, false); - assert_eq!(resolved[0].queue_pairs(), 4); - - // With netd present nothing is touched. - let requested = vec![cvm.networking.nic.clone()]; - let mut resolved = resolved_networks(&manifest_with(8, vec![]), &cvm); - clamp_queues_without_netd(&requested, &mut resolved, &cvm, true); - assert_eq!(resolved[0].queue_pairs(), 8); - } - #[test] fn validation_never_depends_on_this_process_reaching_vhost_net() { // QEMU may run under different credentials, so a NIC that asks for @@ -593,133 +493,113 @@ mod tests { #[test] fn settling_vhost_records_what_the_launch_decided() { let mut cvm = node_config(NetworkingMode::Bridge); - cvm.qemu_bridge_helper = String::new(); - let mut single = cvm.networking.nic.clone(); - single.queues = Some(1); - let manifest = manifest_with(8, vec![single]); - - // Whether this host has a helper is not the test's business; that it - // gets written down, once, is. - let helper_missing = super::bridge_helper(&cvm).is_none(); + let manifest = manifest_with(8, vec![]); let mut networks = resolved_networks(&manifest, &cvm); - assert!(networks[0].vhost_enabled(), "the request starts out on"); - assert_eq!( - settle_vhost(&mut networks, &cvm), - usize::from(helper_missing) - ); - assert_eq!(networks[0].nic.vhost, Some(!helper_missing)); - // Settling an already-settled list reports nothing new, so a relaunch - // does not warn about a fallback that already happened. - assert_eq!(settle_vhost(&mut networks, &cvm), 0); + assert_eq!(networks[0].nic.vhost, Some(true)); + settle_vhost(&mut networks); + assert_eq!(networks[0].nic.vhost, Some(true)); - // A configured helper is taken at its word, so the same NIC settles on. - cvm.qemu_bridge_helper = "/opt/qemu-bridge-helper".into(); - let mut with_helper = resolved_networks(&manifest, &cvm); - assert_eq!(settle_vhost(&mut with_helper, &cvm), 0); - assert_eq!(with_helper[0].nic.vhost, Some(true)); + // The node turns vhost off under a VM that is already running. The + // entry the launch settled keeps its answer; the next boot gets the + // new one. + cvm.networking.nic.vhost = Some(false); + assert_eq!(networks[0].nic.vhost, Some(true)); + let mut next_boot = resolved_networks(&manifest, &cvm); + settle_vhost(&mut next_boot); + assert_eq!(next_boot[0].nic.vhost, Some(false)); - // The entry the first launch settled keeps its answer: nothing about a - // running VM is recomputed from the configuration as it stands now. - assert_eq!(networks[0].nic.vhost, Some(!helper_missing)); + // An inherited `None` becomes a decision rather than staying a request. + let mut unset = resolved_networks(&manifest, &cvm); + unset[0].nic.vhost = None; + settle_vhost(&mut unset); + assert_eq!(unset[0].nic.vhost, Some(false)); } - /// Dropping to a single queue pair is only worth doing when it makes the - /// NIC launchable. A filtered bridge needs netd whatever its queue count, - /// so clamping it would report a shape no launch can produce. #[test] - fn a_filtered_bridge_is_not_clamped_because_it_cannot_help() { - use crate::config::NetworkFilterMode; - - let mut cvm = node_config(NetworkingMode::Bridge); - cvm.network_filter.mode = NetworkFilterMode::Libvirt; - let requested = vec![cvm.networking.nic.clone()]; - let mut resolved = resolved_networks(&manifest_with(8, vec![]), &cvm); - assert_eq!(resolved[0].queue_pairs(), 8); + fn primary_mac_keeps_legacy_derivation_and_later_nics_are_distinct() { assert_eq!( - clamp_queues_without_netd(&requested, &mut resolved, &cvm, false), - 0 + mac_address_for_vm_index("vm-123", &[], 0), + "96:b1:d8:b9:08:e6" ); - assert_eq!(resolved[0].queue_pairs(), 8); - - // Unfiltered, the same NIC does drop, because then it can launch. - let cvm = node_config(NetworkingMode::Bridge); - let mut resolved = resolved_networks(&manifest_with(8, vec![]), &cvm); assert_eq!( - clamp_queues_without_netd(&requested, &mut resolved, &cvm, false), - 1 + mac_address_for_vm_index("vm-123", &[], 1), + "c6:74:2c:65:14:b9" ); - assert_eq!(resolved[0].queue_pairs(), 1); } - /// Teardown has to undo what was built. Node configuration is mutable and - /// a VM outlives an edit to it, so re-deriving "did netd build this?" at - /// removal time orphans TAPs and leaks nwfilter bindings whose ebtables - /// rules the next VM at the same deterministic interface name inherits. - #[test] - fn teardown_follows_what_was_built_not_what_configuration_now_says() { - use crate::config::{NetdInterface, NetworkFilterMode}; + fn nic(mode: NetworkingMode) -> Networking { + Networking { + nic: NicNetworking { + mode, + ..NicNetworking::default() + }, + ..Networking::default() + } + } + + fn mapping(host_port: u16, nic_index: Option) -> PortMapping { + PortMapping { + address: "0.0.0.0".parse().unwrap(), + protocol: Protocol::Tcp, + from: host_port, + to: host_port, + nic_index, + } + } - let filtering = { - let mut cvm = node_config(NetworkingMode::Bridge); - cvm.network_filter.mode = NetworkFilterMode::Libvirt; - cvm - }; - let unfiltered = node_config(NetworkingMode::Bridge); - - let mut built_filtered = filtering.networking.clone(); - built_filtered.nic.queues = Some(1); - built_filtered.netd_interface = NetdInterface::Filtered; - // The operator turns filtering off while the VM runs. The binding is - // still there and still has to be deleted. - assert_eq!(netd_teardown(&built_filtered, &unfiltered), Some(true)); - - let mut built_unfiltered = unfiltered.networking.clone(); - built_unfiltered.nic.queues = Some(4); - built_unfiltered.netd_interface = NetdInterface::Unfiltered; - // The operator turns filtering on. There is no binding to delete, and - // asking libvirt for one would fail the removal. - assert_eq!(netd_teardown(&built_unfiltered, &filtering), Some(false)); - - // A NIC netd never touched stays untouched, whatever the node now says. - let mut untouched = unfiltered.networking.clone(); - untouched.nic.queues = Some(1); - assert_eq!(netd_teardown(&untouched, &unfiltered), None); - - // An entry persisted before preparation recorded the fact still gets - // torn down by the rule that created it. - let mut legacy = filtering.networking.clone(); - legacy.nic.queues = Some(1); - assert_eq!(legacy.netd_interface, NetdInterface::None); - assert_eq!(netd_teardown(&legacy, &filtering), Some(true)); - } - - /// Resolution produces launch input, never a claim about what exists. #[test] - fn resolution_never_carries_a_stale_interface_record() { - use crate::config::NetdInterface; + fn an_unpinned_mapping_still_lands_where_hostfwd_always_put_it() { + // Existing VMs must not move. QEMU's `hostfwd=` has always gone to the + // first user-mode NIC, so that stays the answer wherever there is one. + let networks = [nic(NetworkingMode::Bridge), nic(NetworkingMode::User)]; + assert_eq!(default_ingress_nic(&networks), Some(1)); + assert_eq!(ingress_nic(&mapping(443, None), &networks), Some(1)); - let cvm = node_config(NetworkingMode::Bridge); - // Single queue and no filtering, so nothing but a stale record could - // make teardown believe netd built something. - let mut previous = cvm.networking.clone(); - previous.nic.queues = Some(1); - previous.netd_interface = NetdInterface::Filtered; - assert_eq!(netd_teardown(&previous, &cvm), Some(true)); + // With no user-mode NIC there is nowhere at all. netd builds a bridge + // TAP but does not forward host ports, and macvtap and custom have no + // path either. + let networks = [nic(NetworkingMode::Bridge), nic(NetworkingMode::Bridge)]; + assert_eq!(default_ingress_nic(&networks), None); - let resolved = resolve_networking(&previous.nic, &cvm, 4); - assert_eq!(resolved.netd_interface, NetdInterface::None); - assert_eq!(netd_teardown(&resolved, &cvm), None); + let networks = [nic(NetworkingMode::Macvtap), nic(NetworkingMode::Custom)]; + assert_eq!(default_ingress_nic(&networks), None); + assert_eq!(ingress_nic(&mapping(443, None), &networks), None); } #[test] - fn primary_mac_keeps_legacy_derivation_and_later_nics_are_distinct() { - assert_eq!( - mac_address_for_vm_index("vm-123", &[], 0), - "96:b1:d8:b9:08:e6" - ); - assert_eq!( - mac_address_for_vm_index("vm-123", &[], 1), - "c6:74:2c:65:14:b9" - ); + fn a_pinned_mapping_goes_where_it_says() { + let networks = [nic(NetworkingMode::Bridge), nic(NetworkingMode::User)]; + assert_eq!(ingress_nic(&mapping(443, Some(1)), &networks), Some(1)); + // Out of range resolves to nothing rather than to something arbitrary. + // Deployment refuses it outright; a manifest that lost a NIC lands here. + assert_eq!(ingress_nic(&mapping(443, Some(7)), &networks), None); + } + + /// A pin has to be checked against the backend, not just the count. + /// Naming a macvtap or custom NIC used to resolve to that index and then + /// fall out of every branch that could act on it: no `hostfwd=`, no netd + /// request, and no warning either. + #[test] + fn a_pin_to_a_backend_with_no_ingress_resolves_to_nothing() { + let networks = [ + nic(NetworkingMode::Macvtap), + nic(NetworkingMode::Custom), + nic(NetworkingMode::Bridge), + nic(NetworkingMode::User), + ]; + assert_eq!(ingress_nic(&mapping(443, Some(0)), &networks), None); + assert_eq!(ingress_nic(&mapping(443, Some(1)), &networks), None); + // A bridge TAP is netd's, and netd does not forward host ports. + assert_eq!(ingress_nic(&mapping(443, Some(2)), &networks), None); + assert_eq!(ingress_nic(&mapping(443, Some(3)), &networks), Some(3)); + + // And an unpinned mapping on a VM with nowhere to put it is named, + // rather than counted as delivered. + let networks = [nic(NetworkingMode::Macvtap)]; + let port_map = [mapping(443, None), mapping(8080, Some(0))]; + let stranded: Vec<_> = stranded_ingress(&port_map, &networks) + .map(|mapping| mapping.from) + .collect(); + assert_eq!(stranded, vec![443, 8080]); } } diff --git a/dstack/vmm/src/app/qemu.rs b/dstack/vmm/src/app/qemu.rs index 94a6348fe..fe420a484 100644 --- a/dstack/vmm/src/app/qemu.rs +++ b/dstack/vmm/src/app/qemu.rs @@ -10,7 +10,7 @@ use super::{ image::Image, mr_config::{snp_host_data, tdx_mr_config_id}, network::{ - bridge_helper, mac_address_for_vm_index, needs_netd_interface, validate_resolved_networks, + ingress_nic, mac_address_for_vm_index, validate_resolved_networks, warn_if_vhost_net_missing, }, pci_numa_node, round_up, GpuConfig, VmWorkDir, @@ -631,11 +631,6 @@ impl QemuCommandBuilder<'_> { fn configure_networking(&self, command: &mut Command) -> Result<()> { let macvtap_fds = macvtap_fd_layout(&self.prepared.networks); - let hostfwd_index = self - .prepared - .networks - .iter() - .position(|networking| networking.nic.mode == NetworkingMode::User); for (index, networking) in self.prepared.networks.iter().enumerate() { let net_id = format!("net{index}"); let mac = mac_address_for_vm_index( @@ -663,16 +658,21 @@ impl QemuCommandBuilder<'_> { networking.dhcp_start, if networking.restrict { "yes" } else { "no" } ); - if hostfwd_index == Some(index) { - for mapping in &self.vm.manifest.port_map { - netdev.push_str(&format!( - ",hostfwd={}:{}:{}-:{}", - mapping.protocol.as_str(), - mapping.address, - mapping.from, - mapping.to - )); + // Only the mappings that resolve to this NIC. A mapping + // lands on exactly one, and that NIC's backend decides the + // mechanism, so a bridge NIC's ports go to netd instead of + // being claimed here as well. + for mapping in &self.vm.manifest.port_map { + if ingress_nic(mapping, &self.prepared.networks) != Some(index) { + continue; } + netdev.push_str(&format!( + ",hostfwd={}:{}:{}-:{}", + mapping.protocol.as_str(), + mapping.address, + mapping.from, + mapping.to + )); } netdev } @@ -681,43 +681,25 @@ impl QemuCommandBuilder<'_> { "bridge networking: mac={mac} bridge={} vhost={vhost} queues={queues}", networking.nic.bridge ); - if needs_netd_interface(networking, self.cfg) { - // netd owns this TAP: libvirt filtering binds an - // nwfilter to it, and multiqueue needs the persistent - // IFF_MULTI_QUEUE device the bridge helper cannot make. - let tap = tap_name(&InterfaceIdentity { - instance_id: self.cfg.instance_id.clone(), - vm_id: self.vm.manifest.id.clone(), - nic_index: index, - }); - let mut netdev = format!( - "tap,id={net_id},ifname={tap},script=no,downscript=no,vhost={}", - on_off(vhost) - ); - if queues > 1 { - netdev.push_str(&format!(",queues={queues}")); - } - netdev - } else if let Some(helper) = vhost.then(|| bridge_helper(self.cfg)).flatten() { - // QEMU's `bridge` netdev has no vhost support, but the - // same setuid helper works behind a `tap` netdev, so - // the VMM still needs no network privileges. - format!( - "tap,id={net_id},br={},helper={helper},vhost=on", - networking.nic.bridge - ) - } else if vhost { - // vhost is a node-wide setting, so a node whose helper - // sits somewhere unusual must keep booting VMs rather - // than lose every bridge NIC to a path lookup. - tracing::warn!( - "{net_id}: no qemu-bridge-helper found, falling back to the \ - non-vhost bridge netdev. set cvm.qemu_bridge_helper to enable vhost" - ); - format!("bridge,id={net_id},br={}", networking.nic.bridge) - } else { - format!("bridge,id={net_id},br={}", networking.nic.bridge) + // netd owns the TAP. It is the one component here with + // CAP_NET_ADMIN, so it is the only one that can bind an + // nwfilter or create a persistent IFF_MULTI_QUEUE device -- + // and having it own every bridge TAP is what keeps a VM's + // networking from depending on which of those a node + // happens to use. + let tap = tap_name(&InterfaceIdentity { + instance_id: self.cfg.instance_id.clone(), + vm_id: self.vm.manifest.id.clone(), + nic_index: index, + }); + let mut netdev = format!( + "tap,id={net_id},ifname={tap},script=no,downscript=no,vhost={}", + on_off(vhost) + ); + if queues > 1 { + netdev.push_str(&format!(",queues={queues}")); } + netdev } NetworkingMode::Custom => { if !networking.netdev.contains(&format!("id={net_id}")) { @@ -1192,6 +1174,7 @@ mod tests { protocol: Protocol::Tcp, from: 18080, to: 8080, + nic_index: None, }], created_at_ms: 0, hugepages: false, @@ -1290,16 +1273,23 @@ mod tests { } #[test] - fn bridge_vhost_uses_the_bridge_helper_behind_a_tap_netdev() { - // QEMU's `bridge` netdev has no vhost support at all, so enabling the - // kernel data plane has to switch netdev types while keeping the same - // unprivileged setuid helper. + fn every_bridge_nic_gets_the_netd_tap() { + // Not only the filtered or multiqueue ones. A bridge NIC's host + // interface has one owner, so the netdev QEMU is handed does not + // change with the node's filter mode or its queue count. let (mut config, ..) = test_launch_fixture(); - config.cvm.qemu_bridge_helper = "/usr/lib/qemu/qemu-bridge-helper".into(); - let args = net_args(&config, vec![bridge_network(&config)]); - assert!(args.contains( - &"tap,id=net0,br=br0,helper=/usr/lib/qemu/qemu-bridge-helper,vhost=on".to_string() - )); + config.cvm.instance_id = "vmm-a".into(); + let mut networking = bridge_network(&config); + networking.nic.queues = Some(1); + let args = net_args(&config, vec![networking]); + let tap = tap_name(&InterfaceIdentity { + instance_id: "vmm-a".into(), + vm_id: "vm-1".into(), + nic_index: 0, + }); + assert!(args.contains(&format!( + "tap,id=net0,ifname={tap},script=no,downscript=no,vhost=on" + ))); // A single queue pair must keep the historical device line byte for byte. assert!(args.iter().any( |arg| arg.starts_with("virtio-net-pci,netdev=net0,mac=") && !arg.contains("mq=on") @@ -1307,26 +1297,20 @@ mod tests { } #[test] - fn a_missing_bridge_helper_is_reported_rather_than_guessed() { - // Configured paths are trusted verbatim: QEMU execs them, and it need - // not share this filesystem. - assert_eq!( - crate::app::network::find_bridge_helper(" /opt/qemu-bridge-helper ", &[]), - Some("/opt/qemu-bridge-helper") - ); - assert_eq!( - crate::app::network::find_bridge_helper("", &["/nonexistent/a", "/nonexistent/b"]), - None - ); - } - - #[test] - fn disabling_vhost_restores_the_legacy_bridge_netdev() { - let (config, ..) = test_launch_fixture(); + fn disabling_vhost_keeps_the_netd_tap_and_turns_the_data_plane_off() { + let (mut config, ..) = test_launch_fixture(); + config.cvm.instance_id = "vmm-a".into(); let mut networking = bridge_network(&config); networking.nic.vhost = Some(false); let args = net_args(&config, vec![networking]); - assert!(args.contains(&"bridge,id=net0,br=br0".to_string())); + let tap = tap_name(&InterfaceIdentity { + instance_id: "vmm-a".into(), + vm_id: "vm-1".into(), + nic_index: 0, + }); + assert!(args.contains(&format!( + "tap,id=net0,ifname={tap},script=no,downscript=no,vhost=off" + ))); } #[test] @@ -1490,19 +1474,6 @@ mod tests { network.nic.bridge = "br0".into(); network.nic.vhost = Some(false); } - let process = QemuCommandBuilder { - vm: &vm, - cfg: &config.cvm, - gpus: &GpuConfig::default(), - prepared: &prepared, - } - .build() - .unwrap(); - assert!(process - .args - .iter() - .any(|arg| arg == "bridge,id=net0,br=br0")); - config.cvm.instance_id = "vmm-a".into(); config.cvm.network_filter.mode = NetworkFilterMode::Libvirt; let process = QemuCommandBuilder { diff --git a/dstack/vmm/src/app/vm_info.rs b/dstack/vmm/src/app/vm_info.rs index 19d72118a..48af6f864 100644 --- a/dstack/vmm/src/app/vm_info.rs +++ b/dstack/vmm/src/app/vm_info.rs @@ -33,15 +33,6 @@ pub(crate) struct VmInfo { pub runtime_networks: Vec, } -fn networking_mode_name(mode: NetworkingMode) -> &'static str { - match mode { - NetworkingMode::Bridge => "bridge", - NetworkingMode::User => "user", - NetworkingMode::Custom => "custom", - NetworkingMode::Macvtap => "macvtap", - } -} - fn networking_backend_name(mode: NetworkingMode) -> &'static str { match mode { NetworkingMode::Bridge => "tap_bridge", @@ -62,7 +53,7 @@ fn interfaces_to_proto( .map(|(index, networking)| { let mac = mac_address_for_vm_index(vm_id, &networking.mac_prefix_bytes(), index); pb::NetworkInterfaceStatus { - mode: networking_mode_name(networking.nic.mode).into(), + mode: networking.nic.mode.as_str().into(), backend: networking_backend_name(networking.nic.mode).into(), mac, bridge_name: (networking.nic.mode == NetworkingMode::Bridge) @@ -106,7 +97,7 @@ pub(crate) fn networking_to_proto(networking: &NicNetworking) -> pb::NetworkingC mode: if networking.inherit_mode { String::new() } else { - networking_mode_name(networking.mode).into() + networking.mode.as_str().into() }, bridge_name: if pins_backend && networking.mode == NetworkingMode::Bridge { networking.bridge.clone() @@ -207,6 +198,7 @@ impl VmInfo { .port_map .iter() .map(|mapping| pb::PortMapping { + nic_index: mapping.nic_index.map(|index| index as u32), protocol: mapping.protocol.as_str().into(), host_address: mapping.address.to_string(), host_port: mapping.from as u32, diff --git a/dstack/vmm/src/config.rs b/dstack/vmm/src/config.rs index d7479abc4..9ec20a115 100644 --- a/dstack/vmm/src/config.rs +++ b/dstack/vmm/src/config.rs @@ -357,11 +357,6 @@ pub struct CvmConfig { pub qemu_pci_hole64_size: u64, /// QEMU hotplug_off pub qemu_hotplug_off: bool, - /// Path to `qemu-bridge-helper`, used to attach an unprivileged TAP to a - /// host bridge. Empty probes the known distribution locations. - #[serde(default)] - pub qemu_bridge_helper: String, - /// TDX attestation/hash scheme policy. `legacy` keeps the existing /// digest.txt measurement path; `lite` opts into split measurement CBOR; /// `auto` selects `legacy` for @@ -772,16 +767,6 @@ impl Config { (1..=MAX_NET_QUEUES).contains(&self.cvm.max_net_queues), "cvm.max_net_queues must be between 1 and {MAX_NET_QUEUES}" ); - // The helper path is interpolated into QEMU's `-netdev` option list, - // which QEMU splits on ',' and '='. A path carrying either would not be - // passed through, it would end the option and start a bogus one, and the - // launch failure names neither this setting nor the file. Volume sources - // are rejected for the same reason. - anyhow::ensure!( - !self.cvm.qemu_bridge_helper.contains([',', '=']), - "cvm.qemu_bridge_helper must not contain ',' or '=': {}", - self.cvm.qemu_bridge_helper - ); anyhow::ensure!( !self .cvm @@ -906,10 +891,6 @@ fn validate_networking(networking: &Networking) -> Result<()> { !networking.nic.inherit_mode, "cvm.networking.inherit_mode is per-deployment state and cannot be set on the node default" ); - anyhow::ensure!( - networking.netd_interface.is_none(), - "cvm.networking.netd_interface is runtime state and cannot be set in configuration" - ); anyhow::ensure!( networking.device.is_empty(), "cvm.networking.device is runtime state and cannot be set in configuration" @@ -978,6 +959,19 @@ pub enum NetworkingMode { Macvtap, } +impl NetworkingMode { + /// The name this mode is written as in `vmm.toml`, in the RPC, and in + /// anything an operator reads. + pub fn as_str(self) -> &'static str { + match self { + NetworkingMode::User => "user", + NetworkingMode::Bridge => "bridge", + NetworkingMode::Custom => "custom", + NetworkingMode::Macvtap => "macvtap", + } + } +} + /// What a single NIC pins: the fields a deployment may name, a VM's manifest /// stores, and `GetInfo` reports back. /// @@ -1062,41 +1056,7 @@ pub struct Networking { // ── Custom fields ────────────────────────────────────────────── #[serde(default)] pub netdev: String, - // ── Runtime markers ──────────────────────────────────────────── - /// What netd built for this NIC, recorded when it was built. - /// - /// Runtime state, like `device`: resolution always clears it. Teardown - /// reads this rather than re-deriving it from node configuration, because - /// an operator may change `network_filter.mode` or `max_net_queues` while - /// the VM runs, and what has to be removed is what was created. - #[serde(default, skip_serializing_if = "NetdInterface::is_none")] - pub netd_interface: NetdInterface, -} - -/// The host interface netd created for a NIC, if any. -#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Deserialize, Serialize)] -#[serde(rename_all = "snake_case")] -pub enum NetdInterface { - /// netd was not involved: user mode, custom mode, or a bridge NIC that - /// QEMU's own bridge helper attaches. - #[default] - None, - /// netd created the interface and bound no libvirt nwfilter to it. - Unfiltered, - /// netd created the interface and bound a libvirt nwfilter to it, which - /// removal has to delete before the interface goes away. - Filtered, -} - -impl NetdInterface { - pub fn is_none(&self) -> bool { - matches!(self, NetdInterface::None) - } - - pub fn is_filtered(&self) -> bool { - matches!(self, NetdInterface::Filtered) - } } impl Networking { @@ -1449,25 +1409,21 @@ mod tests { .expect("default VMM config should parse") } - /// The two ownership markers are additive on disk: manifests and runtime - /// network snapshots written before they existed still load, and an entry - /// that carries neither serializes exactly as it used to. + /// The ownership marker is additive on disk: manifests and runtime network + /// snapshots written before it existed still load, and an entry that does + /// not carry it serializes exactly as it used to. #[test] fn ownership_markers_are_omitted_when_unset_and_default_when_absent() { let mut networking: Networking = serde_json::from_str(r#"{"mode":"bridge","bridge":"br0"}"#).unwrap(); assert!(!networking.nic.inherit_mode); - assert_eq!(networking.netd_interface, NetdInterface::None); let json = serde_json::to_string(&networking).unwrap(); assert!(!json.contains("inherit_mode"), "{json}"); - assert!(!json.contains("netd_interface"), "{json}"); networking.nic.inherit_mode = true; - networking.netd_interface = NetdInterface::Filtered; let json = serde_json::to_string(&networking).unwrap(); assert!(json.contains(r#""inherit_mode":true"#), "{json}"); - assert!(json.contains(r#""netd_interface":"filtered""#), "{json}"); assert_eq!( serde_json::from_str::(&json).unwrap(), networking @@ -1665,7 +1621,6 @@ mod networking_shape_tests { "vhost": false, "queues": 4, "inherit_mode": true, - "netd_interface": "filtered", }); // A resolved value keeps every field, at the same names as before. @@ -1694,26 +1649,6 @@ mod networking_shape_tests { assert!(stored.get("net").is_none()); } - /// The path is interpolated into QEMU's `-netdev` option list, which QEMU - /// splits on ',' and '='. A path carrying either would end the option and - /// start a bogus one, and the launch failure names neither the setting nor - /// the file. - #[test] - fn a_bridge_helper_path_cannot_end_the_qemu_option_it_sits_in() { - use rocket::figment::providers::Format as _; - let mut config: Config = rocket::figment::Figment::from( - rocket::figment::providers::Toml::string(DEFAULT_CONFIG), - ) - .extract() - .unwrap(); - config.cvm.qemu_bridge_helper = "/opt/qemu,helper".into(); - let error = config.validate().unwrap_err(); - assert!(error.to_string().contains("qemu_bridge_helper"), "{error}"); - - config.cvm.qemu_bridge_helper = "/usr/libexec/qemu-bridge-helper".into(); - config.validate().unwrap(); - } - /// The TOML section still deserializes through the flatten. #[test] fn the_node_section_still_parses_from_toml() { diff --git a/dstack/vmm/src/main.rs b/dstack/vmm/src/main.rs index 17b93d194..d35864dc4 100644 --- a/dstack/vmm/src/main.rs +++ b/dstack/vmm/src/main.rs @@ -77,6 +77,48 @@ struct NetdArgs { /// Override the Unix socket configured in [netd]. #[arg(long)] socket: Option, + /// Inspect a running netd instead of starting one. + #[command(subcommand)] + command: Option, +} + +#[derive(Subcommand)] +enum NetdCommand { + /// List every host interface netd holds. + /// + /// Answers the question a leak is made of -- whose is this interface -- + /// which deriving a name from an identity cannot. + List { + /// Only this VMM instance's interfaces. Defaults to every one netd + /// owns, including those it cannot attribute. + #[arg(long)] + instance: Option, + }, + /// Delete one interface by name. + /// + /// For what nothing else can name: an interface built before netd recorded + /// ownership, or by another netd, whose VM is gone. `netd list` shows these + /// with no instance and no VM, so nothing can derive the sweep that would + /// take them; an operator who can tell what they are says so here. + RemoveInterface { + /// The interface name, as `netd list` prints it. + name: String, + }, + /// Delete every interface netd holds for one VM. + /// + /// For a VM whose VMM will never ask again -- one whose directory was + /// deleted by hand, or whose instance is gone. A VMM sweeps its own VMs on + /// every stop and every removal, and keeps a removal pending until the + /// sweep lands; this is for when no VMM will ever run that sweep. + RemoveVm { + /// The `cvm.instance_id` of the VMM that created them. `netd list` + /// shows it. + #[arg(long)] + instance: String, + /// The VM's ID. + #[arg(long)] + vm: String, + }, } #[derive(ClapArgs)] @@ -192,6 +234,64 @@ async fn log_rotation_task(app: App) { } } +/// Client-side netd subcommands. Talks to the socket like the VMM does, so it +/// needs whatever the socket's permissions ask for and not root. +async fn run_netd_command(config: &NetdConfig, command: &NetdCommand) -> Result<()> { + match command { + NetdCommand::List { instance } => { + let interfaces = netd::list(&config.socket, instance.as_deref().unwrap_or_default()) + .await + .context("failed to list netd interfaces")?; + println!( + "{:<16} {:<8} {:<24} {:<38} {:>3}", + "INTERFACE", "KIND", "INSTANCE", "VM", "NIC" + ); + let mut unattributed = 0; + for record in &interfaces { + if record.instance_id.is_none() { + unattributed += 1; + } + println!( + "{:<16} {:<8} {:<24} {:<38} {:>3}", + record.tap, + record.kind, + record.instance_id.as_deref().unwrap_or("-"), + record.vm_id.as_deref().unwrap_or("-"), + record + .nic_index + .map_or_else(|| "-".to_string(), |index| index.to_string()), + ); + } + println!(); + println!("{} interface(s)", interfaces.len()); + if unattributed > 0 { + // Not a fault to fix by hand: an interface built before netd + // recorded ownership, or by another netd, carries no record and + // gets one the next time its VM launches. + println!( + "{unattributed} carry no ownership record; `netd remove-interface` takes \ + one by name" + ); + } + Ok(()) + } + NetdCommand::RemoveInterface { name } => { + netd::remove_interface_named(&config.socket, name) + .await + .with_context(|| format!("failed to remove {name}"))?; + println!("removed {name}"); + Ok(()) + } + NetdCommand::RemoveVm { instance, vm } => { + let removed = netd::remove_all(&config.socket, instance, vm) + .await + .context("failed to remove the VM's interfaces")?; + println!("removed {removed} interface(s) for {vm}"); + Ok(()) + } + } +} + #[rocket::main] async fn main() -> Result<()> { { @@ -235,6 +335,9 @@ async fn main() -> Result<()> { .context("failed to load [cvm.network_filter] for netd")?, ); } + if let Some(command) = &netd_args.command { + return run_netd_command(&netd_config, command).await; + } return netd::serve(netd_config).await; } @@ -246,6 +349,7 @@ async fn main() -> Result<()> { // Preserve the existing startup validation. The broader static checks are // opt-in through `check-config` until they have seen wider deployment use. + netd::validate_instance_id(&config.cvm.instance_id)?; config .host_api .validate() diff --git a/dstack/vmm/src/main_service.rs b/dstack/vmm/src/main_service.rs index 6f9a0905d..1c2835942 100644 --- a/dstack/vmm/src/main_service.rs +++ b/dstack/vmm/src/main_service.rs @@ -25,8 +25,9 @@ use ra_rpc::{CallContext, RpcCall}; use tracing::{info, warn}; use crate::app::{ - needs_swtpm, resolve_networking, validate_resolved_network, validate_resolved_networks, App, - AttachMode, GpuConfig, GpuSpec, Manifest, PortMapping, VmWorkDir, + mode_carries_ingress, needs_swtpm, resolve_networking, validate_resolved_network, + validate_resolved_networks, App, AttachMode, GpuConfig, GpuSpec, Manifest, PortMapping, + VmWorkDir, }; use crate::config::{CvmConfig, Networking, NetworkingMode, NicNetworking}; @@ -168,6 +169,62 @@ fn port_mappings_conflict(left: &PortMapping, right: &PortMapping) -> bool { || right.address.is_unspecified()) } +/// The backend each of a VM's NICs resolves to, with an empty list standing for +/// the node default's single NIC. +fn resolved_nic_modes( + networks: &[NicNetworking], + cvm_config: &CvmConfig, + vcpu: u32, +) -> Vec { + let node_default = [cvm_config.networking.nic.clone()]; + let requested = if networks.is_empty() { + &node_default[..] + } else { + networks + }; + requested + .iter() + .map(|networking| resolve_networking(networking, cvm_config, vcpu).nic.mode) + .collect() +} + +/// Rejects a mapping pinned to a NIC that cannot carry it. +/// +/// Both ways of getting that wrong are refused here, at deployment, where the +/// caller is present to be told: a NIC the VM does not have, and one whose +/// backend has no mechanism to publish a host port. Macvtap bypasses the host +/// bridge and a custom netdev is a string the VMM does not interpret, so a +/// mapping pinned to either used to resolve to that NIC and then fall out of +/// every branch that could act on it -- no `hostfwd=`, no netd request, and no +/// warning either. +fn validate_port_mapping_nics(mappings: &[PortMapping], modes: &[NetworkingMode]) -> Result<()> { + let nic_count = modes.len(); + for mapping in mappings { + let Some(index) = mapping.nic_index else { + continue; + }; + let Some(mode) = modes.get(index) else { + bail!( + "port mapping {} {}:{} names NIC {index}, but this VM has {nic_count}", + mapping.protocol.as_str(), + mapping.address, + mapping.from + ); + }; + if !mode_carries_ingress(*mode) { + bail!( + "port mapping {} {}:{} names NIC {index}, which is {} and cannot publish a host \ + port; use a user-mode or bridge NIC", + mapping.protocol.as_str(), + mapping.address, + mapping.from, + mode.as_str(), + ); + } + } + Ok(()) +} + fn validate_unique_port_mappings(mappings: &[PortMapping]) -> Result<()> { for (index, mapping) in mappings.iter().enumerate() { if mappings[..index] @@ -216,10 +273,16 @@ pub fn create_manifest_from_vm_config( protocol, from, to, + nic_index: p.nic_index.map(|index| index as usize), }) }) .collect::>>()?; validate_unique_port_mappings(&port_map)?; + let networks = networks_from_vm_config(&request, cvm_config)?; + validate_port_mapping_nics( + &port_map, + &resolved_nic_modes(&networks, cvm_config, request.vcpu), + )?; let app_id = match &request.app_id { Some(id) => id.strip_prefix("0x").unwrap_or(id).to_lowercase(), @@ -268,7 +331,7 @@ pub fn create_manifest_from_vm_config( no_tee: request.no_tee || simulated_tee.is_some(), simulated_tee, swtpm, - networks: networks_from_vm_config(&request, cvm_config)?, + networks, volumes, }) } @@ -865,6 +928,21 @@ impl VmmRpc for RpcHandler { async fn update_vm(self, request: UpdateVmRequest) -> Result { info!(vm_id = %request.id, "update_vm RPC called"); + // A VM being removed is not one to reconfigure. Before the lock, + // because removal holds it across the whole teardown and anything that + // only asked afterwards would wait that out in order to be told no. + self.app.refuse_if_removing(&request.id)?; + // Held from here rather than around the parts that touch the host, + // because everything below writes into the workdir -- the compose + // file first, the manifest last -- and `put_manifest` creates the + // directory it writes into. An update that resumed after a removal + // deleted that directory would recreate it holding nothing but a + // manifest: invisible to `list_vms`, unloadable at every start, and + // claiming the VM's netd interfaces against collection forever. + let _launch = self.app.launch_lock(&request.id).await; + // Again under the lock: removal can have claimed the VM while this + // waited for it. + self.app.refuse_if_removing(&request.id)?; let new_id = if !request.compose_file.is_empty() { // check the compose file is valid let _app_compose: AppCompose = @@ -918,6 +996,7 @@ impl VmmRpc for RpcHandler { protocol: p.protocol.parse().context("Invalid protocol")?, from: p.host_port.try_into().context("Invalid host port")?, to: p.vm_port.try_into().context("Invalid vm port")?, + nic_index: p.nic_index.map(|index| index as usize), }) }) .collect::>>()?; @@ -944,6 +1023,12 @@ impl VmmRpc for RpcHandler { let networks = networks_from_proto(&request.networks, &cvm)?; resolve_requested_networks(&networks, &cvm, manifest.vcpu)? }; + // Under the launch lock this whole call holds. Reading "not + // running" outside it and acting on the answer inside is the exact + // race the lock exists to close: a launch can start, prepare its + // interfaces and deploy QEMU in between, and the release would + // then delete the interfaces of a VM that is running -- silently, + // since QEMU stays up and the supervisor still reports it healthy. let is_running = self .app .supervisor @@ -951,15 +1036,24 @@ impl VmmRpc for RpcHandler { .await? .is_some_and(|info| info.state.status.is_running()); if !is_running { - let runtime_networks = vm_work_dir.runtime_networks(); - self.app - .remove_filtered_networks(&request.id, &runtime_networks) - .await - .context("failed to remove previous filtered networking")?; + self.app.release_vm_interfaces(&request.id).await; vm_work_dir.clear_runtime_networks()?; } manifest.networks = networks; } + // Both only when this request moved one of the two halves, and after + // both, since either half can move and the other still has to agree + // with it. A VM deployed before the node could answer for its ports + // must stay editable in every other respect: read-modify-write sends + // the whole configuration back, and refusing a memory change over a + // port mapping nobody touched -- or over a node default that changed + // under it -- would make the VM unmanageable rather than fixed. + if request.update_ports || request.update_networking { + validate_port_mapping_nics( + &manifest.port_map, + &resolved_nic_modes(&manifest.networks, &self.app.config.cvm, manifest.vcpu), + )?; + } let compose_file = fs::read_to_string(vm_work_dir.app_compose_path()) .context("failed to read app compose for swtpm decision")?; manifest.swtpm = needs_swtpm( @@ -1344,6 +1438,67 @@ mod tests { } } + fn pinned(nic_index: Option) -> PortMapping { + PortMapping { + address: "0.0.0.0".parse().unwrap(), + protocol: crate::config::Protocol::Tcp, + from: 443, + to: 443, + nic_index, + } + } + + /// Both ways of naming a NIC that cannot publish a port are refused where + /// the caller is present to be told. A pin to macvtap or to a custom + /// netdev used to resolve to that NIC and then fall out of every branch + /// that could act on it, so the port simply never appeared. + #[test] + fn a_pin_is_checked_against_the_backend_and_not_only_the_count() { + let mut cvm = test_cvm_config(); + cvm.networking.nic.parent = "eth0".into(); + cvm.networking.nic.bridge = "br0".into(); + + let bridge_then_macvtap = vec![ + NicNetworking { + mode: NetworkingMode::Bridge, + bridge: "br0".into(), + ..NicNetworking::default() + }, + NicNetworking { + mode: NetworkingMode::Macvtap, + parent: "eth0".into(), + ..NicNetworking::default() + }, + ]; + let modes = resolved_nic_modes(&bridge_then_macvtap, &cvm, 2); + assert_eq!(modes, vec![NetworkingMode::Bridge, NetworkingMode::Macvtap]); + + // A bridge TAP is netd's, and netd does not forward host ports. + let error = validate_port_mapping_nics(&[pinned(Some(0))], &modes).unwrap_err(); + assert!(error.to_string().contains("bridge"), "{error}"); + + let error = validate_port_mapping_nics(&[pinned(Some(1))], &modes).unwrap_err(); + assert!(error.to_string().contains("macvtap"), "{error}"); + assert!( + error.to_string().contains("cannot publish a host port"), + "{error}" + ); + + let error = validate_port_mapping_nics(&[pinned(Some(2))], &modes).unwrap_err(); + assert!(error.to_string().contains("this VM has 2"), "{error}"); + + // An unpinned mapping is never refused here: where it lands is + // resolved at launch, from the topology in force then. + validate_port_mapping_nics(&[pinned(None)], &modes).unwrap(); + + // An empty list is the node default's one NIC, resolved the same way a + // launch would resolve it rather than assumed to be user mode. + cvm.networking.nic.mode = NetworkingMode::Macvtap; + let modes = resolved_nic_modes(&[], &cvm, 2); + assert_eq!(modes, vec![NetworkingMode::Macvtap]); + assert!(validate_port_mapping_nics(&[pinned(Some(0))], &modes).is_err()); + } + #[test] fn create_without_networking_persists_following_default() { let manifest = @@ -1634,6 +1789,9 @@ mod tests { .expect("a named backend is an override"); } + /// Deliberately restated rather than calling `NetworkingMode::as_str`: the + /// test below checks that what `GetInfo` reports is accepted back, and a + /// helper that shares the production mapping could only ever agree with it. fn networking_mode_name_for_test(mode: NetworkingMode) -> &'static str { match mode { NetworkingMode::Bridge => "bridge", diff --git a/dstack/vmm/src/netd.rs b/dstack/vmm/src/netd.rs index 6a760d1eb..aca287a9c 100644 --- a/dstack/vmm/src/netd.rs +++ b/dstack/vmm/src/netd.rs @@ -5,6 +5,7 @@ //! Small privileged broker for TAP creation and libvirt nwfilter bindings. use std::{ + collections::HashSet, fs::{File, OpenOptions, Permissions}, io::Write as _, os::{ @@ -43,7 +44,20 @@ const LOCK_PATH: &str = "/run/lock/dstack-netd.lock"; /// Upper bound on TAP queue pairs netd will create. Mirrors the VMM's own cap /// so a malformed request cannot ask the kernel for an unbounded device. const MAX_QUEUES: u32 = 64; - +/// Highest NIC index an identity may name. Also the width of the space a +/// whole-VM sweep has to enumerate, since it derives names instead of reading a +/// record. +const MAX_NIC_INDEX: usize = 255; +/// The interface names netd may create. Reserved: anything matching it is +/// netd's to delete, and nothing else on the host may take one. +const TAP_PREFIX: &str = "dt"; +/// Hex characters of digest in a TAP name, after [`TAP_PREFIX`]. +const TAP_DIGEST_CHARS: usize = 12; +/// Version tag on the ownership record. Present so a later format can be told +/// from this one rather than mis-parsed as it. +const ALIAS_PREFIX: &str = "dstack1"; +/// What the kernel stores in an interface alias, minus the terminator. +const MAX_IFALIAS: usize = 255; #[derive(Debug, Clone, Serialize, Deserialize)] pub struct InterfaceIdentity { pub instance_id: String, @@ -69,6 +83,37 @@ pub struct PrepareBridgeRequest { /// rejects a device whose `IFF_MULTI_QUEUE` state differs from its own /// `queues=` argument, so this must match the launch exactly. pub queues: u32, + /// The VM's working directory on the host, for logs and diagnostics. + /// + /// Untrusted and never read for a decision: any process that can reach the + /// socket can assert anything here. It is carried so an operator reading + /// netd's log can get from an opaque TAP name back to the VM that asked for + /// it without going through the VMM. + #[serde(default)] + pub workdir: String, +} + +/// One host resource netd holds. +/// +/// `instance_id` and `vm_id` are absent when the interface carries no record +/// that checks out: built by a netd too old to write one, by a third-party +/// netd, or by this one in the instant between creating the interface and +/// recording it. Absent is not "nobody's" -- it is "not known to be anybody's", +/// which is a materially different thing to a collection. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct InterfaceRecord { + pub tap: String, + /// `"tap"`, `"macvtap"`, or `"binding"` for an nwfilter binding whose + /// interface is already gone. A binding outlives the interface it was + /// bound to, so a collection that only looked at interfaces would leave + /// the one piece of state that survives them. + pub kind: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub instance_id: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub vm_id: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub nic_index: Option, } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -84,6 +129,10 @@ pub struct PrepareMacvtapRequest { /// queues; QEMU then opens the character device once per queue. #[serde(default)] pub queues: u32, + /// The VM's working directory on the host. Informational only; see + /// [`PrepareBridgeRequest::workdir`]. + #[serde(default)] + pub workdir: String, } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -94,10 +143,43 @@ pub enum Request { Remove { #[serde(flatten)] identity: InterfaceIdentity, - /// Whether this interface was created with an nwfilter binding. - /// Macvtap TAPs never carry one, and removal detects them rather than - /// trusting this field. - filtered: bool, + }, + /// Delete every interface netd holds for one VM. + /// + /// Teardown by identity can only reach the NIC indices its caller still has + /// a record of, and that record is written after the interface exists: a + /// VMM killed in between leaves a TAP nothing on disk points at. A manifest + /// that lost a NIC leaves the same thing behind. Both are found here + /// without a record, because every name netd can produce for a VM is + /// derivable from its identity. + RemoveAll { + instance_id: String, + vm_id: String, + }, + /// Everything netd holds, so that an operator can see the host's + /// interfaces without being told what to look for. + /// + /// Deriving a name answers "where is this VM's interface". It cannot + /// answer "whose is this interface", which is the question a leak is made + /// of: a VM whose directory was deleted, a VMM instance that was + /// decommissioned, an interface built by a netd that has since been + /// upgraded. Enumeration answers it. + List { + /// Only interfaces recorded as this instance's. Empty lists every one + /// netd owns, whatever it is recorded as and whether or not it is. + #[serde(default)] + instance_id: String, + }, + /// Delete one interface by name. + /// + /// For what nothing else can reach: an interface built before netd recorded + /// ownership, or by another netd, whose VM is gone. Nothing can attribute + /// it, so nothing can decide about it -- but an operator looking at + /// `list` can, and this is how they say so. Guarded the same way every + /// other removal is: the name must be one netd could have created, and the + /// device must be one netd creates. + RemoveInterface { + tap: String, }, /// Verify a deterministic TAP and binding for operations and integration /// diagnostics. The VMM startup path uses Prepare rather than Check. @@ -120,26 +202,73 @@ struct Response { /// between "one queue was requested" and "this netd ignored the request". #[serde(default, skip_serializing_if = "Option::is_none")] queues: Option, + /// How many interfaces a whole-VM sweep deleted. + #[serde(default, skip_serializing_if = "Option::is_none")] + removed: Option, + /// For a listing, everything netd holds; for a collection, what it took, + /// or would take on a dry run. Absent, rather than empty, from a netd that + /// cannot enumerate: "I hold nothing" and "I cannot say" are answers a + /// collection must not confuse. + #[serde(default, skip_serializing_if = "Option::is_none")] + interfaces: Option>, #[serde(default, skip_serializing_if = "Option::is_none")] error: Option, } -/// What netd built, echoed back so the caller can verify it matches the -/// request before handing the interface to QEMU. -struct Prepared { - tap: String, - device: Option, - queues: Option, +/// What one request produced. +/// +/// An enum rather than a struct of options, because the shapes do not overlap: +/// a prepare names an interface, a sweep counts them, a listing enumerates. +/// One flat [`Response`] still carries all of them on the wire, so a netd that +/// grows an operation stays readable to a caller that does not know it. +enum Outcome { + /// One interface, named because the caller hands that name to QEMU. + Interface { + tap: String, + device: Option, + queues: Option, + }, + /// A sweep names no single interface, so it reports how many it deleted. + Swept { + removed: usize, + }, + Listed(Vec), } -impl Prepared { +impl Outcome { fn tap(tap: String) -> Self { - Self { + Self::Interface { tap, device: None, queues: None, } } + + fn into_response(self) -> Response { + let mut response = Response { + ok: true, + tap: None, + device: None, + queues: None, + removed: None, + interfaces: None, + error: None, + }; + match self { + Self::Interface { + tap, + device, + queues, + } => { + response.tap = Some(tap); + response.device = device; + response.queues = queues; + } + Self::Swept { removed } => response.removed = Some(removed), + Self::Listed(interfaces) => response.interfaces = Some(interfaces), + } + response + } } pub fn tap_name(identity: &InterfaceIdentity) -> String { @@ -148,7 +277,82 @@ pub fn tap_name(identity: &InterfaceIdentity) -> String { identity.instance_id, identity.vm_id, identity.nic_index ); let digest = Sha256::digest(input.as_bytes()); - format!("dt{}", hex::encode(&digest[..6])) + format!( + "{TAP_PREFIX}{}", + hex::encode(&digest[..TAP_DIGEST_CHARS / 2]) + ) +} + +/// The ownership record netd writes onto every interface it creates. +/// +/// The record lives on the resource, so it has exactly the resource's +/// lifetime. A file under `/run` would be a second thing to keep in step with +/// the first, and the failure this whole path exists to fix is precisely a +/// record that got out of step: written after the interface, lost with the +/// directory, and unreadable to anything but the process that wrote it. +/// +/// Never trusted as *authority*. Anything that can reach this socket can also +/// name an identity, and the interface name is a digest of that identity -- +/// so a record is believed only when re-deriving the name from it reproduces +/// the name it is written on. Ambiguity (a separator inside an identity), +/// truncation, and forgery all fail that check and land in the same bucket as +/// no record at all, which is the bucket handled conservatively. +pub fn interface_alias(identity: &InterfaceIdentity) -> String { + format!( + "{ALIAS_PREFIX}:{}:{}:{}", + identity.nic_index, identity.instance_id, identity.vm_id + ) +} + +/// The identity an interface claims, if the claim checks out. +/// +/// `nic_index` first, so the two free-form fields are the last two and a +/// `vm_id` containing the separator still parses. An `instance_id` containing +/// one does not, and is refused at prepare rather than mis-parsed here. +pub fn owner_of(tap: &str, alias: &str) -> Option { + // `trim_end_matches`, not `trim`: sysfs adds a newline, and a `vm_id` + // whose own trailing whitespace were trimmed off here would re-derive a + // name that is not the one it is on, making the interface permanently + // unattributable -- never collected, only removable by hand. + let rest = alias + .trim_end_matches(['\n', '\r']) + .strip_prefix(ALIAS_PREFIX)? + .strip_prefix(':')?; + let (nic_index, rest) = rest.split_once(':')?; + let (instance_id, vm_id) = rest.split_once(':')?; + let identity = InterfaceIdentity { + instance_id: instance_id.to_string(), + vm_id: vm_id.to_string(), + nic_index: nic_index.parse().ok()?, + }; + // The name is the proof. A record that does not reproduce it describes + // some other interface, or nothing. + (tap_name(&identity) == tap).then_some(identity) +} + +/// Whether this name is one netd can have created. See [`TAP_PREFIX`]. +pub fn is_managed_name(interface: &str) -> bool { + let Some(digest) = interface.strip_prefix(TAP_PREFIX) else { + return false; + }; + digest.len() == TAP_DIGEST_CHARS + && digest + .bytes() + .all(|byte| byte.is_ascii_digit() || (b'a'..=b'f').contains(&byte)) +} + +/// Rejects an instance ID no interface could be recorded as belonging to. +/// +/// At startup rather than at the first launch. The VMM derives one that is +/// always valid; an operator who configured their own learns here rather than +/// from the first VM that fails to get a NIC. +pub fn validate_instance_id(instance_id: &str) -> Result<()> { + validate_identity(&InterfaceIdentity { + instance_id: instance_id.to_string(), + vm_id: "0".repeat(64), + nic_index: MAX_NIC_INDEX, + }) + .context("invalid cvm.instance_id") } pub fn instance_id(configured: &str, run_path: &Path) -> String { @@ -181,23 +385,96 @@ impl std::fmt::Display for Unreachable { impl std::error::Error for Unreachable {} /// Whether this error means netd was never reached. +/// +/// `downcast_ref` rather than a walk over `chain()`: a marker attached with +/// `context` is not a link in the source chain, it is the context *of* a link, +/// and `chain()` yields the wrapper rather than the marker inside it. Asking +/// the chain therefore always answered no -- which made every "an unreachable +/// netd is not a failure" branch in this crate unreachable itself. pub fn is_unreachable(error: &anyhow::Error) -> bool { - error.chain().any(|cause| cause.is::()) + error.downcast_ref::().is_some() } pub async fn request(socket: &Path, request: &Request) -> Result { + let response = exchange(socket, request).await?; + if response.tap.as_deref().unwrap_or_default().is_empty() { + bail!("netd response omitted TAP name"); + } + Ok(PreparedInterface { + device: response.device, + queues: response.queues, + }) +} + +/// Deletes every interface netd holds for one VM, returning how many there +/// were. See [`Request::RemoveAll`]. +/// +/// A netd that answers without a count did not sweep. Reading that as zero +/// would report a netd that cannot do this as a VM that had nothing to remove, +/// which is a netd that cannot do this reported as a VM that had nothing to +/// remove -- so it is an error here. +pub async fn remove_all(socket: &Path, instance_id: &str, vm_id: &str) -> Result { + let request = Request::RemoveAll { + instance_id: instance_id.to_string(), + vm_id: vm_id.to_string(), + }; + exchange(socket, &request) + .await? + .removed + .context("netd answered a sweep without saying what it removed") +} + +/// Deletes one interface by name. See [`Request::RemoveInterface`]. +pub async fn remove_interface_named(socket: &Path, tap: &str) -> Result<()> { + let request = Request::RemoveInterface { + tap: tap.to_string(), + }; + exchange(socket, &request).await.map(|_| ()) +} + +/// Everything netd holds, optionally narrowed to one VMM instance. See +/// [`Request::List`]. +pub async fn list(socket: &Path, instance_id: &str) -> Result> { + let request = Request::List { + instance_id: instance_id.to_string(), + }; + exchange(socket, &request) + .await? + .interfaces + .context("netd answered a listing without one") +} + +async fn exchange(socket: &Path, request: &Request) -> Result { let operation = match request { Request::PrepareBridge(_) => "prepare_bridge", Request::PrepareMacvtap(_) => "prepare_macvtap", Request::Remove { .. } => "remove", + Request::RemoveAll { .. } => "remove_all", + Request::List { .. } => "list", + Request::RemoveInterface { .. } => "remove_interface", Request::Check { .. } => "check", }; let exchange = async { - let mut stream = UnixStream::connect(socket) - .await - .map_err(anyhow::Error::from) - .context(Unreachable) - .with_context(|| format!("failed to connect to netd at {}", socket.display()))?; + let mut stream = UnixStream::connect(socket).await.map_err(|error| { + // Only the two errnos that mean "nothing is listening". A socket + // the VMM's user cannot open (`EACCES`, the default `0660` on a + // root-owned socket) or a VMM out of descriptors would otherwise + // read as an absent netd, and every caller that treats absence as + // "nothing to do here" would skip its work at `debug!` -- leaking + // interfaces on a host whose netd is running fine, while telling + // the operator to go start one. + let absent = matches!( + error.kind(), + std::io::ErrorKind::NotFound | std::io::ErrorKind::ConnectionRefused + ); + let error = anyhow::Error::from(error) + .context(format!("failed to connect to netd at {}", socket.display())); + if absent { + error.context(Unreachable) + } else { + error + } + })?; let message = serde_json::to_vec(request)?; if message.len() as u64 > MAX_MESSAGE_SIZE { bail!("netd request is too large"); @@ -220,11 +497,7 @@ pub async fn request(socket: &Path, request: &Request) -> Result Result<()> { None => bind_listener(&config)?, }; info!(address = ?listener.local_addr()?, "netd listening"); + let config = std::sync::Arc::new(config); loop { let (mut stream, _) = listener.accept().await?; - // This timeout bounds async socket reads and writes. handle_request is - // synchronous, so helper execution is bounded separately by - // COMMAND_TIMEOUT rather than preempted by this future timeout. - match timeout(CONNECTION_TIMEOUT, serve_connection(&config, &mut stream)).await { - Ok(Ok(())) => {} - Ok(Err(error)) => warn!(%error, "netd connection failed"), - Err(_) => warn!("netd connection timed out"), - } + // One task per connection, so a request that takes minutes does not + // stop the next one from being *accepted*. + // + // Serialization still holds where it matters, and holds where it is + // actually stated: `handle_request` takes the operation lock, which is + // an flock and blocks between two open descriptions in one process + // just as it does between processes. What a single-connection accept + // loop added on top of that was head-of-line blocking -- a caller + // asking netd a question it answers in microseconds waited for whatever + // netd happened to be doing, and gave up believing netd was not there. + // A collection can run for twenty seconds and a `virsh` for thirty, so + // this was not a corner: it made a busy netd indistinguishable from an + // absent one, and teardown skips an absent netd. + let config = config.clone(); + tokio::spawn(async move { + if let Err(error) = serve_connection(&config, &mut stream).await { + warn!(%error, "netd connection failed"); + } + }); } } @@ -287,19 +572,46 @@ fn bind_listener(config: &NetdConfig) -> Result { Ok(listener) } -async fn serve_connection(config: &NetdConfig, stream: &mut UnixStream) -> Result<()> { +/// Serves one connection. +/// +/// The timeouts bound the socket reads and writes and nothing else. They used +/// to wrap the whole exchange, which read as a bound on the request but was +/// not one: `handle_request` is synchronous and shells out, so the timeout +/// could not cancel it -- it only threw away the answer to work that went on +/// running. A caller has its own deadline and will have gone by then; what it +/// cost was netd's own log, which reported "timed out" for a request it in +/// fact completed. Helper execution is bounded where it can be, by +/// COMMAND_TIMEOUT per invocation. +async fn serve_connection( + config: &std::sync::Arc, + stream: &mut UnixStream, +) -> Result<()> { // Access is authorized by the Unix socket's owner, group, and mode. Any // process that can connect is trusted with the complete netd protocol. - let outcome = match read_request(stream).await { - // A peer that connects and closes without sending is the VMM's - // reachability check: netd that died leaves its socket behind, so the - // VMM connects to tell the two apart. Answering that with a parse error - // and a warning would fill the log with reports of it working. + let request = timeout(CONNECTION_TIMEOUT, read_request(stream)) + .await + .context("timed out reading a netd request")?; + let outcome = match request { + // A peer that connects and closes without sending is asking whether + // anything is listening: netd that died leaves its socket behind. + // Answering that with a parse error and a warning would fill the log + // with reports of it working. Ok(None) => { debug!("netd liveness probe"); return Ok(()); } - Ok(Some(request)) => handle_request(config, request), + Ok(Some(request)) => { + // `handle_request` shells out to `ip` and `virsh` and waits on the + // operation lock, so it can block for as long as those take. On a + // runtime worker that would stall every other connection's reads + // and writes, which is the head-of-line blocking this daemon just + // stopped having. + let config = config.clone(); + match tokio::task::spawn_blocking(move || handle_request(&config, request)).await { + Ok(outcome) => outcome, + Err(error) => Err(anyhow::anyhow!("netd worker failed: {error}")), + } + } // A request that arrived but could not be understood still gets an // answer. A VMM newer than this netd sends operations it does not // know, and "unknown variant `prepare_foo`" is what tells the operator @@ -307,13 +619,7 @@ async fn serve_connection(config: &NetdConfig, stream: &mut UnixStream) -> Resul Err(error) => Err(error), }; let response = match outcome { - Ok(prepared) => Response { - ok: true, - tap: Some(prepared.tap), - device: prepared.device, - queues: prepared.queues, - error: None, - }, + Ok(outcome) => outcome.into_response(), Err(error) => { warn!(%error, "netd request failed"); Response { @@ -321,13 +627,19 @@ async fn serve_connection(config: &NetdConfig, stream: &mut UnixStream) -> Resul tap: None, device: None, queues: None, + removed: None, + interfaces: None, error: Some(format!("{error:#}")), } } }; let encoded = serde_json::to_vec(&response)?; - stream.write_all(&encoded).await?; - stream.shutdown().await?; + timeout(CONNECTION_TIMEOUT, async { + stream.write_all(&encoded).await?; + stream.shutdown().await + }) + .await + .context("timed out answering a netd request")??; Ok(()) } @@ -349,7 +661,7 @@ async fn read_request(stream: &mut UnixStream) -> Result> { .context("invalid netd request") } -fn handle_request(config: &NetdConfig, request: Request) -> Result { +fn handle_request(config: &NetdConfig, request: Request) -> Result { let libvirt_uri = config.libvirt_uri.as_str(); let _lock = OperationLock::acquire()?; match request { @@ -359,11 +671,32 @@ fn handle_request(config: &NetdConfig, request: Request) -> Result { Request::PrepareMacvtap(request) => { prepare_macvtap(libvirt_uri, &request, config.filter_policy()) } - Request::Remove { identity, filtered } => { + Request::List { instance_id } => { + Ok(Outcome::Listed(list_interfaces(libvirt_uri, &instance_id))) + } + Request::RemoveAll { instance_id, vm_id } => { + let removed = sweep_vm_interfaces(libvirt_uri, &instance_id, &vm_id)?; + Ok(Outcome::Swept { removed }) + } + Request::RemoveInterface { tap } => { + if !is_managed_name(&tap) { + bail!("{tap} is not a name netd could have created"); + } + remove_interface(libvirt_uri, &tap, BindingCleanup::BestEffort)?; + Ok(Outcome::tap(tap)) + } + Request::Remove { identity } => { validate_identity(&identity)?; let tap = tap_name(&identity); - remove_interface(libvirt_uri, &tap, binding_cleanup(filtered))?; - Ok(Prepared::tap(tap)) + // Best effort, whatever the caller says was built. The strict rule + // exists for prepare, where a binding left at the name would block + // the one about to be created; at removal nothing is about to take + // the name, and failing here leaves the interface itself up on the + // bridge rather than just a binding libvirt will hand back on its + // next listing. It also makes this agree with the whole-VM sweep, + // which has always been best effort. + remove_interface(libvirt_uri, &tap, BindingCleanup::BestEffort)?; + Ok(Outcome::tap(tap)) } Request::Check { identity, filtered } => { validate_identity(&identity)?; @@ -376,7 +709,7 @@ fn handle_request(config: &NetdConfig, request: Request) -> Result { if filtered && !is_macvtap(&tap) { virsh(libvirt_uri, &["nwfilter-binding-dumpxml", &tap], None)?; } - Ok(Prepared::tap(tap)) + Ok(Outcome::tap(tap)) } } } @@ -385,7 +718,7 @@ fn prepare_macvtap( libvirt_uri: &str, request: &PrepareMacvtapRequest, filter: &NetworkFilterConfig, -) -> Result { +) -> Result { let identity = &request.identity; let parent = request.parent.as_str(); let qemu_uid = request.qemu_uid; @@ -431,6 +764,7 @@ fn prepare_macvtap( add.extend_from_slice(&["type", "macvtap", "mode", mode]); ip(&add)?; let result = (|| { + set_alias(&tap, identity)?; let ifindex = std::fs::read_to_string(Path::new("/sys/class/net").join(&tap).join("ifindex")) .context("failed to read macvtap ifindex")?; @@ -455,7 +789,7 @@ fn prepare_macvtap( match result { Ok(device) => { info!(%tap, %parent, %mode, %device, %queues, "prepared macvtap"); - Ok(Prepared { + Ok(Outcome::Interface { tap, device: Some(device), queues: Some(queues), @@ -500,13 +834,25 @@ fn prepare_bridge( libvirt_uri: &str, request: &PrepareBridgeRequest, filter: &NetworkFilterConfig, -) -> Result { +) -> Result { validate_prepare_bridge(request, filter)?; let filtered = request.filtered; let tap = tap_name(&request.identity); // A failed VMM start may leave a deterministic resource behind. Replacing // it makes prepare idempotent without accepting a caller-selected TAP. - remove_interface(libvirt_uri, &tap, binding_cleanup(filtered))?; + // A binding outlives the interface it was bound to and TAP names are + // derived, so the same name comes back: clear whatever is there. Insist + // only when this prepare is about to create a replacement libvirt would + // refuse as a duplicate. + remove_interface( + libvirt_uri, + &tap, + if filtered { + BindingCleanup::Required + } else { + BindingCleanup::BestEffort + }, + )?; let uid = request.qemu_uid.to_string(); let queues = validate_queues(request.queues)?; @@ -519,6 +865,10 @@ fn prepare_bridge( add.extend_from_slice(&["user", &uid]); ip(&add)?; let result = (|| { + // Before anything else it could fail at. An interface that exists + // without a record is one nothing can attribute, and the window in + // which that is true is the window a crash turns permanent. + set_alias(&tap, &request.identity)?; ip(&["link", "set", "dev", &tap, "master", &request.bridge])?; if filtered { let xml = binding_xml(request, &tap, filter); @@ -536,10 +886,13 @@ fn prepare_bridge( return Err(error); } info!(%tap, bridge = %request.bridge, %filtered, %queues, "prepared TAP"); - Ok(Prepared { + Ok(Outcome::Interface { tap, device: None, queues: Some(queues), + // This netd builds interfaces; it is not the host's forwarder. Saying + // nothing here is what tells the caller that, so ports it asked for are + // reported as unmet rather than assumed done. }) } @@ -555,11 +908,160 @@ enum BindingCleanup { /// be running at all, and a stale binding left by an earlier, filtered /// interface at this name is still worth clearing when it is. BestEffort, + /// The caller has already decided about the binding. Used by a pass over + /// many interfaces, which asks libvirt once about all of them rather than + /// once per interface. + Skip, +} + +/// Deletes every interface a VM could hold, by deriving each name rather than +/// consulting a record. +/// +/// `validate_identity` caps the NIC index, so the whole space a VM can occupy +/// is enumerable: 256 names, each a `stat` that usually misses. Cleanup is +/// best-effort about bindings -- nothing is about to take these names, and a +/// node running unfiltered TAPs need not have libvirtd at all. +/// +/// A name with no interface is not skipped. An nwfilter binding outlives the +/// TAP it was bound to, so the one state teardown must not leave behind is +/// exactly the one a `/sys/class/net` check cannot see: the per-name Remove +/// this replaced deleted the binding unconditionally, and a sweep that reaches +/// less than the thing it replaced is not a sweep. Those names are decided +/// against a single listing, because the whole point of enumerating a bounded +/// space is that deciding one name stays cheap. +fn sweep_vm_interfaces(libvirt_uri: &str, instance_id: &str, vm_id: &str) -> Result { + let identity = InterfaceIdentity { + instance_id: instance_id.to_string(), + vm_id: vm_id.to_string(), + nic_index: 0, + }; + validate_identity(&identity)?; + let bindings = existing_bindings(libvirt_uri); + // Not `bindings.is_some()`. A listing that could not be produced says + // nothing about whether a *deletion* will work, and reading it as "libvirt + // is down, skip the bindings" would mean a node whose listing breaks for + // any reason silently stops cleaning up bindings at all -- which is worse + // than the per-name asking this listing exists to avoid. The pass finds + // out by trying, once. + let mut libvirt = true; + let mut removed = 0; + let mut first_error = None; + for nic_index in 0..=MAX_NIC_INDEX { + let tap = tap_name(&InterfaceIdentity { + nic_index, + ..identity.clone() + }); + let present = Path::new("/sys/class/net").join(&tap).exists(); + // A pass gets one answer about libvirt, not one per interface. Asking + // again after it has failed is how a hung `libvirtd` turns a bounded + // collection into an unbounded one. + let wanted = present || bindings.as_ref().is_some_and(|held| held.contains(&tap)); + if libvirt && wanted && !is_macvtap(&tap) { + if let Err(error) = delete_binding(libvirt_uri, &tap) { + warn!(%tap, %error, "failed to remove an nwfilter binding"); + libvirt = false; + first_error.get_or_insert(error); + } else if !present { + info!(%tap, %vm_id, "removed orphaned nwfilter binding"); + } + } + if !present { + continue; + } + // Keep going after a failure. Stopping at the first one would leave the + // rest of a VM's interfaces behind over one that is stuck. + match remove_interface(libvirt_uri, &tap, BindingCleanup::Skip) { + Err(error) => { + warn!(%tap, %error, "failed to remove interface"); + first_error.get_or_insert(error); + } + Ok(()) => { + info!(%tap, %vm_id, "removed interface"); + removed += 1; + } + } + } + match first_error { + Some(error) => Err(error).context("failed to remove every interface for this VM"), + None => Ok(removed), + } +} + +/// Every resource netd owns, read off the host rather than out of a record. +/// +/// Ownership is the reserved name plus the kernel's own answer about what kind +/// of device it is; attribution is the interface's alias, checked by +/// re-deriving the name from it. A listing never fails for want of libvirt: on +/// a node that does not filter, `libvirtd` need not be running, and an +/// interface inventory that refused to be produced without it would be +/// unavailable exactly where unfiltered TAPs live. +fn list_interfaces(libvirt_uri: &str, instance_id: &str) -> Vec { + let bindings = existing_bindings(libvirt_uri); + let mut records = Vec::new(); + let mut seen = HashSet::new(); + if let Ok(entries) = std::fs::read_dir("/sys/class/net") { + for entry in entries.flatten() { + let Ok(tap) = entry.file_name().into_string() else { + continue; + }; + if !is_managed_name(&tap) { + continue; + } + let kind = if is_macvtap(&tap) { + "macvtap" + } else if is_tuntap(&tap) { + "tap" + } else { + // The name is netd's to use, but this is not a device netd + // creates. Listing it would invite a caller to delete it. + continue; + }; + let alias = + std::fs::read_to_string(Path::new("/sys/class/net").join(&tap).join("ifalias")) + .unwrap_or_default(); + let owner = owner_of(&tap, &alias); + seen.insert(tap.clone()); + records.push(InterfaceRecord { + kind: kind.to_string(), + nic_index: owner.as_ref().map(|identity| identity.nic_index), + instance_id: owner.as_ref().map(|identity| identity.instance_id.clone()), + vm_id: owner.map(|identity| identity.vm_id), + tap, + }); + } + } + // A binding outlives its interface, and an interface is the only thing that + // carries a record, so an orphaned binding can never be attributed. It is + // still netd's: nothing else creates a binding at one of these names. + for name in bindings.into_iter().flatten() { + if !seen.contains(&name) { + records.push(InterfaceRecord { + tap: name, + kind: "binding".to_string(), + instance_id: None, + vm_id: None, + nic_index: None, + }); + } + } + if !instance_id.is_empty() { + records.retain(|record| record.instance_id.as_deref() == Some(instance_id)); + } + records.sort_by(|left, right| left.tap.cmp(&right.tap)); + records } fn remove_interface(libvirt_uri: &str, tap: &str, cleanup: BindingCleanup) -> Result<()> { let macvtap = is_macvtap(tap); if Path::new("/sys/class/net").join(tap).exists() { + // The name is 48 bits of SHA-256, so a collision is not the worry. A + // caller asserting an identity that happens to derive to some + // pre-existing device is: netd runs as root and `ip link delete` does + // not ask what it is deleting. netd creates exactly two kinds of + // device, and the kernel publishes an attribute unique to each. + if !macvtap && !is_tuntap(tap) { + bail!("refusing to delete {tap}: it is neither a tun/tap nor a macvtap device"); + } let _ = ip(&["link", "set", "dev", tap, "down"]); } // A macvtap interface never carries a binding. Anything else might: this @@ -567,6 +1069,7 @@ fn remove_interface(libvirt_uri: &str, tap: &str, cleanup: BindingCleanup) -> Re // outlives the interface. if !macvtap { match cleanup { + BindingCleanup::Skip => {} BindingCleanup::Required => delete_binding(libvirt_uri, tap)?, BindingCleanup::BestEffort => { // netd refuses to start without virsh, so the binary is always @@ -585,6 +1088,24 @@ fn remove_interface(libvirt_uri: &str, tap: &str, cleanup: BindingCleanup) -> Re Ok(()) } +/// Records who an interface belongs to, on the interface. See +/// [`interface_alias`]. +fn set_alias(tap: &str, identity: &InterfaceIdentity) -> Result<()> { + let alias = interface_alias(identity); + ip(&["link", "set", "dev", tap, "alias", &alias]) + .with_context(|| format!("failed to record ownership on {tap}")) +} + +/// Whether this is a tun/tap device. `tun_flags` is published by the tun +/// driver and by nothing else, so its presence is the kernel's own answer -- +/// as `macvtap/` is for the other kind of device netd creates. +fn is_tuntap(interface: &str) -> bool { + Path::new("/sys/class/net") + .join(interface) + .join("tun_flags") + .exists() +} + fn is_macvtap(interface: &str) -> bool { Path::new("/sys/class/net") .join(interface) @@ -595,8 +1116,9 @@ fn is_macvtap(interface: &str) -> bool { /// Deletes an interface's nwfilter binding, if it has one. /// /// Goes through the same `COMMAND_TIMEOUT`-bounded helper as every other virsh -/// call. netd's accept loop is strictly serialized, so an unbounded call here -/// would let one unreachable libvirt stall every other VM's prepare and remove. +/// call. Every mutating operation holds the operation lock, so an unbounded +/// call here would let one unreachable libvirt stall every other VM's prepare +/// and remove behind it. fn delete_binding(uri: &str, tap: &str) -> Result<()> { match virsh(uri, &["nwfilter-binding-delete", tap], None) { Ok(()) => Ok(()), @@ -694,20 +1216,26 @@ fn validate_identity(identity: &InterfaceIdentity) -> Result<()> { bail!("invalid {label}"); } } - if identity.nic_index > 255 { + if identity.nic_index > MAX_NIC_INDEX { bail!("NIC index is out of range"); } - Ok(()) -} - -/// A caller that knows a binding is there needs it gone; one that does not -/// still clears whatever it finds, without failing when libvirt is absent. -fn binding_cleanup(filtered: bool) -> BindingCleanup { - if filtered { - BindingCleanup::Required - } else { - BindingCleanup::BestEffort + // An identity that cannot be recorded on the interface is refused rather + // than built unattributed. A host resource nothing can name the owner of + // is the thing this whole path exists to stop producing, and the kernel's + // alias is the only place with the interface's exact lifetime to put it. + let alias = interface_alias(identity); + if alias.len() > MAX_IFALIAS { + bail!( + "identity is too long to record on the interface: {} bytes of {MAX_IFALIAS}", + alias.len() + ); + } + // The record puts the two free-form fields last, so only the first of them + // has to be unambiguous. + if identity.instance_id.contains(':') { + bail!("instance ID must not contain ':'"); } + Ok(()) } /// Normalizes a requested queue pair count. Zero means the caller did not ask @@ -767,13 +1295,47 @@ fn ip(args: &[&str]) -> Result<()> { } fn virsh(uri: &str, args: &[&str], stdin: Option<&[u8]>) -> Result<()> { + virsh_output(uri, args, stdin).map(|_| ()) +} + +fn virsh_output(uri: &str, args: &[&str], stdin: Option<&[u8]>) -> Result { let mut full_args = vec!["--connect", uri]; full_args.extend_from_slice(args); - run_command(VIRSH_PATH, &full_args, stdin) + run_command_with_timeout(VIRSH_PATH, &full_args, stdin, COMMAND_TIMEOUT) +} + +/// Every nwfilter binding libvirt holds at a name netd could have created. +/// +/// One call, so that a sweep can decide 256 names against a set instead of +/// asking libvirt 256 times. `None` means libvirt could not be asked at all, +/// which on a node running unfiltered TAPs is the normal state -- `virsh` must +/// be installed for netd to start, but `libvirtd` need not be running. +/// +/// The command has no machine-readable mode: it prints a two-line header and +/// then one binding per line, interface name first, and it accepts no options +/// at all -- `--name` is not one of them, and asking for it fails the whole +/// call. Narrowing to netd's own name space is what makes parsing a human +/// table safe: a header, a rule line, or a column that moves cannot produce a +/// `dt` name, and a binding at any other name is not netd's to reason about. +fn existing_bindings(uri: &str) -> Option> { + match virsh_output(uri, &["nwfilter-binding-list"], None) { + Ok(output) => Some( + output + .lines() + .filter_map(|line| line.split_whitespace().next()) + .filter(|name| is_managed_name(name)) + .map(str::to_string) + .collect(), + ), + Err(error) => { + debug!("could not list nwfilter bindings: {error:#}"); + None + } + } } fn run_command(program: &str, args: &[&str], stdin: Option<&[u8]>) -> Result<()> { - run_command_with_timeout(program, args, stdin, COMMAND_TIMEOUT) + run_command_with_timeout(program, args, stdin, COMMAND_TIMEOUT).map(|_| ()) } fn run_command_with_timeout( @@ -781,7 +1343,7 @@ fn run_command_with_timeout( args: &[&str], stdin: Option<&[u8]>, command_timeout: Duration, -) -> Result<()> { +) -> Result { let mut child = Command::new(program) .args(args) .stdin(if stdin.is_some() { @@ -813,7 +1375,7 @@ fn run_command_with_timeout( let error = String::from_utf8_lossy(&output.stderr); bail!("{} failed: {}", Path::new(program).display(), error.trim()); } - Ok(()) + Ok(String::from_utf8_lossy(&output.stdout).into_owned()) } fn require_executable(path: &str) -> Result<()> { @@ -837,6 +1399,163 @@ fn prepare_socket_path(socket: &Path) -> Result<()> { Ok(()) } +/// A netd that exists only to be talked to. +/// +/// The VMM's side of this protocol -- what it falls back to, what it refuses, +/// what it does when the answer is missing -- had no test at all, because +/// every path needed a privileged daemon. It does not: it needs something that +/// answers on a socket. This is that, scripted per behaviour, recording what +/// it was asked so a test can assert on the conversation rather than on its +/// effects. +#[cfg(test)] +pub(crate) mod testing { + use std::{ + path::{Path, PathBuf}, + sync::{Arc, Mutex}, + }; + + use serde_json::{json, Value}; + use tokio::{ + io::{AsyncReadExt, AsyncWriteExt}, + net::UnixListener, + }; + + /// How the fake answers. + #[derive(Debug, Clone)] + pub(crate) enum Behavior { + /// Answers every listed operation with a plausible success, and + /// anything else with the error `serde` produces for an unknown one. + Handles(Vec), + /// An older netd: every operation this one added is an error. + Legacy, + } + + impl Behavior { + pub(crate) fn handling(operations: &[&str]) -> Self { + Self::Handles(operations.iter().map(|name| name.to_string()).collect()) + } + } + + pub(crate) struct FakeNetd { + _dir: tempfile::TempDir, + socket: PathBuf, + seen: Arc>>, + } + + impl FakeNetd { + pub(crate) fn spawn(behavior: Behavior) -> Self { + Self::spawn_holding(behavior, Vec::new()) + } + + /// A netd that holds these interfaces, whatever else it does. + pub(crate) fn spawn_holding(behavior: Behavior, interfaces: Vec) -> Self { + Self::start(behavior, interfaces) + } + + fn start(behavior: Behavior, interfaces: Vec) -> Self { + let dir = tempfile::tempdir().expect("tempdir"); + let socket = dir.path().join("netd.sock"); + let listener = UnixListener::bind(&socket).expect("bind"); + let seen = Arc::new(Mutex::new(Vec::new())); + let recorder = seen.clone(); + let interfaces = std::sync::Arc::new(interfaces); + tokio::spawn(async move { + loop { + let Ok((mut stream, _)) = listener.accept().await else { + return; + }; + let behavior = behavior.clone(); + let interfaces = interfaces.clone(); + let recorder = recorder.clone(); + tokio::spawn(async move { + let mut message = Vec::new(); + if stream.read_to_end(&mut message).await.is_err() { + return; + } + let Ok(request) = serde_json::from_slice::(&message) else { + return; + }; + recorder.lock().expect("poisoned").push(request.clone()); + let response = answer(&behavior, &interfaces, &request); + let _ = stream + .write_all(&serde_json::to_vec(&response).unwrap()) + .await; + let _ = stream.shutdown().await; + }); + } + }); + Self { + _dir: dir, + socket, + seen, + } + } + + pub(crate) fn socket(&self) -> &Path { + &self.socket + } + + /// Every request it was sent, in order. + pub(crate) fn seen(&self) -> Vec { + self.seen.lock().expect("poisoned").clone() + } + + pub(crate) fn operations(&self) -> Vec { + self.seen() + .iter() + .map(|request| request["operation"].as_str().unwrap_or("?").to_string()) + .collect() + } + } + + fn answer(behavior: &Behavior, interfaces: &[Value], request: &Value) -> Value { + let operation = request["operation"].as_str().unwrap_or_default(); + let operations = match behavior { + Behavior::Legacy => { + return match operation { + // What the real thing answers for an operation it knows. + "prepare_bridge" | "prepare_macvtap" | "remove" | "check" => { + json!({"ok": true, "tap": "dtdeadbeef00"}) + } + other => json!({ + "ok": false, + "error": format!("invalid netd request: unknown variant `{other}`"), + }), + }; + } + Behavior::Handles(operations) => operations, + }; + if !operations.iter().any(|name| name == operation) { + return json!({ + "ok": false, + "error": format!("invalid netd request: unknown variant `{operation}`"), + }); + } + match operation { + "prepare_bridge" | "prepare_macvtap" => json!({ + "ok": true, + "tap": "dtdeadbeef00", + "queues": request["queues"].as_u64().unwrap_or(1).max(1), + }), + "remove" | "check" => json!({"ok": true, "tap": "dtdeadbeef00"}), + "remove_all" => json!({"ok": true, "removed": 0, "incomplete": false}), + "list" => { + let instance = request["instance_id"].as_str().unwrap_or_default(); + let held: Vec = interfaces + .iter() + .filter(|record| { + instance.is_empty() || record["instance_id"].as_str() == Some(instance) + }) + .cloned() + .collect(); + json!({"ok": true, "interfaces": held}) + } + "remove_interface" => json!({"ok": true, "tap": request["tap"]}), + _ => json!({"ok": true}), + } + } +} + #[cfg(test)] mod tests { use super::*; @@ -868,6 +1587,7 @@ mod tests { qemu_uid: 1000, filtered: true, queues: 0, + workdir: String::new(), }; let filter = NetworkFilterConfig { mode: crate::config::NetworkFilterMode::Libvirt, @@ -895,7 +1615,6 @@ mod tests { fn remove_protocol_keeps_identity_fields_flat() { let request = Request::Remove { identity: identity("instance", "vm", 2), - filtered: true, }; let value = serde_json::to_value(request).unwrap(); assert_eq!(value["operation"], "remove"); @@ -914,6 +1633,7 @@ mod tests { qemu_uid: 1000, filtered: true, queues: 0, + workdir: String::new(), }); let value = serde_json::to_value(request).unwrap(); assert_eq!(value["operation"], "prepare_bridge"); @@ -922,50 +1642,6 @@ mod tests { assert!(value.get("identity").is_none()); } - /// `filtered` says which of two shapes was built, and both are reachable - /// on any node this build can produce. There is no released peer that omits - /// it -- netd does not exist before v0.6 -- so it is required rather than - /// defaulted, and a request that leaves it out is a bug, not an old client. - #[test] - fn removal_states_which_shape_it_is_undoing() { - let error = serde_json::from_value::(serde_json::json!({ - "operation": "remove", - "instance_id": "instance", - "vm_id": "vm", - "nic_index": 0, - })) - .unwrap_err(); - assert!(error.to_string().contains("filtered"), "{error}"); - - for filtered in [true, false] { - let decoded: Request = serde_json::from_value(serde_json::json!({ - "operation": "remove", - "instance_id": "instance", - "vm_id": "vm", - "nic_index": 0, - "filtered": filtered, - })) - .unwrap(); - let Request::Remove { - filtered: decoded, .. - } = decoded - else { - panic!("wrong variant"); - }; - assert_eq!(decoded, filtered); - } - } - - /// A binding outlives the interface it was bound to, and TAP names are a - /// deterministic hash of the VM identity, so the same name comes back. - /// Removing an interface therefore clears whatever binding is there, and - /// only insists when the caller is about to create a replacement. - #[test] - fn binding_cleanup_insists_only_when_a_replacement_follows() { - assert_eq!(binding_cleanup(true), BindingCleanup::Required); - assert_eq!(binding_cleanup(false), BindingCleanup::BestEffort); - } - #[test] fn queue_counts_normalize_to_at_least_one_and_stay_bounded() { assert_eq!(validate_queues(0).unwrap(), 1); @@ -983,6 +1659,7 @@ mod tests { qemu_uid: 1000, filtered: false, queues: 4, + workdir: String::new(), }); let value = serde_json::to_value(request).unwrap(); assert_eq!(value["queues"], 4); @@ -1015,6 +1692,7 @@ mod tests { qemu_uid: 1000, mode: "private".into(), queues: 0, + workdir: String::new(), }); let value = serde_json::to_value(request).unwrap(); assert_eq!(value["operation"], "prepare_macvtap"); @@ -1032,6 +1710,7 @@ mod tests { qemu_uid: 1000, filtered: true, queues: 0, + workdir: String::new(), }); let value = serde_json::to_value(request).unwrap(); assert_eq!(value["operation"], "prepare_bridge"); @@ -1052,7 +1731,7 @@ mod tests { drop(client); let result = timeout( Duration::from_secs(1), - serve_connection(&NetdConfig::default(), &mut server), + serve_connection(&std::sync::Arc::new(NetdConfig::default()), &mut server), ) .await; assert!(result.is_ok(), "disconnected peer blocked the handler"); @@ -1075,7 +1754,7 @@ mod tests { .await .unwrap(); client.shutdown().await.unwrap(); - serve_connection(&NetdConfig::default(), &mut server) + serve_connection(&std::sync::Arc::new(NetdConfig::default()), &mut server) .await .unwrap(); @@ -1108,6 +1787,7 @@ mod tests { qemu_uid: 1000, filtered: false, queues: 4, + workdir: String::new(), }; let filtering = NetworkFilterConfig { mode: crate::config::NetworkFilterMode::Libvirt, @@ -1158,6 +1838,7 @@ mod tests { qemu_uid: 1000, mode: "bridge".into(), queues: 4, + workdir: String::new(), }; let error = match prepare_macvtap("test:///default", &request, &filtering) { Err(error) => error, @@ -1179,6 +1860,7 @@ mod tests { qemu_uid: 1000, filtered: true, queues: 1, + workdir: String::new(), }; // Nothing on the wire can name a filter: the field does not exist. let wire = serde_json::to_value(Request::PrepareBridge(request.clone())).unwrap(); @@ -1208,4 +1890,306 @@ mod tests { assert!(error.to_string().contains("timed out")); assert!(started.elapsed() < Duration::from_secs(2)); } + + #[test] + fn the_workdir_travels_but_older_callers_may_omit_it() { + let request = Request::PrepareBridge(PrepareBridgeRequest { + identity: identity("instance", "vm", 0), + bridge: "br0".into(), + mac: "02:00:00:00:00:01".into(), + qemu_uid: 1000, + filtered: true, + queues: 1, + workdir: "/opt/dstack/run/vm/vm".into(), + }); + let value = serde_json::to_value(request).unwrap(); + assert_eq!(value["workdir"], "/opt/dstack/run/vm/vm"); + // It is a log line, not an input, so a caller that never sets it is not + // asking for anything different. + let Request::PrepareBridge(decoded) = decode_minimal_bridge() else { + panic!("expected a bridge prepare"); + }; + assert_eq!(decoded.workdir, ""); + } + + /// A prepare carrying only the fields that predate this change. + fn decode_minimal_bridge() -> Request { + serde_json::from_value(serde_json::json!({ + "operation": "prepare_bridge", + "instance_id": "instance", + "vm_id": "vm", + "nic_index": 0, + "bridge": "br0", + "mac": "02:00:00:00:00:01", + "qemu_uid": 1000, + "filtered": true, + "queues": 1, + })) + .unwrap() + } + + #[test] + fn a_whole_vm_sweep_needs_no_record_of_what_it_is_deleting() { + let value = serde_json::to_value(Request::RemoveAll { + instance_id: "instance".into(), + vm_id: "vm".into(), + }) + .unwrap(); + assert_eq!(value["operation"], "remove_all"); + assert_eq!(value["instance_id"], "instance"); + assert_eq!(value["vm_id"], "vm"); + // No NIC index: the point is reaching the ones the caller can no longer + // name, so it names none and netd derives the whole space instead. + assert!(value.get("nic_index").is_none()); + + // That space is bounded by what an identity may say, which is what + // makes deriving it cheap enough to do on every launch. + let mut identity = identity("instance", "vm", MAX_NIC_INDEX); + assert!(validate_identity(&identity).is_ok()); + identity.nic_index = MAX_NIC_INDEX + 1; + assert!(validate_identity(&identity).is_err()); + } + + /// A sweep names no single interface, so its answer must not carry an + /// empty one. `request()` reads a blank `tap` as a malformed response, and + /// a third-party netd copying this shape would have to send a field that + /// means nothing. + #[test] + fn a_sweep_reports_a_count_and_no_interface() { + let value = serde_json::to_value(Outcome::Swept { removed: 3 }.into_response()).unwrap(); + assert_eq!(value["removed"], 3); + assert!(value.get("tap").is_none()); + + // A prepare still names one, because the caller hands that name to QEMU. + let value = serde_json::to_value(Outcome::tap("dtabc".into()).into_response()).unwrap(); + assert_eq!(value["tap"], "dtabc"); + assert!(value.get("removed").is_none()); + } + + /// The record is a hint; the name is the proof. Everything that can go + /// wrong with reading a string off an interface -- forged, truncated, + /// ambiguous, absent -- has to land in the same bucket, and it has to be + /// the bucket a collection treats conservatively. + #[test] + fn an_interface_says_whose_it_is_and_the_name_is_what_proves_it() { + let nic = identity("path-abc", "vm-1", 3); + let tap = tap_name(&nic); + let alias = interface_alias(&nic); + assert_eq!(alias, "dstack1:3:path-abc:vm-1"); + + let owner = owner_of(&tap, &alias).expect("its own record checks out"); + assert_eq!(owner.instance_id, "path-abc"); + assert_eq!(owner.vm_id, "vm-1"); + assert_eq!(owner.nic_index, 3); + + // A record naming some other interface proves nothing about this one. + // This is what makes the record unforgeable without making it + // authoritative: anything that can reach the socket can write a + // string, but only the true identity re-derives the name. + let forged = interface_alias(&identity("path-abc", "someone-elses-vm", 3)); + assert!(owner_of(&tap, &forged).is_none()); + assert!(owner_of(&tap, "").is_none()); + assert!(owner_of(&tap, "dstack1:3:path-abc").is_none()); + assert!(owner_of(&tap, &alias[..alias.len() - 2]).is_none()); + // A format this build does not know is not this format. + assert!(owner_of(&tap, &alias.replace("dstack1", "dstack2")).is_none()); + + // The two free-form fields are last and only the first of them has to + // be unambiguous, so a VM ID carrying the separator still reads back. + let odd = identity("path-abc", "vm:with:colons", 0); + assert_eq!( + owner_of(&tap_name(&odd), &interface_alias(&odd)).map(|owner| owner.vm_id), + Some("vm:with:colons".to_string()) + ); + // An instance ID carrying it is refused instead of mis-parsed. + assert!(validate_identity(&identity("path:abc", "vm-1", 0)).is_err()); + } + + /// An identity that cannot be recorded would produce an interface nothing + /// can attribute, which is the state this whole path exists to stop + /// creating. Refusing it is the only answer that keeps the invariant. + #[test] + fn an_identity_too_long_to_record_is_refused() { + let long = "v".repeat(128); + assert!(validate_identity(&identity("instance", &long, 0)).is_ok()); + let identity_too_long = identity(&"i".repeat(128), &long, 255); + assert!(interface_alias(&identity_too_long).len() > MAX_IFALIAS); + let error = validate_identity(&identity_too_long) + .unwrap_err() + .to_string(); + assert!(error.contains("too long to record"), "{error}"); + } + + /// The name space netd claims. A collection deletes what matches, so what + /// matches has to be exactly what netd can produce. + #[test] + fn the_managed_name_space_is_exactly_what_netd_produces() { + assert!(is_managed_name(&tap_name(&identity("instance", "vm", 0)))); + assert!(is_managed_name("dt0123456789ab")); + assert!(!is_managed_name("dt0123456789AB"), "digests are lower case"); + assert!(!is_managed_name("dt0123456789a"), "one short"); + assert!(!is_managed_name("dt0123456789abc"), "one long"); + assert!(!is_managed_name("dtzzzzzzzzzzzz")); + assert!(!is_managed_name("virbr0")); + assert!(!is_managed_name("eth0")); + // The whole space fits in IFNAMSIZ, or the kernel would refuse the + // names this reserves. + assert!(tap_name(&identity("instance", "vm", 255)).len() < 16); + } + + /// The command prints a table for a human and accepts no options to make it + /// print anything else, so this parses one. Narrowing to netd's own name + /// space is what makes that safe. + #[test] + fn the_binding_listing_reads_a_table_meant_for_a_person() { + let output = "\ + Port Dev Filter +--------------------------------- + dt1e053266e9f7 clean-traffic + dt28b105b3031a clean-traffic + vnet3 some-other-filter +"; + let names: HashSet = output + .lines() + .filter_map(|line| line.split_whitespace().next()) + .filter(|name| is_managed_name(name)) + .map(str::to_string) + .collect(); + assert_eq!(names.len(), 2); + assert!(names.contains("dt1e053266e9f7")); + // The header, the rule, and a binding that is not netd's all fall out. + assert!(!names.contains("Port")); + assert!(!names.contains("vnet3")); + } + + /// Everything else here reasons about strings. This puts the reasoning + /// next to the kernel: that an alias survives on a device netd actually + /// creates, that enumeration finds it, that the guards refuse what they + /// are meant to, and that removal leaves nothing. + /// + /// Refuses to run in the host's network namespace, so it cannot touch a + /// real node's interfaces even when it fails. Unsharing one from inside + /// the test is not enough: `/sys/class/net` keeps showing the old + /// namespace until sysfs is remounted, which is most of what `ip netns + /// exec` does. So it asks to be put in one: + /// + /// ```text + /// cargo test -p dstack-vmm --bins --no-run + /// sudo ip netns add dstack-netd-test + /// sudo ip netns exec dstack-netd-test \ + /// target/debug/deps/dstack_vmm- --ignored --test-threads=1 + /// sudo ip netns del dstack-netd-test + /// ``` + #[test] + #[ignore = "needs root and its own network namespace; see the doc comment"] + fn a_real_interface_carries_its_record_and_removal_leaves_nothing() { + assert!( + nix::unistd::Uid::effective().is_root(), + "this test needs root" + ); + let (mine, init) = ( + std::fs::read_link("/proc/self/ns/net").unwrap(), + std::fs::read_link("/proc/1/ns/net").unwrap(), + ); + assert_ne!( + mine, init, + "run this inside its own network namespace; it creates and deletes interfaces" + ); + // Nothing in this namespace to talk to, which is also the state of a + // node that does not filter: the listing has to work without libvirt. + let uri = "qemu:///nonexistent-for-this-test"; + + let nic = identity("test-instance", "vm-1", 2); + let tap = tap_name(&nic); + ip(&["tuntap", "add", "dev", &tap, "mode", "tap"]).unwrap(); + set_alias(&tap, &nic).unwrap(); + assert!(is_tuntap(&tap), "the kernel publishes tun_flags for a TAP"); + + let records = list_interfaces(uri, ""); + let record = records + .iter() + .find(|record| record.tap == tap) + .expect("an interface netd created is one netd can find"); + assert_eq!(record.instance_id.as_deref(), Some("test-instance")); + assert_eq!(record.vm_id.as_deref(), Some("vm-1")); + assert_eq!(record.nic_index, Some(2)); + assert_eq!(record.kind, "tap"); + // Narrowing by instance is what keeps one VMM's collection off + // another's interfaces. + assert_eq!(list_interfaces(uri, "test-instance").len(), 1); + assert!(list_interfaces(uri, "someone-else").is_empty()); + + // A device with one of netd's names that netd did not create. The name + // is 48 bits of digest, so this is not about collisions -- it is that + // `ip link delete` does not ask what it is deleting, and netd runs as + // root. + let impostor = tap_name(&identity("test-instance", "not-a-tap", 0)); + ip(&["link", "add", &impostor, "type", "dummy"]).unwrap(); + assert!(is_managed_name(&impostor)); + assert!( + !list_interfaces(uri, "") + .iter() + .any(|record| record.tap == impostor), + "a device netd did not create is not offered up for collection" + ); + let refused = remove_interface(uri, &impostor, BindingCleanup::Skip).unwrap_err(); + assert!(refused.to_string().contains("refusing to delete")); + + // An interface whose record does not re-derive its own name proves + // nothing, and lands in the same bucket as no record at all. + ip(&[ + "link", + "set", + "dev", + &tap, + "alias", + "dstack1:2:test-instance:some-other-vm", + ]) + .unwrap(); + let records = list_interfaces(uri, ""); + let record = records.iter().find(|record| record.tap == tap).unwrap(); + assert!(record.instance_id.is_none(), "a forged record is no record"); + + remove_interface(uri, &tap, BindingCleanup::Skip).unwrap(); + assert!(!Path::new("/sys/class/net").join(&tap).exists()); + assert!(!list_interfaces(uri, "") + .iter() + .any(|record| record.tap == tap)); + // Removing what is not there is not an error: a sweep derives names + // and most of them miss. + remove_interface(uri, &tap, BindingCleanup::Skip).unwrap(); + } + /// A netd that answers a sweep with no count did not sweep. Reading the + /// absent field as zero is the same conflation `queues` is shaped to + /// avoid, and here it would report a netd that cannot collect a VM's + /// interfaces as a VM that had none. + #[tokio::test] + async fn a_sweep_without_a_count_is_not_read_as_an_empty_one() { + // Claims the operation, answers without the field. + let netd = testing::FakeNetd::spawn(testing::Behavior::handling(&[])); + let error = remove_all(netd.socket(), "instance", "vm") + .await + .expect_err("an answer with no count is not a successful sweep"); + assert!(!is_unreachable(&error)); + + let netd = testing::FakeNetd::spawn(testing::Behavior::handling(&["remove_all"])); + let removed = remove_all(netd.socket(), "instance", "vm").await.unwrap(); + assert_eq!(removed, 0); + } + + /// Every "an unreachable netd is not a failure" branch in the VMM hangs + /// off this one predicate, and a marker attached as context is not a link + /// in the source chain. + #[test] + fn an_unreachable_netd_is_recognized_through_the_contexts_stacked_on_it() { + let error = anyhow::Error::from(std::io::Error::from(std::io::ErrorKind::NotFound)) + .context(Unreachable) + .context("failed to connect to netd at /run/netd.sock") + .context("failed to prepare netd-managed networking"); + assert!(is_unreachable(&error)); + + let other = anyhow::anyhow!("netd remove_all failed: no such bridge") + .context("failed to prepare netd-managed networking"); + assert!(!is_unreachable(&other)); + } } diff --git a/dstack/vmm/src/one_shot.rs b/dstack/vmm/src/one_shot.rs index d7500b008..62c4bd78b 100644 --- a/dstack/vmm/src/one_shot.rs +++ b/dstack/vmm/src/one_shot.rs @@ -3,9 +3,8 @@ // SPDX-License-Identifier: Apache-2.0 use crate::app::{ - clamp_queues_without_netd, make_sys_config, needs_netd_interface, resolved_networks, - settle_vhost, simulator_config_for_manifest, sync_tee_simulator_config, Image, VmConfig, - VmWorkDir, + make_sys_config, needs_netd_interface, resolved_networks, settle_vhost, + simulator_config_for_manifest, sync_tee_simulator_config, Image, VmConfig, VmWorkDir, }; use crate::config::Config; use crate::main_service; @@ -280,42 +279,19 @@ Compose file content (first 200 chars): gateway_enabled: app_compose.gateway_enabled(), }; - // One-shot has no netd lifecycle, so a bridge NIC that only wanted the - // vCPU-scaled default drops to a single queue here exactly as it would on a - // server without netd. Anything still needing an interface was asked for - // explicitly, and is refused rather than silently downgraded. - let requested = if manifest.networks.is_empty() { - vec![config.cvm.networking.nic.clone()] - } else { - manifest.networks.clone() - }; let mut runtime_networks = resolved_networks(&manifest, &config.cvm); - let clamped = clamp_queues_without_netd(&requested, &mut runtime_networks, &config.cvm, false); - // The server settles vhost after clamping, because clamping changes whether - // a NIC needs netd and that changes which netdev it gets. Skipping it here - // left `vhost_enabled()` reading as a request rather than a decision, so - // the launch warned about a `/dev/vhost-net` the netdev it then built does - // not open. - let vhost_denied = settle_vhost(&mut runtime_networks, &config.cvm); - if vhost_denied > 0 { - tracing::warn!( - "no qemu-bridge-helper found, so {vhost_denied} bridge interface(s) fall back to the \ - non-vhost bridge netdev; set cvm.qemu_bridge_helper to enable vhost" - ); - } - if clamped > 0 { - tracing::warn!( - "one-shot execution has no netd, so {clamped} bridge interface(s) fall back to a \ - single queue pair; run the VMM server to let queue pairs scale with vCPUs" - ); - } - if !dry_run - && runtime_networks - .iter() - .any(|network| needs_netd_interface(network, &config.cvm)) - { + // Settle the data plane before anything reads it, so `vhost_enabled()` is a + // decision rather than a request and the launch does not warn about a + // `/dev/vhost-net` the netdev it then builds never opens. + settle_vhost(&mut runtime_networks); + // Bridge and macvtap host interfaces belong to netd, whose lifecycle + // one-shot does not manage. Refusing is the honest answer: the alternative + // was to quietly build a different interface here than the server would, + // and then report the VM as if it had the one it asked for. + if !dry_run && runtime_networks.iter().any(needs_netd_interface) { anyhow::bail!( - "one-shot execution does not manage netd interface lifecycle; run the VMM server directly or use --dry-run" + "one-shot execution does not manage netd interface lifecycle, which bridge and \ + macvtap networking need; run the VMM server directly or use --dry-run" ); } diff --git a/dstack/vmm/src/vmm-cli.py b/dstack/vmm/src/vmm-cli.py index 20e515520..f4e1a7b52 100755 --- a/dstack/vmm/src/vmm-cli.py +++ b/dstack/vmm/src/vmm-cli.py @@ -321,17 +321,30 @@ def encrypt_env(envs, hex_public_key: str) -> str: def parse_port_mapping(port_str: str) -> Dict: - """Parse a port mapping string into a dictionary.""" + """Parse a port mapping string into a dictionary. + + Accepts an optional "@" suffix naming which NIC the traffic enters + through. Without it the VMM picks: the first user-mode NIC, else the first + bridge NIC. A single-NIC VM never needs it. + """ + nic_index = None + if "@" in port_str: + port_str, _, nic = port_str.rpartition("@") + # `int()` alone would take "1_0" as 10, " 1" as 1, and "+1" as 1. A NIC + # index is a position in a list the user wrote, so only digits are it. + if not (nic.isascii() and nic.isdigit()): + raise argparse.ArgumentTypeError(f"Invalid NIC index: {nic}") + nic_index = int(nic) parts = port_str.split(":") if len(parts) == 3: - return { + mapping = { "protocol": parts[0], "host_address": "127.0.0.1", "host_port": int(parts[1]), "vm_port": int(parts[2]), } elif len(parts) == 4: - return { + mapping = { "protocol": parts[0], "host_address": parts[1], "host_port": int(parts[2]), @@ -339,6 +352,9 @@ def parse_port_mapping(port_str: str) -> Dict: } else: raise argparse.ArgumentTypeError(f"Invalid port mapping format: {port_str}") + if nic_index is not None: + mapping["nic_index"] = nic_index + return mapping def read_utf8(filepath: str) -> str: @@ -1907,7 +1923,7 @@ def _patched_format_help(): "--port", action="append", type=str, - help="Port mapping in format: protocol[:address]:from:to", + help="Port mapping in format: protocol[:address]:from:to[@nic]", ) deploy_parser.add_argument( "--gpu", @@ -2063,7 +2079,7 @@ def _patched_format_help(): action="append", type=str, required=True, - help="Port mapping in format: protocol[:address]:from:to (can be used multiple times)", + help="Port mapping in format: protocol[:address]:from:to[@nic] (can be used multiple times)", ) # Update (all-in-one) command @@ -2133,7 +2149,7 @@ def _patched_format_help(): "--port", action="append", type=str, - help="Port mapping in format: protocol[:address]:from:to (can be used multiple times)", + help="Port mapping in format: protocol[:address]:from:to[@nic] (can be used multiple times)", ) port_group.add_argument( "--no-ports", diff --git a/dstack/vmm/ui/src/components/CreateVmDialog.ts b/dstack/vmm/ui/src/components/CreateVmDialog.ts index 27adcaecd..b07b36255 100644 --- a/dstack/vmm/ui/src/components/CreateVmDialog.ts +++ b/dstack/vmm/ui/src/components/CreateVmDialog.ts @@ -293,7 +293,10 @@ const CreateVmDialogComponent = {
- +
diff --git a/dstack/vmm/ui/src/components/PortMappingEditor.ts b/dstack/vmm/ui/src/components/PortMappingEditor.ts index 0668bd952..70a4ad278 100644 --- a/dstack/vmm/ui/src/components/PortMappingEditor.ts +++ b/dstack/vmm/ui/src/components/PortMappingEditor.ts @@ -8,6 +8,10 @@ type PortEntry = { host_port: number | null; vm_port: number | null; custom_ip?: string; // User-entered IP for custom mode + // Which NIC the traffic enters through. Blank lets the VMM pick: the first + // user-mode NIC, else the first bridge NIC. A single-NIC VM never needs it, + // which is why the field only appears once a VM has more than one. + nic_index?: number | string | null; }; // ... keep your types as-is ... @@ -21,6 +25,9 @@ const PortMappingEditorComponent = { name: 'PortMappingEditor', props: { ports: { type: Array, required: true }, + // How many NICs the VM has. One NIC has nothing to choose between, so the + // column stays hidden rather than offering a pin that can only be 0. + nicCount: { type: Number, default: 1 }, }, // normalize on initial load @@ -63,6 +70,15 @@ const PortMappingEditorComponent = { + @@ -95,6 +111,7 @@ const PortMappingEditorComponent = { custom_ip: '', host_port: null, vm_port: null, + nic_index: null, }); }, diff --git a/dstack/vmm/ui/src/components/UpdateVmDialog.ts b/dstack/vmm/ui/src/components/UpdateVmDialog.ts index ec2bfbfa6..4fa40e493 100644 --- a/dstack/vmm/ui/src/components/UpdateVmDialog.ts +++ b/dstack/vmm/ui/src/components/UpdateVmDialog.ts @@ -176,7 +176,10 @@ const UpdateVmDialogComponent = {
- +
diff --git a/dstack/vmm/ui/src/composables/useVmManager.ts b/dstack/vmm/ui/src/composables/useVmManager.ts index 70e9db9df..12bbbf6de 100644 --- a/dstack/vmm/ui/src/composables/useVmManager.ts +++ b/dstack/vmm/ui/src/composables/useVmManager.ts @@ -107,6 +107,15 @@ type PortFormEntry = { host_address?: string; host_port?: number | null; vm_port?: number | null; + /** + * Which NIC this mapping's traffic enters through. Unset lets the VMM pick. + * Carried through edits unchanged: `GetInfo` reports it and this form sends + * the whole list back, so dropping it here would silently unpin a mapping + * whenever anyone touched an unrelated field. + */ + // `v-model.number` leaves the raw string here when it does not parse, so + // an emptied box is `''` rather than `null`. See `normalizePorts`. + nic_index?: number | string | null; }; type NetworkFormEntry = { @@ -435,6 +444,7 @@ fi host_address: port.host_address || '127.0.0.1', host_port: typeof port.host_port === 'number' ? port.host_port : null, vm_port: typeof port.vm_port === 'number' ? port.vm_port : null, + nic_index: typeof port.nic_index === 'number' ? port.nic_index : null, })); const normalizePorts = (ports: PortFormEntry[] = []): VmmTypes.IPortMapping[] => @@ -445,11 +455,23 @@ fi port.host_port === null || port.host_port === undefined ? Number.NaN : Number(port.host_port); const vmPort = port.vm_port === null || port.vm_port === undefined ? Number.NaN : Number(port.vm_port); + // An unpinned mapping must stay unpinned rather than become NIC 0: + // the VMM's own default is the first user-mode NIC, not the first NIC. + // `v-model.number` hands back the raw string when it does not parse, + // so a box the operator cleared arrives as `''`. `Number('')` is 0, + // which would pin to NIC 0 the mapping they just unpinned. + const nicIndex = + port.nic_index === null || port.nic_index === undefined || port.nic_index === '' + ? undefined + : Number(port.nic_index); return { protocol, host_address: (port.host_address || '127.0.0.1').trim() || '127.0.0.1', host_port: hostPort, vm_port: vmPort, + ...(Number.isInteger(nicIndex) && (nicIndex as number) >= 0 + ? { nic_index: nicIndex } + : {}), }; }) .filter( @@ -457,13 +479,7 @@ fi port.protocol.length > 0 && Number.isFinite(port.host_port) && Number.isFinite(port.vm_port), - ) - .map((port) => ({ - protocol: port.protocol, - host_address: port.host_address, - host_port: port.host_port, - vm_port: port.vm_port, - })); + ); const cloneNetworks = (configuration?: VmConfiguration | null): NetworkFormEntry[] => { const configured = configuration?.networks && configuration.networks.length > 0 diff --git a/dstack/vmm/vmm.toml b/dstack/vmm/vmm.toml index 3a83d5016..9ef0ecec4 100644 --- a/dstack/vmm/vmm.toml +++ b/dstack/vmm/vmm.toml @@ -46,7 +46,11 @@ max_allocable_memory_in_mb = 100_000 # MB qmp_socket = false # The user to run the VM as. If empty, the VM will be run as the current user. # Unique namespace when multiple dstack-vmm instances share one host. When -# empty, a stable value is derived from run_path. +# empty, a stable value is derived from run_path. It is what netd records on +# every host interface this VMM asks for, and the name space those interfaces' +# names are derived in. Two VMMs on one host must not share one: each would +# build interfaces at names the other can also produce. Left empty it is +# derived from run_path, which cannot collide. May not contain ":". instance_id = "" # Network choices that deployment RPC callers may make. Macvtap is excluded # by default because libvirt nwfilter applies only to bridge interfaces. @@ -58,11 +62,7 @@ allowed_macvtap_parents = [] # vhost on, queue pairs otherwise default to the VM's vCPU count, capped at # 16 (without vhost the default is a single queue pair); raising this # above 16 widens what a caller may request without moving that default, and -# lowering it below 16 lowers the default too. Bridge mode needs netd for -# anything above 1, because qemu-bridge-helper cannot create a multiqueue TAP. -# Without netd an unfiltered bridge NIC that took the default drops to one -# queue; one that asked for a count keeps it and fails to launch instead, so -# the caller learns their request was not met. +# lowering it below 16 lowers the default too. max_net_queues = 16 use_mrconfigid = true @@ -72,10 +72,6 @@ use_mrconfigid = true #qemu_version = "" qemu_pci_hole64_size = 0 qemu_hotplug_off = false -# Path to qemu-bridge-helper, needed by vhost bridge networking because QEMU's -# `tap` netdev, unlike its `bridge` netdev, has no compiled-in default. Empty -# probes the known distribution locations. -#qemu_bridge_helper = "/usr/lib/qemu/qemu-bridge-helper" # TDX attestation/hash scheme policy: # - "legacy": digest.txt + legacy verifier # - "lite": digest.txt + measurement.tdx.cbor + no-QEMU verifier @@ -144,17 +140,18 @@ restrict = false # bridge = "virbr0" # Optional filtering for bridge interfaces only. It does not apply to macvtap. -# "none" installs no nwfilter binding. It does not by itself remove the netd -# dependency: netd also builds the multiqueue TAP that qemu-bridge-helper -# cannot create, so a bridge node without netd is limited to one queue pair. +# "none" installs no nwfilter binding. It does not remove the netd dependency: +# netd builds every bridge and macvtap host interface whatever this says. [cvm.network_filter] mode = "none" filter = "clean-traffic" parameters = {} -# Shared privileged networking service. Used for macvtap NICs, for libvirt -# filtering, and for multiqueue bridge NICs. Socket filesystem permissions -# authorize clients. +# Shared privileged networking service. Required by bridge and macvtap +# networking: it builds every host interface those modes use, binds their +# nwfilters, and releases them again. It does not forward host ports. User mode +# and a caller-supplied netdev need nothing from it. Socket filesystem +# permissions authorize clients. [netd] socket = "/run/dstack/netd.sock" # Applied when netd creates the socket itself. A systemd socket unit controls