Add external_networking_generation to Blueprint#10331
Add external_networking_generation to Blueprint#10331jgallagher wants to merge 17 commits intomainfrom
external_networking_generation to Blueprint#10331Conversation
| } | ||
| } | ||
|
|
||
| fn is_external_networking_config_different( |
There was a problem hiding this comment.
While we're here, thoughts on adding a small set of unit tests for this? Also do we need a blippy check?
There was a problem hiding this comment.
Ehh, I skipped unit tests because (a) generating full Blueprints is relatively onerous for a quite-small function and (b) I thought updating the planner integration tests provided sufficient coverage. But if you strongly want unit tests I can add some.
Adding a blippy check is a great idea; I'll do that shortly.
There was a problem hiding this comment.
Ehh, I skipped unit tests because (a) generating full Blueprints is relatively onerous for a quite-small function and (b) I thought updating the planner integration tests provided sufficient coverage. But if you strongly want unit tests I can add some.
Fair enough -- I guess if you pass in an iterator instead (as mentioned above) you could do this, but yeah, the existing integration tests are generally ok.
The blippy check could also verify that external networking isn't mutated in place, or maybe an even more general property that none of the zone config is mutated in place :) (maybe it already does this today)
There was a problem hiding this comment.
Added a blippy check for this specifically in f3a54fe.
We don't have a blippy check for "zone wasn't mutated in place", but it seems reasonable to add. I'll file an issue.
There was a problem hiding this comment.
Thanks. That looks good. I guess duplicating the diffing code is fine since blippy carries its own source of truth.
There was a problem hiding this comment.
Yeah, I didn't like doing that but it really is exactly the check to perform 🤷
| if blippy.blueprint().external_networking_generation | ||
| != parent_blueprint.external_networking_generation.next() | ||
| { | ||
| blippy.push_planning_input_note( | ||
| Severity::Fatal, | ||
| PlanningInputKind::ExternalNetworkingGenerationBumpedUnexpectedly { | ||
| parent_generation: | ||
| parent_blueprint.external_networking_generation, | ||
| expected_child_generation: | ||
| parent_blueprint.external_networking_generation.next(), | ||
| actual_child_generation: | ||
| blippy.blueprint().external_networking_generation, | ||
| }, | ||
| ); | ||
| } | ||
| } else { | ||
| if blippy.blueprint().external_networking_generation | ||
| != parent_blueprint.external_networking_generation | ||
| { | ||
| blippy.push_planning_input_note( | ||
| Severity::Fatal, | ||
| PlanningInputKind::ExternalNetworkingGenerationNotBumped( | ||
| blippy.blueprint().external_networking_generation, | ||
| ), | ||
| ); | ||
| } |
There was a problem hiding this comment.
This seems reversed to me -- if we expected a bump and did not get it (the first block), we should return ExternalNetworkingGenerationNotBumped I think. And similarly for the second block.
There was a problem hiding this comment.
Whoops, yeah, that was the original intent when I named ExternalNetworkingGenerationNotBumped, but then realized the "not bumped" case should also check "generation changed by more than 1", so NotBumped wasn't enough information.
I reworked this in 8b42f80 - there's now just one error variant, and we always include the parent gen, child gen, and expected child gen in the error.
sunshowers
left a comment
sunshowers
left a comment
There was a problem hiding this comment.
thanks! you'll want to regenerate OpenAPI documents before landing.
2 participants
This is the first half of #10320: this adds a generation number to the blueprint that gets bumped whenever the complete set of "external networking config" changes. A followup PR will change the sync background task mentioned in that issue to actually use this generation number to avoid dueling.
The code changes in this PR are pretty small; most of the files changes are expectorate tests that now show a generation number. The real changes here are:
BlueprintBuilder::build()to bump the generation if needed when emitting the new blueprint