Self-review pt.1.

clients/sled-agent-client/src/lib.rs

@@ -35,7 +35,6 @@ progenitor::generate_api!(
         PortConfigV1 = { derives = [PartialEq, Eq, Hash, Serialize, Deserialize] },
         RouteConfig = { derives = [PartialEq, Eq, Hash, Serialize, Deserialize] },
         IpNet = { derives = [PartialEq, Eq, Hash, Serialize, Deserialize] },
-        RouterId = { derives = [PartialEq, Eq, Hash, Debug, Deserialize, Serialize] },
         VirtualNetworkInterfaceHost = { derives = [PartialEq, Eq, Hash, Serialize, Deserialize] },
         OmicronPhysicalDiskConfig = { derives = [Clone, Debug, Serialize, Deserialize, PartialEq, Eq, Hash, PartialOrd, Ord] },
     },
@@ -54,8 +53,8 @@ progenitor::generate_api!(
         PortFec = omicron_common::api::internal::shared::PortFec,
         PortSpeed = omicron_common::api::internal::shared::PortSpeed,
         RouterId = omicron_common::api::internal::shared::RouterId,
-        ReifiedVpcRoute = omicron_common::api::internal::shared::ReifiedVpcRoute,
-        ReifiedVpcRouteSet = omicron_common::api::internal::shared::ReifiedVpcRouteSet,
+        ResolvedVpcRoute = omicron_common::api::internal::shared::ResolvedVpcRoute,
+        ResolvedVpcRouteSet = omicron_common::api::internal::shared::ResolvedVpcRouteSet,
         RouterTarget = omicron_common::api::internal::shared::RouterTarget,
         RouterVersion = omicron_common::api::internal::shared::RouterVersion,
         SourceNatConfig = omicron_common::api::internal::shared::SourceNatConfig,

common/src/api/internal/shared.rs

@@ -594,7 +594,7 @@ impl TryFrom<&[IpNetwork]> for IpAllowList {
 #[derive(
     Clone, Debug, Deserialize, Serialize, JsonSchema, PartialEq, Eq, Hash,
 )]
-pub struct ReifiedVpcRoute {
+pub struct ResolvedVpcRoute {
     pub dest: IpNet,
     pub target: RouterTarget,
 }
@@ -611,23 +611,28 @@ pub enum RouterTarget {
     VpcSubnet(IpNet),
 }
 
-/// XXX
+/// Information on the current parent router (and version) of a route set
+/// according to the control plane.
 #[derive(
     Copy, Clone, Debug, Deserialize, Serialize, JsonSchema, PartialEq, Eq, Hash,
 )]
 pub struct RouterVersion {
     pub router_id: Uuid,
-    pub generation: u64,
+    pub version: u64,
 }
 
 impl RouterVersion {
+    /// Return whether a new route set should be applied over the current
+    /// values.
+    ///
+    /// This will occur when seeing a new version and a matching parent,
+    /// or a new parent router on the control plane.
     pub fn is_replaced_by(&self, other: &Self) -> bool {
-        (self.router_id != other.router_id)
-            || self.generation < other.generation
+        (self.router_id != other.router_id) || self.version < other.version
     }
 }
 
-/// Implementation details on XXX
+/// Identifier for a VPC and/or subnet.
 #[derive(
     Copy, Clone, Debug, Deserialize, Serialize, JsonSchema, PartialEq, Eq, Hash,
 )]
@@ -636,19 +641,19 @@ pub struct RouterId {
     pub subnet: Option<IpNet>,
 }
 
-/// Version information
+/// Version information for routes on a given VPC subnet.
 #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema, PartialEq, Eq)]
-pub struct ReifiedVpcRouteState {
+pub struct ResolvedVpcRouteState {
     pub id: RouterId,
     pub version: Option<RouterVersion>,
 }
 
-/// An updated set of routes for a given
+/// An updated set of routes for a given VPC and/or subnet.
 #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema, PartialEq, Eq)]
-pub struct ReifiedVpcRouteSet {
+pub struct ResolvedVpcRouteSet {
     pub id: RouterId,
     pub version: Option<RouterVersion>,
-    pub routes: HashSet<ReifiedVpcRoute>,
+    pub routes: HashSet<ResolvedVpcRoute>,
 }
 
 #[cfg(test)]

illumos-utils/src/opte/mod.rs

@@ -73,6 +73,7 @@ impl Gateway {
     }
 }
 
+/// Convert a nexus `IpNet` to an OPTE `IpCidr`.
 fn net_to_cidr(net: IpNet) -> IpCidr {
     match net {
         IpNet::V4(net) => IpCidr::Ip4(Ipv4Cidr::new(
@@ -86,6 +87,7 @@ fn net_to_cidr(net: IpNet) -> IpCidr {
     }
 }
 
+/// Convert a nexus `RouterTarget` to an OPTE `RouterTarget`.
 fn router_target_opte(target: &shared::RouterTarget) -> RouterTarget {
     use shared::RouterTarget::*;
     match target {

illumos-utils/src/opte/port_manager.rs

@@ -16,9 +16,9 @@ use ipnetwork::IpNetwork;
 use omicron_common::api::external;
 use omicron_common::api::internal::shared::NetworkInterface;
 use omicron_common::api::internal::shared::NetworkInterfaceKind;
-use omicron_common::api::internal::shared::ReifiedVpcRoute;
-use omicron_common::api::internal::shared::ReifiedVpcRouteSet;
-use omicron_common::api::internal::shared::ReifiedVpcRouteState;
+use omicron_common::api::internal::shared::ResolvedVpcRoute;
+use omicron_common::api::internal::shared::ResolvedVpcRouteSet;
+use omicron_common::api::internal::shared::ResolvedVpcRouteState;
 use omicron_common::api::internal::shared::RouterId;
 use omicron_common::api::internal::shared::RouterTarget as ApiRouterTarget;
 use omicron_common::api::internal::shared::RouterVersion;
@@ -55,28 +55,29 @@ use uuid::Uuid;
 // Prefix used to identify xde data links.
 const XDE_LINK_PREFIX: &str = "opte";
 
+/// Stored routes (and usage count) for a given VPC/subnet.
 #[derive(Debug, Clone)]
 struct RouteSet {
     version: Option<RouterVersion>,
-    routes: HashSet<ReifiedVpcRoute>,
+    routes: HashSet<ResolvedVpcRoute>,
     active_ports: usize,
 }
 
 #[derive(Debug)]
 struct PortManagerInner {
     log: Logger,
 
-    // Sequential identifier for each port on the system.
+    /// Sequential identifier for each port on the system.
     next_port_id: AtomicU64,
 
-    // IP address of the hosting sled on the underlay.
+    /// IP address of the hosting sled on the underlay.
     underlay_ip: Ipv6Addr,
 
-    // Map of all ports, keyed on the interface Uuid and its kind
-    // (which includes the Uuid of the parent instance or service)
+    /// Map of all ports, keyed on the interface Uuid and its kind
+    /// (which includes the Uuid of the parent instance or service)
     ports: Mutex<BTreeMap<(Uuid, NetworkInterfaceKind), Port>>,
 
-    // Map of all current resolved routes.
+    /// Map of all current resolved routes.
     routes: Mutex<HashMap<RouterId, RouteSet>>,
 }
 
@@ -377,6 +378,10 @@ impl PortManager {
             (port, ticket)
         };
 
+        // Check locally to see whether we have any routes from the
+        // control plane for this port already installed. If not,
+        // create a record to show that we're interested in receiving
+        // those routes.
         let mut routes = self.inner.routes.lock().unwrap();
         let system_routes =
             routes.entry(port.system_router_key()).or_insert_with(|| {
@@ -386,11 +391,11 @@ impl PortManager {
                 // to reach out over the Internet *before* nexus is up to give
                 // us real rules. The easiest bet is to instantiate these here.
                 if is_service {
-                    routes.insert(ReifiedVpcRoute {
+                    routes.insert(ResolvedVpcRoute {
                         dest: "0.0.0.0/0".parse().unwrap(),
                         target: ApiRouterTarget::InternetGateway,
                     });
-                    routes.insert(ReifiedVpcRoute {
+                    routes.insert(ResolvedVpcRoute {
                         dest: "::/0".parse().unwrap(),
                         target: ApiRouterTarget::InternetGateway,
                     });
@@ -399,7 +404,7 @@ impl PortManager {
                 RouteSet { version: None, routes, active_ports: 0 }
             });
         system_routes.active_ports += 1;
-        // Needed to get borrowck on our side, sadly.
+        // Clone is needed to get borrowck on our side, sadly.
         let system_routes = system_routes.clone();
 
         let custom_routes = routes
@@ -443,23 +448,23 @@ impl PortManager {
         Ok((port, ticket))
     }
 
-    pub fn vpc_routes_list(&self) -> Vec<ReifiedVpcRouteState> {
+    pub fn vpc_routes_list(&self) -> Vec<ResolvedVpcRouteState> {
         let routes = self.inner.routes.lock().unwrap();
         routes
             .iter()
-            .map(|(k, v)| ReifiedVpcRouteState { id: *k, version: v.version })
+            .map(|(k, v)| ResolvedVpcRouteState { id: *k, version: v.version })
             .collect()
     }
 
     pub fn vpc_routes_ensure(
         &self,
-        new_routes: Vec<ReifiedVpcRouteSet>,
+        new_routes: Vec<ResolvedVpcRouteSet>,
     ) -> Result<(), Error> {
         let mut routes = self.inner.routes.lock().unwrap();
         let mut deltas = HashMap::new();
-        for set in new_routes {
+        for new in new_routes {
             // Disregard any route information for a subnet we don't have.
-            let Some(old) = routes.get(&set.id) else {
+            let Some(old) = routes.get(&new.id) else {
                 continue;
             };
 
@@ -468,34 +473,38 @@ impl PortManager {
             // If there's a UUID match, only update if vers increased,
             // otherwise take the update verbatim (including loss of version).
             let (to_add, to_delete): (HashSet<_>, HashSet<_>) =
-                match (old.version, set.version) {
+                match (old.version, new.version) {
                     (Some(old_vers), Some(new_vers))
                         if !old_vers.is_replaced_by(&new_vers) =>
                     {
                         continue;
                     }
                     _ => (
-                        set.routes.difference(&old.routes).cloned().collect(),
-                        old.routes.difference(&set.routes).cloned().collect(),
+                        new.routes.difference(&old.routes).cloned().collect(),
+                        old.routes.difference(&new.routes).cloned().collect(),
                     ),
                 };
-            deltas.insert(set.id, (to_add, to_delete));
+            deltas.insert(new.id, (to_add, to_delete));
 
             let active_ports = old.active_ports;
             routes.insert(
-                set.id,
+                new.id,
                 RouteSet {
-                    version: set.version,
-                    routes: set.routes,
+                    version: new.version,
+                    routes: new.routes,
                     active_ports,
                 },
             );
         }
 
+        // Note: We're deliberately holding both locks here
+        // to prevent several nexuses computng and applying deltas
+        // out of order.
         let ports = self.inner.ports.lock().unwrap();
         #[cfg(target_os = "illumos")]
         let hdl = opte_ioctl::OpteHdl::open(opte_ioctl::OpteHdl::XDE_CTL)?;
 
+        // Propagate deltas out to all ports.
         for port in ports.values() {
             let system_id = port.system_router_key();
             let system_delta = deltas.get(&system_id);

nexus/db-model/src/schema_versions.rs

@@ -17,7 +17,7 @@ use std::collections::BTreeMap;
 ///
 /// This must be updated when you change the database schema.  Refer to
 /// schema/crdb/README.adoc in the root of this repository for details.
-pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(64, 0, 0);
+pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(65, 0, 0);
 
 /// List of all past database schema versions, in *reverse* order
 ///

nexus/db-queries/src/db/datastore/instance.rs

@@ -441,58 +441,6 @@ impl DataStore {
         Ok(result)
     }
 
-    /// Lists all instances on in-service sleds with active Propolis VMM
-    /// processes, returning the instance along with the VMM on which it's
-    /// running, the sled on which the VMM is running, and the project that owns
-    /// the instance.
-    ///
-    /// The query performed by this function is paginated by the sled's UUID.
-    pub async fn instance_and_vpc_list_by_sled_agent(
-        &self,
-        opctx: &OpContext,
-        pagparams: &DataPageParams<'_, Uuid>,
-    ) -> ListResultVec<(Sled, Instance, Vmm, Project)> {
-        use crate::db::schema::{
-            instance::dsl as instance_dsl, project::dsl as project_dsl,
-            sled::dsl as sled_dsl, vmm::dsl as vmm_dsl,
-        };
-        opctx.authorize(authz::Action::Read, &authz::FLEET).await?;
-        let conn = self.pool_connection_authorized(opctx).await?;
-
-        let result = paginated(sled_dsl::sled, sled_dsl::id, pagparams)
-            .filter(sled_dsl::time_deleted.is_null())
-            .sled_filter(SledFilter::InService)
-            .inner_join(
-                vmm_dsl::vmm
-                    .on(vmm_dsl::sled_id
-                        .eq(sled_dsl::id)
-                        .and(vmm_dsl::time_deleted.is_null()))
-                    .inner_join(
-                        instance_dsl::instance
-                            .on(instance_dsl::id
-                                .eq(vmm_dsl::instance_id)
-                                .and(instance_dsl::time_deleted.is_null()))
-                            .inner_join(
-                                project_dsl::project.on(project_dsl::id
-                                    .eq(instance_dsl::project_id)
-                                    .and(project_dsl::time_deleted.is_null())),
-                            ),
-                    ),
-            )
-            .sled_filter(SledFilter::InService)
-            .select((
-                Sled::as_select(),
-                Instance::as_select(),
-                Vmm::as_select(),
-                Project::as_select(),
-            ))
-            .load_async::<(Sled, Instance, Vmm, Project)>(&*conn)
-            .await
-            .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?;
-
-        Ok(result)
-    }
-
     pub async fn project_delete_instance(
         &self,
         opctx: &OpContext,

nexus/db-queries/src/db/datastore/network_interface.rs

@@ -149,8 +149,8 @@ impl DataStore {
         // NIC of that instance. Accordingly, NIC create may cause dangling
         // entries to re-resolve to a valid instance (even if it is not yet
         // started).
-        // This will not trigger the RPW directly, we still need to do so
-        // in e.g. the instance watcher task.
+        // This will not trigger the route RPW directly, we still need to do
+        // so in e.g. the instance watcher task.
         if out.primary {
             self.vpc_increment_rpw_version(opctx, out.vpc_id)
                 .await