fix(api-db,rpc): report ipv6 free ip counts beyond u32#3750
Merged
Conversation
Contributor
WalkthroughFree-IP accounting now uses exact per-prefix wide arithmetic, optional ChangesFree-IP accounting
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SegmentQuery
participant IpAllocator
participant NetworkPrefixModel
participant RpcEncoder
participant AdminCLI
SegmentQuery->>IpAllocator: calculate num_free(prefix.id)
IpAllocator-->>SegmentQuery: return exact u128 count
SegmentQuery->>NetworkPrefixModel: store optional num_free_ips
NetworkPrefixModel->>RpcEncoder: encode free-IP count
RpcEncoder-->>AdminCLI: return legacy, v2, and saturation fields
AdminCLI->>AdminCLI: format free-IP count for display
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-07-20 23:54:28 UTC | Commit: a705e8d |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/network-segment-controller/src/handler.rs (1)
196-200: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the conversion test table-driven.
This is a total input-to-output mapping; use
check_valuesorvalue_scenarios!rather than individual assertions.Proposed test structure
#[cfg(test)] mod tests { + use carbide_test_support::{Check, check_values}; + use super::available_ip_metric_value; #[test] fn available_ip_metric_preserves_or_saturates_counts() { - assert_eq!(available_ip_metric_value(None), 0); - assert_eq!(available_ip_metric_value(Some(42)), 42); - assert_eq!(available_ip_metric_value(Some(u128::MAX)), usize::MAX); + struct Row { + count: Option<u128>, + } + + check_values( + [ + Check { + scenario: "omitted count", + input: Row { count: None }, + expect: 0, + }, + Check { + scenario: "representable count", + input: Row { count: Some(42) }, + expect: 42, + }, + Check { + scenario: "overflowing count", + input: Row { + count: Some(u128::MAX), + }, + expect: usize::MAX, + }, + ], + |row| available_ip_metric_value(row.count), + ); } }As per coding guidelines, “Use table-driven tests for functions mapping inputs to outputs”; as per path instructions, new mapping logic must have table-driven coverage.
🤖 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/network-segment-controller/src/handler.rs` around lines 196 - 200, Convert available_ip_metric_preserves_or_saturates_counts into a table-driven test using the existing check_values or value_scenarios! helper. Represent the None, ordinary value, and u128::MAX-to-usize::MAX mappings as test cases, then run them through the helper instead of separate assert_eq! statements.Sources: Coding guidelines, Path instructions
🤖 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/network-segment-controller/src/handler.rs`:
- Around line 196-200: Convert available_ip_metric_preserves_or_saturates_counts
into a table-driven test using the existing check_values or value_scenarios!
helper. Represent the None, ordinary value, and u128::MAX-to-usize::MAX mappings
as test cases, then run them through the helper instead of separate assert_eq!
statements.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8f9db13d-a260-4a79-b672-7c9cba8f3575
⛔ Files ignored due to path filters (1)
rest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (20)
crates/admin-cli/src/network_segment/create/args.rscrates/admin-cli/src/network_segment/show/cmd.rscrates/admin-cli/src/rpc.rscrates/api-core/src/test_support/network_segment.rscrates/api-core/src/tests/common/network_segment.rscrates/api-core/src/tests/machine_dhcp.rscrates/api-core/src/tests/network_segment.rscrates/api-core/src/tests/network_segment_find.rscrates/api-core/src/tests/network_segment_lifecycle.rscrates/api-db/src/ip_allocator.rscrates/api-db/src/network_segment.rscrates/api-model/src/network_prefix.rscrates/api-model/src/network_segment/mod.rscrates/machine-a-tron/src/api_client.rscrates/network-segment-controller/src/handler.rscrates/rpc/proto/forge.protocrates/rpc/src/model/network_prefix.rscrates/rpc/src/model/network_segment.rscrates/test-harness/src/network/controller.rsrest-api/proto/core/src/v1/nico_nico.proto
chet
changed the title
`get_network_size` saturated every address space larger than `u32`, and network-segment queries populated only the first prefix. Allocation still worked, but IPv6 and dual-stack capacity reporting was wrong. Keep per-prefix accounting exact as `u128`, with an explicit `FullIpv6Space` case for `2^128`. Preserve omitted versus exact-zero counts, retain protobuf tag 8 for old clients, and expose the wider value plus saturation state to new clients and the CLI. The existing first-prefix metrics model remains a separate dual-stack design question. This supports NVIDIA#2243 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
kensimon
approved these changes
Jul 21, 2026
| // not requested; clients also see it absent in responses from older servers. | ||
| // When `free_ip_count_saturated` is true, this is `2^64 - 1` and the exact | ||
| // count is larger. | ||
| optional uint64 free_ip_count_v2 = 10; |
Contributor
There was a problem hiding this comment.
"Show me what backwards compatibility looks like"
"This is what backwards compatibility looks like"
✊
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
2 participants
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.
An IPv6
/64contains2^64addresses, butget_network_sizereturned au32and saturated anything larger atu32::MAX. Allocation still worked; the free count reported to operators and clients did not. The database path also populated only the first prefix, so a dual-stack segment could report one real count and omit the other.This change keeps free-IP accounting exact and per-prefix until it reaches a boundary that cannot represent the value.
AddressCountcarries either a normalu128count orFullIpv6Spacefor2^128,IpAllocator::num_freecalculates each prefix independently, andOption<u128>keeps skipped accounting distinct from an exact zero.Wire compatibility is preserved rather than requiring existing clients to move in lockstep. Protobuf tag 8 remains the legacy capped
uint32; new clients can readfree_ip_count_v2andfree_ip_count_saturated. The admin CLI prefers the wider value and printsEffectively unlimitedwhen the count exceedsu64, while the existingusizemetrics boundary saturates instead of wrapping.Related issues
This supports #2243
Type of Change
Breaking Changes
Testing
Focused coverage includes IPv4
/0and IPv6/0,/63, and/64arithmetic; IPv6-first dual-stack SQL rows; skipped, exact-zero, legacy, v2, and saturated RPC counts; and CLI fallback and display behavior.Local verification passes
cargo make format-nightly,cargo make clippy,cargo make carbide-lints,cargo make check-workspace-deps,cargo make --no-workspace check-rest-core-proto-sync, affected Rust tests, and Core and REST protobuf build and branch-parent compatibility checks.Additional Notes
The protobuf change is additive:
NetworkSegmentMetricsstill reports onlyprefixes[0], so dual-stack values depend on prefix order until the metric schema can emit one series per address family. That separate metrics design is intentionally outside this count fix.Closes #2243