Perform semantic blueprint diffs via daft#7466
Conversation
This PR introduces the generation of automated blueprint diffs via [daft](https://github.com/oxidecomputer/daft). It is still necessary to massage the automated diff output into existing types so that we can present them graphically. This is performed via a wrapper, `BlueprintDiffSummary`, which is also used in tests. This change is 100% backwards compatible and generates identical diff output to the hand rolled diffs. Functionality was recently added to `daft` to [allow lazy diffing](oxidecomputer/daft#16), via stopping recursion and returning a `Leaf` even if a field of a struct is itself a diffable struct. Doing this allows us to not recures into smaller structures like `ZpoolName` that we want to treat opaquely, and also allows us to maintain some handwritten diffs for output that is not changing any time soon, such as `BlueprintMetadata` and `ClickhouseClusterConfig`. This PR supercedes #7402 and removes all previously merged diffus related visitors. It also removes some left over types dealing with diffing collections and blueprints which is not currently supported. Follow up PRs will expand our diff output using the new functionality to add things like dataset disposition, etc...
jgallagher
left a comment
jgallagher
left a comment
There was a problem hiding this comment.
Nice - daft is very slick!
|
|
||
| // We can't do the same for removals unfortunately. We prematurely | ||
| // prune decommissioned sleds, but there may still be zones, disks, | ||
| // or datasets that have not yet been removed. We must check for this. |
There was a problem hiding this comment.
I was going to ask if we could switch this to just looking at sled_state once #7470 lands, but I guess that would break diffs for blueprints involving decommissioned sleds made before that change landed. Maybe once we start pruning old blueprints, then?
There was a problem hiding this comment.
Yeah, all things that break diffs should come in follow ups.
| } | ||
|
|
||
| self.diff.clickhouse_cluster_config.before | ||
| != self.diff.clickhouse_cluster_config.after |
There was a problem hiding this comment.
Do we also need to check the cockroach stuff (fingerprint, preserve_downgrade)?
There was a problem hiding this comment.
Maybe? I was just keeping this compatible with what was already here, but I will add that.
There was a problem hiding this comment.
Now I'm wondering if we also want to consider internal_dns_version and external_dns_version as well ?
@davepacheco any thoughts here?
| // Do we have any errors? If so, create a "reason" string. | ||
| let mut reason = String::new(); | ||
| // These first two checks are only for backwards compatibility. They are | ||
| // all included in the zone_type comparison below. |
There was a problem hiding this comment.
Can we drop these two since they're covered by the later zone_type check? If that changes the diffs from tests, maybe do that as a small followup PR?
There was a problem hiding this comment.
I anticipate a few cleanup tasks in a follow up that break backwards compatibility. I'll add this one.
| generation_before: Generation, | ||
| generation_after: Generation, | ||
| zone_diffs: impl Iterator<Item = &'a BlueprintZoneConfigDiff<'a>>, | ||
| ) -> (BpDiffZonesModified, BpDiffZoneErrors) { |
There was a problem hiding this comment.
I don't love this interface, but I'm not sure what to suggest instead. Do you think an enum with 3 cases might be clearer? Something like
enum BpDiffZonesResult {
Ok(BpDiffZonesModified), // all comparisons are fine
Err(BpDiffZoneErrors), // all comparisons failed
Mixed(BpDiffZonesModified, BpDiffZoneErrors), // some of each
}since we expect to basically always be in the Ok(_) case?
There was a problem hiding this comment.
Eh, matching that in callers seems worse.
2 participants
This PR introduces the generation of automated blueprint diffs via daft.
It is still necessary to massage the automated diff output into existing types so that we can present them graphically. This is performed via a wrapper,
BlueprintDiffSummary, which is also used in tests.This change is 100% backwards compatible and generates identical diff output to the hand rolled diffs. Functionality was recently added to
daftto allow lazy diffing, via stopping recursion and returning aLeafeven if a field of a struct is itself a diffable struct. Doing this allows us to not recures into smaller structures likeZpoolNamethat we want to treat opaquely, and also allows us to maintain some handwritten diffs for output that is not changing any time soon, such asBlueprintMetadataandClickhouseClusterConfig.This PR supercedes #7402 and removes all previously merged diffus related visitors. It also removes some left over types dealing with diffing collections and blueprints which is not currently supported.
Follow up PRs will expand our diff output using the new functionality to add things like dataset disposition, etc...