[reconfigurator] Planner: partition external DNS IP allocation separately from other service IP allocation#7056
Conversation
Removes three tests that aren't particularly useful: they're validating that the builder rejects bogus PlanningInputs, but since those tests were written we've updating PlanningInput itself to reject those inputs. With the rearranging done in this PR it's not possible to construct a PlanningInput that causes these tests to fail in the same way.
| // Check the planning input: there shouldn't be any external | ||
| // networking resources in the database (the source of `input`) | ||
| // that we don't know about from the parent blueprint. | ||
| ensure_input_networking_records_appear_in_parent_blueprint( |
There was a problem hiding this comment.
This was previously called inside BuilderExternalNetworking::new(); I moved it out here because it's the only thing that actually looked at the parent blueprint, and moving it made it much simpler to test BuilderExternalNetworking in isolation (it no longer requires constructing an entire blueprint).
| } | ||
|
|
||
| #[test] | ||
| fn test_invalid_parent_blueprint_two_zones_with_same_external_ip() { |
There was a problem hiding this comment.
I removed these tests because they were sensitive to the change in ordering of moving ensure_input_networking_records_appear_in_parent_blueprint from where it appeared in BuilderExternalNetworking::new() to just before calling BuilderExternalNetworking::new() - we get different errors instead of the specific errors these tests were checking for.
I don't think there's an easy way to fix the tests, because since these tests were written, we've introduced checks on constructing PlanningInput itself that ensures we don't have any duplicate IPs / NIC IPs / MACs. Particularly given the checks we're doing there, these seemed worthy of being dropped.
plotnick
left a comment
There was a problem hiding this comment.
Thanks for fixing this! The fix and the test both look great, and the simplifications and deletions along the way are an added bonus.
| use omicron_uuid_kinds::GenericUuid; | ||
| use omicron_uuid_kinds::OmicronZoneUuid; | ||
| use omicron_uuid_kinds::ZpoolUuid; | ||
| use slog_error_chain::InlineErrorChain; |
| input: &'a PlanningInput, | ||
| service_ip_pool_ranges: &'a [IpRange], |
There was a problem hiding this comment.
Decoupling this builder from the PlanningInput seems like an unambiguous win.
Fixes #7049; see it for context.