From 68456800d8d9d0c585045d96ef3d7fae30f5af90 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Thu, 18 Jan 2024 09:04:14 -0500 Subject: [PATCH 01/24] add blueprints to database schema and models --- nexus/db-model/src/deployment.rs | 193 +++++++++ nexus/db-model/src/inventory.rs | 448 ++++----------------- nexus/db-model/src/lib.rs | 2 + nexus/db-model/src/omicron_zone_config.rs | 452 ++++++++++++++++++++++ nexus/db-model/src/schema.rs | 74 +++- nexus/types/src/deployment.rs | 1 + schema/crdb/25.0.0/up1.sql | 7 + schema/crdb/25.0.0/up2.sql | 6 + schema/crdb/25.0.0/up3.sql | 31 ++ schema/crdb/25.0.0/up4.sql | 13 + schema/crdb/25.0.0/up5.sql | 6 + schema/crdb/dbinit.sql | 155 +++++++- 12 files changed, 1009 insertions(+), 379 deletions(-) create mode 100644 nexus/db-model/src/deployment.rs create mode 100644 nexus/db-model/src/omicron_zone_config.rs create mode 100644 schema/crdb/25.0.0/up1.sql create mode 100644 schema/crdb/25.0.0/up2.sql create mode 100644 schema/crdb/25.0.0/up3.sql create mode 100644 schema/crdb/25.0.0/up4.sql create mode 100644 schema/crdb/25.0.0/up5.sql diff --git a/nexus/db-model/src/deployment.rs b/nexus/db-model/src/deployment.rs new file mode 100644 index 0000000000..3100306ed9 --- /dev/null +++ b/nexus/db-model/src/deployment.rs @@ -0,0 +1,193 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +//! Types for representing the deployed software and configuration in the +//! database + +use crate::inventory::ZoneType; +use crate::omicron_zone_config::{OmicronZoneNic, OmicronZone}; +use crate::schema::{ + blueprint, bp_omicron_zone, bp_omicron_zone_nic, + bp_omicron_zones_not_in_service, bp_sled_omicron_zones, +}; +use crate::{ipv6, Generation, MacAddr, Name, SqlU16, SqlU32, SqlU8}; +use chrono::{DateTime, Utc}; +use ipnetwork::IpNetwork; +use uuid::Uuid; + +/// See [`nexus_types::deployment::Blueprint`]. +#[derive(Queryable, Insertable, Clone, Debug, Selectable)] +#[diesel(table_name = blueprint)] +pub struct Blueprint { + pub id: Uuid, + pub parent_blueprint_id: Option, + pub time_created: DateTime, + pub creator: String, + pub comment: String, +} + +/// See [`nexus_types::deployment::OmicronZonesConfig`]. +#[derive(Queryable, Clone, Debug, Selectable, Insertable)] +#[diesel(table_name = bp_sled_omicron_zones)] +pub struct BpSledOmicronZones { + pub blueprint_id: Uuid, + pub sled_id: Uuid, + pub generation: Generation, +} + +/// See [`nexus_types::deployment::OmicronZoneConfig`]. +#[derive(Queryable, Clone, Debug, Selectable, Insertable)] +#[diesel(table_name = bp_omicron_zone)] +pub struct BpOmicronZone { + pub blueprint_id: Uuid, + pub sled_id: Uuid, + pub id: Uuid, + pub underlay_address: ipv6::Ipv6Addr, + pub zone_type: ZoneType, + pub primary_service_ip: ipv6::Ipv6Addr, + pub primary_service_port: SqlU16, + pub second_service_ip: Option, + pub second_service_port: Option, + pub dataset_zpool_name: Option, + pub bp_nic_id: Option, + pub dns_gz_address: Option, + pub dns_gz_address_index: Option, + pub ntp_ntp_servers: Option>, + pub ntp_dns_servers: Option>, + pub ntp_domain: Option, + pub nexus_external_tls: Option, + pub nexus_external_dns_servers: Option>, + pub snat_ip: Option, + pub snat_first_port: Option, + pub snat_last_port: Option, +} + +impl BpOmicronZone { + pub fn new( + blueprint_id: Uuid, + sled_id: Uuid, + zone: &nexus_types::inventory::OmicronZoneConfig, + ) -> Result { + let zone = OmicronZone::new(sled_id, zone)?; + Ok(Self { + blueprint_id, + sled_id: zone.sled_id, + id: zone.id, + underlay_address: zone.underlay_address, + zone_type: zone.zone_type, + primary_service_ip: zone.primary_service_ip, + primary_service_port: zone.primary_service_port, + second_service_ip: zone.second_service_ip, + second_service_port: zone.second_service_port, + dataset_zpool_name: zone.dataset_zpool_name, + bp_nic_id: zone.nic_id, + dns_gz_address: zone.dns_gz_address, + dns_gz_address_index: zone.dns_gz_address_index, + ntp_ntp_servers: zone.ntp_ntp_servers, + ntp_dns_servers: zone.ntp_dns_servers, + ntp_domain: zone.ntp_domain, + nexus_external_tls: zone.nexus_external_tls, + nexus_external_dns_servers: zone.nexus_external_dns_servers, + snat_ip: zone.snat_ip, + snat_first_port: zone.snat_first_port, + snat_last_port: zone.snat_last_port, + }) + } + + pub fn into_omicron_zone_config( + self, + nic_row: Option, + ) -> Result { + let zone = OmicronZone { + sled_id: self.sled_id, + id: self.id, + underlay_address: self.underlay_address, + zone_type: self.zone_type, + primary_service_ip: self.primary_service_ip, + primary_service_port: self.primary_service_port, + second_service_ip: self.second_service_ip, + second_service_port: self.second_service_port, + dataset_zpool_name: self.dataset_zpool_name, + nic_id: self.bp_nic_id, + dns_gz_address: self.dns_gz_address, + dns_gz_address_index: self.dns_gz_address_index, + ntp_ntp_servers: self.ntp_ntp_servers, + ntp_dns_servers: self.ntp_dns_servers, + ntp_domain: self.ntp_domain, + nexus_external_tls: self.nexus_external_tls, + nexus_external_dns_servers: self.nexus_external_dns_servers, + snat_ip: self.snat_ip, + snat_first_port: self.snat_first_port, + snat_last_port: self.snat_last_port, + }; + zone.into_omicron_zone_config(nic_row.map(OmicronZoneNic::from)) + } +} + +#[derive(Queryable, Clone, Debug, Selectable, Insertable)] +#[diesel(table_name = bp_omicron_zone_nic)] +pub struct BpOmicronZoneNic { + blueprint_id: Uuid, + pub id: Uuid, + name: Name, + ip: IpNetwork, + mac: MacAddr, + subnet: IpNetwork, + vni: SqlU32, + is_primary: bool, + slot: SqlU8, +} + +impl From for OmicronZoneNic { + fn from(value: BpOmicronZoneNic) -> Self { + OmicronZoneNic { + id: value.id, + name: value.name, + ip: value.ip, + mac: value.mac, + subnet: value.subnet, + vni: value.vni, + is_primary: value.is_primary, + slot: value.slot, + } + } +} + +impl BpOmicronZoneNic { + pub fn new( + blueprint_id: Uuid, + zone: &nexus_types::inventory::OmicronZoneConfig, + ) -> Result, anyhow::Error> { + let zone_nic = OmicronZoneNic::new(zone)?; + Ok(zone_nic.map(|nic| Self { + blueprint_id, + id: nic.id, + name: nic.name, + ip: nic.ip, + mac: nic.mac, + subnet: nic.subnet, + vni: nic.vni, + is_primary: nic.is_primary, + slot: nic.slot, + })) + } + + pub fn into_network_interface_for_zone( + self, + zone_id: Uuid, + ) -> Result { + let zone_nic = OmicronZoneNic::from(self); + zone_nic.into_network_interface_for_zone(zone_id) + } +} + +/// Nexus wants to think in terms of "zones in service", but since most zones of +/// most blueprints are in service, we store the zones NOT in service in the +/// database. We handle that inversion internally in the db-queries layer. +#[derive(Queryable, Clone, Debug, Selectable, Insertable)] +#[diesel(table_name = bp_omicron_zones_not_in_service)] +pub struct BpOmicronZoneNotInService { + blueprint_id: Uuid, + bp_omicron_zone_id: Uuid, +} diff --git a/nexus/db-model/src/inventory.rs b/nexus/db-model/src/inventory.rs index 17d74be0aa..d8314f97b8 100644 --- a/nexus/db-model/src/inventory.rs +++ b/nexus/db-model/src/inventory.rs @@ -4,6 +4,7 @@ //! Types for representing the hardware/software inventory in the database +use crate::omicron_zone_config::{OmicronZone, OmicronZoneNic}; use crate::schema::{ hw_baseboard_id, inv_caboose, inv_collection, inv_collection_error, inv_omicron_zone, inv_omicron_zone_nic, inv_root_of_trust, @@ -14,8 +15,7 @@ use crate::{ impl_enum_type, ipv6, ByteCount, Generation, MacAddr, Name, SqlU16, SqlU32, SqlU8, }; -use anyhow::{anyhow, ensure}; -use anyhow::{bail, Context}; +use anyhow::anyhow; use chrono::DateTime; use chrono::Utc; use diesel::backend::Backend; @@ -26,10 +26,8 @@ use diesel::serialize::ToSql; use diesel::{serialize, sql_types}; use ipnetwork::IpNetwork; use nexus_types::inventory::{ - BaseboardId, Caboose, Collection, OmicronZoneType, PowerState, RotPage, - RotSlot, + BaseboardId, Caboose, Collection, PowerState, RotPage, RotSlot, }; -use std::net::SocketAddrV6; use uuid::Uuid; // See [`nexus_types::inventory::PowerState`]. @@ -750,165 +748,29 @@ impl InvOmicronZone { sled_id: Uuid, zone: &nexus_types::inventory::OmicronZoneConfig, ) -> Result { - let id = zone.id; - let underlay_address = ipv6::Ipv6Addr::from(zone.underlay_address); - let mut nic_id = None; - let mut dns_gz_address = None; - let mut dns_gz_address_index = None; - let mut ntp_ntp_servers = None; - let mut ntp_dns_servers = None; - let mut ntp_ntp_domain = None; - let mut nexus_external_tls = None; - let mut nexus_external_dns_servers = None; - let mut snat_ip = None; - let mut snat_first_port = None; - let mut snat_last_port = None; - let mut second_service_ip = None; - let mut second_service_port = None; - - let (zone_type, primary_service_sockaddr_str, dataset) = match &zone - .zone_type - { - OmicronZoneType::BoundaryNtp { - address, - ntp_servers, - dns_servers, - domain, - nic, - snat_cfg, - } => { - ntp_ntp_servers = Some(ntp_servers.clone()); - ntp_dns_servers = Some(dns_servers.clone()); - ntp_ntp_domain = domain.clone(); - snat_ip = Some(IpNetwork::from(snat_cfg.ip)); - snat_first_port = Some(SqlU16::from(snat_cfg.first_port)); - snat_last_port = Some(SqlU16::from(snat_cfg.last_port)); - nic_id = Some(nic.id); - (ZoneType::BoundaryNtp, address, None) - } - OmicronZoneType::Clickhouse { address, dataset } => { - (ZoneType::Clickhouse, address, Some(dataset)) - } - OmicronZoneType::ClickhouseKeeper { address, dataset } => { - (ZoneType::ClickhouseKeeper, address, Some(dataset)) - } - OmicronZoneType::CockroachDb { address, dataset } => { - (ZoneType::CockroachDb, address, Some(dataset)) - } - OmicronZoneType::Crucible { address, dataset } => { - (ZoneType::Crucible, address, Some(dataset)) - } - OmicronZoneType::CruciblePantry { address } => { - (ZoneType::CruciblePantry, address, None) - } - OmicronZoneType::ExternalDns { - dataset, - http_address, - dns_address, - nic, - } => { - nic_id = Some(nic.id); - let sockaddr = dns_address - .parse::() - .with_context(|| { - format!( - "parsing address for external DNS server {:?}", - dns_address - ) - })?; - second_service_ip = Some(sockaddr.ip()); - second_service_port = Some(SqlU16::from(sockaddr.port())); - (ZoneType::ExternalDns, http_address, Some(dataset)) - } - OmicronZoneType::InternalDns { - dataset, - http_address, - dns_address, - gz_address, - gz_address_index, - } => { - dns_gz_address = Some(ipv6::Ipv6Addr::from(gz_address)); - dns_gz_address_index = Some(SqlU32::from(*gz_address_index)); - let sockaddr = dns_address - .parse::() - .with_context(|| { - format!( - "parsing address for internal DNS server {:?}", - dns_address - ) - })?; - second_service_ip = Some(sockaddr.ip()); - second_service_port = Some(SqlU16::from(sockaddr.port())); - (ZoneType::InternalDns, http_address, Some(dataset)) - } - OmicronZoneType::InternalNtp { - address, - ntp_servers, - dns_servers, - domain, - } => { - ntp_ntp_servers = Some(ntp_servers.clone()); - ntp_dns_servers = Some(dns_servers.clone()); - ntp_ntp_domain = domain.clone(); - (ZoneType::InternalNtp, address, None) - } - OmicronZoneType::Nexus { - internal_address, - external_ip, - nic, - external_tls, - external_dns_servers, - } => { - nic_id = Some(nic.id); - nexus_external_tls = Some(*external_tls); - nexus_external_dns_servers = Some(external_dns_servers.clone()); - second_service_ip = Some(*external_ip); - (ZoneType::Nexus, internal_address, None) - } - OmicronZoneType::Oximeter { address } => { - (ZoneType::Oximeter, address, None) - } - }; - - let dataset_zpool_name = - dataset.map(|d| d.pool_name.as_str().to_string()); - let primary_service_sockaddr = primary_service_sockaddr_str - .parse::() - .with_context(|| { - format!( - "parsing socket address for primary IP {:?}", - primary_service_sockaddr_str - ) - })?; - let (primary_service_ip, primary_service_port) = ( - ipv6::Ipv6Addr::from(*primary_service_sockaddr.ip()), - SqlU16::from(primary_service_sockaddr.port()), - ); - - Ok(InvOmicronZone { + let zone = OmicronZone::new(sled_id, zone)?; + Ok(Self { inv_collection_id, - sled_id, - id, - underlay_address, - zone_type, - primary_service_ip, - primary_service_port, - second_service_ip: second_service_ip.map(IpNetwork::from), - second_service_port, - dataset_zpool_name, - nic_id, - dns_gz_address, - dns_gz_address_index, - ntp_ntp_servers, - ntp_dns_servers: ntp_dns_servers - .map(|list| list.into_iter().map(IpNetwork::from).collect()), - ntp_domain: ntp_ntp_domain, - nexus_external_tls, - nexus_external_dns_servers: nexus_external_dns_servers - .map(|list| list.into_iter().map(IpNetwork::from).collect()), - snat_ip, - snat_first_port, - snat_last_port, + sled_id: zone.sled_id, + id: zone.id, + underlay_address: zone.underlay_address, + zone_type: zone.zone_type, + primary_service_ip: zone.primary_service_ip, + primary_service_port: zone.primary_service_port, + second_service_ip: zone.second_service_ip, + second_service_port: zone.second_service_port, + dataset_zpool_name: zone.dataset_zpool_name, + nic_id: zone.nic_id, + dns_gz_address: zone.dns_gz_address, + dns_gz_address_index: zone.dns_gz_address_index, + ntp_ntp_servers: zone.ntp_ntp_servers, + ntp_dns_servers: zone.ntp_dns_servers, + ntp_domain: zone.ntp_domain, + nexus_external_tls: zone.nexus_external_tls, + nexus_external_dns_servers: zone.nexus_external_dns_servers, + snat_ip: zone.snat_ip, + snat_first_port: zone.snat_first_port, + snat_last_port: zone.snat_last_port, }) } @@ -916,169 +778,29 @@ impl InvOmicronZone { self, nic_row: Option, ) -> Result { - let address = SocketAddrV6::new( - std::net::Ipv6Addr::from(self.primary_service_ip), - *self.primary_service_port, - 0, - 0, - ) - .to_string(); - - // Assemble a value that we can use to extract the NIC _if necessary_ - // and report an error if it was needed but not found. - // - // Any error here should be impossible. By the time we get here, the - // caller should have provided `nic_row` iff there's a corresponding - // `nic_id` in this row, and the ids should match up. And whoever - // created this row ought to have provided a nic_id iff this type of - // zone needs a NIC. This last issue is not under our control, though, - // so we definitely want to handle that as an operational error. The - // others could arguably be programmer errors (i.e., we could `assert`), - // but it seems excessive to crash here. - // - // Note that we immediately return for any of the caller errors here. - // For the other error, we will return only later, if some code path - // below tries to use `nic` when it's not present. - let nic = match (self.nic_id, nic_row) { - (Some(expected_id), Some(nic_row)) => { - ensure!(expected_id == nic_row.id, "caller provided wrong NIC"); - Ok(nic_row.into_network_interface_for_zone(self.id)?) - } - (None, None) => Err(anyhow!( - "expected zone to have an associated NIC, but it doesn't" - )), - (Some(_), None) => bail!("caller provided no NIC"), - (None, Some(_)) => bail!("caller unexpectedly provided a NIC"), - }; - - // Similarly, assemble a value that we can use to extract the dataset, - // if necessary. We only return this error if code below tries to use - // this value. - let dataset = self - .dataset_zpool_name - .map(|zpool_name| -> Result<_, anyhow::Error> { - Ok(nexus_types::inventory::OmicronZoneDataset { - pool_name: zpool_name.parse().map_err(|e| { - anyhow!("parsing zpool name {:?}: {}", zpool_name, e) - })?, - }) - }) - .transpose()? - .ok_or_else(|| anyhow!("expected dataset zpool name, found none")); - - // Do the same for the DNS server address. - let dns_address = - match (self.second_service_ip, self.second_service_port) { - (Some(dns_ip), Some(dns_port)) => { - Ok(std::net::SocketAddr::new(dns_ip.ip(), *dns_port) - .to_string()) - } - _ => Err(anyhow!( - "expected second service IP and port, \ - found one missing" - )), - }; - - // Do the same for NTP zone properties. - let ntp_dns_servers = self - .ntp_dns_servers - .ok_or_else(|| anyhow!("expected list of DNS servers, found null")) - .map(|list| { - list.into_iter().map(|ipnetwork| ipnetwork.ip()).collect() - }); - let ntp_ntp_servers = - self.ntp_ntp_servers.ok_or_else(|| anyhow!("expected ntp_servers")); - - let zone_type = match self.zone_type { - ZoneType::BoundaryNtp => { - let snat_cfg = match ( - self.snat_ip, - self.snat_first_port, - self.snat_last_port, - ) { - (Some(ip), Some(first_port), Some(last_port)) => { - nexus_types::inventory::SourceNatConfig { - ip: ip.ip(), - first_port: *first_port, - last_port: *last_port, - } - } - _ => bail!( - "expected non-NULL snat properties, \ - found at least one NULL" - ), - }; - OmicronZoneType::BoundaryNtp { - address, - dns_servers: ntp_dns_servers?, - domain: self.ntp_domain, - nic: nic?, - ntp_servers: ntp_ntp_servers?, - snat_cfg, - } - } - ZoneType::Clickhouse => { - OmicronZoneType::Clickhouse { address, dataset: dataset? } - } - ZoneType::ClickhouseKeeper => { - OmicronZoneType::ClickhouseKeeper { address, dataset: dataset? } - } - ZoneType::CockroachDb => { - OmicronZoneType::CockroachDb { address, dataset: dataset? } - } - ZoneType::Crucible => { - OmicronZoneType::Crucible { address, dataset: dataset? } - } - ZoneType::CruciblePantry => { - OmicronZoneType::CruciblePantry { address } - } - ZoneType::ExternalDns => OmicronZoneType::ExternalDns { - dataset: dataset?, - dns_address: dns_address?, - http_address: address, - nic: nic?, - }, - ZoneType::InternalDns => OmicronZoneType::InternalDns { - dataset: dataset?, - dns_address: dns_address?, - http_address: address, - gz_address: *self.dns_gz_address.ok_or_else(|| { - anyhow!("expected dns_gz_address, found none") - })?, - gz_address_index: *self.dns_gz_address_index.ok_or_else( - || anyhow!("expected dns_gz_address_index, found none"), - )?, - }, - ZoneType::InternalNtp => OmicronZoneType::InternalNtp { - address, - dns_servers: ntp_dns_servers?, - domain: self.ntp_domain, - ntp_servers: ntp_ntp_servers?, - }, - ZoneType::Nexus => OmicronZoneType::Nexus { - internal_address: address, - nic: nic?, - external_tls: self - .nexus_external_tls - .ok_or_else(|| anyhow!("expected 'external_tls'"))?, - external_ip: self - .second_service_ip - .ok_or_else(|| anyhow!("expected second service IP"))? - .ip(), - external_dns_servers: self - .nexus_external_dns_servers - .ok_or_else(|| anyhow!("expected 'external_dns_servers'"))? - .into_iter() - .map(|i| i.ip()) - .collect(), - }, - ZoneType::Oximeter => OmicronZoneType::Oximeter { address }, - }; - Ok(nexus_types::inventory::OmicronZoneConfig { + let zone = OmicronZone { + sled_id: self.sled_id, id: self.id, - underlay_address: std::net::Ipv6Addr::from(self.underlay_address), - zone_type, - }) + underlay_address: self.underlay_address, + zone_type: self.zone_type, + primary_service_ip: self.primary_service_ip, + primary_service_port: self.primary_service_port, + second_service_ip: self.second_service_ip, + second_service_port: self.second_service_port, + dataset_zpool_name: self.dataset_zpool_name, + nic_id: self.nic_id, + dns_gz_address: self.dns_gz_address, + dns_gz_address_index: self.dns_gz_address_index, + ntp_ntp_servers: self.ntp_ntp_servers, + ntp_dns_servers: self.ntp_dns_servers, + ntp_domain: self.ntp_domain, + nexus_external_tls: self.nexus_external_tls, + nexus_external_dns_servers: self.nexus_external_dns_servers, + snat_ip: self.snat_ip, + snat_first_port: self.snat_first_port, + snat_last_port: self.snat_last_port, + }; + zone.into_omicron_zone_config(nic_row.map(OmicronZoneNic::from)) } } @@ -1096,63 +818,45 @@ pub struct InvOmicronZoneNic { slot: SqlU8, } +impl From for OmicronZoneNic { + fn from(value: InvOmicronZoneNic) -> Self { + OmicronZoneNic { + id: value.id, + name: value.name, + ip: value.ip, + mac: value.mac, + subnet: value.subnet, + vni: value.vni, + is_primary: value.is_primary, + slot: value.slot, + } + } +} + impl InvOmicronZoneNic { pub fn new( inv_collection_id: Uuid, zone: &nexus_types::inventory::OmicronZoneConfig, ) -> Result, anyhow::Error> { - match &zone.zone_type { - OmicronZoneType::ExternalDns { nic, .. } - | OmicronZoneType::BoundaryNtp { nic, .. } - | OmicronZoneType::Nexus { nic, .. } => { - // We do not bother storing the NIC's kind and associated id - // because it should be inferrable from the other information - // that we have. Verify that here. - ensure!( - matches!( - nic.kind, - nexus_types::inventory::NetworkInterfaceKind::Service( - id - ) if id == zone.id - ), - "expected zone's NIC kind to be \"service\" and the \ - id to match the zone's id ({})", - zone.id - ); - - Ok(Some(InvOmicronZoneNic { - inv_collection_id, - id: nic.id, - name: Name::from(nic.name.clone()), - ip: IpNetwork::from(nic.ip), - mac: MacAddr::from(nic.mac), - subnet: IpNetwork::from(nic.subnet.clone()), - vni: SqlU32::from(u32::from(nic.vni)), - is_primary: nic.primary, - slot: SqlU8::from(nic.slot), - })) - } - _ => Ok(None), - } + let zone_nic = OmicronZoneNic::new(zone)?; + Ok(zone_nic.map(|nic| Self { + inv_collection_id, + id: nic.id, + name: nic.name, + ip: nic.ip, + mac: nic.mac, + subnet: nic.subnet, + vni: nic.vni, + is_primary: nic.is_primary, + slot: nic.slot, + })) } pub fn into_network_interface_for_zone( self, zone_id: Uuid, ) -> Result { - Ok(nexus_types::inventory::NetworkInterface { - id: self.id, - ip: self.ip.ip(), - kind: nexus_types::inventory::NetworkInterfaceKind::Service( - zone_id, - ), - mac: *self.mac, - name: self.name.into(), - primary: self.is_primary, - slot: *self.slot, - vni: omicron_common::api::external::Vni::try_from(*self.vni) - .context("parsing VNI")?, - subnet: self.subnet.into(), - }) + let zone_nic = OmicronZoneNic::from(self); + zone_nic.into_network_interface_for_zone(zone_id) } } diff --git a/nexus/db-model/src/lib.rs b/nexus/db-model/src/lib.rs index 5c0a68c253..48f9f66229 100644 --- a/nexus/db-model/src/lib.rs +++ b/nexus/db-model/src/lib.rs @@ -52,7 +52,9 @@ mod switch_port; // These actually represent subqueries, not real table. // However, they must be defined in the same crate as our tables // for join-based marker trait generation. +mod deployment; mod ipv4_nat_entry; +mod omicron_zone_config; pub mod queries; mod quota; mod rack; diff --git a/nexus/db-model/src/omicron_zone_config.rs b/nexus/db-model/src/omicron_zone_config.rs new file mode 100644 index 0000000000..e85242266e --- /dev/null +++ b/nexus/db-model/src/omicron_zone_config.rs @@ -0,0 +1,452 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +//! Types for sharing nontrivial conversions between various `OmicronZoneConfig` +//! database serializations and the corresponding Nexus/sled-agent type +//! +//! Both inventory and deployment have nearly-identical tables to serialize +//! `OmicronZoneConfigs` that are collected or generated, respectively. We +//! expect those tables to diverge over time (e.g., inventory may start +//! collecting extra metadata like uptime). This module provides conversion +//! helpers for the parts of those tables that are common between the two. + +use std::net::SocketAddrV6; + +use crate::inventory::ZoneType; +use crate::{ipv6, MacAddr, Name, SqlU16, SqlU32, SqlU8}; +use anyhow::{anyhow, bail, ensure, Context}; +use ipnetwork::IpNetwork; +use nexus_types::inventory::OmicronZoneType; +use uuid::Uuid; + +#[derive(Debug)] +pub(crate) struct OmicronZone { + pub(crate) sled_id: Uuid, + pub(crate) id: Uuid, + pub(crate) underlay_address: ipv6::Ipv6Addr, + pub(crate) zone_type: ZoneType, + pub(crate) primary_service_ip: ipv6::Ipv6Addr, + pub(crate) primary_service_port: SqlU16, + pub(crate) second_service_ip: Option, + pub(crate) second_service_port: Option, + pub(crate) dataset_zpool_name: Option, + pub(crate) nic_id: Option, + pub(crate) dns_gz_address: Option, + pub(crate) dns_gz_address_index: Option, + pub(crate) ntp_ntp_servers: Option>, + pub(crate) ntp_dns_servers: Option>, + pub(crate) ntp_domain: Option, + pub(crate) nexus_external_tls: Option, + pub(crate) nexus_external_dns_servers: Option>, + pub(crate) snat_ip: Option, + pub(crate) snat_first_port: Option, + pub(crate) snat_last_port: Option, +} + +impl OmicronZone { + pub(crate) fn new( + sled_id: Uuid, + zone: &nexus_types::inventory::OmicronZoneConfig, + ) -> anyhow::Result { + let id = zone.id; + let underlay_address = ipv6::Ipv6Addr::from(zone.underlay_address); + let mut nic_id = None; + let mut dns_gz_address = None; + let mut dns_gz_address_index = None; + let mut ntp_ntp_servers = None; + let mut ntp_dns_servers = None; + let mut ntp_ntp_domain = None; + let mut nexus_external_tls = None; + let mut nexus_external_dns_servers = None; + let mut snat_ip = None; + let mut snat_first_port = None; + let mut snat_last_port = None; + let mut second_service_ip = None; + let mut second_service_port = None; + + let (zone_type, primary_service_sockaddr_str, dataset) = match &zone + .zone_type + { + OmicronZoneType::BoundaryNtp { + address, + ntp_servers, + dns_servers, + domain, + nic, + snat_cfg, + } => { + ntp_ntp_servers = Some(ntp_servers.clone()); + ntp_dns_servers = Some(dns_servers.clone()); + ntp_ntp_domain = domain.clone(); + snat_ip = Some(IpNetwork::from(snat_cfg.ip)); + snat_first_port = Some(SqlU16::from(snat_cfg.first_port)); + snat_last_port = Some(SqlU16::from(snat_cfg.last_port)); + nic_id = Some(nic.id); + (ZoneType::BoundaryNtp, address, None) + } + OmicronZoneType::Clickhouse { address, dataset } => { + (ZoneType::Clickhouse, address, Some(dataset)) + } + OmicronZoneType::ClickhouseKeeper { address, dataset } => { + (ZoneType::ClickhouseKeeper, address, Some(dataset)) + } + OmicronZoneType::CockroachDb { address, dataset } => { + (ZoneType::CockroachDb, address, Some(dataset)) + } + OmicronZoneType::Crucible { address, dataset } => { + (ZoneType::Crucible, address, Some(dataset)) + } + OmicronZoneType::CruciblePantry { address } => { + (ZoneType::CruciblePantry, address, None) + } + OmicronZoneType::ExternalDns { + dataset, + http_address, + dns_address, + nic, + } => { + nic_id = Some(nic.id); + let sockaddr = dns_address + .parse::() + .with_context(|| { + format!( + "parsing address for external DNS server {:?}", + dns_address + ) + })?; + second_service_ip = Some(sockaddr.ip()); + second_service_port = Some(SqlU16::from(sockaddr.port())); + (ZoneType::ExternalDns, http_address, Some(dataset)) + } + OmicronZoneType::InternalDns { + dataset, + http_address, + dns_address, + gz_address, + gz_address_index, + } => { + dns_gz_address = Some(ipv6::Ipv6Addr::from(gz_address)); + dns_gz_address_index = Some(SqlU32::from(*gz_address_index)); + let sockaddr = dns_address + .parse::() + .with_context(|| { + format!( + "parsing address for internal DNS server {:?}", + dns_address + ) + })?; + second_service_ip = Some(sockaddr.ip()); + second_service_port = Some(SqlU16::from(sockaddr.port())); + (ZoneType::InternalDns, http_address, Some(dataset)) + } + OmicronZoneType::InternalNtp { + address, + ntp_servers, + dns_servers, + domain, + } => { + ntp_ntp_servers = Some(ntp_servers.clone()); + ntp_dns_servers = Some(dns_servers.clone()); + ntp_ntp_domain = domain.clone(); + (ZoneType::InternalNtp, address, None) + } + OmicronZoneType::Nexus { + internal_address, + external_ip, + nic, + external_tls, + external_dns_servers, + } => { + nic_id = Some(nic.id); + nexus_external_tls = Some(*external_tls); + nexus_external_dns_servers = Some(external_dns_servers.clone()); + second_service_ip = Some(*external_ip); + (ZoneType::Nexus, internal_address, None) + } + OmicronZoneType::Oximeter { address } => { + (ZoneType::Oximeter, address, None) + } + }; + + let dataset_zpool_name = + dataset.map(|d| d.pool_name.as_str().to_string()); + let primary_service_sockaddr = primary_service_sockaddr_str + .parse::() + .with_context(|| { + format!( + "parsing socket address for primary IP {:?}", + primary_service_sockaddr_str + ) + })?; + let (primary_service_ip, primary_service_port) = ( + ipv6::Ipv6Addr::from(*primary_service_sockaddr.ip()), + SqlU16::from(primary_service_sockaddr.port()), + ); + + Ok(Self { + sled_id, + id, + underlay_address, + zone_type, + primary_service_ip, + primary_service_port, + second_service_ip: second_service_ip.map(IpNetwork::from), + second_service_port, + dataset_zpool_name, + nic_id, + dns_gz_address, + dns_gz_address_index, + ntp_ntp_servers, + ntp_dns_servers: ntp_dns_servers + .map(|list| list.into_iter().map(IpNetwork::from).collect()), + ntp_domain: ntp_ntp_domain, + nexus_external_tls, + nexus_external_dns_servers: nexus_external_dns_servers + .map(|list| list.into_iter().map(IpNetwork::from).collect()), + snat_ip, + snat_first_port, + snat_last_port, + }) + } + + pub(crate) fn into_omicron_zone_config( + self, + nic_row: Option, + ) -> anyhow::Result { + let address = SocketAddrV6::new( + std::net::Ipv6Addr::from(self.primary_service_ip), + *self.primary_service_port, + 0, + 0, + ) + .to_string(); + + // Assemble a value that we can use to extract the NIC _if necessary_ + // and report an error if it was needed but not found. + // + // Any error here should be impossible. By the time we get here, the + // caller should have provided `nic_row` iff there's a corresponding + // `nic_id` in this row, and the ids should match up. And whoever + // created this row ought to have provided a nic_id iff this type of + // zone needs a NIC. This last issue is not under our control, though, + // so we definitely want to handle that as an operational error. The + // others could arguably be programmer errors (i.e., we could `assert`), + // but it seems excessive to crash here. + // + // Note that we immediately return for any of the caller errors here. + // For the other error, we will return only later, if some code path + // below tries to use `nic` when it's not present. + let nic = match (self.nic_id, nic_row) { + (Some(expected_id), Some(nic_row)) => { + ensure!(expected_id == nic_row.id, "caller provided wrong NIC"); + Ok(nic_row.into_network_interface_for_zone(self.id)?) + } + (None, None) => Err(anyhow!( + "expected zone to have an associated NIC, but it doesn't" + )), + (Some(_), None) => bail!("caller provided no NIC"), + (None, Some(_)) => bail!("caller unexpectedly provided a NIC"), + }; + + // Similarly, assemble a value that we can use to extract the dataset, + // if necessary. We only return this error if code below tries to use + // this value. + let dataset = self + .dataset_zpool_name + .map(|zpool_name| -> Result<_, anyhow::Error> { + Ok(nexus_types::inventory::OmicronZoneDataset { + pool_name: zpool_name.parse().map_err(|e| { + anyhow!("parsing zpool name {:?}: {}", zpool_name, e) + })?, + }) + }) + .transpose()? + .ok_or_else(|| anyhow!("expected dataset zpool name, found none")); + + // Do the same for the DNS server address. + let dns_address = + match (self.second_service_ip, self.second_service_port) { + (Some(dns_ip), Some(dns_port)) => { + Ok(std::net::SocketAddr::new(dns_ip.ip(), *dns_port) + .to_string()) + } + _ => Err(anyhow!( + "expected second service IP and port, \ + found one missing" + )), + }; + + // Do the same for NTP zone properties. + let ntp_dns_servers = self + .ntp_dns_servers + .ok_or_else(|| anyhow!("expected list of DNS servers, found null")) + .map(|list| { + list.into_iter().map(|ipnetwork| ipnetwork.ip()).collect() + }); + let ntp_ntp_servers = + self.ntp_ntp_servers.ok_or_else(|| anyhow!("expected ntp_servers")); + + let zone_type = match self.zone_type { + ZoneType::BoundaryNtp => { + let snat_cfg = match ( + self.snat_ip, + self.snat_first_port, + self.snat_last_port, + ) { + (Some(ip), Some(first_port), Some(last_port)) => { + nexus_types::inventory::SourceNatConfig { + ip: ip.ip(), + first_port: *first_port, + last_port: *last_port, + } + } + _ => bail!( + "expected non-NULL snat properties, \ + found at least one NULL" + ), + }; + OmicronZoneType::BoundaryNtp { + address, + dns_servers: ntp_dns_servers?, + domain: self.ntp_domain, + nic: nic?, + ntp_servers: ntp_ntp_servers?, + snat_cfg, + } + } + ZoneType::Clickhouse => { + OmicronZoneType::Clickhouse { address, dataset: dataset? } + } + ZoneType::ClickhouseKeeper => { + OmicronZoneType::ClickhouseKeeper { address, dataset: dataset? } + } + ZoneType::CockroachDb => { + OmicronZoneType::CockroachDb { address, dataset: dataset? } + } + ZoneType::Crucible => { + OmicronZoneType::Crucible { address, dataset: dataset? } + } + ZoneType::CruciblePantry => { + OmicronZoneType::CruciblePantry { address } + } + ZoneType::ExternalDns => OmicronZoneType::ExternalDns { + dataset: dataset?, + dns_address: dns_address?, + http_address: address, + nic: nic?, + }, + ZoneType::InternalDns => OmicronZoneType::InternalDns { + dataset: dataset?, + dns_address: dns_address?, + http_address: address, + gz_address: *self.dns_gz_address.ok_or_else(|| { + anyhow!("expected dns_gz_address, found none") + })?, + gz_address_index: *self.dns_gz_address_index.ok_or_else( + || anyhow!("expected dns_gz_address_index, found none"), + )?, + }, + ZoneType::InternalNtp => OmicronZoneType::InternalNtp { + address, + dns_servers: ntp_dns_servers?, + domain: self.ntp_domain, + ntp_servers: ntp_ntp_servers?, + }, + ZoneType::Nexus => OmicronZoneType::Nexus { + internal_address: address, + nic: nic?, + external_tls: self + .nexus_external_tls + .ok_or_else(|| anyhow!("expected 'external_tls'"))?, + external_ip: self + .second_service_ip + .ok_or_else(|| anyhow!("expected second service IP"))? + .ip(), + external_dns_servers: self + .nexus_external_dns_servers + .ok_or_else(|| anyhow!("expected 'external_dns_servers'"))? + .into_iter() + .map(|i| i.ip()) + .collect(), + }, + ZoneType::Oximeter => OmicronZoneType::Oximeter { address }, + }; + Ok(nexus_types::inventory::OmicronZoneConfig { + id: self.id, + underlay_address: std::net::Ipv6Addr::from(self.underlay_address), + zone_type, + }) + } +} + +#[derive(Debug)] +pub(crate) struct OmicronZoneNic { + pub(crate) id: Uuid, + pub(crate) name: Name, + pub(crate) ip: IpNetwork, + pub(crate) mac: MacAddr, + pub(crate) subnet: IpNetwork, + pub(crate) vni: SqlU32, + pub(crate) is_primary: bool, + pub(crate) slot: SqlU8, +} + +impl OmicronZoneNic { + pub(crate) fn new( + zone: &nexus_types::inventory::OmicronZoneConfig, + ) -> anyhow::Result> { + match &zone.zone_type { + OmicronZoneType::ExternalDns { nic, .. } + | OmicronZoneType::BoundaryNtp { nic, .. } + | OmicronZoneType::Nexus { nic, .. } => { + // We do not bother storing the NIC's kind and associated id + // because it should be inferrable from the other information + // that we have. Verify that here. + ensure!( + matches!( + nic.kind, + nexus_types::inventory::NetworkInterfaceKind::Service( + id + ) if id == zone.id + ), + "expected zone's NIC kind to be \"service\" and the \ + id to match the zone's id ({})", + zone.id + ); + + Ok(Some(Self { + id: nic.id, + name: Name::from(nic.name.clone()), + ip: IpNetwork::from(nic.ip), + mac: MacAddr::from(nic.mac), + subnet: IpNetwork::from(nic.subnet.clone()), + vni: SqlU32::from(u32::from(nic.vni)), + is_primary: nic.primary, + slot: SqlU8::from(nic.slot), + })) + } + _ => Ok(None), + } + } + + pub(crate) fn into_network_interface_for_zone( + self, + zone_id: Uuid, + ) -> anyhow::Result { + Ok(nexus_types::inventory::NetworkInterface { + id: self.id, + ip: self.ip.ip(), + kind: nexus_types::inventory::NetworkInterfaceKind::Service( + zone_id, + ), + mac: *self.mac, + name: self.name.into(), + primary: self.is_primary, + slot: *self.slot, + vni: omicron_common::api::external::Vni::try_from(*self.vni) + .context("parsing VNI")?, + subnet: self.subnet.into(), + }) + } +} diff --git a/nexus/db-model/src/schema.rs b/nexus/db-model/src/schema.rs index eb71a12f04..9b274efc6b 100644 --- a/nexus/db-model/src/schema.rs +++ b/nexus/db-model/src/schema.rs @@ -13,7 +13,7 @@ use omicron_common::api::external::SemverVersion; /// /// This should be updated whenever the schema is changed. For more details, /// refer to: schema/crdb/README.adoc -pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(27, 0, 0); +pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(28, 0, 0); table! { disk (id) { @@ -1388,6 +1388,78 @@ table! { } } +/* blueprints */ + +table! { + blueprint (id) { + id -> Uuid, + + parent_blueprint_id -> Nullable, + + time_created -> Timestamptz, + creator -> Text, + comment -> Text, + } +} + +table! { + bp_sled_omicron_zones (blueprint_id, sled_id) { + blueprint_id -> Uuid, + sled_id -> Uuid, + + generation -> Int8, + } +} + +table! { + bp_omicron_zone (blueprint_id, id) { + blueprint_id -> Uuid, + sled_id -> Uuid, + + id -> Uuid, + underlay_address -> Inet, + zone_type -> crate::ZoneTypeEnum, + + primary_service_ip -> Inet, + primary_service_port -> Int4, + second_service_ip -> Nullable, + second_service_port -> Nullable, + dataset_zpool_name -> Nullable, + bp_nic_id -> Nullable, + dns_gz_address -> Nullable, + dns_gz_address_index -> Nullable, + ntp_ntp_servers -> Nullable>, + ntp_dns_servers -> Nullable>, + ntp_domain -> Nullable, + nexus_external_tls -> Nullable, + nexus_external_dns_servers -> Nullable>, + snat_ip -> Nullable, + snat_first_port -> Nullable, + snat_last_port -> Nullable, + } +} + +table! { + bp_omicron_zone_nic (blueprint_id, id) { + blueprint_id -> Uuid, + id -> Uuid, + name -> Text, + ip -> Inet, + mac -> Int8, + subnet -> Inet, + vni -> Int8, + is_primary -> Bool, + slot -> Int2, + } +} + +table! { + bp_omicron_zones_not_in_service (blueprint_id, bp_omicron_zone_id) { + blueprint_id -> Uuid, + bp_omicron_zone_id -> Uuid, + } +} + table! { bootstore_keys (key, generation) { key -> Text, diff --git a/nexus/types/src/deployment.rs b/nexus/types/src/deployment.rs index 95404a2c17..64f14da4a6 100644 --- a/nexus/types/src/deployment.rs +++ b/nexus/types/src/deployment.rs @@ -16,6 +16,7 @@ pub use crate::inventory::OmicronZoneConfig; pub use crate::inventory::OmicronZoneDataset; pub use crate::inventory::OmicronZoneType; pub use crate::inventory::OmicronZonesConfig; +pub use crate::inventory::SourceNatConfig; pub use crate::inventory::ZpoolName; use omicron_common::address::Ipv6Subnet; use omicron_common::address::SLED_PREFIX; diff --git a/schema/crdb/25.0.0/up1.sql b/schema/crdb/25.0.0/up1.sql new file mode 100644 index 0000000000..fda4e3ed5c --- /dev/null +++ b/schema/crdb/25.0.0/up1.sql @@ -0,0 +1,7 @@ +CREATE TABLE IF NOT EXISTS omicron.public.blueprint ( + id UUID PRIMARY KEY, + parent_blueprint_id UUID, + time_created TIMESTAMPTZ NOT NULL, + creator TEXT NOT NULL, + comment TEXT NOT NULL +); diff --git a/schema/crdb/25.0.0/up2.sql b/schema/crdb/25.0.0/up2.sql new file mode 100644 index 0000000000..a51c1a31fa --- /dev/null +++ b/schema/crdb/25.0.0/up2.sql @@ -0,0 +1,6 @@ +CREATE TABLE IF NOT EXISTS omicron.public.bp_sled_omicron_zones ( + blueprint_id UUID NOT NULL, + sled_id UUID NOT NULL, + generation INT8 NOT NULL, + PRIMARY KEY (blueprint_id, sled_id) +); diff --git a/schema/crdb/25.0.0/up3.sql b/schema/crdb/25.0.0/up3.sql new file mode 100644 index 0000000000..55e09ca719 --- /dev/null +++ b/schema/crdb/25.0.0/up3.sql @@ -0,0 +1,31 @@ +CREATE TABLE IF NOT EXISTS omicron.public.bp_omicron_zone ( + blueprint_id UUID NOT NULL, + sled_id UUID NOT NULL, + id UUID NOT NULL, + underlay_address INET NOT NULL, + zone_type omicron.public.zone_type NOT NULL, + primary_service_ip INET NOT NULL, + primary_service_port INT4 + CHECK (primary_service_port BETWEEN 0 AND 65535) + NOT NULL, + second_service_ip INET, + second_service_port INT4 + CHECK (second_service_port IS NULL + OR second_service_port BETWEEN 0 AND 65535), + dataset_zpool_name TEXT, + bp_nic_id UUID, + dns_gz_address INET, + dns_gz_address_index INT8, + ntp_ntp_servers TEXT[], + ntp_dns_servers INET[], + ntp_domain TEXT, + nexus_external_tls BOOLEAN, + nexus_external_dns_servers INET ARRAY, + snat_ip INET, + snat_first_port INT4 + CHECK (snat_first_port IS NULL OR snat_first_port BETWEEN 0 AND 65535), + snat_last_port INT4 + CHECK (snat_last_port IS NULL OR snat_last_port BETWEEN 0 AND 65535), + + PRIMARY KEY (blueprint_id, id) +); diff --git a/schema/crdb/25.0.0/up4.sql b/schema/crdb/25.0.0/up4.sql new file mode 100644 index 0000000000..beff4da802 --- /dev/null +++ b/schema/crdb/25.0.0/up4.sql @@ -0,0 +1,13 @@ +CREATE TABLE IF NOT EXISTS omicron.public.bp_omicron_zone_nic ( + blueprint_id UUID NOT NULL, + id UUID NOT NULL, + name TEXT NOT NULL, + ip INET NOT NULL, + mac INT8 NOT NULL, + subnet INET NOT NULL, + vni INT8 NOT NULL, + is_primary BOOLEAN NOT NULL, + slot INT2 NOT NULL, + + PRIMARY KEY (blueprint_id, id) +); diff --git a/schema/crdb/25.0.0/up5.sql b/schema/crdb/25.0.0/up5.sql new file mode 100644 index 0000000000..72c34400a3 --- /dev/null +++ b/schema/crdb/25.0.0/up5.sql @@ -0,0 +1,6 @@ +CREATE TABLE IF NOT EXISTS omicron.public.bp_omicron_zones_not_in_service ( + blueprint_id UUID NOT NULL, + bp_omicron_zone_id UUID NOT NULL, + + PRIMARY KEY (blueprint_id, bp_omicron_zone_id) +); diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index c91bb669a9..fc68799048 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -2954,8 +2954,8 @@ CREATE TABLE IF NOT EXISTS omicron.public.inv_omicron_zone ( -- service in them) primary_service_ip INET NOT NULL, primary_service_port INT4 - CHECK (primary_service_port BETWEEN 0 AND 65535) - NOT NULL, + CHECK (primary_service_port BETWEEN 0 AND 65535) + NOT NULL, -- The remaining properties may be NULL for different kinds of zones. The -- specific constraints are not enforced at the database layer, basically @@ -2967,7 +2967,7 @@ CREATE TABLE IF NOT EXISTS omicron.public.inv_omicron_zone ( second_service_ip INET, second_service_port INT4 CHECK (second_service_port IS NULL - OR second_service_port BETWEEN 0 AND 65535), + OR second_service_port BETWEEN 0 AND 65535), -- Zones may have an associated dataset. They're currently always on a U.2. -- The only thing we need to identify it here is the name of the zpool that @@ -2995,9 +2995,9 @@ CREATE TABLE IF NOT EXISTS omicron.public.inv_omicron_zone ( -- Source NAT configuration (currently used for boundary NTP only) snat_ip INET, snat_first_port INT4 - CHECK (snat_first_port IS NULL OR snat_first_port BETWEEN 0 AND 65535), + CHECK (snat_first_port IS NULL OR snat_first_port BETWEEN 0 AND 65535), snat_last_port INT4 - CHECK (snat_last_port IS NULL OR snat_last_port BETWEEN 0 AND 65535), + CHECK (snat_last_port IS NULL OR snat_last_port BETWEEN 0 AND 65535), PRIMARY KEY (inv_collection_id, id) ); @@ -3016,6 +3016,149 @@ CREATE TABLE IF NOT EXISTS omicron.public.inv_omicron_zone_nic ( PRIMARY KEY (inv_collection_id, id) ); +/* + * System policy and blueprints + * + * TODO docs + */ + +-- list of all blueprints +CREATE TABLE IF NOT EXISTS omicron.public.blueprint ( + id UUID PRIMARY KEY, + + -- This is effectively a foreign key back to this table; however, it is + -- allowed to be NULL: the initial blueprint has no parent. Additionally, + -- it may be non-NULL but no longer reference a row in this table: once a + -- child blueprint has been created from a parent, it's possible for the + -- parent to be deleted. An open question is whether we should NULL out this + -- field on such a deletion; currently we do not, so we can always see that + -- there had been a particular parent even if it's now gone. + parent_blueprint_id UUID, + + -- These fields are for debugging only. + time_created TIMESTAMPTZ NOT NULL, + creator TEXT NOT NULL, + comment TEXT NOT NULL +); + +-- generation number for the OmicronZonesConfig for each sled in a blueprint +CREATE TABLE IF NOT EXISTS omicron.public.bp_sled_omicron_zones ( + -- foreign key into `blueprint` table + blueprint_id UUID NOT NULL, + + -- unique id for this sled (should be foreign keys into `sled` table, though + -- it's conceivable a blueprint could refer to a sled that no longer exists, + -- particularly if the blueprint is older than the current target) + sled_id UUID NOT NULL, + + -- OmicronZonesConfig generation for the zones on this sled in this + -- blueprint + generation INT8 NOT NULL, + + PRIMARY KEY (blueprint_id, sled_id) +); + +-- description of omicron zones specified in a blueprint +-- +-- this is currently identical to `inv_omicron_zone`, except that the foreign +-- keys reference other blueprint tables intead of inventory tables. we expect +-- their sameness to diverge over time as either inventory or blueprints (or +-- both) grow context-specific properties +CREATE TABLE IF NOT EXISTS omicron.public.bp_omicron_zone ( + -- foreign key into the `blueprint` table + blueprint_id UUID NOT NULL, + + -- unique id for this sled (should be foreign keys into `sled` table, though + -- it's conceivable a blueprint could refer to a sled that no longer exists, + -- particularly if the blueprint is older than the current target) + sled_id UUID NOT NULL, + + -- unique id for this zone + id UUID NOT NULL, + underlay_address INET NOT NULL, + zone_type omicron.public.zone_type NOT NULL, + + -- SocketAddr of the "primary" service for this zone + -- (what this describes varies by zone type, but all zones have at least one + -- service in them) + primary_service_ip INET NOT NULL, + primary_service_port INT4 + CHECK (primary_service_port BETWEEN 0 AND 65535) + NOT NULL, + + -- The remaining properties may be NULL for different kinds of zones. The + -- specific constraints are not enforced at the database layer, basically + -- because it's really complicated to do that and it's not obvious that it's + -- worthwhile. + + -- Some zones have a second service. Like the primary one, the meaning of + -- this is zone-type-dependent. + second_service_ip INET, + second_service_port INT4 + CHECK (second_service_port IS NULL + OR second_service_port BETWEEN 0 AND 65535), + + -- Zones may have an associated dataset. They're currently always on a U.2. + -- The only thing we need to identify it here is the name of the zpool that + -- it's on. + dataset_zpool_name TEXT, + + -- Zones with external IPs have an associated NIC and sockaddr for listening + -- (first is a foreign key into `bp_omicron_zone_nic`) + bp_nic_id UUID, + + -- Properties for internal DNS servers + -- address attached to this zone from outside the sled's subnet + dns_gz_address INET, + dns_gz_address_index INT8, + + -- Properties common to both kinds of NTP zones + ntp_ntp_servers TEXT[], + ntp_dns_servers INET[], + ntp_domain TEXT, + + -- Properties specific to Nexus zones + nexus_external_tls BOOLEAN, + nexus_external_dns_servers INET ARRAY, + + -- Source NAT configuration (currently used for boundary NTP only) + snat_ip INET, + snat_first_port INT4 + CHECK (snat_first_port IS NULL OR snat_first_port BETWEEN 0 AND 65535), + snat_last_port INT4 + CHECK (snat_last_port IS NULL OR snat_last_port BETWEEN 0 AND 65535), + + PRIMARY KEY (blueprint_id, id) +); + +CREATE TABLE IF NOT EXISTS omicron.public.bp_omicron_zone_nic ( + blueprint_id UUID NOT NULL, + id UUID NOT NULL, + name TEXT NOT NULL, + ip INET NOT NULL, + mac INT8 NOT NULL, + subnet INET NOT NULL, + vni INT8 NOT NULL, + is_primary BOOLEAN NOT NULL, + slot INT2 NOT NULL, + + PRIMARY KEY (blueprint_id, id) +); + +-- list of omicron zones that are considered NOT in-service for a blueprint +-- +-- In Rust code, we generally want to deal with "zones in service", which means +-- they should appear in DNS. However, almost all zones in almost all blueprints +-- will be in service, so we can induce considerably less database work by +-- storing the zones _not_ in service. Our DB wrapper layer handles this +-- inversion, so the rest of our Rust code can ignore it. +CREATE TABLE IF NOT EXISTS omicron.public.bp_omicron_zones_not_in_service ( + blueprint_id UUID NOT NULL, + bp_omicron_zone_id UUID NOT NULL, + + PRIMARY KEY (blueprint_id, bp_omicron_zone_id) +); + /*******************************************************************/ /* @@ -3196,7 +3339,7 @@ INSERT INTO omicron.public.db_metadata ( version, target_version ) VALUES - ( TRUE, NOW(), NOW(), '27.0.0', NULL) + ( TRUE, NOW(), NOW(), '28.0.0', NULL) ON CONFLICT DO NOTHING; COMMIT; From 07e2cef9c1734ca115c54bda0b373e17300a5610 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Mon, 22 Jan 2024 12:40:26 -0500 Subject: [PATCH 02/24] add queries to read/write/delete blueprints from the db --- Cargo.lock | 2 + nexus/db-model/src/deployment.rs | 32 +- nexus/db-model/src/lib.rs | 1 + nexus/db-queries/Cargo.toml | 2 + nexus/db-queries/src/authz/api_resources.rs | 55 ++ nexus/db-queries/src/authz/omicron.polar | 11 + nexus/db-queries/src/authz/oso_generic.rs | 1 + .../src/authz/policy_test/resource_builder.rs | 1 + .../src/authz/policy_test/resources.rs | 3 +- .../db-queries/src/db/datastore/deployment.rs | 834 ++++++++++++++++++ nexus/db-queries/src/db/datastore/mod.rs | 1 + nexus/deployment/Cargo.toml | 2 +- nexus/deployment/src/blueprint_builder.rs | 9 +- nexus/inventory/src/builder.rs | 26 +- nexus/inventory/src/lib.rs | 2 + 15 files changed, 960 insertions(+), 22 deletions(-) create mode 100644 nexus/db-queries/src/db/datastore/deployment.rs diff --git a/Cargo.lock b/Cargo.lock index 7ea3d2b96d..7d3ebe428f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4237,12 +4237,14 @@ dependencies = [ "http 0.2.11", "hyper 0.14.27", "hyper-rustls 0.26.0", + "illumos-utils", "internal-dns", "ipnetwork", "itertools 0.12.0", "macaddr", "newtype_derive", "nexus-db-model", + "nexus-deployment", "nexus-inventory", "nexus-test-utils", "nexus-types", diff --git a/nexus/db-model/src/deployment.rs b/nexus/db-model/src/deployment.rs index 3100306ed9..fdf0cdacd1 100644 --- a/nexus/db-model/src/deployment.rs +++ b/nexus/db-model/src/deployment.rs @@ -6,7 +6,7 @@ //! database use crate::inventory::ZoneType; -use crate::omicron_zone_config::{OmicronZoneNic, OmicronZone}; +use crate::omicron_zone_config::{OmicronZone, OmicronZoneNic}; use crate::schema::{ blueprint, bp_omicron_zone, bp_omicron_zone_nic, bp_omicron_zones_not_in_service, bp_sled_omicron_zones, @@ -27,6 +27,18 @@ pub struct Blueprint { pub comment: String, } +impl From<&'_ nexus_types::deployment::Blueprint> for Blueprint { + fn from(bp: &'_ nexus_types::deployment::Blueprint) -> Self { + Self { + id: bp.id, + parent_blueprint_id: bp.parent_blueprint_id, + time_created: bp.time_created, + creator: bp.creator.clone(), + comment: bp.comment.clone(), + } + } +} + /// See [`nexus_types::deployment::OmicronZonesConfig`]. #[derive(Queryable, Clone, Debug, Selectable, Insertable)] #[diesel(table_name = bp_sled_omicron_zones)] @@ -36,6 +48,20 @@ pub struct BpSledOmicronZones { pub generation: Generation, } +impl BpSledOmicronZones { + pub fn new( + blueprint_id: Uuid, + sled_id: Uuid, + zones_config: &nexus_types::deployment::OmicronZonesConfig, + ) -> Self { + Self { + blueprint_id, + sled_id, + generation: Generation(zones_config.generation), + } + } +} + /// See [`nexus_types::deployment::OmicronZoneConfig`]. #[derive(Queryable, Clone, Debug, Selectable, Insertable)] #[diesel(table_name = bp_omicron_zone)] @@ -188,6 +214,6 @@ impl BpOmicronZoneNic { #[derive(Queryable, Clone, Debug, Selectable, Insertable)] #[diesel(table_name = bp_omicron_zones_not_in_service)] pub struct BpOmicronZoneNotInService { - blueprint_id: Uuid, - bp_omicron_zone_id: Uuid, + pub blueprint_id: Uuid, + pub bp_omicron_zone_id: Uuid, } diff --git a/nexus/db-model/src/lib.rs b/nexus/db-model/src/lib.rs index 48f9f66229..7fa95822a7 100644 --- a/nexus/db-model/src/lib.rs +++ b/nexus/db-model/src/lib.rs @@ -116,6 +116,7 @@ pub use console_session::*; pub use dataset::*; pub use dataset_kind::*; pub use db_metadata::*; +pub use deployment::*; pub use device_auth::*; pub use digest::*; pub use disk::*; diff --git a/nexus/db-queries/Cargo.toml b/nexus/db-queries/Cargo.toml index 3240c54f3f..9cdcc88e6a 100644 --- a/nexus/db-queries/Cargo.toml +++ b/nexus/db-queries/Cargo.toml @@ -64,8 +64,10 @@ camino-tempfile.workspace = true expectorate.workspace = true hyper-rustls.workspace = true gateway-client.workspace = true +illumos-utils.workspace = true internal-dns.workspace = true itertools.workspace = true +nexus-deployment.workspace = true nexus-inventory.workspace = true nexus-test-utils.workspace = true omicron-sled-agent.workspace = true diff --git a/nexus/db-queries/src/authz/api_resources.rs b/nexus/db-queries/src/authz/api_resources.rs index b4fd4e1890..d8460486ef 100644 --- a/nexus/db-queries/src/authz/api_resources.rs +++ b/nexus/db-queries/src/authz/api_resources.rs @@ -577,6 +577,61 @@ impl AuthorizedResource for Inventory { } } +/// Synthetic resource used for modeling access to deployment configuration +/// data (e.g., blueprints and policy) +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub struct DeploymentConfig; +pub const DEPLOYMENT_CONFIG: DeploymentConfig = DeploymentConfig {}; + +impl oso::PolarClass for DeploymentConfig { + fn get_polar_class_builder() -> oso::ClassBuilder { + // Roles are not directly attached to DeploymentConfig + oso::Class::builder() + .with_equality_check() + .add_method( + "has_role", + |_: &DeploymentConfig, + _actor: AuthenticatedActor, + _role: String| { false }, + ) + .add_attribute_getter("fleet", |_| FLEET) + } +} + +impl AuthorizedResource for DeploymentConfig { + fn load_roles<'a, 'b, 'c, 'd, 'e, 'f>( + &'a self, + opctx: &'b OpContext, + datastore: &'c DataStore, + authn: &'d authn::Context, + roleset: &'e mut RoleSet, + ) -> futures::future::BoxFuture<'f, Result<(), Error>> + where + 'a: 'f, + 'b: 'f, + 'c: 'f, + 'd: 'f, + 'e: 'f, + { + load_roles_for_resource_tree(&FLEET, opctx, datastore, authn, roleset) + .boxed() + } + + fn on_unauthorized( + &self, + _: &Authz, + error: Error, + _: AnyActor, + _: Action, + ) -> Error { + error + } + + fn polar_class(&self) -> oso::Class { + Self::get_polar_class() + } +} + /// Synthetic resource describing the list of Certificates associated with a /// Silo #[derive(Clone, Debug, Eq, PartialEq)] diff --git a/nexus/db-queries/src/authz/omicron.polar b/nexus/db-queries/src/authz/omicron.polar index f9382401fd..cef6874673 100644 --- a/nexus/db-queries/src/authz/omicron.polar +++ b/nexus/db-queries/src/authz/omicron.polar @@ -393,6 +393,17 @@ resource Inventory { has_relation(fleet: Fleet, "parent_fleet", inventory: Inventory) if inventory.fleet = fleet; +# Describes the policy for reading and modifying deployment configuration and +# policy +resource DeploymentConfig { + permissions = [ "read", "modify" ]; + relations = { parent_fleet: Fleet }; + "read" if "viewer" on "parent_fleet"; + "modify" if "admin" on "parent_fleet"; +} +has_relation(fleet: Fleet, "parent_fleet", deployment_config: DeploymentConfig) + if deployment_config.fleet = fleet; + # Describes the policy for accessing "/v1/system/ip-pools" in the API resource IpPoolList { permissions = [ diff --git a/nexus/db-queries/src/authz/oso_generic.rs b/nexus/db-queries/src/authz/oso_generic.rs index dd646a1c98..b6b9c59afe 100644 --- a/nexus/db-queries/src/authz/oso_generic.rs +++ b/nexus/db-queries/src/authz/oso_generic.rs @@ -105,6 +105,7 @@ pub fn make_omicron_oso(log: &slog::Logger) -> Result { AuthenticatedActor::get_polar_class(), BlueprintConfig::get_polar_class(), Database::get_polar_class(), + DeploymentConfig::get_polar_class(), DnsConfig::get_polar_class(), Fleet::get_polar_class(), Inventory::get_polar_class(), diff --git a/nexus/db-queries/src/authz/policy_test/resource_builder.rs b/nexus/db-queries/src/authz/policy_test/resource_builder.rs index dc18b2e47f..c86407d438 100644 --- a/nexus/db-queries/src/authz/policy_test/resource_builder.rs +++ b/nexus/db-queries/src/authz/policy_test/resource_builder.rs @@ -245,6 +245,7 @@ macro_rules! impl_dyn_authorized_resource_for_global { impl_dyn_authorized_resource_for_global!(authz::oso_generic::Database); impl_dyn_authorized_resource_for_global!(authz::BlueprintConfig); impl_dyn_authorized_resource_for_global!(authz::ConsoleSessionList); +impl_dyn_authorized_resource_for_global!(authz::DeploymentConfig); impl_dyn_authorized_resource_for_global!(authz::DeviceAuthRequestList); impl_dyn_authorized_resource_for_global!(authz::DnsConfig); impl_dyn_authorized_resource_for_global!(authz::IpPoolList); diff --git a/nexus/db-queries/src/authz/policy_test/resources.rs b/nexus/db-queries/src/authz/policy_test/resources.rs index 3e87f6db51..83a48e1bd2 100644 --- a/nexus/db-queries/src/authz/policy_test/resources.rs +++ b/nexus/db-queries/src/authz/policy_test/resources.rs @@ -68,8 +68,9 @@ pub async fn make_resources( builder.new_resource_with_users(authz::FLEET).await; builder.new_resource(authz::BLUEPRINT_CONFIG); builder.new_resource(authz::CONSOLE_SESSION_LIST); - builder.new_resource(authz::DNS_CONFIG); + builder.new_resource(authz::DEPLOYMENT_CONFIG); builder.new_resource(authz::DEVICE_AUTH_REQUEST_LIST); + builder.new_resource(authz::DNS_CONFIG); builder.new_resource(authz::INVENTORY); builder.new_resource(authz::IP_POOL_LIST); diff --git a/nexus/db-queries/src/db/datastore/deployment.rs b/nexus/db-queries/src/db/datastore/deployment.rs new file mode 100644 index 0000000000..059e6aa093 --- /dev/null +++ b/nexus/db-queries/src/db/datastore/deployment.rs @@ -0,0 +1,834 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +use super::DataStore; +use crate::authz; +use crate::context::OpContext; +use crate::db; +use crate::db::error::public_error_from_diesel; +use crate::db::error::ErrorHandler; +use crate::db::pagination::paginated; +use crate::db::pagination::Paginator; +use crate::db::TransactionError; +use anyhow::Context; +use async_bb8_diesel::AsyncConnection; +use async_bb8_diesel::AsyncRunQueryDsl; +use diesel::expression::SelectableHelper; +use diesel::ExpressionMethods; +use diesel::QueryDsl; +use nexus_db_model::Blueprint as DbBlueprint; +use nexus_db_model::BpOmicronZone; +use nexus_db_model::BpOmicronZoneNic; +use nexus_db_model::BpOmicronZoneNotInService; +use nexus_db_model::BpSledOmicronZones; +use nexus_types::deployment::Blueprint; +use nexus_types::deployment::OmicronZonesConfig; +use omicron_common::api::external::Error; +use omicron_common::bail_unless; +use std::collections::BTreeMap; +use std::collections::BTreeSet; +use std::num::NonZeroU32; +use uuid::Uuid; + +/// "limit" used in SQL queries that paginate through all sleds, omicron +/// zones, etc. +/// +/// While we always load an entire blueprint in one operation, we use a +/// [`Paginator`] to guard against single queries returning an unchecked number +/// of rows. +// unsafe: `new_unchecked` is only unsound if the argument is 0. +// TODO XXX set to 1000; currently 1 for testing +const SQL_BATCH_SIZE: NonZeroU32 = unsafe { NonZeroU32::new_unchecked(1) }; + +impl DataStore { + /// Store a complete blueprint into the database + pub async fn blueprint_insert( + &self, + opctx: &OpContext, + blueprint: &Blueprint, + ) -> Result<(), Error> { + opctx + .authorize(authz::Action::Modify, &authz::DEPLOYMENT_CONFIG) + .await?; + + // In the database, the blueprint is represented essentially as a tree + // rooted at an `blueprint` row. Other nodes in the tree point + // back at the `blueprint` via `blueprint_id`. + // + // It's helpful to assemble some values before entering the transaction + // so that we can produce the `Error` type that we want here. + let row_blueprint = DbBlueprint::from(blueprint); + let blueprint_id = row_blueprint.id; + let sled_omicron_zones = blueprint + .omicron_zones + .iter() + .map(|(sled_id, zones_config)| { + BpSledOmicronZones::new(blueprint_id, *sled_id, zones_config) + }) + .collect::>(); + let omicron_zones = blueprint + .omicron_zones + .iter() + .flat_map(|(sled_id, zones_config)| { + zones_config.zones.iter().map(|zone| { + BpOmicronZone::new(blueprint_id, *sled_id, zone) + .map_err(|e| Error::internal_error(&format!("{:#}", e))) + }) + }) + .collect::, Error>>()?; + let omicron_zone_nics = blueprint + .omicron_zones + .values() + .flat_map(|zones_config| { + zones_config.zones.iter().filter_map(|zone| { + BpOmicronZoneNic::new(blueprint_id, zone) + .with_context(|| format!("zone {:?}", zone.id)) + .map_err(|e| Error::internal_error(&format!("{:#}", e))) + .transpose() + }) + }) + .collect::, _>>()?; + + // `Blueprint` stores a set of zones in service, but in the database we + // store the set of zones NOT in service (which we expect to be much + // smaller, often empty). Build that inverted set here. + let omicron_zones_not_in_service = { + let mut zones_not_in_service = Vec::new(); + for zone in &omicron_zones { + if !blueprint.zones_in_service.contains(&zone.id) { + zones_not_in_service.push(BpOmicronZoneNotInService { + blueprint_id, + bp_omicron_zone_id: zone.id, + }); + } + } + zones_not_in_service + }; + + // This implementation inserts all records associated with the + // blueprint in one transaction. This is required: we don't want + // any planner or executor to see a half-inserted blueprint, nor do we + // want to leave a partial blueprint around if we crash. However, it + // does mean this is likely to be a big transaction and if that becomes + // a problem we could break this up as long as we address those + // problems. + // + // The SQL here is written so that it doesn't have to be an + // *interactive* transaction. That is, it should in principle be + // possible to generate all this SQL up front and send it as one big + // batch rather than making a bunch of round-trips to the database. + // We'd do that if we had an interface for doing that with bound + // parameters, etc. See oxidecomputer/omicron#973. + let pool = self.pool_connection_authorized(opctx).await?; + pool.transaction_async(|conn| async move { + // Insert the row for the blueprint. + { + use db::schema::blueprint::dsl; + let _: usize = diesel::insert_into(dsl::blueprint) + .values(row_blueprint) + .execute_async(&conn) + .await?; + } + + // Insert all the Omicron zones for this blueprint. + { + use db::schema::bp_sled_omicron_zones::dsl as sled_zones; + let _ = diesel::insert_into(sled_zones::bp_sled_omicron_zones) + .values(sled_omicron_zones) + .execute_async(&conn) + .await?; + } + + { + use db::schema::bp_omicron_zone::dsl as omicron_zone; + let _ = diesel::insert_into(omicron_zone::bp_omicron_zone) + .values(omicron_zones) + .execute_async(&conn) + .await?; + } + + { + use db::schema::bp_omicron_zone_nic::dsl as omicron_zone_nic; + let _ = + diesel::insert_into(omicron_zone_nic::bp_omicron_zone_nic) + .values(omicron_zone_nics) + .execute_async(&conn) + .await?; + } + + { + use db::schema::bp_omicron_zones_not_in_service::dsl; + let _ = + diesel::insert_into(dsl::bp_omicron_zones_not_in_service) + .values(omicron_zones_not_in_service) + .execute_async(&conn) + .await?; + } + + Ok(()) + }) + .await + .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; + + info!( + &opctx.log, + "inserted blueprint"; + "blueprint_id" => %blueprint.id, + ); + + Ok(()) + } + + /// Read a complete blueprint from the database + pub async fn blueprint_read( + &self, + opctx: &OpContext, + blueprint_id: Uuid, + ) -> Result { + opctx.authorize(authz::Action::Read, &authz::DEPLOYMENT_CONFIG).await?; + let conn = self.pool_connection_authorized(opctx).await?; + + // Read the metadata from the primary blueprint row, and ensure that it + // exists. + let (parent_blueprint_id, time_created, creator, comment) = { + use db::schema::blueprint::dsl; + + let blueprints = dsl::blueprint + .filter(dsl::id.eq(blueprint_id)) + .limit(2) + .select(DbBlueprint::as_select()) + .load_async(&*conn) + .await + .map_err(|e| { + public_error_from_diesel(e, ErrorHandler::Server) + })?; + bail_unless!(blueprints.len() == 1); + let blueprint = blueprints.into_iter().next().unwrap(); + ( + blueprint.parent_blueprint_id, + blueprint.time_created, + blueprint.creator, + blueprint.comment, + ) + }; + + // Read this blueprint's `bp_sled_omicron_zones` rows, which describes + // the `OmicronZonesConfig` generation number for each sled that is a + // part of this blueprint. Construct the BTreeMap we ultimately need, + // but all the `zones` vecs will be empty until our next query below. + let mut omicron_zones: BTreeMap = { + use db::schema::bp_sled_omicron_zones::dsl; + + let mut omicron_zones = BTreeMap::new(); + let mut paginator = Paginator::new(SQL_BATCH_SIZE); + while let Some(p) = paginator.next() { + let batch = paginated( + dsl::bp_sled_omicron_zones, + dsl::sled_id, + &p.current_pagparams(), + ) + .filter(dsl::blueprint_id.eq(blueprint_id)) + .select(BpSledOmicronZones::as_select()) + .load_async(&*conn) + .await + .map_err(|e| { + public_error_from_diesel(e, ErrorHandler::Server) + })?; + + paginator = p.found_batch(&batch, &|s| s.sled_id); + + for s in batch { + let old = omicron_zones.insert( + s.sled_id, + OmicronZonesConfig { + generation: *s.generation, + zones: Vec::new(), + }, + ); + bail_unless!( + old.is_none(), + "found duplicate sled ID in bp_sled_omicron_zones: {}", + s.sled_id + ); + } + } + + omicron_zones + }; + + // Assemble a mutable map of all the NICs found, by NIC id. As we + // match these up with the corresponding zone below, we'll remove items + // from this set. That way we can tell if the same NIC was used twice + // or not used at all. + let mut omicron_zone_nics = { + use db::schema::bp_omicron_zone_nic::dsl; + + let mut omicron_zone_nics = BTreeMap::new(); + let mut paginator = Paginator::new(SQL_BATCH_SIZE); + while let Some(p) = paginator.next() { + let batch = paginated( + dsl::bp_omicron_zone_nic, + dsl::id, + &p.current_pagparams(), + ) + .filter(dsl::blueprint_id.eq(blueprint_id)) + .select(BpOmicronZoneNic::as_select()) + .load_async(&*conn) + .await + .map_err(|e| { + public_error_from_diesel(e, ErrorHandler::Server) + })?; + + paginator = p.found_batch(&batch, &|n| n.id); + + for n in batch { + let nic_id = n.id; + let old = omicron_zone_nics.insert(nic_id, n); + bail_unless!( + old.is_none(), + "found duplicate NIC ID in bp_omicron_zone_nic: {}", + nic_id, + ); + } + } + + omicron_zone_nics + }; + + // Load the list of not-in-service zones. Similar to NICs, we'll use a + // mutable set of zone IDs so we can tell if a zone we expected to be + // inactive wasn't present in the blueprint at all. + let mut omicron_zones_not_in_service = { + use db::schema::bp_omicron_zones_not_in_service::dsl; + + let mut omicron_zones_not_in_service = BTreeSet::new(); + let mut paginator = Paginator::new(SQL_BATCH_SIZE); + while let Some(p) = paginator.next() { + let batch = paginated( + dsl::bp_omicron_zones_not_in_service, + dsl::bp_omicron_zone_id, + &p.current_pagparams(), + ) + .filter(dsl::blueprint_id.eq(blueprint_id)) + .select(BpOmicronZoneNotInService::as_select()) + .load_async(&*conn) + .await + .map_err(|e| { + public_error_from_diesel(e, ErrorHandler::Server) + })?; + + paginator = p.found_batch(&batch, &|z| z.bp_omicron_zone_id); + + for z in batch { + let inserted = omicron_zones_not_in_service + .insert(z.bp_omicron_zone_id); + bail_unless!( + inserted, + "found duplicate zone ID in \ + bp_omicron_zones_not_in_service: {}", + z.bp_omicron_zone_id, + ); + } + } + + omicron_zones_not_in_service + }; + + // Create the in-memory list of zones _in_ service, which we'll + // calculate below as we load zones. (Any zone that isn't present in + // `omicron_zones_not_in_service` is considered in service.) + let mut zones_in_service = BTreeSet::new(); + + // Load all the zones for each sled. + { + use db::schema::bp_omicron_zone::dsl; + + let mut paginator = Paginator::new(SQL_BATCH_SIZE); + while let Some(p) = paginator.next() { + let batch = paginated( + dsl::bp_omicron_zone, + dsl::id, + &p.current_pagparams(), + ) + .filter(dsl::blueprint_id.eq(blueprint_id)) + // It's not strictly necessary to order these by id. Doing so + // ensures a consistent representation for `Blueprint`, which + // makes testing easier. It's already indexed to do this, too. + .order_by(dsl::id) + .select(BpOmicronZone::as_select()) + .load_async(&*conn) + .await + .map_err(|e| { + public_error_from_diesel(e, ErrorHandler::Server) + })?; + + paginator = p.found_batch(&batch, &|z| z.id); + + for z in batch { + let nic_row = z + .bp_nic_id + .map(|id| { + // This error means that we found a row in + // bp_omicron_zone that references a NIC by id but + // there's no corresponding row in + // bp_omicron_zone_nic with that id. This should be + // impossible and reflects either a bug or database + // corruption. + omicron_zone_nics.remove(&id).ok_or_else(|| { + Error::internal_error(&format!( + "zone {:?}: expected to find NIC {:?}, \ + but didn't", + z.id, z.bp_nic_id + )) + }) + }) + .transpose()?; + let sled_zones = + omicron_zones.get_mut(&z.sled_id).ok_or_else(|| { + // This error means that we found a row in + // bp_omicron_zone with no associated record in + // bp_sled_omicron_zones. This should be + // impossible and reflects either a bug or database + // corruption. + Error::internal_error(&format!( + "zone {:?}: unknown sled: {:?}", + z.id, z.sled_id + )) + })?; + let zone_id = z.id; + let zone = z + .into_omicron_zone_config(nic_row) + .with_context(|| { + format!("zone {:?}: parse from database", zone_id) + }) + .map_err(|e| { + Error::internal_error(&format!( + "{:#}", + e.to_string() + )) + })?; + sled_zones.zones.push(zone); + + // If we can remove `zone_id` from + // `omicron_zones_not_in_service`, then the zone is not in + // service. Otherwise, add it to the list of in-service + // zones. + if !omicron_zones_not_in_service.remove(&zone_id) { + zones_in_service.insert(zone_id); + } + } + } + } + + bail_unless!( + omicron_zone_nics.is_empty(), + "found extra Omicron zone NICs: {:?}", + omicron_zone_nics.keys() + ); + bail_unless!( + omicron_zones_not_in_service.is_empty(), + "found extra Omicron zones not in service: {:?}", + omicron_zones_not_in_service, + ); + + Ok(Blueprint { + id: blueprint_id, + omicron_zones, + zones_in_service, + parent_blueprint_id, + time_created, + creator, + comment, + }) + } + + /// Delete a blueprint from the database + pub async fn blueprint_delete( + &self, + opctx: &OpContext, + blueprint_id: Uuid, + ) -> Result<(), Error> { + opctx.authorize(authz::Action::Modify, &authz::INVENTORY).await?; + + // As with inserting a whole blueprint, we remove it in one big + // transaction. Similar considerations apply. We could + // break it up if these transactions become too big. But we'd need a + // way to stop other clients from discovering a collection after we + // start removing it and we'd also need to make sure we didn't leak a + // collection if we crash while deleting it. + let conn = self.pool_connection_authorized(opctx).await?; + + // TODO XXX check that this blueprint is not the target + + let ( + nblueprints, + nsled_agent_zones, + nzones, + nnics, + nzones_not_in_service, + ) = conn + .transaction_async(|conn| async move { + // Remove the record describing the blueprint itself. + let nblueprints = { + use db::schema::blueprint::dsl; + diesel::delete( + dsl::blueprint.filter(dsl::id.eq(blueprint_id)), + ) + .execute_async(&conn) + .await? + }; + + // Remove rows associated with Omicron zones + let nsled_agent_zones = { + use db::schema::bp_sled_omicron_zones::dsl; + diesel::delete( + dsl::bp_sled_omicron_zones + .filter(dsl::blueprint_id.eq(blueprint_id)), + ) + .execute_async(&conn) + .await? + }; + + let nzones = { + use db::schema::bp_omicron_zone::dsl; + diesel::delete( + dsl::bp_omicron_zone + .filter(dsl::blueprint_id.eq(blueprint_id)), + ) + .execute_async(&conn) + .await? + }; + + let nnics = { + use db::schema::bp_omicron_zone_nic::dsl; + diesel::delete( + dsl::bp_omicron_zone_nic + .filter(dsl::blueprint_id.eq(blueprint_id)), + ) + .execute_async(&conn) + .await? + }; + + let nzones_not_in_service = { + use db::schema::bp_omicron_zones_not_in_service::dsl; + diesel::delete( + dsl::bp_omicron_zones_not_in_service + .filter(dsl::blueprint_id.eq(blueprint_id)), + ) + .execute_async(&conn) + .await? + }; + + Ok(( + nblueprints, + nsled_agent_zones, + nzones, + nnics, + nzones_not_in_service, + )) + }) + .await + .map_err(|error| match error { + TransactionError::CustomError(e) => e, + TransactionError::Database(e) => { + public_error_from_diesel(e, ErrorHandler::Server) + } + })?; + + info!(&opctx.log, "removed blueprint"; + "blueprint_id" => blueprint_id.to_string(), + "nblueprints" => nblueprints, + "nsled_agent_zones" => nsled_agent_zones, + "nzones" => nzones, + "nnics" => nnics, + "nzones_not_in_service" => nzones_not_in_service, + ); + + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::db::datastore::datastore_test; + use nexus_deployment::blueprint_builder::BlueprintBuilder; + use nexus_deployment::blueprint_builder::Ensure; + use nexus_test_utils::db::test_setup_database; + use nexus_types::deployment::Policy; + use nexus_types::deployment::SledResources; + use nexus_types::inventory::Collection; + use omicron_common::address::Ipv6Subnet; + use omicron_test_utils::dev; + use rand::thread_rng; + use rand::Rng; + use std::mem; + use std::net::Ipv6Addr; + + static EMPTY_POLICY: Policy = Policy { sleds: BTreeMap::new() }; + + // This is a not-super-future-maintainer-friendly helper to check that all + // the subtables related to blueprints have been pruned of a specific + // blueprint ID. If additional blueprint tables are added in the future, + // this function will silently ignore them unless they're manually added. + async fn ensure_blueprint_fully_deleted( + datastore: &DataStore, + blueprint_id: Uuid, + ) { + let conn = datastore.pool_connection_for_tests().await.unwrap(); + + macro_rules! query_count { + ($table:ident, $blueprint_id_col:ident) => {{ + use db::schema::$table::dsl; + let result = dsl::$table + .filter(dsl::$blueprint_id_col.eq(blueprint_id)) + .count() + .get_result_async(&*conn) + .await; + (stringify!($table), result) + }}; + } + + for (table_name, result) in [ + query_count!(blueprint, id), + query_count!(bp_omicron_zone, blueprint_id), + query_count!(bp_omicron_zone_nic, blueprint_id), + query_count!(bp_omicron_zones_not_in_service, blueprint_id), + ] { + let count: i64 = result.unwrap(); + assert_eq!( + count, 0, + "nonzero row count for blueprint \ + {blueprint_id} in table {table_name}" + ); + } + } + + // Create a fake set of `SledResources`, either with a subnet matching + // `ip` or with an arbitrary one. + fn fake_sled_resources(ip: Option) -> SledResources { + use illumos_utils::zpool::ZpoolName; + let zpools = (0..4) + .map(|_| { + let name = ZpoolName::new_external(Uuid::new_v4()).to_string(); + name.parse().unwrap() + }) + .collect(); + let ip = ip.unwrap_or_else(|| thread_rng().gen::().into()); + SledResources { zpools, subnet: Ipv6Subnet::new(ip) } + } + + // Create a `Policy` that contains all the sleds found in `collection` + fn policy_from_collection(collection: &Collection) -> Policy { + Policy { + sleds: collection + .sled_agents + .iter() + .map(|(sled_id, agent)| { + // `Collection` doesn't currently hold zpool names, so + // we'll construct fake resources for each sled. + ( + *sled_id, + fake_sled_resources(Some( + *agent.sled_agent_address.ip(), + )), + ) + }) + .collect(), + } + } + + fn representative() -> (Collection, Policy, Blueprint) { + // We'll start with a representative collection... + let mut collection = + nexus_inventory::examples::representative().builder.build(); + + // ...and then mutate it such that the omicron zones it reports match + // the sled agent IDs it reports. Steal the sled agent info and drop the + // fake sled-agent IDs: + let mut empty_map = BTreeMap::new(); + mem::swap(&mut empty_map, &mut collection.sled_agents); + let mut sled_agents = empty_map.into_values().collect::>(); + + // Now reinsert them with IDs pulled from the omicron zones. This + // assumes we have more fake sled agents than omicron zones, which is + // currently true for the representative collection. + for &sled_id in collection.omicron_zones.keys() { + let some_sled_agent = sled_agents.pop().expect( + "fewer representative sled agents than \ + representative omicron zones sleds", + ); + collection.sled_agents.insert(sled_id, some_sled_agent); + } + + let policy = policy_from_collection(&collection); + let blueprint = BlueprintBuilder::build_initial_from_collection( + &collection, + &policy, + "test", + ) + .unwrap(); + + (collection, policy, blueprint) + } + + #[tokio::test] + async fn test_empty_blueprint() { + // Setup + let logctx = dev::test_setup_log("inventory_insert"); + let mut db = test_setup_database(&logctx.log).await; + let (opctx, datastore) = datastore_test(&logctx, &db).await; + + // Create an empty collection and a blueprint from it + let collection = + nexus_inventory::CollectionBuilder::new("test").build(); + let blueprint1 = BlueprintBuilder::build_initial_from_collection( + &collection, + &EMPTY_POLICY, + "test", + ) + .unwrap(); + + // Write it to the database and read it back. + datastore + .blueprint_insert(&opctx, &blueprint1) + .await + .expect("failed to insert blueprint"); + let blueprint_read = datastore + .blueprint_read(&opctx, blueprint1.id) + .await + .expect("failed to read collection back"); + assert_eq!(blueprint1, blueprint_read); + + // There ought to be no sleds or zones in service, and no parent + // blueprint. + assert_eq!(blueprint1.omicron_zones.len(), 0); + assert_eq!(blueprint1.zones_in_service.len(), 0); + assert_eq!(blueprint1.parent_blueprint_id, None); + + // Delete the blueprint and ensure it's really gone. + datastore.blueprint_delete(&opctx, blueprint1.id).await.unwrap(); + ensure_blueprint_fully_deleted(&datastore, blueprint1.id).await; + + // Clean up. + db.cleanup().await.unwrap(); + logctx.cleanup_successful(); + } + + #[tokio::test] + async fn test_representative_blueprint() { + // Setup + let logctx = dev::test_setup_log("inventory_insert"); + let mut db = test_setup_database(&logctx.log).await; + let (opctx, datastore) = datastore_test(&logctx, &db).await; + + // Create a cohesive representative collection/policy/blueprint + let (collection, mut policy, blueprint1) = representative(); + + // Write it to the database and read it back. + datastore + .blueprint_insert(&opctx, &blueprint1) + .await + .expect("failed to insert blueprint"); + let blueprint_read = datastore + .blueprint_read(&opctx, blueprint1.id) + .await + .expect("failed to read collection back"); + assert_eq!(blueprint1, blueprint_read); + + // Check the number of blueprint elements against our collection. + assert_eq!(blueprint1.omicron_zones.len(), policy.sleds.len()); + assert_eq!( + blueprint1.omicron_zones.len(), + collection.omicron_zones.len() + ); + assert_eq!( + blueprint1.all_omicron_zones().count(), + collection.all_omicron_zones().count() + ); + // All zones should be in service. + assert_eq!( + blueprint1.zones_in_service.len(), + blueprint1.all_omicron_zones().count() + ); + assert_eq!(blueprint1.parent_blueprint_id, None); + + // TODO SET AS TARGET AND ENSURE WE CAN'T DELETE + + // Add a new sled to `policy`. + let new_sled_id = Uuid::new_v4(); + policy.sleds.insert(new_sled_id, fake_sled_resources(None)); + let new_sled_zpools = &policy.sleds.get(&new_sled_id).unwrap().zpools; + + // Create a builder for a child blueprint. + let mut builder = + BlueprintBuilder::new_based_on(&blueprint1, &policy, "test"); + + // Add zones to our new sled. + assert_eq!( + builder.sled_ensure_zone_ntp(new_sled_id).unwrap(), + Ensure::Added + ); + for zpool_name in new_sled_zpools { + assert_eq!( + builder + .sled_ensure_zone_crucible(new_sled_id, zpool_name.clone()) + .unwrap(), + Ensure::Added + ); + } + let num_new_sled_zones = 1 + new_sled_zpools.len(); + + let mut blueprint2 = builder.build(); + + // Sort the new sled's omicron zones by ID to match how we fetch them + // from the db. + blueprint2 + .omicron_zones + .get_mut(&new_sled_id) + .unwrap() + .zones + .sort_by_key(|zone| zone.id); + + // Check that we added the new sled and its zones. + assert_eq!( + blueprint1.omicron_zones.len() + 1, + blueprint2.omicron_zones.len() + ); + assert_eq!( + blueprint1.all_omicron_zones().count() + num_new_sled_zones, + blueprint2.all_omicron_zones().count() + ); + + // All zones should be in service. + assert_eq!( + blueprint2.zones_in_service.len(), + blueprint2.all_omicron_zones().count() + ); + assert_eq!(blueprint2.parent_blueprint_id, Some(blueprint1.id)); + + // Check that we can write it to the DB and read it back. + datastore + .blueprint_insert(&opctx, &blueprint2) + .await + .expect("failed to insert blueprint"); + let blueprint_read = datastore + .blueprint_read(&opctx, blueprint2.id) + .await + .expect("failed to read collection back"); + println!("diff: {}", blueprint2.diff(&blueprint_read)); + assert_eq!(blueprint2, blueprint_read); + + // TODO SET AS TARGET AND ENSURE WE CAN'T DELETE + + // Now that blueprint2 is the target, we should be able to delete + // blueprint1. + datastore.blueprint_delete(&opctx, blueprint1.id).await.unwrap(); + ensure_blueprint_fully_deleted(&datastore, blueprint1.id).await; + + // Clean up. + db.cleanup().await.unwrap(); + logctx.cleanup_successful(); + } +} diff --git a/nexus/db-queries/src/db/datastore/mod.rs b/nexus/db-queries/src/db/datastore/mod.rs index 78a7aeda87..96832b25bf 100644 --- a/nexus/db-queries/src/db/datastore/mod.rs +++ b/nexus/db-queries/src/db/datastore/mod.rs @@ -54,6 +54,7 @@ mod certificate; mod console_session; mod dataset; mod db_metadata; +mod deployment; mod device_auth; mod disk; mod dns; diff --git a/nexus/deployment/Cargo.toml b/nexus/deployment/Cargo.toml index b166f947bf..115dec98a5 100644 --- a/nexus/deployment/Cargo.toml +++ b/nexus/deployment/Cargo.toml @@ -9,6 +9,7 @@ chrono.workspace = true internal-dns.workspace = true ipnet.workspace = true ipnetwork.workspace = true +nexus-inventory.workspace = true nexus-types.workspace = true omicron-common.workspace = true slog.workspace = true @@ -18,6 +19,5 @@ uuid.workspace = true omicron-workspace-hack.workspace = true [dev-dependencies] -nexus-inventory.workspace = true omicron-test-utils.workspace = true sled-agent-client.workspace = true diff --git a/nexus/deployment/src/blueprint_builder.rs b/nexus/deployment/src/blueprint_builder.rs index 689e2d8e2c..2a51080683 100644 --- a/nexus/deployment/src/blueprint_builder.rs +++ b/nexus/deployment/src/blueprint_builder.rs @@ -9,6 +9,7 @@ use anyhow::anyhow; use internal_dns::config::Host; use internal_dns::config::ZoneVariant; use ipnet::IpAdd; +use nexus_inventory::now_db_precision; use nexus_types::deployment::Blueprint; use nexus_types::deployment::OmicronZoneConfig; use nexus_types::deployment::OmicronZoneDataset; @@ -125,10 +126,10 @@ impl<'a> BlueprintBuilder<'a> { collection.all_omicron_zones().map(|z| z.id).collect(); Ok(Blueprint { id: Uuid::new_v4(), - omicron_zones: omicron_zones, + omicron_zones, zones_in_service, parent_blueprint_id: None, - time_created: chrono::Utc::now(), + time_created: now_db_precision(), creator: creator.to_owned(), comment: format!("from collection {}", collection.id), }) @@ -185,10 +186,10 @@ impl<'a> BlueprintBuilder<'a> { .collect(); Blueprint { id: Uuid::new_v4(), - omicron_zones: omicron_zones, + omicron_zones, zones_in_service: self.zones_in_service, parent_blueprint_id: Some(self.parent_blueprint.id), - time_created: chrono::Utc::now(), + time_created: now_db_precision(), creator: self.creator, comment: self.comments.join(", "), } diff --git a/nexus/inventory/src/builder.rs b/nexus/inventory/src/builder.rs index 62d338c1ee..08a905143c 100644 --- a/nexus/inventory/src/builder.rs +++ b/nexus/inventory/src/builder.rs @@ -96,7 +96,7 @@ impl CollectionBuilder { pub fn new(collector: &str) -> Self { CollectionBuilder { errors: vec![], - time_started: now(), + time_started: now_db_precision(), collector: collector.to_owned(), baseboards: BTreeSet::new(), cabooses: BTreeSet::new(), @@ -122,7 +122,7 @@ impl CollectionBuilder { id: Uuid::new_v4(), errors: self.errors.into_iter().map(|e| e.to_string()).collect(), time_started: self.time_started, - time_done: now(), + time_done: now_db_precision(), collector: self.collector, baseboards: self.baseboards, cabooses: self.cabooses, @@ -178,7 +178,7 @@ impl CollectionBuilder { // Separate the SP state into the SP-specific state and the RoT state, // if any. - let now = now(); + let now = now_db_precision(); let _ = self.sps.entry(baseboard.clone()).or_insert_with(|| { ServiceProcessor { time_collected: now, @@ -279,7 +279,7 @@ impl CollectionBuilder { if let Some(previous) = by_id.insert( baseboard.clone(), CabooseFound { - time_collected: now(), + time_collected: now_db_precision(), source: source.to_owned(), caboose: sw_caboose.clone(), }, @@ -348,7 +348,7 @@ impl CollectionBuilder { if let Some(previous) = by_id.insert( baseboard.clone(), RotPageFound { - time_collected: now(), + time_collected: now_db_precision(), source: source.to_owned(), page: sw_rot_page.clone(), }, @@ -456,7 +456,7 @@ impl CollectionBuilder { usable_hardware_threads: inventory.usable_hardware_threads, usable_physical_ram: inventory.usable_physical_ram, reservoir_size: inventory.reservoir_size, - time_collected: now(), + time_collected: now_db_precision(), sled_id, }; @@ -491,7 +491,7 @@ impl CollectionBuilder { self.omicron_zones.insert( sled_id, OmicronZonesFound { - time_collected: now(), + time_collected: now_db_precision(), source: source.to_string(), sled_id, zones, @@ -507,7 +507,7 @@ impl CollectionBuilder { /// This exists because the database doesn't store nanosecond-precision, so if /// we store nanosecond-precision timestamps, then DateTime conversion is lossy /// when round-tripping through the database. That's rather inconvenient. -fn now() -> DateTime { +pub fn now_db_precision() -> DateTime { let ts = Utc::now(); let nanosecs = ts.timestamp_subsec_nanos(); let micros = ts.timestamp_subsec_micros(); @@ -517,7 +517,7 @@ fn now() -> DateTime { #[cfg(test)] mod test { - use super::now; + use super::now_db_precision; use super::CollectionBuilder; use crate::examples::representative; use crate::examples::sp_state; @@ -541,10 +541,10 @@ mod test { // Verify the contents of an empty collection. #[test] fn test_empty() { - let time_before = now(); + let time_before = now_db_precision(); let builder = CollectionBuilder::new("test_empty"); let collection = builder.build(); - let time_after = now(); + let time_after = now_db_precision(); assert!(collection.errors.is_empty()); assert!(time_before <= collection.time_started); @@ -577,7 +577,7 @@ mod test { // a useful quick check. #[test] fn test_basic() { - let time_before = now(); + let time_before = now_db_precision(); let Representative { builder, sleds: [sled1_bb, sled2_bb, sled3_bb, sled4_bb], @@ -587,7 +587,7 @@ mod test { [sled_agent_id_basic, sled_agent_id_extra, sled_agent_id_pc, sled_agent_id_unknown], } = representative(); let collection = builder.build(); - let time_after = now(); + let time_after = now_db_precision(); println!("{:#?}", collection); assert!(time_before <= collection.time_started); assert!(collection.time_started <= collection.time_done); diff --git a/nexus/inventory/src/lib.rs b/nexus/inventory/src/lib.rs index f11af8fede..6dee7bb7ec 100644 --- a/nexus/inventory/src/lib.rs +++ b/nexus/inventory/src/lib.rs @@ -27,6 +27,8 @@ pub use builder::CollectionBuilder; pub use builder::CollectorBug; pub use builder::InventoryError; +pub use builder::now_db_precision; + pub use collector::Collector; pub use sled_agent_enumerator::SledAgentEnumerator; From 8d3cc25b799f1c1d01fbf04f23bab20ca411d7ed Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Wed, 24 Jan 2024 14:21:10 -0500 Subject: [PATCH 03/24] Add queries for getting and setting the current blueprint target --- nexus/db-model/src/deployment.rs | 34 +- nexus/db-model/src/schema.rs | 11 + nexus/db-queries/src/authz/api_resources.rs | 55 -- nexus/db-queries/src/authz/omicron.polar | 11 - nexus/db-queries/src/authz/oso_generic.rs | 1 - .../src/authz/policy_test/resource_builder.rs | 1 - .../src/authz/policy_test/resources.rs | 3 +- .../db-queries/src/db/datastore/deployment.rs | 664 +++++++++++++++++- nexus/src/app/deployment.rs | 56 +- nexus/src/internal_api/http_entrypoints.rs | 45 +- nexus/types/src/deployment.rs | 4 +- schema/crdb/dbinit.sql | 24 + 12 files changed, 764 insertions(+), 145 deletions(-) diff --git a/nexus/db-model/src/deployment.rs b/nexus/db-model/src/deployment.rs index fdf0cdacd1..c23bf9df61 100644 --- a/nexus/db-model/src/deployment.rs +++ b/nexus/db-model/src/deployment.rs @@ -9,11 +9,12 @@ use crate::inventory::ZoneType; use crate::omicron_zone_config::{OmicronZone, OmicronZoneNic}; use crate::schema::{ blueprint, bp_omicron_zone, bp_omicron_zone_nic, - bp_omicron_zones_not_in_service, bp_sled_omicron_zones, + bp_omicron_zones_not_in_service, bp_sled_omicron_zones, bp_target, }; use crate::{ipv6, Generation, MacAddr, Name, SqlU16, SqlU32, SqlU8}; use chrono::{DateTime, Utc}; use ipnetwork::IpNetwork; +use nexus_types::deployment::BlueprintTarget; use uuid::Uuid; /// See [`nexus_types::deployment::Blueprint`]. @@ -39,6 +40,37 @@ impl From<&'_ nexus_types::deployment::Blueprint> for Blueprint { } } +/// See [`nexus_types::deployment::BlueprintTarget`]. +#[derive(Queryable, Clone, Debug, Selectable, Insertable)] +#[diesel(table_name = bp_target)] +pub struct BpTarget { + pub version: i64, // i64 only for db serialization; should never be negative + pub blueprint_id: Uuid, + pub enabled: bool, + pub time_made_target: DateTime, +} + +impl BpTarget { + pub fn new(version: i64, target: BlueprintTarget) -> Self { + Self { + version, + blueprint_id: target.target_id, + enabled: target.enabled, + time_made_target: target.time_set, + } + } +} + +impl From for nexus_types::deployment::BlueprintTarget { + fn from(value: BpTarget) -> Self { + Self { + target_id: value.blueprint_id, + enabled: value.enabled, + time_set: value.time_made_target, + } + } +} + /// See [`nexus_types::deployment::OmicronZonesConfig`]. #[derive(Queryable, Clone, Debug, Selectable, Insertable)] #[diesel(table_name = bp_sled_omicron_zones)] diff --git a/nexus/db-model/src/schema.rs b/nexus/db-model/src/schema.rs index 9b274efc6b..ddb5ba8e03 100644 --- a/nexus/db-model/src/schema.rs +++ b/nexus/db-model/src/schema.rs @@ -1402,6 +1402,17 @@ table! { } } +table! { + bp_target (version) { + version -> Int8, + + blueprint_id -> Uuid, + + enabled -> Bool, + time_made_target -> Timestamptz, + } +} + table! { bp_sled_omicron_zones (blueprint_id, sled_id) { blueprint_id -> Uuid, diff --git a/nexus/db-queries/src/authz/api_resources.rs b/nexus/db-queries/src/authz/api_resources.rs index d8460486ef..b4fd4e1890 100644 --- a/nexus/db-queries/src/authz/api_resources.rs +++ b/nexus/db-queries/src/authz/api_resources.rs @@ -577,61 +577,6 @@ impl AuthorizedResource for Inventory { } } -/// Synthetic resource used for modeling access to deployment configuration -/// data (e.g., blueprints and policy) -#[derive(Clone, Copy, Debug, Eq, PartialEq)] -pub struct DeploymentConfig; -pub const DEPLOYMENT_CONFIG: DeploymentConfig = DeploymentConfig {}; - -impl oso::PolarClass for DeploymentConfig { - fn get_polar_class_builder() -> oso::ClassBuilder { - // Roles are not directly attached to DeploymentConfig - oso::Class::builder() - .with_equality_check() - .add_method( - "has_role", - |_: &DeploymentConfig, - _actor: AuthenticatedActor, - _role: String| { false }, - ) - .add_attribute_getter("fleet", |_| FLEET) - } -} - -impl AuthorizedResource for DeploymentConfig { - fn load_roles<'a, 'b, 'c, 'd, 'e, 'f>( - &'a self, - opctx: &'b OpContext, - datastore: &'c DataStore, - authn: &'d authn::Context, - roleset: &'e mut RoleSet, - ) -> futures::future::BoxFuture<'f, Result<(), Error>> - where - 'a: 'f, - 'b: 'f, - 'c: 'f, - 'd: 'f, - 'e: 'f, - { - load_roles_for_resource_tree(&FLEET, opctx, datastore, authn, roleset) - .boxed() - } - - fn on_unauthorized( - &self, - _: &Authz, - error: Error, - _: AnyActor, - _: Action, - ) -> Error { - error - } - - fn polar_class(&self) -> oso::Class { - Self::get_polar_class() - } -} - /// Synthetic resource describing the list of Certificates associated with a /// Silo #[derive(Clone, Debug, Eq, PartialEq)] diff --git a/nexus/db-queries/src/authz/omicron.polar b/nexus/db-queries/src/authz/omicron.polar index cef6874673..f9382401fd 100644 --- a/nexus/db-queries/src/authz/omicron.polar +++ b/nexus/db-queries/src/authz/omicron.polar @@ -393,17 +393,6 @@ resource Inventory { has_relation(fleet: Fleet, "parent_fleet", inventory: Inventory) if inventory.fleet = fleet; -# Describes the policy for reading and modifying deployment configuration and -# policy -resource DeploymentConfig { - permissions = [ "read", "modify" ]; - relations = { parent_fleet: Fleet }; - "read" if "viewer" on "parent_fleet"; - "modify" if "admin" on "parent_fleet"; -} -has_relation(fleet: Fleet, "parent_fleet", deployment_config: DeploymentConfig) - if deployment_config.fleet = fleet; - # Describes the policy for accessing "/v1/system/ip-pools" in the API resource IpPoolList { permissions = [ diff --git a/nexus/db-queries/src/authz/oso_generic.rs b/nexus/db-queries/src/authz/oso_generic.rs index b6b9c59afe..dd646a1c98 100644 --- a/nexus/db-queries/src/authz/oso_generic.rs +++ b/nexus/db-queries/src/authz/oso_generic.rs @@ -105,7 +105,6 @@ pub fn make_omicron_oso(log: &slog::Logger) -> Result { AuthenticatedActor::get_polar_class(), BlueprintConfig::get_polar_class(), Database::get_polar_class(), - DeploymentConfig::get_polar_class(), DnsConfig::get_polar_class(), Fleet::get_polar_class(), Inventory::get_polar_class(), diff --git a/nexus/db-queries/src/authz/policy_test/resource_builder.rs b/nexus/db-queries/src/authz/policy_test/resource_builder.rs index c86407d438..dc18b2e47f 100644 --- a/nexus/db-queries/src/authz/policy_test/resource_builder.rs +++ b/nexus/db-queries/src/authz/policy_test/resource_builder.rs @@ -245,7 +245,6 @@ macro_rules! impl_dyn_authorized_resource_for_global { impl_dyn_authorized_resource_for_global!(authz::oso_generic::Database); impl_dyn_authorized_resource_for_global!(authz::BlueprintConfig); impl_dyn_authorized_resource_for_global!(authz::ConsoleSessionList); -impl_dyn_authorized_resource_for_global!(authz::DeploymentConfig); impl_dyn_authorized_resource_for_global!(authz::DeviceAuthRequestList); impl_dyn_authorized_resource_for_global!(authz::DnsConfig); impl_dyn_authorized_resource_for_global!(authz::IpPoolList); diff --git a/nexus/db-queries/src/authz/policy_test/resources.rs b/nexus/db-queries/src/authz/policy_test/resources.rs index 83a48e1bd2..3e87f6db51 100644 --- a/nexus/db-queries/src/authz/policy_test/resources.rs +++ b/nexus/db-queries/src/authz/policy_test/resources.rs @@ -68,9 +68,8 @@ pub async fn make_resources( builder.new_resource_with_users(authz::FLEET).await; builder.new_resource(authz::BLUEPRINT_CONFIG); builder.new_resource(authz::CONSOLE_SESSION_LIST); - builder.new_resource(authz::DEPLOYMENT_CONFIG); - builder.new_resource(authz::DEVICE_AUTH_REQUEST_LIST); builder.new_resource(authz::DNS_CONFIG); + builder.new_resource(authz::DEVICE_AUTH_REQUEST_LIST); builder.new_resource(authz::INVENTORY); builder.new_resource(authz::IP_POOL_LIST); diff --git a/nexus/db-queries/src/db/datastore/deployment.rs b/nexus/db-queries/src/db/datastore/deployment.rs index 059e6aa093..9309ca29e0 100644 --- a/nexus/db-queries/src/db/datastore/deployment.rs +++ b/nexus/db-queries/src/db/datastore/deployment.rs @@ -10,19 +10,34 @@ use crate::db::error::public_error_from_diesel; use crate::db::error::ErrorHandler; use crate::db::pagination::paginated; use crate::db::pagination::Paginator; +use crate::db::DbConnection; use crate::db::TransactionError; use anyhow::Context; use async_bb8_diesel::AsyncConnection; use async_bb8_diesel::AsyncRunQueryDsl; +use chrono::DateTime; +use chrono::Utc; use diesel::expression::SelectableHelper; +use diesel::pg::Pg; +use diesel::query_builder::AstPass; +use diesel::query_builder::QueryFragment; +use diesel::query_builder::QueryId; +use diesel::result::DatabaseErrorKind; +use diesel::result::Error as DieselError; +use diesel::sql_types; +use diesel::Column; use diesel::ExpressionMethods; +use diesel::OptionalExtension; use diesel::QueryDsl; +use diesel::RunQueryDsl; use nexus_db_model::Blueprint as DbBlueprint; use nexus_db_model::BpOmicronZone; use nexus_db_model::BpOmicronZoneNic; use nexus_db_model::BpOmicronZoneNotInService; use nexus_db_model::BpSledOmicronZones; +use nexus_db_model::BpTarget; use nexus_types::deployment::Blueprint; +use nexus_types::deployment::BlueprintTarget; use nexus_types::deployment::OmicronZonesConfig; use omicron_common::api::external::Error; use omicron_common::bail_unless; @@ -38,8 +53,7 @@ use uuid::Uuid; /// [`Paginator`] to guard against single queries returning an unchecked number /// of rows. // unsafe: `new_unchecked` is only unsound if the argument is 0. -// TODO XXX set to 1000; currently 1 for testing -const SQL_BATCH_SIZE: NonZeroU32 = unsafe { NonZeroU32::new_unchecked(1) }; +const SQL_BATCH_SIZE: NonZeroU32 = unsafe { NonZeroU32::new_unchecked(1000) }; impl DataStore { /// Store a complete blueprint into the database @@ -49,7 +63,7 @@ impl DataStore { blueprint: &Blueprint, ) -> Result<(), Error> { opctx - .authorize(authz::Action::Modify, &authz::DEPLOYMENT_CONFIG) + .authorize(authz::Action::Modify, &authz::BLUEPRINT_CONFIG) .await?; // In the database, the blueprint is represented essentially as a tree @@ -186,7 +200,7 @@ impl DataStore { opctx: &OpContext, blueprint_id: Uuid, ) -> Result { - opctx.authorize(authz::Action::Read, &authz::DEPLOYMENT_CONFIG).await?; + opctx.authorize(authz::Action::Read, &authz::BLUEPRINT_CONFIG).await?; let conn = self.pool_connection_authorized(opctx).await?; // Read the metadata from the primary blueprint row, and ensure that it @@ -459,8 +473,6 @@ impl DataStore { // collection if we crash while deleting it. let conn = self.pool_connection_authorized(opctx).await?; - // TODO XXX check that this blueprint is not the target - let ( nblueprints, nsled_agent_zones, @@ -469,6 +481,21 @@ impl DataStore { nzones_not_in_service, ) = conn .transaction_async(|conn| async move { + // Ensure that blueprint we're about to delete is not the + // current target. + let current_target = + self.blueprint_current_target_only(&conn).await?; + if let Some(current_target) = current_target { + if current_target.target_id == blueprint_id { + return Err(TransactionError::CustomError( + Error::conflict(format!( + "blueprint {blueprint_id} is the \ + current target and cannot be deleted", + )), + )); + } + } + // Remove the record describing the blueprint itself. let nblueprints = { use db::schema::blueprint::dsl; @@ -547,14 +574,422 @@ impl DataStore { Ok(()) } + + /// Set the current target blueprint + /// + /// In order to become the target blueprint, `target`'s parent blueprint + /// must be the current target + pub async fn blueprint_target_set_current( + &self, + opctx: &OpContext, + target: BlueprintTarget, + ) -> Result<(), Error> { + opctx + .authorize(authz::Action::Modify, &authz::BLUEPRINT_CONFIG) + .await?; + + let query = InsertTargetQuery { + target_id: target.target_id, + enabled: target.enabled, + time_made_target: target.time_set, + }; + + let conn = self.pool_connection_authorized(opctx).await?; + + query + .execute_async(&*conn) + .await + .map_err(|e| Error::from(query.decode_error(e)))?; + + Ok(()) + } + + /// Get the current target blueprint, if one exists + pub async fn blueprint_target_get_current( + &self, + opctx: &OpContext, + ) -> Result, Error> { + opctx.authorize(authz::Action::Read, &authz::BLUEPRINT_CONFIG).await?; + + let conn = self.pool_connection_authorized(opctx).await?; + let Some(target) = self.blueprint_current_target_only(&conn).await? + else { + return Ok(None); + }; + + // The blueprint for the current target cannot be deleted while it is + // the current target, but it's possible someone else (a) made a new + // blueprint the target and (b) deleted the blueprint pointed to by our + // `target` between the above query and the below query. In such a case, + // this query will fail with an "unknown blueprint ID" error. This + // should be rare in practice, and is always possible during + // `blueprint_read` anyway since it has to read from multiple tables to + // construct the blueprint. + let blueprint = self.blueprint_read(opctx, target.target_id).await?; + + Ok(Some((target, blueprint))) + } + + // Helper to fetch the current blueprint target (without fetching the entire + // blueprint for that target). + // + // Caller is responsible for checking authz for this operation. + async fn blueprint_current_target_only( + &self, + conn: &async_bb8_diesel::Connection, + ) -> Result, Error> { + use db::schema::bp_target::dsl; + + let current_target = dsl::bp_target + .order_by(dsl::version.desc()) + .first_async::(conn) + .await + .optional() + .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; + + Ok(current_target.map(BlueprintTarget::from)) + } +} + +/// Errors related to inserting a target blueprint +#[derive(Debug)] +enum InsertTargetError { + /// The requested target blueprint ID does not exist in the blueprint table. + NoSuchBlueprint(Uuid), + /// The requested target blueprint's parent does not match the current + /// target. + ParentNotTarget(Uuid), + /// Any other error + Other(DieselError), } +impl From for Error { + fn from(value: InsertTargetError) -> Self { + match value { + InsertTargetError::NoSuchBlueprint(id) => { + Error::invalid_request(format!("Blueprint {id} does not exist")) + } + InsertTargetError::ParentNotTarget(id) => { + Error::invalid_request(format!( + "Blueprint {id}'s parent blueprint is not the current \ + target blueprint" + )) + } + InsertTargetError::Other(e) => { + public_error_from_diesel(e, ErrorHandler::Server) + } + } + } +} + +/// Query to insert a new current target blueprint. +/// +/// The `bp_target` table's primary key is the `version` field, and we enforce +/// the following invariants: +/// +/// * The first "current target" blueprint is assigned version 1. +/// * In order to be inserted as the first current target blueprint, a +/// blueprint must have a parent_blueprint_id of NULL. +/// * After the first, any subsequent blueprint can only be assigned as the +/// current target if its parent_blueprint_id is the current target blueprint. +/// * When inserting a new child blueprint as the current target, it is assigned +/// a version of 1 + its parent's version. +/// +/// The result of this is a linear history of blueprints, where each target is a +/// direct child of the previous current target. Enforcing the above has some +/// subtleties (particularly around handling the "first blueprint with no +/// parent" case). These are expanded on below through inline comments on the +/// query we generate: +/// +/// ```sql +/// WITH +/// -- Subquery to fetch the current target (i.e., the row with the max +/// -- veresion in `bp_target`). +/// current_target AS ( +/// SELECT +/// "version" AS version, +/// "blueprint_id" AS blueprint_id +/// FROM "bp_target" +/// ORDER BY "version" DESC +/// LIMIT 1 +/// ), +/// +/// -- Error checking subquery: This uses similar tricks as elsewhere in +/// -- this crate to `CAST(... AS UUID)` with non-UUID values that result +/// -- in runtime errors in specific cases, allowing us to give accurate +/// -- error messages. +/// -- +/// -- These checks are not required for correct behavior by the insert +/// -- below. If we removed them, the insert would insert 0 rows if +/// -- these checks would have failed. But they make it easier to report +/// -- specific problems to our caller. +/// -- +/// -- The specific cases we check here are noted below. +/// check_validity AS MATERIALIZED ( +/// SELECT CAST(IF( +/// -- Return `no-such-blueprint` if the ID we're being told to +/// -- set as the target doesn't exist in the blueprint table. +/// (SELECT "id" FROM "blueprint" WHERE "id" = ) IS NULL, +/// 'no-such-blueprint', +/// IF( +/// -- Check for whether our new target's parent matches our current +/// -- target. There are two cases here: The first is the common case +/// -- (i.e., the new target has a parent: does it match the current +/// -- target ID?). The second is the bootstrapping check: if we're +/// -- trying to insert a new target that does not have a parent, +/// -- we should not have a current target at all. +/// -- +/// -- If either of these cases fails, we return `parent-not-target`. +/// ( +/// SELECT "parent_blueprint_id" FROM "blueprint", current_target +/// WHERE +/// "id" = +/// AND current_target.blueprint_id = "parent_blueprint_id" +/// ) IS NOT NULL +/// OR +/// ( +/// SELECT 1 FROM "blueprint" +/// WHERE +/// "id" = +/// AND "parent_blueprint_id" IS NULL +/// AND NOT EXISTS (SELECT version FROM current_target) +/// ) = 1, +/// , +/// 'parent-not-target' +/// ) +/// ) AS UUID) +/// ), +/// +/// -- Determine the new version number to use: either 1 if this is the +/// -- first blueprint being made the current target, or 1 higher than +/// -- the previous target's version. +/// -- +/// -- The final clauses of each of these WHERE clauses repeat the +/// -- checks performed above in `check_validity`, and will cause this +/// -- subquery to return no rows if we should not allow the new +/// -- target to be set. +/// new_target AS ( +/// SELECT 1 AS new_version FROM "blueprint" +/// WHERE +/// "id" = +/// AND "parent_blueprint_id" IS NULL +/// AND NOT EXISTS (SELECT version FROM current_target) +/// UNION +/// SELECT current_target.version + 1 FROM current_target, "blueprint" +/// WHERE +/// "id" = +/// AND "parent_blueprint_id" IS NOT NULL +/// AND "parent_blueprint_id" = current_target.blueprint_id +/// ) +/// +/// -- Perform the actual insertion. +/// INSERT INTO "bp_target"( +/// "version","blueprint_id","enabled","time_made_target" +/// ) +/// SELECT +/// new_target.new_version, +/// , +/// , +/// +/// FROM new_target +/// ``` +#[derive(Debug, Clone, Copy)] +struct InsertTargetQuery { + target_id: Uuid, + enabled: bool, + time_made_target: DateTime, +} + +// Uncastable sentinel used to detect we attempt to make a blueprint the target +// when it does not exist in the blueprint table. +const NO_SUCH_BLUEPRINT_SENTINEL: &str = "no-such-blueprint"; + +// Uncastable sentinel used to detect we attempt to make a blueprint the target +// when its parent_blueprint_id is not the current target. +const PARENT_NOT_TARGET_SENTINEL: &str = "parent-not-target"; + +// Error messages generated from the above sentinel values. +const NO_SUCH_BLUEPRINT_ERROR_MESSAGE: &str = + "could not parse \"no-such-blueprint\" as type uuid: \ + uuid: incorrect UUID length: no-such-blueprint"; +const PARENT_NOT_TARGET_ERROR_MESSAGE: &str = + "could not parse \"parent-not-target\" as type uuid: \ + uuid: incorrect UUID length: parent-not-target"; + +impl InsertTargetQuery { + fn decode_error(&self, err: DieselError) -> InsertTargetError { + match err { + DieselError::DatabaseError(DatabaseErrorKind::Unknown, info) + if info.message() == NO_SUCH_BLUEPRINT_ERROR_MESSAGE => + { + InsertTargetError::NoSuchBlueprint(self.target_id) + } + DieselError::DatabaseError(DatabaseErrorKind::Unknown, info) + if info.message() == PARENT_NOT_TARGET_ERROR_MESSAGE => + { + InsertTargetError::ParentNotTarget(self.target_id) + } + other => InsertTargetError::Other(other), + } + } +} + +impl QueryId for InsertTargetQuery { + type QueryId = (); + const HAS_STATIC_QUERY_ID: bool = false; +} + +impl QueryFragment for InsertTargetQuery { + fn walk_ast<'a>( + &'a self, + mut out: AstPass<'_, 'a, Pg>, + ) -> diesel::QueryResult<()> { + use crate::db::schema::blueprint::dsl as bp_dsl; + use crate::db::schema::bp_target::dsl; + + type FromClause = + diesel::internal::table_macro::StaticQueryFragmentInstance; + type BpTargetFromClause = FromClause; + type BlueprintFromClause = FromClause; + const BP_TARGET_FROM_CLAUSE: BpTargetFromClause = + BpTargetFromClause::new(); + const BLUEPRINT_FROM_CLAUSE: BlueprintFromClause = + BlueprintFromClause::new(); + + out.push_sql("WITH "); + + out.push_sql("current_target AS (SELECT "); + out.push_identifier(dsl::version::NAME)?; + out.push_sql(" AS version,"); + out.push_identifier(dsl::blueprint_id::NAME)?; + out.push_sql(" AS blueprint_id FROM "); + BP_TARGET_FROM_CLAUSE.walk_ast(out.reborrow())?; + out.push_sql(" ORDER BY "); + out.push_identifier(dsl::version::NAME)?; + out.push_sql(" DESC LIMIT 1),"); + + out.push_sql( + "check_validity AS MATERIALIZED ( \ + SELECT \ + CAST( \ + IF( \ + (SELECT ", + ); + out.push_identifier(bp_dsl::id::NAME)?; + out.push_sql(" FROM "); + BLUEPRINT_FROM_CLAUSE.walk_ast(out.reborrow())?; + out.push_sql(" WHERE "); + out.push_identifier(bp_dsl::id::NAME)?; + out.push_sql(" = "); + out.push_bind_param::(&self.target_id)?; + out.push_sql(") IS NULL, "); + out.push_bind_param::( + &NO_SUCH_BLUEPRINT_SENTINEL, + )?; + out.push_sql( + ", \ + IF( \ + (SELECT ", + ); + out.push_identifier(bp_dsl::parent_blueprint_id::NAME)?; + out.push_sql(" FROM "); + BLUEPRINT_FROM_CLAUSE.walk_ast(out.reborrow())?; + out.push_sql(", current_target WHERE "); + out.push_identifier(bp_dsl::id::NAME)?; + out.push_sql(" = "); + out.push_bind_param::(&self.target_id)?; + out.push_sql(" AND current_target.blueprint_id = "); + out.push_identifier(bp_dsl::parent_blueprint_id::NAME)?; + out.push_sql( + " ) IS NOT NULL \ + OR \ + (SELECT 1 FROM ", + ); + BLUEPRINT_FROM_CLAUSE.walk_ast(out.reborrow())?; + out.push_sql(" WHERE "); + out.push_identifier(bp_dsl::id::NAME)?; + out.push_sql(" = "); + out.push_bind_param::(&self.target_id)?; + out.push_sql(" AND "); + out.push_identifier(bp_dsl::parent_blueprint_id::NAME)?; + out.push_sql( + " IS NULL \ + AND NOT EXISTS ( \ + SELECT version FROM current_target) \ + ) = 1, ", + ); + out.push_bind_param::(&self.target_id)?; + out.push_sql(", "); + out.push_bind_param::( + &PARENT_NOT_TARGET_SENTINEL, + )?; + out.push_sql( + " ) \ + ) \ + AS UUID) \ + ), ", + ); + + out.push_sql("new_target AS (SELECT 1 AS new_version FROM "); + BLUEPRINT_FROM_CLAUSE.walk_ast(out.reborrow())?; + out.push_sql(" WHERE "); + out.push_identifier(bp_dsl::id::NAME)?; + out.push_sql(" = "); + out.push_bind_param::(&self.target_id)?; + out.push_sql(" AND "); + out.push_identifier(bp_dsl::parent_blueprint_id::NAME)?; + out.push_sql( + " IS NULL \ + AND NOT EXISTS \ + (SELECT version FROM current_target) \ + UNION \ + SELECT current_target.version + 1 FROM \ + current_target, ", + ); + BLUEPRINT_FROM_CLAUSE.walk_ast(out.reborrow())?; + out.push_sql(" WHERE "); + out.push_identifier(bp_dsl::id::NAME)?; + out.push_sql(" = "); + out.push_bind_param::(&self.target_id)?; + out.push_sql(" AND "); + out.push_identifier(bp_dsl::parent_blueprint_id::NAME)?; + out.push_sql(" IS NOT NULL AND "); + out.push_identifier(bp_dsl::parent_blueprint_id::NAME)?; + out.push_sql(" = current_target.blueprint_id) "); + + out.push_sql("INSERT INTO "); + BP_TARGET_FROM_CLAUSE.walk_ast(out.reborrow())?; + out.push_sql("("); + out.push_identifier(dsl::version::NAME)?; + out.push_sql(","); + out.push_identifier(dsl::blueprint_id::NAME)?; + out.push_sql(","); + out.push_identifier(dsl::enabled::NAME)?; + out.push_sql(","); + out.push_identifier(dsl::time_made_target::NAME)?; + out.push_sql(") SELECT new_target.new_version, "); + out.push_bind_param::(&self.target_id)?; + out.push_sql(","); + out.push_bind_param::(&self.enabled)?; + out.push_sql(","); + out.push_bind_param::>( + &self.time_made_target, + )?; + out.push_sql(" FROM new_target"); + + Ok(()) + } +} + +impl RunQueryDsl for InsertTargetQuery {} + #[cfg(test)] mod tests { use super::*; use crate::db::datastore::datastore_test; use nexus_deployment::blueprint_builder::BlueprintBuilder; use nexus_deployment::blueprint_builder::Ensure; + use nexus_inventory::now_db_precision; use nexus_test_utils::db::test_setup_database; use nexus_types::deployment::Policy; use nexus_types::deployment::SledResources; @@ -754,7 +1189,32 @@ mod tests { ); assert_eq!(blueprint1.parent_blueprint_id, None); - // TODO SET AS TARGET AND ENSURE WE CAN'T DELETE + // Set blueprint1 as the current target, and ensure that we cannot + // delete it (as the current target cannot be deleted). + let bp1_target = BlueprintTarget { + target_id: blueprint1.id, + enabled: true, + time_set: now_db_precision(), + }; + datastore + .blueprint_target_set_current(&opctx, bp1_target) + .await + .unwrap(); + assert_eq!( + datastore.blueprint_target_get_current(&opctx).await.unwrap(), + Some((bp1_target, blueprint1.clone())) + ); + let err = datastore + .blueprint_delete(&opctx, blueprint1.id) + .await + .unwrap_err(); + assert!( + err.to_string().contains(&format!( + "blueprint {} is the current target and cannot be deleted", + blueprint1.id + )), + "unexpected error: {err}" + ); // Add a new sled to `policy`. let new_sled_id = Uuid::new_v4(); @@ -820,7 +1280,32 @@ mod tests { println!("diff: {}", blueprint2.diff(&blueprint_read)); assert_eq!(blueprint2, blueprint_read); - // TODO SET AS TARGET AND ENSURE WE CAN'T DELETE + // Set blueprint2 as the current target and ensure that means we can not + // delete it. + let bp2_target = BlueprintTarget { + target_id: blueprint2.id, + enabled: true, + time_set: now_db_precision(), + }; + datastore + .blueprint_target_set_current(&opctx, bp2_target) + .await + .unwrap(); + assert_eq!( + datastore.blueprint_target_get_current(&opctx).await.unwrap(), + Some((bp2_target, blueprint2.clone())) + ); + let err = datastore + .blueprint_delete(&opctx, blueprint2.id) + .await + .unwrap_err(); + assert!( + err.to_string().contains(&format!( + "blueprint {} is the current target and cannot be deleted", + blueprint2.id + )), + "unexpected error: {err}" + ); // Now that blueprint2 is the target, we should be able to delete // blueprint1. @@ -831,4 +1316,167 @@ mod tests { db.cleanup().await.unwrap(); logctx.cleanup_successful(); } + + #[tokio::test] + async fn test_set_target() { + // Setup + let logctx = dev::test_setup_log("inventory_insert"); + let mut db = test_setup_database(&logctx.log).await; + let (opctx, datastore) = datastore_test(&logctx, &db).await; + + // Trying to insert a target that doesn't reference a blueprint should + // fail with a relevant error message. + let nonexistent_blueprint_id = Uuid::new_v4(); + let err = datastore + .blueprint_target_set_current( + &opctx, + BlueprintTarget { + target_id: nonexistent_blueprint_id, + enabled: true, + time_set: now_db_precision(), + }, + ) + .await + .unwrap_err(); + assert_eq!( + err, + Error::from(InsertTargetError::NoSuchBlueprint( + nonexistent_blueprint_id + )) + ); + + // There should be no current target still. + assert_eq!( + datastore.blueprint_target_get_current(&opctx).await.unwrap(), + None + ); + + // Create three blueprints: + // * `blueprint1` has no parent + // * `blueprint2` and `blueprint3` both have `blueprint1` as parent + let collection = + nexus_inventory::CollectionBuilder::new("test").build(); + let blueprint1 = BlueprintBuilder::build_initial_from_collection( + &collection, + &EMPTY_POLICY, + "test1", + ) + .unwrap(); + let blueprint2 = + BlueprintBuilder::new_based_on(&blueprint1, &EMPTY_POLICY, "test2") + .build(); + let blueprint3 = + BlueprintBuilder::new_based_on(&blueprint1, &EMPTY_POLICY, "test3") + .build(); + assert_eq!(blueprint1.parent_blueprint_id, None); + assert_eq!(blueprint2.parent_blueprint_id, Some(blueprint1.id)); + assert_eq!(blueprint3.parent_blueprint_id, Some(blueprint1.id)); + + // Insert all three into the blueprint table. + datastore.blueprint_insert(&opctx, &blueprint1).await.unwrap(); + datastore.blueprint_insert(&opctx, &blueprint2).await.unwrap(); + datastore.blueprint_insert(&opctx, &blueprint3).await.unwrap(); + + let bp1_target = BlueprintTarget { + target_id: blueprint1.id, + enabled: true, + time_set: now_db_precision(), + }; + let bp2_target = BlueprintTarget { + target_id: blueprint2.id, + enabled: true, + time_set: now_db_precision(), + }; + let bp3_target = BlueprintTarget { + target_id: blueprint3.id, + enabled: true, + time_set: now_db_precision(), + }; + + // Attempting to make blueprint2 the current target should fail because + // it has a non-NULL parent_blueprint_id, but there is no current target + // (i.e., only a blueprint with no parent can be made the current + // target). + let err = datastore + .blueprint_target_set_current(&opctx, bp2_target) + .await + .unwrap_err(); + assert_eq!( + err, + Error::from(InsertTargetError::ParentNotTarget(blueprint2.id)) + ); + + // There should be no current target still. + assert_eq!( + datastore.blueprint_target_get_current(&opctx).await.unwrap(), + None + ); + + // We should be able to insert blueprint1, which has no parent (matching + // the currently-empty `bp_target` table's lack of a target). + datastore + .blueprint_target_set_current(&opctx, bp1_target) + .await + .unwrap(); + assert_eq!( + datastore.blueprint_target_get_current(&opctx).await.unwrap(), + Some((bp1_target, blueprint1.clone())) + ); + + // Now that blueprint1 is the current target, we should be able to + // insert blueprint2 or blueprint3. WLOG, pick blueprint3. + datastore + .blueprint_target_set_current(&opctx, bp3_target) + .await + .unwrap(); + assert_eq!( + datastore.blueprint_target_get_current(&opctx).await.unwrap(), + Some((bp3_target, blueprint3.clone())) + ); + + // Now that blueprint3 is the target, trying to insert blueprint1 or + // blueprint2 should fail, because neither of their parents (NULL and + // blueprint1, respectively) match the current target. + let err = datastore + .blueprint_target_set_current(&opctx, bp1_target) + .await + .unwrap_err(); + assert_eq!( + err, + Error::from(InsertTargetError::ParentNotTarget(blueprint1.id)) + ); + let err = datastore + .blueprint_target_set_current(&opctx, bp2_target) + .await + .unwrap_err(); + assert_eq!( + err, + Error::from(InsertTargetError::ParentNotTarget(blueprint2.id)) + ); + + // Create a child of blueprint3, and ensure when we set it as the target + // with enabled=false, that status is serialized. + let blueprint4 = + BlueprintBuilder::new_based_on(&blueprint3, &EMPTY_POLICY, "test3") + .build(); + assert_eq!(blueprint4.parent_blueprint_id, Some(blueprint3.id)); + datastore.blueprint_insert(&opctx, &blueprint4).await.unwrap(); + let bp4_target = BlueprintTarget { + target_id: blueprint4.id, + enabled: false, + time_set: now_db_precision(), + }; + datastore + .blueprint_target_set_current(&opctx, bp4_target) + .await + .unwrap(); + assert_eq!( + datastore.blueprint_target_get_current(&opctx).await.unwrap(), + Some((bp4_target, blueprint4)) + ); + + // Clean up. + db.cleanup().await.unwrap(); + logctx.cleanup_successful(); + } } diff --git a/nexus/src/app/deployment.rs b/nexus/src/app/deployment.rs index 9439cdc6d5..db6648068e 100644 --- a/nexus/src/app/deployment.rs +++ b/nexus/src/app/deployment.rs @@ -53,19 +53,12 @@ const SQL_LIMIT_INVENTORY: NonZeroU32 = /// the need for this structure. pub struct Blueprints { all_blueprints: BTreeMap, - target: BlueprintTarget, + target: Option, } impl Blueprints { pub fn new() -> Blueprints { - Blueprints { - all_blueprints: BTreeMap::new(), - target: BlueprintTarget { - target_id: None, - enabled: false, - time_set: chrono::Utc::now(), - }, - } + Blueprints { all_blueprints: BTreeMap::new(), target: None } } } @@ -135,7 +128,8 @@ impl super::Nexus { opctx.authorize(Action::Delete, &blueprint).await?; let mut blueprints = self.blueprints.lock().unwrap(); - if let Some(target_id) = blueprints.target.target_id { + if let Some(target_id) = blueprints.target.as_ref().map(|t| t.target_id) + { if target_id == blueprint_id { return Err(Error::conflict(format!( "blueprint {} is the current target and cannot be deleted", @@ -154,8 +148,10 @@ impl super::Nexus { pub async fn blueprint_target_view( &self, opctx: &OpContext, - ) -> Result { - self.blueprint_target(opctx).await.map(|(target, _)| target) + ) -> Result, Error> { + self.blueprint_target(opctx) + .await + .map(|maybe_target| maybe_target.map(|(target, _blueprint)| target)) } // This is a stand-in for a datastore function that fetches the current @@ -165,15 +161,18 @@ impl super::Nexus { async fn blueprint_target( &self, opctx: &OpContext, - ) -> Result<(BlueprintTarget, Option), Error> { + ) -> Result, Error> { opctx.authorize(Action::Read, &authz::BLUEPRINT_CONFIG).await?; let blueprints = self.blueprints.lock().unwrap(); - Ok(( - blueprints.target.clone(), - blueprints.target.target_id.and_then(|target_id| { - blueprints.all_blueprints.get(&target_id).cloned() - }), - )) + let Some(target) = blueprints.target.clone() else { + return Ok(None); + }; + let blueprint = blueprints + .all_blueprints + .get(&target.target_id) + .expect("current target does not exist") + .clone(); + Ok(Some((target, blueprint))) } // Once we store blueprints in the database, this function will likely just @@ -188,7 +187,9 @@ impl super::Nexus { let enabled = params.enabled; let mut blueprints = self.blueprints.lock().unwrap(); if let Some(blueprint) = blueprints.all_blueprints.get(&new_target_id) { - if blueprint.parent_blueprint_id != blueprints.target.target_id { + if blueprint.parent_blueprint_id + != blueprints.target.as_ref().map(|t| t.target_id) + { return Err(Error::conflict(&format!( "blueprint {:?}: parent is {:?}, which is not the current \ target {:?}", @@ -199,20 +200,21 @@ impl super::Nexus { .unwrap_or_else(|| String::from("")), blueprints .target - .target_id - .map(|p| p.to_string()) + .as_ref() + .map(|t| t.target_id.to_string()) .unwrap_or_else(|| String::from("")), ))); } - blueprints.target = BlueprintTarget { - target_id: Some(new_target_id), + let new_target = BlueprintTarget { + target_id: new_target_id, enabled, time_set: chrono::Utc::now(), }; + blueprints.target = Some(new_target.clone()); // When we add a background task executing the target blueprint, // this is the point where we'd signal it to update its target. - Ok(blueprints.target.clone()) + Ok(new_target) } else { Err(Error::not_found_by_id(ResourceType::Blueprint, &new_target_id)) } @@ -337,8 +339,8 @@ impl super::Nexus { &self, opctx: &OpContext, ) -> CreateResult { - let (_, maybe_parent) = self.blueprint_target(opctx).await?; - let Some(parent_blueprint) = maybe_parent else { + let maybe_parent = self.blueprint_target(opctx).await?; + let Some((_, parent_blueprint)) = maybe_parent else { return Err(Error::conflict( "cannot regenerate blueprint without existing target", )); diff --git a/nexus/src/internal_api/http_entrypoints.rs b/nexus/src/internal_api/http_entrypoints.rs index 58038cb37a..e5f7239814 100644 --- a/nexus/src/internal_api/http_entrypoints.rs +++ b/nexus/src/internal_api/http_entrypoints.rs @@ -26,6 +26,7 @@ use dropshot::TypedBody; use hyper::Body; use nexus_db_model::Ipv4NatEntryView; use nexus_types::deployment::Blueprint; +use nexus_types::deployment::BlueprintTarget; use nexus_types::deployment::BlueprintTargetSet; use nexus_types::internal_api::params::SwitchPutRequest; use nexus_types::internal_api::params::SwitchPutResponse; @@ -45,7 +46,6 @@ use oximeter::types::ProducerResults; use oximeter_producer::{collect, ProducerIdPathParams}; use schemars::JsonSchema; use serde::Deserialize; -use serde::Serialize; use std::collections::BTreeMap; use std::sync::Arc; use uuid::Uuid; @@ -680,35 +680,6 @@ async fn blueprint_delete( // Managing the current target blueprint -/// Describes what blueprint, if any, the system is currently working toward -#[derive(Debug, Serialize, JsonSchema)] -pub struct BlueprintTarget { - /// id of the blueprint that the system is trying to make real - pub target_id: Uuid, - /// policy: should the system actively work towards this blueprint - /// - /// This should generally be left enabled. - pub enabled: bool, - /// when this blueprint was made the target - pub time_set: chrono::DateTime, -} - -impl TryFrom for BlueprintTarget { - type Error = Error; - - fn try_from( - value: nexus_types::deployment::BlueprintTarget, - ) -> Result { - Ok(BlueprintTarget { - target_id: value.target_id.ok_or_else(|| { - Error::conflict("no target blueprint has been configured") - })?, - enabled: value.enabled, - time_set: value.time_set, - }) - } -} - /// Fetches the current target blueprint, if any #[endpoint { method = GET, @@ -721,8 +692,11 @@ async fn blueprint_target_view( let handler = async { let opctx = crate::context::op_context_for_internal_api(&rqctx).await; let nexus = &apictx.nexus; - let target = nexus.blueprint_target_view(&opctx).await?; - Ok(HttpResponseOk(BlueprintTarget::try_from(target)?)) + let target = + nexus.blueprint_target_view(&opctx).await?.ok_or_else(|| { + Error::conflict("no target blueprint has been configured") + })?; + Ok(HttpResponseOk(target)) }; apictx.internal_latencies.instrument_dropshot_handler(&rqctx, handler).await } @@ -741,11 +715,8 @@ async fn blueprint_target_set( let opctx = crate::context::op_context_for_internal_api(&rqctx).await; let nexus = &apictx.nexus; let target = target.into_inner(); - let result = nexus.blueprint_target_set(&opctx, target).await?; - Ok(HttpResponseOk( - BlueprintTarget::try_from(result) - .map_err(|e| Error::conflict(e.to_string()))?, - )) + let target = nexus.blueprint_target_set(&opctx, target).await?; + Ok(HttpResponseOk(target)) }; apictx.internal_latencies.instrument_dropshot_handler(&rqctx, handler).await } diff --git a/nexus/types/src/deployment.rs b/nexus/types/src/deployment.rs index 64f14da4a6..c2789d1803 100644 --- a/nexus/types/src/deployment.rs +++ b/nexus/types/src/deployment.rs @@ -187,9 +187,9 @@ impl Blueprint { /// Describes which blueprint the system is currently trying to make real // This is analogous to the db model type until we have that. -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, JsonSchema)] pub struct BlueprintTarget { - pub target_id: Option, + pub target_id: Uuid, pub enabled: bool, pub time_set: chrono::DateTime, } diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index fc68799048..48794bd12b 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -3041,6 +3041,30 @@ CREATE TABLE IF NOT EXISTS omicron.public.blueprint ( comment TEXT NOT NULL ); +-- table describing both the current and historical target blueprints of the +-- system +CREATE TABLE IF NOT EXISTS omicron.public.bp_target ( + -- Monotonically increasing version for all bp_targets + version INT8 PRIMARY KEY, + + -- Effectively a foreign key into the `blueprint` table, but may reference a + -- blueprint that has been deleted (if this target is no longer the current + -- target: the current target must not be deleted). + blueprint_id UUID NOT NULL, + + -- Is this blueprint enabled? + -- + -- Currently, we have no code that acts on this value; however, it exists as + -- an escape hatch once we have automated blueprint planning and execution. + -- An operator can set the current blueprint to disabled, which should stop + -- planning and execution (presumably until a support case can address + -- whatever issue the update system is causing). + enabled BOOL NOT NULL, + + -- Timestamp for when this blueprint was made the current target + time_made_target TIMESTAMPTZ NOT NULL +); + -- generation number for the OmicronZonesConfig for each sled in a blueprint CREATE TABLE IF NOT EXISTS omicron.public.bp_sled_omicron_zones ( -- foreign key into `blueprint` table From d5c3d24bdcf549c644cd4e1d5feb5410c8159079 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Thu, 25 Jan 2024 10:54:29 -0500 Subject: [PATCH 04/24] doc comment fixes --- nexus/types/src/deployment.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/nexus/types/src/deployment.rs b/nexus/types/src/deployment.rs index c2789d1803..bbd802696d 100644 --- a/nexus/types/src/deployment.rs +++ b/nexus/types/src/deployment.rs @@ -185,12 +185,16 @@ impl Blueprint { } } -/// Describes which blueprint the system is currently trying to make real -// This is analogous to the db model type until we have that. +/// Describes what blueprint, if any, the system is currently working toward #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, JsonSchema)] pub struct BlueprintTarget { + /// id of the blueprint that the system is trying to make real pub target_id: Uuid, + /// policy: should the system actively work towards this blueprint + /// + /// This should generally be left enabled. pub enabled: bool, + /// when this blueprint was made the target pub time_set: chrono::DateTime, } From bfe97bb88e1330dddcb916261258502a49e5bd02 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Thu, 25 Jan 2024 11:01:10 -0500 Subject: [PATCH 05/24] sort zones in blueprint builder for testing niceness --- nexus/db-queries/src/db/datastore/deployment.rs | 11 +---------- nexus/deployment/src/blueprint_builder.rs | 14 ++++++++++++-- nexus/src/app/deployment.rs | 4 ++-- 3 files changed, 15 insertions(+), 14 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/deployment.rs b/nexus/db-queries/src/db/datastore/deployment.rs index 9309ca29e0..45beeaf64c 100644 --- a/nexus/db-queries/src/db/datastore/deployment.rs +++ b/nexus/db-queries/src/db/datastore/deployment.rs @@ -1240,16 +1240,7 @@ mod tests { } let num_new_sled_zones = 1 + new_sled_zpools.len(); - let mut blueprint2 = builder.build(); - - // Sort the new sled's omicron zones by ID to match how we fetch them - // from the db. - blueprint2 - .omicron_zones - .get_mut(&new_sled_id) - .unwrap() - .zones - .sort_by_key(|zone| zone.id); + let blueprint2 = builder.build(); // Check that we added the new sled and its zones. assert_eq!( diff --git a/nexus/deployment/src/blueprint_builder.rs b/nexus/deployment/src/blueprint_builder.rs index 2a51080683..ac2fe70e6b 100644 --- a/nexus/deployment/src/blueprint_builder.rs +++ b/nexus/deployment/src/blueprint_builder.rs @@ -95,7 +95,7 @@ impl<'a> BlueprintBuilder<'a> { .sleds .keys() .map(|sled_id| { - let zones = collection + let mut zones = collection .omicron_zones .get(sled_id) .map(|z| z.zones.clone()) @@ -119,6 +119,11 @@ impl<'a> BlueprintBuilder<'a> { sled_id )) })?; + + // This is not strictly necessary. But for testing, it's + // helpful for things to be in sorted order. + zones.zones.sort_by_key(|zone| zone.id); + Ok((*sled_id, zones)) }) .collect::>()?; @@ -163,7 +168,7 @@ impl<'a> BlueprintBuilder<'a> { .map(|sled_id| { // Start with self.omicron_zones, which contains entries for any // sled whose zones config is changing in this blueprint. - let zones = self + let mut zones = self .omicron_zones .remove(sled_id) // If it's not there, use the config from the parent @@ -181,6 +186,11 @@ impl<'a> BlueprintBuilder<'a> { generation: Generation::new(), zones: vec![], }); + + // This is not strictly necessary. But for testing, it's + // helpful for things to be in sorted order. + zones.zones.sort_by_key(|zone| zone.id); + (*sled_id, zones) }) .collect(); diff --git a/nexus/src/app/deployment.rs b/nexus/src/app/deployment.rs index db6648068e..d3a2224b75 100644 --- a/nexus/src/app/deployment.rs +++ b/nexus/src/app/deployment.rs @@ -164,7 +164,7 @@ impl super::Nexus { ) -> Result, Error> { opctx.authorize(Action::Read, &authz::BLUEPRINT_CONFIG).await?; let blueprints = self.blueprints.lock().unwrap(); - let Some(target) = blueprints.target.clone() else { + let Some(target) = blueprints.target else { return Ok(None); }; let blueprint = blueprints @@ -210,7 +210,7 @@ impl super::Nexus { enabled, time_set: chrono::Utc::now(), }; - blueprints.target = Some(new_target.clone()); + blueprints.target = Some(new_target); // When we add a background task executing the target blueprint, // this is the point where we'd signal it to update its target. From eaafb239c88c05e52631c651d7b5082ce5a7ffc5 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Thu, 25 Jan 2024 11:01:49 -0500 Subject: [PATCH 06/24] move schema to rebase onto main --- schema/crdb/{25.0.0 => 28.0.0}/up1.sql | 0 schema/crdb/{25.0.0 => 28.0.0}/up2.sql | 0 schema/crdb/{25.0.0 => 28.0.0}/up3.sql | 0 schema/crdb/{25.0.0 => 28.0.0}/up4.sql | 0 schema/crdb/{25.0.0 => 28.0.0}/up5.sql | 0 5 files changed, 0 insertions(+), 0 deletions(-) rename schema/crdb/{25.0.0 => 28.0.0}/up1.sql (100%) rename schema/crdb/{25.0.0 => 28.0.0}/up2.sql (100%) rename schema/crdb/{25.0.0 => 28.0.0}/up3.sql (100%) rename schema/crdb/{25.0.0 => 28.0.0}/up4.sql (100%) rename schema/crdb/{25.0.0 => 28.0.0}/up5.sql (100%) diff --git a/schema/crdb/25.0.0/up1.sql b/schema/crdb/28.0.0/up1.sql similarity index 100% rename from schema/crdb/25.0.0/up1.sql rename to schema/crdb/28.0.0/up1.sql diff --git a/schema/crdb/25.0.0/up2.sql b/schema/crdb/28.0.0/up2.sql similarity index 100% rename from schema/crdb/25.0.0/up2.sql rename to schema/crdb/28.0.0/up2.sql diff --git a/schema/crdb/25.0.0/up3.sql b/schema/crdb/28.0.0/up3.sql similarity index 100% rename from schema/crdb/25.0.0/up3.sql rename to schema/crdb/28.0.0/up3.sql diff --git a/schema/crdb/25.0.0/up4.sql b/schema/crdb/28.0.0/up4.sql similarity index 100% rename from schema/crdb/25.0.0/up4.sql rename to schema/crdb/28.0.0/up4.sql diff --git a/schema/crdb/25.0.0/up5.sql b/schema/crdb/28.0.0/up5.sql similarity index 100% rename from schema/crdb/25.0.0/up5.sql rename to schema/crdb/28.0.0/up5.sql From 2bf00ff619c8f688ffbe005fc9e8accf19b859fa Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Thu, 25 Jan 2024 13:17:15 -0500 Subject: [PATCH 07/24] replace nexus in-memory blueprints with database --- nexus/db-model/src/deployment.rs | 12 ++ .../db-queries/src/db/datastore/deployment.rs | 126 ++++++++++++++--- nexus/src/app/deployment.rs | 127 +++--------------- nexus/src/app/mod.rs | 5 - nexus/src/internal_api/http_entrypoints.rs | 5 +- nexus/types/src/deployment.rs | 22 +++ openapi/nexus-internal.json | 42 +++++- 7 files changed, 202 insertions(+), 137 deletions(-) diff --git a/nexus/db-model/src/deployment.rs b/nexus/db-model/src/deployment.rs index c23bf9df61..8e06237948 100644 --- a/nexus/db-model/src/deployment.rs +++ b/nexus/db-model/src/deployment.rs @@ -40,6 +40,18 @@ impl From<&'_ nexus_types::deployment::Blueprint> for Blueprint { } } +impl From for nexus_types::deployment::BlueprintMetadata { + fn from(value: Blueprint) -> Self { + Self { + id: value.id, + parent_blueprint_id: value.parent_blueprint_id, + time_created: value.time_created, + creator: value.creator, + comment: value.comment, + } + } +} + /// See [`nexus_types::deployment::BlueprintTarget`]. #[derive(Queryable, Clone, Debug, Selectable, Insertable)] #[diesel(table_name = bp_target)] diff --git a/nexus/db-queries/src/db/datastore/deployment.rs b/nexus/db-queries/src/db/datastore/deployment.rs index 45beeaf64c..95d333cdc8 100644 --- a/nexus/db-queries/src/db/datastore/deployment.rs +++ b/nexus/db-queries/src/db/datastore/deployment.rs @@ -4,6 +4,7 @@ use super::DataStore; use crate::authz; +use crate::authz::ApiResource; use crate::context::OpContext; use crate::db; use crate::db::error::public_error_from_diesel; @@ -37,9 +38,14 @@ use nexus_db_model::BpOmicronZoneNotInService; use nexus_db_model::BpSledOmicronZones; use nexus_db_model::BpTarget; use nexus_types::deployment::Blueprint; +use nexus_types::deployment::BlueprintMetadata; use nexus_types::deployment::BlueprintTarget; use nexus_types::deployment::OmicronZonesConfig; +use omicron_common::api::external::DataPageParams; use omicron_common::api::external::Error; +use omicron_common::api::external::ListResultVec; +use omicron_common::api::external::LookupType; +use omicron_common::api::external::ResourceType; use omicron_common::bail_unless; use std::collections::BTreeMap; use std::collections::BTreeSet; @@ -56,6 +62,27 @@ use uuid::Uuid; const SQL_BATCH_SIZE: NonZeroU32 = unsafe { NonZeroU32::new_unchecked(1000) }; impl DataStore { + /// List blueprints + pub async fn blueprints_list( + &self, + opctx: &OpContext, + pagparams: &DataPageParams<'_, Uuid>, + ) -> ListResultVec { + use db::schema::blueprint; + + opctx + .authorize(authz::Action::ListChildren, &authz::BLUEPRINT_CONFIG) + .await?; + + let blueprints = paginated(blueprint::table, blueprint::id, pagparams) + .select(DbBlueprint::as_select()) + .get_results_async(&*self.pool_connection_authorized(opctx).await?) + .await + .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; + + Ok(blueprints.into_iter().map(BlueprintMetadata::from).collect()) + } + /// Store a complete blueprint into the database pub async fn blueprint_insert( &self, @@ -198,27 +225,35 @@ impl DataStore { pub async fn blueprint_read( &self, opctx: &OpContext, - blueprint_id: Uuid, + authz_blueprint: &authz::Blueprint, ) -> Result { - opctx.authorize(authz::Action::Read, &authz::BLUEPRINT_CONFIG).await?; + opctx.authorize(authz::Action::Read, authz_blueprint).await?; let conn = self.pool_connection_authorized(opctx).await?; + let blueprint_id = authz_blueprint.id(); // Read the metadata from the primary blueprint row, and ensure that it // exists. let (parent_blueprint_id, time_created, creator, comment) = { use db::schema::blueprint::dsl; - let blueprints = dsl::blueprint + let mut blueprints = dsl::blueprint .filter(dsl::id.eq(blueprint_id)) .limit(2) .select(DbBlueprint::as_select()) .load_async(&*conn) .await - .map_err(|e| { - public_error_from_diesel(e, ErrorHandler::Server) - })?; - bail_unless!(blueprints.len() == 1); - let blueprint = blueprints.into_iter().next().unwrap(); + .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))? + .into_iter(); + + // Check that this blueprint exists. + let Some(blueprint) = blueprints.next() else { + return Err(authz_blueprint.not_found()); + }; + + // Ensure we don't have another blueprint with this ID (should be + // impossible given we filtered on an exact primary key) + bail_unless!(blueprints.next().is_none()); + ( blueprint.parent_blueprint_id, blueprint.time_created, @@ -461,9 +496,10 @@ impl DataStore { pub async fn blueprint_delete( &self, opctx: &OpContext, - blueprint_id: Uuid, + authz_blueprint: &authz::Blueprint, ) -> Result<(), Error> { - opctx.authorize(authz::Action::Modify, &authz::INVENTORY).await?; + opctx.authorize(authz::Action::Delete, authz_blueprint).await?; + let blueprint_id = authz_blueprint.id(); // As with inserting a whole blueprint, we remove it in one big // transaction. Similar considerations apply. We could @@ -506,6 +542,15 @@ impl DataStore { .await? }; + // Bail out if this blueprint didn't exist; there won't be + // references to it in any of the remaining tables either, since + // deletion always goes through this transaction. + if nblueprints == 0 { + return Err(TransactionError::CustomError( + authz_blueprint.not_found(), + )); + } + // Remove rows associated with Omicron zones let nsled_agent_zones = { use db::schema::bp_sled_omicron_zones::dsl; @@ -625,7 +670,8 @@ impl DataStore { // should be rare in practice, and is always possible during // `blueprint_read` anyway since it has to read from multiple tables to // construct the blueprint. - let blueprint = self.blueprint_read(opctx, target.target_id).await?; + let authz_blueprint = authz_blueprint_from_id(target.target_id); + let blueprint = self.blueprint_read(opctx, &authz_blueprint).await?; Ok(Some((target, blueprint))) } @@ -651,6 +697,15 @@ impl DataStore { } } +// Helper to create an `authz::Blueprint` for a specific blueprint ID +fn authz_blueprint_from_id(blueprint_id: Uuid) -> authz::Blueprint { + authz::Blueprint::new( + authz::FLEET, + blueprint_id, + LookupType::ById(blueprint_id), + ) +} + /// Errors related to inserting a target blueprint #[derive(Debug)] enum InsertTargetError { @@ -667,7 +722,7 @@ impl From for Error { fn from(value: InsertTargetError) -> Self { match value { InsertTargetError::NoSuchBlueprint(id) => { - Error::invalid_request(format!("Blueprint {id} does not exist")) + Error::not_found_by_id(ResourceType::Blueprint, &id) } InsertTargetError::ParentNotTarget(id) => { Error::invalid_request(format!( @@ -1108,6 +1163,19 @@ mod tests { (collection, policy, blueprint) } + async fn blueprint_list_all_ids( + opctx: &OpContext, + datastore: &DataStore, + ) -> Vec { + datastore + .blueprints_list(opctx, &DataPageParams::max_page()) + .await + .unwrap() + .into_iter() + .map(|bp| bp.id) + .collect() + } + #[tokio::test] async fn test_empty_blueprint() { // Setup @@ -1124,6 +1192,7 @@ mod tests { "test", ) .unwrap(); + let authz_blueprint = authz_blueprint_from_id(blueprint1.id); // Write it to the database and read it back. datastore @@ -1131,10 +1200,14 @@ mod tests { .await .expect("failed to insert blueprint"); let blueprint_read = datastore - .blueprint_read(&opctx, blueprint1.id) + .blueprint_read(&opctx, &authz_blueprint) .await .expect("failed to read collection back"); assert_eq!(blueprint1, blueprint_read); + assert_eq!( + blueprint_list_all_ids(&opctx, &datastore).await, + [blueprint1.id] + ); // There ought to be no sleds or zones in service, and no parent // blueprint. @@ -1143,8 +1216,9 @@ mod tests { assert_eq!(blueprint1.parent_blueprint_id, None); // Delete the blueprint and ensure it's really gone. - datastore.blueprint_delete(&opctx, blueprint1.id).await.unwrap(); + datastore.blueprint_delete(&opctx, &authz_blueprint).await.unwrap(); ensure_blueprint_fully_deleted(&datastore, blueprint1.id).await; + assert_eq!(blueprint_list_all_ids(&opctx, &datastore).await, []); // Clean up. db.cleanup().await.unwrap(); @@ -1160,6 +1234,7 @@ mod tests { // Create a cohesive representative collection/policy/blueprint let (collection, mut policy, blueprint1) = representative(); + let authz_blueprint1 = authz_blueprint_from_id(blueprint1.id); // Write it to the database and read it back. datastore @@ -1167,10 +1242,14 @@ mod tests { .await .expect("failed to insert blueprint"); let blueprint_read = datastore - .blueprint_read(&opctx, blueprint1.id) + .blueprint_read(&opctx, &authz_blueprint1) .await .expect("failed to read collection back"); assert_eq!(blueprint1, blueprint_read); + assert_eq!( + blueprint_list_all_ids(&opctx, &datastore).await, + [blueprint1.id] + ); // Check the number of blueprint elements against our collection. assert_eq!(blueprint1.omicron_zones.len(), policy.sleds.len()); @@ -1205,7 +1284,7 @@ mod tests { Some((bp1_target, blueprint1.clone())) ); let err = datastore - .blueprint_delete(&opctx, blueprint1.id) + .blueprint_delete(&opctx, &authz_blueprint1) .await .unwrap_err(); assert!( @@ -1241,6 +1320,7 @@ mod tests { let num_new_sled_zones = 1 + new_sled_zpools.len(); let blueprint2 = builder.build(); + let authz_blueprint2 = authz_blueprint_from_id(blueprint2.id); // Check that we added the new sled and its zones. assert_eq!( @@ -1265,11 +1345,15 @@ mod tests { .await .expect("failed to insert blueprint"); let blueprint_read = datastore - .blueprint_read(&opctx, blueprint2.id) + .blueprint_read(&opctx, &authz_blueprint2) .await .expect("failed to read collection back"); println!("diff: {}", blueprint2.diff(&blueprint_read)); assert_eq!(blueprint2, blueprint_read); + assert_eq!( + blueprint_list_all_ids(&opctx, &datastore).await, + [blueprint1.id, blueprint2.id] + ); // Set blueprint2 as the current target and ensure that means we can not // delete it. @@ -1287,7 +1371,7 @@ mod tests { Some((bp2_target, blueprint2.clone())) ); let err = datastore - .blueprint_delete(&opctx, blueprint2.id) + .blueprint_delete(&opctx, &authz_blueprint2) .await .unwrap_err(); assert!( @@ -1300,8 +1384,12 @@ mod tests { // Now that blueprint2 is the target, we should be able to delete // blueprint1. - datastore.blueprint_delete(&opctx, blueprint1.id).await.unwrap(); + datastore.blueprint_delete(&opctx, &authz_blueprint1).await.unwrap(); ensure_blueprint_fully_deleted(&datastore, blueprint1.id).await; + assert_eq!( + blueprint_list_all_ids(&opctx, &datastore).await, + [blueprint2.id] + ); // Clean up. db.cleanup().await.unwrap(); diff --git a/nexus/src/app/deployment.rs b/nexus/src/app/deployment.rs index d3a2224b75..f56e4e3fc7 100644 --- a/nexus/src/app/deployment.rs +++ b/nexus/src/app/deployment.rs @@ -6,12 +6,12 @@ use nexus_db_queries::authz; use nexus_db_queries::authz::Action; -use nexus_db_queries::authz::ApiResource; use nexus_db_queries::context::OpContext; use nexus_db_queries::db::pagination::Paginator; use nexus_deployment::blueprint_builder::BlueprintBuilder; use nexus_deployment::planner::Planner; use nexus_types::deployment::Blueprint; +use nexus_types::deployment::BlueprintMetadata; use nexus_types::deployment::BlueprintTarget; use nexus_types::deployment::BlueprintTargetSet; use nexus_types::deployment::Policy; @@ -27,7 +27,6 @@ use omicron_common::api::external::Error; use omicron_common::api::external::ListResultVec; use omicron_common::api::external::LookupResult; use omicron_common::api::external::LookupType; -use omicron_common::api::external::ResourceType; use slog_error_chain::InlineErrorChain; use std::collections::BTreeMap; use std::collections::BTreeSet; @@ -47,21 +46,6 @@ const SQL_BATCH_SIZE: NonZeroU32 = unsafe { NonZeroU32::new_unchecked(1000) }; const SQL_LIMIT_INVENTORY: NonZeroU32 = unsafe { NonZeroU32::new_unchecked(1000) }; -/// Temporary in-memory store of blueprints -/// -/// Blueprints eventually need to be stored in the database. That will obviate -/// the need for this structure. -pub struct Blueprints { - all_blueprints: BTreeMap, - target: Option, -} - -impl Blueprints { - pub fn new() -> Blueprints { - Blueprints { all_blueprints: BTreeMap::new(), target: None } - } -} - /// Common structure for collecting information that the planner needs struct PlanningContext { policy: Policy, @@ -75,20 +59,9 @@ impl super::Nexus { &self, opctx: &OpContext, pagparams: &DataPageParams<'_, Uuid>, - ) -> ListResultVec { + ) -> ListResultVec { opctx.authorize(Action::ListChildren, &authz::BLUEPRINT_CONFIG).await?; - Ok(self - .blueprints - .lock() - .unwrap() - .all_blueprints - .values() - .filter_map(|f| match pagparams.marker { - None => Some(f.clone()), - Some(marker) if f.id > *marker => Some(f.clone()), - _ => None, - }) - .collect()) + self.db_datastore.blueprints_list(opctx, pagparams).await } // Once we store blueprints in the database, this function will likely just @@ -104,13 +77,7 @@ impl super::Nexus { LookupType::ById(blueprint_id), ); opctx.authorize(Action::Read, &blueprint).await?; - self.blueprints - .lock() - .unwrap() - .all_blueprints - .get(&blueprint_id) - .cloned() - .ok_or_else(|| blueprint.not_found()) + self.db_datastore.blueprint_read(opctx, &blueprint).await } // Once we store blueprints in the database, this function will likely just @@ -126,23 +93,7 @@ impl super::Nexus { LookupType::ById(blueprint_id), ); opctx.authorize(Action::Delete, &blueprint).await?; - - let mut blueprints = self.blueprints.lock().unwrap(); - if let Some(target_id) = blueprints.target.as_ref().map(|t| t.target_id) - { - if target_id == blueprint_id { - return Err(Error::conflict(format!( - "blueprint {} is the current target and cannot be deleted", - blueprint_id - ))); - } - } - - if blueprints.all_blueprints.remove(&blueprint_id).is_none() { - return Err(blueprint.not_found()); - } - - Ok(()) + self.db_datastore.blueprint_delete(opctx, &blueprint).await } pub async fn blueprint_target_view( @@ -163,16 +114,7 @@ impl super::Nexus { opctx: &OpContext, ) -> Result, Error> { opctx.authorize(Action::Read, &authz::BLUEPRINT_CONFIG).await?; - let blueprints = self.blueprints.lock().unwrap(); - let Some(target) = blueprints.target else { - return Ok(None); - }; - let blueprint = blueprints - .all_blueprints - .get(&target.target_id) - .expect("current target does not exist") - .clone(); - Ok(Some((target, blueprint))) + self.db_datastore.blueprint_target_get_current(opctx).await } // Once we store blueprints in the database, this function will likely just @@ -183,41 +125,15 @@ impl super::Nexus { params: BlueprintTargetSet, ) -> Result { opctx.authorize(Action::Modify, &authz::BLUEPRINT_CONFIG).await?; - let new_target_id = params.target_id; - let enabled = params.enabled; - let mut blueprints = self.blueprints.lock().unwrap(); - if let Some(blueprint) = blueprints.all_blueprints.get(&new_target_id) { - if blueprint.parent_blueprint_id - != blueprints.target.as_ref().map(|t| t.target_id) - { - return Err(Error::conflict(&format!( - "blueprint {:?}: parent is {:?}, which is not the current \ - target {:?}", - new_target_id, - blueprint - .parent_blueprint_id - .map(|p| p.to_string()) - .unwrap_or_else(|| String::from("")), - blueprints - .target - .as_ref() - .map(|t| t.target_id.to_string()) - .unwrap_or_else(|| String::from("")), - ))); - } - let new_target = BlueprintTarget { - target_id: new_target_id, - enabled, - time_set: chrono::Utc::now(), - }; - blueprints.target = Some(new_target); - - // When we add a background task executing the target blueprint, - // this is the point where we'd signal it to update its target. - Ok(new_target) - } else { - Err(Error::not_found_by_id(ResourceType::Blueprint, &new_target_id)) - } + let new_target = BlueprintTarget { + target_id: params.target_id, + enabled: params.enabled, + time_set: chrono::Utc::now(), + }; + self.db_datastore + .blueprint_target_set_current(opctx, new_target) + .await?; + Ok(new_target) } async fn blueprint_planning_context( @@ -293,15 +209,10 @@ impl super::Nexus { async fn blueprint_add( &self, opctx: &OpContext, - blueprint: Blueprint, + blueprint: &Blueprint, ) -> Result<(), Error> { opctx.authorize(Action::Modify, &authz::BLUEPRINT_CONFIG).await?; - let mut blueprints = self.blueprints.lock().unwrap(); - assert!(blueprints - .all_blueprints - .insert(blueprint.id, blueprint) - .is_none()); - Ok(()) + self.db_datastore.blueprint_insert(opctx, blueprint).await } pub async fn blueprint_generate_from_collection( @@ -331,7 +242,7 @@ impl super::Nexus { )) })?; - self.blueprint_add(&opctx, blueprint.clone()).await?; + self.blueprint_add(&opctx, &blueprint).await?; Ok(blueprint) } @@ -360,7 +271,7 @@ impl super::Nexus { )) })?; - self.blueprint_add(&opctx, blueprint.clone()).await?; + self.blueprint_add(&opctx, &blueprint).await?; Ok(blueprint) } } diff --git a/nexus/src/app/mod.rs b/nexus/src/app/mod.rs index d6ad7c98ea..bf8522452a 100644 --- a/nexus/src/app/mod.rs +++ b/nexus/src/app/mod.rs @@ -183,10 +183,6 @@ pub struct Nexus { /// Default Crucible region allocation strategy default_region_allocation_strategy: RegionAllocationStrategy, - - /// information about blueprints (deployment configurations) - // This will go away once these are stored in the database. - blueprints: std::sync::Mutex, } impl Nexus { @@ -419,7 +415,6 @@ impl Nexus { .pkg .default_region_allocation_strategy .clone(), - blueprints: std::sync::Mutex::new(deployment::Blueprints::new()), }; // TODO-cleanup all the extra Arcs here seems wrong diff --git a/nexus/src/internal_api/http_entrypoints.rs b/nexus/src/internal_api/http_entrypoints.rs index e5f7239814..0122d9b439 100644 --- a/nexus/src/internal_api/http_entrypoints.rs +++ b/nexus/src/internal_api/http_entrypoints.rs @@ -26,6 +26,7 @@ use dropshot::TypedBody; use hyper::Body; use nexus_db_model::Ipv4NatEntryView; use nexus_types::deployment::Blueprint; +use nexus_types::deployment::BlueprintMetadata; use nexus_types::deployment::BlueprintTarget; use nexus_types::deployment::BlueprintTargetSet; use nexus_types::internal_api::params::SwitchPutRequest; @@ -620,7 +621,7 @@ async fn ipv4_nat_changeset( async fn blueprint_list( rqctx: RequestContext>, query_params: Query, -) -> Result>, HttpError> { +) -> Result>, HttpError> { let apictx = rqctx.context(); let handler = async { let nexus = &apictx.nexus; @@ -631,7 +632,7 @@ async fn blueprint_list( Ok(HttpResponseOk(ScanById::results_page( &query, blueprints, - &|_, blueprint: &Blueprint| blueprint.id, + &|_, blueprint: &BlueprintMetadata| blueprint.id, )?)) }; diff --git a/nexus/types/src/deployment.rs b/nexus/types/src/deployment.rs index bbd802696d..c0cf1715df 100644 --- a/nexus/types/src/deployment.rs +++ b/nexus/types/src/deployment.rs @@ -185,6 +185,28 @@ impl Blueprint { } } +/// Describe high-level metadata about a blueprint +// These fields are a subset of [`Blueprint`], and include only the data we can +// quickly fetch from the main blueprint table (e.g., when listing all +// blueprints). +#[derive(Debug, Clone, Eq, PartialEq, JsonSchema, Serialize)] +pub struct BlueprintMetadata { + /// unique identifier for this blueprint + pub id: Uuid, + + /// which blueprint this blueprint is based on + pub parent_blueprint_id: Option, + + /// when this blueprint was generated (for debugging) + pub time_created: chrono::DateTime, + /// identity of the component that generated the blueprint (for debugging) + /// This would generally be the Uuid of a Nexus instance. + pub creator: String, + /// human-readable string describing why this blueprint was created + /// (for debugging) + pub comment: String, +} + /// Describes what blueprint, if any, the system is currently working toward #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, JsonSchema)] pub struct BlueprintTarget { diff --git a/openapi/nexus-internal.json b/openapi/nexus-internal.json index 8b0807d52c..314ff1eb15 100644 --- a/openapi/nexus-internal.json +++ b/openapi/nexus-internal.json @@ -164,7 +164,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/BlueprintResultsPage" + "$ref": "#/components/schemas/BlueprintMetadataResultsPage" } } } @@ -2132,7 +2132,43 @@ "zones_in_service" ] }, - "BlueprintResultsPage": { + "BlueprintMetadata": { + "description": "Describe high-level metadata about a blueprint", + "type": "object", + "properties": { + "comment": { + "description": "human-readable string describing why this blueprint was created (for debugging)", + "type": "string" + }, + "creator": { + "description": "identity of the component that generated the blueprint (for debugging) This would generally be the Uuid of a Nexus instance.", + "type": "string" + }, + "id": { + "description": "unique identifier for this blueprint", + "type": "string", + "format": "uuid" + }, + "parent_blueprint_id": { + "nullable": true, + "description": "which blueprint this blueprint is based on", + "type": "string", + "format": "uuid" + }, + "time_created": { + "description": "when this blueprint was generated (for debugging)", + "type": "string", + "format": "date-time" + } + }, + "required": [ + "comment", + "creator", + "id", + "time_created" + ] + }, + "BlueprintMetadataResultsPage": { "description": "A single page of results", "type": "object", "properties": { @@ -2140,7 +2176,7 @@ "description": "list of items on this page of results", "type": "array", "items": { - "$ref": "#/components/schemas/Blueprint" + "$ref": "#/components/schemas/BlueprintMetadata" } }, "next_page": { From db84a5dd235d87f359b34c17faa86d1f0f599dd2 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Thu, 25 Jan 2024 14:21:01 -0500 Subject: [PATCH 08/24] remove duplicate authz checks These were performed by nexus when blueprints were in memory, but are now performed by the datastore --- nexus/src/app/deployment.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/nexus/src/app/deployment.rs b/nexus/src/app/deployment.rs index f56e4e3fc7..b4a7e9227f 100644 --- a/nexus/src/app/deployment.rs +++ b/nexus/src/app/deployment.rs @@ -60,7 +60,6 @@ impl super::Nexus { opctx: &OpContext, pagparams: &DataPageParams<'_, Uuid>, ) -> ListResultVec { - opctx.authorize(Action::ListChildren, &authz::BLUEPRINT_CONFIG).await?; self.db_datastore.blueprints_list(opctx, pagparams).await } @@ -76,7 +75,6 @@ impl super::Nexus { blueprint_id, LookupType::ById(blueprint_id), ); - opctx.authorize(Action::Read, &blueprint).await?; self.db_datastore.blueprint_read(opctx, &blueprint).await } @@ -92,7 +90,6 @@ impl super::Nexus { blueprint_id, LookupType::ById(blueprint_id), ); - opctx.authorize(Action::Delete, &blueprint).await?; self.db_datastore.blueprint_delete(opctx, &blueprint).await } @@ -113,7 +110,6 @@ impl super::Nexus { &self, opctx: &OpContext, ) -> Result, Error> { - opctx.authorize(Action::Read, &authz::BLUEPRINT_CONFIG).await?; self.db_datastore.blueprint_target_get_current(opctx).await } @@ -124,7 +120,6 @@ impl super::Nexus { opctx: &OpContext, params: BlueprintTargetSet, ) -> Result { - opctx.authorize(Action::Modify, &authz::BLUEPRINT_CONFIG).await?; let new_target = BlueprintTarget { target_id: params.target_id, enabled: params.enabled, @@ -211,7 +206,6 @@ impl super::Nexus { opctx: &OpContext, blueprint: &Blueprint, ) -> Result<(), Error> { - opctx.authorize(Action::Modify, &authz::BLUEPRINT_CONFIG).await?; self.db_datastore.blueprint_insert(opctx, blueprint).await } From c5e471844781205359e3acbabc308f80068d09da Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Thu, 25 Jan 2024 14:54:18 -0500 Subject: [PATCH 09/24] minor cleanup, mostly comments --- .../db-queries/src/db/datastore/deployment.rs | 43 ++++++++++++------- nexus/src/app/deployment.rs | 12 +++--- schema/crdb/dbinit.sql | 37 +++++++++++++++- 3 files changed, 68 insertions(+), 24 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/deployment.rs b/nexus/db-queries/src/db/datastore/deployment.rs index 95d333cdc8..f6cc1d5049 100644 --- a/nexus/db-queries/src/db/datastore/deployment.rs +++ b/nexus/db-queries/src/db/datastore/deployment.rs @@ -395,16 +395,14 @@ impl DataStore { let mut paginator = Paginator::new(SQL_BATCH_SIZE); while let Some(p) = paginator.next() { + // `paginated` implicitly orders by our `id`, which is also + // handy for testing: the zones are always consistently ordered let batch = paginated( dsl::bp_omicron_zone, dsl::id, &p.current_pagparams(), ) .filter(dsl::blueprint_id.eq(blueprint_id)) - // It's not strictly necessary to order these by id. Doing so - // ensures a consistent representation for `Blueprint`, which - // makes testing easier. It's already indexed to do this, too. - .order_by(dsl::id) .select(BpOmicronZone::as_select()) .load_async(&*conn) .await @@ -650,7 +648,11 @@ impl DataStore { } /// Get the current target blueprint, if one exists - pub async fn blueprint_target_get_current( + /// + /// Returns both the metadata about the target and the full blueprint + /// contents. If you only need the target metadata, use + /// `blueprint_target_get_current` instead. + pub async fn blueprint_target_get_current_full( &self, opctx: &OpContext, ) -> Result, Error> { @@ -667,15 +669,26 @@ impl DataStore { // blueprint the target and (b) deleted the blueprint pointed to by our // `target` between the above query and the below query. In such a case, // this query will fail with an "unknown blueprint ID" error. This - // should be rare in practice, and is always possible during - // `blueprint_read` anyway since it has to read from multiple tables to - // construct the blueprint. + // should be rare in practice. + // + // TODO-correctness Should we loop here if we get a "blueprint not + // found" error to catch this race? let authz_blueprint = authz_blueprint_from_id(target.target_id); let blueprint = self.blueprint_read(opctx, &authz_blueprint).await?; Ok(Some((target, blueprint))) } + /// Get the current target blueprint, if one exists + pub async fn blueprint_target_get_current( + &self, + opctx: &OpContext, + ) -> Result, Error> { + opctx.authorize(authz::Action::Read, &authz::BLUEPRINT_CONFIG).await?; + let conn = self.pool_connection_authorized(opctx).await?; + self.blueprint_current_target_only(&conn).await + } + // Helper to fetch the current blueprint target (without fetching the entire // blueprint for that target). // @@ -1280,7 +1293,7 @@ mod tests { .await .unwrap(); assert_eq!( - datastore.blueprint_target_get_current(&opctx).await.unwrap(), + datastore.blueprint_target_get_current_full(&opctx).await.unwrap(), Some((bp1_target, blueprint1.clone())) ); let err = datastore @@ -1367,7 +1380,7 @@ mod tests { .await .unwrap(); assert_eq!( - datastore.blueprint_target_get_current(&opctx).await.unwrap(), + datastore.blueprint_target_get_current_full(&opctx).await.unwrap(), Some((bp2_target, blueprint2.clone())) ); let err = datastore @@ -1426,7 +1439,7 @@ mod tests { // There should be no current target still. assert_eq!( - datastore.blueprint_target_get_current(&opctx).await.unwrap(), + datastore.blueprint_target_get_current_full(&opctx).await.unwrap(), None ); @@ -1487,7 +1500,7 @@ mod tests { // There should be no current target still. assert_eq!( - datastore.blueprint_target_get_current(&opctx).await.unwrap(), + datastore.blueprint_target_get_current_full(&opctx).await.unwrap(), None ); @@ -1498,7 +1511,7 @@ mod tests { .await .unwrap(); assert_eq!( - datastore.blueprint_target_get_current(&opctx).await.unwrap(), + datastore.blueprint_target_get_current_full(&opctx).await.unwrap(), Some((bp1_target, blueprint1.clone())) ); @@ -1509,7 +1522,7 @@ mod tests { .await .unwrap(); assert_eq!( - datastore.blueprint_target_get_current(&opctx).await.unwrap(), + datastore.blueprint_target_get_current_full(&opctx).await.unwrap(), Some((bp3_target, blueprint3.clone())) ); @@ -1550,7 +1563,7 @@ mod tests { .await .unwrap(); assert_eq!( - datastore.blueprint_target_get_current(&opctx).await.unwrap(), + datastore.blueprint_target_get_current_full(&opctx).await.unwrap(), Some((bp4_target, blueprint4)) ); diff --git a/nexus/src/app/deployment.rs b/nexus/src/app/deployment.rs index b4a7e9227f..55c95a6534 100644 --- a/nexus/src/app/deployment.rs +++ b/nexus/src/app/deployment.rs @@ -5,7 +5,6 @@ //! Configuration of the deployment system use nexus_db_queries::authz; -use nexus_db_queries::authz::Action; use nexus_db_queries::context::OpContext; use nexus_db_queries::db::pagination::Paginator; use nexus_deployment::blueprint_builder::BlueprintBuilder; @@ -97,9 +96,7 @@ impl super::Nexus { &self, opctx: &OpContext, ) -> Result, Error> { - self.blueprint_target(opctx) - .await - .map(|maybe_target| maybe_target.map(|(target, _blueprint)| target)) + self.db_datastore.blueprint_target_get_current(opctx).await } // This is a stand-in for a datastore function that fetches the current @@ -110,7 +107,7 @@ impl super::Nexus { &self, opctx: &OpContext, ) -> Result, Error> { - self.db_datastore.blueprint_target_get_current(opctx).await + self.db_datastore.blueprint_target_get_current_full(opctx).await } // Once we store blueprints in the database, this function will likely just @@ -244,8 +241,9 @@ impl super::Nexus { &self, opctx: &OpContext, ) -> CreateResult { - let maybe_parent = self.blueprint_target(opctx).await?; - let Some((_, parent_blueprint)) = maybe_parent else { + let maybe_target = + self.db_datastore.blueprint_target_get_current_full(opctx).await?; + let Some((_, parent_blueprint)) = maybe_target else { return Err(Error::conflict( "cannot regenerate blueprint without existing target", )); diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index 48794bd12b..7b895c82c1 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -3017,9 +3017,42 @@ CREATE TABLE IF NOT EXISTS omicron.public.inv_omicron_zone_nic ( ); /* - * System policy and blueprints + * System-level blueprints * - * TODO docs + * See RFD 457 and 459 for context. + * + * A blueprint describes a potential system configuration. The primary table is + * the `blueprint` table, which stores only a small amount of metadata about the + * blueprint. The bulk of the information is stored in the `bp_*` tables below, + * each of which references back to `blueprint` by ID. + * + * `bp_target` describes the "target blueprints" of the system. Insertion must + * follow a strict set of rules: + * + * * The first target blueprint must have version=1, and must have no parent + * blueprint. + * * The Nth target blueprint must have version=N, and its parent blueprint must + * be the blueprint that was the target at version=N-1. + * + * The result is that the current target blueprint can always be found by + * looking at the maximally-versioned row in `bp_target`, and there is a linear + * history from that blueprint all the way back to the version=1 blueprint. We + * will eventually prune old blueprint targets, so it will not always be + * possible to view the entire history. + * + * `bp_sled_omicron_zones`, `bp_omicron_zone`, and `bp_omicron_zone_nic` are + * nearly identical to their `inv_*` counterparts, and record the + * `OmicronZonesConfig` for each sled. + * + * `bp_omicron_zones_not_in_service` stores a list of Omicron zones (present in + * `bp_omicron_zone` that are NOT in service; e.g., should not appear in + * internal DNS. Nexus's in-memory `Blueprint` representation stores the set of + * zones that ARE in service. We invert that logic at this layer because we + * expect most blueprints to have a relatively large number of omicron zones, + * almost all of which will be in service. This is a minor and perhaps + * unnecessary optimization at the database layer, but it's also relatively + * simple and hidden by the relevant read and insert queries in + * `nexus-db-queries`. */ -- list of all blueprints From 0681a8d0682b73393cc3d7fb2f7e756241df76e6 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Thu, 25 Jan 2024 14:59:51 -0500 Subject: [PATCH 10/24] remove unused method --- nexus/src/app/deployment.rs | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/nexus/src/app/deployment.rs b/nexus/src/app/deployment.rs index 55c95a6534..d887625458 100644 --- a/nexus/src/app/deployment.rs +++ b/nexus/src/app/deployment.rs @@ -99,17 +99,6 @@ impl super::Nexus { self.db_datastore.blueprint_target_get_current(opctx).await } - // This is a stand-in for a datastore function that fetches the current - // target information and the target blueprint's contents. This helper - // exists to combine the authz check with the lookup, which is what the - // datastore function will eventually do. - async fn blueprint_target( - &self, - opctx: &OpContext, - ) -> Result, Error> { - self.db_datastore.blueprint_target_get_current_full(opctx).await - } - // Once we store blueprints in the database, this function will likely just // delegate to a corresponding datastore function. pub async fn blueprint_target_set( From e9d4bb1ff99ab1a63562003d5069e04820495602 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Thu, 25 Jan 2024 15:04:30 -0500 Subject: [PATCH 11/24] fix nondeterministic test --- nexus/db-queries/src/db/datastore/deployment.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/deployment.rs b/nexus/db-queries/src/db/datastore/deployment.rs index f6cc1d5049..10f0531e6f 100644 --- a/nexus/db-queries/src/db/datastore/deployment.rs +++ b/nexus/db-queries/src/db/datastore/deployment.rs @@ -1363,10 +1363,14 @@ mod tests { .expect("failed to read collection back"); println!("diff: {}", blueprint2.diff(&blueprint_read)); assert_eq!(blueprint2, blueprint_read); - assert_eq!( - blueprint_list_all_ids(&opctx, &datastore).await, - [blueprint1.id, blueprint2.id] - ); + { + let mut expected_ids = [blueprint1.id, blueprint2.id]; + expected_ids.sort(); + assert_eq!( + blueprint_list_all_ids(&opctx, &datastore).await, + expected_ids + ); + } // Set blueprint2 as the current target and ensure that means we can not // delete it. From cfe797a649b24d218b1f60614a9686e11fcc0db9 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Thu, 25 Jan 2024 16:10:30 -0500 Subject: [PATCH 12/24] add missing migration file --- schema/crdb/28.0.0/up6.sql | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 schema/crdb/28.0.0/up6.sql diff --git a/schema/crdb/28.0.0/up6.sql b/schema/crdb/28.0.0/up6.sql new file mode 100644 index 0000000000..41e69ca3da --- /dev/null +++ b/schema/crdb/28.0.0/up6.sql @@ -0,0 +1,6 @@ +CREATE TABLE IF NOT EXISTS omicron.public.bp_target ( + version INT8 PRIMARY KEY, + blueprint_id UUID NOT NULL, + enabled BOOL NOT NULL, + time_made_target TIMESTAMPTZ NOT NULL +); From afc72437090b8a770155c7e4cca98ae7c27195de Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Fri, 26 Jan 2024 11:29:20 -0500 Subject: [PATCH 13/24] remove "one we store blueprints in the db" comments --- nexus/src/app/deployment.rs | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/nexus/src/app/deployment.rs b/nexus/src/app/deployment.rs index d887625458..29eacb8697 100644 --- a/nexus/src/app/deployment.rs +++ b/nexus/src/app/deployment.rs @@ -52,8 +52,6 @@ struct PlanningContext { } impl super::Nexus { - // Once we store blueprints in the database, this function will likely just - // delegate to a corresponding datastore function. pub async fn blueprint_list( &self, opctx: &OpContext, @@ -62,8 +60,6 @@ impl super::Nexus { self.db_datastore.blueprints_list(opctx, pagparams).await } - // Once we store blueprints in the database, this function will likely just - // delegate to a corresponding datastore function. pub async fn blueprint_view( &self, opctx: &OpContext, @@ -77,8 +73,6 @@ impl super::Nexus { self.db_datastore.blueprint_read(opctx, &blueprint).await } - // Once we store blueprints in the database, this function will likely just - // delegate to a corresponding datastore function. pub async fn blueprint_delete( &self, opctx: &OpContext, @@ -99,8 +93,6 @@ impl super::Nexus { self.db_datastore.blueprint_target_get_current(opctx).await } - // Once we store blueprints in the database, this function will likely just - // delegate to a corresponding datastore function. pub async fn blueprint_target_set( &self, opctx: &OpContext, @@ -185,8 +177,6 @@ impl super::Nexus { Ok(PlanningContext { creator, policy: Policy { sleds } }) } - // Once we store blueprints in the database, this function will likely just - // delegate to a corresponding datastore function. async fn blueprint_add( &self, opctx: &OpContext, From dccd0d2c13859f0416a5de5aa4fd913dc6386aba Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Fri, 26 Jan 2024 11:30:22 -0500 Subject: [PATCH 14/24] restore placeholder comment for blueprint execution --- nexus/src/app/deployment.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/nexus/src/app/deployment.rs b/nexus/src/app/deployment.rs index 29eacb8697..ae695f1ff4 100644 --- a/nexus/src/app/deployment.rs +++ b/nexus/src/app/deployment.rs @@ -103,9 +103,14 @@ impl super::Nexus { enabled: params.enabled, time_set: chrono::Utc::now(), }; + self.db_datastore .blueprint_target_set_current(opctx, new_target) .await?; + + // When we add a background task executing the target blueprint, + // this is the point where we'd signal it to update its target. + Ok(new_target) } From ff88b37352caff909ace00b9d85fbaf8ca2a3d69 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Fri, 26 Jan 2024 11:32:30 -0500 Subject: [PATCH 15/24] comment cleanup --- schema/crdb/dbinit.sql | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index 7b895c82c1..989b52e2d0 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -3045,7 +3045,7 @@ CREATE TABLE IF NOT EXISTS omicron.public.inv_omicron_zone_nic ( * `OmicronZonesConfig` for each sled. * * `bp_omicron_zones_not_in_service` stores a list of Omicron zones (present in - * `bp_omicron_zone` that are NOT in service; e.g., should not appear in + * `bp_omicron_zone`) that are NOT in service; e.g., should not appear in * internal DNS. Nexus's in-memory `Blueprint` representation stores the set of * zones that ARE in service. We invert that logic at this layer because we * expect most blueprints to have a relatively large number of omicron zones, @@ -3098,29 +3098,23 @@ CREATE TABLE IF NOT EXISTS omicron.public.bp_target ( time_made_target TIMESTAMPTZ NOT NULL ); --- generation number for the OmicronZonesConfig for each sled in a blueprint +-- see inv_sled_omicron_zones, which is identical except it references a +-- collection whereas this table references a blueprint CREATE TABLE IF NOT EXISTS omicron.public.bp_sled_omicron_zones ( -- foreign key into `blueprint` table blueprint_id UUID NOT NULL, - -- unique id for this sled (should be foreign keys into `sled` table, though - -- it's conceivable a blueprint could refer to a sled that no longer exists, - -- particularly if the blueprint is older than the current target) sled_id UUID NOT NULL, - - -- OmicronZonesConfig generation for the zones on this sled in this - -- blueprint generation INT8 NOT NULL, - PRIMARY KEY (blueprint_id, sled_id) ); -- description of omicron zones specified in a blueprint -- --- this is currently identical to `inv_omicron_zone`, except that the foreign --- keys reference other blueprint tables intead of inventory tables. we expect +-- This is currently identical to `inv_omicron_zone`, except that the foreign +-- keys reference other blueprint tables intead of inventory tables. We expect -- their sameness to diverge over time as either inventory or blueprints (or --- both) grow context-specific properties +-- both) grow context-specific properties. CREATE TABLE IF NOT EXISTS omicron.public.bp_omicron_zone ( -- foreign key into the `blueprint` table blueprint_id UUID NOT NULL, From 14e7d5ad5213795853424695ce3b3328f50e395a Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Fri, 26 Jan 2024 11:33:19 -0500 Subject: [PATCH 16/24] rename time_set -> time_made_target --- nexus/db-model/src/deployment.rs | 4 ++-- nexus/db-queries/src/db/datastore/deployment.rs | 16 ++++++++-------- nexus/src/app/deployment.rs | 2 +- nexus/types/src/deployment.rs | 2 +- 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/nexus/db-model/src/deployment.rs b/nexus/db-model/src/deployment.rs index 8e06237948..07a1d10382 100644 --- a/nexus/db-model/src/deployment.rs +++ b/nexus/db-model/src/deployment.rs @@ -68,7 +68,7 @@ impl BpTarget { version, blueprint_id: target.target_id, enabled: target.enabled, - time_made_target: target.time_set, + time_made_target: target.time_made_target, } } } @@ -78,7 +78,7 @@ impl From for nexus_types::deployment::BlueprintTarget { Self { target_id: value.blueprint_id, enabled: value.enabled, - time_set: value.time_made_target, + time_made_target: value.time_made_target, } } } diff --git a/nexus/db-queries/src/db/datastore/deployment.rs b/nexus/db-queries/src/db/datastore/deployment.rs index 10f0531e6f..952f8c03e0 100644 --- a/nexus/db-queries/src/db/datastore/deployment.rs +++ b/nexus/db-queries/src/db/datastore/deployment.rs @@ -634,7 +634,7 @@ impl DataStore { let query = InsertTargetQuery { target_id: target.target_id, enabled: target.enabled, - time_made_target: target.time_set, + time_made_target: target.time_made_target, }; let conn = self.pool_connection_authorized(opctx).await?; @@ -1286,7 +1286,7 @@ mod tests { let bp1_target = BlueprintTarget { target_id: blueprint1.id, enabled: true, - time_set: now_db_precision(), + time_made_target: now_db_precision(), }; datastore .blueprint_target_set_current(&opctx, bp1_target) @@ -1377,7 +1377,7 @@ mod tests { let bp2_target = BlueprintTarget { target_id: blueprint2.id, enabled: true, - time_set: now_db_precision(), + time_made_target: now_db_precision(), }; datastore .blueprint_target_set_current(&opctx, bp2_target) @@ -1429,7 +1429,7 @@ mod tests { BlueprintTarget { target_id: nonexistent_blueprint_id, enabled: true, - time_set: now_db_precision(), + time_made_target: now_db_precision(), }, ) .await @@ -1476,17 +1476,17 @@ mod tests { let bp1_target = BlueprintTarget { target_id: blueprint1.id, enabled: true, - time_set: now_db_precision(), + time_made_target: now_db_precision(), }; let bp2_target = BlueprintTarget { target_id: blueprint2.id, enabled: true, - time_set: now_db_precision(), + time_made_target: now_db_precision(), }; let bp3_target = BlueprintTarget { target_id: blueprint3.id, enabled: true, - time_set: now_db_precision(), + time_made_target: now_db_precision(), }; // Attempting to make blueprint2 the current target should fail because @@ -1560,7 +1560,7 @@ mod tests { let bp4_target = BlueprintTarget { target_id: blueprint4.id, enabled: false, - time_set: now_db_precision(), + time_made_target: now_db_precision(), }; datastore .blueprint_target_set_current(&opctx, bp4_target) diff --git a/nexus/src/app/deployment.rs b/nexus/src/app/deployment.rs index ae695f1ff4..b9718a0367 100644 --- a/nexus/src/app/deployment.rs +++ b/nexus/src/app/deployment.rs @@ -101,7 +101,7 @@ impl super::Nexus { let new_target = BlueprintTarget { target_id: params.target_id, enabled: params.enabled, - time_set: chrono::Utc::now(), + time_made_target: chrono::Utc::now(), }; self.db_datastore diff --git a/nexus/types/src/deployment.rs b/nexus/types/src/deployment.rs index c0cf1715df..3b4c3b3142 100644 --- a/nexus/types/src/deployment.rs +++ b/nexus/types/src/deployment.rs @@ -217,7 +217,7 @@ pub struct BlueprintTarget { /// This should generally be left enabled. pub enabled: bool, /// when this blueprint was made the target - pub time_set: chrono::DateTime, + pub time_made_target: chrono::DateTime, } /// Specifies what blueprint, if any, the system should be working toward From 02d0540f1b35d4a369edd04d437d96d6400722bd Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Fri, 26 Jan 2024 11:37:52 -0500 Subject: [PATCH 17/24] use SqlU32 instead of not-really-signed i64 --- nexus/db-model/src/deployment.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/nexus/db-model/src/deployment.rs b/nexus/db-model/src/deployment.rs index 07a1d10382..34fe08d78c 100644 --- a/nexus/db-model/src/deployment.rs +++ b/nexus/db-model/src/deployment.rs @@ -56,16 +56,16 @@ impl From for nexus_types::deployment::BlueprintMetadata { #[derive(Queryable, Clone, Debug, Selectable, Insertable)] #[diesel(table_name = bp_target)] pub struct BpTarget { - pub version: i64, // i64 only for db serialization; should never be negative + pub version: SqlU32, pub blueprint_id: Uuid, pub enabled: bool, pub time_made_target: DateTime, } impl BpTarget { - pub fn new(version: i64, target: BlueprintTarget) -> Self { + pub fn new(version: u32, target: BlueprintTarget) -> Self { Self { - version, + version: version.into(), blueprint_id: target.target_id, enabled: target.enabled, time_made_target: target.time_made_target, From f2f445cec8e88ca839119bfcdd43af91186b5936 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Fri, 26 Jan 2024 13:58:15 -0500 Subject: [PATCH 18/24] fix OpenAPI spec --- openapi/nexus-internal.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/openapi/nexus-internal.json b/openapi/nexus-internal.json index 314ff1eb15..bc26736b37 100644 --- a/openapi/nexus-internal.json +++ b/openapi/nexus-internal.json @@ -2202,7 +2202,7 @@ "type": "string", "format": "uuid" }, - "time_set": { + "time_made_target": { "description": "when this blueprint was made the target", "type": "string", "format": "date-time" @@ -2211,7 +2211,7 @@ "required": [ "enabled", "target_id", - "time_set" + "time_made_target" ] }, "BlueprintTargetSet": { From 0c14431ca24fd06f0b36a0421970d24c3c5a26d6 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Fri, 26 Jan 2024 13:58:21 -0500 Subject: [PATCH 19/24] add clarifying comment --- nexus/db-model/src/omicron_zone_config.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/nexus/db-model/src/omicron_zone_config.rs b/nexus/db-model/src/omicron_zone_config.rs index e85242266e..f4726ccd92 100644 --- a/nexus/db-model/src/omicron_zone_config.rs +++ b/nexus/db-model/src/omicron_zone_config.rs @@ -242,6 +242,10 @@ impl OmicronZone { ensure!(expected_id == nic_row.id, "caller provided wrong NIC"); Ok(nic_row.into_network_interface_for_zone(self.id)?) } + // We don't expect and don't have a NIC. This is reasonable, so we + // don't `bail!` like we do in the next two cases, but we also + // _don't have a NIC_. Put an error into `nic`, and then if we land + // in a zone below that expects one, we'll fail then. (None, None) => Err(anyhow!( "expected zone to have an associated NIC, but it doesn't" )), From fe8dcb09494309860e953261e320b525b996bbc0 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Fri, 26 Jan 2024 13:58:54 -0500 Subject: [PATCH 20/24] typo fix --- nexus/db-queries/src/db/datastore/deployment.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nexus/db-queries/src/db/datastore/deployment.rs b/nexus/db-queries/src/db/datastore/deployment.rs index 952f8c03e0..a93df8faab 100644 --- a/nexus/db-queries/src/db/datastore/deployment.rs +++ b/nexus/db-queries/src/db/datastore/deployment.rs @@ -94,7 +94,7 @@ impl DataStore { .await?; // In the database, the blueprint is represented essentially as a tree - // rooted at an `blueprint` row. Other nodes in the tree point + // rooted at a `blueprint` row. Other nodes in the tree point // back at the `blueprint` via `blueprint_id`. // // It's helpful to assemble some values before entering the transaction From 45d04214ea7e53bd336ff55689ceb8e8d741ba58 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Fri, 26 Jan 2024 13:59:46 -0500 Subject: [PATCH 21/24] address "open question" comments --- nexus/db-queries/src/db/datastore/deployment.rs | 3 --- schema/crdb/dbinit.sql | 6 +++--- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/deployment.rs b/nexus/db-queries/src/db/datastore/deployment.rs index a93df8faab..7a4b79a8c1 100644 --- a/nexus/db-queries/src/db/datastore/deployment.rs +++ b/nexus/db-queries/src/db/datastore/deployment.rs @@ -670,9 +670,6 @@ impl DataStore { // `target` between the above query and the below query. In such a case, // this query will fail with an "unknown blueprint ID" error. This // should be rare in practice. - // - // TODO-correctness Should we loop here if we get a "blueprint not - // found" error to catch this race? let authz_blueprint = authz_blueprint_from_id(target.target_id); let blueprint = self.blueprint_read(opctx, &authz_blueprint).await?; diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index 989b52e2d0..86d1340379 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -3063,9 +3063,9 @@ CREATE TABLE IF NOT EXISTS omicron.public.blueprint ( -- allowed to be NULL: the initial blueprint has no parent. Additionally, -- it may be non-NULL but no longer reference a row in this table: once a -- child blueprint has been created from a parent, it's possible for the - -- parent to be deleted. An open question is whether we should NULL out this - -- field on such a deletion; currently we do not, so we can always see that - -- there had been a particular parent even if it's now gone. + -- parent to be deleted. We do not NULL out this field on such a deletion, + -- so we can always see that there had been a particular parent even if it's + -- now gone. parent_blueprint_id UUID, -- These fields are for debugging only. From bf84a0e99948083237da5092c257604c68947e63 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Fri, 26 Jan 2024 14:46:07 -0500 Subject: [PATCH 22/24] remove extra-cautious primary key check --- .../db-queries/src/db/datastore/deployment.rs | 34 ++++++++++++------- 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/deployment.rs b/nexus/db-queries/src/db/datastore/deployment.rs index 7a4b79a8c1..e77e9537ec 100644 --- a/nexus/db-queries/src/db/datastore/deployment.rs +++ b/nexus/db-queries/src/db/datastore/deployment.rs @@ -236,24 +236,19 @@ impl DataStore { let (parent_blueprint_id, time_created, creator, comment) = { use db::schema::blueprint::dsl; - let mut blueprints = dsl::blueprint + let Some(blueprint) = dsl::blueprint .filter(dsl::id.eq(blueprint_id)) - .limit(2) .select(DbBlueprint::as_select()) - .load_async(&*conn) + .get_result_async(&*conn) .await - .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))? - .into_iter(); - - // Check that this blueprint exists. - let Some(blueprint) = blueprints.next() else { + .optional() + .map_err(|e| { + public_error_from_diesel(e, ErrorHandler::Server) + })? + else { return Err(authz_blueprint.not_found()); }; - // Ensure we don't have another blueprint with this ID (should be - // impossible given we filtered on an exact primary key) - bail_unless!(blueprints.next().is_none()); - ( blueprint.parent_blueprint_id, blueprint.time_created, @@ -1204,6 +1199,14 @@ mod tests { .unwrap(); let authz_blueprint = authz_blueprint_from_id(blueprint1.id); + // Trying to read it from the database should fail with the relevant + // "not found" error. + let err = datastore + .blueprint_read(&opctx, &authz_blueprint) + .await + .unwrap_err(); + assert_eq!(err, authz_blueprint.not_found()); + // Write it to the database and read it back. datastore .blueprint_insert(&opctx, &blueprint1) @@ -1225,6 +1228,13 @@ mod tests { assert_eq!(blueprint1.zones_in_service.len(), 0); assert_eq!(blueprint1.parent_blueprint_id, None); + // Trying to insert the same blueprint again should fail. + let err = datastore + .blueprint_insert(&opctx, &blueprint1) + .await + .unwrap_err(); + assert!(err.to_string().contains("duplicate key")); + // Delete the blueprint and ensure it's really gone. datastore.blueprint_delete(&opctx, &authz_blueprint).await.unwrap(); ensure_blueprint_fully_deleted(&datastore, blueprint1.id).await; From 17abc3ff1bd75e00982c3dd36114f421b14a22b9 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Fri, 26 Jan 2024 14:50:32 -0500 Subject: [PATCH 23/24] cargo fmt --- nexus/db-queries/src/db/datastore/deployment.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/deployment.rs b/nexus/db-queries/src/db/datastore/deployment.rs index e77e9537ec..72adb1d3df 100644 --- a/nexus/db-queries/src/db/datastore/deployment.rs +++ b/nexus/db-queries/src/db/datastore/deployment.rs @@ -1229,10 +1229,8 @@ mod tests { assert_eq!(blueprint1.parent_blueprint_id, None); // Trying to insert the same blueprint again should fail. - let err = datastore - .blueprint_insert(&opctx, &blueprint1) - .await - .unwrap_err(); + let err = + datastore.blueprint_insert(&opctx, &blueprint1).await.unwrap_err(); assert!(err.to_string().contains("duplicate key")); // Delete the blueprint and ensure it's really gone. From df8a9e37bb7c09c40ac033ce0a5a282517109be7 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Fri, 26 Jan 2024 14:53:38 -0500 Subject: [PATCH 24/24] catch rename in omdb --- dev-tools/omdb/src/bin/omdb/nexus.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dev-tools/omdb/src/bin/omdb/nexus.rs b/dev-tools/omdb/src/bin/omdb/nexus.rs index fef069d536..ea89923caa 100644 --- a/dev-tools/omdb/src/bin/omdb/nexus.rs +++ b/dev-tools/omdb/src/bin/omdb/nexus.rs @@ -866,7 +866,7 @@ async fn cmd_nexus_blueprints_target_show( .await .context("fetching target blueprint")?; println!("target blueprint: {}", target.target_id); - println!("set at: {}", target.time_set); + println!("made target at: {}", target.time_made_target); println!("enabled: {}", target.enabled); Ok(()) }