-
Notifications
You must be signed in to change notification settings - Fork 53
initial draft of blueprints #4804
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
Merged
Merged
Changes from all commits
Commits
Show all changes
42 commits
Select commit
Hold shift + click to select a range
91059b2
first cut at "add sled" blueprint maker
davepacheco 95a7cd0
Merge branch 'main' into dap/update-control-2
davepacheco 9c7a40d
WIP: make BlueprintBuilder more general because planning logic will g…
davepacheco 7f7235e
refactor towards incremental plans
davepacheco 0fb4e95
when planning, we want to be using the parent blueprint and not the c…
davepacheco a46520d
WIP: adding internal APIs to be able to play with this stuff
davepacheco e9bbb5c
WIP: flesh out those internal APIs
davepacheco 155c0be
fetch sleds and zpools for planning
davepacheco cf3aa30
omdb support for these APIs
davepacheco 52c7d9f
sled agent client could use more common networking types
davepacheco 8e4ef98
print out more about blueprints, plus some cleanup
davepacheco 9a8ec62
add blueprint authz
davepacheco 9670e8b
no more table scan
davepacheco 2646426
remove XXX for issue filed
davepacheco 8f0b344
have simulated sled agent simulate storage too
davepacheco ef1fa8e
general cleanup -- fix lots of XXXs
davepacheco a1d40ca
follow-up to sled-agent-client changes
davepacheco c8f7685
clean up various XXXs
davepacheco d4b9813
blueprint builder could be idempotent; comment about internal DNS
davepacheco faa9dde
the case I was worried about should not be a problem
davepacheco b5a9d47
clippy backlog
davepacheco 2897254
Merge branch 'main' into dap/update-control-2
davepacheco f50ee0f
use new IP address range
davepacheco a2abb97
test run + fixups
davepacheco f0b0451
remove one XXX
davepacheco b1f2fb9
omdb nits, regenerate spec
davepacheco 52fc677
remove XXXs now tracked elsewhere
davepacheco d8ad0ee
nit
davepacheco c39c75d
add diff, simplify some types
davepacheco 81842f6
cleanup, programmatic diff, fix up test
davepacheco c0da2e5
rustfmt
davepacheco 1342d9f
Merge branch 'main' into dap/update-control-2
davepacheco 58f7e39
support diff collection and blueprint and use that in the test
davepacheco ce76d0f
add test for ip_allocator
davepacheco 0ec1d8a
add basic tests for blueprint builder
davepacheco b49794a
Merge branch 'main' into dap/update-control-2
davepacheco c58c7ed
review feedback
davepacheco 06825c0
make user specify collection
davepacheco 701d2c7
review feedback
davepacheco 2e17346
Merge branch 'main' into dap/update-control-2
davepacheco 80bec1a
stray comment
davepacheco 402ea0c
Merge branch 'main' into dap/update-control-2
davepacheco File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
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
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
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
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
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
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
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
Oops, something went wrong.
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.
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.
@ahl installed some healthy fear of
replace
in me a while back - how painful would it be toimpl From
for blueprints instead ofreplace
ing?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 share yours and Adam's reluctance about
replace
. In this case, the functionality being shared (diff'ing blueprints) is pretty non-trivial and I think it would be fairly painful to impl the requisiteFrom
s: I think it would require implementingFrom
forBlueprint
, andOmicronZonesConfig
, which in turn requiresOmicronZoneConfig
, thenOmicronZoneType
(a big enum with a bunch of properties in many variants),OmicronZoneDataset
,SledRole
,ZpoolName
,NetworkInterface
,NetworkInterfaceKind
, andSourceNatConfig
. (I'm not positive, but I think that's right.)I think better approaches here might be to have Progenitor validate that your
replace
type really is compatible with the spec, or alternatively if there were a way to automatically generateFrom
impls between two types that are identical (which would presumably break -- correctly -- if they weren't identical). It doesn't feel sustainable to me to either duplicate complex functionality on multiple types or hand-roll so many complexFrom
impls. What do you think?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 actually had the opposite reaction, and didn't think it was unfortunate to use
replace
at all. However, clearly I am missing what is problematic here.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 think what sucks about using
replace
is that you lose the ability to notice when your local type doesn't match the spec. In turn that means we lose all of the benefit of Rust's strong static typing across the OpenAPI boundary.Concretely: in the normal case where Progenitor generates the types for you, say we pull in a new version of the spec with a new required field. Progenitor's type will have that new field in it. So if we had a code path that creates an instance of that type (i.e., to make the request) it will now fail to compile. That's good: if we didn't know it before, we now know we've made an incompatible change to the spec. If we did know it, now's when we update our code to fill in the new value.
If on the other hand we use
replace
to supply the type, when we pull in the updated spec, everything still compiles -- there's no reason it wouldn't. Instead, that code path will fail at runtime when the server rejects the request because it's missing the required field. That really sucks.Similarly on the output side, if we use
replace
to supply our own type, we don't find out at compile time when our type is incompatible. A particularly bad case would be an enum, where the server might add a new variant (which one might even consider a non-breaking change). But if the client sees it, it will fail to deserialize the response. By contrast, if we use the Progenitor-generated type, we'll fail to build if, say, we try tomatch
on the enum and don't include the new variant.In summary, using Progenitor types gets a lot of the benefit of Rust across the OpenAPI boundary. Using "replace" bypasses all of it (for that type). The downside of Progenitor types is that when there's non-trivial functionality on the original type, you either have to impl that functionality twice (sucks) or convert the type to your own type (okay for simple types in a few places, but kind of sucks in the large). For simple, stable types like
Name
I think the benefits of "replace" probably outweigh the risks. For complex types that are likely to change, I'm less sure (but still feel what I said earlier about this case).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 feels like it would be huge - it would allow use to use
replace
fearlessly, even on big / complex / volatile types like this one, I think?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.
Potentially, yeah. I spoke with Adam about this a bit. It's not clear how we could do this at compile time, but I imagine we might be able to do it at test-time.