Fixes improves OPTE installation and improves errors

sled-agent/Cargo.toml

@@ -48,8 +48,8 @@ vsss-rs = { version = "2.0.0-pre2", default-features = false, features = ["std"]
 zone = "0.1"
 
 [target.'cfg(target_os = "illumos")'.dependencies]
-opte-ioctl = { git = "https://github.com/oxidecomputer/opte", rev = "cb1767c" }
-opte = { git = "https://github.com/oxidecomputer/opte", rev = "cb1767c", features = [ "api", "std" ] }
+opte-ioctl = { git = "https://github.com/oxidecomputer/opte", rev = "b998015" }
+opte = { git = "https://github.com/oxidecomputer/opte", rev = "b998015", features = [ "api", "std" ] }
 
 [dev-dependencies]
 expectorate = "1.0.5"

sled-agent/src/illumos/dladm.rs

@@ -5,6 +5,7 @@
 //! Utilities for poking at data links.
 
 use crate::common::vlan::VlanID;
+use crate::illumos::vnic::VnicKind;
 use crate::illumos::{execute, ExecutionError, PFEXEC};
 use omicron_common::api::external::MacAddr;
 use serde::{Deserialize, Serialize};
@@ -13,6 +14,11 @@ use std::str::FromStr;
 pub const VNIC_PREFIX: &str = "ox";
 pub const VNIC_PREFIX_CONTROL: &str = "oxControl";
 
+/// Prefix used to name VNICs over xde devices / OPTE ports.
+// TODO-correctness: Remove this when `xde` devices can be directly used beneath
+// Viona, and thus plumbed directly to guests.
+pub const VNIC_PREFIX_GUEST: &str = "vopte";
+
 pub const DLADM: &str = "/usr/sbin/dladm";
 
 /// Errors returned from [`Dladm::find_physical`].
@@ -164,16 +170,22 @@ impl Dladm {
         Ok(())
     }
 
-    /// Returns all VNICs that may be managed by the Sled Agent.
+    /// Returns VNICs that may be managed by the Sled Agent.
     pub fn get_vnics() -> Result<Vec<String>, GetVnicError> {
         let mut command = std::process::Command::new(PFEXEC);
         let cmd = command.args(&[DLADM, "show-vnic", "-p", "-o", "LINK"]);
         let output = execute(cmd).map_err(|err| GetVnicError { err })?;
 
         let vnics = String::from_utf8_lossy(&output.stdout)
             .lines()
-            .filter(|vnic| vnic.starts_with(VNIC_PREFIX))
-            .map(|s| s.to_owned())
+            .filter_map(|name| {
+                // Ensure this is a kind of VNIC that the sled agent could be
+                // responsible for.
+                match VnicKind::from_name(name) {
+                    Some(_) => Some(name.to_owned()),
+                    None => None,
+                }
+            })
             .collect();
         Ok(vnics)
     }

sled-agent/src/illumos/running_zone.rs

@@ -221,11 +221,14 @@ impl RunningZone {
             },
         )?;
 
+        let control_vnic = Vnic::wrap_existing(vnic_name)
+            .expect("Failed to wrap valid control VNIC");
+
         Ok(Self {
             inner: InstalledZone {
                 log: log.new(o!("zone" => zone_name.to_string())),
                 name: zone_name.to_string(),
-                control_vnic: Vnic::wrap_existing(vnic_name),
+                control_vnic,
                 // TODO(https://github.com/oxidecomputer/omicron/issues/725)
                 //
                 // Re-initialize guest_vnic state by inspecting the zone.

sled-agent/src/illumos/vnic.rs

@@ -6,7 +6,7 @@
 
 use crate::illumos::dladm::{
     CreateVnicError, DeleteVnicError, PhysicalLink, VNIC_PREFIX,
-    VNIC_PREFIX_CONTROL,
+    VNIC_PREFIX_CONTROL, VNIC_PREFIX_GUEST,
 };
 use omicron_common::api::external::MacAddr;
 use std::sync::{
@@ -60,7 +60,7 @@ impl VnicAllocator {
         debug_assert!(name.starts_with(VNIC_PREFIX));
         debug_assert!(name.starts_with(VNIC_PREFIX_CONTROL));
         Dladm::create_vnic(&self.data_link, &name, mac, None)?;
-        Ok(Vnic { name, deleted: false })
+        Ok(Vnic { name, deleted: false, kind: VnicKind::OxideControl })
     }
 
     fn new_superscope<S: AsRef<str>>(&self, scope: S) -> Self {
@@ -82,6 +82,32 @@ impl VnicAllocator {
     }
 }
 
+/// Represents the kind of a VNIC, such as whether it's for guest networking or
+/// communicating with Oxide services.
+#[derive(Debug, Clone, Copy, PartialEq)]
+pub enum VnicKind {
+    OxideControl,
+    Guest,
+}
+
+impl VnicKind {
+    /// Infer the kind from a VNIC's name, if this one the sled agent can
+    /// manage, and `None` otherwise.
+    pub fn from_name(name: &str) -> Option<Self> {
+        if name.starts_with(VNIC_PREFIX) {
+            Some(VnicKind::OxideControl)
+        } else if name.starts_with(VNIC_PREFIX_GUEST) {
+            Some(VnicKind::Guest)
+        } else {
+            None
+        }
+    }
+}
+
+#[derive(thiserror::Error, Debug)]
+#[error("VNIC with name '{0}' is not valid for sled agent management")]
+pub struct InvalidVnicKind(String);
+
 /// Represents an allocated VNIC on the system.
 /// The VNIC is de-allocated when it goes out of scope.
 ///
@@ -92,12 +118,22 @@ impl VnicAllocator {
 pub struct Vnic {
     name: String,
     deleted: bool,
+    kind: VnicKind,
 }
 
 impl Vnic {
     /// Takes ownership of an existing VNIC.
-    pub fn wrap_existing<S: AsRef<str>>(name: S) -> Self {
-        Vnic { name: name.as_ref().to_owned(), deleted: false }
+    pub fn wrap_existing<S: AsRef<str>>(
+        name: S,
+    ) -> Result<Self, InvalidVnicKind> {
+        match VnicKind::from_name(name.as_ref()) {
+            Some(kind) => Ok(Vnic {
+                name: name.as_ref().to_owned(),
+                deleted: false,
+                kind,
+            }),
+            None => Err(InvalidVnicKind(name.as_ref().to_owned())),
+        }
     }
 
     /// Deletes a NIC (if it has not already been deleted).
@@ -113,6 +149,10 @@ impl Vnic {
     pub fn name(&self) -> &str {
         &self.name
     }
+
+    pub fn kind(&self) -> VnicKind {
+        self.kind
+    }
 }
 
 impl Drop for Vnic {

sled-agent/src/opte/mock_opte.rs

@@ -183,3 +183,8 @@ pub fn initialize_xde_driver(log: &Logger) -> Result<(), Error> {
     slog::warn!(log, "`xde` driver is a fiction on non-illumos systems");
     Ok(())
 }
+
+pub fn delete_all_xde_devices(log: &Logger) -> Result<(), Error> {
+    slog::warn!(log, "`xde` driver is a fiction on non-illumos systems");
+    Ok(())
+}

sled-agent/src/opte/opte.rs

@@ -28,6 +28,12 @@ use std::sync::atomic::AtomicU64;
 use std::sync::atomic::Ordering;
 use std::sync::Arc;
 
+// Names of VNICs used as underlay devices for the xde driver.
+const XDE_VNIC_NAMES: [&str; 2] = ["net0", "net1"];
+
+// Prefix used to identify xde data links.
+const XDE_LINK_PREFIX: &str = "opte";
+
 #[derive(thiserror::Error, Debug)]
 pub enum Error {
     #[error("Failure interacting with the OPTE ioctl(2) interface: {0}")]
@@ -37,7 +43,15 @@ pub enum Error {
     CreateVnic(#[from] dladm::CreateVnicError),
 
     #[error("Failed to create an IPv6 link-local address for xde underlay devices: {0}")]
-    UnderlayDevice(#[from] crate::illumos::ExecutionError),
+    UnderlayDeviceAddress(#[from] crate::illumos::ExecutionError),
+
+    #[error("Failed to get VNICs for xde underlay devices: {0}")]
+    GetVnic(#[from] crate::illumos::dladm::GetVnicError),
+
+    #[error(
+        "No xde driver configuration file exists at '/kernel/drv/xde.conf'"
+    )]
+    NoXdeConf,
 
     #[error(transparent)]
     BadAddrObj(#[from] addrobj::ParseError),
@@ -54,7 +68,7 @@ impl OptePortAllocator {
     }
 
     fn next(&self) -> String {
-        format!("opte{}", self.next_id())
+        format!("{}{}", XDE_LINK_PREFIX, self.next_id())
     }
 
     fn next_id(&self) -> u64 {
@@ -150,7 +164,9 @@ impl OptePortAllocator {
                 Some(omicron_common::api::external::MacAddr(mac)),
                 None,
             )?;
-            Some(Vnic::wrap_existing(vnic_name))
+            // Safety: We're explicitly creating the VNIC with the prefix
+            // `VNIC_PREFIX_GUEST`, so this call must return Some(_).
+            Some(Vnic::wrap_existing(vnic_name).unwrap())
         };
 
         Ok(OptePort {
@@ -258,16 +274,41 @@ impl Drop for OptePort {
     }
 }
 
+/// Delete all xde devices on the system.
+pub fn delete_all_xde_devices(log: &Logger) -> Result<(), Error> {
+    let hdl = OpteHdl::open(OpteHdl::DLD_CTL)?;
+    for port_info in hdl.list_ports()?.ports.into_iter() {
+        let name = &port_info.name;
+        info!(
+            log,
+            "deleting existing OPTE port and xde device";
+            "device_name" => name
+        );
+        hdl.delete_xde(name)?;
+    }
+    Ok(())
+}
+
 /// Initialize the underlay devices required for the xde kernel module.
 ///
 /// The xde driver needs information about the physical devices out which it can
 /// send traffic from the guests.
 pub fn initialize_xde_driver(log: &Logger) -> Result<(), Error> {
+    if !std::path::Path::new("/kernel/drv/xde.conf").exists() {
+        return Err(Error::NoXdeConf);
+    }
     let underlay_nics = find_chelsio_links()?;
     info!(log, "using '{:?}' as data links for xde driver", underlay_nics);
     if underlay_nics.len() < 2 {
+        const MESSAGE: &str = concat!(
+            "There must be at least two underlay NICs for the xde ",
+            "driver to operate. These are currently created by ",
+            "`./tools/create_virtual_hardware.sh`. Please ensure that ",
+            "script has been run, and that two VNICs named `net{0,1}` ",
+            "exist on the system."
+        );
         return Err(Error::Opte(opte_ioctl::Error::InvalidArgument(
-            String::from("There must be at least two underlay NICs"),
+            String::from(MESSAGE),
         )));
     }
     for nic in &underlay_nics {
@@ -294,5 +335,8 @@ fn find_chelsio_links() -> Result<Vec<PhysicalLink>, Error> {
     // `Dladm` to get the real Chelsio links on a Gimlet. These will likely be
     // called `cxgbeN`, but we explicitly call them `netN` to be clear that
     // they're likely VNICs for the time being.
-    Ok((0..2).map(|i| PhysicalLink(format!("net{}", i))).collect())
+    Ok(XDE_VNIC_NAMES
+        .into_iter()
+        .map(|name| PhysicalLink(name.to_string()))
+        .collect())
 }

sled-agent/src/sled_agent.rs

@@ -5,6 +5,7 @@
 //! Sled agent implementation
 
 use crate::config::Config;
+use crate::illumos::vnic::VnicKind;
 use crate::illumos::zfs::{
     Mountpoint, ZONE_ZFS_DATASET, ZONE_ZFS_DATASET_MOUNTPOINT,
 };
@@ -148,7 +149,7 @@ impl SledAgent {
         // to leave the running Zones intact).
         let zones = Zones::get()?;
         for z in zones {
-            warn!(log, "Deleting zone: {}", z.name());
+            warn!(log, "Deleting existing zone"; "zone_name" => z.name());
             Zones::halt_and_remove_logged(&log, z.name())?;
         }
 
@@ -162,18 +163,28 @@ impl SledAgent {
         // This should be accessible via:
         // $ dladm show-linkprop -c -p zone -o LINK,VALUE
         //
-        // Note that this currently deletes only VNICs that start with the
-        // prefix the sled-agent uses. We'll need to generate an alert or
-        // otherwise handle VNICs that we _don't_ expect.
-        let vnics = Dladm::get_vnics()?;
-        for vnic in vnics
-            .iter()
-            .filter(|vnic| vnic.starts_with(crate::illumos::dladm::VNIC_PREFIX))
-        {
-            warn!(log, "Deleting VNIC: {}", vnic);
+        // Note that we don't currently delete the VNICs in any particular
+        // order. That should be OK, since we're definitely deleting the guest
+        // VNICs before the xde devices, which is the main constraint.
+        for vnic in Dladm::get_vnics()? {
+            warn!(
+              log,
+              "Deleting existing VNIC";
+                "vnic_name" => &vnic,
+                "vnic_kind" => ?VnicKind::from_name(&vnic).unwrap(),
+            );
             Dladm::delete_vnic(&vnic)?;
         }
 
+        // Also delete any extant xde devices. These should also eventually be
+        // recovered / tracked, to avoid interruption of any guests that are
+        // still running. That's currently irrelevant, since we're deleting the
+        // zones anyway.
+        //
+        // This is also tracked by
+        // https://github.com/oxidecomputer/omicron/issues/725.
+        crate::opte::delete_all_xde_devices(&log)?;
+
         let storage = StorageManager::new(
             &log,
             *id,