Conversation
huaweic-nv
force-pushed
the
for-review/test-new-gha-ci
branch
from
bf71d8b to
6ffc376
Compare
10 tasks
chet
added a commit
that referenced
this pull request
Feb 10, 2026
…217) ## Description Continuing the [IPv6 support](#84) work. This is the third PR. Work thus far has included: - Moving to `IpNetwork` and `IpAddress` throughout ([#192](#192)). - Accepting IPv6 site prefixes and network segments. ([#204](#204)). And today: adding IPv6 support to IP address allocation. Similar to the previous PRs, this does NOT enable IPv6 support (i.e. there won't be any `DHCPv6` traffic coming in yet), but this DOES contribute to the foundational work of supporting IPv6. Eventually, we'll be able to start actually accepting requests and config for using IPv6, but there's a lot of backend work that needs to be done to get there. Trying to take it a layer at a time, and pieces of each layer at that. This is PR #3 of what is going to be many. In any case, as mentioned, this PR is focused on adding IPv6 support to address allocation, specifically the `IpAllocator`, which is being made IPv6-capable by removing all IPv4-only guards, [re]introducing `AddressSelectionStrategy` to cleanly drive allocation behavior, and unblocking IPv6 allocation across all code paths, including: - Machine interfaces (underlay). - Instance addresses (tenant/overlay) The goal is to prepare the allocation layer so that when DHCPv6 is added later, everything beneath it already works -- existing deployments and config are unaffected, as existing config is all parsed as IPv4 and uses [the same] IPv4 code paths and allocation strategies (which are verified with both existing AND new tests). One of the particularly exciting highlights here is starting to use the `AddressSelectionStrategy` for *something*. Previously, we just passed around `AddressSelectionStrategy::Automatic` for *everything*. Now, we actually use it for address selection, by saying how we want to select the next IP. - `::NextAvailableIp` is synonymous with `::Automatic` -- we pick the next `/32` or `/128` depending on the address family. - `::NextAvailablePrefix(uint)` gives us support for allocating a prefix that is wider than `/32` or `/128`. This allowed us to remove the `prefix_length` from the `IpAllocator` constructor. I was never fully happy with it being there. It seemed to make sense at the time, but it also seemed like there was something better we could do. Making it a part of `AddressSelectionStrategy` is actually what should have happened to begin with. It's also worth noting the SQL `next_machine_interface_v4_ip` fast-path got removed here. It was a nice idea to have, but it didn't play well with dual-stacking interfaces (it actually got rid of dual stack support entirely when it was introduced), and it was only ever going to support IPv4. If we have performance concerns about quickly allocating many machines, we can definitely revisit it, but unfortunately it won't work as-is for now. The previous logic has been put back, with some enhancements for dual stacking. Some callouts around backwards compatibility (in addition to tests): **DHCP is still IPv4-only** `carbide-dhcp` speaks DHCPv4 exclusively — it parses `Ipv4Addr::from_str(&forge_response.address)` on the response and calls `set_yiaddr(allocated_address)`. Even if a tenant segment is configured to support IPv6 prefixes, the DHCP server will never request an IPv6 lease. The allocator is ready, but the trigger isn't there yet. **Machine interface (underlay) IPv4 AND IPv6 work end-to-end** If an operator configures an underlay/admin segment with IPv6 prefixes, `machine_interface::create()` will allocate IPv6 addresses via the `IpAllocator`. The hostname generation, DB writes, and uniqueness constraints all work correctly, in addition to IPv4 continuing to work (using the SQL-based IPv4 allocator when relevant). **Existing IPv4 behavior is untouched** Again, `NextAvailableIp` resolves to `/32` for IPv4 — identical to the old hardcoded `prefix_length: 32`. All existing IPv4 unit and integration tests continue to pass unmodified. Tests added: | Test Name | Purpose | | --------- | ------- | | `test_ip_allocation_ipv6` | IPv6 /112 prefix allocates a /128 correctly and respects reserved addresses. | | `test_ipv6_allocation_single_address` | Allocates from /112 correctly w/ reserved and used IPs + verifies correct skip to `::4`. | | `test_ipv6_num_free_capped` | `num_free()` caps at `u32::MAX` for large IPv6 prefixes (/64). | | `test_ipv6_build_allocated_networks` | Gateway, reserved, network/broadcast exclusions all use /128 for IPv6. | | `test_address_to_hostname_v4` | Ensures IPv4 hostname formatting unchanged. | | `test_address_to_hostname_v6` | Ensures IPv6 hostname formatting is expanded to full form with dashes. | | `test_address_to_hostname_v6_loopback` | Ensures `::1` correctly expands to `0000-0000-...-0001`. | | `test_next_machine_interface_v4_ip` | Ensures existing IPv4 SQL fast-path still works. | | `test_machine_interface_create_with_ipv6_prefix` | Full end-to-end: creates underlay segment with IPv6 /112, allocates two machine interfaces, verifies sequential IPv6 addresses and correct hostnames. | Tests updated: - `test_ip_allocation_ipv4_and_6` - `test_get_network_size` Signed-off-by: Chet Nichols III <chetn@nvidia.com> ## Type of Change <!-- Check one that best describes this PR --> - [x] **Add** - New feature or capability - [ ] **Change** - Changes in existing functionality - [x] **Fix** - Bug fixes - [ ] **Remove** - Removed features or deprecated functionality - [x] **Internal** - Internal changes (refactoring, tests, docs, etc.) ## Related Issues (Optional) #84 ## Breaking Changes - [ ] This PR contains breaking changes <!-- If checked above, describe the breaking changes and migration steps --> ## Testing <!-- How was this tested? Check all that apply --> - [x] Unit tests added/updated - [x] Integration tests added/updated - [ ] Manual testing performed - [ ] No testing required (docs, internal refactor, etc.) ## Additional Notes <!-- Any additional context, deployment notes, or reviewer guidance --> Signed-off-by: Chet Nichols III <chetn@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.