feat(api): persist DPU IPv6 loopback reservations - #3913
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (15)
🚧 Files skipped from review as they are similar to previous changes (15)
Summary by CodeRabbit
WalkthroughAdds optional IPv6 loopback configuration and resource-pool support for DPUs, including allocation, backfill, CAS-safe updates, startup and pool-growth reconciliation, stale-snapshot cleanup, persistence compatibility, and expanded integration coverage. ChangesIPv6 DPU loopback lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant DPUProvisioning
participant IPv6LoopbackPool
participant MachineDatabase
participant Reconciliation
DPUProvisioning->>IPv6LoopbackPool: allocate loopback_ip_v6
IPv6LoopbackPool-->>DPUProvisioning: reserved IPv6 address
DPUProvisioning->>MachineDatabase: persist network configuration
Reconciliation->>MachineDatabase: find DPUs missing IPv6 loopback
Reconciliation->>IPv6LoopbackPool: allocate missing reservations
Reconciliation->>MachineDatabase: CAS-update loopback_ip_v6
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai full_review, thanks! |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/api-core/src/setup.rs (1)
1028-1033: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider aligning the log severity with the adjacent best-effort reconciliation. This backfill is intentionally non-fatal and its most likely failure (
ConcurrentModificationError) is transient and self-healing on the next startup orAdminGrowResourcePool. Emitting it aterrorwhile the immediately followingupdate_dpu_asnsbest-effort step useswarnrisks spurious alerts for a benign condition. Awarnlevel would keep the two swallow-and-continue paths consistent.♻️ Suggested alignment
if let Err(error) = update_dpu_loopback_ips_v6(db_pool, common_pools).await { - tracing::error!( + tracing::warn!( error = %error, "Failed to update IPv6 loopback IPs for DPUs", ); }🤖 Prompt for 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. In `@crates/api-core/src/setup.rs` around lines 1028 - 1033, Change the tracing severity in the update_dpu_loopback_ips_v6 error branch from error to warn, keeping the existing error context and message unchanged so this non-fatal reconciliation matches the adjacent update_dpu_asns best-effort path.
🤖 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.
Nitpick comments:
In `@crates/api-core/src/setup.rs`:
- Around line 1028-1033: Change the tracing severity in the
update_dpu_loopback_ips_v6 error branch from error to warn, keeping the existing
error context and message unchanged so this non-fatal reconciliation matches the
adjacent update_dpu_asns best-effort path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5b963dff-dbf4-4443-a848-fbfa4f3ae8be
📒 Files selected for processing (18)
crates/api-core/src/handlers/dpu.rscrates/api-core/src/handlers/machine.rscrates/api-core/src/handlers/machine_discovery.rscrates/api-core/src/handlers/resource_pool.rscrates/api-core/src/setup.rscrates/api-core/src/tests/machine_admin_force_delete.rscrates/api-core/src/tests/machine_discovery.rscrates/api-core/src/tests/resource_pool.rscrates/api-db/migrations/20260722120000_preserve_machine_ipv6_loopback.sqlcrates/api-db/src/machine.rscrates/api-db/src/resource_pool.rscrates/api-model/src/machine/mod.rscrates/api-model/src/machine/network.rscrates/api-model/src/resource_pool/common.rscrates/api-model/src/test_support/machine_snapshot.rscrates/site-explorer/src/machine_creator.rscrates/site-explorer/tests/integration/machine_creator.rscrates/test-harness/src/resource_pool.rs
As it stood, the IPv6 underlay work had nowhere durable to keep one loopback address per DPU. NVIDIA#2390 needs that value before it can render the IPv6 underlay, but there was no pool-backed `ManagedHostNetworkConfig.loopback_ip_v6` to provide one. So, this adds the optional `lo-ip-v6` pool and reserves one typed `Ipv6Addr` per DPU. New DPU rows allocate it in `db::machine::create`; existing rows get a bounded backfill during startup or `AdminGrowResourcePool`; discovery and `MachineCreator` fill any remaining gap; and `admin_force_delete_machine` returns the reservation to the pool. Mixed-version deployments need a little extra care here: an older API pod replaces the complete `network_config` document without fields it doesn't know. The targeted `preserve_machine_ipv6_loopback` trigger restores only an omitted `loopback_ip_v6`, while an explicit `null` from a current writer can still clear it. Backfill reuses the DPU's existing reservation, retries compare-and-swap conflicts, and refuses to write once `ForceDeletion` wins; deletion asks the pool for the owner's current reservation instead of trusting its earlier machine snapshot. While collecting the missing address fields into one compare-and-swap write, this also fixes an existing secondary-VTEP leak: discovery could reserve `secondary_overlay_vtep_ip`, make a second write with the now-stale `network_config_version`, ignore the `false` return, and commit the reservation without persisting it. A failed compare-and-swap now fails the transaction, which returns every new address to its pool. This intentionally does **NOT** enable IPv6 FNN rendering by itself. `lo-ip-v6` is optional, so sites without it keep their IPv4-only behavior; NVIDIA#2390 is the step that plumbs the persisted value into the underlay. Deploy this change before configuring `lo-ip-v6` or rolling out NVIDIA#2390, so every API pod knows how to preserve the field before it can be written. The grow-time backfill sees committed DPU rows. A creation transaction that crosses the first `lo-ip-v6` grow can land just after that scan; replaying the additive grow or restarting the API reruns the idempotent pass. The new coverage walks typed serialization, IPv6 pool listing, reservation reuse and exhaustion, mixed-version writes, startup/runtime backfill, compare-and-swap retries, force-deletion races, discovery, cleanup, and multi-DPU `MachineCreator` creation. Tests added! This supports NVIDIA#2389 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
With #2389, each DPU can reserve an optional IPv6 loopback, but the managed-host response still stops at the IPv4 address. That leaves FNN's already-enabled `ipv6-unicast` session without an IPv6 address on `lo` to use as its next hop. So, this plumbs `loopback_ip_v6` through `ManagedHostNetworkConfig`, parses it as `Ipv6Addr` at the agent boundary, renders it as a `/128` on the FNN loopback, and permits that route through the IPv6 underlay export policy. In practice: - `get_managed_host_network_config` uses the requesting DPU's reservation, so two DPUs on one host do not collapse onto one address. - ETV and sites without `lo-ip-v6` stay IPv4-only. - The existing IPv4 loopback remains the router ID and VXLAN VTEP. - `CurrentNetworkVersion` hashes the nested managed-host config so a missed machine-group version fan-out cannot leave a DPU-specific loopback hidden behind the agent cache. - The manual `write nvue` path accepts `--loopback-ip-v6` so operators can reproduce the generated FNN config. ## Related issues This supports #2390 ## Type of Change - [x] **Add** - New feature or capability - [ ] **Change** - Changes in existing functionality - [ ] **Fix** - Bug fixes - [ ] **Remove** - Removed features or deprecated functionality - [ ] **Internal** - Internal changes (refactoring, tests, docs, etc.) ## Breaking Changes - [ ] **This PR contains breaking changes** ## Testing - [x] Unit tests added/updated - [x] Integration tests added/updated - [ ] Manual testing performed - [ ] No testing required (docs, internal refactor, etc.) - `cargo test -p carbide-agent --lib` - `cargo test -p carbide-api-core handlers::dpu::consolidated_network_config_tests::dpu_ipv6_loopback_carries_through_independently --lib -- --exact --nocapture` - `cargo test -p carbide-api-core tests::machine_network::test_managed_host_network_config --lib -- --exact --nocapture` - `cargo test -p carbide-api-core tests::machine_network::test_managed_host_network_config_multi_dpu --lib -- --exact --nocapture` - `cargo test -p carbide-api-core tests::machine_network::test_managed_host_network_config_multi_dpu_fnn_ipv6_loopbacks --lib -- --exact --nocapture` - `cargo make format-nightly` - `cargo make clippy` - `cargo make carbide-lints` - `cargo make --no-workspace generate-rest-core-proto` ## Additional Notes FNN only: ETV intentionally does not receive the optional IPv6 address, and the existing IPv4 loopback remains the router ID and VXLAN VTEP. The optional protobuf field is wire-compatible; older agents ignore it, while newer agents treat its absence as the existing IPv4-only behavior. The REST Core protobuf snapshot and Go binding are regenerated for repository sync, but the curated public REST/OpenAPI model intentionally remains unchanged. The per-DPU reservation dependency landed through #3913. Signed-off-by: Chet Nichols III <chetn@nvidia.com>
…4432) PR #4389 and PR #4428 independently fixed the same duplicate migration version from opposite sides. The loopback preservation migration first reached `main` in #3913 as `20260722120000`, while the BMC suppression migration came later. #4428 correctly moved the newer BMC migration to `20260722120001`, but #4389 had already renamed the older loopback migration to that same version on an earlier base. Since #4428 merged first, #4389 left `main` with two `20260722120001` migrations and removed the identity existing databases may already have in `_sqlx_migrations`. So, restore `20260722120000_preserve_machine_ipv6_loopback.sql` and leave `20260722120001_bmc_suppressions.sql` where it is. The loopback migration SQL is byte-for-byte identical to #3913, existing database histories keep matching the published version and checksum, and SQLx sees each migration version exactly once again. ## Related issues None -- urgent post-merge repair for #4389 and #4428. ## Type of Change - [ ] **Add** - New feature or capability - [ ] **Change** - Changes in existing functionality - [x] **Fix** - Bug fixes - [ ] **Remove** - Removed features or deprecated functionality - [ ] **Internal** - Internal changes (refactoring, tests, docs, etc.) ## Breaking Changes - [ ] **This PR contains breaking changes** ## Testing - [ ] Unit tests added/updated - [ ] Integration tests added/updated - [x] Manual testing performed - [ ] No testing required (docs, internal refactor, etc.) ## Additional Notes - `cargo test -p carbide-api-db migrations::tests --lib` (4 passed) - `cargo make format-nightly` - `cargo make clippy` - `cargo make carbide-lints` - `git diff --check` - Restored migration blob matches the original #3913 blob exactly (`eaff208e4ea6d3956bdb158a84c45ee6c1362ecf`) - This restores the migration identities intended by #4428. A database first initialized during either brief duplicate-version window may have recorded the wrong checksum at `20260722120000` or `20260722120001`; inspect its schema and migration history and perform site-specific repair before retrying rather than blindly replaying these non-idempotent migrations Signed-off-by: Chet Nichols III <chetn@nvidia.com>
…VIDIA#4432) PR NVIDIA#4389 and PR NVIDIA#4428 independently fixed the same duplicate migration version from opposite sides. The loopback preservation migration first reached `main` in NVIDIA#3913 as `20260722120000`, while the BMC suppression migration came later. NVIDIA#4428 correctly moved the newer BMC migration to `20260722120001`, but NVIDIA#4389 had already renamed the older loopback migration to that same version on an earlier base. Since NVIDIA#4428 merged first, NVIDIA#4389 left `main` with two `20260722120001` migrations and removed the identity existing databases may already have in `_sqlx_migrations`. So, restore `20260722120000_preserve_machine_ipv6_loopback.sql` and leave `20260722120001_bmc_suppressions.sql` where it is. The loopback migration SQL is byte-for-byte identical to NVIDIA#3913, existing database histories keep matching the published version and checksum, and SQLx sees each migration version exactly once again. ## Related issues None -- urgent post-merge repair for NVIDIA#4389 and NVIDIA#4428. ## Type of Change - [ ] **Add** - New feature or capability - [ ] **Change** - Changes in existing functionality - [x] **Fix** - Bug fixes - [ ] **Remove** - Removed features or deprecated functionality - [ ] **Internal** - Internal changes (refactoring, tests, docs, etc.) ## Breaking Changes - [ ] **This PR contains breaking changes** ## Testing - [ ] Unit tests added/updated - [ ] Integration tests added/updated - [x] Manual testing performed - [ ] No testing required (docs, internal refactor, etc.) ## Additional Notes - `cargo test -p carbide-api-db migrations::tests --lib` (4 passed) - `cargo make format-nightly` - `cargo make clippy` - `cargo make carbide-lints` - `git diff --check` - Restored migration blob matches the original NVIDIA#3913 blob exactly (`eaff208e4ea6d3956bdb158a84c45ee6c1362ecf`) - This restores the migration identities intended by NVIDIA#4428. A database first initialized during either brief duplicate-version window may have recorded the wrong checksum at `20260722120000` or `20260722120001`; inspect its schema and migration history and perform site-specific repair before retrying rather than blindly replaying these non-idempotent migrations Signed-off-by: Chet Nichols III <chetn@nvidia.com> Signed-off-by: Alex Ball <aball@nvidia.com>
As it stood, the IPv6 underlay work had nowhere durable to keep one loopback address per DPU. #2390 needs that value before it can render the IPv6 underlay, but there was no pool-backed
ManagedHostNetworkConfig.loopback_ip_v6to provide one.So, this adds the optional
lo-ip-v6pool and reserves one typedIpv6Addrper DPU. New DPU rows allocate it indb::machine::create; existing rows get a bounded backfill during startup orAdminGrowResourcePool; discovery andMachineCreatorfill any remaining gap; andadmin_force_delete_machinereturns the reservation to the pool.Mixed-version deployments need a little extra care here: an older API pod replaces the complete
network_configdocument without fields it doesn't know. The targetedpreserve_machine_ipv6_loopbacktrigger restores only an omittedloopback_ip_v6, while an explicitnullfrom a current writer can still clear it. Backfill reuses the DPU's existing reservation, retries compare-and-swap conflicts, and refuses to write onceForceDeletionwins; deletion asks the pool for the owner's current reservation instead of trusting its earlier machine snapshot.While collecting the missing address fields into one compare-and-swap write, this also fixes an existing secondary-VTEP leak: discovery could reserve
secondary_overlay_vtep_ip, make a second write with the now-stalenetwork_config_version, ignore thefalsereturn, and commit the reservation without persisting it. A failed compare-and-swap now fails the transaction, which returns every new address to its pool.Related issues
This supports #2389
Type of Change
Breaking Changes
Testing
The new coverage walks typed serialization, IPv6 pool listing, reservation reuse and exhaustion, mixed-version writes, startup/runtime backfill, compare-and-swap retries, force-deletion races, discovery, cleanup, and multi-DPU
MachineCreatorcreation.Tests added!
Additional Notes
This intentionally does NOT enable IPv6 FNN rendering by itself.
lo-ip-v6is optional, so sites without it keep their IPv4-only behavior; #2390 is the step that plumbs the persisted value into the underlay.Deploy this change before configuring
lo-ip-v6or rolling out #2390, so every API pod knows how to preserve the field before it can be written.The grow-time backfill sees committed DPU rows. A creation transaction that crosses the first
lo-ip-v6grow can land just after that scan; replaying the additive grow or restarting the API reruns the idempotent pass.AdminGrowResourcePoolcommits the additive pool change before it starts per-DPU reconciliation. If reconciliation reports exhaustion or a persistent compare-and-swap conflict, the grow itself is already durable and the request can be replayed safely after correcting the pool or contention.Startup uses the same per-DPU pass but logs a failure and continues, so one exhausted or busy DPU does not take the API offline while completed reservations remain persisted.
Closes #2389