Fixes improves OPTE installation and improves errors (#1052)

* Fixes improves OPTE installation and improves errors

- Fixes mismerge in `tools/install_opte.sh` that prevented installing
  OPTE package on a system that didn't previously have it
- Improves error messages when either xde driver fails or the expected
  virtual networking devices don't exist

* Fmt, fix lifetime for automock

* Add better handling and cleanup of VNICs

- Adds a `VnicKind` enum for tracking the flavor of each VNIC the sled
  agent is responsible for.
- Adds parameter to the `Dladm::get_vnics()` call which filters the
  returned list to a particular kind. The goal here is to be more
  explicit about which VNICs we're looking for and ultimately operating
  on in the sled agent.
- The sled agent now cleans up guest VNICs and the underlying xde
  devices (OPTE ports) when it starts up, similar to how it clears out
  any extant control VNICs, to ensure things are in a reliable state
  before accepting any requests from Nexus
- Improves the `tools/install_opte.sh` script, trying to be less
  intrusive and only modifying the state we need to change when adding
  the OPTE / xde package repositories.

* Add mock_opte::delete_all_xde_devices for non-illumos systems

* Update sled-agent mock context calls

* Be more conservative about what VnicKinds are allowed

* non-sticky is the goal

* Remove kind parameter to get_vnics, no longer needed

Author: bnaecker

Cargo.lock

@@ -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/Cargo.toml

@@ -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/dladm.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/running_zone.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/illumos/vnic.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/mock_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/opte/opte.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,