Skip to content
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

(5/5) [nexus] Implement Affinity/Anti-Affinity Groups in external API #7447

Open
wants to merge 10 commits into
base: affinity-instance-integration
Choose a base branch
from

Conversation

smklein
Copy link
Collaborator

@smklein smklein commented Jan 30, 2025

Pulled out of #7076

This PR is a partial implementation of RFD 522

It adds:

  • Affinity and Anti-Affinity groups, contained within projects. These groups are configured with a policy and failure domain can currently contain zero or more members. Affinity groups attempt to co-locate members, anti-affinity groups attempt to avoid co-locating members.
    • Policy describes "what to do if we cannot fulfill the co-location request". Currently, these options are "fail" (reject the request) or "allow" (continue with provisioning of the group member regardless).
    • Failure Domain describes the scope of what is considered "co-located". In this PR, the only option is "sled", but in the future, this may be expanded to e.g. "rack".
    • Members describe what can be added to affinity/anti-affinity groups. In this PR, the only option is "instance". RFD 522 describes how "anti-affinity groups may also contain affinity groups" -- which is why this "member" terminology is introduced -- but it is not yet implemented.
  • (anti-)Affinity groups are exposed by the API, through a CRUD interface
  • (anti-)Affinity groups are considered during "sled reservation", where instances are placed on a sled. This is most significantly implemented (and tested) within nexus/db-queries/src/db/datastore/sled.rs, within (4/5) [nexus] Consider Affinity/Anti-Affinity Groups during instance placement #7446

Fixes #1705

..
} => {
Err(Error::invalid_request(
"when providing affinity_group as an ID project should not be specified",
Copy link
Member

Choose a reason for hiding this comment

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

nit: this reads like "(ID project) should not be specified", which is a bit nonsensical, so i think it should have a comma

Suggested change
"when providing affinity_group as an ID project should not be specified",
"when providing affinity_group as an ID, project should not be specified",

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, and unfortunately there are quite a few more: #7298

..
} => {
Err(Error::invalid_request(
"when providing anti_affinity_group as an ID project should not be specified",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"when providing anti_affinity_group as an ID project should not be specified",
"when providing anti_affinity_group as an ID, project should not be specified",

self.db_datastore
.affinity_group_update(opctx, &authz_group, updates.clone().into())
.await
.map(|g| g.into())
Copy link
Member

Choose a reason for hiding this comment

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

huh, can this not be

Suggested change
.map(|g| g.into())
.map(Into::into)

updates.clone().into(),
)
.await
.map(|g| g.into())
Copy link
Member

Choose a reason for hiding this comment

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

similarly, could be

Suggested change
.map(|g| g.into())
.map(Into::into)

right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants