diff --git a/docs/bridge-networking.md b/docs/bridge-networking.md index 6bf3e6d5e..5724e719d 100644 --- a/docs/bridge-networking.md +++ b/docs/bridge-networking.md @@ -199,6 +199,30 @@ 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. + ### 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..e2ee77778 100644 --- a/docs/libvirt-network-filter.md +++ b/docs/libvirt-network-filter.md @@ -105,6 +105,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, diff --git a/dstack/vmm/rpc/proto/vmm_rpc.proto b/dstack/vmm/rpc/proto/vmm_rpc.proto index 31fef51f4..239ac2d79 100644 --- a/dstack/vmm/rpc/proto/vmm_rpc.proto +++ b/dstack/vmm/rpc/proto/vmm_rpc.proto @@ -182,6 +182,15 @@ 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 failing that the first bridge NIC. + // + // 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..528cd44b9 100644 --- a/dstack/vmm/src/app.rs +++ b/dstack/vmm/src/app.rs @@ -46,9 +46,9 @@ 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, - validate_resolved_networks, + clamp_queues_without_netd, filters_bridge_traffic, ingress_for, needs_netd_interface, + netd_available, netd_teardown, resolve_networking, resolved_networks, settle_vhost, + validate_resolved_network, validate_resolved_networks, }; pub use qemu::VmConfig; // Exported so the RPC layer can assert that everything it reports is @@ -97,6 +97,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` @@ -563,7 +567,39 @@ impl App { { return Ok(()); } + // Whatever an earlier boot left behind, from a crash between creating + // an interface and recording it or from a NIC this VM no longer has. + // Prepare replaces the names it is about to use, but only those; an + // index nothing will claim again is only reachable from here. + if let Err(error) = netd::remove_all( + &self.config.netd.socket, + &self.config.cvm.instance_id, + &vm.manifest.id, + ) + .await + { + if !netd::is_unreachable(&error) { + warn!(vm_id = %vm.manifest.id, %error, "failed to sweep stale netd interfaces"); + } + } + // Resolved before the loop borrows `networks` mutably, and once rather + // than per NIC, so both the request and the warning below read the same + // answer. + let ingress: Vec> = (0..networks.len()) + .map(|nic_index| ingress_for(&vm.manifest.port_map, networks, nic_index)) + .collect(); 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) { @@ -593,6 +629,12 @@ impl App { // libvirt at all. filtered, queues, + workdir: workdir.clone(), + // Only the mappings that resolve to this NIC. One mapping + // lands on exactly one, and a user-mode NIC's are emitted + // as QEMU `hostfwd=` instead, so no host port is claimed + // twice. + ingress: ingress[nic_index].clone(), }), NetworkingMode::Macvtap => NetdRequest::PrepareMacvtap(PrepareMacvtapRequest { identity: identity.clone(), @@ -601,6 +643,7 @@ impl App { qemu_uid, mode: network.macvtap_mode.clone(), queues, + workdir: workdir.clone(), }), NetworkingMode::User | NetworkingMode::Custom => continue, }; @@ -678,6 +721,18 @@ impl App { } Ok(()) })(); + // Ports asked for and not answered for used to vanish in silence: + // no warning, and `GetInfo` still listing them. A netd that forwards + // says what it built, so nothing said means nothing forwarded. + let asked = ingress[nic_index].len(); + if asked > 0 && response.ingress.is_none() { + warn!( + vm_id = %vm.manifest.id, + ports = asked, + "netd on this node does not forward host ports, so this VM's \ + port mappings do not apply to its bridge interface" + ); + } if let Err(error) = accepted { self.roll_back_prepared_networks(prepared).await; return Err(error); @@ -800,40 +855,51 @@ impl App { } } + /// Deletes every host interface netd holds for this VM. + /// + /// A sweep rather than one removal per recorded NIC. The record is written + /// after the interface exists, so a VMM killed in between leaves a TAP + /// nothing on disk points at; a lost or unreadable record reads as an empty + /// list, which used to mean "nothing to remove"; and a manifest that lost a + /// NIC leaves an index the list no longer reaches. netd derives the names + /// instead, so none of that has to be true for teardown to work. + /// + /// `networks` now only decides whether to ask at all. An unreachable netd + /// is not a failure: most nodes run none, and stopping a VM must not depend + /// on one being up. 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()) - { + // An empty list is not "no interfaces", it is "no record" -- exactly + // the case a sweep exists for. A record that names only backends netd + // never touches is the one case worth skipping. + let recorded_none = !networks.is_empty() + && networks + .iter() + .all(|network| netd_teardown(network, &self.config.cvm).is_none()); + if recorded_none { 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); + match netd::remove_all( + &self.config.netd.socket, + &self.config.cvm.instance_id, + vm_id, + ) + .await + { + Ok(0) => Ok(()), + Ok(removed) => { + info!(vm_id, removed, "removed netd-managed interfaces"); + Ok(()) } + Err(error) if netd::is_unreachable(&error) => { + debug!(vm_id, %error, "no netd to remove interfaces from"); + Ok(()) + } + Err(error) => Err(error).context("failed to remove netd-managed networking"), } - 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<()> { diff --git a/dstack/vmm/src/app/network.rs b/dstack/vmm/src/app/network.rs index 4017e89ff..0ee7c4e38 100644 --- a/dstack/vmm/src/app/network.rs +++ b/dstack/vmm/src/app/network.rs @@ -9,7 +9,7 @@ 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, @@ -329,6 +329,54 @@ 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, because that is where QEMU's `hostfwd=` entries +/// have always gone and existing VMs must keep behaving the same way; failing +/// that the first bridge NIC, which is the only other backend with a path into +/// the guest. `macvtap` bypasses the host bridge and `custom` owns its own +/// netdev string, so neither can carry one. +pub(crate) fn default_ingress_nic(networks: &[Networking]) -> Option { + networks + .iter() + .position(|network| network.nic.mode == NetworkingMode::User) + .or_else(|| { + networks + .iter() + .position(|network| network.nic.mode == NetworkingMode::Bridge) + }) +} + +/// Which NIC a port mapping's traffic enters through. +/// +/// One mapping resolves to at most one NIC, and that NIC's backend decides the +/// mechanism: `hostfwd=` for user mode, netd for a bridge. That is what keeps +/// QEMU and netd from both claiming one host port. +pub(crate) fn ingress_nic(mapping: &PortMapping, networks: &[Networking]) -> Option { + mapping + .nic_index + .or_else(|| default_ingress_nic(networks)) + .filter(|index| *index < networks.len()) +} + +/// The host ports one NIC carries, as netd requests. +pub(crate) fn ingress_for( + port_map: &[PortMapping], + networks: &[Networking], + nic_index: usize, +) -> Vec { + port_map + .iter() + .filter(|mapping| ingress_nic(mapping, networks) == Some(nic_index)) + .map(|mapping| crate::netd::IngressRequest { + protocol: mapping.protocol.as_str().to_string(), + host_address: mapping.address.to_string(), + host_port: mapping.from, + guest_port: mapping.to, + }) + .collect() +} + /// Derives a deterministic, locally administered unicast MAC address. /// /// Index zero preserves the legacy single-NIC derivation. Later interfaces @@ -356,10 +404,12 @@ 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, + clamp_queues_without_netd, default_ingress_nic, effective_vhost, ingress_for, ingress_nic, + mac_address_for_vm_index, needs_netd_interface, netd_teardown, resolve_networking, + resolved_networks, settle_vhost, validate_resolved_networks, }; + use crate::app::PortMapping; + use crate::config::Protocol; use crate::config::{Networking, NetworkingMode, NicNetworking}; fn macvtap_network() -> NicNetworking { @@ -722,4 +772,74 @@ mod tests { "c6:74:2c:65:14:b9" ); } + + 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, + } + } + + #[test] + 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)); + + // With no user-mode NIC there was nowhere at all, which is the hole + // this closes: a bridge NIC is the only other backend with a path. + let networks = [nic(NetworkingMode::Bridge), nic(NetworkingMode::Bridge)]; + assert_eq!(default_ingress_nic(&networks), Some(0)); + + // macvtap bypasses the host bridge and custom owns its netdev string. + 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 a_pinned_mapping_goes_where_it_says() { + let networks = [nic(NetworkingMode::Bridge), nic(NetworkingMode::User)]; + assert_eq!(ingress_nic(&mapping(443, Some(0)), &networks), Some(0)); + // 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); + } + + #[test] + fn one_mapping_reaches_exactly_one_nic() { + // The property that keeps QEMU and netd from both claiming a host port: + // every mapping appears under one NIC and no other. + let networks = [nic(NetworkingMode::Bridge), nic(NetworkingMode::User)]; + let port_map = [ + mapping(443, Some(0)), + mapping(8080, None), + mapping(9090, Some(1)), + ]; + let per_nic: Vec<_> = (0..networks.len()) + .map(|index| ingress_for(&port_map, &networks, index)) + .collect(); + // Only NIC 0 is a bridge, so only its list becomes netd requests; the + // other two ride QEMU's hostfwd on NIC 1. + assert_eq!(per_nic[0].len(), 1); + assert_eq!(per_nic[0][0].host_port, 443); + assert_eq!(per_nic[1].len(), 2); + let total: usize = per_nic.iter().map(Vec::len).sum(); + assert_eq!(total, port_map.len()); + } } diff --git a/dstack/vmm/src/app/qemu.rs b/dstack/vmm/src/app/qemu.rs index 94a6348fe..828230068 100644 --- a/dstack/vmm/src/app/qemu.rs +++ b/dstack/vmm/src/app/qemu.rs @@ -10,8 +10,8 @@ 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, - warn_if_vhost_net_missing, + bridge_helper, ingress_nic, mac_address_for_vm_index, needs_netd_interface, + 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 } @@ -1192,6 +1192,7 @@ mod tests { protocol: Protocol::Tcp, from: 18080, to: 8080, + nic_index: None, }], created_at_ms: 0, hugepages: false, diff --git a/dstack/vmm/src/app/vm_info.rs b/dstack/vmm/src/app/vm_info.rs index 19d72118a..4cd10762b 100644 --- a/dstack/vmm/src/app/vm_info.rs +++ b/dstack/vmm/src/app/vm_info.rs @@ -207,6 +207,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/main_service.rs b/dstack/vmm/src/main_service.rs index 6f9a0905d..158b49c10 100644 --- a/dstack/vmm/src/main_service.rs +++ b/dstack/vmm/src/main_service.rs @@ -168,6 +168,28 @@ fn port_mappings_conflict(left: &PortMapping, right: &PortMapping) -> bool { || right.address.is_unspecified()) } +/// Rejects a mapping pinned to a NIC the VM does not have. +/// +/// Range only. Whether the named NIC's backend can carry a host port is +/// resolved at launch, where the node configuration that decides it is the one +/// in force -- and where an existing VM gets a warning rather than a refusal. +fn validate_port_mapping_nics(mappings: &[PortMapping], nic_count: usize) -> Result<()> { + for mapping in mappings { + let Some(index) = mapping.nic_index else { + continue; + }; + if index >= nic_count { + bail!( + "port mapping {} {}:{} names NIC {index}, but this VM has {nic_count}", + mapping.protocol.as_str(), + mapping.address, + mapping.from + ); + } + } + Ok(()) +} + fn validate_unique_port_mappings(mappings: &[PortMapping]) -> Result<()> { for (index, mapping) in mappings.iter().enumerate() { if mappings[..index] @@ -216,10 +238,14 @@ 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)?; + // An empty list inherits the node default, which is one NIC. + validate_port_mapping_nics(&port_map, networks.len().max(1))?; let app_id = match &request.app_id { Some(id) => id.strip_prefix("0x").unwrap_or(id).to_lowercase(), @@ -268,7 +294,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, }) } @@ -918,6 +944,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::>>()?; @@ -960,6 +987,9 @@ impl VmmRpc for RpcHandler { } manifest.networks = networks; } + // After both, since either half can move and the other still has to + // agree with it. + validate_port_mapping_nics(&manifest.port_map, manifest.networks.len().max(1))?; 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( diff --git a/dstack/vmm/src/netd.rs b/dstack/vmm/src/netd.rs index 6a760d1eb..024dc7db0 100644 --- a/dstack/vmm/src/netd.rs +++ b/dstack/vmm/src/netd.rs @@ -43,6 +43,10 @@ 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; #[derive(Debug, Clone, Serialize, Deserialize)] pub struct InterfaceIdentity { @@ -69,6 +73,56 @@ 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, + /// Host ports this VM wants reachable at its guest. + /// + /// Empty asks for nothing, which is also what a caller predating the field + /// sends. Whether a netd forwards them is its own business; this states the + /// requirement rather than assuming it is met, and the response says what + /// was actually done. + #[serde(default)] + pub ingress: Vec, +} + +/// One host port a VM wants reachable at its guest. +/// +/// Every field is named by the caller, which is what `bridge`, `mac` and +/// `queues` already get and the opposite of `filtered`. The difference is +/// whether netd can check what it is handed: an nwfilter name cannot be checked +/// for whether it filters anything, while a host port is a closed space a node +/// policy can be stated over. Naming is not deciding -- which ports may be +/// handed out stays netd's own configuration, exactly as `allowed_bridges` +/// governs the bridge a caller names. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct IngressRequest { + /// `"tcp"` or `"udp"`. + pub protocol: String, + /// Host address to accept on. Empty leaves the choice to netd. + /// + /// Not decoration: an admin port bound to loopback and a published one + /// differ only here. + #[serde(default)] + pub host_address: String, + /// Host port. Zero asks netd to choose one. + pub host_port: u16, + pub guest_port: u16, +} + +/// One forwarding rule a netd established, echoed so the caller can report what +/// the VM actually got rather than what it asked for. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct IngressBinding { + pub protocol: String, + pub host_address: String, + pub host_port: u16, + pub guest_port: u16, } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -84,6 +138,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)] @@ -99,6 +157,18 @@ pub enum Request { /// 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, + }, /// Verify a deterministic TAP and binding for operations and integration /// diagnostics. The VMM startup path uses Prepare rather than Check. Check { @@ -120,6 +190,15 @@ struct Response { /// between "one queue was requested" and "this netd ignored the request". #[serde(default, skip_serializing_if = "Option::is_none")] queues: Option, + /// Forwarding rules netd established. Absent from a netd that does not + /// forward host ports, which is how the caller tells "nothing was asked + /// for" apart from "this request was ignored" -- the same reading `queues` + /// gets above. + #[serde(default, skip_serializing_if = "Option::is_none")] + ingress: Option>, + /// How many interfaces a whole-VM sweep deleted. + #[serde(default, skip_serializing_if = "Option::is_none")] + removed: Option, #[serde(default, skip_serializing_if = "Option::is_none")] error: Option, } @@ -130,6 +209,8 @@ struct Prepared { tap: String, device: Option, queues: Option, + ingress: Option>, + removed: Option, } impl Prepared { @@ -138,6 +219,19 @@ impl Prepared { tap, device: None, queues: None, + ingress: None, + removed: None, + } + } + + /// A sweep names no single interface, so it reports how many it deleted. + fn removed(removed: usize) -> Self { + Self { + tap: String::new(), + device: None, + queues: None, + ingress: None, + removed: Some(removed), } } } @@ -162,6 +256,8 @@ pub fn instance_id(configured: &str, run_path: &Path) -> String { pub struct PreparedInterface { pub device: Option, pub queues: Option, + /// The forwarding rules netd established, if it forwards host ports at all. + pub ingress: Option>, } /// Marker carried in the error chain when the VMM could not reach netd at all. @@ -186,10 +282,36 @@ pub fn is_unreachable(error: &anyhow::Error) -> bool { } 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, + ingress: response.ingress, + }) +} + +/// Deletes every interface netd holds for one VM, returning how many there +/// were. See [`Request::RemoveAll`]. +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(), + }; + Ok(exchange(socket, &request) + .await? + .removed + .unwrap_or_default()) +} + +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::Check { .. } => "check", }; let exchange = async { @@ -220,11 +342,7 @@ pub async fn request(socket: &Path, request: &Request) -> Result Resul tap: Some(prepared.tap), device: prepared.device, queues: prepared.queues, + ingress: prepared.ingress, + removed: prepared.removed, error: None, }, Err(error) => { @@ -321,6 +441,8 @@ async fn serve_connection(config: &NetdConfig, stream: &mut UnixStream) -> Resul tap: None, device: None, queues: None, + ingress: None, + removed: None, error: Some(format!("{error:#}")), } } @@ -359,6 +481,10 @@ fn handle_request(config: &NetdConfig, request: Request) -> Result { Request::PrepareMacvtap(request) => { prepare_macvtap(libvirt_uri, &request, config.filter_policy()) } + Request::RemoveAll { instance_id, vm_id } => { + let removed = sweep_vm_interfaces(libvirt_uri, &instance_id, &vm_id)?; + Ok(Prepared::removed(removed)) + } Request::Remove { identity, filtered } => { validate_identity(&identity)?; let tap = tap_name(&identity); @@ -459,6 +585,8 @@ fn prepare_macvtap( tap, device: Some(device), queues: Some(queues), + ingress: None, + removed: None, }) } Err(error) => { @@ -540,6 +668,11 @@ fn prepare_bridge( 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. + ingress: None, + removed: None, }) } @@ -557,6 +690,49 @@ enum BindingCleanup { BestEffort, } +/// 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. +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 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() + }); + if !Path::new("/sys/class/net").join(&tap).exists() { + continue; + } + match remove_interface(libvirt_uri, &tap, BindingCleanup::BestEffort) { + // 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. + 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), + } +} + 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() { @@ -694,7 +870,7 @@ 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(()) @@ -868,6 +1044,8 @@ mod tests { qemu_uid: 1000, filtered: true, queues: 0, + workdir: String::new(), + ingress: Vec::new(), }; let filter = NetworkFilterConfig { mode: crate::config::NetworkFilterMode::Libvirt, @@ -914,6 +1092,8 @@ mod tests { qemu_uid: 1000, filtered: true, queues: 0, + workdir: String::new(), + ingress: Vec::new(), }); let value = serde_json::to_value(request).unwrap(); assert_eq!(value["operation"], "prepare_bridge"); @@ -983,6 +1163,8 @@ mod tests { qemu_uid: 1000, filtered: false, queues: 4, + workdir: String::new(), + ingress: Vec::new(), }); let value = serde_json::to_value(request).unwrap(); assert_eq!(value["queues"], 4); @@ -1015,6 +1197,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 +1215,8 @@ mod tests { qemu_uid: 1000, filtered: true, queues: 0, + workdir: String::new(), + ingress: Vec::new(), }); let value = serde_json::to_value(request).unwrap(); assert_eq!(value["operation"], "prepare_bridge"); @@ -1108,6 +1293,8 @@ mod tests { qemu_uid: 1000, filtered: false, queues: 4, + workdir: String::new(), + ingress: Vec::new(), }; let filtering = NetworkFilterConfig { mode: crate::config::NetworkFilterMode::Libvirt, @@ -1158,6 +1345,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 +1367,8 @@ mod tests { qemu_uid: 1000, filtered: true, queues: 1, + workdir: String::new(), + ingress: Vec::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 +1398,121 @@ 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(), + ingress: Vec::new(), + }); + 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, ""); + } + + #[test] + fn host_ports_travel_with_the_bridge_prepare_and_default_to_none() { + 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: String::new(), + ingress: vec![IngressRequest { + protocol: "udp".into(), + host_address: "0.0.0.0".into(), + host_port: 7483, + guest_port: 51820, + }], + }); + let value = serde_json::to_value(request).unwrap(); + assert_eq!(value["ingress"][0]["protocol"], "udp"); + assert_eq!(value["ingress"][0]["host_port"], 7483); + assert_eq!(value["ingress"][0]["guest_port"], 51820); + // The bind address separates an admin port from a published one, so a + // forwarder that lost it would publish the admin port. + assert_eq!(value["ingress"][0]["host_address"], "0.0.0.0"); + + let Request::PrepareBridge(decoded) = decode_minimal_bridge() else { + panic!("expected a bridge prepare"); + }; + assert!(decoded.ingress.is_empty()); + } + + #[test] + fn saying_nothing_about_ports_is_how_a_netd_reports_it_forwards_none() { + // The same reading `queues` gets: absent distinguishes "this netd does + // not do that" from "nothing was asked for", so ports are never assumed + // forwarded just because the TAP came back. + let response: Response = serde_json::from_value(serde_json::json!({ + "ok": true, + "tap": "dt000000000000", + })) + .unwrap(); + assert!(response.ingress.is_none()); + + let response: Response = serde_json::from_value(serde_json::json!({ + "ok": true, + "tap": "dt000000000000", + "ingress": [{ + "protocol": "udp", + "host_address": "0.0.0.0", + "host_port": 7483, + "guest_port": 51820, + }], + })) + .unwrap(); + assert_eq!(response.ingress.unwrap().len(), 1); + } + + /// 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()); + } } diff --git a/dstack/vmm/src/vmm-cli.py b/dstack/vmm/src/vmm-cli.py index 20e515520..faebc9d0f 100755 --- a/dstack/vmm/src/vmm-cli.py +++ b/dstack/vmm/src/vmm-cli.py @@ -321,17 +321,31 @@ 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("@") + try: + nic_index = int(nic) + except ValueError: + raise argparse.ArgumentTypeError(f"Invalid NIC index: {nic}") + if nic_index < 0: + raise argparse.ArgumentTypeError(f"Invalid NIC index: {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 +353,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 +1924,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 +2080,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 +2150,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",