Revert "[sled-agent] Allocate VNICs over etherstubs, fix inter-zone r…

…outing (#1066)"

This reverts commit 813a859.

sled-agent/src/bootstrap/agent.rs

@@ -157,23 +157,19 @@ impl Agent {
                 err,
             })?;
 
-        let etherstub = Dladm::create_etherstub().map_err(|e| {
-            BootstrapError::SledError(format!(
-                "Can't access etherstub device: {}",
-                e
-            ))
-        })?;
-
-        let etherstub_vnic =
-            Dladm::create_etherstub_vnic(&etherstub).map_err(|e| {
+        let data_link = if let Some(link) = sled_config.data_link.clone() {
+            link
+        } else {
+            Dladm::find_physical().map_err(|err| {
                 BootstrapError::SledError(format!(
-                    "Can't access etherstub VNIC device: {}",
-                    e
+                    "Can't access physical link, and none in config: {}",
+                    err
                 ))
-            })?;
+            })?
+        };
 
         Zones::ensure_has_global_zone_v6_address(
-            etherstub_vnic,
+            data_link,
             address,
             "bootstrap6",
         )

sled-agent/src/config.rs

@@ -27,7 +27,7 @@ pub struct Config {
     /// Optional list of zpools to be used as "discovered disks".
     pub zpools: Option<Vec<ZpoolName>>,
 
-    /// The data link on which we infer the bootstrap address.
+    /// The data link on which to allocate VNICs.
     ///
     /// If unsupplied, we default to the first physical device.
     pub data_link: Option<PhysicalLink>,

sled-agent/src/illumos/dladm.rs

@@ -19,15 +19,8 @@ pub const VNIC_PREFIX_CONTROL: &str = "oxControl";
 // Viona, and thus plumbed directly to guests.
 pub const VNIC_PREFIX_GUEST: &str = "vopte";
 
-/// Path to the DLADM command.
 pub const DLADM: &str = "/usr/sbin/dladm";
 
-/// The name of the etherstub to be created for the underlay.
-pub const ETHERSTUB_NAME: &str = "stub0";
-
-/// The name of the etherstub VNIC to be created in the global zone.
-pub const ETHERSTUB_VNIC_NAME: &str = "underlay0";
-
 /// Errors returned from [`Dladm::find_physical`].
 #[derive(thiserror::Error, Debug)]
 pub enum FindPhysicalLinkError {
@@ -56,7 +49,7 @@ pub enum GetMacError {
 #[error("Failed to create VNIC {name} on link {link:?}: {err}")]
 pub struct CreateVnicError {
     name: String,
-    link: String,
+    link: PhysicalLink,
     #[source]
     err: ExecutionError,
 }
@@ -82,77 +75,11 @@ pub struct DeleteVnicError {
 #[derive(Debug, Clone, Deserialize, Serialize, PartialEq)]
 pub struct PhysicalLink(pub String);
 
-/// The name of an etherstub
-#[derive(Debug, Clone, Deserialize, Serialize, PartialEq)]
-pub struct Etherstub(pub String);
-
-/// The name of an etherstub's underlay VNIC
-#[derive(Debug, Clone, Deserialize, Serialize, PartialEq)]
-pub struct EtherstubVnic(pub String);
-
-/// Identifies that an object may be used to create a VNIC.
-pub trait VnicSource {
-    fn name(&self) -> &str;
-}
-
-impl VnicSource for Etherstub {
-    fn name(&self) -> &str {
-        &self.0
-    }
-}
-
-impl VnicSource for PhysicalLink {
-    fn name(&self) -> &str {
-        &self.0
-    }
-}
-
 /// Wraps commands for interacting with data links.
 pub struct Dladm {}
 
 #[cfg_attr(test, mockall::automock, allow(dead_code))]
 impl Dladm {
-    /// Creates an etherstub, or returns one which already exists.
-    pub fn create_etherstub() -> Result<Etherstub, ExecutionError> {
-        if let Ok(stub) = Self::get_etherstub() {
-            return Ok(stub);
-        }
-        let mut command = std::process::Command::new(PFEXEC);
-        let cmd =
-            command.args(&[DLADM, "create-etherstub", "-t", ETHERSTUB_NAME]);
-        execute(cmd)?;
-        Ok(Etherstub(ETHERSTUB_NAME.to_string()))
-    }
-
-    /// Finds an etherstub.
-    fn get_etherstub() -> Result<Etherstub, ExecutionError> {
-        let mut command = std::process::Command::new(PFEXEC);
-        let cmd = command.args(&[DLADM, "show-etherstub", ETHERSTUB_NAME]);
-        execute(cmd)?;
-        Ok(Etherstub(ETHERSTUB_NAME.to_string()))
-    }
-
-    /// Creates a VNIC on top of the etherstub.
-    ///
-    /// This VNIC is not tracked like [`crate::illumos::vnic::Vnic`], because
-    /// it is expected to exist for the lifetime of the sled.
-    pub fn create_etherstub_vnic(
-        source: &Etherstub,
-    ) -> Result<EtherstubVnic, CreateVnicError> {
-        if let Ok(vnic) = Self::get_etherstub_vnic() {
-            return Ok(vnic);
-        }
-        Self::create_vnic(source, ETHERSTUB_VNIC_NAME, None, None)?;
-        Ok(EtherstubVnic(ETHERSTUB_VNIC_NAME.to_string()))
-    }
-
-    fn get_etherstub_vnic() -> Result<EtherstubVnic, ExecutionError> {
-        let mut command = std::process::Command::new(PFEXEC);
-        let cmd = command.args(&[DLADM, "show-vnic", ETHERSTUB_VNIC_NAME]);
-        execute(cmd)?;
-        Ok(EtherstubVnic(ETHERSTUB_VNIC_NAME.to_string()))
-    }
-
     /// Returns the name of the first observed physical data link.
     pub fn find_physical() -> Result<PhysicalLink, FindPhysicalLinkError> {
         let mut command = std::process::Command::new(PFEXEC);
@@ -208,8 +135,8 @@ impl Dladm {
     /// * `vnic_name`: Exact name of the VNIC to be created.
     /// * `mac`: An optional unicast MAC address for the newly created NIC.
     /// * `vlan`: An optional VLAN ID for VLAN tagging.
-    pub fn create_vnic<T: VnicSource + 'static>(
-        source: &T,
+    pub fn create_vnic(
+        physical: &PhysicalLink,
         vnic_name: &str,
         mac: Option<MacAddr>,
         vlan: Option<VlanID>,
@@ -220,7 +147,7 @@ impl Dladm {
             "create-vnic".to_string(),
             "-t".to_string(),
             "-l".to_string(),
-            source.name().to_string(),
+            physical.0.to_string(),
         ];
 
         if let Some(mac) = mac {
@@ -237,7 +164,7 @@ impl Dladm {
         let cmd = command.args(&args);
         execute(cmd).map_err(|err| CreateVnicError {
             name: vnic_name.to_string(),
-            link: source.name().to_string(),
+            link: physical.clone(),
             err,
         })?;
         Ok(())

sled-agent/src/illumos/mod.rs

@@ -15,20 +15,19 @@ pub mod zfs;
 pub mod zone;
 pub mod zpool;
 
-pub const PFEXEC: &str = "/usr/bin/pfexec";
+const PFEXEC: &str = "/usr/bin/pfexec";
 
 #[derive(thiserror::Error, Debug)]
 pub enum ExecutionError {
     #[error("Failed to start execution of [{command}]: {err}")]
     ExecutionStart { command: String, err: std::io::Error },
 
     #[error(
-        "Command [{command}] executed and failed with status: {status}. Stdout: {stdout}, Stderr: {stderr}"
+        "Command [{command}] executed and failed with status: {status}. Output: {stderr}"
     )]
     CommandFailure {
         command: String,
         status: std::process::ExitStatus,
-        stdout: String,
         stderr: String,
     },
 }
@@ -64,7 +63,6 @@ mod inner {
                     .collect::<Vec<String>>()
                     .join(" "),
                 status: output.status,
-                stdout: String::from_utf8_lossy(&output.stdout).to_string(),
                 stderr: String::from_utf8_lossy(&output.stderr).to_string(),
             });
         }

sled-agent/src/illumos/running_zone.rs

@@ -171,21 +171,6 @@ impl RunningZone {
         Ok(network)
     }
 
-    pub async fn add_route(
-        &self,
-        destination: ipnetwork::Ipv6Network,
-    ) -> Result<(), RunCommandError> {
-        self.run_cmd(&[
-            "/usr/sbin/route",
-            "add",
-            "-inet6",
-            &format!("{}/{}", destination.network(), destination.prefix()),
-            "-inet6",
-            &destination.ip().to_string(),
-        ])?;
-        Ok(())
-    }
-
     /// Looks up a running zone based on the `zone_prefix`, if one already exists.
     ///
     /// - If the zone was found, is running, and has a network interface, it is

sled-agent/src/illumos/vnic.rs

@@ -5,7 +5,7 @@
 //! API for controlling a single instance.
 
 use crate::illumos::dladm::{
-    CreateVnicError, DeleteVnicError, Etherstub, VNIC_PREFIX,
+    CreateVnicError, DeleteVnicError, PhysicalLink, VNIC_PREFIX,
     VNIC_PREFIX_CONTROL, VNIC_PREFIX_GUEST,
 };
 use omicron_common::api::external::MacAddr;
@@ -26,7 +26,7 @@ use crate::illumos::dladm::MockDladm as Dladm;
 pub struct VnicAllocator {
     value: Arc<AtomicU64>,
     scope: String,
-    data_link: Etherstub,
+    data_link: PhysicalLink,
 }
 
 impl VnicAllocator {
@@ -41,11 +41,11 @@ impl VnicAllocator {
     ///
     /// VnicAllocator::new("Storage") produces
     /// - oxControlStorage[NNN]
-    pub fn new<S: AsRef<str>>(scope: S, etherstub: Etherstub) -> Self {
+    pub fn new<S: AsRef<str>>(scope: S, physical_link: PhysicalLink) -> Self {
         Self {
             value: Arc::new(AtomicU64::new(0)),
             scope: scope.as_ref().to_string(),
-            data_link: etherstub,
+            data_link: physical_link,
         }
     }
 
@@ -171,7 +171,7 @@ mod test {
     #[test]
     fn test_allocate() {
         let allocator =
-            VnicAllocator::new("Foo", Etherstub("mystub".to_string()));
+            VnicAllocator::new("Foo", PhysicalLink("mylink".to_string()));
         assert_eq!("oxFoo0", allocator.next());
         assert_eq!("oxFoo1", allocator.next());
         assert_eq!("oxFoo2", allocator.next());
@@ -180,7 +180,7 @@ mod test {
     #[test]
     fn test_allocate_within_scopes() {
         let allocator =
-            VnicAllocator::new("Foo", Etherstub("mystub".to_string()));
+            VnicAllocator::new("Foo", PhysicalLink("mylink".to_string()));
         assert_eq!("oxFoo0", allocator.next());
         let allocator = allocator.new_superscope("Baz");
         assert_eq!("oxBazFoo1", allocator.next());

sled-agent/src/illumos/zone.rs

@@ -10,10 +10,9 @@ use slog::Logger;
 use std::net::{IpAddr, Ipv6Addr};
 
 use crate::illumos::addrobj::AddrObject;
-use crate::illumos::dladm::{EtherstubVnic, VNIC_PREFIX_CONTROL};
+use crate::illumos::dladm::{PhysicalLink, VNIC_PREFIX_CONTROL};
 use crate::illumos::zfs::ZONE_ZFS_DATASET_MOUNTPOINT;
 use crate::illumos::{execute, PFEXEC};
-use omicron_common::address::SLED_PREFIX;
 
 const DLADM: &str = "/usr/sbin/dladm";
 const IPADM: &str = "/usr/sbin/ipadm";
@@ -99,14 +98,13 @@ pub struct EnsureAddressError {
 
 /// Errors from [`Zones::ensure_has_global_zone_v6_address`].
 #[derive(thiserror::Error, Debug)]
-#[error("Failed to create address {address} with name {name} in the GZ on {link:?}: {err}. Note to developers: {extra_note}")]
+#[error("Failed to create address {address} with name {name} in the GZ on {link:?}: {err}")]
 pub struct EnsureGzAddressError {
     address: Ipv6Addr,
-    link: EtherstubVnic,
+    link: PhysicalLink,
     name: String,
     #[source]
     err: anyhow::Error,
-    extra_note: String,
 }
 
 /// Describes the type of addresses which may be requested from a zone.
@@ -124,7 +122,7 @@ impl AddressRequest {
     pub fn new_static(ip: IpAddr, prefix: Option<u8>) -> Self {
         let prefix = prefix.unwrap_or_else(|| match ip {
             IpAddr::V4(_) => 24,
-            IpAddr::V6(_) => SLED_PREFIX,
+            IpAddr::V6(_) => 64,
         });
         let addr = IpNetwork::new(ip, prefix).unwrap();
         AddressRequest::Static(addr)
@@ -545,13 +543,13 @@ impl Zones {
     // should remove this function when Sled Agents are provided IPv6 addresses
     // from RSS.
     pub fn ensure_has_global_zone_v6_address(
-        link: EtherstubVnic,
+        link: PhysicalLink,
         address: Ipv6Addr,
         name: &str,
     ) -> Result<(), EnsureGzAddressError> {
         // Call the guts of this function within a closure to make it easier
         // to wrap the error with appropriate context.
-        |link: EtherstubVnic, address, name| -> Result<(), anyhow::Error> {
+        |link: PhysicalLink, address, name| -> Result<(), anyhow::Error> {
             let gz_link_local_addrobj = AddrObject::new(&link.0, "linklocal")
                 .map_err(|err| anyhow!(err))?;
             Self::ensure_has_link_local_v6_address(
@@ -584,22 +582,6 @@ impl Zones {
             link,
             name: name.to_string(),
             err,
-            extra_note:
-                r#"As of https://github.com/oxidecomputer/omicron/pull/1066, we are changing the
-                physical device on which Global Zone addresses are allocated.
-
-                Before this PR, we allocated addresses and VNICs directly on a physical link.
-                After this PR, we are allocating them on etherstubs.
-
-                As a result, however, if your machine previously ran Omicron, it
-                may have addresses on the physical link which we'd like to
-                allocate from the etherstub instead.
-
-                This can be fixed with the following commands:
-
-                $ pfexec ipadm delete-addr <your-link>/bootstrap6
-                $ pfexec ipadm delete-addr <your-link>/sled6
-                $ pfexec ipadm delete-addr <your-link>/internaldns"#.to_string()
         })?;
         Ok(())
     }

sled-agent/src/instance.rs

@@ -715,7 +715,7 @@ impl Instance {
 #[cfg(test)]
 mod test {
     use super::*;
-    use crate::illumos::dladm::Etherstub;
+    use crate::illumos::dladm::PhysicalLink;
     use crate::mocks::MockNexusClient;
     use crate::opte::OptePortAllocator;
     use crate::params::InstanceStateRequested;
@@ -786,7 +786,7 @@ mod test {
         let log = logger();
         let vnic_allocator = VnicAllocator::new(
             "Test".to_string(),
-            Etherstub("mylink".to_string()),
+            PhysicalLink("mylink".to_string()),
         );
         let port_allocator = OptePortAllocator::new();
         let nexus_client = MockNexusClient::default();