Remove ability to generate a Blueprint from an inventory collection #5583
Conversation
This file contains 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
jgallagher
commented
Apr 19, 2024
| .blueprint_disks | ||
| .values() | ||
| .map(|c| c.disks.len()) | ||
| .sum::<usize>() |
There was a problem hiding this comment.
This assertion change is, I believe, a fix for a test that was passing incorrectly. blueprint_disks is a BTreeMap<SledUuid, PhysicalDisksConfig>, which means blueprint_disks.len() reported "for how many sleds do we have a PhysicalDisksConfig?", not "how many disks are in the blueprint" as the check seemed to mean. I think this was passing purely by coincidence:
blueprint1.blueprint_diskswas empty, because a blueprint created from an inventory collection didn't populate it at allnew_sled_zpoolswas 4, because we added 4 disksblueprint2.blueprint_disks.len()was 4, because there are 4 sleds in blueprint2, and going through the builder populatesblueprint_disks
so we were previously passing because 0 + 4 == 4, even though the units of these values made the comparison nonsensical.
davepacheco
approved these changes
Apr 19, 2024
jgallagher
added a commit
that referenced
this pull request
Apr 22, 2024
…5584) This builds on #5583; see it for rationale. We remove half of the remaining callers that want to convert `OmicronZoneConfig` into `BlueprintZoneConfig`: * `nexus-test-utils`'s quasi-RSS: This now works in terms of `BlueprintZoneConfig`, and converts down to `OmicronZoneConfig` when necessary. This is the kind of transformation we'd like to make in real RSS, too, although it will be more complex there. * One unit test in `nexus-reconfigurator-execution`: replaced using the `representative()` collection (which includes a collection from a live system, but no blueprint) with the `example()` system (which produces both a collection and a matching blueprint). * Blueprint diffing - the types for the "before" side (which can be a collection or a blueprint) are now wrapped in two-variant enums, with some helpers for comparisons, instead of converting the collection to a blueprint. The remaining users of this conversion direction are the most difficult, but at least we're down to three: * `nexus-db-model` does this conversion to use some shared database serialization logic. This will necessarily change when we add to blueprints, since the serialization logic won't be shared anymore (or at least not _as_ shared). * RSS; see note above. We might be able to handle this like we did `nexus-test-utils`; I'll see when it becomes necessary. * The DNS test that uses data from a live system. Even if the last two have to keep this conversion direction for now (which will become more complex as data is added), at least it's only two! That should be manageable even if the process gets a little messy.
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.
We want to add information in blueprints, particularly
BlueprintZoneConfigandBlueprintZoneType, that is not present in the sled-agent typesOmicronZoneConfigandOmicronZoneType. Today on main conversion between those types is bidirectional. As we add to blueprints, we will continue to be able to convert aBlueprintZoneConfiginto anOmicronZoneConfig, but the opposite direction will become more and more difficult as callers need to provide the additional information required for blueprints.We have enough users of the inventory -> blueprint direction that it's quite painful to try to add to blueprints, so this PR attempts to knock out one of the more common uses: converting an inventory collection into a blueprint via
BlueprintBuilder::build_initial_from_collection(). This method is removed, and all the remaining changes are fallout from that. Most uses of this have been replaced by either the blueprint produced byExampleSystemor the newBlueprintBuilder::build_empty_with_sleds()helper for constructing an empty blueprint. One test needed the full blueprint-from-inventory, so that one actually gained a new use of converting OmicronZoneConfig -> BlueprintZoneConfig. (Not ideal, but fine for now!)