[wip] Sketching out API, DB models for affinity and anti-affinity #7076
Conversation
|
|
||
| #[derive(Clone, Copy, Debug, Deserialize, Serialize, PartialEq, JsonSchema)] | ||
| #[serde(rename_all = "snake_case")] | ||
| pub enum AffinityDistance { |
There was a problem hiding this comment.
Could rename this to "failure domain", if that seems useful?
There was a problem hiding this comment.
I like "failure domain", seems more semantically meaningful IMO.
|
|
||
| #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] | ||
| pub enum AffinityGroupMember { | ||
| Instance(Uuid), |
There was a problem hiding this comment.
I know this is an "enum of one", but RFD 522 discusses having anti-affinity groups which contain "either instances or affinity groups". I figured I'd just define these as "members" to be flexible for future work
| #[derive(Clone, Copy, Debug, Deserialize, Serialize, PartialEq, JsonSchema)] | ||
| #[serde(rename_all = "snake_case")] | ||
| pub enum AffinityPolicy { | ||
| Allow, |
There was a problem hiding this comment.
I know I'm calling this "Allow" instead of "Alert", but without a fleshed-out alerting system, I wanted to keep this honest.
We can add "Alert" later, when it can actually be implemented.
There was a problem hiding this comment.
That makes sense to me. Perhaps we should document the behavior here?
There was a problem hiding this comment.
Hopefully a quick drive-by review wasn't too premature, but this stuff is relevant to my interests, so I wanted to take a peek. Overall, everything looks very straightforward and reasonable --- I commented on a few small things, but it's quite possible you were planning to get to all of them and just hadn't gotten around to it yet.
|
|
||
| #[derive(Clone, Copy, Debug, Deserialize, Serialize, PartialEq, JsonSchema)] | ||
| #[serde(rename_all = "snake_case")] | ||
| pub enum AffinityDistance { |
There was a problem hiding this comment.
I like "failure domain", seems more semantically meaningful IMO.
| #[derive(Queryable, Insertable, Clone, Debug, Selectable)] | ||
| #[diesel(table_name = affinity_group)] | ||
| pub struct AffinityGroup { | ||
| pub id: DbTypedUuid<AffinityGroupKind>, | ||
| pub name: String, | ||
| pub description: String, | ||
| pub time_created: DateTime<Utc>, | ||
| pub time_modified: DateTime<Utc>, | ||
| pub time_deleted: Option<DateTime<Utc>>, | ||
| pub policy: AffinityPolicy, | ||
| pub distance: AffinityDistance, | ||
| } | ||
|
|
||
| #[derive(Queryable, Insertable, Clone, Debug, Selectable)] | ||
| #[diesel(table_name = anti_affinity_group)] | ||
| pub struct AntiAffinityGroup { | ||
| pub id: DbTypedUuid<AntiAffinityGroupKind>, | ||
| pub name: String, | ||
| pub description: String, | ||
| pub time_created: DateTime<Utc>, | ||
| pub time_modified: DateTime<Utc>, | ||
| pub time_deleted: Option<DateTime<Utc>>, | ||
| pub policy: AffinityPolicy, | ||
| pub distance: AffinityDistance, | ||
| } |
There was a problem hiding this comment.
Part of me wonders whether it makes sense to use the same model to represent these, since they share all the same fields...but I think the type-level distinction between what's an affinity group and what's an anti-affinity group will probably prevent bugs in the future, and using different typed UUIDs for the IDs is definitely the right move. Potentially shared bits like the policy and distance fields could be factored out later if there was a need to have functions that can operate on either, but at present it seems like there probably isn't!
| #[derive(Queryable, Insertable, Clone, Debug, Selectable)] | ||
| #[diesel(table_name = affinity_group_instance_membership)] | ||
| pub struct AffinityGroupInstanceMembership { | ||
| pub group_id: DbTypedUuid<AffinityGroupKind>, | ||
| pub instance_id: DbTypedUuid<InstanceKind>, | ||
| } | ||
|
|
||
| #[derive(Queryable, Insertable, Clone, Debug, Selectable)] | ||
| #[diesel(table_name = anti_affinity_group_instance_membership)] | ||
| pub struct AntiAffinityGroupInstanceMembership { | ||
| pub group_id: DbTypedUuid<AntiAffinityGroupKind>, | ||
| pub instance_id: DbTypedUuid<InstanceKind>, | ||
| } |
There was a problem hiding this comment.
I note that these lack created/deleted timestamps, implying that:
- we intend to hard-delete rather than soft-delete them, and,
- we don't presently record when instances were added to affinity/anti-affinity groups, so we can't present that in UIs in the future.
I'm not sure if either of these matter to us, but I figured I'd comment on it.
| table! { | ||
| affinity_group_instance_membership (group_id, instance_id) { | ||
| group_id -> Uuid, | ||
| instance_id -> Uuid, | ||
| } | ||
| } | ||
|
|
||
| table! { | ||
| anti_affinity_group_instance_membership (group_id, instance_id) { | ||
| group_id -> Uuid, | ||
| instance_id -> Uuid, | ||
| } | ||
| } |
There was a problem hiding this comment.
I presume we'll want to add joinable!/allow_tables_to_appear_in_same_query! for joining the affinity/anti-affinity group tables on the instances table?
| /// List members of an affinity group | ||
| #[endpoint { | ||
| method = GET, | ||
| path = "/v1/affinity-groups/{affinity_group}", | ||
| tags = ["affinity"], | ||
| }] | ||
| async fn affinity_group_member_list( | ||
| rqctx: RequestContext<Self::Context>, | ||
| query_params: Query<params::OptionalProjectSelector>, | ||
| path_params: Path<params::AffinityGroupPath>, | ||
| ) -> Result<HttpResponseOk<ResultsPage<AffinityGroupMember>>, HttpError>; |
There was a problem hiding this comment.
This returns a ResultsPage but appears to be missing pagination params --- should it be paginated? Or am I missing something?
| /// List members of an anti-affinity group | ||
| #[endpoint { | ||
| method = GET, | ||
| path = "/v1/anti-affinity-groups/{anti_affinity_group}", | ||
| tags = ["affinity"], | ||
| }] | ||
| async fn anti_affinity_group_member_list( | ||
| rqctx: RequestContext<Self::Context>, | ||
| query_params: Query<params::OptionalProjectSelector>, | ||
| path_params: Path<params::AntiAffinityGroupPath>, | ||
| ) -> Result<HttpResponseOk<ResultsPage<AntiAffinityGroupMember>>, HttpError>; |
There was a problem hiding this comment.
Similarly, should this be paginated?
| #[derive(Clone, Copy, Debug, Deserialize, Serialize, PartialEq, JsonSchema)] | ||
| #[serde(rename_all = "snake_case")] | ||
| pub enum AffinityPolicy { | ||
| Allow, |
There was a problem hiding this comment.
That makes sense to me. Perhaps we should document the behavior here?
Extremely WIP, this is an exploratory implementation of RFD 522
See: #1705