From 82345ea7b8ec271c3804ea43788dd29d6f664663 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Mon, 17 Jun 2024 16:07:26 -0700 Subject: [PATCH] Add instance ID column to migration table --- nexus/db-model/src/migration.rs | 6 + nexus/db-model/src/schema.rs | 1 + nexus/db-queries/src/db/datastore/instance.rs | 7 +- .../db-queries/src/db/datastore/migration.rs | 193 ++++++++++++++++++ nexus/src/app/sagas/instance_migrate.rs | 1 + .../add-instance-id-to-migrations/up01.sql | 2 + .../add-instance-id-to-migrations/up02.sql | 45 ++++ .../add-instance-id-to-migrations/up03.sql | 4 + schema/crdb/dbinit.sql | 8 + 9 files changed, 266 insertions(+), 1 deletion(-) create mode 100644 schema/crdb/add-instance-id-to-migrations/up01.sql create mode 100644 schema/crdb/add-instance-id-to-migrations/up02.sql create mode 100644 schema/crdb/add-instance-id-to-migrations/up03.sql diff --git a/nexus/db-model/src/migration.rs b/nexus/db-model/src/migration.rs index 5739122a468..4e3ca1b35d0 100644 --- a/nexus/db-model/src/migration.rs +++ b/nexus/db-model/src/migration.rs @@ -8,6 +8,7 @@ use crate::MigrationState; use chrono::DateTime; use chrono::Utc; use omicron_common::api::internal::nexus; +use omicron_uuid_kinds::{GenericUuid, InstanceUuid}; use serde::Deserialize; use serde::Serialize; use uuid::Uuid; @@ -32,6 +33,9 @@ pub struct Migration { /// `instance` table's `migration_id` field. pub id: Uuid, + /// The instance that was migrated. + pub instance_id: Uuid, + /// The time at which this migration record was created. pub time_created: DateTime, @@ -66,11 +70,13 @@ pub struct Migration { impl Migration { pub fn new( migration_id: Uuid, + instance_id: InstanceUuid, source_propolis_id: Uuid, target_propolis_id: Uuid, ) -> Self { Self { id: migration_id, + instance_id: instance_id.into_untyped_uuid(), time_created: Utc::now(), time_deleted: None, source_state: nexus::MigrationState::Pending.into(), diff --git a/nexus/db-model/src/schema.rs b/nexus/db-model/src/schema.rs index b1fabcf9762..045c78232ba 100644 --- a/nexus/db-model/src/schema.rs +++ b/nexus/db-model/src/schema.rs @@ -1769,6 +1769,7 @@ table! { table! { migration (id) { id -> Uuid, + instance_id -> Uuid, time_created -> Timestamptz, time_deleted -> Nullable, source_state -> crate::MigrationStateEnum, diff --git a/nexus/db-queries/src/db/datastore/instance.rs b/nexus/db-queries/src/db/datastore/instance.rs index f4b31b24ff3..c5ae4256a28 100644 --- a/nexus/db-queries/src/db/datastore/instance.rs +++ b/nexus/db-queries/src/db/datastore/instance.rs @@ -1343,7 +1343,12 @@ mod tests { let migration = datastore .migration_insert( &opctx, - Migration::new(Uuid::new_v4(), active_vmm.id, target_vmm.id), + Migration::new( + Uuid::new_v4(), + instance_id, + active_vmm.id, + target_vmm.id, + ), ) .await .expect("migration should be inserted successfully!"); diff --git a/nexus/db-queries/src/db/datastore/migration.rs b/nexus/db-queries/src/db/datastore/migration.rs index ba8a4e03921..4cd5b230962 100644 --- a/nexus/db-queries/src/db/datastore/migration.rs +++ b/nexus/db-queries/src/db/datastore/migration.rs @@ -5,10 +5,12 @@ //! [`DataStore`] methods on [`Migration`]s. use super::DataStore; +use crate::authz; use crate::context::OpContext; use crate::db::error::public_error_from_diesel; use crate::db::error::ErrorHandler; use crate::db::model::{Migration, MigrationState}; +use crate::db::pagination::paginated; use crate::db::schema::migration::dsl; use crate::db::update_and_check::UpdateAndCheck; use crate::db::update_and_check::UpdateStatus; @@ -16,6 +18,8 @@ use async_bb8_diesel::AsyncRunQueryDsl; use chrono::Utc; use diesel::prelude::*; use omicron_common::api::external::CreateResult; +use omicron_common::api::external::DataPageParams; +use omicron_common::api::external::ListResultVec; use omicron_common::api::external::UpdateResult; use omicron_common::api::internal::nexus; use uuid::Uuid; @@ -38,6 +42,22 @@ impl DataStore { .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) } + /// List all migrations associated with the provided instance ID. + pub async fn migration_list_by_instance( + &self, + opctx: &OpContext, + authz_instance: &authz::Instance, + pagparams: &DataPageParams<'_, Uuid>, + ) -> ListResultVec { + paginated(dsl::migration, dsl::id, pagparams) + .filter(dsl::time_deleted.is_null()) + .filter(dsl::instance_id.eq(authz_instance.id())) + .select(Migration::as_select()) + .load_async(&*self.pool_connection_authorized(opctx).await?) + .await + .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) + } + /// Marks a migration record as deleted if and only if both sides of the /// migration are in a terminal state. pub async fn migration_terminate( @@ -90,3 +110,176 @@ impl DataStore { .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::db::datastore::test_utils::datastore_test; + use crate::db::lookup::LookupPath; + use crate::db::model::Instance; + use nexus_db_model::Project; + use nexus_test_utils::db::test_setup_database; + use nexus_types::external_api::params; + use omicron_common::api::external::ByteCount; + use omicron_common::api::external::IdentityMetadataCreateParams; + use omicron_test_utils::dev; + use omicron_uuid_kinds::GenericUuid; + use omicron_uuid_kinds::InstanceUuid; + + async fn create_test_instance( + datastore: &DataStore, + opctx: &OpContext, + ) -> authz::Instance { + let silo_id = *nexus_db_fixed_data::silo::DEFAULT_SILO_ID; + let project_id = Uuid::new_v4(); + let instance_id = InstanceUuid::new_v4(); + + let (authz_project, _project) = datastore + .project_create( + &opctx, + Project::new_with_id( + project_id, + silo_id, + params::ProjectCreate { + identity: IdentityMetadataCreateParams { + name: "stuff".parse().unwrap(), + description: "Where I keep my stuff".into(), + }, + }, + ), + ) + .await + .expect("project must be created successfully"); + let _ = datastore + .project_create_instance( + &opctx, + &authz_project, + Instance::new( + instance_id, + project_id, + ¶ms::InstanceCreate { + identity: IdentityMetadataCreateParams { + name: "myinstance".parse().unwrap(), + description: "It's an instance".into(), + }, + ncpus: 2i64.try_into().unwrap(), + memory: ByteCount::from_gibibytes_u32(16), + hostname: "myhostname".try_into().unwrap(), + user_data: Vec::new(), + network_interfaces: + params::InstanceNetworkInterfaceAttachment::None, + external_ips: Vec::new(), + disks: Vec::new(), + ssh_public_keys: None, + start: false, + }, + ), + ) + .await + .expect("instance must be created successfully"); + + let (.., authz_instance) = LookupPath::new(&opctx, &datastore) + .instance_id(instance_id.into_untyped_uuid()) + .lookup_for(authz::Action::Modify) + .await + .expect("instance must exist"); + authz_instance + } + + #[tokio::test] + async fn test_migration_list_by_instance() { + // Setup + let logctx = dev::test_setup_log("test_instance_fetch_all"); + let mut db = test_setup_database(&logctx.log).await; + let (opctx, datastore) = datastore_test(&logctx, &db).await; + let authz_instance = create_test_instance(&datastore, &opctx).await; + let instance_id = InstanceUuid::from_untyped_uuid(authz_instance.id()); + let migration1 = Migration::new( + Uuid::new_v4(), + instance_id, + Uuid::new_v4(), + Uuid::new_v4(), + ); + datastore + .migration_insert(&opctx, migration1.clone()) + .await + .expect("must insert migration 1"); + let migration2 = Migration::new( + Uuid::new_v4(), + instance_id, + Uuid::new_v4(), + Uuid::new_v4(), + ); + datastore + .migration_insert(&opctx, migration2.clone()) + .await + .expect("must insert migration 2"); + + let list = datastore + .migration_list_by_instance( + &opctx, + &authz_instance, + &DataPageParams::max_page(), + ) + .await + .expect("must list migrations"); + assert_all_migrations_found(&[&migration1, &migration2], &list[..]); + + let migration3 = Migration::new( + Uuid::new_v4(), + instance_id, + Uuid::new_v4(), + Uuid::new_v4(), + ); + datastore + .migration_insert(&opctx, migration3.clone()) + .await + .expect("must insert migration 3"); + + let list = datastore + .migration_list_by_instance( + &opctx, + &authz_instance, + &DataPageParams::max_page(), + ) + .await + .expect("must list migrations"); + assert_all_migrations_found( + &[&migration1, &migration2, &migration3], + &list[..], + ); + + datastore + .migration_mark_deleted(&opctx, migration3.id) + .await + .expect("must delete migration"); + let list = datastore + .migration_list_by_instance( + &opctx, + &authz_instance, + &DataPageParams::max_page(), + ) + .await + .expect("must list migrations"); + assert_all_migrations_found(&[&migration1, &migration2], &list[..]); + + // Clean up. + db.cleanup().await.unwrap(); + logctx.cleanup_successful(); + } + + #[track_caller] + fn assert_all_migrations_found( + expected: &[&Migration], + actual: &[Migration], + ) { + assert_eq!(expected.len(), actual.len()); + for migration in expected { + assert!( + actual.iter().any(|m| m.id == migration.id), + "couldn't find migration {:?} in {actual:#?}", + migration.id, + ); + } + } +} diff --git a/nexus/src/app/sagas/instance_migrate.rs b/nexus/src/app/sagas/instance_migrate.rs index 3546642bbb3..e9ffaa194d0 100644 --- a/nexus/src/app/sagas/instance_migrate.rs +++ b/nexus/src/app/sagas/instance_migrate.rs @@ -230,6 +230,7 @@ async fn sim_create_migration_record( &opctx, db::model::Migration::new( migration_id, + InstanceUuid::from_untyped_uuid(params.instance.id()), source_propolis_id, target_propolis_id, ), diff --git a/schema/crdb/add-instance-id-to-migrations/up01.sql b/schema/crdb/add-instance-id-to-migrations/up01.sql new file mode 100644 index 00000000000..ac5ecd14b23 --- /dev/null +++ b/schema/crdb/add-instance-id-to-migrations/up01.sql @@ -0,0 +1,2 @@ +-- shake hands with DANGER! +DROP TABLE IF EXISTS omicron.public.migration; diff --git a/schema/crdb/add-instance-id-to-migrations/up02.sql b/schema/crdb/add-instance-id-to-migrations/up02.sql new file mode 100644 index 00000000000..ad64c599197 --- /dev/null +++ b/schema/crdb/add-instance-id-to-migrations/up02.sql @@ -0,0 +1,45 @@ +-- A table of the states of current migrations. +CREATE TABLE IF NOT EXISTS omicron.public.migration ( + id UUID PRIMARY KEY, + + /* The ID of the instance that was migrated */ + instance_id UUID NOT NULL, + + /* The time this migration record was created. */ + time_created TIMESTAMPTZ NOT NULL, + + /* The time this migration record was deleted. */ + time_deleted TIMESTAMPTZ, + + /* The state of the migration source */ + source_state omicron.public.migration_state NOT NULL, + + /* The ID of the migration source Propolis */ + source_propolis_id UUID NOT NULL, + + /* Generation number owned and incremented by the source sled-agent */ + source_gen INT8 NOT NULL DEFAULT 1, + + /* Timestamp of when the source field was last updated. + * + * This is provided by the sled-agent when publishing a migration state + * update. + */ + time_source_updated TIMESTAMPTZ, + + /* The state of the migration target */ + target_state omicron.public.migration_state NOT NULL, + + /* The ID of the migration target Propolis */ + target_propolis_id UUID NOT NULL, + + /* Generation number owned and incremented by the target sled-agent */ + target_gen INT8 NOT NULL DEFAULT 1, + + /* Timestamp of when the source field was last updated. + * + * This is provided by the sled-agent when publishing a migration state + * update. + */ + time_target_updated TIMESTAMPTZ +); diff --git a/schema/crdb/add-instance-id-to-migrations/up03.sql b/schema/crdb/add-instance-id-to-migrations/up03.sql new file mode 100644 index 00000000000..549c6a48fa4 --- /dev/null +++ b/schema/crdb/add-instance-id-to-migrations/up03.sql @@ -0,0 +1,4 @@ +/* Lookup migrations by instance ID */ +CREATE INDEX IF NOT EXISTS lookup_migrations_by_instance_id ON omicron.public.migration ( + instance_id +); diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index 338f52f8543..923ac8b38d7 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -4090,6 +4090,9 @@ CREATE TYPE IF NOT EXISTS omicron.public.migration_state AS ENUM ( CREATE TABLE IF NOT EXISTS omicron.public.migration ( id UUID PRIMARY KEY, + /* The ID of the instance that was migrated */ + instance_id UUID NOT NULL, + /* The time this migration record was created. */ time_created TIMESTAMPTZ NOT NULL, @@ -4129,6 +4132,11 @@ CREATE TABLE IF NOT EXISTS omicron.public.migration ( time_target_updated TIMESTAMPTZ ); +/* Lookup migrations by instance ID */ +CREATE INDEX IF NOT EXISTS lookup_migrations_by_instance_id ON omicron.public.migration ( + instance_id +); + /* Lookup region snapshot by snapshot id */ CREATE INDEX IF NOT EXISTS lookup_region_snapshot_by_snapshot_id on omicron.public.region_snapshot ( snapshot_id