-
Notifications
You must be signed in to change notification settings - Fork 39
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
Discretionary external DNS zones #6581
Conversation
External DNS addresses are not yet in the policy, so we must grovel for them in the parent blueprint. See `test_reuse_external_dns_ips_from_expunged_zones()`.
3562f3d
to
0fd9b5c
Compare
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 is looking good! I have a few questions below about allocating datasets and addresses. And yeah it'd be good to get @jgallagher's eyes on this too.
nexus/reconfigurator/planning/src/blueprint_builder/external_networking.rs
Outdated
Show resolved
Hide resolved
nexus/reconfigurator/planning/src/blueprint_builder/external_networking.rs
Outdated
Show resolved
Hide resolved
nexus/reconfigurator/planning/src/blueprint_builder/external_networking.rs
Outdated
Show resolved
Hide resolved
nexus/reconfigurator/planning/src/blueprint_builder/external_networking.rs
Outdated
Show resolved
Hide resolved
nexus/reconfigurator/planning/src/blueprint_builder/external_networking.rs
Outdated
Show resolved
Hide resolved
nexus/reconfigurator/planning/src/blueprint_builder/external_networking.rs
Outdated
Show resolved
Hide resolved
The "Check the planning input" block of code needs to consider _all_ zones in the parent blueprint, including expunged zones. #6483 fixed the planner's ability to reuse external IPs from expunged zones, but accidentally made these checks incorrect, because it's certainly valid for the planning input to still have records for expunged zones. See #6581 (comment) for more context. We also get to remove the somewhat-awkward update_network_resources_from_blueprint() test helper that was needed in #6483 to make some tests pass (because we didn't realize the checks being performed here were wrong).
The "Check the planning input" block of code needs to consider _all_ zones in the parent blueprint, including expunged zones. #6483 fixed the planner's ability to reuse external IPs from expunged zones, but accidentally made these checks incorrect, because it's certainly valid for the planning input to still have records for expunged zones. See #6581 (comment) for more context. We also get to remove the somewhat-awkward update_network_resources_from_blueprint() test helper that was needed in #6483 to make some tests pass (because we didn't realize the checks being performed here were wrong).
Fails the `planner_reuse_external_dns_ips_from_expunged_zones` test, but because of Nexus, not external DNS.
…ng (#6599) The "Check the planning input" block of code needs to consider _all_ zones in the parent blueprint, including expunged zones. #6483 fixed the planner's ability to reuse external IPs from expunged zones, but accidentally made these checks incorrect, because it's certainly valid for the planning input to still have records for expunged zones. See #6581 (comment) for more context. We also get to remove the somewhat-awkward `update_network_resources_from_blueprint()` test helper that was needed to make some tests pass (because we didn't realize the checks being performed here were wrong).
Makes running out of external DNS address *not* be a fatal planning error. Requires relaxing the constraint that the requested number of zones equal the added number of zones. Also collect plain IP addresses for available DNS addresses, not floating IPs.
nexus/reconfigurator/planning/src/blueprint_builder/external_networking.rs
Outdated
Show resolved
Hide resolved
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.
LGTM! Thanks for fighting all the dragons here.
External DNS addresses are not yet in the policy (#3732), so we must grovel for them in the parent blueprint (and also a bit in the planning input). Most of the grubbiness here will go away when that's fixed; see the
TODO-cleanup
notes.Still validating on
a4x2
, but otherwise ready for review.