From 3bf35a2286ef24a3ee2ae0c1049f5116ac8531e0 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Thu, 20 Feb 2025 16:44:30 -0800 Subject: [PATCH 01/16] Basic implementation of affinity group member within anti-affinity group --- common/src/api/external/mod.rs | 5 + nexus/db-model/src/affinity.rs | 28 ++ nexus/db-model/src/schema.rs | 7 + nexus/db-queries/src/db/datastore/affinity.rs | 292 +++++++++++-- nexus/db-queries/src/db/datastore/sled.rs | 386 ++++++++++++++++++ .../src/db/queries/sled_reservation.rs | 144 +++++-- .../tests/output/sled_find_targets_query.sql | 80 +++- .../output/sled_insert_resource_query.sql | 80 +++- schema/crdb/dbinit.sql | 19 + 9 files changed, 946 insertions(+), 95 deletions(-) diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index b0ef9d23884..8b70a4228a5 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -22,6 +22,7 @@ use dropshot::HttpError; pub use dropshot::PaginationOrder; pub use error::*; use futures::stream::BoxStream; +use omicron_uuid_kinds::AffinityGroupUuid; use omicron_uuid_kinds::GenericUuid; use omicron_uuid_kinds::InstanceUuid; use oxnet::IpNet; @@ -1391,12 +1392,16 @@ impl SimpleIdentity for AffinityGroupMember { pub enum AntiAffinityGroupMember { /// An instance belonging to this group, identified by UUID. Instance(InstanceUuid), + + /// An affinity group belonging to this group, identified by UUID. + AffinityGroup(AffinityGroupUuid), } impl SimpleIdentity for AntiAffinityGroupMember { fn id(&self) -> Uuid { match self { AntiAffinityGroupMember::Instance(id) => *id.as_untyped_uuid(), + AntiAffinityGroupMember::AffinityGroup(id) => *id.as_untyped_uuid(), } } } diff --git a/nexus/db-model/src/affinity.rs b/nexus/db-model/src/affinity.rs index 5309ac95275..89214f85778 100644 --- a/nexus/db-model/src/affinity.rs +++ b/nexus/db-model/src/affinity.rs @@ -11,6 +11,7 @@ use super::Name; use crate::schema::affinity_group; use crate::schema::affinity_group_instance_membership; use crate::schema::anti_affinity_group; +use crate::schema::anti_affinity_group_affinity_membership; use crate::schema::anti_affinity_group_instance_membership; use crate::typed_uuid::DbTypedUuid; use chrono::{DateTime, Utc}; @@ -257,3 +258,30 @@ impl From Self::Instance(member.instance_id.into()) } } + +#[derive(Queryable, Insertable, Clone, Debug, Selectable)] +#[diesel(table_name = anti_affinity_group_affinity_membership)] +pub struct AntiAffinityGroupAffinityMembership { + pub anti_affinity_group_id: DbTypedUuid, + pub affinity_group_id: DbTypedUuid, +} + +impl AntiAffinityGroupAffinityMembership { + pub fn new( + anti_affinity_group_id: AntiAffinityGroupUuid, + affinity_group_id: AffinityGroupUuid, + ) -> Self { + Self { + anti_affinity_group_id: anti_affinity_group_id.into(), + affinity_group_id: affinity_group_id.into(), + } + } +} + +impl From + for external::AntiAffinityGroupMember +{ + fn from(member: AntiAffinityGroupAffinityMembership) -> Self { + Self::AffinityGroup(member.affinity_group_id.into()) + } +} diff --git a/nexus/db-model/src/schema.rs b/nexus/db-model/src/schema.rs index 433a8979f1d..139374eb16f 100644 --- a/nexus/db-model/src/schema.rs +++ b/nexus/db-model/src/schema.rs @@ -510,6 +510,13 @@ table! { } } +table! { + anti_affinity_group_affinity_membership (anti_affinity_group_id, affinity_group_id) { + anti_affinity_group_id -> Uuid, + affinity_group_id -> Uuid, + } +} + table! { metric_producer (id) { id -> Uuid, diff --git a/nexus/db-queries/src/db/datastore/affinity.rs b/nexus/db-queries/src/db/datastore/affinity.rs index 3fe3660e7dd..56caf545e5e 100644 --- a/nexus/db-queries/src/db/datastore/affinity.rs +++ b/nexus/db-queries/src/db/datastore/affinity.rs @@ -18,6 +18,7 @@ use crate::db::model::AffinityGroup; use crate::db::model::AffinityGroupInstanceMembership; use crate::db::model::AffinityGroupUpdate; use crate::db::model::AntiAffinityGroup; +use crate::db::model::AntiAffinityGroupAffinityMembership; use crate::db::model::AntiAffinityGroupInstanceMembership; use crate::db::model::AntiAffinityGroupUpdate; use crate::db::model::Name; @@ -433,27 +434,60 @@ impl DataStore { 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; - 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()) - .map_err(|e| { - public_error_from_diesel( - e, - ErrorHandler::NotFoundByLookup( - ResourceType::AntiAffinityGroupMember, - LookupType::by_id(instance_id.into_untyped_uuid()), - ), - ) - }) + match member { + external::AntiAffinityGroupMember::Instance(instance_id) => { + use db::schema::anti_affinity_group_instance_membership::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()) + .map_err(|e| { + public_error_from_diesel( + e, + ErrorHandler::NotFoundByLookup( + ResourceType::AntiAffinityGroupMember, + LookupType::by_id( + instance_id.into_untyped_uuid(), + ), + ), + ) + }) + } + external::AntiAffinityGroupMember::AffinityGroup( + affinity_group_id, + ) => { + use db::schema::anti_affinity_group_affinity_membership::dsl; + dsl::anti_affinity_group_affinity_membership + .filter( + dsl::anti_affinity_group_id + .eq(authz_anti_affinity_group.id()), + ) + .filter( + dsl::affinity_group_id + .eq(affinity_group_id.into_untyped_uuid()), + ) + .select(AntiAffinityGroupAffinityMembership::as_select()) + .get_result_async(&*conn) + .await + .map(|m| m.into()) + .map_err(|e| { + public_error_from_diesel( + e, + ErrorHandler::NotFoundByLookup( + ResourceType::AntiAffinityGroupMember, + LookupType::by_id( + affinity_group_id.into_untyped_uuid(), + ), + ), + ) + }) + } + } } pub async fn affinity_group_member_add( @@ -593,13 +627,35 @@ impl DataStore { .authorize(authz::Action::Modify, authz_anti_affinity_group) .await?; - let instance_id = match member { - external::AntiAffinityGroupMember::Instance(id) => id, - }; + match member { + external::AntiAffinityGroupMember::Instance(id) => { + self.anti_affinity_group_member_add_instance( + opctx, + authz_anti_affinity_group, + id, + ) + .await + } + external::AntiAffinityGroupMember::AffinityGroup(id) => { + self.anti_affinity_group_member_add_group( + opctx, + authz_anti_affinity_group, + id, + ) + .await + } + } + } + async fn anti_affinity_group_member_add_instance( + &self, + opctx: &OpContext, + authz_anti_affinity_group: &authz::AntiAffinityGroup, + instance_id: InstanceUuid, + ) -> Result<(), Error> { 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_add_instance") .transaction(&conn, |conn| { let err = err.clone(); use db::schema::anti_affinity_group::dsl as group_dsl; @@ -703,6 +759,95 @@ impl DataStore { Ok(()) } + async fn anti_affinity_group_member_add_group( + &self, + opctx: &OpContext, + authz_anti_affinity_group: &authz::AntiAffinityGroup, + affinity_group_id: AffinityGroupUuid, + ) -> Result<(), Error> { + let err = OptionalError::new(); + let conn = self.pool_connection_authorized(opctx).await?; + self.transaction_retry_wrapper("anti_affinity_group_member_add_group") + .transaction(&conn, |conn| { + let err = err.clone(); + use db::schema::anti_affinity_group::dsl as anti_affinity_group_dsl; + use db::schema::affinity_group::dsl as affinity_group_dsl; + use db::schema::anti_affinity_group_affinity_membership::dsl as membership_dsl; + + async move { + // Check that the anti-affinity group exists + anti_affinity_group_dsl::anti_affinity_group + .filter(anti_affinity_group_dsl::time_deleted.is_null()) + .filter(anti_affinity_group_dsl::id.eq(authz_anti_affinity_group.id())) + .select(anti_affinity_group_dsl::id) + .first_async::(&conn) + .await + .map_err(|e| { + err.bail_retryable_or_else(e, |e| { + public_error_from_diesel( + e, + ErrorHandler::NotFoundByResource( + authz_anti_affinity_group, + ), + ) + }) + })?; + + // Check that the affinity group exists + affinity_group_dsl::affinity_group + .filter(affinity_group_dsl::time_deleted.is_null()) + .filter(affinity_group_dsl::id.eq(affinity_group_id.into_untyped_uuid())) + .select(affinity_group_dsl::id) + .first_async::(&conn) + .await + .map_err(|e| { + err.bail_retryable_or_else(e, |e| { + public_error_from_diesel( + e, + ErrorHandler::NotFoundByLookup( + ResourceType::AffinityGroup, + LookupType::ById(affinity_group_id.into_untyped_uuid()) + ), + ) + }) + })?; + + // TODO: It's possible that the affinity group has members + // which are already running. We should probably check this, + // and prevent it, otherwise we could circumvent "policy = + // fail" stances. + + diesel::insert_into(membership_dsl::anti_affinity_group_affinity_membership) + .values(AntiAffinityGroupAffinityMembership::new( + AntiAffinityGroupUuid::from_untyped_uuid(authz_anti_affinity_group.id()), + affinity_group_id, + )) + .execute_async(&conn) + .await + .map_err(|e| { + err.bail_retryable_or_else(e, |e| { + public_error_from_diesel( + e, + ErrorHandler::Conflict( + ResourceType::AntiAffinityGroupMember, + &affinity_group_id.to_string(), + ), + ) + }) + })?; + Ok(()) + } + }) + .await + .map_err(|e| { + if let Some(err) = err.take() { + return err; + } + public_error_from_diesel(e, ErrorHandler::Server) + })?; + Ok(()) + } + pub async fn instance_affinity_group_memberships_delete( &self, opctx: &OpContext, @@ -810,13 +955,38 @@ impl DataStore { .authorize(authz::Action::Modify, authz_anti_affinity_group) .await?; - let instance_id = match member { - external::AntiAffinityGroupMember::Instance(id) => id, - }; + match member { + external::AntiAffinityGroupMember::Instance(id) => { + self.anti_affinity_group_instance_member_delete( + opctx, + authz_anti_affinity_group, + id, + ) + .await + } + external::AntiAffinityGroupMember::AffinityGroup(id) => { + self.anti_affinity_group_affinity_member_delete( + opctx, + authz_anti_affinity_group, + id, + ) + .await + } + } + } + // Deletes an anti-affinity member, when that member is an instance + // + // See: [`Self::anti_affinity_group_member_delete`] + async fn anti_affinity_group_instance_member_delete( + &self, + opctx: &OpContext, + authz_anti_affinity_group: &authz::AntiAffinityGroup, + instance_id: InstanceUuid, + ) -> Result<(), Error> { 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_instance_member_delete") .transaction(&conn, |conn| { let err = err.clone(); use db::schema::anti_affinity_group::dsl as group_dsl; @@ -868,6 +1038,70 @@ impl DataStore { })?; Ok(()) } + + // Deletes an anti-affinity member, when that member is an affinity group + // + // See: [`Self::anti_affinity_group_member_delete`] + async fn anti_affinity_group_affinity_member_delete( + &self, + opctx: &OpContext, + authz_anti_affinity_group: &authz::AntiAffinityGroup, + affinity_group_id: AffinityGroupUuid, + ) -> Result<(), Error> { + let err = OptionalError::new(); + let conn = self.pool_connection_authorized(opctx).await?; + self.transaction_retry_wrapper("anti_affinity_group_affinity_member_delete") + .transaction(&conn, |conn| { + let err = err.clone(); + use db::schema::anti_affinity_group::dsl as group_dsl; + use db::schema::anti_affinity_group_affinity_membership::dsl as membership_dsl; + + async move { + // Check that the anti-affinity group exists + group_dsl::anti_affinity_group + .filter(group_dsl::time_deleted.is_null()) + .filter(group_dsl::id.eq(authz_anti_affinity_group.id())) + .select(group_dsl::id) + .first_async::(&conn) + .await + .map_err(|e| { + err.bail_retryable_or_else(e, |e| { + public_error_from_diesel( + e, + ErrorHandler::NotFoundByResource( + authz_anti_affinity_group, + ), + ) + }) + })?; + + let rows = diesel::delete(membership_dsl::anti_affinity_group_affinity_membership) + .filter(membership_dsl::anti_affinity_group_id.eq(authz_anti_affinity_group.id())) + .filter(membership_dsl::affinity_group_id.eq(affinity_group_id.into_untyped_uuid())) + .execute_async(&conn) + .await + .map_err(|e| { + err.bail_retryable_or_else(e, |e| { + public_error_from_diesel(e, ErrorHandler::Server) + }) + })?; + if rows == 0 { + return Err(err.bail(LookupType::ById(affinity_group_id.into_untyped_uuid()).into_not_found( + ResourceType::AntiAffinityGroupMember, + ))); + } + Ok(()) + } + }) + .await + .map_err(|e| { + if let Some(err) = err.take() { + return err; + } + public_error_from_diesel(e, ErrorHandler::Server) + })?; + Ok(()) + } } #[cfg(test)] diff --git a/nexus/db-queries/src/db/datastore/sled.rs b/nexus/db-queries/src/db/datastore/sled.rs index 227f258c174..815d88e2490 100644 --- a/nexus/db-queries/src/db/datastore/sled.rs +++ b/nexus/db-queries/src/db/datastore/sled.rs @@ -1479,6 +1479,31 @@ pub(in crate::db::datastore) mod test { .unwrap(); } + async fn add_affinity_group_to_anti_affinity_group( + datastore: &DataStore, + aa_group_id: AntiAffinityGroupUuid, + a_group_id: AffinityGroupUuid, + ) { + use db::model::AntiAffinityGroupAffinityMembership; + use db::schema::anti_affinity_group_affinity_membership::dsl as membership_dsl; + + diesel::insert_into( + membership_dsl::anti_affinity_group_affinity_membership, + ) + .values(AntiAffinityGroupAffinityMembership::new( + aa_group_id, + a_group_id, + )) + .on_conflict(( + membership_dsl::anti_affinity_group_id, + membership_dsl::affinity_group_id, + )) + .do_nothing() + .execute_async(&*datastore.pool_connection_for_tests().await.unwrap()) + .await + .unwrap(); + } + async fn create_affinity_group( opctx: &OpContext, datastore: &DataStore, @@ -1609,6 +1634,42 @@ pub(in crate::db::datastore) mod test { Self { id_by_name } } + + async fn add_affinity_groups_to_anti_affinity_group( + &self, + datastore: &DataStore, + aa_group: &'static str, + a_groups: &[&'static str], + ) { + let (affinity, aa_group_id) = + self.id_by_name.get(aa_group).unwrap_or_else(|| { + panic!( + "Anti-affinity group {aa_group} not part of AllGroups" + ) + }); + assert!( + matches!(affinity, Affinity::Negative), + "{aa_group} should be an anti-affinity group, but is not" + ); + + for a_group in a_groups { + let (affinity, a_group_id) = + self.id_by_name.get(a_group).unwrap_or_else(|| { + panic!("Affinity group {a_group} not part of AllGroups") + }); + assert!( + matches!(affinity, Affinity::Positive), + "{a_group} should be an affinity group, but is not" + ); + + add_affinity_group_to_anti_affinity_group( + datastore, + AntiAffinityGroupUuid::from_untyped_uuid(*aa_group_id), + AffinityGroupUuid::from_untyped_uuid(*a_group_id), + ) + .await; + } + } } struct Instance { @@ -2323,6 +2384,331 @@ pub(in crate::db::datastore) mod test { logctx.cleanup_successful(); } + #[tokio::test] + async fn anti_affinity_group_containing_affinity_groups() { + let logctx = dev::test_setup_log( + "anti_affinity_group_containing_affinity_groups", + ); + let db = TestDatabase::new_with_datastore(&logctx.log).await; + let (opctx, datastore) = (db.opctx(), db.datastore()); + let (authz_project, _project) = + create_project(&opctx, &datastore).await; + + const SLED_COUNT: usize = 4; + let sleds = create_sleds(&datastore, SLED_COUNT).await; + + let groups = [ + Group { + affinity: Affinity::Negative, + name: "anti-affinity", + policy: external::AffinityPolicy::Fail, + }, + Group { + affinity: Affinity::Positive, + name: "affinity1", + policy: external::AffinityPolicy::Allow, + }, + Group { + affinity: Affinity::Positive, + name: "affinity2", + policy: external::AffinityPolicy::Allow, + }, + ]; + let all_groups = + AllGroups::create(&opctx, &datastore, &authz_project, &groups) + .await; + all_groups + .add_affinity_groups_to_anti_affinity_group( + &datastore, + "anti-affinity", + &["affinity1", "affinity2"], + ) + .await; + + // The instances on sled 0 and 1 belong to "affinity1" + // The instances on sled 2 and 3 belong to "affinity2" + // + // A new instance in "affinity2" can only be placed on sled 2. + let instances = [ + Instance::new().group("affinity1").sled(sleds[0].id()), + Instance::new().group("affinity1").sled(sleds[1].id()), + Instance::new().group("affinity2").sled(sleds[2].id()), + Instance::new() + .group("affinity2") + .sled(sleds[3].id()) + .use_many_resources(), + ]; + for instance in instances { + instance + .add_to_groups_and_reserve(&opctx, &datastore, &all_groups) + .await + .expect("Failed to set up instances"); + } + + let test_instance = Instance::new().group("affinity2"); + let resource = test_instance + .add_to_groups_and_reserve(&opctx, &datastore, &all_groups) + .await + .expect("Should have succeeded allocation"); + + assert_eq!( + resource.sled_id.into_untyped_uuid(), + sleds[2].id(), + "All sleds: {sled_ids:#?}", + sled_ids = sleds.iter().map(|s| s.identity.id).collect::>(), + ); + + db.terminate().await; + logctx.cleanup_successful(); + } + + #[tokio::test] + async fn anti_affinity_group_containing_affinity_groups_force_away_from_affine( + ) { + let logctx = dev::test_setup_log("anti_affinity_group_containing_affinity_groups_force_away_from_affine"); + let db = TestDatabase::new_with_datastore(&logctx.log).await; + let (opctx, datastore) = (db.opctx(), db.datastore()); + let (authz_project, _project) = + create_project(&opctx, &datastore).await; + + const SLED_COUNT: usize = 5; + let sleds = create_sleds(&datastore, SLED_COUNT).await; + + let groups = [ + Group { + affinity: Affinity::Negative, + name: "anti-affinity", + policy: external::AffinityPolicy::Fail, + }, + Group { + affinity: Affinity::Positive, + name: "affinity1", + policy: external::AffinityPolicy::Allow, + }, + Group { + affinity: Affinity::Positive, + name: "affinity2", + policy: external::AffinityPolicy::Allow, + }, + ]; + let all_groups = + AllGroups::create(&opctx, &datastore, &authz_project, &groups) + .await; + all_groups + .add_affinity_groups_to_anti_affinity_group( + &datastore, + "anti-affinity", + &["affinity1", "affinity2"], + ) + .await; + + // The instances on sled 0 and 1 belong to "affinity1" + // The instance on sled 2 belongs to "affinity2" + // The instance on sled 3 directly belongs to "anti-affinity" + // + // Even though the instance belongs to the affinity groups - and + // would "prefer" to be co-located with them - it's forced to be placed + // on sleds[4], since that's the only sled which doesn't violate + // the hard "anti-affinity" requirement. + let instances = [ + Instance::new().group("affinity1").sled(sleds[0].id()), + Instance::new().group("affinity1").sled(sleds[1].id()), + Instance::new().group("affinity2").sled(sleds[2].id()), + Instance::new().group("anti-affinity").sled(sleds[3].id()), + ]; + for instance in instances { + instance + .add_to_groups_and_reserve(&opctx, &datastore, &all_groups) + .await + .expect("Failed to set up instances"); + } + + // This instance is not part of "anti-affinity" directly. However, it + // is indirectly part of the anti-affinity group, because of its + // affinity group memberships. + let test_instance = + Instance::new().group("affinity1").group("affinity2"); + let resource = test_instance + .add_to_groups_and_reserve(&opctx, &datastore, &all_groups) + .await + .expect("Should have succeeded allocation"); + + assert_eq!( + resource.sled_id.into_untyped_uuid(), + sleds[4].id(), + "All sleds: {sled_ids:#?}", + sled_ids = sleds.iter().map(|s| s.identity.id).collect::>(), + ); + + db.terminate().await; + logctx.cleanup_successful(); + } + + #[tokio::test] + async fn anti_affinity_group_containing_affinity_groups_multigroup() { + let logctx = dev::test_setup_log( + "anti_affinity_group_containing_affinity_groups_multigroup", + ); + let db = TestDatabase::new_with_datastore(&logctx.log).await; + let (opctx, datastore) = (db.opctx(), db.datastore()); + let (authz_project, _project) = + create_project(&opctx, &datastore).await; + + const SLED_COUNT: usize = 3; + let sleds = create_sleds(&datastore, SLED_COUNT).await; + + let groups = [ + Group { + affinity: Affinity::Negative, + name: "wolf-eat-goat", + policy: external::AffinityPolicy::Fail, + }, + Group { + affinity: Affinity::Negative, + name: "goat-eat-cabbage", + policy: external::AffinityPolicy::Fail, + }, + Group { + affinity: Affinity::Positive, + name: "wolf", + policy: external::AffinityPolicy::Allow, + }, + Group { + affinity: Affinity::Positive, + name: "goat", + policy: external::AffinityPolicy::Allow, + }, + Group { + affinity: Affinity::Positive, + name: "cabbage", + policy: external::AffinityPolicy::Allow, + }, + ]; + let all_groups = + AllGroups::create(&opctx, &datastore, &authz_project, &groups) + .await; + all_groups + .add_affinity_groups_to_anti_affinity_group( + &datastore, + "wolf-eat-goat", + &["wolf", "goat"], + ) + .await; + all_groups + .add_affinity_groups_to_anti_affinity_group( + &datastore, + "goat-eat-cabbage", + &["goat", "cabbage"], + ) + .await; + + let instances = [ + Instance::new().group("wolf").sled(sleds[0].id()), + Instance::new().group("cabbage").sled(sleds[1].id()), + Instance::new().group("goat").sled(sleds[2].id()), + ]; + for instance in instances { + instance + .add_to_groups_and_reserve(&opctx, &datastore, &all_groups) + .await + .expect("Failed to set up instances"); + } + + let test_instance = Instance::new().group("wolf").group("cabbage"); + let resource = test_instance + .add_to_groups_and_reserve(&opctx, &datastore, &all_groups) + .await + .expect("Should have succeeded allocation"); + + // This instance can go on either sled[0] or sled[1], but not sled[2] + assert_ne!( + resource.sled_id.into_untyped_uuid(), + sleds[2].id(), + "All sleds: {sled_ids:#?}", + sled_ids = sleds.iter().map(|s| s.identity.id).collect::>(), + ); + + db.terminate().await; + logctx.cleanup_successful(); + } + + #[tokio::test] + async fn anti_affinity_group_containing_overlapping_affinity_groups() { + let logctx = dev::test_setup_log( + "anti_affinity_group_containing_overlapping_affinity_groups", + ); + let db = TestDatabase::new_with_datastore(&logctx.log).await; + let (opctx, datastore) = (db.opctx(), db.datastore()); + let (authz_project, _project) = + create_project(&opctx, &datastore).await; + + const SLED_COUNT: usize = 4; + let sleds = create_sleds(&datastore, SLED_COUNT).await; + + let groups = [ + Group { + affinity: Affinity::Negative, + name: "anti-affinity", + policy: external::AffinityPolicy::Fail, + }, + Group { + affinity: Affinity::Positive, + name: "affinity1", + policy: external::AffinityPolicy::Allow, + }, + Group { + affinity: Affinity::Positive, + name: "affinity2", + policy: external::AffinityPolicy::Allow, + }, + ]; + let all_groups = + AllGroups::create(&opctx, &datastore, &authz_project, &groups) + .await; + all_groups + .add_affinity_groups_to_anti_affinity_group( + &datastore, + "anti-affinity", + &["affinity1", "affinity2"], + ) + .await; + + // The instances on sled 0 and 1 belong to "affinity1" + // The instances on sled 2 and 3 belong to "affinity2" + // + // If a new instance belongs to both affinity groups, it cannot satisfy + // the anti-affinity requirements. + let instances = [ + Instance::new().group("affinity1").sled(sleds[0].id()), + Instance::new().group("affinity1").sled(sleds[1].id()), + Instance::new().group("affinity2").sled(sleds[2].id()), + Instance::new().group("affinity2").sled(sleds[3].id()), + ]; + for instance in instances { + instance + .add_to_groups_and_reserve(&opctx, &datastore, &all_groups) + .await + .expect("Failed to set up instances"); + } + + let test_instance = + Instance::new().group("affinity1").group("affinity2"); + let err = test_instance + .add_to_groups_and_reserve(&opctx, &datastore, &all_groups) + .await + .expect_err("Should have failed to place instance"); + + let SledReservationTransactionError::Reservation( + SledReservationError::NotFound, + ) = err + else { + panic!("Unexpected error: {err:?}"); + }; + + db.terminate().await; + logctx.cleanup_successful(); + } + #[tokio::test] async fn affinity_multi_group() { let logctx = dev::test_setup_log("affinity_multi_group"); diff --git a/nexus/db-queries/src/db/queries/sled_reservation.rs b/nexus/db-queries/src/db/queries/sled_reservation.rs index 912fd644da4..7fe28101a79 100644 --- a/nexus/db-queries/src/db/queries/sled_reservation.rs +++ b/nexus/db-queries/src/db/queries/sled_reservation.rs @@ -57,18 +57,70 @@ pub fn sled_find_targets_query( COALESCE(SUM(CAST(sled_resource_vmm.reservoir_ram AS INT8)), 0) + " ).param().sql(" <= sled.reservoir_size ), - our_aa_groups AS ( + our_a_groups AS ( + SELECT group_id + FROM affinity_group_instance_membership + WHERE instance_id = ").param().sql(" + ), + other_a_instances AS ( + SELECT affinity_group_instance_membership.group_id,instance_id + FROM affinity_group_instance_membership + JOIN our_a_groups + ON affinity_group_instance_membership.group_id = our_a_groups.group_id + WHERE instance_id != ").param().sql(" + ), + our_direct_aa_groups AS ( + -- Anti-affinity groups to which our instance belongs SELECT group_id FROM anti_affinity_group_instance_membership WHERE instance_id = ").param().sql(" ), - other_aa_instances AS ( + other_direct_aa_instances AS ( SELECT anti_affinity_group_instance_membership.group_id,instance_id FROM anti_affinity_group_instance_membership - JOIN our_aa_groups - ON anti_affinity_group_instance_membership.group_id = our_aa_groups.group_id + JOIN our_direct_aa_groups + ON anti_affinity_group_instance_membership.group_id = our_direct_aa_groups.group_id WHERE instance_id != ").param().sql(" ), + our_indirect_aa_groups AS ( + -- Anti-affinity groups to which our instance's affinity groups belong + SELECT + anti_affinity_group_id, + affinity_group_id, + CASE + WHEN COUNT(*) OVER (PARTITION BY anti_affinity_group_id) = 1 THEN TRUE + ELSE FALSE + END AS exactly_one_affinity_group + FROM anti_affinity_group_affinity_membership + WHERE affinity_group_id IN (SELECT group_id FROM our_a_groups) + ), + other_indirect_aa_instances_via_instances AS ( + SELECT anti_affinity_group_id AS group_id,instance_id + FROM anti_affinity_group_instance_membership + JOIN our_indirect_aa_groups + ON anti_affinity_group_instance_membership.group_id = our_indirect_aa_groups.anti_affinity_group_id + ), + other_indirect_aa_instances_via_groups AS ( + SELECT anti_affinity_group_id AS group_id,instance_id + FROM affinity_group_instance_membership + JOIN our_indirect_aa_groups + ON affinity_group_instance_membership.group_id = our_indirect_aa_groups.affinity_group_id + WHERE + -- If our instance belongs to exactly one of these groups... + CASE WHEN our_indirect_aa_groups.exactly_one_affinity_group + -- ... exclude that group from our anti-affinity + THEN affinity_group_instance_membership.group_id NOT IN (SELECT group_id FROM our_a_groups) + -- ... otherwise, consider all groups anti-affine + ELSE TRUE + END + ), + other_aa_instances AS ( + SELECT * FROM other_direct_aa_instances + UNION + SELECT * FROM other_indirect_aa_instances_via_instances + UNION + SELECT * FROM other_indirect_aa_instances_via_groups + ), other_aa_instances_by_policy AS ( SELECT policy,instance_id FROM other_aa_instances @@ -85,18 +137,6 @@ pub fn sled_find_targets_query( ON sled_resource_vmm.instance_id = other_aa_instances_by_policy.instance_id ), - our_a_groups AS ( - SELECT group_id - FROM affinity_group_instance_membership - WHERE instance_id = ").param().sql(" - ), - other_a_instances AS ( - SELECT affinity_group_instance_membership.group_id,instance_id - FROM affinity_group_instance_membership - JOIN our_a_groups - ON affinity_group_instance_membership.group_id = our_a_groups.group_id - WHERE instance_id != ").param().sql(" - ), other_a_instances_by_policy AS ( SELECT policy,instance_id FROM other_a_instances @@ -172,18 +212,70 @@ pub fn sled_insert_resource_query( COALESCE(SUM(CAST(sled_resource_vmm.reservoir_ram AS INT8)), 0) + " ).param().sql(" <= sled.reservoir_size ), - our_aa_groups AS ( + our_a_groups AS ( + SELECT group_id + FROM affinity_group_instance_membership + WHERE instance_id = ").param().sql(" + ), + other_a_instances AS ( + SELECT affinity_group_instance_membership.group_id,instance_id + FROM affinity_group_instance_membership + JOIN our_a_groups + ON affinity_group_instance_membership.group_id = our_a_groups.group_id + WHERE instance_id != ").param().sql(" + ), + our_direct_aa_groups AS ( + -- Anti-affinity groups to which our instance belongs SELECT group_id FROM anti_affinity_group_instance_membership WHERE instance_id = ").param().sql(" ), - other_aa_instances AS ( + other_direct_aa_instances AS ( SELECT anti_affinity_group_instance_membership.group_id,instance_id FROM anti_affinity_group_instance_membership - JOIN our_aa_groups - ON anti_affinity_group_instance_membership.group_id = our_aa_groups.group_id + JOIN our_direct_aa_groups + ON anti_affinity_group_instance_membership.group_id = our_direct_aa_groups.group_id WHERE instance_id != ").param().sql(" ), + our_indirect_aa_groups AS ( + -- Anti-affinity groups to which our instance's affinity groups belong + SELECT + anti_affinity_group_id, + affinity_group_id, + CASE + WHEN COUNT(*) OVER (PARTITION BY anti_affinity_group_id) = 1 THEN TRUE + ELSE FALSE + END AS only_one_affinity_group + FROM anti_affinity_group_affinity_membership + WHERE affinity_group_id IN (SELECT group_id FROM our_a_groups) + ), + other_indirect_aa_instances_via_instances AS ( + SELECT anti_affinity_group_id AS group_id,instance_id + FROM anti_affinity_group_instance_membership + JOIN our_indirect_aa_groups + ON anti_affinity_group_instance_membership.group_id = our_indirect_aa_groups.anti_affinity_group_id + ), + other_indirect_aa_instances_via_groups AS ( + SELECT anti_affinity_group_id AS group_id,instance_id + FROM affinity_group_instance_membership + JOIN our_indirect_aa_groups + ON affinity_group_instance_membership.group_id = our_indirect_aa_groups.affinity_group_id + WHERE + -- If our instance belongs to exactly one of these groups... + CASE WHEN our_indirect_aa_groups.only_one_affinity_group + -- ... exclude that group from our anti-affinity + THEN affinity_group_instance_membership.group_id NOT IN (SELECT group_id FROM our_a_groups) + -- ... otherwise, consider all groups anti-affine + ELSE TRUE + END + ), + other_aa_instances AS ( + SELECT * FROM other_direct_aa_instances + UNION + SELECT * FROM other_indirect_aa_instances_via_instances + UNION + SELECT * FROM other_indirect_aa_instances_via_groups + ), banned_instances AS ( SELECT instance_id FROM other_aa_instances @@ -201,18 +293,6 @@ pub fn sled_insert_resource_query( ON sled_resource_vmm.instance_id = banned_instances.instance_id ), - our_a_groups AS ( - SELECT group_id - FROM affinity_group_instance_membership - WHERE instance_id = ").param().sql(" - ), - other_a_instances AS ( - SELECT affinity_group_instance_membership.group_id,instance_id - FROM affinity_group_instance_membership - JOIN our_a_groups - ON affinity_group_instance_membership.group_id = our_a_groups.group_id - WHERE instance_id != ").param().sql(" - ), required_instances AS ( SELECT policy,instance_id FROM other_a_instances diff --git a/nexus/db-queries/tests/output/sled_find_targets_query.sql b/nexus/db-queries/tests/output/sled_find_targets_query.sql index 9de50f47f7f..f230a4b6dcb 100644 --- a/nexus/db-queries/tests/output/sled_find_targets_query.sql +++ b/nexus/db-queries/tests/output/sled_find_targets_query.sql @@ -17,18 +17,75 @@ WITH AND COALESCE(sum(CAST(sled_resource_vmm.reservoir_ram AS INT8)), 0) + $3 <= sled.reservoir_size ), - our_aa_groups - AS (SELECT group_id FROM anti_affinity_group_instance_membership WHERE instance_id = $4), - other_aa_instances + our_a_groups AS (SELECT group_id FROM affinity_group_instance_membership WHERE instance_id = $4), + other_a_instances + AS ( + SELECT + affinity_group_instance_membership.group_id, instance_id + FROM + affinity_group_instance_membership + JOIN our_a_groups ON affinity_group_instance_membership.group_id = our_a_groups.group_id + WHERE + instance_id != $5 + ), + our_direct_aa_groups + AS (SELECT group_id FROM anti_affinity_group_instance_membership WHERE instance_id = $6), + other_direct_aa_instances AS ( SELECT anti_affinity_group_instance_membership.group_id, instance_id FROM anti_affinity_group_instance_membership - JOIN our_aa_groups ON - anti_affinity_group_instance_membership.group_id = our_aa_groups.group_id + JOIN our_direct_aa_groups ON + anti_affinity_group_instance_membership.group_id = our_direct_aa_groups.group_id WHERE - instance_id != $5 + instance_id != $7 + ), + our_indirect_aa_groups + AS ( + SELECT + anti_affinity_group_id, + affinity_group_id, + CASE + WHEN count(*) OVER (PARTITION BY anti_affinity_group_id) = 1 THEN true + ELSE false + END + AS exactly_one_affinity_group + FROM + anti_affinity_group_affinity_membership + WHERE + affinity_group_id IN (SELECT group_id FROM our_a_groups) + ), + other_indirect_aa_instances_via_instances + AS ( + SELECT + anti_affinity_group_id AS group_id, instance_id + FROM + anti_affinity_group_instance_membership + JOIN our_indirect_aa_groups ON + anti_affinity_group_instance_membership.group_id + = our_indirect_aa_groups.anti_affinity_group_id + ), + other_indirect_aa_instances_via_groups + AS ( + SELECT + anti_affinity_group_id AS group_id, instance_id + FROM + affinity_group_instance_membership + JOIN our_indirect_aa_groups ON + affinity_group_instance_membership.group_id = our_indirect_aa_groups.affinity_group_id + WHERE + CASE + WHEN our_indirect_aa_groups.exactly_one_affinity_group + THEN affinity_group_instance_membership.group_id NOT IN (SELECT group_id FROM our_a_groups) + ELSE true + END + ), + other_aa_instances + AS ( + SELECT * FROM other_direct_aa_instances + UNION SELECT * FROM other_indirect_aa_instances_via_instances + UNION SELECT * FROM other_indirect_aa_instances_via_groups ), other_aa_instances_by_policy AS ( @@ -51,17 +108,6 @@ WITH JOIN sled_resource_vmm ON sled_resource_vmm.instance_id = other_aa_instances_by_policy.instance_id ), - our_a_groups AS (SELECT group_id FROM affinity_group_instance_membership WHERE instance_id = $6), - other_a_instances - AS ( - SELECT - affinity_group_instance_membership.group_id, instance_id - FROM - affinity_group_instance_membership - JOIN our_a_groups ON affinity_group_instance_membership.group_id = our_a_groups.group_id - WHERE - instance_id != $7 - ), other_a_instances_by_policy AS ( SELECT diff --git a/nexus/db-queries/tests/output/sled_insert_resource_query.sql b/nexus/db-queries/tests/output/sled_insert_resource_query.sql index 9cfb68e3008..a0ae17555dd 100644 --- a/nexus/db-queries/tests/output/sled_insert_resource_query.sql +++ b/nexus/db-queries/tests/output/sled_insert_resource_query.sql @@ -20,18 +20,75 @@ WITH AND COALESCE(sum(CAST(sled_resource_vmm.reservoir_ram AS INT8)), 0) + $4 <= sled.reservoir_size ), - our_aa_groups - AS (SELECT group_id FROM anti_affinity_group_instance_membership WHERE instance_id = $5), - other_aa_instances + our_a_groups AS (SELECT group_id FROM affinity_group_instance_membership WHERE instance_id = $5), + other_a_instances + AS ( + SELECT + affinity_group_instance_membership.group_id, instance_id + FROM + affinity_group_instance_membership + JOIN our_a_groups ON affinity_group_instance_membership.group_id = our_a_groups.group_id + WHERE + instance_id != $6 + ), + our_direct_aa_groups + AS (SELECT group_id FROM anti_affinity_group_instance_membership WHERE instance_id = $7), + other_direct_aa_instances AS ( SELECT anti_affinity_group_instance_membership.group_id, instance_id FROM anti_affinity_group_instance_membership - JOIN our_aa_groups ON - anti_affinity_group_instance_membership.group_id = our_aa_groups.group_id + JOIN our_direct_aa_groups ON + anti_affinity_group_instance_membership.group_id = our_direct_aa_groups.group_id WHERE - instance_id != $6 + instance_id != $8 + ), + our_indirect_aa_groups + AS ( + SELECT + anti_affinity_group_id, + affinity_group_id, + CASE + WHEN count(*) OVER (PARTITION BY anti_affinity_group_id) = 1 THEN true + ELSE false + END + AS only_one_affinity_group + FROM + anti_affinity_group_affinity_membership + WHERE + affinity_group_id IN (SELECT group_id FROM our_a_groups) + ), + other_indirect_aa_instances_via_instances + AS ( + SELECT + anti_affinity_group_id AS group_id, instance_id + FROM + anti_affinity_group_instance_membership + JOIN our_indirect_aa_groups ON + anti_affinity_group_instance_membership.group_id + = our_indirect_aa_groups.anti_affinity_group_id + ), + other_indirect_aa_instances_via_groups + AS ( + SELECT + anti_affinity_group_id AS group_id, instance_id + FROM + affinity_group_instance_membership + JOIN our_indirect_aa_groups ON + affinity_group_instance_membership.group_id = our_indirect_aa_groups.affinity_group_id + WHERE + CASE + WHEN our_indirect_aa_groups.only_one_affinity_group + THEN affinity_group_instance_membership.group_id NOT IN (SELECT group_id FROM our_a_groups) + ELSE true + END + ), + other_aa_instances + AS ( + SELECT * FROM other_direct_aa_instances + UNION SELECT * FROM other_indirect_aa_instances_via_instances + UNION SELECT * FROM other_indirect_aa_instances_via_groups ), banned_instances AS ( @@ -54,17 +111,6 @@ WITH banned_instances JOIN sled_resource_vmm ON sled_resource_vmm.instance_id = banned_instances.instance_id ), - our_a_groups AS (SELECT group_id FROM affinity_group_instance_membership WHERE instance_id = $7), - other_a_instances - AS ( - SELECT - affinity_group_instance_membership.group_id, instance_id - FROM - affinity_group_instance_membership - JOIN our_a_groups ON affinity_group_instance_membership.group_id = our_a_groups.group_id - WHERE - instance_id != $8 - ), required_instances AS ( SELECT diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index ce6e4ccb2ec..f788b1f610e 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -4214,6 +4214,25 @@ CREATE INDEX IF NOT EXISTS lookup_anti_affinity_group_instance_membership_by_ins instance_id ); +-- Describes an affinity group's membership within an anti-affinity group +-- +-- Since the naming here is a little confusing: +-- This allows an anti-affinity group to contain affinity groups as members. +-- This is useful for saying "I want these groups of VMMs to be anti-affine from +-- one another", rather than "I want individual VMMs to be anti-affine from each other". +CREATE TABLE IF NOT EXISTS omicron.public.anti_affinity_group_affinity_membership ( + anti_affinity_group_id UUID NOT NULL, + affinity_group_id UUID NOT NULL, + + PRIMARY KEY (anti_affinity_group_id, affinity_group_id) +); + +-- We need to look up all memberships of an affinity group so we can revoke these +-- memberships efficiently when affinity groups are deleted +CREATE INDEX IF NOT EXISTS lookup_anti_affinity_group_affinity_membership_by_affinity_group ON omicron.public.anti_affinity_group_affinity_membership ( + affinity_group_id +); + -- Per-VMM state. CREATE TABLE IF NOT EXISTS omicron.public.vmm ( id UUID PRIMARY KEY, From daeb2e1a08b5c6cfe1e3fc81d8a848717d3c5b1b Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 21 Feb 2025 11:42:19 -0800 Subject: [PATCH 02/16] Update API --- openapi/nexus.json | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/openapi/nexus.json b/openapi/nexus.json index 17088ed5c9c..ec00fdab967 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -12504,6 +12504,25 @@ "type", "value" ] + }, + { + "description": "An affinity group belonging to this group, identified by UUID.", + "type": "object", + "properties": { + "type": { + "type": "string", + "enum": [ + "affinity_group" + ] + }, + "value": { + "$ref": "#/components/schemas/TypedUuidForAffinityGroupKind" + } + }, + "required": [ + "type", + "value" + ] } ] }, @@ -23053,6 +23072,10 @@ } } }, + "TypedUuidForAffinityGroupKind": { + "type": "string", + "format": "uuid" + }, "TypedUuidForInstanceKind": { "type": "string", "format": "uuid" From 2d6504804b8f8bbb2d53512fafa0f74c037d21fb Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 21 Feb 2025 13:00:58 -0800 Subject: [PATCH 03/16] Schema migration for db updates --- nexus/db-model/src/schema_versions.rs | 3 ++- schema/crdb/anti-affinity-group-affinity-member/up01.sql | 6 ++++++ schema/crdb/anti-affinity-group-affinity-member/up02.sql | 4 ++++ schema/crdb/dbinit.sql | 2 +- 4 files changed, 13 insertions(+), 2 deletions(-) create mode 100644 schema/crdb/anti-affinity-group-affinity-member/up01.sql create mode 100644 schema/crdb/anti-affinity-group-affinity-member/up02.sql diff --git a/nexus/db-model/src/schema_versions.rs b/nexus/db-model/src/schema_versions.rs index 6ba30a87623..c59975f9ee0 100644 --- a/nexus/db-model/src/schema_versions.rs +++ b/nexus/db-model/src/schema_versions.rs @@ -16,7 +16,7 @@ use std::{collections::BTreeMap, sync::LazyLock}; /// /// 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(127, 0, 0); +pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(128, 0, 0); /// List of all past database schema versions, in *reverse* order /// @@ -28,6 +28,7 @@ static KNOWN_VERSIONS: LazyLock> = LazyLock::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(128, "anti-affinity-group-affinity-member"), KnownVersion::new(127, "sled-resource-for-vmm"), KnownVersion::new(126, "affinity"), KnownVersion::new(125, "blueprint-disposition-expunged-cleanup"), diff --git a/schema/crdb/anti-affinity-group-affinity-member/up01.sql b/schema/crdb/anti-affinity-group-affinity-member/up01.sql new file mode 100644 index 00000000000..4e16d622466 --- /dev/null +++ b/schema/crdb/anti-affinity-group-affinity-member/up01.sql @@ -0,0 +1,6 @@ +CREATE TABLE IF NOT EXISTS omicron.public.anti_affinity_group_affinity_membership ( + anti_affinity_group_id UUID NOT NULL, + affinity_group_id UUID NOT NULL, + + PRIMARY KEY (anti_affinity_group_id, affinity_group_id) +); diff --git a/schema/crdb/anti-affinity-group-affinity-member/up02.sql b/schema/crdb/anti-affinity-group-affinity-member/up02.sql new file mode 100644 index 00000000000..459c36df1bb --- /dev/null +++ b/schema/crdb/anti-affinity-group-affinity-member/up02.sql @@ -0,0 +1,4 @@ +CREATE INDEX IF NOT EXISTS lookup_anti_affinity_group_affinity_membership_by_affinity_group ON omicron.public.anti_affinity_group_affinity_membership ( + affinity_group_id +); + diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index e5291893e7a..402ab650efb 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -4977,7 +4977,7 @@ INSERT INTO omicron.public.db_metadata ( version, target_version ) VALUES - (TRUE, NOW(), NOW(), '127.0.0', NULL) + (TRUE, NOW(), NOW(), '128.0.0', NULL) ON CONFLICT DO NOTHING; COMMIT; From 4e6362de35618a8636ef9bdc16e3ad8de98ac446 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Mon, 24 Feb 2025 08:07:43 -0800 Subject: [PATCH 04/16] boilerplate, gotta fix pagination on list next --- nexus/db-queries/src/db/datastore/affinity.rs | 81 ++++++++-- nexus/external-api/src/lib.rs | 42 +++++- nexus/src/app/affinity.rs | 81 +++++++++- nexus/src/external_api/http_entrypoints.rs | 142 +++++++++++++++++- nexus/types/src/external_api/params.rs | 6 + 5 files changed, 329 insertions(+), 23 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/affinity.rs b/nexus/db-queries/src/db/datastore/affinity.rs index 56caf545e5e..e07239fdffa 100644 --- a/nexus/db-queries/src/db/datastore/affinity.rs +++ b/nexus/db-queries/src/db/datastore/affinity.rs @@ -365,7 +365,7 @@ impl DataStore { .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) } - pub async fn anti_affinity_group_member_list( + pub async fn anti_affinity_group_member_instance_list( &self, opctx: &OpContext, authz_anti_affinity_group: &authz::AntiAffinityGroup, @@ -373,6 +373,13 @@ impl DataStore { ) -> ListResultVec { opctx.authorize(authz::Action::Read, authz_anti_affinity_group).await?; + // TODO: Need to also look up "group_membership" here + // TODO: definitely test listing both? + // TODO: atlernately - make this API "instance member" specific, make a + // different one for "affinity group members", and paginate over both. + // + // That might be preferable. + use db::schema::anti_affinity_group_instance_membership::dsl; match pagparams { PaginatedBy::Id(pagparams) => paginated( @@ -1804,7 +1811,11 @@ mod tests { }; let pagbyid = PaginatedBy::Id(pagparams_id); let members = datastore - .anti_affinity_group_member_list(&opctx, &authz_group, &pagbyid) + .anti_affinity_group_member_instance_list( + &opctx, + &authz_group, + &pagbyid, + ) .await .unwrap(); assert!(members.is_empty()); @@ -1832,7 +1843,11 @@ 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_instance_list( + &opctx, + &authz_group, + &pagbyid, + ) .await .unwrap(); assert_eq!(members.len(), 1); @@ -1855,7 +1870,11 @@ mod tests { .await .unwrap(); let members = datastore - .anti_affinity_group_member_list(&opctx, &authz_group, &pagbyid) + .anti_affinity_group_member_instance_list( + &opctx, + &authz_group, + &pagbyid, + ) .await .unwrap(); assert!(members.is_empty()); @@ -2036,7 +2055,11 @@ mod tests { }; let pagbyid = PaginatedBy::Id(pagparams_id); let members = datastore - .anti_affinity_group_member_list(&opctx, &authz_group, &pagbyid) + .anti_affinity_group_member_instance_list( + &opctx, + &authz_group, + &pagbyid, + ) .await .unwrap(); assert!(members.is_empty()); @@ -2100,7 +2123,11 @@ 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_instance_list( + &opctx, + &authz_group, + &pagbyid, + ) .await .unwrap(); assert_eq!(members.len(), 1); @@ -2124,7 +2151,11 @@ mod tests { .await .unwrap(); let members = datastore - .anti_affinity_group_member_list(&opctx, &authz_group, &pagbyid) + .anti_affinity_group_member_instance_list( + &opctx, + &authz_group, + &pagbyid, + ) .await .unwrap(); assert!(members.is_empty()); @@ -2242,7 +2273,11 @@ mod tests { }; let pagbyid = PaginatedBy::Id(pagparams_id); let members = datastore - .anti_affinity_group_member_list(&opctx, &authz_group, &pagbyid) + .anti_affinity_group_member_instance_list( + &opctx, + &authz_group, + &pagbyid, + ) .await .unwrap(); assert!(members.is_empty()); @@ -2274,7 +2309,11 @@ mod tests { // Confirm that no instance members exist let members = datastore - .anti_affinity_group_member_list(&opctx, &authz_group, &pagbyid) + .anti_affinity_group_member_instance_list( + &opctx, + &authz_group, + &pagbyid, + ) .await .unwrap(); assert!(members.is_empty()); @@ -2401,7 +2440,11 @@ mod tests { }; let pagbyid = PaginatedBy::Id(pagparams_id); let members = datastore - .anti_affinity_group_member_list(&opctx, &authz_group, &pagbyid) + .anti_affinity_group_member_instance_list( + &opctx, + &authz_group, + &pagbyid, + ) .await .unwrap(); assert!(members.is_empty()); @@ -2438,7 +2481,11 @@ mod tests { // Confirm that no instance members exist let members = datastore - .anti_affinity_group_member_list(&opctx, &authz_group, &pagbyid) + .anti_affinity_group_member_instance_list( + &opctx, + &authz_group, + &pagbyid, + ) .await .unwrap(); assert!(members.is_empty()); @@ -2974,7 +3021,11 @@ mod tests { }; let pagbyid = PaginatedBy::Id(pagparams_id); let members = datastore - .anti_affinity_group_member_list(&opctx, &authz_group, &pagbyid) + .anti_affinity_group_member_instance_list( + &opctx, + &authz_group, + &pagbyid, + ) .await .unwrap(); assert_eq!(members.len(), 1); @@ -3012,7 +3063,11 @@ mod tests { ); let members = datastore - .anti_affinity_group_member_list(&opctx, &authz_group, &pagbyid) + .anti_affinity_group_member_instance_list( + &opctx, + &authz_group, + &pagbyid, + ) .await .unwrap(); assert!(members.is_empty()); diff --git a/nexus/external-api/src/lib.rs b/nexus/external-api/src/lib.rs index c843090eddc..0f6b23286dc 100644 --- a/nexus/external-api/src/lib.rs +++ b/nexus/external-api/src/lib.rs @@ -1409,7 +1409,7 @@ pub trait NexusExternalApi { path_params: Path, ) -> Result>, HttpError>; - /// Fetch an anti-affinity group member + /// Fetch an anti-affinity group member (where that member is an instance) #[endpoint { method = GET, path = "/v1/anti-affinity-groups/{anti_affinity_group}/members/instance/{instance}", @@ -1421,7 +1421,7 @@ pub trait NexusExternalApi { path_params: Path, ) -> Result, HttpError>; - /// Add a member to an anti-affinity group + /// Add a member to an anti-affinity group (where that member is an instance) #[endpoint { method = POST, path = "/v1/anti-affinity-groups/{anti_affinity_group}/members/instance/{instance}", @@ -1433,7 +1433,7 @@ pub trait NexusExternalApi { path_params: Path, ) -> Result, HttpError>; - /// Remove a member from an anti-affinity group + /// Remove a member from an anti-affinity group (where that member is an instance) #[endpoint { method = DELETE, path = "/v1/anti-affinity-groups/{anti_affinity_group}/members/instance/{instance}", @@ -1445,6 +1445,42 @@ pub trait NexusExternalApi { path_params: Path, ) -> Result; + /// Fetch an anti-affinity group member (where that member is an affinity group) + #[endpoint { + method = GET, + path = "/v1/anti-affinity-groups/{anti_affinity_group}/members/affinity-group/{affinity_group}", + tags = ["affinity"], + }] + async fn anti_affinity_group_member_affinity_group_view( + rqctx: RequestContext, + query_params: Query, + path_params: Path, + ) -> Result, HttpError>; + + /// Add a member to an anti-affinity group (where that member is an affinity group) + #[endpoint { + method = POST, + path = "/v1/anti-affinity-groups/{anti_affinity_group}/members/affinity-group/{affinity_group}", + tags = ["affinity"], + }] + async fn anti_affinity_group_member_affinity_group_add( + rqctx: RequestContext, + query_params: Query, + path_params: Path, + ) -> Result, HttpError>; + + /// Remove a member from an anti-affinity group (where that member is an affinity group) + #[endpoint { + method = DELETE, + path = "/v1/anti-affinity-groups/{anti_affinity_group}/members/affinity-group/{affinity_group}", + tags = ["affinity"], + }] + async fn anti_affinity_group_member_affinity_group_delete( + rqctx: RequestContext, + query_params: Query, + path_params: Path, + ) -> Result; + /// Create an anti-affinity group #[endpoint { method = POST, diff --git a/nexus/src/app/affinity.rs b/nexus/src/app/affinity.rs index ca8d0c708cd..11c2fb10cea 100644 --- a/nexus/src/app/affinity.rs +++ b/nexus/src/app/affinity.rs @@ -21,6 +21,7 @@ use omicron_common::api::external::ListResultVec; use omicron_common::api::external::LookupResult; use omicron_common::api::external::NameOrId; use omicron_common::api::external::UpdateResult; +use omicron_uuid_kinds::AffinityGroupUuid; use omicron_uuid_kinds::GenericUuid; use omicron_uuid_kinds::InstanceUuid; @@ -260,7 +261,7 @@ impl super::Nexus { .await?; Ok(self .db_datastore - .anti_affinity_group_member_list( + .anti_affinity_group_member_instance_list( opctx, &authz_anti_affinity_group, pagparams, @@ -290,7 +291,7 @@ impl super::Nexus { .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<'_>, @@ -313,6 +314,29 @@ impl super::Nexus { .await } + pub(crate) async fn anti_affinity_group_member_affinity_group_view( + &self, + opctx: &OpContext, + anti_affinity_group_lookup: &lookup::AntiAffinityGroup<'_>, + affinity_group_lookup: &lookup::AffinityGroup<'_>, + ) -> Result { + let (.., authz_anti_affinity_group) = + anti_affinity_group_lookup.lookup_for(authz::Action::Read).await?; + let (.., authz_affinity_group) = + affinity_group_lookup.lookup_for(authz::Action::Read).await?; + let member = external::AntiAffinityGroupMember::AffinityGroup( + AffinityGroupUuid::from_untyped_uuid(authz_affinity_group.id()), + ); + + self.db_datastore + .anti_affinity_group_member_view( + opctx, + &authz_anti_affinity_group, + member, + ) + .await + } + pub(crate) async fn affinity_group_member_add( &self, opctx: &OpContext, @@ -337,7 +361,7 @@ impl super::Nexus { Ok(member) } - 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<'_>, @@ -362,6 +386,31 @@ impl super::Nexus { Ok(member) } + pub(crate) async fn anti_affinity_group_member_affinity_group_add( + &self, + opctx: &OpContext, + anti_affinity_group_lookup: &lookup::AntiAffinityGroup<'_>, + affinity_group_lookup: &lookup::AffinityGroup<'_>, + ) -> Result { + let (.., authz_anti_affinity_group) = anti_affinity_group_lookup + .lookup_for(authz::Action::Modify) + .await?; + let (.., authz_affinity_group) = + affinity_group_lookup.lookup_for(authz::Action::Read).await?; + let member = external::AntiAffinityGroupMember::AffinityGroup( + AffinityGroupUuid::from_untyped_uuid(authz_affinity_group.id()), + ); + + self.db_datastore + .anti_affinity_group_member_add( + opctx, + &authz_anti_affinity_group, + member.clone(), + ) + .await?; + Ok(member) + } + pub(crate) async fn affinity_group_member_delete( &self, opctx: &OpContext, @@ -381,7 +430,7 @@ impl super::Nexus { .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<'_>, @@ -404,4 +453,28 @@ impl super::Nexus { ) .await } + + pub(crate) async fn anti_affinity_group_member_affinity_group_delete( + &self, + opctx: &OpContext, + anti_affinity_group_lookup: &lookup::AntiAffinityGroup<'_>, + affinity_group_lookup: &lookup::AffinityGroup<'_>, + ) -> Result<(), Error> { + let (.., authz_anti_affinity_group) = anti_affinity_group_lookup + .lookup_for(authz::Action::Modify) + .await?; + let (.., authz_affinity_group) = + affinity_group_lookup.lookup_for(authz::Action::Read).await?; + let member = external::AntiAffinityGroupMember::AffinityGroup( + AffinityGroupUuid::from_untyped_uuid(authz_affinity_group.id()), + ); + + self.db_datastore + .anti_affinity_group_member_delete( + opctx, + &authz_anti_affinity_group, + member, + ) + .await + } } diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index b1cf5b59e5a..d9298f1af36 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -2983,7 +2983,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 +3029,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 +3074,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, @@ -3089,6 +3089,142 @@ impl NexusExternalApi for NexusExternalApiImpl { .await } + async fn anti_affinity_group_member_affinity_group_view( + rqctx: RequestContext, + query_params: Query, + path_params: Path, + ) -> Result, HttpError> { + let apictx = rqctx.context(); + let handler = async { + let nexus = &apictx.context.nexus; + let path = path_params.into_inner(); + let opctx = + crate::context::op_context_for_external_api(&rqctx).await?; + let query = query_params.into_inner(); + + // Select anti-affinity group + let group_selector = params::AntiAffinityGroupSelector { + anti_affinity_group: path.anti_affinity_group, + project: query.project.clone(), + }; + let group_lookup = + nexus.anti_affinity_group_lookup(&opctx, group_selector)?; + + // Select affinity group + let affinity_group_selector = params::AffinityGroupSelector { + project: query.project, + affinity_group: path.affinity_group, + }; + let affinity_group_lookup = + nexus.affinity_group_lookup(&opctx, affinity_group_selector)?; + + let group = nexus + .anti_affinity_group_member_affinity_group_view( + &opctx, + &group_lookup, + &affinity_group_lookup, + ) + .await?; + + Ok(HttpResponseOk(group)) + }; + apictx + .context + .external_latencies + .instrument_dropshot_handler(&rqctx, handler) + .await + } + + async fn anti_affinity_group_member_affinity_group_add( + rqctx: RequestContext, + query_params: Query, + path_params: Path, + ) -> Result, HttpError> { + let apictx = rqctx.context(); + let handler = async { + let opctx = + crate::context::op_context_for_external_api(&rqctx).await?; + let nexus = &apictx.context.nexus; + let path = path_params.into_inner(); + let query = query_params.into_inner(); + + // Select anti-affinity group + let group_selector = params::AntiAffinityGroupSelector { + anti_affinity_group: path.anti_affinity_group, + project: query.project.clone(), + }; + let group_lookup = + nexus.anti_affinity_group_lookup(&opctx, group_selector)?; + + // Select affinity group + let affinity_group_selector = params::AffinityGroupSelector { + project: query.project, + affinity_group: path.affinity_group, + }; + let affinity_group_lookup = + nexus.affinity_group_lookup(&opctx, affinity_group_selector)?; + + let member = nexus + .anti_affinity_group_member_affinity_group_add( + &opctx, + &group_lookup, + &affinity_group_lookup, + ) + .await?; + Ok(HttpResponseCreated(member)) + }; + apictx + .context + .external_latencies + .instrument_dropshot_handler(&rqctx, handler) + .await + } + + async fn anti_affinity_group_member_affinity_group_delete( + rqctx: RequestContext, + query_params: Query, + path_params: Path, + ) -> Result { + let apictx = rqctx.context(); + let handler = async { + let opctx = + crate::context::op_context_for_external_api(&rqctx).await?; + let nexus = &apictx.context.nexus; + let path = path_params.into_inner(); + let query = query_params.into_inner(); + + // Select anti-affinity group + let group_selector = params::AntiAffinityGroupSelector { + anti_affinity_group: path.anti_affinity_group, + project: query.project.clone(), + }; + let group_lookup = + nexus.anti_affinity_group_lookup(&opctx, group_selector)?; + + // Select affinity group + let affinity_group_selector = params::AffinityGroupSelector { + project: query.project, + affinity_group: path.affinity_group, + }; + let affinity_group_lookup = + nexus.affinity_group_lookup(&opctx, affinity_group_selector)?; + + nexus + .anti_affinity_group_member_affinity_group_delete( + &opctx, + &group_lookup, + &affinity_group_lookup, + ) + .await?; + Ok(HttpResponseDeleted()) + }; + apictx + .context + .external_latencies + .instrument_dropshot_handler(&rqctx, handler) + .await + } + async fn anti_affinity_group_create( rqctx: RequestContext, query_params: Query, diff --git a/nexus/types/src/external_api/params.rs b/nexus/types/src/external_api/params.rs index e87acd4e590..5614f25c3dc 100644 --- a/nexus/types/src/external_api/params.rs +++ b/nexus/types/src/external_api/params.rs @@ -839,6 +839,12 @@ pub struct AntiAffinityInstanceGroupMemberPath { pub instance: NameOrId, } +#[derive(Clone, Debug, Serialize, Deserialize, JsonSchema)] +pub struct AntiAffinityAffinityGroupMemberPath { + pub anti_affinity_group: NameOrId, + pub affinity_group: NameOrId, +} + /// Create-time parameters for an `AntiAffinityGroup` #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] pub struct AntiAffinityGroupCreate { From f50749daa9d945be7a64ca0dadc18d13681a329c Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Mon, 24 Feb 2025 11:37:34 -0800 Subject: [PATCH 05/16] Manual pagination of both tables at once --- common/src/api/external/mod.rs | 8 +- nexus/db-model/src/schema.rs | 1 + nexus/db-queries/src/db/datastore/affinity.rs | 181 ++++++++---------- nexus/src/app/affinity.rs | 13 +- nexus/src/external_api/http_entrypoints.rs | 3 +- 5 files changed, 95 insertions(+), 111 deletions(-) diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index 8b70a4228a5..b422b6d85ff 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -1390,18 +1390,18 @@ 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 affinity group belonging to this group, identified by UUID. AffinityGroup(AffinityGroupUuid), + + /// An instance belonging to this group, identified by UUID. + Instance(InstanceUuid), } impl SimpleIdentity for AntiAffinityGroupMember { fn id(&self) -> Uuid { match self { - AntiAffinityGroupMember::Instance(id) => *id.as_untyped_uuid(), AntiAffinityGroupMember::AffinityGroup(id) => *id.as_untyped_uuid(), + AntiAffinityGroupMember::Instance(id) => *id.as_untyped_uuid(), } } } diff --git a/nexus/db-model/src/schema.rs b/nexus/db-model/src/schema.rs index 954f1d0d0b7..8a7c43f5f82 100644 --- a/nexus/db-model/src/schema.rs +++ b/nexus/db-model/src/schema.rs @@ -2079,6 +2079,7 @@ allow_tables_to_appear_in_same_query!(hw_baseboard_id, inv_sled_agent,); allow_tables_to_appear_in_same_query!( anti_affinity_group, + anti_affinity_group_affinity_membership, anti_affinity_group_instance_membership, affinity_group, affinity_group_instance_membership, diff --git a/nexus/db-queries/src/db/datastore/affinity.rs b/nexus/db-queries/src/db/datastore/affinity.rs index e07239fdffa..d5efeb56925 100644 --- a/nexus/db-queries/src/db/datastore/affinity.rs +++ b/nexus/db-queries/src/db/datastore/affinity.rs @@ -24,6 +24,7 @@ use crate::db::model::AntiAffinityGroupUpdate; use crate::db::model::Name; use crate::db::model::Project; 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; @@ -31,6 +32,7 @@ use diesel::prelude::*; use omicron_common::api::external; use omicron_common::api::external::http_pagination::PaginatedBy; 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; @@ -42,6 +44,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( @@ -365,39 +368,76 @@ impl DataStore { .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) } - pub async fn anti_affinity_group_member_instance_list( + pub async fn anti_affinity_group_member_list( &self, opctx: &OpContext, authz_anti_affinity_group: &authz::AntiAffinityGroup, - pagparams: &PaginatedBy<'_>, - ) -> ListResultVec { + pagparams: &DataPageParams<'_, Uuid>, + ) -> ListResultVec { opctx.authorize(authz::Action::Read, authz_anti_affinity_group).await?; - // TODO: Need to also look up "group_membership" here - // TODO: definitely test listing both? - // TODO: atlernately - make this API "instance member" specific, make a - // different one for "affinity group members", and paginate over both. - // - // That might be preferable. - - use db::schema::anti_affinity_group_instance_membership::dsl; - 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", - )); + let mut query = QueryBuilder::new() + .sql( + " + SELECT instance_id as id, 'instance' as label + FROM anti_affinity_group_instance_membership + WHERE group_id = ", + ) + .param() + .bind::(authz_anti_affinity_group.id()) + .sql( + " + UNION + SELECT affinity_group_id as id, 'affinity_group' as label + FROM anti_affinity_group_affinity_membership + WHERE anti_affinity_group_id = ", + ) + .param() + .bind::(authz_anti_affinity_group.id()) + .sql(" "); + + let (sort, cmp) = match pagparams.direction { + dropshot::PaginationOrder::Ascending => (" ORDER BY id ASC ", ">"), + dropshot::PaginationOrder::Descending => { + (" ORDER BY id DESC ", "<") } - } - .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 let Some(id) = pagparams.marker { + query = query + .sql("WHERE id ") + .sql(cmp) + .sql(" ") + .param() + .bind::(*id); + }; + + query = query.sql(sort); + query = + query.sql(" LIMIT ").param().bind::( + i64::from(pagparams.limit.get()), + ); + + Ok(query + .query::<(diesel::sql_types::Uuid, diesel::sql_types::Text)>() + .load_async::<(Uuid, String)>( + &*self.pool_connection_authorized(opctx).await?, + ) + .await + .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))? + .into_iter() + .map(|(id, label)| { + use external::AntiAffinityGroupMember as Member; + match label.as_str() { + "affinity_group" => Member::AffinityGroup( + AffinityGroupUuid::from_untyped_uuid(id), + ), + "instance" => { + Member::Instance(InstanceUuid::from_untyped_uuid(id)) + } + other => panic!("Unexpected label from query: {other}"), + } + }) + .collect()) } pub async fn affinity_group_member_view( @@ -1804,18 +1844,13 @@ mod tests { .unwrap(); // A new group should have no members - let pagparams_id = DataPageParams { + let pagparams = 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_instance_list( - &opctx, - &authz_group, - &pagbyid, - ) + .anti_affinity_group_member_list(&opctx, &authz_group, &pagparams) .await .unwrap(); assert!(members.is_empty()); @@ -1843,11 +1878,7 @@ mod tests { // We should now be able to list the new member let members = datastore - .anti_affinity_group_member_instance_list( - &opctx, - &authz_group, - &pagbyid, - ) + .anti_affinity_group_member_list(&opctx, &authz_group, &pagparams) .await .unwrap(); assert_eq!(members.len(), 1); @@ -1870,11 +1901,7 @@ mod tests { .await .unwrap(); let members = datastore - .anti_affinity_group_member_instance_list( - &opctx, - &authz_group, - &pagbyid, - ) + .anti_affinity_group_member_list(&opctx, &authz_group, &pagparams) .await .unwrap(); assert!(members.is_empty()); @@ -2048,18 +2075,13 @@ mod tests { .unwrap(); // A new group should have no members - let pagparams_id = DataPageParams { + let pagparams = 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_instance_list( - &opctx, - &authz_group, - &pagbyid, - ) + .anti_affinity_group_member_list(&opctx, &authz_group, &pagparams) .await .unwrap(); assert!(members.is_empty()); @@ -2123,11 +2145,7 @@ mod tests { // We should now be able to list the new member let members = datastore - .anti_affinity_group_member_instance_list( - &opctx, - &authz_group, - &pagbyid, - ) + .anti_affinity_group_member_list(&opctx, &authz_group, &pagparams) .await .unwrap(); assert_eq!(members.len(), 1); @@ -2151,11 +2169,7 @@ mod tests { .await .unwrap(); let members = datastore - .anti_affinity_group_member_instance_list( - &opctx, - &authz_group, - &pagbyid, - ) + .anti_affinity_group_member_list(&opctx, &authz_group, &pagparams) .await .unwrap(); assert!(members.is_empty()); @@ -2266,18 +2280,13 @@ mod tests { .unwrap(); // A new group should have no members - let pagparams_id = DataPageParams { + let pagparams = 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_instance_list( - &opctx, - &authz_group, - &pagbyid, - ) + .anti_affinity_group_member_list(&opctx, &authz_group, &pagparams) .await .unwrap(); assert!(members.is_empty()); @@ -2309,11 +2318,7 @@ mod tests { // Confirm that no instance members exist let members = datastore - .anti_affinity_group_member_instance_list( - &opctx, - &authz_group, - &pagbyid, - ) + .anti_affinity_group_member_list(&opctx, &authz_group, &pagparams) .await .unwrap(); assert!(members.is_empty()); @@ -2433,18 +2438,13 @@ mod tests { .unwrap(); // A new group should have no members - let pagparams_id = DataPageParams { + let pagparams = 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_instance_list( - &opctx, - &authz_group, - &pagbyid, - ) + .anti_affinity_group_member_list(&opctx, &authz_group, &pagparams) .await .unwrap(); assert!(members.is_empty()); @@ -2481,11 +2481,7 @@ mod tests { // Confirm that no instance members exist let members = datastore - .anti_affinity_group_member_instance_list( - &opctx, - &authz_group, - &pagbyid, - ) + .anti_affinity_group_member_list(&opctx, &authz_group, &pagparams) .await .unwrap(); assert!(members.is_empty()); @@ -3014,18 +3010,13 @@ mod tests { // // Two calls to "anti_affinity_group_member_add" should be the same // as a single call. - let pagparams_id = DataPageParams { + let pagparams = 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_instance_list( - &opctx, - &authz_group, - &pagbyid, - ) + .anti_affinity_group_member_list(&opctx, &authz_group, &pagparams) .await .unwrap(); assert_eq!(members.len(), 1); @@ -3063,11 +3054,7 @@ mod tests { ); let members = datastore - .anti_affinity_group_member_instance_list( - &opctx, - &authz_group, - &pagbyid, - ) + .anti_affinity_group_member_list(&opctx, &authz_group, &pagparams) .await .unwrap(); assert!(members.is_empty()); diff --git a/nexus/src/app/affinity.rs b/nexus/src/app/affinity.rs index 11c2fb10cea..bd19311fe94 100644 --- a/nexus/src/app/affinity.rs +++ b/nexus/src/app/affinity.rs @@ -15,6 +15,7 @@ use nexus_types::external_api::views; use omicron_common::api::external; use omicron_common::api::external::http_pagination::PaginatedBy; 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; @@ -254,22 +255,18 @@ impl super::Nexus { &self, opctx: &OpContext, anti_affinity_group_lookup: &lookup::AntiAffinityGroup<'_>, - pagparams: &PaginatedBy<'_>, + pagparams: &DataPageParams<'_, uuid::Uuid>, ) -> ListResultVec { let (.., authz_anti_affinity_group) = anti_affinity_group_lookup .lookup_for(authz::Action::ListChildren) .await?; - Ok(self - .db_datastore - .anti_affinity_group_member_instance_list( + 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( diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index d9298f1af36..246cc86c551 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -2925,7 +2925,6 @@ impl NexusExternalApi for NexusExternalApiImpl { 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 group_selector = params::AntiAffinityGroupSelector { project: scan_params.selector.project.clone(), @@ -2937,7 +2936,7 @@ impl NexusExternalApi for NexusExternalApiImpl { .anti_affinity_group_member_list( &opctx, &group_lookup, - &paginated_by, + &pag_params, ) .await?; Ok(HttpResponseOk(ScanById::results_page( From 167a2d865af57072289ca279d721ea4a66cf2f2c Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Tue, 25 Feb 2025 12:06:04 -0800 Subject: [PATCH 06/16] Clippy --- nexus/db-queries/src/db/datastore/affinity.rs | 12 ++++++------ nexus/db-queries/src/db/datastore/sled.rs | 8 ++++---- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/affinity.rs b/nexus/db-queries/src/db/datastore/affinity.rs index 3797560aca9..858778b89db 100644 --- a/nexus/db-queries/src/db/datastore/affinity.rs +++ b/nexus/db-queries/src/db/datastore/affinity.rs @@ -1711,7 +1711,7 @@ mod tests { .unwrap(); assert_eq!(members.len(), 1); assert_eq!( - external::AffinityGroupMember::Instance(instance,), + external::AffinityGroupMember::Instance(instance), members[0].clone().into() ); @@ -1800,8 +1800,8 @@ mod tests { .unwrap(); assert_eq!(members.len(), 1); assert_eq!( - external::AntiAffinityGroupMember::Instance(instance,), - members[0].clone().into() + external::AntiAffinityGroupMember::Instance(instance), + members[0].clone() ); // We can delete the member and observe an empty member list @@ -1915,7 +1915,7 @@ mod tests { .unwrap(); assert_eq!(members.len(), 1); assert_eq!( - external::AffinityGroupMember::Instance(instance,), + external::AffinityGroupMember::Instance(instance), members[0].clone().into() ); @@ -2029,8 +2029,8 @@ mod tests { .unwrap(); assert_eq!(members.len(), 1); assert_eq!( - external::AntiAffinityGroupMember::Instance(instance,), - members[0].clone().into() + external::AntiAffinityGroupMember::Instance(instance), + members[0].clone() ); // We can delete the member and observe an empty member list -- even diff --git a/nexus/db-queries/src/db/datastore/sled.rs b/nexus/db-queries/src/db/datastore/sled.rs index 0ee75fbd478..29c5ab4b35e 100644 --- a/nexus/db-queries/src/db/datastore/sled.rs +++ b/nexus/db-queries/src/db/datastore/sled.rs @@ -2248,7 +2248,7 @@ pub(in crate::db::datastore) mod test { let db = TestDatabase::new_with_datastore(&logctx.log).await; let (opctx, datastore) = (db.opctx(), db.datastore()); let (authz_project, _project) = - create_project(&opctx, &datastore).await; + create_project(&opctx, &datastore, "project").await; const SLED_COUNT: usize = 4; let sleds = create_sleds(&datastore, SLED_COUNT).await; @@ -2325,7 +2325,7 @@ pub(in crate::db::datastore) mod test { let db = TestDatabase::new_with_datastore(&logctx.log).await; let (opctx, datastore) = (db.opctx(), db.datastore()); let (authz_project, _project) = - create_project(&opctx, &datastore).await; + create_project(&opctx, &datastore, "project").await; const SLED_COUNT: usize = 5; let sleds = create_sleds(&datastore, SLED_COUNT).await; @@ -2408,7 +2408,7 @@ pub(in crate::db::datastore) mod test { let db = TestDatabase::new_with_datastore(&logctx.log).await; let (opctx, datastore) = (db.opctx(), db.datastore()); let (authz_project, _project) = - create_project(&opctx, &datastore).await; + create_project(&opctx, &datastore, "project").await; const SLED_COUNT: usize = 3; let sleds = create_sleds(&datastore, SLED_COUNT).await; @@ -2496,7 +2496,7 @@ pub(in crate::db::datastore) mod test { let db = TestDatabase::new_with_datastore(&logctx.log).await; let (opctx, datastore) = (db.opctx(), db.datastore()); let (authz_project, _project) = - create_project(&opctx, &datastore).await; + create_project(&opctx, &datastore, "project").await; const SLED_COUNT: usize = 4; let sleds = create_sleds(&datastore, SLED_COUNT).await; From 17887d4db77a4fc7ab6e800aa4f187a3316c1962 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Tue, 25 Feb 2025 12:44:50 -0800 Subject: [PATCH 07/16] please no panic --- nexus/db-queries/src/db/datastore/affinity.rs | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/affinity.rs b/nexus/db-queries/src/db/datastore/affinity.rs index 858778b89db..81f80d0ae7d 100644 --- a/nexus/db-queries/src/db/datastore/affinity.rs +++ b/nexus/db-queries/src/db/datastore/affinity.rs @@ -417,7 +417,7 @@ impl DataStore { i64::from(pagparams.limit.get()), ); - Ok(query + query .query::<(diesel::sql_types::Uuid, diesel::sql_types::Text)>() .load_async::<(Uuid, String)>( &*self.pool_connection_authorized(opctx).await?, @@ -428,16 +428,18 @@ impl DataStore { .map(|(id, label)| { use external::AntiAffinityGroupMember as Member; match label.as_str() { - "affinity_group" => Member::AffinityGroup( + "affinity_group" => Ok(Member::AffinityGroup( AffinityGroupUuid::from_untyped_uuid(id), - ), - "instance" => { - Member::Instance(InstanceUuid::from_untyped_uuid(id)) - } - other => panic!("Unexpected label from query: {other}"), + )), + "instance" => Ok(Member::Instance( + InstanceUuid::from_untyped_uuid(id), + )), + other => Err(external::Error::internal_error(&format!( + "Unexpected label from database query: {other}" + ))), } }) - .collect()) + .collect() } pub async fn affinity_group_member_view( From 5865732ffb42123e660188e79c4bc625eeadf6b6 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Tue, 25 Feb 2025 13:06:36 -0800 Subject: [PATCH 08/16] Safer group-to-group rule respecting --- nexus/db-queries/src/db/datastore/affinity.rs | 46 ++++++++++++++++--- 1 file changed, 40 insertions(+), 6 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/affinity.rs b/nexus/db-queries/src/db/datastore/affinity.rs index 81f80d0ae7d..2562429728b 100644 --- a/nexus/db-queries/src/db/datastore/affinity.rs +++ b/nexus/db-queries/src/db/datastore/affinity.rs @@ -821,7 +821,9 @@ impl DataStore { let err = err.clone(); use db::schema::anti_affinity_group::dsl as anti_affinity_group_dsl; use db::schema::affinity_group::dsl as affinity_group_dsl; - use db::schema::anti_affinity_group_affinity_membership::dsl as membership_dsl; + use db::schema::affinity_group_instance_membership::dsl as a_instance_membership_dsl; + use db::schema::anti_affinity_group_affinity_membership::dsl as aa_affinity_membership_dsl; + use db::schema::sled_resource_vmm::dsl as resource_dsl; async move { // Check that the anti-affinity group exists @@ -861,12 +863,44 @@ impl DataStore { }) })?; - // TODO: It's possible that the affinity group has members - // which are already running. We should probably check this, - // and prevent it, otherwise we could circumvent "policy = - // fail" stances. - diesel::insert_into(membership_dsl::anti_affinity_group_affinity_membership) + // Check that the affinity group's members are not reserved. + let has_reservation: bool = diesel::select( + diesel::dsl::exists( + a_instance_membership_dsl::affinity_group_instance_membership + .inner_join( + resource_dsl::sled_resource_vmm + .on(resource_dsl::instance_id.eq(a_instance_membership_dsl::instance_id.nullable())) + ) + .filter(a_instance_membership_dsl::group_id.eq( + affinity_group_id.into_untyped_uuid() + )) + ) + ).get_result_async(&conn) + .await + .map_err(|e| { + err.bail_retryable_or_else(e, |e| { + public_error_from_diesel( + e, + ErrorHandler::Server, + ) + }) + })?; + + // NOTE: This check prevents us from violating affinity rules with "policy = + // fail" stances, but it is possible that running instances already would + // satisfy the affinity rules proposed by this new group addition. + // + // It would be possible to remove this error if we replaced it with affinity + // rule checks. + if has_reservation { + return Err(err.bail(Error::invalid_request( + "Affinity group with running instances cannot be \ + added to anti-affinity group. Try stopping them first.".to_string() + ))); + } + + diesel::insert_into(aa_affinity_membership_dsl::anti_affinity_group_affinity_membership) .values(AntiAffinityGroupAffinityMembership::new( AntiAffinityGroupUuid::from_untyped_uuid(authz_anti_affinity_group.id()), affinity_group_id, From 6ab1c67c4129d56945de4df4c86af5814607d56a Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Tue, 25 Feb 2025 15:02:19 -0800 Subject: [PATCH 09/16] Add integration test for group of group --- nexus/tests/integration_tests/affinity.rs | 313 ++++++++++++++-------- 1 file changed, 206 insertions(+), 107 deletions(-) diff --git a/nexus/tests/integration_tests/affinity.rs b/nexus/tests/integration_tests/affinity.rs index 1549db51cba..89a5aa80a9f 100644 --- a/nexus/tests/integration_tests/affinity.rs +++ b/nexus/tests/integration_tests/affinity.rs @@ -162,78 +162,60 @@ impl ProjectScopedApiHelper<'_, T> { object_get_error(&self.client, &url, status).await } - async fn group_member_add(&self, group: &str, instance: &str) -> T::Member { - let url = group_member_instance_url( - T::URL_COMPONENT, - self.project, - group, - instance, - ); + async fn group_member_add( + &self, + group: &str, + member: &str, + ) -> T::Member { + let url = M::url(T::URL_COMPONENT, self.project, group, member); object_create(&self.client, &url, &()).await } - async fn group_member_add_expect_error( + async fn group_member_add_expect_error( &self, group: &str, - instance: &str, + member: &str, status: StatusCode, ) -> HttpErrorResponseBody { - let url = group_member_instance_url( - T::URL_COMPONENT, - self.project, - group, - instance, - ); + let url = M::url(T::URL_COMPONENT, self.project, group, member); object_create_error(&self.client, &url, &(), status).await } - async fn group_member_get(&self, group: &str, instance: &str) -> T::Member { - let url = group_member_instance_url( - T::URL_COMPONENT, - self.project, - group, - instance, - ); + async fn group_member_get( + &self, + group: &str, + member: &str, + ) -> T::Member { + let url = M::url(T::URL_COMPONENT, self.project, group, member); object_get(&self.client, &url).await } - async fn group_member_get_expect_error( + async fn group_member_get_expect_error( &self, group: &str, - instance: &str, + member: &str, status: StatusCode, ) -> HttpErrorResponseBody { - let url = group_member_instance_url( - T::URL_COMPONENT, - self.project, - group, - instance, - ); + let url = M::url(T::URL_COMPONENT, self.project, group, member); object_get_error(&self.client, &url, status).await } - async fn group_member_delete(&self, group: &str, instance: &str) { - let url = group_member_instance_url( - T::URL_COMPONENT, - self.project, - group, - instance, - ); + async fn group_member_delete( + &self, + group: &str, + member: &str, + ) { + let url = M::url(T::URL_COMPONENT, self.project, group, member); object_delete(&self.client, &url).await } - async fn group_member_delete_expect_error( + async fn group_member_delete_expect_error( &self, group: &str, - instance: &str, + member: &str, status: StatusCode, ) -> HttpErrorResponseBody { - let url = group_member_instance_url( - T::URL_COMPONENT, - self.project, - group, - instance, - ); + let url = M::url(T::URL_COMPONENT, self.project, group, member); object_delete_error(&self.client, &url, status).await } } @@ -416,14 +398,44 @@ fn group_members_url(ty: &str, project: Option<&str>, group: &str) -> String { format!("/v1/{ty}/{group}/members{query_params}") } -fn group_member_instance_url( - ty: &str, - project: Option<&str>, - group: &str, - instance: &str, -) -> String { - let query_params = project_query_param_suffix(project); - format!("/v1/{ty}/{group}/members/instance/{instance}{query_params}") +/// Describes shared logic between "things that can be group members". +trait GroupMemberish { + fn url( + ty: &str, + project: Option<&str>, + group: &str, + member: &str, + ) -> String; +} + +struct MemberInstance {} + +impl GroupMemberish for MemberInstance { + fn url( + ty: &str, + project: Option<&str>, + group: &str, + member: &str, + ) -> String { + let query_params = project_query_param_suffix(project); + format!("/v1/{ty}/{group}/members/instance/{member}{query_params}") + } +} + +struct MemberAffinityGroup {} + +impl GroupMemberish for MemberAffinityGroup { + fn url( + ty: &str, + project: Option<&str>, + group: &str, + member: &str, + ) -> String { + let query_params = project_query_param_suffix(project); + format!( + "/v1/{ty}/{group}/members/affinity-group/{member}{query_params}" + ) + } } #[nexus_test(extra_sled_agents = 2)] @@ -484,7 +496,10 @@ async fn test_affinity_group_usage(cptestctx: &ControlPlaneTestContext) { // Add these instances to an affinity group for instance in &instances { project_api - .group_member_add(GROUP_NAME, &instance.identity.name.to_string()) + .group_member_add::( + GROUP_NAME, + &instance.identity.name.to_string(), + ) .await; } @@ -495,7 +510,10 @@ async fn test_affinity_group_usage(cptestctx: &ControlPlaneTestContext) { // We can also list each member for instance in &instances { project_api - .group_member_get(GROUP_NAME, instance.identity.name.as_str()) + .group_member_get::( + GROUP_NAME, + instance.identity.name.as_str(), + ) .await; } @@ -543,8 +561,8 @@ async fn test_anti_affinity_group_usage(cptestctx: &ControlPlaneTestContext) { const PROJECT_NAME: &'static str = "test-project"; const GROUP_NAME: &'static str = "group"; + const AFFINITY_GROUP_NAME: &'static str = "a-group"; const EXPECTED_SLEDS: usize = 3; - const INSTANCE_COUNT: usize = EXPECTED_SLEDS; let api = ApiHelper::new(external_client); @@ -562,69 +580,118 @@ async fn test_anti_affinity_group_usage(cptestctx: &ControlPlaneTestContext) { create_default_ip_pool(&external_client).await; api.create_project(PROJECT_NAME).await; - let project_api = api.use_project::(PROJECT_NAME); + let aa_project_api = api.use_project::(PROJECT_NAME); + let a_project_api = api.use_project::(PROJECT_NAME); - let mut instances = Vec::new(); - for i in 0..INSTANCE_COUNT { - instances.push( - project_api - .create_stopped_instance(&format!("test-instance-{i}")) + // Create both stopped instances and some affinity groups. + // + // All but two of the instances are going to be anti-affine from each other. + let mut aa_instances = Vec::new(); + for i in 0..EXPECTED_SLEDS - 1 { + aa_instances.push( + aa_project_api + .create_stopped_instance(&format!("test-aa-instance-{i}")) .await, ); } + // These two instances will be affine with each other, but anti-affine from + // everything else (indirectly) through their affinity group. + let mut a_instances = Vec::new(); + a_project_api.group_create(AFFINITY_GROUP_NAME).await; + for i in 0..2 { + let instance = a_project_api + .create_stopped_instance(&format!("test-a-instance-{i}")) + .await; + a_project_api + .group_member_add::( + AFFINITY_GROUP_NAME, + &instance.identity.name.to_string(), + ) + .await; + a_instances.push(instance); + } // When we start, we observe no anti-affinity groups - let groups = project_api.groups_list().await; + let groups = aa_project_api.groups_list().await; assert!(groups.is_empty()); // We can now create a group and observe it - let group = project_api.group_create(GROUP_NAME).await; + let group = aa_project_api.group_create(GROUP_NAME).await; // We can list it and also GET the group specifically - let groups = project_api.groups_list().await; + let groups = aa_project_api.groups_list().await; assert_eq!(groups.len(), 1); assert_eq!(groups[0].identity.id, group.identity.id); - let observed_group = project_api.group_get(GROUP_NAME).await; + let observed_group = aa_project_api.group_get(GROUP_NAME).await; assert_eq!(observed_group.identity.id, group.identity.id); // List all members of the anti-affinity group (expect nothing) - let members = project_api.group_members_list(GROUP_NAME).await; + let members = aa_project_api.group_members_list(GROUP_NAME).await; assert!(members.is_empty()); // Add these instances to the anti-affinity group - for instance in &instances { - project_api - .group_member_add(GROUP_NAME, &instance.identity.name.to_string()) + for instance in &aa_instances { + aa_project_api + .group_member_add::( + GROUP_NAME, + &instance.identity.name.to_string(), + ) .await; } + // Add the affinity group to the anti-affinity group + aa_project_api + .group_member_add::( + GROUP_NAME, + AFFINITY_GROUP_NAME, + ) + .await; - // List members again (expect all instances) - let members = project_api.group_members_list(GROUP_NAME).await; - assert_eq!(members.len(), instances.len()); + // List members again (expect all instances, and the affinity group) + let members = aa_project_api.group_members_list(GROUP_NAME).await; + assert_eq!(members.len(), aa_instances.len() + 1); // We can also list each member - for instance in &instances { - project_api - .group_member_get(GROUP_NAME, instance.identity.name.as_str()) + for instance in &aa_instances { + aa_project_api + .group_member_get::( + GROUP_NAME, + instance.identity.name.as_str(), + ) .await; } + aa_project_api + .group_member_get::( + GROUP_NAME, + AFFINITY_GROUP_NAME, + ) + .await; // Start the instances we created earlier. // // We don't actually care that they're "running" from the perspective of the // simulated sled agent, we just want placement to be triggered from Nexus. - for instance in &instances { + for instance in &aa_instances { + api.start_instance(&instance).await; + } + for instance in &a_instances { api.start_instance(&instance).await; } - let mut expected_instances = instances + let mut expected_aa_instances = aa_instances + .iter() + .map(|instance| instance.identity.id) + .collect::>(); + let mut expected_a_instances = a_instances .iter() .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. + // + // The only exception will be a single sled containing both of the + // affine instances, which should be separate from all others. for sled in &sleds { let observed_instances = api .sled_instance_list(&sled.identity.id.to_string()) @@ -633,22 +700,39 @@ async fn test_anti_affinity_group_usage(cptestctx: &ControlPlaneTestContext) { .map(|sled_instance| sled_instance.identity.id) .collect::>(); - assert_eq!( - observed_instances.len(), - 1, - "All instances should be placed on distinct sleds" - ); - - assert!( - expected_instances.remove(&observed_instances[0]), - "The instance {} was observed on multiple sleds", - observed_instances[0] - ); + match observed_instances.len() { + 1 => { + // This should be one of the anti-affine instances + assert!( + expected_aa_instances.remove(&observed_instances[0]), + "The instance {} was observed too many times", + observed_instances[0] + ); + } + 2 => { + // This should be one of the affine instances, anti-affine from + // others because of their affinity groups + for observed_instance in observed_instances { + assert!( + expected_a_instances.remove(&observed_instance), + "The instance {} was observed too many times", + observed_instance + ); + } + } + _ => panic!( + "Unexpected instance count on sled: {observed_instances:?}" + ), + } } assert!( - expected_instances.is_empty(), - "Did not find allocations for some instances: {expected_instances:?}" + expected_aa_instances.is_empty(), + "Did not find allocations for some anti-affine instances: {expected_aa_instances:?}" + ); + assert!( + expected_a_instances.is_empty(), + "Did not find allocations for some affine instances: {expected_a_instances:?}" ); } @@ -702,9 +786,11 @@ async fn test_group_crud(client: &ClientTestContext) { // Add the instance to the affinity group let instance_name = &instance.identity.name.to_string(); - project_api.group_member_add(GROUP_NAME, &instance_name).await; + project_api + .group_member_add::(GROUP_NAME, &instance_name) + .await; let response = project_api - .group_member_add_expect_error( + .group_member_add_expect_error::( GROUP_NAME, &instance_name, StatusCode::BAD_REQUEST, @@ -723,13 +809,18 @@ async fn test_group_crud(client: &ClientTestContext) { let members = project_api.group_members_list(GROUP_NAME).await; assert_eq!(members.len(), 1); project_api - .group_member_get(GROUP_NAME, instance.identity.name.as_str()) + .group_member_get::( + GROUP_NAME, + instance.identity.name.as_str(), + ) .await; // Delete the member, observe that it is gone - project_api.group_member_delete(GROUP_NAME, &instance_name).await; project_api - .group_member_delete_expect_error( + .group_member_delete::(GROUP_NAME, &instance_name) + .await; + project_api + .group_member_delete_expect_error::( GROUP_NAME, &instance_name, StatusCode::NOT_FOUND, @@ -738,7 +829,7 @@ async fn test_group_crud(client: &ClientTestContext) { let members = project_api.group_members_list(GROUP_NAME).await; assert_eq!(members.len(), 0); project_api - .group_member_get_expect_error( + .group_member_get_expect_error::( GROUP_NAME, &instance_name, StatusCode::NOT_FOUND, @@ -840,21 +931,29 @@ async fn test_group_project_selector( // Group Members can be added by name or UUID let instance_name = instance.identity.name.as_str(); let instance_id = instance.identity.id.to_string(); - project_api.group_member_add(GROUP_NAME, instance_name).await; - project_api.group_member_delete(GROUP_NAME, instance_name).await; - no_project_api.group_member_add(&group_id, &instance_id).await; - no_project_api.group_member_delete(&group_id, &instance_id).await; + project_api + .group_member_add::(GROUP_NAME, instance_name) + .await; + project_api + .group_member_delete::(GROUP_NAME, instance_name) + .await; + no_project_api + .group_member_add::(&group_id, &instance_id) + .await; + no_project_api + .group_member_delete::(&group_id, &instance_id) + .await; // Trying to use UUIDs with the project selector is invalid project_api - .group_member_add_expect_error( + .group_member_add_expect_error::( GROUP_NAME, &instance_id, StatusCode::BAD_REQUEST, ) .await; project_api - .group_member_add_expect_error( + .group_member_add_expect_error::( &group_id, instance_name, StatusCode::BAD_REQUEST, @@ -863,21 +962,21 @@ async fn test_group_project_selector( // Using any names without the project selector is invalid no_project_api - .group_member_add_expect_error( + .group_member_add_expect_error::( GROUP_NAME, &instance_id, StatusCode::BAD_REQUEST, ) .await; no_project_api - .group_member_add_expect_error( + .group_member_add_expect_error::( &group_id, instance_name, StatusCode::BAD_REQUEST, ) .await; no_project_api - .group_member_add_expect_error( + .group_member_add_expect_error::( GROUP_NAME, instance_name, StatusCode::BAD_REQUEST, From 0cb05b5271e80f19e62799544c75c42a4ac21aac Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Tue, 25 Feb 2025 16:03:50 -0800 Subject: [PATCH 10/16] Expanding testing, improving deletion semantics --- nexus/db-queries/src/db/datastore/affinity.rs | 419 +++++++++++++++--- 1 file changed, 367 insertions(+), 52 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/affinity.rs b/nexus/db-queries/src/db/datastore/affinity.rs index 2562429728b..842062bc330 100644 --- a/nexus/db-queries/src/db/datastore/affinity.rs +++ b/nexus/db-queries/src/db/datastore/affinity.rs @@ -228,17 +228,35 @@ impl DataStore { })?; // Ensure all memberships in the affinity group are deleted - use db::schema::affinity_group_instance_membership::dsl as member_dsl; - diesel::delete(member_dsl::affinity_group_instance_membership) - .filter(member_dsl::group_id.eq(authz_affinity_group.id())) - .execute_async(&conn) - .await - .map_err(|e| { - err.bail_retryable_or_else(e, |e| { - public_error_from_diesel(e, ErrorHandler::Server) - }) - })?; + { + use db::schema::affinity_group_instance_membership::dsl as member_dsl; + diesel::delete(member_dsl::affinity_group_instance_membership) + .filter(member_dsl::group_id.eq(authz_affinity_group.id())) + .execute_async(&conn) + .await + .map_err(|e| { + err.bail_retryable_or_else(e, |e| { + public_error_from_diesel(e, ErrorHandler::Server) + }) + })?; + } + // If this affinity group is a member in other anti-affinity + // groups, remove those memberships + // + // TODO: This needs testing + { + use db::schema::anti_affinity_group_affinity_membership::dsl as member_dsl; + diesel::delete(member_dsl::anti_affinity_group_affinity_membership) + .filter(member_dsl::affinity_group_id.eq(authz_affinity_group.id())) + .execute_async(&conn) + .await + .map_err(|e| { + err.bail_retryable_or_else(e, |e| { + public_error_from_diesel(e, ErrorHandler::Server) + }) + })?; + } Ok(()) } }) @@ -315,17 +333,31 @@ impl DataStore { }) })?; - // Ensure all memberships in the anti affinity group are deleted - use db::schema::anti_affinity_group_instance_membership::dsl as member_dsl; - diesel::delete(member_dsl::anti_affinity_group_instance_membership) - .filter(member_dsl::group_id.eq(authz_anti_affinity_group.id())) - .execute_async(&conn) - .await - .map_err(|e| { - err.bail_retryable_or_else(e, |e| { - public_error_from_diesel(e, ErrorHandler::Server) - }) - })?; + // Ensure all memberships in the anti-affinity group are deleted + { + use db::schema::anti_affinity_group_instance_membership::dsl as member_dsl; + diesel::delete(member_dsl::anti_affinity_group_instance_membership) + .filter(member_dsl::group_id.eq(authz_anti_affinity_group.id())) + .execute_async(&conn) + .await + .map_err(|e| { + err.bail_retryable_or_else(e, |e| { + public_error_from_diesel(e, ErrorHandler::Server) + }) + })?; + } + { + use db::schema::anti_affinity_group_affinity_membership::dsl as member_dsl; + diesel::delete(member_dsl::anti_affinity_group_affinity_membership) + .filter(member_dsl::anti_affinity_group_id.eq(authz_anti_affinity_group.id())) + .execute_async(&conn) + .await + .map_err(|e| { + err.bail_retryable_or_else(e, |e| { + public_error_from_diesel(e, ErrorHandler::Server) + }) + })?; + } Ok(()) } @@ -896,7 +928,7 @@ impl DataStore { if has_reservation { return Err(err.bail(Error::invalid_request( "Affinity group with running instances cannot be \ - added to anti-affinity group. Try stopping them first.".to_string() + added to an anti-affinity group. Try stopping them first.".to_string() ))); } @@ -2090,6 +2122,167 @@ mod tests { logctx.cleanup_successful(); } + #[tokio::test] + async fn anti_affinity_group_membership_add_remove_group_with_vmm() { + // Setup + let logctx = dev::test_setup_log( + "anti_affinity_group_membership_add_remove_group_with_vmm", + ); + 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(); + + // Also, create an affinity group. It'll be a member within the + // anti-affinity group. + let affinity_group = create_affinity_group( + &opctx, + &datastore, + &authz_project, + "my-affinity-group", + ) + .await + .unwrap(); + let affinity_group_id = + AffinityGroupUuid::from_untyped_uuid(affinity_group.id()); + + let (.., authz_aa_group) = LookupPath::new(opctx, datastore) + .anti_affinity_group_id(group.id()) + .lookup_for(authz::Action::Modify) + .await + .unwrap(); + let (.., authz_a_group) = LookupPath::new(opctx, datastore) + .affinity_group_id(affinity_group.id()) + .lookup_for(authz::Action::Modify) + .await + .unwrap(); + + // A new group should have no members + let pagparams = DataPageParams { + marker: None, + limit: NonZeroU32::new(100).unwrap(), + direction: dropshot::PaginationOrder::Ascending, + }; + let members = datastore + .anti_affinity_group_member_list( + &opctx, + &authz_aa_group, + &pagparams, + ) + .await + .unwrap(); + assert!(members.is_empty()); + + // Create an instance without a VMM. + let instance = create_stopped_instance_record( + &opctx, + &datastore, + &authz_project, + "my-instance", + ) + .await; + + // Add the instance to the affinity group + datastore + .affinity_group_member_add( + &opctx, + &authz_a_group, + external::AffinityGroupMember::Instance(instance), + ) + .await + .unwrap(); + + // Reserve the VMM for the instance. + allocate_instance_reservation(&datastore, instance).await; + + // Now, we cannot add the affinity group to the anti-affinity group + let err = datastore + .anti_affinity_group_member_add( + &opctx, + &authz_aa_group, + external::AntiAffinityGroupMember::AffinityGroup(affinity_group_id), + ) + .await + .expect_err( + "Shouldn't be able to add running instances to anti-affinity groups", + ); + assert!(matches!(err, Error::InvalidRequest { .. })); + assert!( + err.to_string().contains( + "Affinity group with running instances cannot be added to an anti-affinity group" + ), + "{err:?}" + ); + + // If we have no reservation for the affinity group, we can add it to the + // anti-affinity group. + delete_instance_reservation(&datastore, instance).await; + datastore + .anti_affinity_group_member_add( + &opctx, + &authz_aa_group, + external::AntiAffinityGroupMember::AffinityGroup( + affinity_group_id, + ), + ) + .await + .unwrap(); + + // Now we can reserve a sled for the instance once more. + allocate_instance_reservation(&datastore, instance).await; + + // We should now be able to list the new member + let members = datastore + .anti_affinity_group_member_list( + &opctx, + &authz_aa_group, + &pagparams, + ) + .await + .unwrap(); + assert_eq!(members.len(), 1); + assert_eq!( + external::AntiAffinityGroupMember::AffinityGroup(affinity_group_id), + members[0].clone() + ); + + // We can delete the member and observe an empty member list -- even + // though it has an instance which is running! + datastore + .anti_affinity_group_member_delete( + &opctx, + &authz_aa_group, + external::AntiAffinityGroupMember::AffinityGroup( + affinity_group_id, + ), + ) + .await + .unwrap(); + let members = datastore + .anti_affinity_group_member_list( + &opctx, + &authz_aa_group, + &pagparams, + ) + .await + .unwrap(); + assert!(members.is_empty()); + + // Clean up. + db.terminate().await; + logctx.cleanup_successful(); + } + #[tokio::test] async fn affinity_group_delete_group_deletes_members() { // Setup @@ -2182,7 +2375,16 @@ mod tests { .await .unwrap(); - let (.., authz_group) = LookupPath::new(opctx, datastore) + let affinity_group = create_affinity_group( + &opctx, + &datastore, + &authz_project, + "my-affinity-group", + ) + .await + .unwrap(); + + let (.., authz_aa_group) = LookupPath::new(opctx, datastore) .anti_affinity_group_id(group.id()) .lookup_for(authz::Action::Modify) .await @@ -2195,7 +2397,11 @@ mod tests { direction: dropshot::PaginationOrder::Ascending, }; let members = datastore - .anti_affinity_group_member_list(&opctx, &authz_group, &pagparams) + .anti_affinity_group_member_list( + &opctx, + &authz_aa_group, + &pagparams, + ) .await .unwrap(); assert!(members.is_empty()); @@ -2211,24 +2417,42 @@ mod tests { datastore .anti_affinity_group_member_add( &opctx, - &authz_group, + &authz_aa_group, external::AntiAffinityGroupMember::Instance(instance), ) .await .unwrap(); + // Also, add the affinity member to the anti-affinity group as a member + datastore + .anti_affinity_group_member_add( + &opctx, + &authz_aa_group, + external::AntiAffinityGroupMember::AffinityGroup( + AffinityGroupUuid::from_untyped_uuid(affinity_group.id()), + ), + ) + .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, &pagparams) + .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; @@ -2563,17 +2787,60 @@ mod tests { let (authz_project, ..) = create_project(&opctx, &datastore, "my-project").await; + enum Member { + Instance, + AffinityGroup, + } + + impl Member { + fn resource_type(&self) -> ResourceType { + match self { + Member::Instance => ResourceType::Instance, + Member::AffinityGroup => ResourceType::AffinityGroup, + } + } + } + struct TestArgs { // Does the group exist? group: bool, - // Does the instance exist? - instance: bool, + // What's the type of the member? + member_type: Member, + // Does the member exist? + member: bool, } let args = [ - TestArgs { group: false, instance: false }, - TestArgs { group: true, instance: false }, - TestArgs { group: false, instance: true }, + TestArgs { + group: false, + member_type: Member::Instance, + member: false, + }, + TestArgs { + group: true, + member_type: Member::Instance, + member: false, + }, + TestArgs { + group: false, + member_type: Member::Instance, + member: true, + }, + TestArgs { + group: false, + member_type: Member::AffinityGroup, + member: false, + }, + TestArgs { + group: true, + member_type: Member::AffinityGroup, + member: false, + }, + TestArgs { + group: false, + member_type: Member::AffinityGroup, + member: true, + }, ]; for arg in args { @@ -2600,7 +2867,7 @@ mod tests { .unwrap(); } - // Create an instance, and maybe delete it. + // Create an instance let instance = create_stopped_instance_record( &opctx, &datastore, @@ -2608,32 +2875,78 @@ mod tests { "my-instance", ) .await; + let mut instance_exists = true; + + // Create an affinity group + let affinity_group = create_affinity_group( + &opctx, + &datastore, + &authz_project, + "my-affinity-group", + ) + .await + .unwrap(); + let mut affinity_group_exists = true; + 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) - .await - .unwrap(); + let (.., authz_affinity_group) = LookupPath::new(opctx, datastore) + .affinity_group_id(affinity_group.id()) + .lookup_for(authz::Action::Modify) + .await + .unwrap(); + + if !arg.member { + match arg.member_type { + Member::Instance => { + datastore + .project_delete_instance(&opctx, &authz_instance) + .await + .unwrap(); + instance_exists = false; + } + Member::AffinityGroup => { + datastore + .affinity_group_delete( + &opctx, + &authz_affinity_group, + ) + .await + .unwrap(); + affinity_group_exists = false; + } + } } - // Try to add the instance to the group. + // Try to add the member to the group. // // Expect to see specific errors, depending on whether or not the - // group/instance exist. + // group/member exist. + let member = match arg.member_type { + Member::Instance => { + external::AntiAffinityGroupMember::Instance(instance) + } + Member::AffinityGroup => { + external::AntiAffinityGroupMember::AffinityGroup( + AffinityGroupUuid::from_untyped_uuid( + affinity_group.id(), + ), + ) + } + }; let err = datastore .anti_affinity_group_member_add( &opctx, &authz_group, - external::AntiAffinityGroupMember::Instance(instance), + member.clone(), ) .await .expect_err("Should have failed"); - match (arg.group, arg.instance) { + match (arg.group, arg.member) { (false, _) => { assert!( matches!(err, Error::ObjectNotFound { @@ -2646,7 +2959,7 @@ mod tests { assert!( matches!(err, Error::ObjectNotFound { type_name, .. - } if type_name == ResourceType::Instance), + } if type_name == arg.member_type.resource_type()), "{err:?}" ); } @@ -2657,14 +2970,10 @@ mod tests { // Do the same thing, but for group membership removal. let err = datastore - .anti_affinity_group_member_delete( - &opctx, - &authz_group, - external::AntiAffinityGroupMember::Instance(instance), - ) + .anti_affinity_group_member_delete(&opctx, &authz_group, member) .await .expect_err("Should have failed"); - match (arg.group, arg.instance) { + match (arg.group, arg.member) { (false, _) => { assert!( matches!(err, Error::ObjectNotFound { @@ -2687,12 +2996,18 @@ mod tests { } // Cleanup, if we actually created anything. - if arg.instance { + if instance_exists { datastore .project_delete_instance(&opctx, &authz_instance) .await .unwrap(); } + if affinity_group_exists { + datastore + .affinity_group_delete(&opctx, &authz_affinity_group) + .await + .unwrap(); + } if arg.group { datastore .anti_affinity_group_delete(&opctx, &authz_group) From 9ad231328cf82e6067456c5091af79d0b68156dd Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 26 Feb 2025 12:14:12 -0800 Subject: [PATCH 11/16] More testing --- nexus/db-queries/src/db/datastore/affinity.rs | 97 ++++++++++++++++++- 1 file changed, 95 insertions(+), 2 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/affinity.rs b/nexus/db-queries/src/db/datastore/affinity.rs index 842062bc330..76385b7b5a7 100644 --- a/nexus/db-queries/src/db/datastore/affinity.rs +++ b/nexus/db-queries/src/db/datastore/affinity.rs @@ -243,8 +243,6 @@ impl DataStore { // If this affinity group is a member in other anti-affinity // groups, remove those memberships - // - // TODO: This needs testing { use db::schema::anti_affinity_group_affinity_membership::dsl as member_dsl; diesel::delete(member_dsl::anti_affinity_group_affinity_membership) @@ -2459,6 +2457,101 @@ mod tests { logctx.cleanup_successful(); } + // Since this name is gnarly, just to be clear: + // - Affinity groups can be "members" within anti-affinity groups + // - If one of these memberships is alive when the affinity group is + // deleted, that membership should be automatically removed + // + // Basically, do not keep around a reference to a dead affinity group. + #[tokio::test] + async fn affinity_group_delete_group_deletes_membership_in_anti_affinity_groups( + ) { + // Setup + let logctx = + dev::test_setup_log("affinity_group_delete_group_deletes_membership_in_anti_affinity_groups"); + let db = TestDatabase::new_with_datastore(&logctx.log).await; + let (opctx, datastore) = (db.opctx(), db.datastore()); + + // Create a project and the groups + let (authz_project, ..) = + create_project(&opctx, &datastore, "my-project").await; + let affinity_group = create_affinity_group( + &opctx, + &datastore, + &authz_project, + "affinity", + ) + .await + .unwrap(); + let anti_affinity_group = create_anti_affinity_group( + &opctx, + &datastore, + &authz_project, + "anti-affinity", + ) + .await + .unwrap(); + + let (.., authz_a_group) = LookupPath::new(opctx, datastore) + .affinity_group_id(affinity_group.id()) + .lookup_for(authz::Action::Modify) + .await + .unwrap(); + let (.., authz_aa_group) = LookupPath::new(opctx, datastore) + .anti_affinity_group_id(anti_affinity_group.id()) + .lookup_for(authz::Action::Modify) + .await + .unwrap(); + + // Add the affinity group to the anti-affinity group. + let member = external::AntiAffinityGroupMember::AffinityGroup( + AffinityGroupUuid::from_untyped_uuid(affinity_group.id()), + ); + datastore + .anti_affinity_group_member_add( + &opctx, + &authz_aa_group, + member.clone(), + ) + .await + .unwrap(); + + // Right now, the affinity group is observable + datastore + .anti_affinity_group_member_view( + &opctx, + &authz_aa_group, + member.clone(), + ) + .await + .expect("Group member should be visible - we just added it"); + + // Delete the affinity group (the member) + datastore.affinity_group_delete(&opctx, &authz_a_group).await.unwrap(); + + // The affinity group membership should have been revoked + let err = datastore + .anti_affinity_group_member_view( + &opctx, + &authz_aa_group, + member.clone(), + ) + .await + .expect_err("Group member should no longer exist"); + assert!( + matches!( + err, + Error::ObjectNotFound { type_name, .. } + if type_name == ResourceType::AntiAffinityGroupMember + ), + "Unexpected error: {err:?}" + ); + + // Clean up. + db.terminate().await; + logctx.cleanup_successful(); + } + #[tokio::test] async fn affinity_group_delete_instance_deletes_membership() { // Setup From 56f300a7129734f603e1052ae2240b0237edebb1 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 26 Feb 2025 13:12:10 -0800 Subject: [PATCH 12/16] Fix listing, add listing test --- common/src/api/external/mod.rs | 4 +- nexus/db-queries/src/db/datastore/affinity.rs | 193 +++++++++++++++++- 2 files changed, 185 insertions(+), 12 deletions(-) diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index a5af4647b7e..fa30f94bb20 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -1337,7 +1337,9 @@ impl SimpleIdentity for AffinityGroupMember { /// /// Membership in a group is not exclusive - members may belong to multiple /// affinity / anti-affinity groups. -#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema, PartialEq)] +#[derive( + Clone, Debug, Deserialize, Serialize, JsonSchema, PartialEq, Hash, Eq, +)] #[serde(tag = "type", content = "value", rename_all = "snake_case")] pub enum AntiAffinityGroupMember { /// An affinity group belonging to this group, identified by UUID. diff --git a/nexus/db-queries/src/db/datastore/affinity.rs b/nexus/db-queries/src/db/datastore/affinity.rs index 76385b7b5a7..550d6a3970d 100644 --- a/nexus/db-queries/src/db/datastore/affinity.rs +++ b/nexus/db-queries/src/db/datastore/affinity.rs @@ -406,9 +406,14 @@ impl DataStore { ) -> ListResultVec { opctx.authorize(authz::Action::Read, authz_anti_affinity_group).await?; + let asc = match pagparams.direction { + dropshot::PaginationOrder::Ascending => true, + dropshot::PaginationOrder::Descending => false, + }; + let mut query = QueryBuilder::new() .sql( - " + "SELECT id,label FROM ( SELECT instance_id as id, 'instance' as label FROM anti_affinity_group_instance_membership WHERE group_id = ", @@ -424,24 +429,23 @@ impl DataStore { ) .param() .bind::(authz_anti_affinity_group.id()) - .sql(" "); - - let (sort, cmp) = match pagparams.direction { - dropshot::PaginationOrder::Ascending => (" ORDER BY id ASC ", ">"), - dropshot::PaginationOrder::Descending => { - (" ORDER BY id DESC ", "<") - } - }; + .sql(") "); if let Some(id) = pagparams.marker { query = query .sql("WHERE id ") - .sql(cmp) + .sql(if asc { ">" } else { "<" }) .sql(" ") .param() .bind::(*id); }; - query = query.sql(sort); + query = query.sql(" ORDER BY id "); + if asc { + query = query.sql("ASC "); + } else { + query = query.sql("DESC "); + } + query = query.sql(" LIMIT ").param().bind::( i64::from(pagparams.limit.get()), @@ -1230,6 +1234,7 @@ mod tests { use nexus_types::external_api::params; use omicron_common::api::external::{ self, ByteCount, DataPageParams, IdentityMetadataCreateParams, + SimpleIdentity, }; use omicron_test_utils::dev; use omicron_uuid_kinds::GenericUuid; @@ -1890,6 +1895,172 @@ mod tests { logctx.cleanup_successful(); } + // Anti-affinity group member listing has a slightly more complicated + // implementation, because it queries multiple tables and UNIONs 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 = DataPageParams { + marker: None, + limit: NonZeroU32::new(100).unwrap(), + direction: dropshot::PaginationOrder::Ascending, + }; + let members = datastore + .anti_affinity_group_member_list( + &opctx, + &authz_aa_group, + &pagparams, + ) + .await + .unwrap(); + assert!(members.is_empty()); + + // Add some groups and instances, so we have data to list over. + + const INSTANCE_COUNT: usize = 3; + const AFFINITY_GROUP_COUNT: usize = 3; + + let mut members = Vec::new(); + + for i in 0..INSTANCE_COUNT { + let instance = create_stopped_instance_record( + &opctx, + &datastore, + &authz_project, + &format!("instance-{i}"), + ) + .await; + + // Add the instance as a member to the group + let member = external::AntiAffinityGroupMember::Instance(instance); + datastore + .anti_affinity_group_member_add( + &opctx, + &authz_aa_group, + member.clone(), + ) + .await + .unwrap(); + members.push(member); + } + + for i in 0..AFFINITY_GROUP_COUNT { + let affinity_group = create_affinity_group( + &opctx, + &datastore, + &authz_project, + &format!("affinity-{i}"), + ) + .await + .unwrap(); + + // Add the instance as a member to the group + let member = external::AntiAffinityGroupMember::AffinityGroup( + AffinityGroupUuid::from_untyped_uuid(affinity_group.id()), + ); + datastore + .anti_affinity_group_member_add( + &opctx, + &authz_aa_group, + member.clone(), + ) + .await + .unwrap(); + members.push(member); + } + + // Order by UUID, regardless of member type + members.sort_unstable_by(|m1, m2| m1.id().cmp(&m2.id())); + + // We can list all members + let mut pagparams = 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(); + pagparams.marker = Some(&marker); + 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 + pagparams.marker = Some(&marker); + pagparams.limit = NonZeroU32::new(2).unwrap(); + 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(); + pagparams.marker = None; + pagparams.limit = NonZeroU32::new(100).unwrap(); + pagparams.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(); + } + #[tokio::test] async fn affinity_group_membership_add_remove_instance_with_vmm() { // Setup From 05ebab788d9fb6a861889405c3e568b99b8bdcbd Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 26 Feb 2025 13:27:31 -0800 Subject: [PATCH 13/16] clipperoni --- nexus/db-queries/src/db/datastore/affinity.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nexus/db-queries/src/db/datastore/affinity.rs b/nexus/db-queries/src/db/datastore/affinity.rs index 550d6a3970d..b8ea09161ab 100644 --- a/nexus/db-queries/src/db/datastore/affinity.rs +++ b/nexus/db-queries/src/db/datastore/affinity.rs @@ -1997,7 +1997,7 @@ mod tests { } // Order by UUID, regardless of member type - members.sort_unstable_by(|m1, m2| m1.id().cmp(&m2.id())); + members.sort_unstable_by_key(|m1| m1.id()); // We can list all members let mut pagparams = DataPageParams { From 388b6cd3dea17cf1e95c8f3e77946fd4c40c6f1c Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 26 Feb 2025 16:51:27 -0800 Subject: [PATCH 14/16] Update API --- nexus/external-api/output/nexus_tags.txt | 3 + openapi/nexus.json | 166 +++++++++++++++++++++-- 2 files changed, 160 insertions(+), 9 deletions(-) diff --git a/nexus/external-api/output/nexus_tags.txt b/nexus/external-api/output/nexus_tags.txt index 59ba14e5fb8..f233a45b8d6 100644 --- a/nexus/external-api/output/nexus_tags.txt +++ b/nexus/external-api/output/nexus_tags.txt @@ -12,6 +12,9 @@ affinity_group_view GET /v1/affinity-groups/{affinity_ anti_affinity_group_create POST /v1/anti-affinity-groups anti_affinity_group_delete DELETE /v1/anti-affinity-groups/{anti_affinity_group} anti_affinity_group_list GET /v1/anti-affinity-groups +anti_affinity_group_member_affinity_group_add POST /v1/anti-affinity-groups/{anti_affinity_group}/members/affinity-group/{affinity_group} +anti_affinity_group_member_affinity_group_delete DELETE /v1/anti-affinity-groups/{anti_affinity_group}/members/affinity-group/{affinity_group} +anti_affinity_group_member_affinity_group_view GET /v1/anti-affinity-groups/{anti_affinity_group}/members/affinity-group/{affinity_group} anti_affinity_group_member_instance_add POST /v1/anti-affinity-groups/{anti_affinity_group}/members/instance/{instance} anti_affinity_group_member_instance_delete DELETE /v1/anti-affinity-groups/{anti_affinity_group}/members/instance/{instance} anti_affinity_group_member_instance_view GET /v1/anti-affinity-groups/{anti_affinity_group}/members/instance/{instance} diff --git a/openapi/nexus.json b/openapi/nexus.json index ec00fdab967..e69b7cff1bd 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -1487,12 +1487,160 @@ } } }, + "/v1/anti-affinity-groups/{anti_affinity_group}/members/affinity-group/{affinity_group}": { + "get": { + "tags": [ + "affinity" + ], + "summary": "Fetch an anti-affinity group member (where that member is an affinity group)", + "operationId": "anti_affinity_group_member_affinity_group_view", + "parameters": [ + { + "in": "query", + "name": "project", + "description": "Name or ID of the project", + "schema": { + "$ref": "#/components/schemas/NameOrId" + } + }, + { + "in": "path", + "name": "affinity_group", + "required": true, + "schema": { + "$ref": "#/components/schemas/NameOrId" + } + }, + { + "in": "path", + "name": "anti_affinity_group", + "required": true, + "schema": { + "$ref": "#/components/schemas/NameOrId" + } + } + ], + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/AntiAffinityGroupMember" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + }, + "post": { + "tags": [ + "affinity" + ], + "summary": "Add a member to an anti-affinity group (where that member is an affinity group)", + "operationId": "anti_affinity_group_member_affinity_group_add", + "parameters": [ + { + "in": "query", + "name": "project", + "description": "Name or ID of the project", + "schema": { + "$ref": "#/components/schemas/NameOrId" + } + }, + { + "in": "path", + "name": "affinity_group", + "required": true, + "schema": { + "$ref": "#/components/schemas/NameOrId" + } + }, + { + "in": "path", + "name": "anti_affinity_group", + "required": true, + "schema": { + "$ref": "#/components/schemas/NameOrId" + } + } + ], + "responses": { + "201": { + "description": "successful creation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/AntiAffinityGroupMember" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + }, + "delete": { + "tags": [ + "affinity" + ], + "summary": "Remove a member from an anti-affinity group (where that member is an affinity group)", + "operationId": "anti_affinity_group_member_affinity_group_delete", + "parameters": [ + { + "in": "query", + "name": "project", + "description": "Name or ID of the project", + "schema": { + "$ref": "#/components/schemas/NameOrId" + } + }, + { + "in": "path", + "name": "affinity_group", + "required": true, + "schema": { + "$ref": "#/components/schemas/NameOrId" + } + }, + { + "in": "path", + "name": "anti_affinity_group", + "required": true, + "schema": { + "$ref": "#/components/schemas/NameOrId" + } + } + ], + "responses": { + "204": { + "description": "successful deletion" + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + }, "/v1/anti-affinity-groups/{anti_affinity_group}/members/instance/{instance}": { "get": { "tags": [ "affinity" ], - "summary": "Fetch an anti-affinity group member", + "summary": "Fetch an anti-affinity group member (where that member is an instance)", "operationId": "anti_affinity_group_member_instance_view", "parameters": [ { @@ -1543,7 +1691,7 @@ "tags": [ "affinity" ], - "summary": "Add a member to an anti-affinity group", + "summary": "Add a member to an anti-affinity group (where that member is an instance)", "operationId": "anti_affinity_group_member_instance_add", "parameters": [ { @@ -1594,7 +1742,7 @@ "tags": [ "affinity" ], - "summary": "Remove a member from an anti-affinity group", + "summary": "Remove a member from an anti-affinity group (where that member is an instance)", "operationId": "anti_affinity_group_member_instance_delete", "parameters": [ { @@ -12487,17 +12635,17 @@ "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 affinity group belonging to this group, identified by UUID.", "type": "object", "properties": { "type": { "type": "string", "enum": [ - "instance" + "affinity_group" ] }, "value": { - "$ref": "#/components/schemas/TypedUuidForInstanceKind" + "$ref": "#/components/schemas/TypedUuidForAffinityGroupKind" } }, "required": [ @@ -12506,17 +12654,17 @@ ] }, { - "description": "An affinity group belonging to this group, identified by UUID.", + "description": "An instance belonging to this group, identified by UUID.", "type": "object", "properties": { "type": { "type": "string", "enum": [ - "affinity_group" + "instance" ] }, "value": { - "$ref": "#/components/schemas/TypedUuidForAffinityGroupKind" + "$ref": "#/components/schemas/TypedUuidForInstanceKind" } }, "required": [ From aa9de032193c1a35cc502921750557fc4c92367c Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Thu, 27 Feb 2025 12:29:29 -0800 Subject: [PATCH 15/16] Unauthorized coverage test --- nexus/tests/integration_tests/endpoints.rs | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index 618752227e6..6c2dc7b8db3 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -516,6 +516,17 @@ pub static DEMO_ANTI_AFFINITY_GROUP_INSTANCE_MEMBER_URL: LazyLock = *DEMO_PROJECT_SELECTOR ) }); +pub static DEMO_ANTI_AFFINITY_GROUP_AFFINITY_GROUP_MEMBER_URL: LazyLock< + String, +> = LazyLock::new(|| { + format!( + "/v1/anti-affinity-groups/{}/members/affinity-group/{}?{}", + *DEMO_ANTI_AFFINITY_GROUP_NAME, + *DEMO_AFFINITY_GROUP_NAME, + *DEMO_PROJECT_SELECTOR + ) +}); + pub static DEMO_ANTI_AFFINITY_GROUP_CREATE: LazyLock< params::AntiAffinityGroupCreate, > = LazyLock::new(|| params::AntiAffinityGroupCreate { @@ -1994,6 +2005,17 @@ pub static VERIFY_ENDPOINTS: LazyLock> = AllowedMethod::Post(serde_json::Value::Null), ], }, + VerifyEndpoint { + url: &DEMO_ANTI_AFFINITY_GROUP_AFFINITY_GROUP_MEMBER_URL, + visibility: Visibility::Protected, + unprivileged_access: UnprivilegedAccess::None, + + allowed_methods: vec![ + AllowedMethod::Get, + AllowedMethod::Delete, + AllowedMethod::Post(serde_json::Value::Null), + ], + }, VerifyEndpoint { url: &DEMO_IMPORT_DISK_BULK_WRITE_START_URL, visibility: Visibility::Protected, From 20496ce0aa0958b38aa2544caad6f60b1e1ba1c2 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Thu, 27 Feb 2025 17:04:03 -0800 Subject: [PATCH 16/16] i scream you scream we all scream for authcream --- nexus/tests/integration_tests/unauthorized.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/nexus/tests/integration_tests/unauthorized.rs b/nexus/tests/integration_tests/unauthorized.rs index 1f45597ce74..d6802f06b8c 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 member to the affinity group SetupReq::Post { url: &DEMO_AFFINITY_GROUP_INSTANCE_MEMBER_URL, body: serde_json::Value::Null, @@ -317,12 +317,18 @@ 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 member to the anti-affinity group SetupReq::Post { url: &DEMO_ANTI_AFFINITY_GROUP_INSTANCE_MEMBER_URL, body: serde_json::Value::Null, id_routes: vec![], }, + // Add an affinity group member to the affinity group + SetupReq::Post { + url: &DEMO_ANTI_AFFINITY_GROUP_AFFINITY_GROUP_MEMBER_URL, + body: serde_json::Value::Null, + id_routes: vec![], + }, // Lookup the previously created NIC SetupReq::Get { url: &DEMO_INSTANCE_NIC_URL,