(9/5) [nexus] Allow anti-affinity members to be affinity groups by smklein · Pull Request #7572 · oxidecomputer/omicron

smklein · 2025-02-21T00:48:44Z

Expands group membership for anti-affinity groups to allow both:

Instances (this existed before this PR), and
Affinity groups (this is new)
... to be anti-affine from each other.

Without this feature, putting instances in an anti-affinity group would force them all to be separate from
one another. However, this can be unsatisfactory when multiple instances in that anti-affinity group do not care
if they are co-located.

As an example: Suppose you have a collection of several VMMs each representing a vertical deployment of a stack, and you want redundancy between these groups at a "collection of VMMs" layer. It's fine - and sometimes desirable - for everything in that vertical deployment to be co-located. However, it's the "groups of VMMs" that need to be isolated from each other. With this PR, one could put each of these "VMM collections" into distinct affinity groups, and put each of those affinity groups within an anti-affinity group.

Fixes #7567

nexus/external-api/src/lib.rs

smklein · 2025-02-26T20:15:20Z

nexus/db-queries/src/db/datastore/affinity.rs

+                            })?;
+                    }

+                    // If this affinity group is a member in other anti-affinity


This is tested by affinity_group_delete_group_deletes_membership_in_anti_affinity_groups below

smklein · 2025-02-26T20:17:25Z

nexus/db-queries/src/db/datastore/affinity.rs

+                        })?;
+
+
+                    // Check that the affinity group's members are not reserved.


This is tested within anti_affinity_group_membership_add_remove_group_with_vmm

smklein · 2025-02-26T20:18:29Z

nexus/db-queries/src/db/datastore/affinity.rs

+                            })?;
+                    }
+                    {
+                        use db::schema::anti_affinity_group_affinity_membership::dsl as member_dsl;


I expanded anti_affinity_group_delete_group_deletes_members to test this

smklein · 2025-02-26T20:20:45Z

nexus/db-queries/src/db/datastore/affinity.rs

+                            err.bail_retryable_or_else(e, |e| {
+                                public_error_from_diesel(
+                                    e,
+                                    ErrorHandler::NotFoundByResource(


I expanded anti_affinity_group_membership_for_deleted_objects to check for these NotFound errors

smklein · 2025-02-26T20:21:35Z

nexus/db-queries/src/db/datastore/affinity.rs

+                            err.bail_retryable_or_else(e, |e| {
+                                public_error_from_diesel(
+                                    e,
+                                    ErrorHandler::NotFoundByResource(


As mentioned above, I'm testing these NotFound errors in anti_affinity_group_membership_for_deleted_objects

smklein · 2025-02-26T21:11:08Z

nexus/db-queries/src/db/datastore/affinity.rs

-        pagparams: &PaginatedBy<'_>,
-    ) -> ListResultVec<AntiAffinityGroupInstanceMembership> {
+        pagparams: &DataPageParams<'_, Uuid>,
+    ) -> ListResultVec<external::AntiAffinityGroupMember> {


This is tested by anti_affinity_group_membership_list_extended

smklein · 2025-02-27T20:47:11Z

nexus/db-queries/src/db/datastore/affinity.rs

+            dropshot::PaginationOrder::Descending => false,
+        };
+
+        let mut query = QueryBuilder::new()


I didn't use the paginated macro because it doesn't really support reading from the UNION of two tables, and IMO, it's easier to write the SQL manually that it is to mess with those trait bounds.

I'm also experimenting with replacing the paginated macro with some utilities, closer to this implementation, in #7717

smklein · 2025-02-27T20:49:01Z

nexus/db-queries/src/db/queries/sled_reservation.rs

This file, as usual, contains the actual changes that we need to consider during instance placement.

I'll try to comment below to explain some of the rationale. For what it's worth, I also consider this to be the most "worth scrutinizing" file.

smklein · 2025-02-27T20:49:49Z

nexus/db-queries/src/db/queries/sled_reservation.rs

            ).param().sql(" <= sled.reservoir_size
        ),
-        our_aa_groups AS (
+        our_a_groups AS (


our_a_groups and our_a_instances are pulled up from below, because we're now using them when evaluating what the "anti-affinity instances" should be.

smklein · 2025-02-27T21:22:10Z

nexus/db-queries/src/db/queries/sled_reservation.rs

+            ON affinity_group_instance_membership.group_id = our_a_groups.group_id
+            WHERE instance_id != ").param().sql("
+        ),
+        our_direct_aa_groups AS (


I use the term "direct" to mean "this thing belongs as a member within the anti-affinity group we're observing".

I use the term "indirect" below for "this instance is being considered, but only because it's part of an affinity group that is a member within our anti-affinity group"

smklein · 2025-02-27T23:59:26Z

nexus/db-queries/src/db/queries/sled_reservation.rs

            WHERE instance_id = ").param().sql("
        ),
-        other_aa_instances AS (
+        other_direct_aa_instances AS (


These are instances we're "anti-affine" with, due to their membership in an anti-affinity group our instance also belongs to.

Our Instance Other Instances

Member in anti-affinity group Member in anti-affinity group

smklein · 2025-02-28T00:04:20Z

nexus/db-queries/src/db/queries/sled_reservation.rs

+            ON anti_affinity_group_instance_membership.group_id = our_direct_aa_groups.group_id
            WHERE instance_id != ").param().sql("
        ),
+        our_indirect_aa_groups AS (


The input to this whole query is "our instance ID".

We looked up the affinity groups this instance belongs to earlier - that's saved as our_a_groups.

So this subquery looks up "memberships in any anti-affinity groups, based on this instance's affinity groups".

This is "indirect" because our instance may not directly belong to this anti-affinity group, but we're looking it up because our instance needs to consider it through the affinity-group member relationship.

(The exactly_one_affinity_group value we return here is used below...)

smklein · 2025-02-28T00:12:26Z

nexus/db-queries/src/db/queries/sled_reservation.rs

+            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 (


These are instances we're "anti-affine" with, due to their instance membership in the anti-affinity group and also due to our membership in an affinity group (see: our_indirect_aa_groups).

Our Instance Other Instances

Member in affinity group belonging to anti-affinity group Member in anti-affinity group

smklein · 2025-02-28T00:35:48Z

nexus/db-queries/src/db/queries/sled_reservation.rs

+                ELSE TRUE
+                END
+        ),
+        other_aa_instances AS (


... and this sums up all the cases of anti-affinity:

Our Instance Other Instances

Member in anti-affinity group Member in anti-affinity group

Member in affinity group belonging to anti-affinity group Member in anti-affinity group

Member in affinity group belonging to anti-affinity group Member in affinity group belonging to anti-affinity group

smklein · 2025-02-28T00:53:22Z

nexus/db-queries/src/db/queries/sled_reservation.rs

+            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 (


These are instances we're "anti-affine" with, due to their group membership in an anti-affinity group and due to our membership in an affinity group that also belongs to that anti-affinity group.

This is basically the "indirect in both directions case". Imagine:

Affinity group "sheep": inst1, inst2

Affinity group "wolves": inst3, inst4

Anti-affinity group "do-not-eat-sheep": "sheep", "wolves"

If we're trying to place "inst1", we are:

Indirectly anti-affine with "wolves" (see: our_indirect_aa_groups)

So we want to look up all the members of "wolves", because we're anti-affine with all of them

Our Instance Other Instances

Member in affinity group belonging to anti-affinity group Member in affinity group belonging to anti-affinity group

We care about exactly_one_affinity_group here because if we have an instance which belongs to multiple affinity groups that are anti-affine from each other, we need to consider all those indirect instance members.

In the example above, the sheep "inst1" and "inst2" are not anti-affine from each other, because they both only belong to "exactly one affinity group" in the anti-affinity group. This hits the THEN clause of the CASE statement below, because we don't consider our instance anti-affine from sheep.

If we instead imagine the case of:

Affinity group "sweet": inst1, inst2

Affinity group "sour": inst1, inst3

Affinity group "spicy": inst3, inst4

Anti-affinity group "dont-mix-flavors": "sweet", "sour", "spicy"

If we now try to place "inst1", it belongs to both group "sweet" and group "sour". Here, our_indirect_aa_groups.exactly_one_affinity_group is false - so, "inst1" is considered anti-affine with all other groups, including ones it belongs to. This hits the ELSE TRUE clause below, because "inst1" is anti-affine to all indirect groups members here.

hawkw

there's a lot more of this PR i still need to get through, but here are a few notes from the first couple files...i haven't actually gotten to the meat yet.

hawkw · 2025-03-05T22:58:35Z

nexus/db-model/src/affinity.rs

+#[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>,
+}


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.

hawkw · 2025-03-05T23:01:41Z

nexus/db-model/src/affinity.rs

+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(),
+        }
+    }
+}


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

hawkw · 2025-03-05T23:04:13Z

nexus/db-queries/src/db/datastore/affinity.rs

+            dropshot::PaginationOrder::Descending => false,
+        };
+
+        let mut query = QueryBuilder::new()


nitpicky: is it worth, perhaps, factoring the raw query builder out from the code that actually runs it, so we can have an expectorate test for it?

hawkw · 2025-03-05T23:06:55Z

nexus/db-queries/src/db/datastore/affinity.rs

+            .sql(
+                "SELECT id,label FROM (
+                SELECT instance_id as id, 'instance' as label
+                FROM anti_affinity_group_instance_membership
+                WHERE group_id = ",
+            )
+            .param()
+            .bind::<diesel::sql_types::Uuid, _>(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 = ",
+            )


this use of union plus a label to zip together two tables is quite cute, it kinda makes me want to rewrite some code in my webhooks branch...

hawkw · 2025-03-05T23:08:20Z

nexus/db-queries/src/db/datastore/affinity.rs

+                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}"
+                    ))),
+                }


this is kind of a silly idea, but: could we avoid the "other" case if we defined a SQL enum for these labels? not sure if it's actually worth the effort to do that, and having a type that doesn't actually appear in any tables is kinda weird, so take it or leave it.

hawkw · 2025-03-05T23:10:48Z

nexus/db-queries/src/db/datastore/affinity.rs

+                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;


turbo nit, sorry: i usually expect all value-level code in a block to be below all the imports in that block, so i found the let err = err.clone(); above the imports a bit easy to miss. i might prefer:

Suggested change

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;

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;

let err = err.clone();

on the other hand, i will be the first to admit that i'm probably being obnoxious about this, so feel free to disregard me.

hawkw · 2025-03-05T23:26:21Z

nexus/db-queries/src/db/datastore/affinity.rs

+                    // 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::<uuid::Uuid>(&conn)


Could this instead be implemented using the existing DatastoreCollection machinery, which IIUC is mostly for dealing with this sort of thing?

gjcolombo · 2025-03-10T18:54:03Z

nexus/db-queries/src/db/queries/sled_reservation.rs

+            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


I'm probably completely missing something here, but I'm not quite following how this subquery works. If I may propose a slightly different example, suppose we have

affinity group Outcasts contains instances Janis and Damian

affinity group Plastics contains instances Regina, Gretchen, and Karen

anti-affinity group Dislikes contains affinity groups Outcasts and Plastics (but no instances)

Suppose I'm starting instance Janis. Then (IIUC):

our_indirect_aa_groups will contain one row: (Dislikes, Outcasts, TRUE); the other membership, (Dislikes, Plastics), is excluded by this subquery's WHERE clause

other_indirect_aa_instances_via_instances will be empty

Next, the query for other_indirect_aa_instances_via_groups will end up SELECTing from

affinity group instance membership JOIN our_indirect_aa_groups ON affinity_group_instance_membership.group_id = our_indirect_aa_groups.affinity_group_id

The rows of affinity_group_instance_membership are

Outcasts, Janis Outcasts, Damian Plastics, Regina Plastics, Gretchen Plastics, Karen

And the rows of our_indirect_aa_groups are

Dislikes, Outcasts, TRUE

So the SELECT on the joined table will produce two rows:

Outcasts, Janis Outcasts, Damian

This seems like the inverse of what we wanted, since this is the subquery that needs to add Regina, Gretchen, and Karen to the anti-affine instance set. I think my brain is expecting the right side of the join to refer to a list of the other ~~cliques~~ affinity groups that are members of the anti-affinity groups in which our instance is transitively a member.

What am I overlooking here?

smklein · 2025-03-18T00:31:46Z

So, in my attempt to follow-up on this, I:

Discussed the issues with membership subtlety with @gjcolombo
Wrote up our determinations in https://github.com/oxidecomputer/rfd/pull/869
Discussed that with @ahl
Determined that allowing this "anti-affinity group which might contain affinity groups" API could prevent live upgrade from proceeding

So:

For now, I'm going to close this PR
I've left the notes on "how to go about implementing this" in RFD 522, if we decide we need it later

Basic implementation of affinity group member within anti-affinity group

3bf35a2

smklein changed the title ~~Basic implementation of affinity group member within anti-affinity group~~ [nexus] Allow anti-affinity members to be affinity groups Feb 21, 2025

smklein changed the title ~~[nexus] Allow anti-affinity members to be affinity groups~~ [9/5][nexus] Allow anti-affinity members to be affinity groups Feb 21, 2025

smklein changed the title ~~[9/5][nexus] Allow anti-affinity members to be affinity groups~~ (9/5) [nexus] Allow anti-affinity members to be affinity groups Feb 21, 2025

smklein added 7 commits February 21, 2025 11:37

Merge branch 'vmm-reduce-contention' into aa-group-of-group

1430c4a

Update API

daeb2e1

Schema migration for db updates

2d65048

boilerplate, gotta fix pagination on list next

4e6362d

Manual pagination of both tables at once

f50749d

Merge branch 'vmm-reduce-contention' into aa-group-of-group

75869c7

Merge branch 'vmm-reduce-contention' into aa-group-of-group

804b2ee

This was referenced Feb 25, 2025

Affinity group members could expose name information when being listed #7625

Closed

[nexus] Affinity and Anti-Affinity Groups #7076

Closed

smklein added 8 commits February 25, 2025 11:36

Merge branch 'vmm-reduce-contention' into aa-group-of-group

0e8e7fc

Merge branch 'vmm-reduce-contention' into aa-group-of-group

c96034c

Clippy

167a2d8

Merge branch 'vmm-reduce-contention' into aa-group-of-group

1882d74

please no panic

17887d4

Safer group-to-group rule respecting

5865732

Add integration test for group of group

6ab1c67

Expanding testing, improving deletion semantics

0cb05b5

smklein mentioned this pull request Feb 26, 2025

Consider merging affinity / anti-affinity endpoints in the external API #7648

Closed

smklein commented Feb 26, 2025

View reviewed changes

nexus/external-api/src/lib.rs Outdated Show resolved Hide resolved

smklein added 3 commits February 26, 2025 12:14

More testing

9ad2313

Fix listing, add listing test

56f300a

clipperoni

05ebab7

smklein commented Feb 26, 2025

View reviewed changes

smklein marked this pull request as ready for review February 26, 2025 21:32

smklein added 2 commits February 26, 2025 16:51

Update API

388b6cd

Merge branch 'vmm-reduce-contention' into aa-group-of-group

9da8120

Unauthorized coverage test

aa9de03

smklein commented Feb 28, 2025

View reviewed changes

smklein added 2 commits February 27, 2025 16:55

Merge branch 'vmm-reduce-contention' into aa-group-of-group

90d41c8

i scream you scream we all scream for authcream

20496ce

smklein requested review from gjcolombo and hawkw February 28, 2025 23:02

smklein added 2 commits March 3, 2025 11:31

Merge branch 'vmm-reduce-contention' into aa-group-of-group

a0c4ce4

Merge branch 'vmm-reduce-contention' into aa-group-of-group

c39e421

hawkw reviewed Mar 5, 2025

View reviewed changes

Merge branch 'vmm-reduce-contention' into aa-group-of-group

22c3eb0

smklein mentioned this pull request Mar 7, 2025

(8/5) Reduce VMM reservation contention #7533

Merged

gjcolombo reviewed Mar 10, 2025

View reviewed changes

smklein closed this Mar 18, 2025

smklein mentioned this pull request Mar 18, 2025

Anti-Affinity groups should allow memberships consisting of affinity groups #7567

Open

		})?;


		// Check that the affinity group's members are not reserved.

Our Instance	Other Instances
Member in anti-affinity group	Member in anti-affinity group
Member in affinity group belonging to anti-affinity group	Member in anti-affinity group
Member in affinity group belonging to anti-affinity group	Member in affinity group belonging to anti-affinity group

Conversation

smklein commented Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smklein Feb 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gjcolombo Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smklein commented Mar 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

smklein commented Feb 21, 2025 •

edited

Loading

smklein Feb 28, 2025 •

edited

Loading

gjcolombo Mar 10, 2025 •

edited

Loading