-
Notifications
You must be signed in to change notification settings - Fork 66
[ip-pools] Allow multiple default IP pools per silo #9561
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
Conversation
7f3a5b6 to
661d029
Compare
Previously, each silo could only have one default IP pool. This change allows one default pool per (pool_type, ip_version) combination, enabling silos to have separate defaults for: - Unicast IPv4 - Unicast IPv6 - Multicast IPv4 - Multicast IPv6 This work previously branched off #9451, but is now off `main`, involving changes that have to do with the mcast lifecycle changes. Includes: - Each default can now be set or unset and demoted independently. Unsetting the unicast IPv4 default does not affect the multicast IPv4 default, for example. - Add `pool_type` and `ip_version` columns to `ip_pool_resource` (denormalized from parent `ip_pool` for unique index) - Replace unique index with partial index on (resource_id, pool_type, ip_version) WHERE is_default = true - Rename `IpPoolResourceLink` to `IncompleteIpPoolResource` to reflect that pool_type/ip_version are actually populated by the linking query - Add `ip_version` field to API params for default pool disambiguation - API versioning for backwards compatibility with older clients
661d029 to
c11aae0
Compare
|
FYI, even without the implicit lifecycle work, this still has mcast changes (as mcast is on |
bnaecker
left a comment
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.
Thanks for splitting this out! Seems pretty straightforward to me, just a couple of quick questions.
|
@zeeshanlakhani Let me know if you need anything else here. I'd really like to get this merged so we can move forward on the IPv6 integration work. |
|
@bnaecker pushing up the post-review changes and a merge to remove the conflict. |
5538dbd to
af3fb94
Compare
af3fb94 to
0fa862d
Compare
|
@bnaecker post-review commit added. I'll check to make sure this all passes after dinner. I moved up the check higher in the stack (and commented on it). |
bnaecker
left a comment
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.
Looks good, thanks for the quick changes!
|
|
||
| /// IP version to use when allocating from the default pool. | ||
| /// Only used when both `ip` and `pool` are not specified. Required if | ||
| /// multiple default pools of different IP versions exist. Allocation | ||
| /// fails if no pool of the requested version is available. | ||
| #[serde(default)] | ||
| pub ip_version: Option<IpVersion>, |
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.
2 questions:
- where is this implemented? I don't see it
- did you consider modeling this with an enum since specifying both
poolandip_versionseems to be invalid?
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.
On the Q1: It's in FloatingIpCreate ~> https://github.com/oxidecomputer/omicron/blob/main/nexus/types/src/external_api/params.rs#L1380 and EphermalIpCreate ~> https://github.com/oxidecomputer/omicron/blob/main/nexus/types/src/external_api/params.rs#L1520. There was also a follow-up PR that merged for Silo/project IP Pool GETs: #9585.
The previous API version modules are there and do delegation. The main logic happens in external_ip.rs -> resolve_pool_for_allocation (in the datastore's external_ip.rs) and then in ip_pools_fetch_default_by_type (in the datastore's ip_pool.rs).
On Q2: Great point on the enum. We went with two optionals to match the existing patterns, especially as @bnaecker was working on #9508 concurrently.
I do realize that I have to add ip_version to ProbeCreate, so I'll make this Enum change in another PR.
Previously, each silo could only have one default IP pool. This change allows one default pool per (pool_type, ip_version) combination, enabling silos to have separate defaults for:
This work previously branched off #9451, but is now off
main, NOT involving changes that have to do with the mcast lifecycle work. This supersedes #9452.Includes:
pool_typeandip_versioncolumns toip_pool_resource(denormalized from parentip_poolfor unique index)IpPoolResourceLinktoIncompleteIpPoolResourceto reflect that pool_type/ip_version are actually populated by the linking queryip_versionfield to API params for default pool disambiguation