From 1e6cb9c1441a7769d5bd64fc0fca35a1e42c2fb9 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Mon, 17 Jun 2024 16:07:26 -0700 Subject: [PATCH 1/9] 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 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 b1fabcf976..045c78232b 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 0abe491213..7f8498a17f 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 ba8a4e0392..4cd5b23096 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 3546642bbb..e9ffaa194d 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 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..ad64c59919 --- /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 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 338f52f854..923ac8b38d 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 From fe871b71cf469c332a39de7f258f91012dacd7b6 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Tue, 18 Jun 2024 09:47:45 -0700 Subject: [PATCH 2/9] Add DataStore::migration_mark_deleted_by_instance --- nexus/db-queries/src/db/datastore/instance.rs | 8 +- .../db-queries/src/db/datastore/migration.rs | 106 +++++++++++++----- 2 files changed, 78 insertions(+), 36 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/instance.rs b/nexus/db-queries/src/db/datastore/instance.rs index 7f8498a17f..032b16a700 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.migration_mark_deleted_by_instance(opctx, instance_id).await?; Ok(()) } diff --git a/nexus/db-queries/src/db/datastore/migration.rs b/nexus/db-queries/src/db/datastore/migration.rs index 4cd5b23096..7ecc5b630c 100644 --- a/nexus/db-queries/src/db/datastore/migration.rs +++ b/nexus/db-queries/src/db/datastore/migration.rs @@ -22,6 +22,8 @@ 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 { @@ -58,6 +60,23 @@ impl DataStore { .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 migration_mark_deleted_by_instance( + &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( @@ -186,34 +205,39 @@ mod tests { 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( + 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, migration2.clone()) + .migration_insert(&opctx, migration.clone()) .await - .expect("must insert migration 2"); + .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 .migration_list_by_instance( @@ -225,16 +249,8 @@ mod tests { .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 migration3 = + insert_migration(&datastore, &opctx, instance_id).await; let list = datastore .migration_list_by_instance( @@ -263,6 +279,34 @@ mod tests { .expect("must list migrations"); assert_all_migrations_found(&[&migration1, &migration2], &list[..]); + let deleted = datastore + .migration_mark_deleted_by_instance(&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 + .migration_list_by_instance( + &opctx, + &authz_instance, + &DataPageParams::max_page(), + ) + .await + .expect("list must succeed"); + assert!(list.is_empty(), "all migrations must be deleted"); + + let deleted = datastore + .migration_mark_deleted_by_instance(&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(); From 339aad202e8abdc83e1bca63f169cb317f336b9b Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Tue, 18 Jun 2024 09:57:09 -0700 Subject: [PATCH 3/9] more consistent APIs --- nexus/db-queries/src/db/datastore/instance.rs | 2 +- .../db-queries/src/db/datastore/migration.rs | 28 +++++++++---------- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/instance.rs b/nexus/db-queries/src/db/datastore/instance.rs index 032b16a700..9fb94f043e 100644 --- a/nexus/db-queries/src/db/datastore/instance.rs +++ b/nexus/db-queries/src/db/datastore/instance.rs @@ -700,7 +700,7 @@ impl DataStore { let instance_id = InstanceUuid::from_untyped_uuid(authz_instance.id()); self.instance_ssh_keys_delete(opctx, instance_id).await?; - self.migration_mark_deleted_by_instance(opctx, instance_id).await?; + self.instance_mark_migrations_deleted(opctx, instance_id).await?; Ok(()) } diff --git a/nexus/db-queries/src/db/datastore/migration.rs b/nexus/db-queries/src/db/datastore/migration.rs index 7ecc5b630c..87e41b9f94 100644 --- a/nexus/db-queries/src/db/datastore/migration.rs +++ b/nexus/db-queries/src/db/datastore/migration.rs @@ -45,15 +45,15 @@ impl DataStore { } /// List all migrations associated with the provided instance ID. - pub async fn migration_list_by_instance( + pub async fn instance_list_migrations( &self, opctx: &OpContext, - authz_instance: &authz::Instance, + 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()) - .filter(dsl::instance_id.eq(authz_instance.id())) .select(Migration::as_select()) .load_async(&*self.pool_connection_authorized(opctx).await?) .await @@ -63,7 +63,7 @@ impl DataStore { /// Mark *all* migrations for the provided instance as deleted. /// /// This should be called when deleting an instance. - pub(crate) async fn migration_mark_deleted_by_instance( + pub(crate) async fn instance_mark_migrations_deleted( &self, opctx: &OpContext, instance_id: InstanceUuid, @@ -240,9 +240,9 @@ mod tests { insert_migration(&datastore, &opctx, instance_id).await; let list = datastore - .migration_list_by_instance( + .instance_list_migrations( &opctx, - &authz_instance, + instance_id, &DataPageParams::max_page(), ) .await @@ -253,9 +253,9 @@ mod tests { insert_migration(&datastore, &opctx, instance_id).await; let list = datastore - .migration_list_by_instance( + .instance_list_migrations( &opctx, - &authz_instance, + instance_id, &DataPageParams::max_page(), ) .await @@ -270,9 +270,9 @@ mod tests { .await .expect("must delete migration"); let list = datastore - .migration_list_by_instance( + .instance_list_migrations( &opctx, - &authz_instance, + instance_id, &DataPageParams::max_page(), ) .await @@ -280,7 +280,7 @@ mod tests { assert_all_migrations_found(&[&migration1, &migration2], &list[..]); let deleted = datastore - .migration_mark_deleted_by_instance(&opctx, instance_id) + .instance_mark_migrations_deleted(&opctx, instance_id) .await .expect("must delete remaining migrations"); assert_eq!( @@ -289,9 +289,9 @@ mod tests { ); let list = datastore - .migration_list_by_instance( + .instance_list_migrations( &opctx, - &authz_instance, + instance_id, &DataPageParams::max_page(), ) .await @@ -299,7 +299,7 @@ mod tests { assert!(list.is_empty(), "all migrations must be deleted"); let deleted = datastore - .migration_mark_deleted_by_instance(&opctx, instance_id) + .instance_mark_migrations_deleted(&opctx, instance_id) .await .expect("must delete remaining migrations"); assert_eq!( From d2fd5232f4248ee66b956abbb621ba5cfc238157 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Tue, 18 Jun 2024 10:41:47 -0700 Subject: [PATCH 4/9] don't mark completed migrations as deleted --- nexus/src/app/instance.rs | 34 ---------------------------------- 1 file changed, 34 deletions(-) diff --git a/nexus/src/app/instance.rs b/nexus/src/app/instance.rs index 517fbf218a..fe3b4cabf9 100644 --- a/nexus/src/app/instance.rs +++ b/nexus/src/app/instance.rs @@ -2094,40 +2094,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. // From 3ec590483b33943bf0eb821a3ec0069ad77e6c30 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Tue, 18 Jun 2024 10:44:09 -0700 Subject: [PATCH 5/9] add comment explaining updated timestamps --- schema/crdb/add-instance-id-to-migrations/up02.sql | 6 ++++++ schema/crdb/dbinit.sql | 6 ++++++ 2 files changed, 12 insertions(+) diff --git a/schema/crdb/add-instance-id-to-migrations/up02.sql b/schema/crdb/add-instance-id-to-migrations/up02.sql index ad64c59919..3064526756 100644 --- a/schema/crdb/add-instance-id-to-migrations/up02.sql +++ b/schema/crdb/add-instance-id-to-migrations/up02.sql @@ -11,6 +11,12 @@ CREATE TABLE IF NOT EXISTS omicron.public.migration ( /* 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, diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index 923ac8b38d..6856288302 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -4099,6 +4099,12 @@ CREATE TABLE IF NOT EXISTS omicron.public.migration ( /* 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, From f95e7a486647de5588d316e726704bc2725658ef Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Tue, 18 Jun 2024 10:50:35 -0700 Subject: [PATCH 6/9] remove unused import --- nexus/db-queries/src/db/datastore/migration.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/nexus/db-queries/src/db/datastore/migration.rs b/nexus/db-queries/src/db/datastore/migration.rs index 87e41b9f94..15ec6f8f21 100644 --- a/nexus/db-queries/src/db/datastore/migration.rs +++ b/nexus/db-queries/src/db/datastore/migration.rs @@ -5,7 +5,6 @@ //! [`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; From 8437975c59a0b5845038b8004e3a8e2074430954 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Tue, 18 Jun 2024 11:45:02 -0700 Subject: [PATCH 7/9] unbreak test --- nexus/db-queries/src/db/datastore/migration.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/nexus/db-queries/src/db/datastore/migration.rs b/nexus/db-queries/src/db/datastore/migration.rs index 15ec6f8f21..5efe88e83f 100644 --- a/nexus/db-queries/src/db/datastore/migration.rs +++ b/nexus/db-queries/src/db/datastore/migration.rs @@ -132,6 +132,7 @@ impl DataStore { #[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; From af97c402135c9ba02767b12777598b8307660ac6 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Tue, 25 Jun 2024 16:14:38 -0700 Subject: [PATCH 8/9] whoops, i forgot to actually add the migration --- nexus/db-model/src/schema_versions.rs | 3 ++- schema/crdb/dbinit.sql | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/nexus/db-model/src/schema_versions.rs b/nexus/db-model/src/schema_versions.rs index eed8c81421..f6eda53074 100644 --- a/nexus/db-model/src/schema_versions.rs +++ b/nexus/db-model/src/schema_versions.rs @@ -17,7 +17,7 @@ use std::collections::BTreeMap; /// /// This must be updated when you change the database schema. Refer to /// schema/crdb/README.adoc in the root of this repository for details. -pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(76, 0, 0); +pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(77, 0, 0); /// List of all past database schema versions, in *reverse* order /// @@ -29,6 +29,7 @@ static KNOWN_VERSIONS: Lazy> = Lazy::new(|| { // | leaving the first copy as an example for the next person. // v // KnownVersion::new(next_int, "unique-dirname-with-the-sql-files"), + KnownVersion::new(77, "add-instance-id-to-migrations"), KnownVersion::new(76, "lookup-region-snapshot-by-snapshot-id"), KnownVersion::new(75, "add-cockroach-zone-id-to-node-id"), KnownVersion::new(74, "add-migration-table"), diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index 6856288302..e723101242 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -4159,7 +4159,7 @@ INSERT INTO omicron.public.db_metadata ( version, target_version ) VALUES - (TRUE, NOW(), NOW(), '76.0.0', NULL) + (TRUE, NOW(), NOW(), '77.0.0', NULL) ON CONFLICT DO NOTHING; COMMIT; From 346dca3a2c1bd00cdc82a1309b98f2faed2c6795 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Fri, 28 Jun 2024 12:57:27 -0700 Subject: [PATCH 9/9] remove time deleted assertion --- nexus/tests/integration_tests/instances.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/nexus/tests/integration_tests/instances.rs b/nexus/tests/integration_tests/instances.rs index 4f7a1d1b77..2cc9f77c69 100644 --- a/nexus/tests/integration_tests/instances.rs +++ b/nexus/tests/integration_tests/instances.rs @@ -832,7 +832,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]