From 0d6b385330c4dddf222074992ba8f609a1d932cf Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Tue, 2 Jul 2024 12:46:49 +0100 Subject: [PATCH] Remove VNICs from XDE devices --- illumos-utils/src/opte/port.rs | 32 +--------------- illumos-utils/src/opte/port_manager.rs | 52 -------------------------- illumos-utils/src/running_zone.rs | 20 +++++----- sled-agent/src/instance.rs | 4 +- sled-agent/src/services.rs | 2 +- 5 files changed, 13 insertions(+), 97 deletions(-) diff --git a/illumos-utils/src/opte/port.rs b/illumos-utils/src/opte/port.rs index a692a02304..4cfe351776 100644 --- 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 index 984e3c55fa..b6d28d1b06 100644 --- 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 index c529a1b6d4..fdfdb64781 100644 --- a/illumos-utils/src/running_zone.rs +++ b/illumos-utils/src/running_zone.rs @@ -657,15 +657,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 = @@ -684,7 +682,7 @@ impl RunningZone { &private_ip.to_string(), "-interface", "-ifp", - port.vnic_name(), + port.name(), ])?; self.run_cmd(&[ "/usr/sbin/route", @@ -1427,7 +1425,7 @@ impl<'a> ZoneBuilder<'a> { let mut net_device_names: Vec = 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())) diff --git a/sled-agent/src/instance.rs b/sled-agent/src/instance.rs index ec4d503e7b..586b6e9ed3 100644 --- a/sled-agent/src/instance.rs +++ b/sled-agent/src/instance.rs @@ -651,9 +651,7 @@ impl InstanceRunner { let nics = running_zone .opte_ports() .map(|port| propolis_client::types::NetworkInterfaceRequest { - // TODO-correctness: Remove `.vnic()` call when we use the port - // directly. - name: port.vnic_name().to_string(), + name: port.name().to_string(), slot: propolis_client::types::Slot(port.slot()), }) .collect(); diff --git a/sled-agent/src/services.rs b/sled-agent/src/services.rs index fb57990f1b..ae1d8243ca 100644 --- a/sled-agent/src/services.rs +++ b/sled-agent/src/services.rs @@ -1361,7 +1361,7 @@ impl ServiceManager { }) })?; - let opte_interface = port.vnic_name(); + let opte_interface = port.name(); let opte_gateway = port.gateway().ip().to_string(); let opte_ip = port.ip().to_string();