Blueprint: Merge the four maps keyed by sled ID into a single map
#7652
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andrewjstone
approved these changes
Feb 28, 2025
| /// Iterate over the ids of all sleds in the blueprint | ||
| pub fn sleds(&self) -> impl Iterator<Item = SledUuid> + '_ { | ||
| self.blueprint_zones.keys().copied() | ||
| self.sleds.keys().copied() |
Contributor
There was a problem hiding this comment.
This is so much more logical now!
| ); | ||
|
|
||
| // Look up the sled state | ||
| let sled_state = sled_state |
| seen_sleds.insert(sled_id); | ||
| } | ||
|
|
||
| // Now create and display a table of zones/datasets on sleds that don't |
| pub fn new(before: &'a Blueprint, after: &'a Blueprint) -> Self { | ||
| let diff = before.diff(after); | ||
|
|
||
| let modified_zones_diff = diff |
Contributor
There was a problem hiding this comment.
Truly a dream come true.
| } | ||
| }; | ||
|
|
||
| // Combine the four separately-stored maps into the one blueprint map. |
Contributor
There was a problem hiding this comment.
Looks good, and makes sense.
| }; | ||
| } | ||
| } | ||
| blueprint1.blueprint_datasets.remove(&decommision_sled_id); |
Contributor
There was a problem hiding this comment.
This seems like it was wrong before. Maybe I missed the disks one at least.
|
|
||
| let zones_added = | ||
| &summary.zones_on_modified_sled(&sled_id).unwrap().zones.added; | ||
| let zones_added = &summary |
Contributor
There was a problem hiding this comment.
Possibly the only drawback I see in this PR. However, I also prefer it as all this code is macro generated rather than the handrolled monstrosity that existed before.
jgallagher
added a commit
that referenced
this pull request
Mar 4, 2025
Builds on #7713, and is followup from #7713 (comment). In #7652 I changed all the executor substeps to take iterators instead of `&BTreeMap` references that no longer existed, but that introduced a weird split where the top-level caller had to filter the blueprint down to just the items that the inner functions expected. @smklein pointed out one place where the inner code was being extra defensive, which was just more confusing than anything. This PR removes that split: the top-level executor now always passes a full `&Blueprint` down, and the inner modules are responsible for doing their own filtering as appropriate. To easy testing, I kept the versions that take an iterator of already-filtered items as private `*_impl` functions that the new functions-that-take-a-full-`Blueprint` themselves call too.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Sorry for the size of this PR. Most of the bulk of the diff is made up of mechanical changes to tests; I don't think there's a way to split this up any smaller. If you ignore whitespace that will trim a few hundred lines off the diff.
This PR should make zero behavioral changes. The main thrust of the PR is to replace the four maps in
Blueprint(sled_state,blueprint_zones,blueprint_disks,blueprint_datasets) with a single map containing the config for each sled (sleds: BTreeMap<SledUuid, BlueprintSledConfig>). Only one expectorate file changed (and that's because I changed the test; it was previously doing a thing we can't do anymore with the combined maps).Nontrivial fallout from this change:
BlueprintBuilderno longer has to do work to ingest a parent blueprint where sleds present inblueprint_zoneswere missing in one or more of the other three maps.nexus-db-modelhas to do that work now instead, to allow us to continue to load old blueprints from the db that didn't require all four maps to have consistent keys. We should be able to drop this and insteadbail!after R14, since upgrading to that release will involve creating a blueprint that uses this combined map (and thus guarantees the blueprint in the db has entries for all its fields).blippygot to delete several checks that are now statically impossible.daftoutput from thesledsmap is now directly usable in place of all those summary fields.I think this is pretty clearly a win, but there are a couple places where users of blueprints or diffs are a little uglier IMO:
sledsmap. This is straightforward and mechanical, but is a little noisy.summary.diff.sledsdirectly in tests instead of the old summary sets is more verbose. (Personally I prefer this change, despite the verbosity, because I found it easier to look atdaft's docs then remember what all the helper methods in the diff summary did. But I could see others feeling differently.)Fixes #7078. After this change, it'll be possible to start work on #7309 (merging the disks/datasets/zone sled-agent endpoints into one), since this restructuring will allow us to tag the combined config with a single generation.