diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index 5b296d4693b..5c69940e621 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -1087,6 +1087,7 @@ pub struct IdentityMetadataUpdateParams { Debug, Deserialize, Eq, + Hash, Ord, PartialEq, PartialOrd, @@ -1341,14 +1342,20 @@ pub enum FailureDomain { #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema, PartialEq)] #[serde(tag = "type", content = "value", rename_all = "snake_case")] pub enum AffinityGroupMember { - /// An instance belonging to this group, identified by UUID. - Instance(InstanceUuid), + /// An instance belonging to this group + Instance { id: InstanceUuid, name: Name, run_state: InstanceState }, } -impl SimpleIdentity for AffinityGroupMember { +impl SimpleIdentityOrName for AffinityGroupMember { fn id(&self) -> Uuid { match self { - AffinityGroupMember::Instance(id) => *id.as_untyped_uuid(), + AffinityGroupMember::Instance { id, .. } => *id.as_untyped_uuid(), + } + } + + fn name(&self) -> &Name { + match self { + AffinityGroupMember::Instance { name, .. } => name, } } } @@ -1360,14 +1367,22 @@ impl SimpleIdentity for AffinityGroupMember { #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema, PartialEq)] #[serde(tag = "type", content = "value", rename_all = "snake_case")] pub enum AntiAffinityGroupMember { - /// An instance belonging to this group, identified by UUID. - Instance(InstanceUuid), + /// An instance belonging to this group + Instance { id: InstanceUuid, name: Name, run_state: InstanceState }, } -impl SimpleIdentity for AntiAffinityGroupMember { +impl SimpleIdentityOrName for AntiAffinityGroupMember { fn id(&self) -> Uuid { match self { - AntiAffinityGroupMember::Instance(id) => *id.as_untyped_uuid(), + AntiAffinityGroupMember::Instance { id, .. } => { + *id.as_untyped_uuid() + } + } + } + + fn name(&self) -> &Name { + match self { + AntiAffinityGroupMember::Instance { name, .. } => name, } } } diff --git a/dev-tools/omdb/src/bin/omdb/db.rs b/dev-tools/omdb/src/bin/omdb/db.rs index ac06776327f..cd4b579583f 100644 --- a/dev-tools/omdb/src/bin/omdb/db.rs +++ b/dev-tools/omdb/src/bin/omdb/db.rs @@ -106,6 +106,7 @@ use nexus_db_queries::db::DataStore; use nexus_db_queries::db::datastore::CrucibleTargets; use nexus_db_queries::db::datastore::DataStoreConnection; use nexus_db_queries::db::datastore::InstanceAndActiveVmm; +use nexus_db_queries::db::datastore::InstanceStateComputer; use nexus_db_queries::db::datastore::SQL_BATCH_SIZE; use nexus_db_queries::db::datastore::VolumeCookedResult; use nexus_db_queries::db::datastore::read_only_resources_associated_with_volume; @@ -3810,10 +3811,9 @@ async fn cmd_db_instance_info( time_last_auto_restarted, } = instance.runtime_state; println!(" {STATE:>WIDTH$}: {nexus_state:?}"); - let effective_state = InstanceAndActiveVmm::determine_effective_state( - &instance, - active_vmm.as_ref(), - ); + let effective_state = + InstanceStateComputer::new(&instance, active_vmm.as_ref()) + .compute_state(); println!( "{} {API_STATE:>WIDTH$}: {effective_state:?}", if effective_state == InstanceState::Failed { "/!\\" } else { "(i)" } diff --git a/nexus/db-model/src/affinity.rs b/nexus/db-model/src/affinity.rs index 596039221b4..55a085e905d 100644 --- a/nexus/db-model/src/affinity.rs +++ b/nexus/db-model/src/affinity.rs @@ -228,11 +228,17 @@ impl AffinityGroupInstanceMembership { pub fn new(group_id: AffinityGroupUuid, instance_id: InstanceUuid) -> Self { Self { group_id: group_id.into(), instance_id: instance_id.into() } } -} -impl From for external::AffinityGroupMember { - fn from(member: AffinityGroupInstanceMembership) -> Self { - Self::Instance(member.instance_id.into()) + pub fn to_external( + self, + member_name: external::Name, + run_state: external::InstanceState, + ) -> external::AffinityGroupMember { + external::AffinityGroupMember::Instance { + id: self.instance_id.into(), + name: member_name, + run_state, + } } } @@ -250,12 +256,16 @@ impl AntiAffinityGroupInstanceMembership { ) -> Self { Self { group_id: group_id.into(), instance_id: instance_id.into() } } -} -impl From - for external::AntiAffinityGroupMember -{ - fn from(member: AntiAffinityGroupInstanceMembership) -> Self { - Self::Instance(member.instance_id.into()) + pub fn to_external( + self, + member_name: external::Name, + run_state: external::InstanceState, + ) -> external::AntiAffinityGroupMember { + external::AntiAffinityGroupMember::Instance { + id: self.instance_id.into(), + name: member_name, + run_state, + } } } diff --git a/nexus/db-queries/src/db/datastore/affinity.rs b/nexus/db-queries/src/db/datastore/affinity.rs index c3839e9d2e5..b9f784acea1 100644 --- a/nexus/db-queries/src/db/datastore/affinity.rs +++ b/nexus/db-queries/src/db/datastore/affinity.rs @@ -10,6 +10,7 @@ use crate::authz::ApiResource; use crate::db; use crate::db::collection_insert::AsyncInsertError; use crate::db::collection_insert::DatastoreCollection; +use crate::db::datastore::InstanceStateComputer; use crate::db::datastore::OpContext; use crate::db::error::ErrorHandler; use crate::db::error::public_error_from_diesel; @@ -20,15 +21,21 @@ use crate::db::model::AffinityGroupUpdate; use crate::db::model::AntiAffinityGroup; use crate::db::model::AntiAffinityGroupInstanceMembership; use crate::db::model::AntiAffinityGroupUpdate; +use crate::db::model::InstanceState; +use crate::db::model::InstanceStateEnum; use crate::db::model::Name; use crate::db::model::Project; +use crate::db::model::VmmState; +use crate::db::model::VmmStateEnum; use crate::db::pagination::paginated; +use crate::db::raw_query_builder::QueryBuilder; use crate::transaction_retry::OptionalError; use async_bb8_diesel::AsyncRunQueryDsl; use chrono::Utc; use diesel::prelude::*; use omicron_common::api::external; use omicron_common::api::external::CreateResult; +use omicron_common::api::external::DataPageParams; use omicron_common::api::external::DeleteResult; use omicron_common::api::external::Error; use omicron_common::api::external::ListResultVec; @@ -41,6 +48,7 @@ use omicron_uuid_kinds::AntiAffinityGroupUuid; use omicron_uuid_kinds::GenericUuid; use omicron_uuid_kinds::InstanceUuid; use ref_cast::RefCast; +use uuid::Uuid; impl DataStore { pub async fn affinity_group_list( @@ -341,27 +349,104 @@ impl DataStore { opctx: &OpContext, authz_affinity_group: &authz::AffinityGroup, pagparams: &PaginatedBy<'_>, - ) -> ListResultVec { + ) -> ListResultVec { opctx.authorize(authz::Action::Read, authz_affinity_group).await?; - use db::schema::affinity_group_instance_membership::dsl; + let mut query = QueryBuilder::new() + .sql( + " + SELECT * FROM ( + SELECT instance.id as id, + instance.name as name, + instance.state, + instance.migration_id, + vmm.state + FROM affinity_group_instance_membership + INNER JOIN instance + ON instance.id = affinity_group_instance_membership.instance_id + LEFT JOIN vmm + ON instance.active_propolis_id = vmm.id + WHERE + instance.time_deleted IS NULL AND + vmm.time_deleted IS NULL AND + group_id = ", + ) + .param() + .bind::(authz_affinity_group.id()) + .sql(") "); + + let (direction, limit) = match pagparams { + PaginatedBy::Id(p) => (p.direction, p.limit), + PaginatedBy::Name(p) => (p.direction, p.limit), + }; + let asc = match direction { + dropshot::PaginationOrder::Ascending => true, + dropshot::PaginationOrder::Descending => false, + }; + match pagparams { - PaginatedBy::Id(pagparams) => paginated( - dsl::affinity_group_instance_membership, - dsl::instance_id, - &pagparams, - ), - PaginatedBy::Name(_) => { - return Err(Error::invalid_request( - "Cannot paginate group members by name", - )); + PaginatedBy::Id(DataPageParams { marker, .. }) => { + if let Some(id) = marker { + query = query + .sql("WHERE id ") + .sql(if asc { ">" } else { "<" }) + .sql(" ") + .param() + .bind::(**id); + }; + query = query.sql(" ORDER BY id "); + } + PaginatedBy::Name(DataPageParams { marker, .. }) => { + if let Some(name) = marker { + query = query + .sql("WHERE name ") + .sql(if asc { ">" } else { "<" }) + .sql(" ") + .param() + .bind::(Name( + (*name).clone(), + )); + }; + query = query.sql(" ORDER BY name "); } } - .filter(dsl::group_id.eq(authz_affinity_group.id())) - .select(AffinityGroupInstanceMembership::as_select()) - .load_async(&*self.pool_connection_authorized(opctx).await?) - .await - .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) + if asc { + query = query.sql("ASC "); + } else { + query = query.sql("DESC "); + } + + query = query + .sql(" LIMIT ") + .param() + .bind::(i64::from(limit.get())); + + query + .query::<( + diesel::sql_types::Uuid, + diesel::sql_types::Text, + InstanceStateEnum, + diesel::sql_types::Nullable, + diesel::sql_types::Nullable, + )>() + .load_async::<(Uuid, Name, InstanceState, Option, Option)>( + &*self.pool_connection_authorized(opctx).await?, + ) + .await + .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))? + .into_iter() + .map(|(id, name, instance_state, migration_id, vmm_state)| { + Ok(external::AffinityGroupMember::Instance { + id: InstanceUuid::from_untyped_uuid(id), + name: name.into(), + run_state: InstanceStateComputer::compute_state_from( + &instance_state, + migration_id.as_ref(), + vmm_state.as_ref(), + ), + }) + }) + .collect() } pub async fn anti_affinity_group_member_list( @@ -369,50 +454,165 @@ impl DataStore { opctx: &OpContext, authz_anti_affinity_group: &authz::AntiAffinityGroup, pagparams: &PaginatedBy<'_>, - ) -> ListResultVec { + ) -> ListResultVec { opctx.authorize(authz::Action::Read, authz_anti_affinity_group).await?; - use db::schema::anti_affinity_group_instance_membership::dsl; + let (direction, limit) = match pagparams { + PaginatedBy::Id(p) => (p.direction, p.limit), + PaginatedBy::Name(p) => (p.direction, p.limit), + }; + let asc = match direction { + dropshot::PaginationOrder::Ascending => true, + dropshot::PaginationOrder::Descending => false, + }; + + let mut query = QueryBuilder::new() + .sql( + "SELECT id,name,instance_state,migration_id,vmm_state + FROM ( + SELECT + instance.id as id, + instance.name as name, + instance.state as instance_state, + instance.migration_id as migration_id, + vmm.state as vmm_state + FROM anti_affinity_group_instance_membership + INNER JOIN instance + ON instance.id = anti_affinity_group_instance_membership.instance_id + LEFT JOIN vmm + ON instance.active_propolis_id = vmm.id + WHERE + instance.time_deleted IS NULL AND + vmm.time_deleted IS NULL AND + group_id = ", + ) + .param() + .bind::(authz_anti_affinity_group.id()) + .sql(") "); + match pagparams { - PaginatedBy::Id(pagparams) => paginated( - dsl::anti_affinity_group_instance_membership, - dsl::instance_id, - &pagparams, - ), - PaginatedBy::Name(_) => { - return Err(Error::invalid_request( - "Cannot paginate group members by name", - )); + PaginatedBy::Id(DataPageParams { marker, .. }) => { + if let Some(id) = marker { + query = query + .sql("WHERE id ") + .sql(if asc { ">" } else { "<" }) + .sql(" ") + .param() + .bind::(**id); + }; + query = query.sql(" ORDER BY id "); + } + PaginatedBy::Name(DataPageParams { marker, .. }) => { + if let Some(name) = marker { + query = query + .sql("WHERE name ") + .sql(if asc { ">" } else { "<" }) + .sql(" ") + .param() + .bind::(Name( + (*name).clone(), + )); + }; + query = query.sql(" ORDER BY name "); } } - .filter(dsl::group_id.eq(authz_anti_affinity_group.id())) - .select(AntiAffinityGroupInstanceMembership::as_select()) - .load_async(&*self.pool_connection_authorized(opctx).await?) - .await - .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) + if asc { + query = query.sql("ASC "); + } else { + query = query.sql("DESC "); + } + + query = query + .sql(" LIMIT ") + .param() + .bind::(i64::from(limit.get())); + + query + .query::<( + diesel::sql_types::Uuid, + diesel::sql_types::Text, + diesel::sql_types::Nullable, + diesel::sql_types::Nullable, + diesel::sql_types::Nullable, + )>() + .load_async::<( + Uuid, + Name, + Option, + Option, + Option, + )>( + &*self.pool_connection_authorized(opctx).await?, + ) + .await + .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))? + .into_iter() + .map(|(id, name, instance_state, migration_id, vmm_state)| { + let Some(instance_state) = instance_state else { + return Err(external::Error::internal_error( + "Anti-Affinity instance member missing state in database" + )); + }; + Ok(external::AntiAffinityGroupMember::Instance { + id: InstanceUuid::from_untyped_uuid(id), + name: name.into(), + run_state: InstanceStateComputer::compute_state_from( + &instance_state, + migration_id.as_ref(), + vmm_state.as_ref(), + ), + }) + }) + .collect() } - pub async fn affinity_group_member_view( + pub async fn affinity_group_member_instance_view( &self, opctx: &OpContext, authz_affinity_group: &authz::AffinityGroup, - member: external::AffinityGroupMember, + instance_id: InstanceUuid, ) -> Result { opctx.authorize(authz::Action::Read, authz_affinity_group).await?; let conn = self.pool_connection_authorized(opctx).await?; - let instance_id = match member { - external::AffinityGroupMember::Instance(id) => id, - }; - use db::schema::affinity_group_instance_membership::dsl; + use db::schema::instance::dsl as instance_dsl; + use db::schema::vmm::dsl as vmm_dsl; dsl::affinity_group_instance_membership .filter(dsl::group_id.eq(authz_affinity_group.id())) .filter(dsl::instance_id.eq(instance_id.into_untyped_uuid())) - .select(AffinityGroupInstanceMembership::as_select()) - .get_result_async(&*conn) - .await - .map(|m| m.into()) + .inner_join( + instance_dsl::instance + .on(instance_dsl::id.eq(dsl::instance_id)), + ) + .filter(instance_dsl::time_deleted.is_null()) + .left_join(vmm_dsl::vmm.on( + instance_dsl::active_propolis_id.eq(vmm_dsl::id.nullable()), + )) + .filter(vmm_dsl::time_deleted.is_null()) + .select(( + AffinityGroupInstanceMembership::as_select(), + instance_dsl::name, + instance_dsl::state, + instance_dsl::migration_id, + vmm_dsl::state.nullable(), + )) + .get_result_async::<( + AffinityGroupInstanceMembership, + Name, + InstanceState, + Option, + Option, + )>(&*conn) + .await + .map(|(member, name, instance_state, migration_id, vmm_state)| { + let run_state = InstanceStateComputer::compute_state_from( + &instance_state, + migration_id.as_ref(), + vmm_state.as_ref(), + ); + member.to_external(name.into(), run_state) + }) .map_err(|e| { public_error_from_diesel( e, @@ -424,27 +624,53 @@ impl DataStore { }) } - pub async fn anti_affinity_group_member_view( + pub async fn anti_affinity_group_member_instance_view( &self, opctx: &OpContext, authz_anti_affinity_group: &authz::AntiAffinityGroup, - member: external::AntiAffinityGroupMember, + instance_id: InstanceUuid, ) -> Result { opctx.authorize(authz::Action::Read, authz_anti_affinity_group).await?; let conn = self.pool_connection_authorized(opctx).await?; - let instance_id = match member { - external::AntiAffinityGroupMember::Instance(id) => id, - }; - use db::schema::anti_affinity_group_instance_membership::dsl; + use db::schema::instance::dsl as instance_dsl; + use db::schema::vmm::dsl as vmm_dsl; dsl::anti_affinity_group_instance_membership .filter(dsl::group_id.eq(authz_anti_affinity_group.id())) .filter(dsl::instance_id.eq(instance_id.into_untyped_uuid())) - .select(AntiAffinityGroupInstanceMembership::as_select()) - .get_result_async(&*conn) - .await - .map(|m| m.into()) + .inner_join( + instance_dsl::instance + .on(instance_dsl::id.eq(dsl::instance_id)), + ) + .filter(instance_dsl::time_deleted.is_null()) + .left_join(vmm_dsl::vmm.on( + instance_dsl::active_propolis_id.eq(vmm_dsl::id.nullable()), + )) + .filter(vmm_dsl::time_deleted.is_null()) + .select(( + AntiAffinityGroupInstanceMembership::as_select(), + instance_dsl::name, + instance_dsl::state, + instance_dsl::migration_id, + vmm_dsl::state.nullable(), + )) + .get_result_async::<( + AntiAffinityGroupInstanceMembership, + Name, + InstanceState, + Option, + Option, + )>(&*conn) + .await + .map(|(member, name, instance_state, migration_id, vmm_state)| { + let run_state = InstanceStateComputer::compute_state_from( + &instance_state, + migration_id.as_ref(), + vmm_state.as_ref(), + ); + member.to_external(name.into(), run_state) + }) .map_err(|e| { public_error_from_diesel( e, @@ -456,21 +682,17 @@ impl DataStore { }) } - pub async fn affinity_group_member_add( + pub async fn affinity_group_member_instance_add( &self, opctx: &OpContext, authz_affinity_group: &authz::AffinityGroup, - member: external::AffinityGroupMember, + instance_id: InstanceUuid, ) -> Result<(), Error> { opctx.authorize(authz::Action::Modify, authz_affinity_group).await?; - let instance_id = match member { - external::AffinityGroupMember::Instance(id) => id, - }; - let err = OptionalError::new(); let conn = self.pool_connection_authorized(opctx).await?; - self.transaction_retry_wrapper("affinity_group_member_add") + self.transaction_retry_wrapper("affinity_group_member_instance_add") .transaction(&conn, |conn| { let err = err.clone(); use db::schema::affinity_group::dsl as group_dsl; @@ -583,23 +805,19 @@ impl DataStore { Ok(()) } - pub async fn anti_affinity_group_member_add( + pub async fn anti_affinity_group_member_instance_add( &self, opctx: &OpContext, authz_anti_affinity_group: &authz::AntiAffinityGroup, - member: external::AntiAffinityGroupMember, + instance_id: InstanceUuid, ) -> Result<(), Error> { opctx .authorize(authz::Action::Modify, authz_anti_affinity_group) .await?; - let instance_id = match member { - external::AntiAffinityGroupMember::Instance(id) => id, - }; - let err = OptionalError::new(); let conn = self.pool_connection_authorized(opctx).await?; - self.transaction_retry_wrapper("anti_affinity_group_member_add") + self.transaction_retry_wrapper("anti_affinity_group_member_instance_add") .transaction(&conn, |conn| { let err = err.clone(); use db::schema::anti_affinity_group::dsl as group_dsl; @@ -733,21 +951,17 @@ impl DataStore { Ok(()) } - pub async fn affinity_group_member_delete( + pub async fn affinity_group_member_instance_delete( &self, opctx: &OpContext, authz_affinity_group: &authz::AffinityGroup, - member: external::AffinityGroupMember, + instance_id: InstanceUuid, ) -> Result<(), Error> { opctx.authorize(authz::Action::Modify, authz_affinity_group).await?; - let instance_id = match member { - external::AffinityGroupMember::Instance(id) => id, - }; - let err = OptionalError::new(); let conn = self.pool_connection_authorized(opctx).await?; - self.transaction_retry_wrapper("affinity_group_member_delete") + self.transaction_retry_wrapper("affinity_group_member_instance_delete") .transaction(&conn, |conn| { let err = err.clone(); use db::schema::affinity_group::dsl as group_dsl; @@ -800,23 +1014,20 @@ impl DataStore { Ok(()) } - pub async fn anti_affinity_group_member_delete( + /// Deletes an anti-affinity member, when that member is an instance + pub async fn anti_affinity_group_member_instance_delete( &self, opctx: &OpContext, authz_anti_affinity_group: &authz::AntiAffinityGroup, - member: external::AntiAffinityGroupMember, + instance_id: InstanceUuid, ) -> Result<(), Error> { opctx .authorize(authz::Action::Modify, authz_anti_affinity_group) .await?; - let instance_id = match member { - external::AntiAffinityGroupMember::Instance(id) => id, - }; - let err = OptionalError::new(); let conn = self.pool_connection_authorized(opctx).await?; - self.transaction_retry_wrapper("anti_affinity_group_member_delete") + self.transaction_retry_wrapper("anti_affinity_group_member_instance_delete") .transaction(&conn, |conn| { let err = err.clone(); use db::schema::anti_affinity_group::dsl as group_dsl; @@ -883,6 +1094,7 @@ mod tests { use nexus_types::external_api::params; use omicron_common::api::external::{ self, ByteCount, DataPageParams, IdentityMetadataCreateParams, + SimpleIdentityOrName, }; use omicron_test_utils::dev; use omicron_uuid_kinds::GenericUuid; @@ -1415,11 +1627,7 @@ mod tests { // Add the instance as a member to the group datastore - .affinity_group_member_add( - &opctx, - &authz_group, - external::AffinityGroupMember::Instance(instance), - ) + .affinity_group_member_instance_add(&opctx, &authz_group, instance) .await .unwrap(); @@ -1427,19 +1635,19 @@ mod tests { let members = datastore .affinity_group_member_list(&opctx, &authz_group, &pagbyid) .await - .unwrap(); + .unwrap() + .into_iter() + .map(|m| InstanceUuid::from_untyped_uuid(m.id())) + .collect::>(); assert_eq!(members.len(), 1); - assert_eq!( - external::AffinityGroupMember::Instance(instance,), - members[0].clone().into() - ); + assert_eq!(instance, members[0]); // We can delete the member and observe an empty member list datastore - .affinity_group_member_delete( + .affinity_group_member_instance_delete( &opctx, &authz_group, - external::AffinityGroupMember::Instance(instance), + instance, ) .await .unwrap(); @@ -1482,14 +1690,13 @@ mod tests { .unwrap(); // A new group should have no members - let pagparams_id = DataPageParams { + let pagparams = PaginatedBy::Id(DataPageParams { marker: None, limit: NonZeroU32::new(100).unwrap(), direction: dropshot::PaginationOrder::Ascending, - }; - let pagbyid = PaginatedBy::Id(pagparams_id); + }); let members = datastore - .anti_affinity_group_member_list(&opctx, &authz_group, &pagbyid) + .anti_affinity_group_member_list(&opctx, &authz_group, &pagparams) .await .unwrap(); assert!(members.is_empty()); @@ -1505,40 +1712,419 @@ mod tests { // Add the instance as a member to the group datastore - .anti_affinity_group_member_add( + .anti_affinity_group_member_instance_add( &opctx, &authz_group, - external::AntiAffinityGroupMember::Instance(instance), + instance, ) .await .unwrap(); // We should now be able to list the new member let members = datastore - .anti_affinity_group_member_list(&opctx, &authz_group, &pagbyid) + .anti_affinity_group_member_list(&opctx, &authz_group, &pagparams) .await .unwrap(); assert_eq!(members.len(), 1); - assert_eq!( - external::AntiAffinityGroupMember::Instance(instance,), - members[0].clone().into() - ); + assert!(matches!( + members[0], + external::AntiAffinityGroupMember::Instance { + id, + .. + } if id == instance, + )); // We can delete the member and observe an empty member list datastore - .anti_affinity_group_member_delete( + .anti_affinity_group_member_instance_delete( &opctx, &authz_group, - external::AntiAffinityGroupMember::Instance(instance), + instance, + ) + .await + .unwrap(); + let members = datastore + .anti_affinity_group_member_list(&opctx, &authz_group, &pagparams) + .await + .unwrap(); + assert!(members.is_empty()); + + // Clean up. + db.terminate().await; + logctx.cleanup_successful(); + } + + #[tokio::test] + async fn affinity_group_membership_list_extended() { + // Setup + let logctx = + dev::test_setup_log("affinity_group_membership_list_extended"); + let db = TestDatabase::new_with_datastore(&logctx.log).await; + let (opctx, datastore) = (db.opctx(), db.datastore()); + + // Create a project and a group + let (authz_project, ..) = + create_project(&opctx, &datastore, "my-project").await; + let group = create_affinity_group( + &opctx, + &datastore, + &authz_project, + "my-group", + ) + .await + .unwrap(); + + let (.., authz_group) = LookupPath::new(opctx, datastore) + .affinity_group_id(group.id()) + .lookup_for(authz::Action::Modify) + .await + .unwrap(); + + // A new group should have no members + let pagparams = PaginatedBy::Id(DataPageParams { + marker: None, + limit: NonZeroU32::new(100).unwrap(), + direction: dropshot::PaginationOrder::Ascending, + }); + let members = datastore + .affinity_group_member_list(&opctx, &authz_group, &pagparams) + .await + .unwrap(); + assert!(members.is_empty()); + + // Add some instances, so we have data to list over. + + const INSTANCE_COUNT: usize = 6; + + let mut members = Vec::new(); + for i in 0..INSTANCE_COUNT { + let name = format!("instance-{i}"); + let instance = create_stopped_instance_record( + &opctx, + &datastore, + &authz_project, + &name, ) + .await; + + // Add the instance as a member to the group + let member = external::AffinityGroupMember::Instance { + id: instance, + name: name.try_into().unwrap(), + run_state: external::InstanceState::Stopped, + }; + datastore + .affinity_group_member_instance_add( + &opctx, + &authz_group, + instance, + ) + .await + .unwrap(); + members.push(member); + } + + // Order by UUID + members.sort_unstable_by_key(|m1| m1.id()); + + // We can list all members + let pagparams = PaginatedBy::Id(DataPageParams { + marker: None, + limit: NonZeroU32::new(100).unwrap(), + direction: dropshot::PaginationOrder::Ascending, + }); + let observed_members = datastore + .affinity_group_member_list(&opctx, &authz_group, &pagparams) + .await + .unwrap(); + assert_eq!(observed_members, members); + + // We can paginate over the results + let marker = members[2].id(); + let pagparams = PaginatedBy::Id(DataPageParams { + marker: Some(&marker), + limit: NonZeroU32::new(100).unwrap(), + direction: dropshot::PaginationOrder::Ascending, + }); + + let observed_members = datastore + .affinity_group_member_list(&opctx, &authz_group, &pagparams) + .await + .unwrap(); + assert_eq!(observed_members, members[3..]); + + // We can list limited results + let pagparams = PaginatedBy::Id(DataPageParams { + marker: Some(&marker), + limit: NonZeroU32::new(2).unwrap(), + direction: dropshot::PaginationOrder::Ascending, + }); + let observed_members = datastore + .affinity_group_member_list(&opctx, &authz_group, &pagparams) + .await + .unwrap(); + assert_eq!(observed_members, members[3..5]); + + // We can list in descending order too + members.reverse(); + let pagparams = PaginatedBy::Id(DataPageParams { + marker: None, + limit: NonZeroU32::new(100).unwrap(), + direction: dropshot::PaginationOrder::Descending, + }); + let observed_members = datastore + .affinity_group_member_list(&opctx, &authz_group, &pagparams) + .await + .unwrap(); + assert_eq!(observed_members, members); + + // Order by name + members.sort_unstable_by_key(|m1| m1.name().clone()); + + // We can list all members + let pagparams = PaginatedBy::Name(DataPageParams { + marker: None, + limit: NonZeroU32::new(100).unwrap(), + direction: dropshot::PaginationOrder::Ascending, + }); + let observed_members = datastore + .affinity_group_member_list(&opctx, &authz_group, &pagparams) + .await + .unwrap(); + assert_eq!(observed_members, members); + let marker = members[2].name(); + let pagparams = PaginatedBy::Name(DataPageParams { + marker: Some(marker), + limit: NonZeroU32::new(100).unwrap(), + direction: dropshot::PaginationOrder::Ascending, + }); + + let observed_members = datastore + .affinity_group_member_list(&opctx, &authz_group, &pagparams) + .await + .unwrap(); + assert_eq!(observed_members, members[3..]); + + // We can list in descending order too + members.reverse(); + let pagparams = PaginatedBy::Name(DataPageParams { + marker: None, + limit: NonZeroU32::new(100).unwrap(), + direction: dropshot::PaginationOrder::Descending, + }); + let observed_members = datastore + .affinity_group_member_list(&opctx, &authz_group, &pagparams) + .await + .unwrap(); + assert_eq!(observed_members, members); + + // Clean up. + db.terminate().await; + logctx.cleanup_successful(); + } + + // Anti-affinity group member listing has a slightly more complicated + // implementation, because it queries multiple tables and JOINs them + // together. + // + // This test exists to validate that manual implementation. + #[tokio::test] + async fn anti_affinity_group_membership_list_extended() { + // Setup + let logctx = + dev::test_setup_log("anti_affinity_group_membership_list_extended"); + let db = TestDatabase::new_with_datastore(&logctx.log).await; + let (opctx, datastore) = (db.opctx(), db.datastore()); + + // Create a project and a group + let (authz_project, ..) = + create_project(&opctx, &datastore, "my-project").await; + let group = create_anti_affinity_group( + &opctx, + &datastore, + &authz_project, + "my-group", + ) + .await + .unwrap(); + + let (.., authz_aa_group) = LookupPath::new(opctx, datastore) + .anti_affinity_group_id(group.id()) + .lookup_for(authz::Action::Modify) .await .unwrap(); + + // A new group should have no members + let pagparams = PaginatedBy::Id(DataPageParams { + marker: None, + limit: NonZeroU32::new(100).unwrap(), + direction: dropshot::PaginationOrder::Ascending, + }); let members = datastore - .anti_affinity_group_member_list(&opctx, &authz_group, &pagbyid) + .anti_affinity_group_member_list( + &opctx, + &authz_aa_group, + &pagparams, + ) .await .unwrap(); assert!(members.is_empty()); + // Add some instances, so we have data to list over. + + const INSTANCE_COUNT: usize = 6; + + let mut members = Vec::new(); + + for i in 0..INSTANCE_COUNT { + let name = format!("instance-{i}"); + let instance = create_stopped_instance_record( + &opctx, + &datastore, + &authz_project, + &name, + ) + .await; + + // Add the instance as a member to the group + let member = external::AntiAffinityGroupMember::Instance { + id: instance, + name: name.try_into().unwrap(), + run_state: external::InstanceState::Stopped, + }; + datastore + .anti_affinity_group_member_instance_add( + &opctx, + &authz_aa_group, + instance, + ) + .await + .unwrap(); + members.push(member); + } + + // Order by UUID + members.sort_unstable_by_key(|m1| m1.id()); + + // We can list all members + let pagparams = PaginatedBy::Id(DataPageParams { + marker: None, + limit: NonZeroU32::new(100).unwrap(), + direction: dropshot::PaginationOrder::Ascending, + }); + let observed_members = datastore + .anti_affinity_group_member_list( + &opctx, + &authz_aa_group, + &pagparams, + ) + .await + .unwrap(); + assert_eq!(observed_members, members); + + // We can paginate over the results + let marker = members[2].id(); + let pagparams = PaginatedBy::Id(DataPageParams { + marker: Some(&marker), + limit: NonZeroU32::new(100).unwrap(), + direction: dropshot::PaginationOrder::Ascending, + }); + + let observed_members = datastore + .anti_affinity_group_member_list( + &opctx, + &authz_aa_group, + &pagparams, + ) + .await + .unwrap(); + assert_eq!(observed_members, members[3..]); + + // We can list limited results + let pagparams = PaginatedBy::Id(DataPageParams { + marker: Some(&marker), + limit: NonZeroU32::new(2).unwrap(), + direction: dropshot::PaginationOrder::Ascending, + }); + let observed_members = datastore + .anti_affinity_group_member_list( + &opctx, + &authz_aa_group, + &pagparams, + ) + .await + .unwrap(); + assert_eq!(observed_members, members[3..5]); + + // We can list in descending order too + members.reverse(); + let pagparams = PaginatedBy::Id(DataPageParams { + marker: None, + limit: NonZeroU32::new(100).unwrap(), + direction: dropshot::PaginationOrder::Descending, + }); + let observed_members = datastore + .anti_affinity_group_member_list( + &opctx, + &authz_aa_group, + &pagparams, + ) + .await + .unwrap(); + assert_eq!(observed_members, members); + + // Order by name + members.sort_unstable_by_key(|m1| m1.name().clone()); + + // We can list all members + let pagparams = PaginatedBy::Name(DataPageParams { + marker: None, + limit: NonZeroU32::new(100).unwrap(), + direction: dropshot::PaginationOrder::Ascending, + }); + let observed_members = datastore + .anti_affinity_group_member_list( + &opctx, + &authz_aa_group, + &pagparams, + ) + .await + .unwrap(); + assert_eq!(observed_members, members); + let marker = members[2].name(); + let pagparams = PaginatedBy::Name(DataPageParams { + marker: Some(marker), + limit: NonZeroU32::new(100).unwrap(), + direction: dropshot::PaginationOrder::Ascending, + }); + + let observed_members = datastore + .anti_affinity_group_member_list( + &opctx, + &authz_aa_group, + &pagparams, + ) + .await + .unwrap(); + assert_eq!(observed_members, members[3..]); + + // We can list in descending order too + members.reverse(); + let pagparams = PaginatedBy::Name(DataPageParams { + marker: None, + limit: NonZeroU32::new(100).unwrap(), + direction: dropshot::PaginationOrder::Descending, + }); + let observed_members = datastore + .anti_affinity_group_member_list( + &opctx, + &authz_aa_group, + &pagparams, + ) + .await + .unwrap(); + assert_eq!(observed_members, members); + // Clean up. db.terminate().await; logctx.cleanup_successful(); @@ -1572,12 +2158,11 @@ mod tests { .unwrap(); // A new group should have no members - let pagparams_id = DataPageParams { + let pagbyid = PaginatedBy::Id(DataPageParams { marker: None, limit: NonZeroU32::new(100).unwrap(), direction: dropshot::PaginationOrder::Ascending, - }; - let pagbyid = PaginatedBy::Id(pagparams_id); + }); let members = datastore .affinity_group_member_list(&opctx, &authz_group, &pagbyid) .await @@ -1597,11 +2182,7 @@ mod tests { // Cannot add the instance to the group while it's running. let err = datastore - .affinity_group_member_add( - &opctx, - &authz_group, - external::AffinityGroupMember::Instance(instance), - ) + .affinity_group_member_instance_add(&opctx, &authz_group, instance) .await .expect_err( "Shouldn't be able to add running instances to affinity groups", @@ -1617,11 +2198,7 @@ mod tests { // If we have no reservation for the instance, we can add it to the group. delete_instance_reservation(&datastore, instance).await; datastore - .affinity_group_member_add( - &opctx, - &authz_group, - external::AffinityGroupMember::Instance(instance), - ) + .affinity_group_member_instance_add(&opctx, &authz_group, instance) .await .unwrap(); @@ -1632,20 +2209,20 @@ mod tests { let members = datastore .affinity_group_member_list(&opctx, &authz_group, &pagbyid) .await - .unwrap(); + .unwrap() + .into_iter() + .map(|m| InstanceUuid::from_untyped_uuid(m.id())) + .collect::>(); assert_eq!(members.len(), 1); - assert_eq!( - external::AffinityGroupMember::Instance(instance,), - members[0].clone().into() - ); + assert_eq!(instance, members[0]); // We can delete the member and observe an empty member list -- even // though it's running! datastore - .affinity_group_member_delete( + .affinity_group_member_instance_delete( &opctx, &authz_group, - external::AffinityGroupMember::Instance(instance), + instance, ) .await .unwrap(); @@ -1688,14 +2265,13 @@ mod tests { .unwrap(); // A new group should have no members - let pagparams_id = DataPageParams { + let pagparams = PaginatedBy::Id(DataPageParams { marker: None, limit: NonZeroU32::new(100).unwrap(), direction: dropshot::PaginationOrder::Ascending, - }; - let pagbyid = PaginatedBy::Id(pagparams_id); + }); let members = datastore - .anti_affinity_group_member_list(&opctx, &authz_group, &pagbyid) + .anti_affinity_group_member_list(&opctx, &authz_group, &pagparams) .await .unwrap(); assert!(members.is_empty()); @@ -1712,10 +2288,10 @@ mod tests { // Cannot add the instance to the group while it's running. let err = datastore - .anti_affinity_group_member_add( + .anti_affinity_group_member_instance_add( &opctx, &authz_group, - external::AntiAffinityGroupMember::Instance(instance), + instance, ) .await .expect_err( @@ -1732,10 +2308,10 @@ mod tests { // If we have no reservation for the instance, we can add it to the group. delete_instance_reservation(&datastore, instance).await; datastore - .anti_affinity_group_member_add( + .anti_affinity_group_member_instance_add( &opctx, &authz_group, - external::AntiAffinityGroupMember::Instance(instance), + instance, ) .await .unwrap(); @@ -1745,27 +2321,29 @@ mod tests { // We should now be able to list the new member let members = datastore - .anti_affinity_group_member_list(&opctx, &authz_group, &pagbyid) + .anti_affinity_group_member_list(&opctx, &authz_group, &pagparams) .await .unwrap(); assert_eq!(members.len(), 1); - assert_eq!( - external::AntiAffinityGroupMember::Instance(instance,), - members[0].clone().into() - ); - + assert!(matches!( + members[0], + external::AntiAffinityGroupMember::Instance { + id, + .. + } if id == instance, + )); // We can delete the member and observe an empty member list -- even // though it's running! datastore - .anti_affinity_group_member_delete( + .anti_affinity_group_member_instance_delete( &opctx, &authz_group, - external::AntiAffinityGroupMember::Instance(instance), + instance, ) .await .unwrap(); let members = datastore - .anti_affinity_group_member_list(&opctx, &authz_group, &pagbyid) + .anti_affinity_group_member_list(&opctx, &authz_group, &pagparams) .await .unwrap(); assert!(members.is_empty()); @@ -1802,12 +2380,11 @@ mod tests { .unwrap(); // A new group should have no members - let pagparams_id = DataPageParams { + let pagbyid = PaginatedBy::Id(DataPageParams { marker: None, limit: NonZeroU32::new(100).unwrap(), direction: dropshot::PaginationOrder::Ascending, - }; - let pagbyid = PaginatedBy::Id(pagparams_id); + }); let members = datastore .affinity_group_member_list(&opctx, &authz_group, &pagbyid) .await @@ -1823,11 +2400,7 @@ mod tests { ) .await; datastore - .affinity_group_member_add( - &opctx, - &authz_group, - external::AffinityGroupMember::Instance(instance), - ) + .affinity_group_member_instance_add(&opctx, &authz_group, instance) .await .unwrap(); @@ -1867,21 +2440,24 @@ mod tests { .await .unwrap(); - let (.., authz_group) = LookupPath::new(opctx, datastore) + let (.., authz_aa_group) = LookupPath::new(opctx, datastore) .anti_affinity_group_id(group.id()) .lookup_for(authz::Action::Modify) .await .unwrap(); // A new group should have no members - let pagparams_id = DataPageParams { + let pagparams = PaginatedBy::Id(DataPageParams { marker: None, limit: NonZeroU32::new(100).unwrap(), direction: dropshot::PaginationOrder::Ascending, - }; - let pagbyid = PaginatedBy::Id(pagparams_id); + }); let members = datastore - .anti_affinity_group_member_list(&opctx, &authz_group, &pagbyid) + .anti_affinity_group_member_list( + &opctx, + &authz_aa_group, + &pagparams, + ) .await .unwrap(); assert!(members.is_empty()); @@ -1895,26 +2471,33 @@ mod tests { ) .await; datastore - .anti_affinity_group_member_add( + .anti_affinity_group_member_instance_add( &opctx, - &authz_group, - external::AntiAffinityGroupMember::Instance(instance), + &authz_aa_group, + instance, ) .await .unwrap(); // Delete the group datastore - .anti_affinity_group_delete(&opctx, &authz_group) + .anti_affinity_group_delete(&opctx, &authz_aa_group) .await .unwrap(); - // Confirm that no instance members exist + // Confirm that no group members exist let members = datastore - .anti_affinity_group_member_list(&opctx, &authz_group, &pagbyid) + .anti_affinity_group_member_list( + &opctx, + &authz_aa_group, + &pagparams, + ) .await .unwrap(); - assert!(members.is_empty()); + assert!( + members.is_empty(), + "No members should exist, but these do: {members:?}" + ); // Clean up. db.terminate().await; @@ -1949,12 +2532,11 @@ mod tests { .unwrap(); // A new group should have no members - let pagparams_id = DataPageParams { + let pagbyid = PaginatedBy::Id(DataPageParams { marker: None, limit: NonZeroU32::new(100).unwrap(), direction: dropshot::PaginationOrder::Ascending, - }; - let pagbyid = PaginatedBy::Id(pagparams_id); + }); let members = datastore .affinity_group_member_list(&opctx, &authz_group, &pagbyid) .await @@ -1970,11 +2552,7 @@ mod tests { ) .await; datastore - .affinity_group_member_add( - &opctx, - &authz_group, - external::AffinityGroupMember::Instance(instance), - ) + .affinity_group_member_instance_add(&opctx, &authz_group, instance) .await .unwrap(); @@ -2029,14 +2607,13 @@ mod tests { .unwrap(); // A new group should have no members - let pagparams_id = DataPageParams { + let pagparams = PaginatedBy::Id(DataPageParams { marker: None, limit: NonZeroU32::new(100).unwrap(), direction: dropshot::PaginationOrder::Ascending, - }; - let pagbyid = PaginatedBy::Id(pagparams_id); + }); let members = datastore - .anti_affinity_group_member_list(&opctx, &authz_group, &pagbyid) + .anti_affinity_group_member_list(&opctx, &authz_group, &pagparams) .await .unwrap(); assert!(members.is_empty()); @@ -2050,10 +2627,10 @@ mod tests { ) .await; datastore - .anti_affinity_group_member_add( + .anti_affinity_group_member_instance_add( &opctx, &authz_group, - external::AntiAffinityGroupMember::Instance(instance), + instance, ) .await .unwrap(); @@ -2071,7 +2648,7 @@ mod tests { // Confirm that no instance members exist let members = datastore - .anti_affinity_group_member_list(&opctx, &authz_group, &pagbyid) + .anti_affinity_group_member_list(&opctx, &authz_group, &pagparams) .await .unwrap(); assert!(members.is_empty()); @@ -2156,10 +2733,10 @@ mod tests { // Expect to see specific errors, depending on whether or not the // group/instance exist. let err = datastore - .affinity_group_member_add( + .affinity_group_member_instance_add( &opctx, &authz_group, - external::AffinityGroupMember::Instance(instance), + instance, ) .await .expect_err("Should have failed"); @@ -2188,10 +2765,10 @@ mod tests { // Do the same thing, but for group membership removal. let err = datastore - .affinity_group_member_delete( + .affinity_group_member_instance_delete( &opctx, &authz_group, - external::AffinityGroupMember::Instance(instance), + instance, ) .await .expect_err("Should have failed"); @@ -2287,7 +2864,7 @@ mod tests { .unwrap(); } - // Create an instance, and maybe delete it. + // Create an instance, and maybe deletes it let instance = create_stopped_instance_record( &opctx, &datastore, @@ -2295,11 +2872,13 @@ mod tests { "my-instance", ) .await; + let (.., authz_instance) = LookupPath::new(opctx, datastore) .instance_id(instance.into_untyped_uuid()) .lookup_for(authz::Action::Modify) .await .unwrap(); + if !arg.instance { datastore .project_delete_instance(&opctx, &authz_instance) @@ -2307,15 +2886,15 @@ mod tests { .unwrap(); } - // Try to add the instance to the group. + // Try to add the instnace to the group. // // Expect to see specific errors, depending on whether or not the // group/instance exist. let err = datastore - .anti_affinity_group_member_add( + .anti_affinity_group_member_instance_add( &opctx, &authz_group, - external::AntiAffinityGroupMember::Instance(instance), + instance, ) .await .expect_err("Should have failed"); @@ -2344,13 +2923,14 @@ mod tests { // Do the same thing, but for group membership removal. let err = datastore - .anti_affinity_group_member_delete( + .anti_affinity_group_member_instance_delete( &opctx, &authz_group, - external::AntiAffinityGroupMember::Instance(instance), + instance, ) .await .expect_err("Should have failed"); + match (arg.group, arg.instance) { (false, _) => { assert!( @@ -2429,21 +3009,13 @@ mod tests { // Add the instance to the group datastore - .affinity_group_member_add( - &opctx, - &authz_group, - external::AffinityGroupMember::Instance(instance), - ) + .affinity_group_member_instance_add(&opctx, &authz_group, instance) .await .unwrap(); // Add the instance to the group again let err = datastore - .affinity_group_member_add( - &opctx, - &authz_group, - external::AffinityGroupMember::Instance(instance), - ) + .affinity_group_member_instance_add(&opctx, &authz_group, instance) .await .unwrap_err(); assert!( @@ -2461,12 +3033,11 @@ mod tests { // // Two calls to "affinity_group_member_add" should be the same // as a single call. - let pagparams_id = DataPageParams { + let pagbyid = PaginatedBy::Id(DataPageParams { marker: None, limit: NonZeroU32::new(100).unwrap(), direction: dropshot::PaginationOrder::Ascending, - }; - let pagbyid = PaginatedBy::Id(pagparams_id); + }); let members = datastore .affinity_group_member_list(&opctx, &authz_group, &pagbyid) .await @@ -2475,18 +3046,18 @@ mod tests { // We should be able to delete the membership idempotently. datastore - .affinity_group_member_delete( + .affinity_group_member_instance_delete( &opctx, &authz_group, - external::AffinityGroupMember::Instance(instance), + instance, ) .await .unwrap(); let err = datastore - .affinity_group_member_delete( + .affinity_group_member_instance_delete( &opctx, &authz_group, - external::AffinityGroupMember::Instance(instance), + instance, ) .await .unwrap_err(); @@ -2548,20 +3119,20 @@ mod tests { // Add the instance to the group datastore - .anti_affinity_group_member_add( + .anti_affinity_group_member_instance_add( &opctx, &authz_group, - external::AntiAffinityGroupMember::Instance(instance), + instance, ) .await .unwrap(); // Add the instance to the group again let err = datastore - .anti_affinity_group_member_add( + .anti_affinity_group_member_instance_add( &opctx, &authz_group, - external::AntiAffinityGroupMember::Instance(instance), + instance, ) .await .unwrap_err(); @@ -2578,34 +3149,33 @@ mod tests { // We should still only observe a single member in the group. // - // Two calls to "anti_affinity_group_member_add" should be the same + // Two calls to "anti_affinity_group_member_instance_add" should be the same // as a single call. - let pagparams_id = DataPageParams { + let pagparams = PaginatedBy::Id(DataPageParams { marker: None, limit: NonZeroU32::new(100).unwrap(), direction: dropshot::PaginationOrder::Ascending, - }; - let pagbyid = PaginatedBy::Id(pagparams_id); + }); let members = datastore - .anti_affinity_group_member_list(&opctx, &authz_group, &pagbyid) + .anti_affinity_group_member_list(&opctx, &authz_group, &pagparams) .await .unwrap(); assert_eq!(members.len(), 1); // We should be able to delete the membership idempotently. datastore - .anti_affinity_group_member_delete( + .anti_affinity_group_member_instance_delete( &opctx, &authz_group, - external::AntiAffinityGroupMember::Instance(instance), + instance, ) .await .unwrap(); let err = datastore - .anti_affinity_group_member_delete( + .anti_affinity_group_member_instance_delete( &opctx, &authz_group, - external::AntiAffinityGroupMember::Instance(instance), + instance, ) .await .unwrap_err(); @@ -2621,7 +3191,7 @@ mod tests { ); let members = datastore - .anti_affinity_group_member_list(&opctx, &authz_group, &pagbyid) + .anti_affinity_group_member_list(&opctx, &authz_group, &pagparams) .await .unwrap(); assert!(members.is_empty()); diff --git a/nexus/db-queries/src/db/datastore/instance.rs b/nexus/db-queries/src/db/datastore/instance.rs index c78b6976f26..7d2c458566e 100644 --- a/nexus/db-queries/src/db/datastore/instance.rs +++ b/nexus/db-queries/src/db/datastore/instance.rs @@ -67,62 +67,39 @@ use omicron_uuid_kinds::SledUuid; use ref_cast::RefCast; use uuid::Uuid; -/// Wraps a record of an `Instance` along with its active `Vmm`, if it has one. -#[derive(Clone, Debug)] -pub struct InstanceAndActiveVmm { - pub instance: Instance, - pub vmm: Option, +/// Returns the operator-visible [external API +/// `InstanceState`](external::InstanceState) for the provided [`Instance`] +/// and its active [`Vmm`], if one exists. +pub struct InstanceStateComputer<'s> { + instance_state: &'s InstanceState, + migration_id: Option<&'s Uuid>, + vmm_state: Option<&'s VmmState>, } -impl InstanceAndActiveVmm { - pub fn instance(&self) -> &Instance { - &self.instance - } - - pub fn vmm(&self) -> &Option { - &self.vmm - } - - pub fn sled_id(&self) -> Option { - self.vmm.as_ref().map(|v| SledUuid::from_untyped_uuid(v.sled_id)) +impl<'s> InstanceStateComputer<'s> { + pub fn new(instance: &'s Instance, vmm: Option<&'s Vmm>) -> Self { + Self { + instance_state: &instance.runtime_state.nexus_state, + migration_id: instance.runtime_state.migration_id.as_ref(), + vmm_state: vmm.as_ref().map(|vmm| &vmm.runtime.state), + } } - /// Returns the operator-visible [external API - /// `InstanceState`](external::InstanceState) for this instance and its - /// active VMM. - pub fn effective_state(&self) -> external::InstanceState { - Self::determine_effective_state(&self.instance, self.vmm.as_ref()) + pub fn compute_state_from( + instance_state: &'s InstanceState, + migration_id: Option<&'s Uuid>, + vmm_state: Option<&'s VmmState>, + ) -> external::InstanceState { + Self { instance_state, migration_id, vmm_state }.compute_state() } - /// Returns the operator-visible [external API - /// `InstanceState`](external::InstanceState) for the provided [`Instance`] - /// and its active [`Vmm`], if one exists. - /// - /// # Arguments - /// - /// - `instance`: the instance - /// - `active_vmm`: the instance's active VMM, if one exists. - /// - /// # Notes - /// - /// Generally, the value of `active_vmm` should be - /// the VMM pointed to by `instance.runtime_state.propolis_id`. However, - /// this is not enforced by this function, as the `instance_migrate` saga - /// must in some cases determine an effective instance state from the - /// instance and *target* VMM states. - pub fn determine_effective_state( - instance: &Instance, - active_vmm: Option<&Vmm>, - ) -> external::InstanceState { + pub fn compute_state(&self) -> external::InstanceState { use crate::db::model::InstanceState; use crate::db::model::VmmState; - let instance_state = instance.runtime_state.nexus_state; - let vmm_state = active_vmm.map(|vmm| vmm.runtime.state); - // We want to only report that an instance is `Stopped` when a new // `instance-start` saga is able to proceed. That means that: - match (instance_state, vmm_state) { + match (self.instance_state, self.vmm_state) { // - If there's an active migration ID for the instance, *always* // treat its state as "migration" regardless of the VMM's state. // @@ -145,9 +122,7 @@ impl InstanceAndActiveVmm { // instance-update saga will come along and remove the active VMM // and migration IDs. // - (InstanceState::Vmm, Some(_)) - if instance.runtime_state.migration_id.is_some() => - { + (InstanceState::Vmm, Some(_)) if self.migration_id.is_some() => { external::InstanceState::Migrating } // - An instance with a "stopped" or "destroyed" VMM needs to be @@ -181,15 +156,49 @@ impl InstanceAndActiveVmm { // If there's a VMM state, and none of the above rules apply, use // that. (_instance_state, Some(vmm_state)) => { - debug_assert_eq!(_instance_state, InstanceState::Vmm); - vmm_state.into() + debug_assert_eq!(_instance_state, &InstanceState::Vmm); + (*vmm_state).into() } // If there's no VMM state, use the instance's state. - (instance_state, None) => instance_state.into(), + (instance_state, None) => (*instance_state).into(), } } } +impl<'s> From<&'s InstanceAndActiveVmm> for InstanceStateComputer<'s> { + fn from(i: &'s InstanceAndActiveVmm) -> Self { + InstanceStateComputer::new(&i.instance, i.vmm.as_ref()) + } +} + +/// Wraps a record of an `Instance` along with its active `Vmm`, if it has one. +#[derive(Clone, Debug)] +pub struct InstanceAndActiveVmm { + pub instance: Instance, + pub vmm: Option, +} + +impl InstanceAndActiveVmm { + pub fn instance(&self) -> &Instance { + &self.instance + } + + pub fn vmm(&self) -> &Option { + &self.vmm + } + + pub fn sled_id(&self) -> Option { + self.vmm.as_ref().map(|v| SledUuid::from_untyped_uuid(v.sled_id)) + } + + /// Returns the operator-visible [external API + /// `InstanceState`](external::InstanceState) for this instance and its + /// active VMM. + pub fn effective_state(&self) -> external::InstanceState { + InstanceStateComputer::from(self).compute_state() + } +} + impl From<(Instance, Option)> for InstanceAndActiveVmm { fn from(value: (Instance, Option)) -> Self { Self { instance: value.0, vmm: value.1 } diff --git a/nexus/db-queries/src/db/datastore/mod.rs b/nexus/db-queries/src/db/datastore/mod.rs index 927526695fb..6d346a17af5 100644 --- a/nexus/db-queries/src/db/datastore/mod.rs +++ b/nexus/db-queries/src/db/datastore/mod.rs @@ -115,7 +115,9 @@ mod zpool; pub use address_lot::AddressLotCreateResult; pub use dns::DataStoreDnsTest; pub use dns::DnsVersionUpdateBuilder; -pub use instance::{InstanceAndActiveVmm, InstanceGestalt}; +pub use instance::{ + InstanceAndActiveVmm, InstanceGestalt, InstanceStateComputer, +}; pub use inventory::DataStoreInventoryTest; use nexus_db_model::AllSchemaVersions; pub use oximeter::CollectorReassignment; diff --git a/nexus/db-queries/src/db/pub_test_utils/helpers.rs b/nexus/db-queries/src/db/pub_test_utils/helpers.rs index 06f31c444ca..b3b7df3d330 100644 --- a/nexus/db-queries/src/db/pub_test_utils/helpers.rs +++ b/nexus/db-queries/src/db/pub_test_utils/helpers.rs @@ -366,12 +366,8 @@ pub async fn create_affinity_group_member( .lookup_for(authz::Action::Modify) .await?; - db.affinity_group_member_add( - opctx, - &authz_group, - external::AffinityGroupMember::Instance(instance_id), - ) - .await?; + db.affinity_group_member_instance_add(opctx, &authz_group, instance_id) + .await?; Ok(()) } @@ -390,10 +386,10 @@ pub async fn create_anti_affinity_group_member( .lookup_for(authz::Action::Modify) .await?; - db.anti_affinity_group_member_add( + db.anti_affinity_group_member_instance_add( opctx, &authz_group, - external::AntiAffinityGroupMember::Instance(instance_id), + instance_id, ) .await?; Ok(()) diff --git a/nexus/external-api/src/lib.rs b/nexus/external-api/src/lib.rs index abcaa0a1122..e29dedf04a0 100644 --- a/nexus/external-api/src/lib.rs +++ b/nexus/external-api/src/lib.rs @@ -1297,7 +1297,9 @@ pub trait NexusExternalApi { }] async fn affinity_group_member_list( rqctx: RequestContext, - query_params: Query>, + query_params: Query< + PaginatedByNameOrId, + >, path_params: Path, ) -> Result>, HttpError>; @@ -1405,7 +1407,9 @@ pub trait NexusExternalApi { }] async fn anti_affinity_group_member_list( rqctx: RequestContext, - query_params: Query>, + query_params: Query< + PaginatedByNameOrId, + >, path_params: Path, ) -> Result>, HttpError>; diff --git a/nexus/src/app/affinity.rs b/nexus/src/app/affinity.rs index e389c24e77b..e55795a19db 100644 --- a/nexus/src/app/affinity.rs +++ b/nexus/src/app/affinity.rs @@ -12,6 +12,7 @@ use nexus_db_queries::db::lookup; use nexus_db_queries::db::lookup::LookupPath; use nexus_types::external_api::params; use nexus_types::external_api::views; +use nexus_types::identity::Resource; use omicron_common::api::external; use omicron_common::api::external::CreateResult; use omicron_common::api::external::DeleteResult; @@ -233,13 +234,9 @@ impl super::Nexus { let (.., authz_affinity_group) = affinity_group_lookup .lookup_for(authz::Action::ListChildren) .await?; - Ok(self - .db_datastore + self.db_datastore .affinity_group_member_list(opctx, &authz_affinity_group, pagparams) - .await? - .into_iter() - .map(Into::into) - .collect()) + .await } pub(crate) async fn anti_affinity_group_member_list( @@ -251,17 +248,13 @@ impl super::Nexus { let (.., authz_anti_affinity_group) = anti_affinity_group_lookup .lookup_for(authz::Action::ListChildren) .await?; - Ok(self - .db_datastore + self.db_datastore .anti_affinity_group_member_list( opctx, &authz_anti_affinity_group, pagparams, ) - .await? - .into_iter() - .map(Into::into) - .collect()) + .await } pub(crate) async fn affinity_group_member_view( @@ -274,16 +267,18 @@ impl super::Nexus { affinity_group_lookup.lookup_for(authz::Action::Read).await?; let (.., authz_instance) = instance_lookup.lookup_for(authz::Action::Read).await?; - let member = external::AffinityGroupMember::Instance( - InstanceUuid::from_untyped_uuid(authz_instance.id()), - ); + let member = InstanceUuid::from_untyped_uuid(authz_instance.id()); self.db_datastore - .affinity_group_member_view(opctx, &authz_affinity_group, member) + .affinity_group_member_instance_view( + opctx, + &authz_affinity_group, + member, + ) .await } - pub(crate) async fn anti_affinity_group_member_view( + pub(crate) async fn anti_affinity_group_member_instance_view( &self, opctx: &OpContext, anti_affinity_group_lookup: &lookup::AntiAffinityGroup<'_>, @@ -293,12 +288,10 @@ impl super::Nexus { anti_affinity_group_lookup.lookup_for(authz::Action::Read).await?; let (.., authz_instance) = instance_lookup.lookup_for(authz::Action::Read).await?; - let member = external::AntiAffinityGroupMember::Instance( - InstanceUuid::from_untyped_uuid(authz_instance.id()), - ); + let member = InstanceUuid::from_untyped_uuid(authz_instance.id()); self.db_datastore - .anti_affinity_group_member_view( + .anti_affinity_group_member_instance_view( opctx, &authz_anti_affinity_group, member, @@ -314,23 +307,28 @@ impl super::Nexus { ) -> Result { let (.., authz_affinity_group) = affinity_group_lookup.lookup_for(authz::Action::Modify).await?; - let (.., authz_instance) = - instance_lookup.lookup_for(authz::Action::Read).await?; - let member = external::AffinityGroupMember::Instance( - InstanceUuid::from_untyped_uuid(authz_instance.id()), - ); + let (.., authz_instance, instance) = + instance_lookup.fetch_for(authz::Action::Read).await?; + let member = InstanceUuid::from_untyped_uuid(authz_instance.id()); self.db_datastore - .affinity_group_member_add( + .affinity_group_member_instance_add( opctx, &authz_affinity_group, - member.clone(), + member, ) .await?; - Ok(member) + Ok(external::AffinityGroupMember::Instance { + id: member, + name: instance.name().clone(), + // TODO: This is kinda a lie - the current implementation of + // "affinity_group_member_instance_add" relies on the instance + // not having a VMM, but that might change in the future. + run_state: external::InstanceState::Stopped, + }) } - pub(crate) async fn anti_affinity_group_member_add( + pub(crate) async fn anti_affinity_group_member_instance_add( &self, opctx: &OpContext, anti_affinity_group_lookup: &lookup::AntiAffinityGroup<'_>, @@ -339,20 +337,25 @@ impl super::Nexus { let (.., authz_anti_affinity_group) = anti_affinity_group_lookup .lookup_for(authz::Action::Modify) .await?; - let (.., authz_instance) = - instance_lookup.lookup_for(authz::Action::Read).await?; - let member = external::AntiAffinityGroupMember::Instance( - InstanceUuid::from_untyped_uuid(authz_instance.id()), - ); + let (.., authz_instance, instance) = + instance_lookup.fetch_for(authz::Action::Read).await?; + let member = InstanceUuid::from_untyped_uuid(authz_instance.id()); self.db_datastore - .anti_affinity_group_member_add( + .anti_affinity_group_member_instance_add( opctx, &authz_anti_affinity_group, - member.clone(), + member, ) .await?; - Ok(member) + Ok(external::AntiAffinityGroupMember::Instance { + id: member, + name: instance.name().clone(), + // TODO: This is kinda a lie - the current implementation of + // "anti_affinity_group_member_instance_add" relies on the instance + // not having a VMM, but that might change in the future. + run_state: external::InstanceState::Stopped, + }) } pub(crate) async fn affinity_group_member_delete( @@ -365,16 +368,18 @@ impl super::Nexus { affinity_group_lookup.lookup_for(authz::Action::Modify).await?; let (.., authz_instance) = instance_lookup.lookup_for(authz::Action::Read).await?; - let member = external::AffinityGroupMember::Instance( - InstanceUuid::from_untyped_uuid(authz_instance.id()), - ); + let member = InstanceUuid::from_untyped_uuid(authz_instance.id()); self.db_datastore - .affinity_group_member_delete(opctx, &authz_affinity_group, member) + .affinity_group_member_instance_delete( + opctx, + &authz_affinity_group, + member, + ) .await } - pub(crate) async fn anti_affinity_group_member_delete( + pub(crate) async fn anti_affinity_group_member_instance_delete( &self, opctx: &OpContext, anti_affinity_group_lookup: &lookup::AntiAffinityGroup<'_>, @@ -385,12 +390,10 @@ impl super::Nexus { .await?; let (.., authz_instance) = instance_lookup.lookup_for(authz::Action::Read).await?; - let member = external::AntiAffinityGroupMember::Instance( - InstanceUuid::from_untyped_uuid(authz_instance.id()), - ); + let member = InstanceUuid::from_untyped_uuid(authz_instance.id()); self.db_datastore - .anti_affinity_group_member_delete( + .anti_affinity_group_member_instance_delete( opctx, &authz_anti_affinity_group, member, diff --git a/nexus/src/app/instance.rs b/nexus/src/app/instance.rs index 240419aa053..b0577aaf6e2 100644 --- a/nexus/src/app/instance.rs +++ b/nexus/src/app/instance.rs @@ -31,6 +31,7 @@ use nexus_db_queries::context::OpContext; use nexus_db_queries::db; use nexus_db_queries::db::DataStore; use nexus_db_queries::db::datastore::InstanceAndActiveVmm; +use nexus_db_queries::db::datastore::InstanceStateComputer; use nexus_db_queries::db::identity::Resource; use nexus_db_queries::db::lookup; use nexus_db_queries::db::lookup::LookupPath; @@ -780,10 +781,9 @@ impl super::Nexus { vmm_state: &Option, requested: &InstanceStateChangeRequest, ) -> Result { - let effective_state = InstanceAndActiveVmm::determine_effective_state( - instance_state, - vmm_state.as_ref(), - ); + let effective_state = + InstanceStateComputer::new(instance_state, vmm_state.as_ref()) + .compute_state(); // Requests that operate on active instances have to be directed to the // instance's current sled agent. If there is none, the request needs to diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index e28c2c3e589..1508ff51ec2 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -94,7 +94,6 @@ use omicron_common::api::external::http_pagination::ScanByName; use omicron_common::api::external::http_pagination::ScanByNameOrId; use omicron_common::api::external::http_pagination::ScanParams; use omicron_common::api::external::http_pagination::data_page_params_for; -use omicron_common::api::external::http_pagination::id_pagination; use omicron_common::api::external::http_pagination::marker_for_id; use omicron_common::api::external::http_pagination::marker_for_name; use omicron_common::api::external::http_pagination::marker_for_name_or_id; @@ -2577,7 +2576,9 @@ impl NexusExternalApi for NexusExternalApiImpl { async fn affinity_group_member_list( rqctx: RequestContext, - query_params: Query>, + query_params: Query< + PaginatedByNameOrId, + >, path_params: Path, ) -> Result>, HttpError> { @@ -2589,8 +2590,8 @@ impl NexusExternalApi for NexusExternalApiImpl { let path = path_params.into_inner(); let query = query_params.into_inner(); let pag_params = data_page_params_for(&rqctx, &query)?; - let scan_params = ScanById::from_query(&query)?; - let paginated_by = id_pagination(&pag_params, scan_params)?; + let scan_params = ScanByNameOrId::from_query(&query)?; + let paginated_by = name_or_id_pagination(&pag_params, scan_params)?; let group_selector = params::AffinityGroupSelector { project: scan_params.selector.project.clone(), @@ -2605,10 +2606,10 @@ impl NexusExternalApi for NexusExternalApiImpl { &paginated_by, ) .await?; - Ok(HttpResponseOk(ScanById::results_page( + Ok(HttpResponseOk(ScanByNameOrId::results_page( &query, affinity_group_member_instances, - &marker_for_id, + &marker_for_name_or_id, )?)) }; apictx @@ -2912,7 +2913,9 @@ impl NexusExternalApi for NexusExternalApiImpl { async fn anti_affinity_group_member_list( rqctx: RequestContext, - query_params: Query>, + query_params: Query< + PaginatedByNameOrId, + >, path_params: Path, ) -> Result>, HttpError> { @@ -2924,8 +2927,8 @@ impl NexusExternalApi for NexusExternalApiImpl { let path = path_params.into_inner(); let query = query_params.into_inner(); let pag_params = data_page_params_for(&rqctx, &query)?; - let scan_params = ScanById::from_query(&query)?; - let paginated_by = id_pagination(&pag_params, scan_params)?; + let scan_params = ScanByNameOrId::from_query(&query)?; + let paginated_by = name_or_id_pagination(&pag_params, scan_params)?; let group_selector = params::AntiAffinityGroupSelector { project: scan_params.selector.project.clone(), @@ -2940,10 +2943,10 @@ impl NexusExternalApi for NexusExternalApiImpl { &paginated_by, ) .await?; - Ok(HttpResponseOk(ScanById::results_page( + Ok(HttpResponseOk(ScanByNameOrId::results_page( &query, group_members, - &marker_for_id, + &marker_for_name_or_id, )?)) }; apictx @@ -2983,7 +2986,7 @@ impl NexusExternalApi for NexusExternalApiImpl { nexus.instance_lookup(&opctx, instance_selector)?; let group = nexus - .anti_affinity_group_member_view( + .anti_affinity_group_member_instance_view( &opctx, &group_lookup, &instance_lookup, @@ -3029,7 +3032,7 @@ impl NexusExternalApi for NexusExternalApiImpl { nexus.instance_lookup(&opctx, instance_selector)?; let member = nexus - .anti_affinity_group_member_add( + .anti_affinity_group_member_instance_add( &opctx, &group_lookup, &instance_lookup, @@ -3074,7 +3077,7 @@ impl NexusExternalApi for NexusExternalApiImpl { nexus.instance_lookup(&opctx, instance_selector)?; nexus - .anti_affinity_group_member_delete( + .anti_affinity_group_member_instance_delete( &opctx, &group_lookup, &instance_lookup, diff --git a/nexus/tests/integration_tests/affinity.rs b/nexus/tests/integration_tests/affinity.rs index 902ab60f1cd..debac93c0e1 100644 --- a/nexus/tests/integration_tests/affinity.rs +++ b/nexus/tests/integration_tests/affinity.rs @@ -623,7 +623,7 @@ async fn test_anti_affinity_group_usage(cptestctx: &ControlPlaneTestContext) { .map(|instance| instance.identity.id) .collect::>(); - // We expect that each sled will have a since instance, as all of the + // We expect that each sled will have a single instance, as all of the // instances will want to be anti-located from each other. for sled in &sleds { let observed_instances = api diff --git a/nexus/tests/integration_tests/unauthorized.rs b/nexus/tests/integration_tests/unauthorized.rs index 8df0bffdb4e..5ccb0b2ceb1 100644 --- a/nexus/tests/integration_tests/unauthorized.rs +++ b/nexus/tests/integration_tests/unauthorized.rs @@ -304,7 +304,7 @@ static SETUP_REQUESTS: LazyLock> = LazyLock::new(|| { body: serde_json::to_value(&*DEMO_AFFINITY_GROUP_CREATE).unwrap(), id_routes: vec!["/v1/affinity-groups/{id}"], }, - // Add a member to the affinity group + // Add an instance to the affinity group SetupReq::Post { url: &DEMO_AFFINITY_GROUP_INSTANCE_MEMBER_URL, body: serde_json::Value::Null, @@ -317,7 +317,7 @@ static SETUP_REQUESTS: LazyLock> = LazyLock::new(|| { .unwrap(), id_routes: vec!["/v1/anti-affinity-groups/{id}"], }, - // Add a member to the anti-affinity group + // Add an instance to the anti-affinity group SetupReq::Post { url: &DEMO_ANTI_AFFINITY_GROUP_INSTANCE_MEMBER_URL, body: serde_json::Value::Null, diff --git a/openapi/nexus.json b/openapi/nexus.json index 43485176b82..4a923b747f8 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -975,7 +975,7 @@ "in": "query", "name": "sort_by", "schema": { - "$ref": "#/components/schemas/IdSortMode" + "$ref": "#/components/schemas/NameOrIdSortMode" } }, { @@ -1451,7 +1451,7 @@ "in": "query", "name": "sort_by", "schema": { - "$ref": "#/components/schemas/IdSortMode" + "$ref": "#/components/schemas/NameOrIdSortMode" } }, { @@ -12277,7 +12277,7 @@ "description": "A member of an Affinity Group\n\nMembership in a group is not exclusive - members may belong to multiple affinity / anti-affinity groups.", "oneOf": [ { - "description": "An instance belonging to this group, identified by UUID.", + "description": "An instance belonging to this group", "type": "object", "properties": { "type": { @@ -12287,7 +12287,23 @@ ] }, "value": { - "$ref": "#/components/schemas/TypedUuidForInstanceKind" + "type": "object", + "properties": { + "id": { + "$ref": "#/components/schemas/TypedUuidForInstanceKind" + }, + "name": { + "$ref": "#/components/schemas/Name" + }, + "run_state": { + "$ref": "#/components/schemas/InstanceState" + } + }, + "required": [ + "id", + "name", + "run_state" + ] } }, "required": [ @@ -12561,7 +12577,7 @@ "description": "A member of an Anti-Affinity Group\n\nMembership in a group is not exclusive - members may belong to multiple affinity / anti-affinity groups.", "oneOf": [ { - "description": "An instance belonging to this group, identified by UUID.", + "description": "An instance belonging to this group", "type": "object", "properties": { "type": { @@ -12571,7 +12587,23 @@ ] }, "value": { - "$ref": "#/components/schemas/TypedUuidForInstanceKind" + "type": "object", + "properties": { + "id": { + "$ref": "#/components/schemas/TypedUuidForInstanceKind" + }, + "name": { + "$ref": "#/components/schemas/Name" + }, + "run_state": { + "$ref": "#/components/schemas/InstanceState" + } + }, + "required": [ + "id", + "name", + "run_state" + ] } }, "required": [