-
Notifications
You must be signed in to change notification settings - Fork 42
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
(1/5) [nexus] Add Affinity/Anti-Affinity Groups to API (unimplemented) #7443
base: main
Are you sure you want to change the base?
Changes from 3 commits
c9fb7a6
8dc0825
4e9cebc
6cfca2d
050b4c5
4b08032
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -982,6 +982,10 @@ impl JsonSchema for Hostname { | |||||||||||||||||||||||||||||
pub enum ResourceType { | ||||||||||||||||||||||||||||||
AddressLot, | ||||||||||||||||||||||||||||||
AddressLotBlock, | ||||||||||||||||||||||||||||||
AffinityGroup, | ||||||||||||||||||||||||||||||
AffinityGroupMember, | ||||||||||||||||||||||||||||||
AntiAffinityGroup, | ||||||||||||||||||||||||||||||
AntiAffinityGroupMember, | ||||||||||||||||||||||||||||||
AllowList, | ||||||||||||||||||||||||||||||
BackgroundTask, | ||||||||||||||||||||||||||||||
BgpConfig, | ||||||||||||||||||||||||||||||
|
@@ -1312,6 +1316,56 @@ pub enum InstanceAutoRestartPolicy { | |||||||||||||||||||||||||||||
BestEffort, | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
// AFFINITY GROUPS | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
#[derive(Clone, Copy, Debug, Deserialize, Serialize, PartialEq, JsonSchema)] | ||||||||||||||||||||||||||||||
#[serde(rename_all = "snake_case")] | ||||||||||||||||||||||||||||||
pub enum AffinityPolicy { | ||||||||||||||||||||||||||||||
/// If the affinity request cannot be satisfied, allow it anyway. | ||||||||||||||||||||||||||||||
/// | ||||||||||||||||||||||||||||||
/// This enables a "best-effort" attempt to satisfy the affinity policy. | ||||||||||||||||||||||||||||||
Allow, | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
/// If the affinity request cannot be satisfied, fail explicitly. | ||||||||||||||||||||||||||||||
Fail, | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
/// Describes the scope of affinity for the purposes of co-location. | ||||||||||||||||||||||||||||||
#[derive(Clone, Copy, Debug, Deserialize, Serialize, PartialEq, JsonSchema)] | ||||||||||||||||||||||||||||||
#[serde(rename_all = "snake_case")] | ||||||||||||||||||||||||||||||
pub enum FailureDomain { | ||||||||||||||||||||||||||||||
/// Instances are considered co-located if they are on the same sled | ||||||||||||||||||||||||||||||
Sled, | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema, PartialEq)] | ||||||||||||||||||||||||||||||
#[serde(tag = "type", content = "value", rename_all = "snake_case")] | ||||||||||||||||||||||||||||||
pub enum AffinityGroupMember { | ||||||||||||||||||||||||||||||
Instance(Uuid), | ||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we use a typed Uuid ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't currently have many in the external API - I think I plumbed one through the operator API with support bundles - but I think I should be able to convert this one. Updated in 4b08032 |
||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be helpful to add a doc comment to the variant saying it's the value is an instance ID. The Here's the OpenAPI schema from below: "AffinityGroupMember": {
"oneOf": [
{
"type": "object",
"properties": {
"type": {
"type": "string",
"enum": [
"instance"
]
},
"value": {
"type": "string",
"format": "uuid"
}
},
"required": [
"type",
"value"
]
}
]
} and here is the TypeScript code we generate for it export type AffinityGroupMember = { type: "instance"; value: string }; Compare to omicron/nexus/types/src/external_api/params.rs Lines 1047 to 1060 in d062c60
which produces export type ExternalIpCreate =
/** An IP address providing both inbound and outbound access. The address is automatically-assigned from the provided IP Pool, or the current silo's default pool if not specified. */
| { pool?: NameOrId; type: "ephemeral" }
/** An IP address providing both inbound and outbound access. The address is an existing floating IP object assigned to the current project.
The floating IP must not be in use by another instance or service. */
| { floatingIp: NameOrId; type: "floating" }; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Though actually, the presence of type Uuid = string and use that for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It'd be helpful for the Go SDK to have the doc comments as well. I haven't looked at the rust SDK in a while but I'm guessing it'd be useful for it to have the doc comments as well? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added some doc comments in 6cfca2d There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For what it's worth - there are spots where we accept the Instance name or UUID as a parameter, in paths, but we don't do that here. This struct is only used in responses, where I'm using UUIDs exclusively. That choice is somewhat arbitrary - I could return the name instead - but I figured that the UUID would at least be more stable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @smklein! UUID is definitely preferred from a terraform provider point of view. It relies on stability There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, yeah. I didn’t realize it’s only in responses, where it will be very obvious that’s it’s an ID. |
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
impl SimpleIdentity for AffinityGroupMember { | ||||||||||||||||||||||||||||||
fn id(&self) -> Uuid { | ||||||||||||||||||||||||||||||
match self { | ||||||||||||||||||||||||||||||
AffinityGroupMember::Instance(id) => *id, | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema, PartialEq)] | ||||||||||||||||||||||||||||||
#[serde(tag = "type", content = "value", rename_all = "snake_case")] | ||||||||||||||||||||||||||||||
pub enum AntiAffinityGroupMember { | ||||||||||||||||||||||||||||||
Instance(Uuid), | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
impl SimpleIdentity for AntiAffinityGroupMember { | ||||||||||||||||||||||||||||||
fn id(&self) -> Uuid { | ||||||||||||||||||||||||||||||
match self { | ||||||||||||||||||||||||||||||
AntiAffinityGroupMember::Instance(id) => *id, | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
// DISKS | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
/// View of a Disk | ||||||||||||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I notice that RFD 522 calls these policies for affinity groups "Allow" and "Fail", but for anti-affinity groups, we call them "Allow" and "Alert". I wonder if the different naming there is intentional --- my guess is that it isn't --- and whether we should change the RFD to use the same names for both affinity and anti-affinity, since it seems like we're going to use one enum for policies in the API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yeah, perhaps I misinterpreted this comment here: #7076 (comment)
My naming choice here was intentional, because we cannot implement this with "Alert" just yet (if you're being generous to me, or "chose not to" if you aren't).
I think it's possible in the future that we tweak the "Policy = Fail" to cause an alert, or possible for us to add "Policy = Alert", which is the same as "Policy = Fail", with the only distinction being triggers in the alerting system? Not sure. I just omitted it because it would be a misnomer until it gets integrated later.
Would you be okay punting on saying "we will implement alerting here"? I'd rather document it explicitly when ti gets integrated, rather than speculate on where alerts might land.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC the intent was for "Alert" to be "allow, but tell me you did that". Personally, I think we ought to stick with the "allow" and "fail" terminology, since we may want to alert in both cases ("we violated an affinity requirement because you said it was okay to" and "we didn't allow an instance to be scheduled because it would have violated an affinity requirement" are both things it seems reasonable to eventually want an alert about). So, my preference would be to go update the RFD to use "Allow" and "Fail" for both affinity and anti-affinity. IMO, the policy should describe whether or not we want an instance to be scheduled if it's impossible to satisfy its affinity constraint, and whether or not we generate an alert is a separate axis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case it's unclear I was not trying to suggest that we go back to calling it "alert" when I previously agreed that we shouldn't back in November. Instead, I was just suggesting that we should probably update the RFD to use the same names as this PR for both affinity and anti-affinity. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated these names on the RFD in https://github.com/oxidecomputer/rfd/pull/812/commits/361e0eeee741e8aa97fb501cc7da6b7a089992db