Skip to content

Conversation

@zeeshanlakhani
Copy link
Collaborator

@zeeshanlakhani zeeshanlakhani commented Dec 23, 2025

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, NOT involving changes that have to do with the mcast lifecycle work. This supersedes #9452.

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
  • Validate pool/ip_version compatibility upfront when both are specified (returns an explicit error if the requested version doesn't match the pool's version)

@zeeshanlakhani zeeshanlakhani force-pushed the zl/multiple-default-pools branch from 7f3a5b6 to 661d029 Compare December 23, 2025 18:13
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
@zeeshanlakhani
Copy link
Collaborator Author

FYI, even without the implicit lifecycle work, this still has mcast changes (as mcast is on main). I'm pinging @rcgoodfellow and @internet-diglett to take a look-see. I'll update #9450 (and its stacked set of PRs) once this is reviewed and merged accordingly, as I know @bnaecker needs this ASAP for the V6 work.

Copy link
Collaborator

@bnaecker bnaecker left a 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.

@bnaecker
Copy link
Collaborator

@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.

@zeeshanlakhani
Copy link
Collaborator Author

@bnaecker pushing up the post-review changes and a merge to remove the conflict.

@zeeshanlakhani zeeshanlakhani force-pushed the zl/multiple-default-pools branch 2 times, most recently from 5538dbd to af3fb94 Compare December 30, 2025 08:41
@zeeshanlakhani zeeshanlakhani force-pushed the zl/multiple-default-pools branch from af3fb94 to 0fa862d Compare December 30, 2025 08:45
@zeeshanlakhani
Copy link
Collaborator Author

zeeshanlakhani commented Dec 30, 2025

@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).

Copy link
Collaborator

@bnaecker bnaecker left a 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!

@zeeshanlakhani zeeshanlakhani enabled auto-merge (squash) December 31, 2025 00:20
@zeeshanlakhani zeeshanlakhani merged commit 3b3f937 into main Dec 31, 2025
16 checks passed
@zeeshanlakhani zeeshanlakhani deleted the zl/multiple-default-pools branch December 31, 2025 23:31
Comment on lines +1118 to +1124

/// 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>,
Copy link
Contributor

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 pool and ip_version seems to be invalid?

Copy link
Collaborator Author

@zeeshanlakhani zeeshanlakhani Jan 5, 2026

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants