BUGFIX: use sled filter to apply v2p mappings (#5917)

It has been reported that marking a sled as "non_provisionable" causes
it to lose the instance v2p mappings. This is a regression. It was
determined that the scoping of the mappings is too strict, relying on
the sled_policy to be set to "in service" via the db view.

The preferred option is to rely on the sled policy machinery to do the
filtering for us, so the view has been removed and the querying logic
updated accordingly.

Also, based on a comment in a previous related PR, we are now selecting
target sleds for instances using the active_propolis_id to avoid
potential conflicts during live migrations.

## Related
---
#5872

dev-tools/omdb/tests/usage_errors.out

@@ -290,6 +290,7 @@ Options:
             for discretionary services)
           - query-during-inventory: Sleds whose sled agents should be queried for inventory
           - reservation-create:     Sleds on which reservations can be created
+          - v2p-mapping:            Sleds which should be sent OPTE V2P mappings
           - vpc-firewall:           Sleds which should be sent VPC firewall rules
 
       --log-level <LOG_LEVEL>

nexus/db-model/src/schema.rs

@@ -285,17 +285,6 @@ table! {
     }
 }
 
-table! {
-    v2p_mapping_view (nic_id) {
-        nic_id -> Uuid,
-        sled_id -> Uuid,
-        sled_ip -> Inet,
-        vni -> Int4,
-        mac -> Int8,
-        ip -> Inet,
-    }
-}
-
 table! {
     bgp_announce_set (id) {
         id -> Uuid,
@@ -1842,6 +1831,7 @@ allow_tables_to_appear_in_same_query!(
     user_builtin,
     role_builtin,
     role_assignment,
+    probe,
 );
 
 allow_tables_to_appear_in_same_query!(dns_zone, dns_version, dns_name);
@@ -1870,3 +1860,5 @@ joinable!(instance_ssh_key -> ssh_key (ssh_key_id));
 joinable!(instance_ssh_key -> instance (instance_id));
 
 allow_tables_to_appear_in_same_query!(sled, sled_instance);
+
+joinable!(network_interface -> probe (parent_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(76, 0, 0);
+pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(77, 0, 0);
 
 /// List of all past database schema versions, in *reverse* order
 ///
@@ -29,6 +29,7 @@ static KNOWN_VERSIONS: Lazy<Vec<KnownVersion>> = Lazy::new(|| {
         // |  leaving the first copy as an example for the next person.
         // v
         // KnownVersion::new(next_int, "unique-dirname-with-the-sql-files"),
+        KnownVersion::new(77, "remove-view-for-v2p-mappings"),
         KnownVersion::new(76, "lookup-region-snapshot-by-snapshot-id"),
         KnownVersion::new(75, "add-cockroach-zone-id-to-node-id"),
         KnownVersion::new(74, "add-migration-table"),

nexus/db-model/src/sled.rs

@@ -44,7 +44,7 @@ pub struct SledSystemHardware {
 #[diesel(table_name = sled)]
 pub struct Sled {
     #[diesel(embed)]
-    identity: SledIdentity,
+    pub identity: SledIdentity,
     time_deleted: Option<DateTime<Utc>>,
     pub rcgen: Generation,
 

nexus/db-model/src/v2p_mapping.rs

@@ -1,15 +1,17 @@
-use crate::schema::v2p_mapping_view;
-use crate::{MacAddr, Vni};
+use crate::{Ipv6Addr, MacAddr, Vni};
 use ipnetwork::IpNetwork;
 use serde::{Deserialize, Serialize};
 use uuid::Uuid;
 
-#[derive(Queryable, Selectable, Clone, Debug, Serialize, Deserialize)]
-#[diesel(table_name = v2p_mapping_view)]
+/// This is not backed by an actual database view,
+/// but it is still essentially a "view" in the sense
+/// that it is a read-only data model derived from
+/// multiple db tables
+#[derive(Clone, Debug, Serialize, Deserialize)]
 pub struct V2PMappingView {
     pub nic_id: Uuid,
     pub sled_id: Uuid,
-    pub sled_ip: IpNetwork,
+    pub sled_ip: Ipv6Addr,
     pub vni: Vni,
     pub mac: MacAddr,
     pub ip: IpNetwork,

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

@@ -7,34 +7,136 @@ use crate::context::OpContext;
 use crate::db;
 use crate::db::datastore::SQL_BATCH_SIZE;
 use crate::db::error::{public_error_from_diesel, ErrorHandler};
+use crate::db::model::ApplySledFilterExt;
 use crate::db::model::V2PMappingView;
 use crate::db::pagination::paginated;
 use crate::db::pagination::Paginator;
 use async_bb8_diesel::AsyncRunQueryDsl;
-use diesel::{QueryDsl, SelectableHelper};
+use diesel::{ExpressionMethods, QueryDsl, SelectableHelper};
+use diesel::{JoinOnDsl, NullableExpressionMethods};
+use nexus_db_model::{NetworkInterface, NetworkInterfaceKind, Sled, Vpc};
+use nexus_types::deployment::SledFilter;
 use omicron_common::api::external::ListResultVec;
 
 impl DataStore {
     pub async fn v2p_mappings(
         &self,
         opctx: &OpContext,
     ) -> ListResultVec<V2PMappingView> {
-        use db::schema::v2p_mapping_view::dsl;
+        use db::schema::instance::dsl as instance_dsl;
+        use db::schema::network_interface::dsl as network_interface_dsl;
+        use db::schema::probe::dsl as probe_dsl;
+        use db::schema::sled::dsl as sled_dsl;
+        use db::schema::vmm::dsl as vmm_dsl;
+        use db::schema::vpc::dsl as vpc_dsl;
+        use db::schema::vpc_subnet::dsl as vpc_subnet_dsl;
+
+        use db::schema::network_interface;
 
         opctx.check_complex_operations_allowed()?;
 
         let mut mappings = Vec::new();
         let mut paginator = Paginator::new(SQL_BATCH_SIZE);
         while let Some(p) = paginator.next() {
-            let batch = paginated(
-                dsl::v2p_mapping_view,
-                dsl::nic_id,
-                &p.current_pagparams(),
-            )
-            .select(V2PMappingView::as_select())
-            .load_async(&*self.pool_connection_authorized(opctx).await?)
-            .await
-            .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?;
+            let batch: Vec<_> =
+                paginated(
+                    network_interface_dsl::network_interface,
+                    network_interface_dsl::id,
+                    &p.current_pagparams(),
+                )
+                .inner_join(
+                    instance_dsl::instance
+                        .on(network_interface_dsl::parent_id
+                            .eq(instance_dsl::id)),
+                )
+                .inner_join(vmm_dsl::vmm.on(
+                    vmm_dsl::id.nullable().eq(instance_dsl::active_propolis_id),
+                ))
+                .inner_join(vpc_subnet_dsl::vpc_subnet.on(
+                    vpc_subnet_dsl::id.eq(network_interface_dsl::subnet_id),
+                ))
+                .inner_join(
+                    vpc_dsl::vpc
+                        .on(vpc_dsl::id.eq(network_interface_dsl::vpc_id)),
+                )
+                .inner_join(
+                    sled_dsl::sled.on(sled_dsl::id.eq(vmm_dsl::sled_id)),
+                )
+                .filter(network_interface::time_deleted.is_null())
+                .filter(
+                    network_interface::kind.eq(NetworkInterfaceKind::Instance),
+                )
+                .sled_filter(SledFilter::V2PMapping)
+                .select((
+                    NetworkInterface::as_select(),
+                    Sled::as_select(),
+                    Vpc::as_select(),
+                ))
+                .load_async(&*self.pool_connection_authorized(opctx).await?)
+                .await
+                .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?
+                .into_iter()
+                .map(|(nic, sled, vpc): (NetworkInterface, Sled, Vpc)| {
+                    V2PMappingView {
+                        nic_id: nic.identity.id,
+                        sled_id: sled.identity.id,
+                        sled_ip: sled.ip,
+                        vni: vpc.vni,
+                        mac: nic.mac,
+                        ip: nic.ip,
+                    }
+                })
+                .collect();
+
+            paginator = p.found_batch(&batch, &|mapping| mapping.nic_id);
+            mappings.extend(batch);
+        }
+
+        let mut paginator = Paginator::new(SQL_BATCH_SIZE);
+        while let Some(p) = paginator.next() {
+            let batch: Vec<_> =
+                paginated(
+                    network_interface_dsl::network_interface,
+                    network_interface_dsl::id,
+                    &p.current_pagparams(),
+                )
+                .inner_join(
+                    probe_dsl::probe
+                        .on(probe_dsl::id.eq(network_interface_dsl::parent_id)),
+                )
+                .inner_join(vpc_subnet_dsl::vpc_subnet.on(
+                    vpc_subnet_dsl::id.eq(network_interface_dsl::subnet_id),
+                ))
+                .inner_join(
+                    vpc_dsl::vpc
+                        .on(vpc_dsl::id.eq(network_interface_dsl::vpc_id)),
+                )
+                .inner_join(sled_dsl::sled.on(sled_dsl::id.eq(probe_dsl::sled)))
+                .filter(network_interface::time_deleted.is_null())
+                .filter(
+                    network_interface::kind.eq(NetworkInterfaceKind::Instance),
+                )
+                .sled_filter(SledFilter::V2PMapping)
+                .select((
+                    NetworkInterface::as_select(),
+                    Sled::as_select(),
+                    Vpc::as_select(),
+                ))
+                .load_async(&*self.pool_connection_authorized(opctx).await?)
+                .await
+                .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?
+                .into_iter()
+                .map(|(nic, sled, vpc): (NetworkInterface, Sled, Vpc)| {
+                    V2PMappingView {
+                        nic_id: nic.identity.id,
+                        sled_id: sled.identity.id,
+                        sled_ip: sled.ip,
+                        vni: vpc.vni,
+                        mac: nic.mac,
+                        ip: nic.ip,
+                    }
+                })
+                .collect();
 
             paginator = p.found_batch(&batch, &|mapping| mapping.nic_id);
             mappings.extend(batch);

nexus/src/app/background/v2p_mappings.rs

@@ -74,28 +74,13 @@ impl BackgroundTask for V2PManager {
             // create a set of updates from the v2p mappings
             let desired_v2p: HashSet<_> = v2p_mappings
                 .into_iter()
-                .filter_map(|mapping| {
-                    let physical_host_ip = match mapping.sled_ip.ip() {
-                        std::net::IpAddr::V4(v) => {
-                            // sled ip should never be ipv4
-                            error!(
-                                &log,
-                                "sled ip should be ipv6 but is ipv4: {v}"
-                            );
-                            return None;
-                        }
-                        std::net::IpAddr::V6(v) => v,
-                    };
-
-                    let vni = mapping.vni.0;
-
-                    let mapping = VirtualNetworkInterfaceHost {
+                .map(|mapping| {
+                    VirtualNetworkInterfaceHost {
                         virtual_ip: mapping.ip.ip(),
                         virtual_mac: *mapping.mac,
-                        physical_host_ip,
-                        vni,
-                    };
-                    Some(mapping)
+                        physical_host_ip: *mapping.sled_ip,
+                        vni: mapping.vni.0,
+                    }
                 })
                 .collect();
 

nexus/types/src/deployment/planning_input.rs

@@ -482,6 +482,9 @@ pub enum SledFilter {
     /// Sleds on which reservations can be created.
     ReservationCreate,
 
+    /// Sleds which should be sent OPTE V2P mappings.
+    V2PMapping,
+
     /// Sleds which should be sent VPC firewall rules.
     VpcFirewall,
 }
@@ -536,6 +539,7 @@ impl SledPolicy {
                 SledFilter::InService => true,
                 SledFilter::QueryDuringInventory => true,
                 SledFilter::ReservationCreate => true,
+                SledFilter::V2PMapping => true,
                 SledFilter::VpcFirewall => true,
             },
             SledPolicy::InService {
@@ -547,6 +551,7 @@ impl SledPolicy {
                 SledFilter::InService => true,
                 SledFilter::QueryDuringInventory => true,
                 SledFilter::ReservationCreate => false,
+                SledFilter::V2PMapping => true,
                 SledFilter::VpcFirewall => true,
             },
             SledPolicy::Expunged => match filter {
@@ -556,6 +561,7 @@ impl SledPolicy {
                 SledFilter::InService => false,
                 SledFilter::QueryDuringInventory => false,
                 SledFilter::ReservationCreate => false,
+                SledFilter::V2PMapping => false,
                 SledFilter::VpcFirewall => false,
             },
         }
@@ -587,6 +593,7 @@ impl SledState {
                 SledFilter::InService => true,
                 SledFilter::QueryDuringInventory => true,
                 SledFilter::ReservationCreate => true,
+                SledFilter::V2PMapping => true,
                 SledFilter::VpcFirewall => true,
             },
             SledState::Decommissioned => match filter {
@@ -596,6 +603,7 @@ impl SledState {
                 SledFilter::InService => false,
                 SledFilter::QueryDuringInventory => false,
                 SledFilter::ReservationCreate => false,
+                SledFilter::V2PMapping => false,
                 SledFilter::VpcFirewall => false,
             },
         }

schema/crdb/dbinit.sql

@@ -3891,53 +3891,6 @@ ON omicron.public.switch_port (port_settings_id, port_name) STORING (switch_loca
 
 CREATE INDEX IF NOT EXISTS switch_port_name ON omicron.public.switch_port (port_name);
 
-COMMIT;
-BEGIN;
-
--- view for v2p mapping rpw
-CREATE VIEW IF NOT EXISTS omicron.public.v2p_mapping_view
-AS
-WITH VmV2pMappings AS (
-  SELECT
-    n.id as nic_id,
-    s.id as sled_id,
-    s.ip as sled_ip,
-    v.vni,
-    n.mac,
-    n.ip
-  FROM omicron.public.network_interface n
-  JOIN omicron.public.vpc_subnet vs ON vs.id = n.subnet_id
-  JOIN omicron.public.vpc v ON v.id = n.vpc_id
-  JOIN omicron.public.vmm vmm ON n.parent_id = vmm.instance_id
-  JOIN omicron.public.sled s ON vmm.sled_id = s.id
-  WHERE n.time_deleted IS NULL
-  AND n.kind = 'instance'
-  AND (vmm.state = 'running' OR vmm.state = 'starting')
-  AND s.sled_policy = 'in_service'
-  AND s.sled_state = 'active'
-),
-ProbeV2pMapping AS (
-  SELECT
-    n.id as nic_id,
-    s.id as sled_id,
-    s.ip as sled_ip,
-    v.vni,
-    n.mac,
-    n.ip
-  FROM omicron.public.network_interface n
-  JOIN omicron.public.vpc_subnet vs ON vs.id = n.subnet_id
-  JOIN omicron.public.vpc v ON v.id = n.vpc_id
-  JOIN omicron.public.probe p ON n.parent_id = p.id
-  JOIN omicron.public.sled s ON p.sled = s.id
-  WHERE n.time_deleted IS NULL
-  AND n.kind = 'probe'
-  AND s.sled_policy = 'in_service'
-  AND s.sled_state = 'active'
-)
-SELECT nic_id, sled_id, sled_ip, vni, mac, ip FROM VmV2pMappings
-UNION
-SELECT nic_id, sled_id, sled_ip, vni, mac, ip FROM ProbeV2pMapping;
-
 CREATE INDEX IF NOT EXISTS network_interface_by_parent
 ON omicron.public.network_interface (parent_id)
 STORING (name, kind, vpc_id, subnet_id, mac, ip, slot);
@@ -4145,7 +4098,7 @@ INSERT INTO omicron.public.db_metadata (
     version,
     target_version
 ) VALUES
-    (TRUE, NOW(), NOW(), '76.0.0', NULL)
+    (TRUE, NOW(), NOW(), '77.0.0', NULL)
 ON CONFLICT DO NOTHING;
 
 COMMIT;

schema/crdb/remove-view-for-v2p-mappings/up01.sql

@@ -0,0 +1 @@
+DROP VIEW IF EXISTS omicron.public.v2p_mapping_view;