Add a Diff Visitor for BlueprintZoneConfigs #7336
Merged
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
This visitor walks a diffus generated diff and provides callbacks for users to hook into the parts of the diff that they care about. To keep things minimal, we don't provide callback trait methods for copies (when nothing changes), and we don't provide methods for most diffus_derive generated types (the ones that start with `Edited`). We anticipate composing these visitors to build up a full diff of a Blueprint. They can also be used individually as demonstrated with the included property based test. In order to easily generate `BlueprintZoneConfigs` for property tests we derived `Arbitrary` for `BlueprintZoneConfigs` and its fields using the `test-strategy` crate. Some of the types in the hierarchy don't actually implement `Arbitrary` and they come from foreign crates. In this case a corresponding generation strategy was added that allows those types to be generated based on more primitive types that do implement arbitrary. This PR is just one of many coming to build up and use automated blueprint diffs. It builds on the following PRs which must be merged in first: * oxidecomputer/newtype-uuid#56 * oxidecomputer/diffus#8
andrewjstone
commented
Jan 11, 2025
Cargo.toml
Outdated
| diesel = { version = "2.2.4", features = ["i-implement-a-third-party-backend-and-opt-into-breaking-changes", "postgres", "r2d2", "chrono", "serde_json", "network-address", "uuid"] } | ||
| diesel-dtrace = "0.4.2" | ||
| diffus = { git = "https://github.com/oxidecomputer/diffus", branch = "oxide/main", features = ["uuid-impl", "derive", "newtype-uuid-impl", "oxnet-impl"] } | ||
| #diffus = { git = "https://github.com/oxidecomputer/diffus", branch = "oxide/main", features = ["uuid-impl", "derive", "newtype-uuid-impl", "oxnet-impl"] } |
Contributor
Author
There was a problem hiding this comment.
This and newtype-uuid below must be merged first and this must be updated.
jgallagher
reviewed
Jan 13, 2025
| ctx: &mut BpVisitorContext, | ||
| node: Edit<'e, BlueprintZonesConfig>, | ||
| ) { | ||
| visit_root(self, ctx, node); |
Contributor
There was a problem hiding this comment.
Are implementations generally expected to only provide the leaf node methods and leave visit_{root,zone_edit} as their default impls? If so, should this trait be split into the bits a consumer should provide to avoid them accidentally overriding methods they shouldn't? (Or do you think some implementers will want to override them and take on the responsibility of calling visit_root() themselves? That might still be worth splitting into two traits for clarify, I'm not sure.)
Contributor
Author
There was a problem hiding this comment.
Are implementations generally expected to only provide the leaf node methods and leave visit_{root,zone_edit} as their default impls?
Not necessarily, no. Consumers don't just include direct users. We expect to compose visitors inside the whole blueprint system, and we may want to hook into non-leaf callbacks to check if there are any copies, without directly exposing those as methods.
If so, should this trait be split into the bits a consumer should provide to avoid them accidentally overriding methods they shouldn't? (Or do you think some implementers will want to override them and take on the responsibility of calling visit_root() themselves?
I do think consumers may want to override them. They would then call the underlying free-standing function to continue recursion. This is why the functions call the visitor methods and are directly exposed. This strategy is used in the toml_edit visitor, and was recommended to me by @sunshowers.
However, this is really a subjective decision. If we only want users to override the leaf methods, then a separate trait may be useful. I fear we may be painting ourselves into a corner this way though.
Contributor
There was a problem hiding this comment.
Yeah -- in practice with visitors, I've often found doing things preorder and postorder to be useful. You do have to remember to actually call the recursive traversal, but in my experience that gets caught pretty quickly.
Contributor
Author
|
@jgallagher and I chatted last night about the use of |
Contributor
Author
|
@jgallagher I removed all uses of Arbitrary and added a few non-proptests as we discussed. I didn't end up bothering with any randomization. |
jgallagher
reviewed
Jan 15, 2025
Contributor
jgallagher
left a comment
jgallagher
left a comment
There was a problem hiding this comment.
LGTM. Thanks for dropping the Arbitrarys (as much as we like proptests!).
Contributor
Author
Thanks for the followup and guidance @jgallagher. |
andrewjstone
added a commit
that referenced
this pull request
Jan 25, 2025
This commit introduces an automated blueprint diff mechanism based on diffus along with the visitor pattern for traversing heterogeneous diff trees. It builds on several prior commits that introduced diffus support and added visitors for types: #7261 #7336 #7362 #7366 The new `BlueprintDiffer` implements the relevant visitors and accumulates change state from visitor method callbacks. The accumulated state is the `BlueprintDiff` which replaces the old `BlueprintDiff`. The new structure intentionally groups resources by sleds, as the 4 primary maps in a `Blueprint` (`sled_state`, `blueprint_zones`, `blueprint_disks`, `blueprint_datasets`) are going to be collapsed into a single map. This allows us to modify the visitors to traverse the new blueprint diffs in one place and not have to worry about modifying the `BlueprintDiff` itself or any consumers. More details about why we are collapsing these maps can be found in #7078. We primarily use `BlueprintDiff`s for testing and omdb output, and therefore the printed representation is absolutely critical for us. This commit reuses the types and formatting contained in `nexus/ types/src/deployment/ blueprint_display.rs` and provides a new type, `BpDiffPrintable` that can be created from a `BlueprintDiff`. `BpDiffPrintable` contains our formatted tables ready to be used by `BlueprintDiffDisplay` along with the orignal blueprints to render the diff. It's possible that we can collapse `BlueprintDiffDisplay` and `BpDiffPrintable` into one type and simplify a bit. I just haven't done that yet. The printable output of the diffs has maintained backwards compatibility in all cases except for errors and warnings. Those have been specifically adapted to work with our visitors and be accumulated while walking the diffus diffs. This only resulted in the change to one test output file, which should leave us confident in the correctness of this new implementation. Usage going forward -------------------- This is a significant change to our blueprint diff code, and in some cases it may take slightly more boilerplate in order to diff newly added fields or structs, than in the older code. However, there are a few things this new style has going for it. Figuring out the differences between blueprints is now done automatically and completely. There is no need to compute these differences, which is both error prone, and complicated. See the old [BpDiffZones](https:// github.com/ oxidecomputer/omicron/blob/888f90db8b36ccdf667d96423ae7805824c48aa9/ nexus/types/src/deployment/blueprint_diff.rs#L195-L340) code for what finding the differences for just zones in a blueprint looked like. With automated diffs output from diffus, we now need to write code to expose the necessary change information to consumers. We want to do this in a unified and rigorous manner, and one that composes nicely. We chose a `visitor` pattern for this. For each somewhat complex type (use your judgement), we provide a visitor trait that walks the heterogeneous diff tree and calls trait methods when a change is found. User code implements only the visitor methods that it cares about and then can accumulate its own internal state based on which callbacks fire. We provide a top-level `VisitBlueprint` trait to detect all changes in a blueprint and implement it in `BlueprintDiffer` to construct a `BlueprintDiff` in a format we can use. There are a few important things to note about our visitors. While diffus provides a complete tree of all changes as well as fields and variants that have not changed, the visitors as currently implemented only expose changes. These changes are unified for simplicity into a `Change` type. We also don't currently expose all changes. For most fields that we don't expect to ever change, there is not a change callback. Mostly this is because the visitors were written before I had to use them :). It turns out it may actually useful to expose these fields so that we can report errors or warnings in our diffs if they do change unexpectedly. You can see an example of error tracking in the `BlueprintDiffer::visit_zone_type_change` method and its test output in `nexus/reconfigurator/planning/tests/output/ planner_nonprovisionable_2_2a.txt`. It is tedious to write visitor traits to walk a diffus diff and then implement these traits to accumulate the state we want. It can reasonably be asked why we wouldn't just walk the tree and generate the state we want directly without the visitors. Indeed, this is quite reasonable for small one off tasks. But we are building a foundation that will grow over time in terms of type structure and number of developers working on the system. For such a long term project, it's usefulf to decouple the tree walking from the state accumulation. For one thing, it makes each bit easier to read and write on its own. For another, it allows us to build different implementations of the visitors for different use cases, such as testing. Remember, users only have to implement the methods they care about. Therefore if they only want to see if a zone changed, they don't have to worry about parsing diffus types, but can just implement a callback. An example of this is the `TestVisitor` in `live-tests/tests/test_nexus_add_remove.rs`. I anticipate we will find ways to make implementing and using diff visitors more ergonomic over time while maintaining the compositional rigor that the separation of concerns provides in this model. Testing -------- I mainly have run planning tests locally to ensure diffs work as expected. I still need to run the live tests and play with omdb.
andrewjstone
added a commit
that referenced
this pull request
Jan 25, 2025
This commit introduces an automated blueprint diff mechanism based on [diffus](https://github.com/oxidecomputer/diffus) along with the visitor pattern for traversing heterogeneous diff trees. It builds on several prior commits that introduced diffus support and added visitors for types: #7261 #7336 #7362 #7366 The new `BlueprintDiffer` implements the relevant visitors and accumulates change state from visitor method callbacks. The accumulated state is the `BlueprintDiff` which replaces the old `BlueprintDiff`. The new structure intentionally groups resources by sleds, as the 4 primary maps in a `Blueprint` (`sled_state`, `blueprint_zones`, `blueprint_disks`, `blueprint_datasets`) are going to be collapsed into a single map. This allows us to modify the visitors to traverse the new blueprint diffs in one place and not have to worry about modifying the `BlueprintDiff` itself or any consumers. More details about why we are collapsing these maps can be found in #7078. We primarily use `BlueprintDiff`s for testing and omdb output, and therefore the printed representation is absolutely critical for us. This commit reuses the types and formatting contained in `nexus/ types/src/deployment/ blueprint_display.rs` and provides a new type, `BpDiffPrintable` that can be created from a `BlueprintDiff`. `BpDiffPrintable` contains our formatted tables ready to be used by `BlueprintDiffDisplay` along with the orignal blueprints to render the diff. It's possible that we can collapse `BlueprintDiffDisplay` and `BpDiffPrintable` into one type and simplify a bit. I just haven't done that yet. The printable output of the diffs has maintained backwards compatibility in all cases except for errors and warnings. Those have been specifically adapted to work with our visitors and be accumulated while walking the diffus diffs. This only resulted in the change to one test output file, which should leave us confident in the correctness of this new implementation. Usage going forward -------------------- This is a significant change to our blueprint diff code, and in some cases it may take slightly more boilerplate in order to diff newly added fields or structs, than in the older code. However, there are a few things this new style has going for it. Figuring out the differences between blueprints is now done automatically and completely. There is no need to compute these differences, which is both error prone, and complicated. See the old [BpDiffZones](https:// github.com/oxidecomputer/omicron/blob/888f90db8b36ccdf667d96423ae7805824c48aa9/nexus/types/src/deployment/blueprint_diff.rs#L195-L340) code for what finding the differences for just zones in a blueprint looked like. With automated diffs output from diffus, we now need to write code to expose the necessary change information to consumers. We want to do this in a unified and rigorous manner, and one that composes nicely. We chose a `visitor` pattern for this. For each somewhat complex type (use your judgement), we provide a visitor trait that walks the heterogeneous diff tree and calls trait methods when a change is found. User code implements only the visitor methods that it cares about and then can accumulate its own internal state based on which callbacks fire. We provide a top-level `VisitBlueprint` trait to detect all changes in a blueprint and implement it in `BlueprintDiffer` to construct a `BlueprintDiff` in a format we can use. There are a few important things to note about our visitors. While diffus provides a complete tree of all changes as well as fields and variants that have not changed, the visitors as currently implemented only expose changes. These changes are unified for simplicity into a `Change` type. We also don't currently expose all changes. For most fields that we don't expect to ever change, there is not a change callback. Mostly this is because the visitors were written before I had to use them :). It turns out it may actually useful to expose these fields so that we can report errors or warnings in our diffs if they do change unexpectedly. You can see an example of error tracking in the `BlueprintDiffer::visit_zone_type_change` method and its test output in `nexus/reconfigurator/planning/tests/output/ planner_nonprovisionable_2_2a.txt`. It is tedious to write visitor traits to walk a diffus diff and then implement these traits to accumulate the state we want. It can reasonably be asked why we wouldn't just walk the tree and generate the state we want directly without the visitors. Indeed, this is quite reasonable for small one off tasks. But we are building a foundation that will grow over time in terms of type structure and number of developers working on the system. For such a long term project, it's usefulf to decouple the tree walking from the state accumulation. For one thing, it makes each bit easier to read and write on its own. For another, it allows us to build different implementations of the visitors for different use cases, such as testing. Remember, users only have to implement the methods they care about. Therefore if they only want to see if a zone changed, they don't have to worry about parsing diffus types, but can just implement a callback. An example of this is the `TestVisitor` in `live-tests/tests/test_nexus_add_remove.rs`. I anticipate we will find ways to make implementing and using diff visitors more ergonomic over time while maintaining the compositional rigor that the separation of concerns provides in this model. Testing -------- I mainly have run planning tests locally to ensure diffs work as expected. I still need to run the live tests and play with omdb.
andrewjstone
added a commit
that referenced
this pull request
Jan 25, 2025
This commit introduces an automated blueprint diff mechanism based on [diffus](https://github.com/oxidecomputer/diffus) along with the visitor pattern for traversing heterogeneous diff trees. It builds on several prior commits that introduced diffus support and added visitors for types: #7261 #7336 #7362 #7366 The new `BlueprintDiffer` implements the relevant visitors and accumulates change state from visitor method callbacks. The accumulated state is the `BlueprintDiff` which replaces the old `BlueprintDiff`. The new structure intentionally groups resources by sleds, as the 4 primary maps in a `Blueprint` (`sled_state`, `blueprint_zones`, `blueprint_disks`, `blueprint_datasets`) are going to be collapsed into a single map. This allows us to modify the visitors to traverse the new blueprint diffs in one place and not have to worry about modifying the `BlueprintDiff` itself or any consumers. More details about why we are collapsing these maps can be found in #7078. We primarily use `BlueprintDiff`s for testing and omdb output, and therefore the printed representation is absolutely critical for us. This commit reuses the types and formatting contained in `nexus/ types/src/deployment/ blueprint_display.rs` and provides a new type, `BpDiffPrintable` that can be created from a `BlueprintDiff`. `BpDiffPrintable` contains our formatted tables ready to be used by `BlueprintDiffDisplay` along with the orignal blueprints to render the diff. It's possible that we can collapse `BlueprintDiffDisplay` and `BpDiffPrintable` into one type and simplify a bit. I just haven't done that yet. The printable output of the diffs has maintained backwards compatibility in all cases except for errors and warnings. Those have been specifically adapted to work with our visitors and be accumulated while walking the diffus diffs. This only resulted in the change to one test output file, which should leave us confident in the correctness of this new implementation. An optional `show_unchanged` flag was added at key points in the code and in the future we plan to change the default to only show actual changes. We didn't do that here so that we could ensure the output of the new implementation matches the existing code. We also plan to add more columns, such as `disposition` for datasets. Usage going forward -------------------- This is a significant change to our blueprint diff code, and in some cases it may take slightly more boilerplate in order to diff newly added fields or structs, than in the older code. However, there are a few things this new style has going for it. Figuring out the differences between blueprints is now done automatically and completely. There is no need to compute these differences, which is both error prone, and complicated. See the old [BpDiffZones](https:// github.com/oxidecomputer/omicron/blob/888f90db8b36ccdf667d96423ae7805824c48aa9/nexus/types/src/deployment/blueprint_diff.rs#L195-L340) code for what finding the differences for just zones in a blueprint looked like. With automated diffs output from diffus, we now need to write code to expose the necessary change information to consumers. We want to do this in a unified and rigorous manner, and one that composes nicely. We chose a `visitor` pattern for this. For each somewhat complex type (use your judgement), we provide a visitor trait that walks the heterogeneous diff tree and calls trait methods when a change is found. User code implements only the visitor methods that it cares about and then can accumulate its own internal state based on which callbacks fire. We provide a top-level `VisitBlueprint` trait to detect all changes in a blueprint and implement it in `BlueprintDiffer` to construct a `BlueprintDiff` in a format we can use. There are a few important things to note about our visitors. While diffus provides a complete tree of all changes as well as fields and variants that have not changed, the visitors as currently implemented only expose changes. These changes are unified for simplicity into a `Change` type. We also don't currently expose all changes. For most fields that we don't expect to ever change, there is not a change callback. Mostly this is because the visitors were written before I had to use them :). It turns out it may actually useful to expose these fields so that we can report errors or warnings in our diffs if they do change unexpectedly. You can see an example of error tracking in the `BlueprintDiffer::visit_zone_type_change` method and its test output in `nexus/reconfigurator/planning/tests/output/ planner_nonprovisionable_2_2a.txt`. It is tedious to write visitor traits to walk a diffus diff and then implement these traits to accumulate the state we want. It can reasonably be asked why we wouldn't just walk the tree and generate the state we want directly without the visitors. Indeed, this is quite reasonable for small one off tasks. But we are building a foundation that will grow over time in terms of type structure and number of developers working on the system. For such a long term project, it's usefulf to decouple the tree walking from the state accumulation. For one thing, it makes each bit easier to read and write on its own. For another, it allows us to build different implementations of the visitors for different use cases, such as testing. Remember, users only have to implement the methods they care about. Therefore if they only want to see if a zone changed, they don't have to worry about parsing diffus types, but can just implement a callback. An example of this is the `TestVisitor` in `live-tests/tests/test_nexus_add_remove.rs`. I anticipate we will find ways to make implementing and using diff visitors more ergonomic over time while maintaining the compositional rigor that the separation of concerns provides in this model. Testing -------- I mainly have run planning tests locally to ensure diffs work as expected. I still need to run the live tests and play with omdb.
andrewjstone
added a commit
that referenced
this pull request
Jan 25, 2025
This commit introduces an automated blueprint diff mechanism based on [diffus](https://github.com/oxidecomputer/diffus) along with the visitor pattern for traversing heterogeneous diff trees. It builds on several prior commits that introduced diffus support and added visitors for types: #7261 #7336 #7362 #7366 The new `BlueprintDiffer` implements the relevant visitors and accumulates change state from visitor method callbacks. The accumulated state is the `BlueprintDiff` which replaces the old `BlueprintDiff`. The new structure intentionally groups resources by sleds, as the 4 primary maps in a `Blueprint` (`sled_state`, `blueprint_zones`, `blueprint_disks`, `blueprint_datasets`) are going to be collapsed into a single map. This allows us to modify the visitors to traverse the new blueprint diffs in one place and not have to worry about modifying the `BlueprintDiff` itself or any consumers. More details about why we are collapsing these maps can be found in #7078. We primarily use `BlueprintDiff`s for testing and omdb output, and therefore the printed representation is absolutely critical for us. This commit reuses the types and formatting contained in `nexus/ types/src/deployment/ blueprint_display.rs` and provides a new type, `BpDiffPrintable` that can be created from a `BlueprintDiff`. `BpDiffPrintable` contains our formatted tables ready to be used by `BlueprintDiffDisplay` along with the orignal blueprints to render the diff. It's possible that we can collapse `BlueprintDiffDisplay` and `BpDiffPrintable` into one type and simplify a bit. I just haven't done that yet. The printable output of the diffs has maintained backwards compatibility in all cases except for errors and warnings. Those have been specifically adapted to work with our visitors and be accumulated while walking the diffus diffs. This only resulted in the change to one test output file, which should leave us confident in the correctness of this new implementation. An optional `show_unchanged` flag was added at key points in the code and in the future we plan to change the default to only show actual changes. We didn't do that here so that we could ensure the output of the new implementation matches the existing code. We also plan to add more columns, such as `disposition` for datasets. Usage going forward -------------------- This is a significant change to our blueprint diff code, and in some cases it may take slightly more boilerplate in order to diff newly added fields or structs, than in the older code. However, there are a few things this new style has going for it. Figuring out the differences between blueprints is now done automatically and completely. There is no need to compute these differences, which is both error prone, and complicated. See the old [BpDiffZones](https:// github.com/oxidecomputer/omicron/blob/888f90db8b36ccdf667d96423ae7805824c48aa9/nexus/types/src/deployment/blueprint_diff.rs#L195-L340) code for what finding the differences for just zones in a blueprint looked like. With automated diffs output from diffus, we now need to write code to expose the necessary change information to consumers. We want to do this in a unified and rigorous manner, and one that composes nicely. We chose a `visitor` pattern for this. For each somewhat complex type (use your judgement), we provide a visitor trait that walks the heterogeneous diff tree and calls trait methods when a change is found. User code implements only the visitor methods that it cares about and then can accumulate its own internal state based on which callbacks fire. We provide a top-level `VisitBlueprint` trait to detect all changes in a blueprint and implement it in `BlueprintDiffer` to construct a `BlueprintDiff` in a format we can use. There are a few important things to note about our visitors. While diffus provides a complete tree of all changes as well as fields and variants that have not changed, the visitors as currently implemented only expose changes. These changes are unified for simplicity into a `Change` type. We also don't currently expose all changes. For most fields that we don't expect to ever change, there is not a change callback. Mostly this is because the visitors were written before I had to use them :). It turns out it may actually be useful to expose these fields so that we can report errors or warnings in our diffs if they do change unexpectedly. You can see an example of error tracking in the `BlueprintDiffer::visit_zone_type_change` method and its test output in `nexus/reconfigurator/planning/tests/output/ planner_nonprovisionable_2_2a.txt`. It is tedious to write visitor traits to walk a diffus diff and then implement these traits to accumulate the state we want. It can reasonably be asked why we wouldn't just walk the tree and generate the state we want directly without the visitors. Indeed, this is quite reasonable for small one off tasks. But we are building a foundation that will grow over time in terms of type structure and number of developers working on the system. For such a long term project, it's usefulf to decouple the tree walking from the state accumulation. For one thing, it makes each bit easier to read and write on its own. For another, it allows us to build different implementations of the visitors for different use cases, such as testing. Remember, users only have to implement the methods they care about. Therefore if they only want to see if a zone changed, they don't have to worry about parsing diffus types, but can just implement a callback. An example of this is the `TestVisitor` in `live-tests/tests/test_nexus_add_remove.rs`. I anticipate we will find ways to make implementing and using diff visitors more ergonomic over time while maintaining the compositional rigor that the separation of concerns provides in this model. Testing -------- I mainly have run planning tests locally to ensure diffs work as expected. I still need to run the live tests and play with omdb.
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.
This visitor walks a diffus generated diff and provides callbacks for users to hook into the parts of the diff that they care about.
To keep things minimal, we don't provide callback trait methods for copies (when nothing changes), and we don't provide methods for most diffus_derive generated types (the ones that start with
Edited).We anticipate composing these visitors to build up a full diff of a Blueprint. They can also be used individually as demonstrated with the included property based test.
In order to easily generate
BlueprintZoneConfigsfor property tests we derivedArbitraryforBlueprintZoneConfigsand its fields using thetest-strategycrate. Some of the types in the hierarchy don't actually implementArbitraryand they come from foreign crates. In this case a corresponding generation strategy was added that allows those types to be generated based on more primitive types that do implement arbitrary.This PR is just one of many coming to build up and use automated blueprint diffs. It builds on the following PRs which must be merged in first: