[sled-agent] Remove VNICs from XDE devices (#5989)

This PR removes the `vopte` range of vnics established for zones/probes/instances with an OPTE port, which have been used as a workaround for some time. This gives us one less 'hop' in the packet processing path for instances and external-facing zones. Closes #2932.
oxidecomputer · Jul 16, 2024 · eb15768 · eb15768
1 parent 8316247
commit eb15768
Show file tree

Hide file tree

Showing 13 changed files with 52 additions and 136 deletions.
diff --git a/.github/buildomat/jobs/deploy.sh b/.github/buildomat/jobs/deploy.sh
@@ -2,7 +2,7 @@
 #:
 #: name = "helios / deploy"
 #: variety = "basic"
-#: target = "lab-2.0-opte-0.32"
+#: target = "lab-2.0-opte-0.33"
 #: output_rules = [
 #:  "%/var/svc/log/oxide-sled-agent:default.log*",
 #:  "%/zone/oxz_*/root/var/svc/log/oxide-*.log*",

diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -343,8 +343,8 @@ macaddr = { version = "1.0.1", features = ["serde_std"] }
 maplit = "1.0.2"
 mockall = "0.12"
 newtype_derive = "0.1.6"
-mg-admin-client = { git = "https://github.com/oxidecomputer/maghemite", rev = "1b385990e8648b221fd11f018f2a7ec425461c6c" }
-ddm-admin-client = { git = "https://github.com/oxidecomputer/maghemite", rev = "1b385990e8648b221fd11f018f2a7ec425461c6c" }
+mg-admin-client = { git = "https://github.com/oxidecomputer/maghemite", rev = "220dd026e83142b83bd93123f465a64dd4600201" }
+ddm-admin-client = { git = "https://github.com/oxidecomputer/maghemite", rev = "220dd026e83142b83bd93123f465a64dd4600201" }
 multimap = "0.10.0"
 nexus-auth = { path = "nexus/auth" }
 nexus-client = { path = "clients/nexus-client" }
@@ -384,14 +384,14 @@ omicron-sled-agent = { path = "sled-agent" }
 omicron-test-utils = { path = "test-utils" }
 omicron-zone-package = "0.11.0"
 oxide-client = { path = "clients/oxide-client" }
-oxide-vpc = { git = "https://github.com/oxidecomputer/opte", rev = "915975f6d1729db95619f752148974016912412f", features = [ "api", "std" ] }
+oxide-vpc = { git = "https://github.com/oxidecomputer/opte", rev = "3dc9a3dd8d3c623f0cf2c659c7119ce0c026a96d", features = [ "api", "std" ] }
 once_cell = "1.19.0"
 openapi-lint = { git = "https://github.com/oxidecomputer/openapi-lint", branch = "main" }
 openapiv3 = "2.0.0"
 # must match samael's crate!
 openssl = "0.10"
 openssl-sys = "0.9"
-opte-ioctl = { git = "https://github.com/oxidecomputer/opte", rev = "915975f6d1729db95619f752148974016912412f" }
+opte-ioctl = { git = "https://github.com/oxidecomputer/opte", rev = "3dc9a3dd8d3c623f0cf2c659c7119ce0c026a96d" }
 oso = "0.27"
 owo-colors = "4.0.0"
 oximeter = { path = "oximeter/oximeter" }
@@ -420,9 +420,9 @@ prettyplease = { version = "0.2.20", features = ["verbatim"] }
 proc-macro2 = "1.0"
 progenitor = { git = "https://github.com/oxidecomputer/progenitor", branch = "main" }
 progenitor-client = { git = "https://github.com/oxidecomputer/progenitor", branch = "main" }
-bhyve_api = { git = "https://github.com/oxidecomputer/propolis", rev = "5ebf9626e0ad274eb515d206d102cb09d2d51f15" }
-propolis-client = { git = "https://github.com/oxidecomputer/propolis", rev = "5ebf9626e0ad274eb515d206d102cb09d2d51f15" }
-propolis-mock-server = { git = "https://github.com/oxidecomputer/propolis", rev = "5ebf9626e0ad274eb515d206d102cb09d2d51f15" }
+bhyve_api = { git = "https://github.com/oxidecomputer/propolis", rev = "66d1ee7d4a5829dbbf02a152091ea051023b5b8b" }
+propolis-client = { git = "https://github.com/oxidecomputer/propolis", rev = "66d1ee7d4a5829dbbf02a152091ea051023b5b8b" }
+propolis-mock-server = { git = "https://github.com/oxidecomputer/propolis", rev = "66d1ee7d4a5829dbbf02a152091ea051023b5b8b" }
 proptest = "1.4.0"
 quote = "1.0"
 rand = "0.8.5"

diff --git a/illumos-utils/src/opte/port.rs b/illumos-utils/src/opte/port.rs
@@ -30,17 +30,6 @@ pub struct PortData {
     pub(crate) subnet: IpNet,
     /// Information about the virtual gateway, aka OPTE
     pub(crate) gateway: Gateway,
-    /// Name of the VNIC the OPTE port is bound to.
-    // TODO-remove(#2932): Remove this once we can put Viona directly on top of an
-    // OPTE port device.
-    //
-    // NOTE: This is intentionally not an actual `Vnic` object. We'd like to
-    // delete the VNIC manually in `PortInner::drop`, because we _can't_ delete
-    // the xde device if we fail to delete the VNIC. See
-    // https://github.com/oxidecomputer/opte/issues/178 for more details. This
-    // can be changed back to a real VNIC when that is resolved, and the Drop
-    // impl below can simplify to just call `drop(self.vnic)`.
-    pub(crate) vnic: String,
 }
 
 #[derive(Debug)]
@@ -57,18 +46,6 @@ impl core::ops::Deref for PortInner {
 #[cfg(target_os = "illumos")]
 impl Drop for PortInner {
     fn drop(&mut self) {
-        if let Err(e) = crate::dladm::Dladm::delete_vnic(&self.vnic) {
-            eprintln!(
-                "WARNING: Failed to delete OPTE port overlay VNIC \
-                while dropping port. The VNIC will not be cleaned up \
-                properly, and the xde device itself will not be deleted. \
-                Both the VNIC and the xde device must be deleted out \
-                of band, and it will not be possible to recreate the xde \
-                device until then. Error: {:?}",
-                e
-            );
-            return;
-        }
         let err = match opte_ioctl::OpteHdl::open(opte_ioctl::OpteHdl::XDE_CTL)
         {
             Ok(hdl) => {
@@ -81,9 +58,8 @@ impl Drop for PortInner {
             Err(e) => e,
         };
         eprintln!(
-            "WARNING: OPTE port overlay VNIC deleted, but failed \
-            to delete the xde device. It must be deleted out \
-            of band, and it will not be possible to recreate the xde \
+            "WARNING: Failed to delete the xde device. It must be deleted
+            out of band, and it will not be possible to recreate the xde \
             device until then. Error: {:?}",
             err,
         );
@@ -130,10 +106,6 @@ impl Port {
         &self.inner.subnet
     }
 
-    pub fn vnic_name(&self) -> &str {
-        &self.inner.vnic
-    }
-
     pub fn slot(&self) -> u8 {
         self.inner.slot
     }

diff --git a/illumos-utils/src/opte/port_manager.rs b/illumos-utils/src/opte/port_manager.rs
@@ -304,57 +304,6 @@ impl PortManager {
             rules,
         })?;
 
-        // TODO-remove(#2932): Create a VNIC on top of this device, to hook Viona into.
-        //
-        // Viona is the illumos MAC provider that implements the VIRTIO
-        // specification. It sits on top of a MAC provider, which is responsible
-        // for delivering frames to the underlying data link. The guest includes
-        // a driver that handles the virtio-net specification on their side,
-        // which talks to Viona.
-        //
-        // In theory, Viona works with any MAC provider. However, there are
-        // implicit assumptions, in both Viona _and_ MAC, that require Viona to
-        // be built on top of a VNIC specifically. There is probably a good deal
-        // of work required to relax that assumption, so in the meantime, we
-        // create a superfluous VNIC on the OPTE device, solely so Viona can use
-        // it.
-        let vnic = {
-            let vnic_name = format!("v{}", port_name);
-            #[cfg(target_os = "illumos")]
-            if let Err(e) = crate::dladm::Dladm::create_vnic(
-                &crate::dladm::PhysicalLink(port_name.clone()),
-                &vnic_name,
-                Some(nic.mac),
-                None,
-                1500,
-            ) {
-                slog::warn!(
-                    self.inner.log,
-                    "Failed to create overlay VNIC for xde device";
-                    "port_name" => &port_name,
-                    "err" => ?e
-                );
-                if let Err(e) = hdl.delete_xde(&port_name) {
-                    slog::warn!(
-                        self.inner.log,
-                        "Failed to clean up xde device after failure to create overlay VNIC";
-                        "err" => ?e
-                    );
-                }
-                return Err(e.into());
-            }
-            debug!(
-                self.inner.log,
-                "Created overlay VNIC for xde device";
-                "port_name" => &port_name,
-                "vnic_name" => &vnic_name,
-            );
-
-            // NOTE: We intentionally use a string rather than the Vnic type
-            // here. See the notes on the `opte::PortInner::vnic` field.
-            vnic_name
-        };
-
         let (port, ticket) = {
             let mut ports = self.inner.ports.lock().unwrap();
             let ticket = PortTicket::new(nic.id, nic.kind, self.inner.clone());
@@ -366,7 +315,6 @@ impl PortManager {
                 vni,
                 subnet: nic.subnet,
                 gateway,
-                vnic,
             });
             let old = ports.insert((nic.id, nic.kind), port.clone());
             assert!(

diff --git a/illumos-utils/src/running_zone.rs b/illumos-utils/src/running_zone.rs
@@ -609,15 +609,13 @@ impl RunningZone {
                 port_idx,
             }
         })?;
-        // TODO-remove(#2932): Switch to using port directly once vnic is no longer needed.
-        let addrobj =
-            AddrObject::new(port.vnic_name(), name).map_err(|err| {
-                EnsureAddressError::AddrObject {
-                    request: AddressRequest::Dhcp,
-                    zone: self.inner.name.clone(),
-                    err,
-                }
-            })?;
+        let addrobj = AddrObject::new(port.name(), name).map_err(|err| {
+            EnsureAddressError::AddrObject {
+                request: AddressRequest::Dhcp,
+                zone: self.inner.name.clone(),
+                err,
+            }
+        })?;
         let zone = Some(self.inner.name.as_ref());
         if let IpAddr::V4(gateway) = port.gateway().ip() {
             let addr =
@@ -636,7 +634,7 @@ impl RunningZone {
                 &private_ip.to_string(),
                 "-interface",
                 "-ifp",
-                port.vnic_name(),
+                port.name(),
             ])?;
             self.run_cmd(&[
                 "/usr/sbin/route",
@@ -1293,7 +1291,7 @@ impl<'a> ZoneBuilder<'a> {
 
         let mut net_device_names: Vec<String> = opte_ports
             .iter()
-            .map(|(port, _)| port.vnic_name().to_string())
+            .map(|(port, _)| port.name().to_string())
             .chain(std::iter::once(control_vnic.name().to_string()))
             .chain(bootstrap_vnic.as_ref().map(|vnic| vnic.name().to_string()))
             .chain(links.iter().map(|nic| nic.name().to_string()))