diff --git a/Cargo.lock b/Cargo.lock index 0c5c27ea5a..2f2123c544 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4279,6 +4279,7 @@ dependencies = [ "authz-macros", "base64 0.21.4", "bb8", + "camino", "chrono", "cookie", "db-macros", diff --git a/nexus/db-queries/Cargo.toml b/nexus/db-queries/Cargo.toml index 6739bf0286..a8256cb60a 100644 --- a/nexus/db-queries/Cargo.toml +++ b/nexus/db-queries/Cargo.toml @@ -13,6 +13,7 @@ async-bb8-diesel.workspace = true async-trait.workspace = true base64.workspace = true bb8.workspace = true +camino.workspace = true chrono.workspace = true cookie.workspace = true diesel.workspace = true diff --git a/nexus/db-queries/src/db/datastore/db_metadata.rs b/nexus/db-queries/src/db/datastore/db_metadata.rs index 36bc8d7845..ac43081601 100644 --- a/nexus/db-queries/src/db/datastore/db_metadata.rs +++ b/nexus/db-queries/src/db/datastore/db_metadata.rs @@ -8,7 +8,11 @@ use super::DataStore; use crate::db; use crate::db::error::public_error_from_diesel_pool; use crate::db::error::ErrorHandler; -use async_bb8_diesel::{AsyncRunQueryDsl, AsyncSimpleConnection}; +use crate::db::TransactionError; +use async_bb8_diesel::{ + AsyncConnection, AsyncRunQueryDsl, AsyncSimpleConnection, +}; +use camino::{Utf8Path, Utf8PathBuf}; use chrono::Utc; use diesel::prelude::*; use omicron_common::api::external::Error; @@ -19,6 +23,48 @@ use std::collections::BTreeSet; use std::ops::Bound; use std::str::FromStr; +pub const EARLIEST_SUPPORTED_VERSION: &'static str = "1.0.0"; + +/// Reads a "version directory" and reads all SQL changes into +/// a result Vec. +/// +/// Any file that starts with "up" and ends with "sql" is considered +/// part of the migration, and fully read to a string. +/// +/// These are sorted lexicographically. +pub async fn all_sql_for_version_migration>( + path: P, +) -> Result, String> { + let target_dir = path.as_ref(); + let mut up_sqls = vec![]; + let entries = target_dir + .read_dir_utf8() + .map_err(|e| format!("Failed to readdir {target_dir}: {e}"))?; + for entry in entries { + let entry = entry.map_err(|err| format!("Invalid entry: {err}"))?; + let pathbuf = entry.into_path(); + let is_up = pathbuf + .file_name() + .map(|name| name.starts_with("up")) + .unwrap_or(false); + let is_sql = matches!(pathbuf.extension(), Some("sql")); + if is_up && is_sql { + up_sqls.push(pathbuf); + } + } + up_sqls.sort(); + + let mut result = vec![]; + for path in up_sqls.into_iter() { + result.push( + tokio::fs::read_to_string(&path) + .await + .map_err(|e| format!("Cannot read {path}: {e}"))?, + ); + } + Ok(result) +} + impl DataStore { // Ensures that the database schema matches "desired_version". // @@ -136,13 +182,12 @@ impl DataStore { "target_version" => target_version.to_string(), ); - let up = config - .schema_dir - .join(target_version.to_string()) - .join("up.sql"); - let sql = tokio::fs::read_to_string(&up).await.map_err(|e| { - format!("Cannot read {up}: {e}", up = up.display()) - })?; + let target_dir = Utf8PathBuf::from_path_buf( + config.schema_dir.join(target_version.to_string()), + ) + .map_err(|e| format!("Invalid schema path: {}", e.display()))?; + + let up_sqls = all_sql_for_version_migration(&target_dir).await?; // Confirm the current version, set the "target_version" // column to indicate that a schema update is in-progress. @@ -160,8 +205,16 @@ impl DataStore { "target_version" => target_version.to_string(), ); - // Perform the schema change. - self.apply_schema_update(&sql).await.map_err(|e| e.to_string())?; + for sql in &up_sqls { + // Perform the schema change. + self.apply_schema_update( + ¤t_version, + &target_version, + &sql, + ) + .await + .map_err(|e| e.to_string())?; + } info!( log, @@ -273,11 +326,37 @@ impl DataStore { // Applies a schema update, using raw SQL read from a caller-supplied // configuration file. - async fn apply_schema_update(&self, sql: &String) -> Result<(), Error> { - self.pool().batch_execute_async(&sql).await.map_err(|e| { - Error::internal_error(&format!("Failed to execute upgrade: {e}")) - })?; - Ok(()) + async fn apply_schema_update( + &self, + current: &SemverVersion, + target: &SemverVersion, + sql: &String, + ) -> Result<(), Error> { + let result = self.pool().transaction_async(|conn| async move { + if target.to_string() != EARLIEST_SUPPORTED_VERSION { + let validate_version_query = format!("SELECT CAST(\ + IF(\ + (\ + SELECT version = '{current}' and target_version = '{target}'\ + FROM omicron.public.db_metadata WHERE singleton = true\ + ),\ + 'true',\ + 'Invalid starting version for schema change'\ + ) AS BOOL\ + );"); + conn.batch_execute_async(&validate_version_query).await?; + } + conn.batch_execute_async(&sql).await?; + Ok::<_, TransactionError<()>>(()) + }).await; + + match result { + Ok(()) => Ok(()), + Err(TransactionError::CustomError(())) => panic!("No custom error"), + Err(TransactionError::Pool(e)) => { + Err(public_error_from_diesel_pool(e, ErrorHandler::Server)) + } + } } // Completes a schema migration, upgrading to the new version. @@ -392,50 +471,22 @@ mod test { // v0 to v1 to v2, but it doesn't need to re-apply it. add_upgrade(v0.clone(), "SELECT true;".to_string()).await; - // Ensure that all schema changes also validate the expected version - // information. - let wrap_in_version_checking_txn = |version, target, sql| -> String { - format!("BEGIN; \ - SELECT CAST(\ - IF(\ - (\ - SELECT version = '{version}' and target_version = '{target}'\ - FROM omicron.public.db_metadata WHERE singleton = true\ - ),\ - 'true',\ - 'Invalid starting version for schema change'\ - ) AS BOOL\ - );\ - {sql};\ - COMMIT;") - }; - // This version adds a new table, but it takes a little while. // // This delay is intentional, so that some Nexus instances issuing // the update act quickly, while others lag behind. add_upgrade( v1.clone(), - wrap_in_version_checking_txn( - &v0, - &v1, - "SELECT pg_sleep(RANDOM()); \ - CREATE TABLE IF NOT EXISTS widget(); \ - SELECT pg_sleep(RANDOM());", - ), + "SELECT pg_sleep(RANDOM() / 10); \ + CREATE TABLE IF NOT EXISTS widget(); \ + SELECT pg_sleep(RANDOM() / 10);" + .to_string(), ) .await; // The table we just created is deleted by a subsequent update. - add_upgrade( - v2.clone(), - wrap_in_version_checking_txn( - &v1, - &v2, - "DROP TABLE IF EXISTS widget;", - ), - ) - .await; + add_upgrade(v2.clone(), "DROP TABLE IF EXISTS widget;".to_string()) + .await; // Show that the datastores can be created concurrently. let config = diff --git a/nexus/db-queries/src/db/datastore/mod.rs b/nexus/db-queries/src/db/datastore/mod.rs index 13fb132abb..f653675728 100644 --- a/nexus/db-queries/src/db/datastore/mod.rs +++ b/nexus/db-queries/src/db/datastore/mod.rs @@ -87,6 +87,9 @@ mod vpc; mod zpool; pub use address_lot::AddressLotCreateResult; +pub use db_metadata::{ + all_sql_for_version_migration, EARLIEST_SUPPORTED_VERSION, +}; pub use dns::DnsVersionUpdateBuilder; pub use rack::RackInit; pub use silo::Discoverability; diff --git a/nexus/tests/integration_tests/schema.rs b/nexus/tests/integration_tests/schema.rs index 9bbfa93f40..67dfa6c255 100644 --- a/nexus/tests/integration_tests/schema.rs +++ b/nexus/tests/integration_tests/schema.rs @@ -2,9 +2,13 @@ // 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 camino::Utf8PathBuf; use chrono::{DateTime, Utc}; use dropshot::test_util::LogContext; use nexus_db_model::schema::SCHEMA_VERSION as LATEST_SCHEMA_VERSION; +use nexus_db_queries::db::datastore::{ + all_sql_for_version_migration, EARLIEST_SUPPORTED_VERSION, +}; use nexus_test_utils::{db, load_test_config, ControlPlaneTestContextBuilder}; use omicron_common::api::external::SemverVersion; use omicron_common::api::internal::shared::SwitchLocation; @@ -22,7 +26,6 @@ use uuid::Uuid; const SCHEMA_DIR: &'static str = concat!(env!("CARGO_MANIFEST_DIR"), "/../schema/crdb"); -const EARLIEST_SUPPORTED_VERSION: &'static str = "1.0.0"; async fn test_setup_just_crdb<'a>( log: &Logger, @@ -71,6 +74,7 @@ async fn apply_update( // We skip this for the earliest supported version because these tables // might not exist yet. if version != EARLIEST_SUPPORTED_VERSION { + info!(log, "Updating schema version in db_metadata (setting target)"); let sql = format!("UPDATE omicron.public.db_metadata SET target_version = '{}' WHERE singleton = true;", version); client .batch_execute(&sql) @@ -78,24 +82,32 @@ async fn apply_update( .expect("Failed to bump version number"); } - let file = "up.sql"; - let sql = tokio::fs::read_to_string( - PathBuf::from(SCHEMA_DIR).join(version).join(file), - ) - .await - .unwrap(); + let target_dir = Utf8PathBuf::from(SCHEMA_DIR).join(version); + let sqls = all_sql_for_version_migration(&target_dir).await.unwrap(); for _ in 0..times_to_apply { - client.batch_execute(&sql).await.expect("failed to apply update"); + for sql in sqls.iter() { + client + .batch_execute("BEGIN;") + .await + .expect("Failed to BEGIN update"); + client.batch_execute(&sql).await.expect("Failed to execute update"); + client + .batch_execute("COMMIT;") + .await + .expect("Failed to COMMIT update"); + } } // Normally, Nexus actually bumps the version number. // // We do so explicitly here. + info!(log, "Updating schema version in db_metadata (removing target)"); let sql = format!("UPDATE omicron.public.db_metadata SET version = '{}', target_version = NULL WHERE singleton = true;", version); client.batch_execute(&sql).await.expect("Failed to bump version number"); client.cleanup().await.expect("cleaning up after wipe"); + info!(log, "Update to {version} applied successfully"); } async fn query_crdb_schema_version(crdb: &CockroachInstance) -> String { diff --git a/schema/crdb/1.0.0/up.sql b/schema/crdb/1.0.0/up.sql index da1b4dc18b..d3c17009f6 100644 --- a/schema/crdb/1.0.0/up.sql +++ b/schema/crdb/1.0.0/up.sql @@ -1,8 +1,6 @@ -- DO NOT EDIT THIS SCHEMA. -- This file is a point-in-time snapshot of revision "1.0.0" of the database. -BEGIN; - /* * We assume the database and user do not already exist so that we don't * inadvertently clobber what's there. If they might exist, the user has to @@ -2524,5 +2522,3 @@ INSERT INTO omicron.public.db_metadata ( ) VALUES ( TRUE, NOW(), NOW(), '1.0.0', NULL) ON CONFLICT DO NOTHING; - -COMMIT; diff --git a/schema/crdb/2.0.0/up.sql b/schema/crdb/2.0.0/up.sql index 24babf3b42..eb8e41772b 100644 --- a/schema/crdb/2.0.0/up.sql +++ b/schema/crdb/2.0.0/up.sql @@ -1,25 +1,2 @@ --- CRDB documentation recommends the following: --- "Execute schema changes either as single statements (as an implicit transaction), --- or in an explicit transaction consisting of the single schema change statement." --- --- For each schema change, we transactionally: --- 1. Check the current version --- 2. Apply the idempotent update - -BEGIN; - -SELECT CAST( - IF( - ( - SELECT version = '1.0.0' and target_version = '2.0.0' - FROM omicron.public.db_metadata WHERE singleton = true - ), - 'true', - 'Invalid starting version for schema change' - ) AS BOOL -); - ALTER TABLE omicron.public.instance ADD COLUMN IF NOT EXISTS boot_on_fault BOOL NOT NULL DEFAULT false; - -COMMIT; diff --git a/schema/crdb/3.0.0/up.sql b/schema/crdb/3.0.0/up.sql index b2883a933a..8183875c24 100644 --- a/schema/crdb/3.0.0/up.sql +++ b/schema/crdb/3.0.0/up.sql @@ -1,36 +1,13 @@ --- CRDB documentation recommends the following: --- "Execute schema changes either as single statements (as an implicit transaction), --- or in an explicit transaction consisting of the single schema change statement." --- --- For each schema change, we transactionally: --- 1. Check the current version --- 2. Apply the idempotent update - -BEGIN; - -SELECT CAST( - IF( - ( - SELECT version = '2.0.0' and target_version = '3.0.0' - FROM omicron.public.db_metadata WHERE singleton = true - ), - 'true', - 'Invalid starting version for schema change' - ) AS BOOL -); - ALTER TABLE omicron.public.ip_pool ADD COLUMN IF NOT EXISTS silo_ID UUID, ADD COLUMN IF NOT EXISTS project_id UUID, - + -- if silo_id is null, then project_id must be null ADD CONSTRAINT IF NOT EXISTS project_implies_silo CHECK ( NOT ((silo_id IS NULL) AND (project_id IS NOT NULL)) ), - -- if internal = true, non-null silo_id and project_id are not allowed + -- if internal = true, non-null silo_id and project_id are not allowed ADD CONSTRAINT IF NOT EXISTS internal_pools_have_null_silo_and_project CHECK ( NOT (INTERNAL AND ((silo_id IS NOT NULL) OR (project_id IS NOT NULL))) ); - -COMMIT; diff --git a/schema/crdb/3.0.1/up.sql b/schema/crdb/3.0.1/up.sql deleted file mode 100644 index a630c170e9..0000000000 --- a/schema/crdb/3.0.1/up.sql +++ /dev/null @@ -1,42 +0,0 @@ -BEGIN; - -SELECT CAST( - IF( - ( - SELECT version = '3.0.0' and target_version = '3.0.1' - FROM omicron.public.db_metadata WHERE singleton = true - ), - 'true', - 'Invalid starting version for schema change' - ) AS BOOL -); - -ALTER TABLE omicron.public.ip_pool - DROP COLUMN IF EXISTS project_id, - ADD COLUMN IF NOT EXISTS is_default BOOLEAN NOT NULL DEFAULT FALSE, - DROP CONSTRAINT IF EXISTS project_implies_silo, - DROP CONSTRAINT IF EXISTS internal_pools_have_null_silo_and_project; - -COMMIT; - --- needs to be in its own transaction because of this thrilling bug --- https://github.com/cockroachdb/cockroach/issues/83593 -BEGIN; - -SELECT CAST( - IF( - ( - SELECT version = '3.0.0' and target_version = '3.0.1' - FROM omicron.public.db_metadata WHERE singleton = true - ), - 'true', - 'Invalid starting version for schema change' - ) AS BOOL -); - -CREATE UNIQUE INDEX IF NOT EXISTS one_default_pool_per_scope ON omicron.public.ip_pool ( - COALESCE(silo_id, '00000000-0000-0000-0000-000000000000'::uuid) -) WHERE - is_default = true AND time_deleted IS NULL; - -COMMIT; diff --git a/schema/crdb/3.0.1/up1.sql b/schema/crdb/3.0.1/up1.sql new file mode 100644 index 0000000000..04682e8612 --- /dev/null +++ b/schema/crdb/3.0.1/up1.sql @@ -0,0 +1,5 @@ +ALTER TABLE omicron.public.ip_pool + DROP COLUMN IF EXISTS project_id, + ADD COLUMN IF NOT EXISTS is_default BOOLEAN NOT NULL DEFAULT FALSE, + DROP CONSTRAINT IF EXISTS project_implies_silo, + DROP CONSTRAINT IF EXISTS internal_pools_have_null_silo_and_project; diff --git a/schema/crdb/3.0.1/up2.sql b/schema/crdb/3.0.1/up2.sql new file mode 100644 index 0000000000..f741b320bb --- /dev/null +++ b/schema/crdb/3.0.1/up2.sql @@ -0,0 +1,4 @@ +CREATE UNIQUE INDEX IF NOT EXISTS one_default_pool_per_scope ON omicron.public.ip_pool ( + COALESCE(silo_id, '00000000-0000-0000-0000-000000000000'::uuid) +) WHERE + is_default = true AND time_deleted IS NULL; diff --git a/schema/crdb/3.0.2/up.sql b/schema/crdb/3.0.2/up.sql index 0bca7678ff..f2732d6986 100644 --- a/schema/crdb/3.0.2/up.sql +++ b/schema/crdb/3.0.2/up.sql @@ -1,16 +1,3 @@ -BEGIN; - -SELECT CAST( - IF( - ( - SELECT version = '3.0.1' and target_version = '3.0.2' - FROM omicron.public.db_metadata WHERE singleton = true - ), - 'true', - 'Invalid starting version for schema change' - ) AS BOOL -); - -- to get ready to drop the internal column, take any IP pools with internal = -- true and set silo_id = INTERNAL_SILO_ID @@ -21,5 +8,3 @@ UPDATE omicron.public.ip_pool UPDATE omicron.public.ip_pool SET is_default = true WHERE name = 'default' and time_deleted is null; - -COMMIT; diff --git a/schema/crdb/3.0.3/up.sql b/schema/crdb/3.0.3/up.sql index d7ed8097ef..cb50a16a43 100644 --- a/schema/crdb/3.0.3/up.sql +++ b/schema/crdb/3.0.3/up.sql @@ -1,17 +1,2 @@ -BEGIN; - -SELECT CAST( - IF( - ( - SELECT version = '3.0.2' and target_version = '3.0.3' - FROM omicron.public.db_metadata WHERE singleton = true - ), - 'true', - 'Invalid starting version for schema change' - ) AS BOOL -); - ALTER TABLE omicron.public.ip_pool DROP COLUMN IF EXISTS internal; - -COMMIT; diff --git a/schema/crdb/4.0.0/up.sql b/schema/crdb/4.0.0/up.sql deleted file mode 100644 index f87308395a..0000000000 --- a/schema/crdb/4.0.0/up.sql +++ /dev/null @@ -1,37 +0,0 @@ --- CRDB documentation recommends the following: --- "Execute schema changes either as single statements (as an implicit transaction), --- or in an explicit transaction consisting of the single schema change statement." --- --- For each schema change, we transactionally: --- 1. Check the current version --- 2. Apply the idempotent update - -BEGIN; -SELECT CAST( - IF( - ( - SELECT version = '3.0.3' and target_version = '4.0.0' - FROM omicron.public.db_metadata WHERE singleton = true - ), - 'true', - 'Invalid starting version for schema change' - ) AS BOOL -); - -ALTER TYPE omicron.public.service_kind ADD VALUE IF NOT EXISTS 'clickhouse_keeper'; -COMMIT; - -BEGIN; -SELECT CAST( - IF( - ( - SELECT version = '3.0.3' and target_version = '4.0.0' - FROM omicron.public.db_metadata WHERE singleton = true - ), - 'true', - 'Invalid starting version for schema change' - ) AS BOOL -); - -ALTER TYPE omicron.public.dataset_kind ADD VALUE IF NOT EXISTS 'clickhouse_keeper'; -COMMIT; \ No newline at end of file diff --git a/schema/crdb/4.0.0/up1.sql b/schema/crdb/4.0.0/up1.sql new file mode 100644 index 0000000000..b26b646d6c --- /dev/null +++ b/schema/crdb/4.0.0/up1.sql @@ -0,0 +1 @@ +ALTER TYPE omicron.public.service_kind ADD VALUE IF NOT EXISTS 'clickhouse_keeper'; diff --git a/schema/crdb/4.0.0/up2.sql b/schema/crdb/4.0.0/up2.sql new file mode 100644 index 0000000000..018d39c137 --- /dev/null +++ b/schema/crdb/4.0.0/up2.sql @@ -0,0 +1 @@ +ALTER TYPE omicron.public.dataset_kind ADD VALUE IF NOT EXISTS 'clickhouse_keeper'; diff --git a/schema/crdb/README.adoc b/schema/crdb/README.adoc index 997170469d..ef96571d00 100644 --- a/schema/crdb/README.adoc +++ b/schema/crdb/README.adoc @@ -8,7 +8,16 @@ This directory describes the schema(s) used by CockroachDB. We use the following conventions: * `schema/crdb/VERSION/up.sql`: The necessary idempotent migrations to transition from the - previous version of CockroachDB to this version. + previous version of CockroachDB to this version. These migrations will always be placed + within one transaction per file. +** If more than one change is needed per version, any number of files starting with `up` + and ending with `.sql` may be used. These files will be sorted in lexicographic order + before being executed, and each will be executed in a separate transaction. +** CockroachDB documentation recommends the following: "Execute schema changes... in a single + explicit transaction consisting of the single schema change statement". + Practically this means: If you want to change multiple tables, columns, + types, indices, or constraints, do so in separate files. +** More information can be found here: https://www.cockroachlabs.com/docs/stable/online-schema-changes * `schema/crdb/dbinit.sql`: The necessary operations to create the latest version of the schema. Should be equivalent to running all `up.sql` migrations, in-order. * `schema/crdb/dbwipe.sql`: The necessary operations to delete the latest version