Skip to content

(9/5) [nexus] Allow anti-affinity members to be affinity groups #7572

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 27 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
3bf35a2
Basic implementation of affinity group member within anti-affinity group
smklein Feb 21, 2025
1430c4a
Merge branch 'vmm-reduce-contention' into aa-group-of-group
smklein Feb 21, 2025
daeb2e1
Update API
smklein Feb 21, 2025
2d65048
Schema migration for db updates
smklein Feb 21, 2025
4e6362d
boilerplate, gotta fix pagination on list next
smklein Feb 24, 2025
f50749d
Manual pagination of both tables at once
smklein Feb 24, 2025
75869c7
Merge branch 'vmm-reduce-contention' into aa-group-of-group
smklein Feb 24, 2025
804b2ee
Merge branch 'vmm-reduce-contention' into aa-group-of-group
smklein Feb 25, 2025
0e8e7fc
Merge branch 'vmm-reduce-contention' into aa-group-of-group
smklein Feb 25, 2025
c96034c
Merge branch 'vmm-reduce-contention' into aa-group-of-group
smklein Feb 25, 2025
167a2d8
Clippy
smklein Feb 25, 2025
1882d74
Merge branch 'vmm-reduce-contention' into aa-group-of-group
smklein Feb 25, 2025
17887d4
please no panic
smklein Feb 25, 2025
5865732
Safer group-to-group rule respecting
smklein Feb 25, 2025
6ab1c67
Add integration test for group of group
smklein Feb 25, 2025
0cb05b5
Expanding testing, improving deletion semantics
smklein Feb 26, 2025
9ad2313
More testing
smklein Feb 26, 2025
56f300a
Fix listing, add listing test
smklein Feb 26, 2025
05ebab7
clipperoni
smklein Feb 26, 2025
388b6cd
Update API
smklein Feb 27, 2025
9da8120
Merge branch 'vmm-reduce-contention' into aa-group-of-group
smklein Feb 27, 2025
aa9de03
Unauthorized coverage test
smklein Feb 27, 2025
90d41c8
Merge branch 'vmm-reduce-contention' into aa-group-of-group
smklein Feb 28, 2025
20496ce
i scream you scream we all scream for authcream
smklein Feb 28, 2025
a0c4ce4
Merge branch 'vmm-reduce-contention' into aa-group-of-group
smklein Mar 3, 2025
c39e421
Merge branch 'vmm-reduce-contention' into aa-group-of-group
smklein Mar 4, 2025
22c3eb0
Merge branch 'vmm-reduce-contention' into aa-group-of-group
smklein Mar 7, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion common/src/api/external/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -1336,16 +1337,22 @@ 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),
}

impl SimpleIdentity for AntiAffinityGroupMember {
fn id(&self) -> Uuid {
match self {
AntiAffinityGroupMember::AffinityGroup(id) => *id.as_untyped_uuid(),
AntiAffinityGroupMember::Instance(id) => *id.as_untyped_uuid(),
}
}
Expand Down
28 changes: 28 additions & 0 deletions nexus/db-model/src/affinity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -257,3 +258,30 @@ impl From<AntiAffinityGroupInstanceMembership>
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<AntiAffinityGroupKind>,
pub affinity_group_id: DbTypedUuid<AffinityGroupKind>,
}
Comment on lines +262 to +267
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely a fan of making this a separate table, rather than changing the existing table to have nullable instance and anti-affinity group ID fields.


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(),
}
}
}
Comment on lines +269 to +279
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is maybe a bit goofy of me, but for stuff like this where there's a clear directional relationship (the anti-affinity group is a member of the affinity group),i kinda wonder whether we ought to have a method on AntiAffinityGroupUuid be the constructor, so you can write

let membership = anti_affinity_group_id.is_member_of(affinity_group_id);

and have that relationship be clearly stated at the call site? On the other hand, this might just be annoying and deviate from the convention for constructors for no real reason. It's the type of thing I would have reached for in my misspent youth[^1] which makes me kind of leery of this suggestion...

[^1] i.e. back when i wrote Scala


impl From<AntiAffinityGroupAffinityMembership>
for external::AntiAffinityGroupMember
{
fn from(member: AntiAffinityGroupAffinityMembership) -> Self {
Self::AffinityGroup(member.affinity_group_id.into())
}
}
8 changes: 8 additions & 0 deletions nexus/db-model/src/schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
3 changes: 2 additions & 1 deletion nexus/db-model/src/schema_versions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
///
Expand All @@ -28,6 +28,7 @@ static KNOWN_VERSIONS: LazyLock<Vec<KnownVersion>> = 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"),
Expand Down
Loading
Loading