feat(vmm): give port mappings a NIC, and carry them to netd

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 <name>` 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 `@<nic>` 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:

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,

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.

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<usize>,
 }
 
 /// 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<Vec<netd::IngressRequest>> = (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<()> {