-
Notifications
You must be signed in to change notification settings - Fork 40
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
Blueprint execution: Add dataset records for Crucible zones #5143
Conversation
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.
Nice! I recommend another review from someone more familiar how we use datasets.
datastore: &DataStore, | ||
all_omicron_zones: impl Iterator<Item = &OmicronZoneConfig>, | ||
) -> anyhow::Result<usize> { | ||
// Before attempting to insert any datasets, first query for any existing |
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 like this behavior (and the comment).
/// | ||
/// Does not update existing rows. If a dataset with the given ID already | ||
/// exists, returns `Ok(None)`. | ||
pub async fn dataset_insert_if_not_exists( |
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.
This seems like it'll fit in well with the existing "sled agent inserts dataset" structure, as we transition more towards "Nexus controls dataset creation".
filter_kind: Option<DatasetKind>, | ||
) -> ListResultVec<Dataset> { | ||
opctx.authorize(authz::Action::ListChildren, &authz::FLEET).await?; | ||
opctx.check_complex_operations_allowed()?; |
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.
huh TIL about this opctx call, good to know!
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.
It's quite new (from #4989)!
} | ||
|
||
#[cfg(test)] | ||
mod test { |
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.
Thanks for all the tests in this PR!
On each invocation, for every Crucible zone present in the blueprint (all of which have already successfully been sent to sled-agent by this point), we attempt to insert a
dataset
record for that zone, but only if a record does not already exist. The datasets themselves are created by sled-agent when we send the zone configs.Fixes #5118.