diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index 5823118c29a..e6e901e9bdf 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; @@ -1336,9 +1337,14 @@ 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. + AffinityGroup(AffinityGroupUuid), + /// An instance belonging to this group, identified by UUID. Instance(InstanceUuid), } @@ -1346,6 +1352,7 @@ pub enum AntiAffinityGroupMember { impl SimpleIdentity for AntiAffinityGroupMember { fn id(&self) -> Uuid { match self { + AntiAffinityGroupMember::AffinityGroup(id) => *id.as_untyped_uuid(), AntiAffinityGroupMember::Instance(id) => *id.as_untyped_uuid(), } } diff --git a/nexus/db-model/src/affinity.rs b/nexus/db-model/src/affinity.rs index f5840ba00dd..88adb7e5d82 100644 --- a/nexus/db-model/src/affinity.rs +++ b/nexus/db-model/src/affinity.rs @@ -11,6 +11,7 @@ use super::impl_enum_type; 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 dbbd27a8df6..1109656f798 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, @@ -2083,6 +2090,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-model/src/schema_versions.rs b/nexus/db-model/src/schema_versions.rs index 99059ded940..2051efb6c80 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: Version = Version::new(129, 0, 0); +pub const SCHEMA_VERSION: Version = Version::new(130, 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(130, "anti-affinity-group-affinity-member"), KnownVersion::new(129, "create-target-release"), KnownVersion::new(128, "sled-resource-for-vmm"), KnownVersion::new(127, "bp-disk-disposition-expunged-cleanup"), diff --git a/nexus/db-queries/src/db/datastore/affinity.rs b/nexus/db-queries/src/db/datastore/affinity.rs index c3839e9d2e5..6ef068c58f1 100644 --- a/nexus/db-queries/src/db/datastore/affinity.rs +++ b/nexus/db-queries/src/db/datastore/affinity.rs @@ -18,17 +18,20 @@ 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; 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; use diesel::prelude::*; use omicron_common::api::external; use omicron_common::api::external::CreateResult; +use omicron_common::api::external::DataPageParams; use omicron_common::api::external::DeleteResult; use omicron_common::api::external::Error; use omicron_common::api::external::ListResultVec; @@ -41,6 +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( @@ -224,17 +228,33 @@ 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 + { + 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(()) } }) @@ -311,17 +331,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(()) } @@ -368,28 +402,78 @@ impl DataStore { &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?; - 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 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 = ", + ) + .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(") "); + if let Some(id) = pagparams.marker { + query = query + .sql("WHERE id ") + .sql(if asc { ">" } else { "<" }) + .sql(" ") + .param() + .bind::(*id); + }; + + query = query.sql(" ORDER BY id "); + if asc { + query = query.sql("ASC "); + } else { + query = query.sql("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)) + + query = + query.sql(" LIMIT ").param().bind::( + i64::from(pagparams.limit.get()), + ); + + 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" => Ok(Member::AffinityGroup( + AffinityGroupUuid::from_untyped_uuid(id), + )), + "instance" => Ok(Member::Instance( + InstanceUuid::from_untyped_uuid(id), + )), + other => Err(external::Error::internal_error(&format!( + "Unexpected label from database query: {other}" + ))), + } + }) + .collect() } pub async fn affinity_group_member_view( @@ -433,27 +517,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 +710,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 +842,129 @@ 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::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 + 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()) + ), + ) + }) + })?; + + + // 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 an 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, + )) + .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 +1072,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 +1155,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)] @@ -883,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; @@ -1430,7 +1782,7 @@ mod tests { .unwrap(); assert_eq!(members.len(), 1); assert_eq!( - external::AffinityGroupMember::Instance(instance,), + external::AffinityGroupMember::Instance(instance), members[0].clone().into() ); @@ -1482,14 +1834,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_list(&opctx, &authz_group, &pagbyid) + .anti_affinity_group_member_list(&opctx, &authz_group, &pagparams) .await .unwrap(); assert!(members.is_empty()); @@ -1515,13 +1866,13 @@ mod tests { // We should now be able to list the new member let members = datastore - .anti_affinity_group_member_list(&opctx, &authz_group, &pagbyid) + .anti_affinity_group_member_list(&opctx, &authz_group, &pagparams) .await .unwrap(); assert_eq!(members.len(), 1); assert_eq!( - external::AntiAffinityGroupMember::Instance(instance,), - members[0].clone().into() + external::AntiAffinityGroupMember::Instance(instance), + members[0].clone() ); // We can delete the member and observe an empty member list @@ -1534,7 +1885,7 @@ mod tests { .await .unwrap(); let members = datastore - .anti_affinity_group_member_list(&opctx, &authz_group, &pagbyid) + .anti_affinity_group_member_list(&opctx, &authz_group, &pagparams) .await .unwrap(); assert!(members.is_empty()); @@ -1544,19 +1895,23 @@ 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 affinity_group_membership_add_remove_instance_with_vmm() { + async fn anti_affinity_group_membership_list_extended() { // Setup - let logctx = dev::test_setup_log( - "affinity_group_membership_add_remove_instance_with_vmm", - ); + 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_affinity_group( + let group = create_anti_affinity_group( &opctx, &datastore, &authz_project, @@ -1565,26 +1920,188 @@ mod tests { .await .unwrap(); - let (.., authz_group) = LookupPath::new(opctx, datastore) - .affinity_group_id(group.id()) + let (.., authz_aa_group) = LookupPath::new(opctx, datastore) + .anti_affinity_group_id(group.id()) .lookup_for(authz::Action::Modify) .await .unwrap(); // A new group should have no members - let pagparams_id = DataPageParams { + let pagparams = DataPageParams { marker: None, limit: NonZeroU32::new(100).unwrap(), direction: dropshot::PaginationOrder::Ascending, }; - let pagbyid = PaginatedBy::Id(pagparams_id); let members = datastore - .affinity_group_member_list(&opctx, &authz_group, &pagbyid) + .anti_affinity_group_member_list( + &opctx, + &authz_aa_group, + &pagparams, + ) .await .unwrap(); assert!(members.is_empty()); - // Create an instance with a VMM. + // 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_key(|m1| m1.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 + let logctx = dev::test_setup_log( + "affinity_group_membership_add_remove_instance_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_affinity_group( + &opctx, + &datastore, + &authz_project, + "my-group", + ) + .await + .unwrap(); + + let (.., authz_group) = LookupPath::new(opctx, datastore) + .affinity_group_id(group.id()) + .lookup_for(authz::Action::Modify) + .await + .unwrap(); + + // A new group should have no members + let pagparams_id = DataPageParams { + marker: None, + limit: NonZeroU32::new(100).unwrap(), + direction: dropshot::PaginationOrder::Ascending, + }; + let pagbyid = PaginatedBy::Id(pagparams_id); + let members = datastore + .affinity_group_member_list(&opctx, &authz_group, &pagbyid) + .await + .unwrap(); + assert!(members.is_empty()); + + // Create an instance with a VMM. let instance = create_stopped_instance_record( &opctx, &datastore, @@ -1635,7 +2152,7 @@ mod tests { .unwrap(); assert_eq!(members.len(), 1); assert_eq!( - external::AffinityGroupMember::Instance(instance,), + external::AffinityGroupMember::Instance(instance), members[0].clone().into() ); @@ -1688,14 +2205,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_list(&opctx, &authz_group, &pagbyid) + .anti_affinity_group_member_list(&opctx, &authz_group, &pagparams) .await .unwrap(); assert!(members.is_empty()); @@ -1745,13 +2261,13 @@ mod tests { // We should now be able to list the new member let members = datastore - .anti_affinity_group_member_list(&opctx, &authz_group, &pagbyid) + .anti_affinity_group_member_list(&opctx, &authz_group, &pagparams) .await .unwrap(); assert_eq!(members.len(), 1); assert_eq!( - external::AntiAffinityGroupMember::Instance(instance,), - members[0].clone().into() + external::AntiAffinityGroupMember::Instance(instance), + members[0].clone() ); // We can delete the member and observe an empty member list -- even @@ -1765,7 +2281,168 @@ mod tests { .await .unwrap(); let members = datastore - .anti_affinity_group_member_list(&opctx, &authz_group, &pagbyid) + .anti_affinity_group_member_list(&opctx, &authz_group, &pagparams) + .await + .unwrap(); + assert!(members.is_empty()); + + // Clean up. + db.terminate().await; + 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()); @@ -1867,21 +2544,33 @@ 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 .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_list(&opctx, &authz_group, &pagbyid) + .anti_affinity_group_member_list( + &opctx, + &authz_aa_group, + &pagparams, + ) .await .unwrap(); assert!(members.is_empty()); @@ -1897,24 +2586,138 @@ 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, &pagbyid) + .anti_affinity_group_member_list( + &opctx, + &authz_aa_group, + &pagparams, + ) .await .unwrap(); - assert!(members.is_empty()); + assert!( + members.is_empty(), + "No members should exist, but these do: {members:?}" + ); + + // Clean up. + db.terminate().await; + 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; @@ -2029,14 +2832,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_list(&opctx, &authz_group, &pagbyid) + .anti_affinity_group_member_list(&opctx, &authz_group, &pagparams) .await .unwrap(); assert!(members.is_empty()); @@ -2071,7 +2873,7 @@ mod tests { // Confirm that no instance members exist let members = datastore - .anti_affinity_group_member_list(&opctx, &authz_group, &pagbyid) + .anti_affinity_group_member_list(&opctx, &authz_group, &pagparams) .await .unwrap(); assert!(members.is_empty()); @@ -2250,17 +3052,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 { @@ -2287,7 +3132,7 @@ mod tests { .unwrap(); } - // Create an instance, and maybe delete it. + // Create an instance let instance = create_stopped_instance_record( &opctx, &datastore, @@ -2295,32 +3140,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 { @@ -2333,7 +3224,7 @@ mod tests { assert!( matches!(err, Error::ObjectNotFound { type_name, .. - } if type_name == ResourceType::Instance), + } if type_name == arg.member_type.resource_type()), "{err:?}" ); } @@ -2344,14 +3235,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 { @@ -2374,12 +3261,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) @@ -2580,14 +3473,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_list(&opctx, &authz_group, &pagbyid) + .anti_affinity_group_member_list(&opctx, &authz_group, &pagparams) .await .unwrap(); assert_eq!(members.len(), 1); @@ -2621,7 +3513,7 @@ mod tests { ); let members = datastore - .anti_affinity_group_member_list(&opctx, &authz_group, &pagbyid) + .anti_affinity_group_member_list(&opctx, &authz_group, &pagparams) .await .unwrap(); assert!(members.is_empty()); diff --git a/nexus/db-queries/src/db/datastore/sled.rs b/nexus/db-queries/src/db/datastore/sled.rs index c6ee5345c6e..1697e587863 100644 --- a/nexus/db-queries/src/db/datastore/sled.rs +++ b/nexus/db-queries/src/db/datastore/sled.rs @@ -1362,6 +1362,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(); + } + // This short-circuits some of the logic and checks we normally have when // creating affinity groups, but makes testing easier. async fn add_instance_to_affinity_group( @@ -1465,6 +1490,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 { @@ -2179,6 +2240,333 @@ 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, "project").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, "project").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, "project").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, "project").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/nexus/external-api/output/nexus_tags.txt b/nexus/external-api/output/nexus_tags.txt index a94d31314d3..10a73676ace 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/nexus/external-api/src/lib.rs b/nexus/external-api/src/lib.rs index abcaa0a1122..681cfed4067 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 anti-affinity group member + /// Fetch anti-affinity group member of type 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 member to anti-affinity group + /// Add instance member to anti-affinity group #[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 member from anti-affinity group + /// Remove instance member from anti-affinity group #[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 anti-affinity group member of type 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 affinity group member to anti-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 affinity group member from anti-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 anti-affinity group #[endpoint { method = POST, diff --git a/nexus/src/app/affinity.rs b/nexus/src/app/affinity.rs index e389c24e77b..21e10b226a7 100644 --- a/nexus/src/app/affinity.rs +++ b/nexus/src/app/affinity.rs @@ -14,6 +14,7 @@ use nexus_types::external_api::params; use nexus_types::external_api::views; use omicron_common::api::external; use omicron_common::api::external::CreateResult; +use omicron_common::api::external::DataPageParams; use omicron_common::api::external::DeleteResult; use omicron_common::api::external::Error; use omicron_common::api::external::ListResultVec; @@ -21,6 +22,7 @@ use omicron_common::api::external::LookupResult; use omicron_common::api::external::NameOrId; use omicron_common::api::external::UpdateResult; use omicron_common::api::external::http_pagination::PaginatedBy; +use omicron_uuid_kinds::AffinityGroupUuid; use omicron_uuid_kinds::GenericUuid; use omicron_uuid_kinds::InstanceUuid; @@ -246,22 +248,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 + 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( @@ -283,7 +281,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<'_>, @@ -306,6 +304,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, @@ -330,7 +351,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<'_>, @@ -355,6 +376,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, @@ -374,7 +420,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<'_>, @@ -397,4 +443,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 40da27505e7..715280beb8d 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( @@ -2983,7 +2982,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 +3028,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 +3073,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 +3088,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/tests/integration_tests/affinity.rs b/nexus/tests/integration_tests/affinity.rs index 902ab60f1cd..556d9fb1608 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, diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index a606a0c94c2..cffa6bca717 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -518,6 +518,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 { @@ -2002,6 +2013,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, diff --git a/nexus/tests/integration_tests/unauthorized.rs b/nexus/tests/integration_tests/unauthorized.rs index 8df0bffdb4e..d7c97ce4ca2 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, diff --git a/nexus/types/src/external_api/params.rs b/nexus/types/src/external_api/params.rs index 90f5e171d5f..91c2dd49735 100644 --- a/nexus/types/src/external_api/params.rs +++ b/nexus/types/src/external_api/params.rs @@ -840,6 +840,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 { diff --git a/openapi/nexus.json b/openapi/nexus.json index c13ede80b04..f8696df92c8 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 anti-affinity group member of type 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 affinity group member to anti-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 affinity group member from anti-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 anti-affinity group member", + "summary": "Fetch anti-affinity group member of type instance", "operationId": "anti_affinity_group_member_instance_view", "parameters": [ { @@ -1543,7 +1691,7 @@ "tags": [ "affinity" ], - "summary": "Add member to anti-affinity group", + "summary": "Add instance member to anti-affinity group", "operationId": "anti_affinity_group_member_instance_add", "parameters": [ { @@ -1594,7 +1742,7 @@ "tags": [ "affinity" ], - "summary": "Remove member from anti-affinity group", + "summary": "Remove instance member from anti-affinity group", "operationId": "anti_affinity_group_member_instance_delete", "parameters": [ { @@ -12550,6 +12698,25 @@ "AntiAffinityGroupMember": { "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 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" + ] + }, { "description": "An instance belonging to this group, identified by UUID.", "type": "object", @@ -23200,6 +23367,10 @@ } } }, + "TypedUuidForAffinityGroupKind": { + "type": "string", + "format": "uuid" + }, "TypedUuidForInstanceKind": { "type": "string", "format": "uuid" 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 217f21b4f08..2b74dbe21ac 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -4273,6 +4273,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, @@ -5005,7 +5024,7 @@ INSERT INTO omicron.public.db_metadata ( version, target_version ) VALUES - (TRUE, NOW(), NOW(), '129.0.0', NULL) + (TRUE, NOW(), NOW(), '130.0.0', NULL) ON CONFLICT DO NOTHING; COMMIT;