diff --git a/nexus/db-model/src/migration.rs b/nexus/db-model/src/migration.rs index 5739122a46..4e3ca1b35d 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 c716c930e3..7fa07aa131 100644 --- a/nexus/db-model/src/schema.rs +++ b/nexus/db-model/src/schema.rs @@ -1762,6 +1762,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-model/src/schema_versions.rs b/nexus/db-model/src/schema_versions.rs index 0ca16498ba..6f537bb522 100644 --- a/nexus/db-model/src/schema_versions.rs +++ b/nexus/db-model/src/schema_versions.rs @@ -17,8 +17,7 @@ use std::collections::BTreeMap; /// /// This must be updated when you change the database schema. Refer to /// schema/crdb/README.adoc in the root of this repository for details. -pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(79, 0, 0); - +pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(80, 0, 0); /// List of all past database schema versions, in *reverse* order /// /// If you want to change the Omicron database schema, you must update this. @@ -29,6 +28,7 @@ static KNOWN_VERSIONS: Lazy> = Lazy::new(|| { // | leaving the first copy as an example for the next person. // v // KnownVersion::new(next_int, "unique-dirname-with-the-sql-files"), + KnownVersion::new(80, "add-instance-id-to-migrations"), KnownVersion::new(79, "nic-spoof-allow"), KnownVersion::new(78, "vpc-subnet-routing"), KnownVersion::new(77, "remove-view-for-v2p-mappings"), diff --git a/nexus/db-queries/src/db/datastore/instance.rs b/nexus/db-queries/src/db/datastore/instance.rs index 0abe491213..9fb94f043e 100644 --- a/nexus/db-queries/src/db/datastore/instance.rs +++ b/nexus/db-queries/src/db/datastore/instance.rs @@ -698,11 +698,9 @@ impl DataStore { } })?; - self.instance_ssh_keys_delete( - opctx, - InstanceUuid::from_untyped_uuid(authz_instance.id()), - ) - .await?; + let instance_id = InstanceUuid::from_untyped_uuid(authz_instance.id()); + self.instance_ssh_keys_delete(opctx, instance_id).await?; + self.instance_mark_migrations_deleted(opctx, instance_id).await?; Ok(()) } @@ -1343,7 +1341,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 ba8a4e0392..5efe88e83f 100644 --- a/nexus/db-queries/src/db/datastore/migration.rs +++ b/nexus/db-queries/src/db/datastore/migration.rs @@ -9,6 +9,7 @@ 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,8 +17,12 @@ 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 omicron_uuid_kinds::GenericUuid; +use omicron_uuid_kinds::InstanceUuid; use uuid::Uuid; impl DataStore { @@ -38,6 +43,39 @@ impl DataStore { .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) } + /// List all migrations associated with the provided instance ID. + pub async fn instance_list_migrations( + &self, + opctx: &OpContext, + instance_id: InstanceUuid, + pagparams: &DataPageParams<'_, Uuid>, + ) -> ListResultVec { + paginated(dsl::migration, dsl::id, pagparams) + .filter(dsl::instance_id.eq(instance_id.into_untyped_uuid())) + .filter(dsl::time_deleted.is_null()) + .select(Migration::as_select()) + .load_async(&*self.pool_connection_authorized(opctx).await?) + .await + .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) + } + + /// Mark *all* migrations for the provided instance as deleted. + /// + /// This should be called when deleting an instance. + pub(crate) async fn instance_mark_migrations_deleted( + &self, + opctx: &OpContext, + instance_id: InstanceUuid, + ) -> UpdateResult { + diesel::update(dsl::migration) + .filter(dsl::instance_id.eq(instance_id.into_untyped_uuid())) + .filter(dsl::time_deleted.is_null()) + .set(dsl::time_deleted.eq(Utc::now())) + .execute_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 +128,202 @@ impl DataStore { .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::authz; + 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 + } + + async fn insert_migration( + datastore: &DataStore, + opctx: &OpContext, + instance_id: InstanceUuid, + ) -> Migration { + let migration = Migration::new( + Uuid::new_v4(), + instance_id, + Uuid::new_v4(), + Uuid::new_v4(), + ); + + datastore + .migration_insert(&opctx, migration.clone()) + .await + .expect("must insert migration successfully"); + + migration + } + + #[tokio::test] + async fn test_migration_query_by_instance() { + // Setup + let logctx = dev::test_setup_log("test_migration_query_by_instance"); + 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 = + insert_migration(&datastore, &opctx, instance_id).await; + let migration2 = + insert_migration(&datastore, &opctx, instance_id).await; + + let list = datastore + .instance_list_migrations( + &opctx, + instance_id, + &DataPageParams::max_page(), + ) + .await + .expect("must list migrations"); + assert_all_migrations_found(&[&migration1, &migration2], &list[..]); + + let migration3 = + insert_migration(&datastore, &opctx, instance_id).await; + + let list = datastore + .instance_list_migrations( + &opctx, + instance_id, + &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 + .instance_list_migrations( + &opctx, + instance_id, + &DataPageParams::max_page(), + ) + .await + .expect("must list migrations"); + assert_all_migrations_found(&[&migration1, &migration2], &list[..]); + + let deleted = datastore + .instance_mark_migrations_deleted(&opctx, instance_id) + .await + .expect("must delete remaining migrations"); + assert_eq!( + deleted, 2, + "should not delete migration that was already marked as deleted" + ); + + let list = datastore + .instance_list_migrations( + &opctx, + instance_id, + &DataPageParams::max_page(), + ) + .await + .expect("list must succeed"); + assert!(list.is_empty(), "all migrations must be deleted"); + + let deleted = datastore + .instance_mark_migrations_deleted(&opctx, instance_id) + .await + .expect("must delete remaining migrations"); + assert_eq!( + deleted, 0, + "should not delete migration that was already marked as deleted" + ); + + // 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/instance.rs b/nexus/src/app/instance.rs index d15eb2c18a..a61cceda8b 100644 --- a/nexus/src/app/instance.rs +++ b/nexus/src/app/instance.rs @@ -2111,40 +2111,6 @@ pub(crate) async fn notify_instance_updated( ) .await; - // Has a migration terminated? If so,mark the migration record as deleted if - // and only if both sides of the migration are in a terminal state. - if let Some(nexus::MigrationRuntimeState { - migration_id, - state, - role, - .. - }) = new_runtime_state.migration_state - { - if state.is_terminal() { - info!( - log, - "migration has terminated, trying to delete it..."; - "instance_id" => %instance_id, - "propolis_id" => %propolis_id, - "migration_id" => %propolis_id, - "migration_state" => %state, - "migration_role" => %role, - ); - if !datastore.migration_terminate(opctx, migration_id).await? { - info!( - log, - "did not mark migration record as deleted (the other half \ - may not yet have reported termination)"; - "instance_id" => %instance_id, - "propolis_id" => %propolis_id, - "migration_id" => %propolis_id, - "migration_state" => %state, - "migration_role" => %role, - ); - } - } - } - // If the VMM is now in a terminal state, make sure its resources get // cleaned up. // diff --git a/nexus/src/app/sagas/instance_migrate.rs b/nexus/src/app/sagas/instance_migrate.rs index 491b916c9d..b8599feb04 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/nexus/tests/integration_tests/instances.rs b/nexus/tests/integration_tests/instances.rs index f17fc3732a..9c965ccf8a 100644 --- a/nexus/tests/integration_tests/instances.rs +++ b/nexus/tests/integration_tests/instances.rs @@ -861,7 +861,6 @@ async fn test_instance_migrate(cptestctx: &ControlPlaneTestContext) { let migration = dbg!(migration_fetch(cptestctx, migration_id).await); assert_eq!(migration.target_state, MigrationState::Completed.into()); assert_eq!(migration.source_state, MigrationState::Completed.into()); - assert!(migration.time_deleted.is_some()); } #[nexus_test] 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 0000000000..ac5ecd14b2 --- /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 0000000000..3064526756 --- /dev/null +++ b/schema/crdb/add-instance-id-to-migrations/up02.sql @@ -0,0 +1,51 @@ +-- 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, + + /* Note that there's no `time_modified/time_updated` timestamp for migration + * records. This is because we track updated time separately for the source + * and target sides of the migration, using separate `time_source_updated` + * and time_target_updated` columns. + */ + + /* 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 0000000000..549c6a48fa --- /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 fcb02af8cf..990b1047a2 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -4060,12 +4060,21 @@ 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, /* The time this migration record was deleted. */ time_deleted TIMESTAMPTZ, + /* Note that there's no `time_modified/time_updated` timestamp for migration + * records. This is because we track updated time separately for the source + * and target sides of the migration, using separate `time_source_updated` + * and time_target_updated` columns. + */ + /* The state of the migration source */ source_state omicron.public.migration_state NOT NULL, @@ -4099,6 +4108,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 @@ -4115,7 +4129,7 @@ INSERT INTO omicron.public.db_metadata ( version, target_version ) VALUES - (TRUE, NOW(), NOW(), '79.0.0', NULL) + (TRUE, NOW(), NOW(), '80.0.0', NULL) ON CONFLICT DO NOTHING; COMMIT;