From 64610780573d0f87475b9abc77fe7c6afe7441ba Mon Sep 17 00:00:00 2001 From: Levon Tarver <11586085+internet-diglett@users.noreply.github.com> Date: Thu, 20 Jun 2024 13:00:05 -0500 Subject: [PATCH] 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 | 1 + nexus/db-model/src/schema.rs | 14 +- nexus/db-model/src/schema_versions.rs | 3 +- nexus/db-model/src/sled.rs | 2 +- nexus/db-model/src/v2p_mapping.rs | 12 +- .../src/db/datastore/v2p_mapping.rs | 124 ++++++++++++++++-- nexus/src/app/background/v2p_mappings.rs | 25 +--- nexus/types/src/deployment/planning_input.rs | 8 ++ schema/crdb/dbinit.sql | 49 +------ .../remove-view-for-v2p-mappings/up01.sql | 1 + 10 files changed, 142 insertions(+), 97 deletions(-) create mode 100644 schema/crdb/remove-view-for-v2p-mappings/up01.sql diff --git a/dev-tools/omdb/tests/usage_errors.out b/dev-tools/omdb/tests/usage_errors.out index 563d23d6f3..b7fe7bdf7f 100644 --- a/dev-tools/omdb/tests/usage_errors.out +++ b/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 diff --git a/nexus/db-model/src/schema.rs b/nexus/db-model/src/schema.rs index b1fabcf976..d08a51edd4 100644 --- a/nexus/db-model/src/schema.rs +++ b/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)); diff --git a/nexus/db-model/src/schema_versions.rs b/nexus/db-model/src/schema_versions.rs index eed8c81421..04fafe4f93 100644 --- a/nexus/db-model/src/schema_versions.rs +++ b/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> = 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"), diff --git a/nexus/db-model/src/sled.rs b/nexus/db-model/src/sled.rs index 5019366733..c177650991 100644 --- a/nexus/db-model/src/sled.rs +++ b/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>, pub rcgen: Generation, diff --git a/nexus/db-model/src/v2p_mapping.rs b/nexus/db-model/src/v2p_mapping.rs index 43831f7503..bb7c74b83f 100644 --- a/nexus/db-model/src/v2p_mapping.rs +++ b/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, diff --git a/nexus/db-queries/src/db/datastore/v2p_mapping.rs b/nexus/db-queries/src/db/datastore/v2p_mapping.rs index 6c00957e7d..2f54dfb9be 100644 --- a/nexus/db-queries/src/db/datastore/v2p_mapping.rs +++ b/nexus/db-queries/src/db/datastore/v2p_mapping.rs @@ -7,11 +7,15 @@ 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 { @@ -19,22 +23,120 @@ impl DataStore { &self, opctx: &OpContext, ) -> ListResultVec { - 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); diff --git a/nexus/src/app/background/v2p_mappings.rs b/nexus/src/app/background/v2p_mappings.rs index a53ac3442f..e2318f94d6 100644 --- a/nexus/src/app/background/v2p_mappings.rs +++ b/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(); diff --git a/nexus/types/src/deployment/planning_input.rs b/nexus/types/src/deployment/planning_input.rs index 10d528bbfd..028f2301ba 100644 --- a/nexus/types/src/deployment/planning_input.rs +++ b/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, }, } diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index 338f52f854..905fd111c1 100644 --- a/schema/crdb/dbinit.sql +++ b/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; diff --git a/schema/crdb/remove-view-for-v2p-mappings/up01.sql b/schema/crdb/remove-view-for-v2p-mappings/up01.sql new file mode 100644 index 0000000000..aebe0119f5 --- /dev/null +++ b/schema/crdb/remove-view-for-v2p-mappings/up01.sql @@ -0,0 +1 @@ +DROP VIEW IF EXISTS omicron.public.v2p_mapping_view;