fix(api): make tenant FQDNs + FNN detection IPv6-aware - #4632
Conversation
|
@coderabbitai full_review, thanks! |
|
ᕱᕱ ✅ Action performedFull review finished. |
|
Warning Review limit reached
Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
Summary by CodeRabbit
WalkthroughThe PR makes DPU physical IP selection deterministic, centralizes tenant FQDN construction, exposes IP hostname conversion, extends IPv6 FNN detection, and removes the deprecated PostgreSQL IPv4 allocation path. ChangesNetwork and DPU addressing
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/api-core/src/handlers/dpu.rs`:
- Around line 384-386: Update the error message passed to CarbideError::internal
in the physical IP lookup error path to use the lowercase phrase “physical IP
address not found,” preserving the existing error handling.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 08dde7e8-05b2-46ef-87a2-c66d463e65b4
📒 Files selected for processing (4)
crates/api-core/src/handlers/dpu.rscrates/api-db/src/host_naming/mod.rscrates/api-db/src/ip_allocator.rscrates/api-model/src/network_prefix.rs
💤 Files with no reviewable changes (1)
- crates/api-db/src/ip_allocator.rs
|
@coderabbitai full_review, thanks! |
|
ᕱᕱ ✅ Action performedFull review finished. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@coderabbitai full_review, thanks! |
|
ᕱᕱ ✅ Action performedFull review finished. |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4632.docs.buildwithfern.com/infra-controller |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/api-core/src/handlers/dpu.rs`:
- Around line 1574-1581: Replace the value_scenarios! usage around
tenant_interface_fqdn with scenarios! so the returned Result is preserved.
Define each scenario’s expected successful or failing outcome using
Outcome::Yields(...), and remove the unwrap() call from the run closure.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 191fc16f-b5de-40fb-b6c1-9391803df71f
📒 Files selected for processing (4)
crates/api-core/src/handlers/dpu.rscrates/api-db/src/host_naming/mod.rscrates/api-db/src/ip_allocator.rscrates/api-model/src/network_prefix.rs
💤 Files with no reviewable changes (1)
- crates/api-db/src/ip_allocator.rs
As it stood, the tenant FQDN fallback grabbed whichever physical address happened to come out of a `HashMap`, then only knew how to turn dotted IPv4 into a DNS label. That made IPv6-only names invalid and let a dual-stack name change with map iteration order. So, this picks IPv4 first for compatibility, uses the lowest address within each family, and runs the fallback through the existing `address_to_hostname` formatter. It also teaches `smells_like_fnn` about `/126` and narrower IPv6 linknets, documents why `gateway_cidr` keeps the segment prefix length, and removes the unused `next_machine_interface_v4_ip` path that could never support IPv6 allocation. I left VPC virtualization updates alone. `ETV -> FNN` is the only supported direction, and FNN already accepts every segment type and address family ETV can use; adding reverse-transition guards would make an unsupported path look intentional. The table tests cover stable IPv4/IPv6 selection, IPv6-safe FQDNs, gateway formatting, and both FNN prefix cutoffs. This supports NVIDIA#2406 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai full_review, thanks! |
|
ᕱᕱ
|
3 participants
As it stood, the tenant FQDN fallback grabbed whichever physical address happened to come out of a
HashMap, then only knew how to turn dotted IPv4 into a DNS label. That made IPv6-only names invalid and let a dual-stack name change with map iteration order.So, this picks IPv4 first for compatibility, uses the lowest address within each family, and runs the fallback through the existing
address_to_hostnameformatter. It also teachessmells_like_fnnabout/126and narrower IPv6 linknets, documents whygateway_cidrkeeps the segment prefix length, and removes the unusednext_machine_interface_v4_ippath that could never support IPv6 allocation.The result is a small cleanup of the remaining IPv4-only assumptions without changing the supported VPC migration model.
Related issues
Type of Change
Breaking Changes
Testing
Additional Notes
UpdateVpcVirtualizationis deliberately unchanged.ETV -> FNNis the only supported direction, and FNN already accepts every segment type and address family ETV can use; adding guards for reverse transitions would make an unsupported path look intentional.Closes #2406