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

(4/5) [nexus] Consider Affinity/Anti-Affinity Groups during instance placement #7446

Open
wants to merge 5 commits into
base: affinity-db-crud
Choose a base branch
from

Conversation

smklein
Copy link
Collaborator

@smklein smklein commented Jan 30, 2025

Pulled out of #7076

Modifies the instance placement logic to consider affinity and anti-affinity groups.
This is still technically "internal-only", because the HTTP endpoints to create affinity groups are, as of this PR, still unimplemented.

.bind::<sql_types::Uuid, _>(instance_id.into_untyped_uuid())
.bind::<sql_types::Uuid, _>(instance_id.into_untyped_uuid())
.query()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

didn't know we were allowed to do this 😈

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This style of query builder? I added this in #5063 , and ripped out the old one in #5966

CTEs seem tricky enough without fighting the type system to make them happen

Copy link
Member

Choose a reason for hiding this comment

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

AFAIK diesel just straight up doesn't have a way to express CTEs?

Comment on lines +235 to +244
SledReservationError::NotFound
| SledReservationError::TooManyAffinityConstraints
| SledReservationError::ConflictingAntiAndAffinityConstraints
| SledReservationError::RequiredAffinitySledNotValid => {
return external::Error::insufficient_capacity(
"No sleds can fit the requested instance",
"No sled targets found that had enough \
capacity to fit the requested instance.",
);
}
Copy link
Member

Choose a reason for hiding this comment

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

hmm, IMO, the error messages from the SledReservationError seem like they ought to also be bubbled up to the user --- they seem like they would be useful to know when debugging why a sled placement failed?

//
// # Rules vs Preferences
//
// Due to the flavors "affinity policy", it's possible to bucket affinity
Copy link
Member

Choose a reason for hiding this comment

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

typo:

Suggested change
// Due to the flavors "affinity policy", it's possible to bucket affinity
// Due to the flavors of "affinity policy", it's possible to bucket affinity

Comment on lines +370 to +442
// We use the following logic to calculate a desirable sled,
// given a possible set of "targets", and the information
// from affinity groups.
//
// # Rules vs Preferences
//
// Due to the flavors "affinity policy", it's possible to bucket affinity
// choices into two categories: "rules" and "preferences". "rules" are affinity
// dispositions for or against sled placement that must be followed, and
// "preferences" are affinity dispositions that should be followed for sled
// selection, in order of "most preferential" to "least preferential".
//
// As example of a "rule" is "an anti-affinity group exists, containing a
// target sled, with affinity_policy = 'fail'".
//
// An example of a "preference" is "an anti-affinity group exists, containing a
// target sled, but the policy is 'allow'. We don't want to use it as a target,
// but we will if there are no other choices."
//
// We apply rules before preferences to ensure they are always respected.
// Furthermore, the evaluation of preferences is a target-seeking operation,
// which identifies the distinct sets of targets, and searches them in
// decreasing preference order.
//
// # Logic
//
// ## Background: Notation
//
// We use the following symbols for sets below:
// - ∩: Intersection of two sets (A ∩ B is "everything that exists in A and
// also exists in B").
// - \: difference of two sets (A \ B is "everything that exists in A that does
// not exist in B).
//
// We also use the following notation for brevity:
// - AA,P=Fail: All sleds sharing an anti-affinity instance within a group with
// policy = 'fail'.
// - AA,P=Allow: Same as above, but with policy = 'allow'.
// - A,P=Fail: All sleds sharing an affinity instance within a group with
// policy = 'fail'.
// - A,P=Allow: Same as above, but with policy = 'allow'.
//
// ## Affinity: Apply Rules
//
// - Targets := All viable sleds for instance placement
// - Banned := AA,P=Fail
// - Required := A,P=Fail
// - if Required.len() > 1: Fail (too many constraints).
// - if Required.len() == 1...
// - ... if the entry exists in the "Banned" set: Fail
// (contradicting constraints 'Banned' + 'Required')
// - ... if the entry does not exist in "Targets": Fail
// ('Required' constraint not satisfiable)
// - ... if the entry does not exist in "Banned": Use it.
//
// If we have not yet picked a target, we can filter the
// set of targets to ignore "banned" sleds, and then apply
// preferences.
//
// - Targets := Targets \ Banned
//
// ## Affinity: Apply Preferences
//
// - Preferred := Targets ∩ A,P=Allow
// - Unpreferred := Targets ∩ AA,P=Allow
// - Neither := Preferred ∩ Unpreferred
// - Preferred := Preferred \ Neither
// - Unpreferred := Unpreferred \ Neither
// - If Preferred isn't empty, pick a target from it.
// - Targets := Targets \ Unpreferred
// - If Targets isn't empty, pick a target from it.
// - If Unpreferred isn't empty, pick a target from it.
// - Fail, no targets are available.
Copy link
Member

Choose a reason for hiding this comment

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

this comment is really lovely, thank you for including it!

Comment on lines +386 to +387
// target sled, but the policy is 'allow'. We don't want to use it as a target,
// but we will if there are no other choices."
Copy link
Member

Choose a reason for hiding this comment

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

teensy edit: i think you meant to end the quote sooner:

Suggested change
// target sled, but the policy is 'allow'. We don't want to use it as a target,
// but we will if there are no other choices."
// target sled, but the policy is 'allow'." We don't want to use it as a target,
// but we will if there are no other choices.

// We use the following symbols for sets below:
// - ∩: Intersection of two sets (A ∩ B is "everything that exists in A and
// also exists in B").
// - \: difference of two sets (A \ B is "everything that exists in A that does
Copy link
Member

Choose a reason for hiding this comment

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

if you want to be a huge dweeb, do note that there is also a unicode code point U+2216 SET MINUS () which is distinct from U+005C REVERSE SOLIDUS, the ASCII backslash (\). depending on the typeface, these characters may be nearly identical, or they may look pretty different.

use this information however you want to, or don't. :)

// - Targets := All viable sleds for instance placement
// - Banned := AA,P=Fail
// - Required := A,P=Fail
// - if Required.len() > 1: Fail (too many constraints).
Copy link
Member

Choose a reason for hiding this comment

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

this took me a moment to understand until i got it through my head that A,P=Fail refers to all sleds with a required instance, not all instances which are required --- I was about to say, "wait, we don't allow an affinity group to require more than one instance?"

Comment on lines +446 to +459
let banned = anti_affinity_sleds.iter().filter_map(|(policy, id)| {
if *policy == AffinityPolicy::Fail {
Some(*id)
} else {
None
}
}).collect::<HashSet<_>>();
let required = affinity_sleds.iter().filter_map(|(policy, id)| {
if *policy == AffinityPolicy::Fail {
Some(*id)
} else {
None
}
}).collect::<HashSet<_>>();
Copy link
Member

Choose a reason for hiding this comment

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

silly code golf nit, feel free to ignore: if you wanted to only iterate over anti_affinity_sleds/affinity_sleds a single time to get both the rules and preferences, you could maybe consider using Iterator::partition to do it in one pass. But, that's a bit annoying because you can't throw away the policy value in the partition closure and there's no partition_map type operation yet...

if !banned.is_empty() {
info!(
opctx.log,
"affinity policy prohibits placement on {} sleds", banned.len();
Copy link
Member

Choose a reason for hiding this comment

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

nitpicky (sorry): i might have this say

Suggested change
"affinity policy prohibits placement on {} sleds", banned.len();
"anti-affinity policy prohibits placement on {} sleds", banned.len();

because i imagine someone grepping the logs for the string "anti-affinity" to find stuff about anti-affinity policies being applied?

Comment on lines +519 to +532
if let Some(target) = preferred.iter().next() {
*target
} else {
targets = targets.difference(&unpreferred).cloned().collect();
if let Some(target) = targets.iter().next() {
*target
} else {
if let Some(target) = unpreferred.iter().next() {
*target
} else {
return Err(err.bail(SledReservationError::NotFound));
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

oh jeez...it's a bummer we can't express this logic with early returns, because we still have to call SledResource::new_for_vmm and actually insert the resource record. I kind of wonder whether it's worth factoring out the selection of a sled target into a standalone function that's called by the transaction, so you can use early returns to express this logic with a bit less rightward drift. it might also make it easier to write tests for the sled selection code without having to actually do a wole DB transaction?

.bind::<sql_types::Uuid, _>(instance_id.into_untyped_uuid())
.bind::<sql_types::Uuid, _>(instance_id.into_untyped_uuid())
.query()
}
Copy link
Member

Choose a reason for hiding this comment

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

AFAIK diesel just straight up doesn't have a way to express CTEs?

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