feat(api): persist and reconcile SitePrefixes in Core#4031
Conversation
WalkthroughAdds typed site-prefix models, PostgreSQL persistence with tenant overlap constraints and configured-prefix reconciliation, protobuf and RPC conversion support, Forge lookup endpoints with RBAC, startup reconciliation, and model, database, and API tests. ChangesSite prefix inventory
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ForgeAPI
participant SitePrefixHandler
participant SitePrefixDB
Client->>ForgeAPI: Submit site-prefix lookup request
ForgeAPI->>SitePrefixHandler: Dispatch lookup RPC
SitePrefixHandler->>SitePrefixDB: Query IDs or records
SitePrefixDB-->>SitePrefixHandler: Return matching data
SitePrefixHandler-->>Client: Return RPC response
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
🔐 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-23 20:02:06 UTC | Commit: 1165819 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/api-db/src/site_prefix.rs (1)
223-238: 🚀 Performance & Scalability | 🔵 TrivialOptional: containment lookups are unindexed. The
prefix >>= $1/prefix <<= $1predicates cannot exploit the tenant-scoped partial GiST exclusion index, so these filters will fall back to a sequential scan acrosssite_prefixes. This is inconsequential at the expected site-prefix cardinality, but should inventory growth ever materialise, consider a dedicated GiST index onprefix(e.g.USING gist (prefix inet_ops)) to keep containment queries index-backed.🤖 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-db/src/site_prefix.rs` around lines 223 - 238, The PrefixMatch::Contains and PrefixMatch::ContainedBy query predicates lack a supporting GiST index. Add a dedicated GiST index on the site_prefixes prefix column using the inet_ops operator class, while leaving the PrefixMatch::Exact query and existing predicate behavior unchanged.
🤖 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-db/src/site_prefix.rs`:
- Around line 223-238: The PrefixMatch::Contains and PrefixMatch::ContainedBy
query predicates lack a supporting GiST index. Add a dedicated GiST index on the
site_prefixes prefix column using the inet_ops operator class, while leaving the
PrefixMatch::Exact query and existing predicate behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a48b2d4c-bac9-400e-8fa5-1d0a37f57065
📒 Files selected for processing (19)
crates/api-core/src/api.rscrates/api-core/src/auth/internal_rbac_rules.rscrates/api-core/src/handlers/mod.rscrates/api-core/src/handlers/site_prefix.rscrates/api-core/src/setup.rscrates/api-core/src/tests/mod.rscrates/api-core/src/tests/site_prefix.rscrates/api-db/migrations/20260723120000_site_prefixes.sqlcrates/api-db/src/lib.rscrates/api-db/src/site_prefix.rscrates/api-model/src/lib.rscrates/api-model/src/site_prefix.rscrates/rpc/build.rscrates/rpc/proto/common.protocrates/rpc/proto/forge.protocrates/rpc/src/model/mod.rscrates/rpc/src/model/site_prefix.rscrates/uuid/src/lib.rscrates/uuid/src/site_prefix/mod.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/api-db/migrations/20260723120000_site_prefixes.sql (1)
76-91: 🚀 Performance & Scalability | 🔵 TrivialContainment lookups over configured prefixes are unindexed.
The GiST index backing
site_prefixes_tenant_prefix_exclis partial (WHERE authority = 'tenant_managed'), andsite_prefixes_configured_prefix_keyis a B-tree that only accelerates exact=matches. Consequently,find_idsqueries using the>>=/<<=containment operators against configured (or authority-unfiltered) rows will fall back to sequential scans.This is acceptable while the configured set remains a small collection of site-fabric roots, but should the inventory grow, consider a dedicated GiST index on
prefix(or a non-partial one) to keep containment lookups index-backed.🤖 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-db/migrations/20260723120000_site_prefixes.sql` around lines 76 - 91, Review the site_prefixes indexes and add a GiST index on prefix that covers configured and authority-unfiltered containment queries using >>= and <<=, rather than relying on the partial tenant-managed exclusion index or exact-match B-tree. Preserve the existing uniqueness and tenant-managed lookup indexes.
🤖 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-db/migrations/20260723120000_site_prefixes.sql`:
- Around line 76-91: Review the site_prefixes indexes and add a GiST index on
prefix that covers configured and authority-unfiltered containment queries using
>>= and <<=, rather than relying on the partial tenant-managed exclusion index
or exact-match B-tree. Preserve the existing uniqueness and tenant-managed
lookup indexes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 44fdec9d-3861-42da-866d-6ec57ca3f5cb
📒 Files selected for processing (19)
crates/api-core/src/api.rscrates/api-core/src/auth/internal_rbac_rules.rscrates/api-core/src/handlers/mod.rscrates/api-core/src/handlers/site_prefix.rscrates/api-core/src/setup.rscrates/api-core/src/tests/mod.rscrates/api-core/src/tests/site_prefix.rscrates/api-db/migrations/20260723120000_site_prefixes.sqlcrates/api-db/src/lib.rscrates/api-db/src/site_prefix.rscrates/api-model/src/lib.rscrates/api-model/src/site_prefix.rscrates/rpc/build.rscrates/rpc/proto/common.protocrates/rpc/proto/forge.protocrates/rpc/src/model/mod.rscrates/rpc/src/model/site_prefix.rscrates/uuid/src/lib.rscrates/uuid/src/site_prefix/mod.rs
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 9 minutes. |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4031.docs.buildwithfern.com/infra-controller |
`site_fabric_prefixes` currently reaches Core as an in-memory config list. That is enough for VpcPrefix admission and DPU rendering, but it does not give each configured root a stable resource ID or lifecycle record for the tenant-managed layers to reference. This adds a Core `SitePrefix` resource with explicit `Configured` and `TenantManaged` authorities. Startup reconciliation canonicalizes configured CIDRs, keeps IDs stable across unchanged restarts, marks removed roots `Deleting`, and reactivates the same row if a CIDR returns. A transaction advisory lock makes concurrent Core startup converge on one row, while source-scoped reconciliation leaves tenant rows alone. Tenant-owned rows use a composite GiST exclusion to reject overlap within one tenant while allowing equal address space across tenants. The migration installs and verifies `btree_gist`, and the read-only RPCs expose owner, authority, routing scope, lifecycle, metadata, versions, and timestamps without opening the tenant mutation path yet. This is additive: VpcPrefix admission and DPU rendering still use the existing in-memory configuration. Tests cover canonicalization, reconciliation no-ops, removal/reactivation, source isolation, concurrent startup and tenant inserts, cross-tenant reuse, RPC conversion, filtering, and request guards. This supports NVIDIA#3884 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
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 `@rest-api/proto/core/src/v1/nico_nico.proto`:
- Around line 9451-9474: Add the repository-standard pagination request fields
to SitePrefixSearchFilter and a continuation/next-page field to
SitePrefixIdList, matching the established FindResourceNameIds and
FindResourceNamesByIds API pattern. Update the SitePrefix lookup contract so
FindSitePrefixIds returns bounded ID pages while FindSitePrefixesByIds remains
responsible for retrieving resource state.
🪄 Autofix (Beta)
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: 8fac0dec-341c-47a7-a1a5-8a6ec92e224c
⛔ Files ignored due to path filters (3)
rest-api/proto/core/gen/v1/common_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.gorest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.gorest-api/proto/core/gen/v1/nico_nico_grpc.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go,!rest-api/**/*_grpc.pb.go
📒 Files selected for processing (21)
crates/api-core/src/api.rscrates/api-core/src/auth/internal_rbac_rules.rscrates/api-core/src/handlers/mod.rscrates/api-core/src/handlers/site_prefix.rscrates/api-core/src/setup.rscrates/api-core/src/tests/mod.rscrates/api-core/src/tests/site_prefix.rscrates/api-db/migrations/20260723120000_site_prefixes.sqlcrates/api-db/src/lib.rscrates/api-db/src/site_prefix.rscrates/api-model/src/lib.rscrates/api-model/src/site_prefix.rscrates/rpc/build.rscrates/rpc/proto/common.protocrates/rpc/proto/forge.protocrates/rpc/src/model/mod.rscrates/rpc/src/model/site_prefix.rscrates/uuid/src/lib.rscrates/uuid/src/site_prefix/mod.rsrest-api/proto/core/src/v1/common_nico.protorest-api/proto/core/src/v1/nico_nico.proto
🚧 Files skipped from review as they are similar to previous changes (19)
- crates/uuid/src/lib.rs
- crates/api-core/src/handlers/mod.rs
- crates/api-model/src/lib.rs
- crates/api-core/src/handlers/site_prefix.rs
- crates/rpc/proto/common.proto
- crates/api-db/src/lib.rs
- crates/api-core/src/tests/mod.rs
- crates/api-core/src/auth/internal_rbac_rules.rs
- crates/api-core/src/api.rs
- crates/rpc/src/model/mod.rs
- crates/rpc/build.rs
- crates/uuid/src/site_prefix/mod.rs
- crates/rpc/proto/forge.proto
- crates/api-core/src/setup.rs
- crates/api-core/src/tests/site_prefix.rs
- crates/api-db/migrations/20260723120000_site_prefixes.sql
- crates/api-model/src/site_prefix.rs
- crates/rpc/src/model/site_prefix.rs
- crates/api-db/src/site_prefix.rs
1 participant
As part of supporting tenant-provided site prefixes (#3883),
site_fabric_prefixescurrently reaches Core as an in-memory config list. That is enough for VpcPrefix admission and DPU rendering, but it does not give each configured root a stable resource ID or lifecycle record for the tenant-managed layers to reference.This adds a Core
SitePrefixresource with explicitConfiguredandTenantManagedauthorities. Startup reconciliation canonicalizes configured CIDRs, keeps IDs stable across unchanged restarts, marks removed rootsDeleting, and reactivates the same row if a CIDR returns. A transaction advisory lock makes concurrent Core startup converge on one row, while source-scoped reconciliation leaves tenant rows alone.Tenant-owned rows use a composite GiST exclusion to reject overlap within one tenant while allowing equal address space across tenants. The migration installs and verifies
btree_gist, and the read-only RPCs expose owner, authority, routing scope, lifecycle, metadata, versions, and timestamps without opening the tenant mutation path yet.This is additive: VpcPrefix admission and DPU rendering still use the existing in-memory configuration.
Related issues
This supports #3884
Type of Change
Breaking Changes
Testing
Unit tests added/updated
Integration tests added/updated
Manual testing performed
No testing required (docs, internal refactor, etc.)
cargo test -p carbide-api-model -p carbide-uuid site_prefix --libcargo test -p carbide-rpc site_prefix --features modelcargo test -p carbide-api-db site_prefix --lib -- --nocapture --test-threads=1cargo test -p carbide-api-core site_prefix --lib -- --nocapture --test-threads=1cargo make check-format-nightlycargo make clippycargo make carbide-lintsAdditional Notes
The migration requires
btree_gistand fails before changing the SitePrefix schema when the extension is unavailable or installed outside thepublicschema.Tenant mutation APIs, VpcPrefix parent linkage, DPU-acknowledged readiness, REST projection, tenant authorization, and overlap-constraint changes remain separate follow-up layers.