perf(instance-address): persist allocated addresses in one INSERT under the lock#3397
Open
chet wants to merge 1 commit into
Open
perf(instance-address): persist allocated addresses in one INSERT under the lock#3397chet wants to merge 1 commit into
chet wants to merge 1 commit into
Conversation
Contributor
|
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 ignored due to path filters (2)
📒 Files selected for processing (3)
Summary by CodeRabbit
WalkthroughInstance address allocation now stores prefix and hostname data, batches database inserts within bind limits, and checks allocations across both relevant tables. Database tests cover persistence and empty inputs. Segment drain logging now uses the handler’s segment identifier. ChangesInstance address batching
Segment drain logging
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Allocation
participant BatchInsert
participant Database
Allocation->>BatchInsert: Accumulated instance addresses
BatchInsert->>Database: Bounded multi-row INSERT
Database-->>BatchInsert: Persisted rows
BatchInsert-->>Allocation: Insert completion
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/api-db/src/instance_address.rs (1)
456-508: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPersist overlay allocations before the next interface is processed
When two interfaces in the same request target the same overlay segment, the second
IpAllocatoris created before the first allocation has been written toinstance_addresses, so it can hand out a duplicate IP. The exclusive lock does not prevent that; the resolver only sees rows already persisted. Either write each overlay row before continuing, or seed the next resolver with the addresses accumulated so far.🤖 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/instance_address.rs` around lines 456 - 508, Ensure allocations for multiple interfaces targeting the same overlay segment account for addresses assigned earlier in the request. Update the allocation flow around IpAllocator and UsedOverlayNetworkIpResolver so each subsequent resolver includes accumulated rows from the current transaction, or persist each overlay allocation before processing the next interface; retain the final batch insert behavior where possible and prevent duplicate IP assignment.
🤖 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.
Outside diff comments:
In `@crates/api-db/src/instance_address.rs`:
- Around line 456-508: Ensure allocations for multiple interfaces targeting the
same overlay segment account for addresses assigned earlier in the request.
Update the allocation flow around IpAllocator and UsedOverlayNetworkIpResolver
so each subsequent resolver includes accumulated rows from the current
transaction, or persist each overlay allocation before processing the next
interface; retain the final batch insert behavior where possible and prevent
duplicate IP assignment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: edae9a07-a83e-400a-a9a8-b24b10005f8c
📒 Files selected for processing (2)
crates/api-db/src/instance_address.rscrates/network-segment-controller/src/handler.rs
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
…er the lock Instance-address allocation holds an `ACCESS EXCLUSIVE` lock on `instance_addresses` while it writes the newly assigned addresses. It inserted them one row at a time -- up to ~18 sequential INSERTs for a host with nine interfaces and dual-stack -- so the lock stayed held across every round-trip and serialized concurrent allocations for that whole span. `allocate` now gathers every interface's addresses and writes them with a single multi-row INSERT, so the write side of the lock window is one statement regardless of address count. The write also uses `execute()` rather than `fetch_all()` (the INSERT returns nothing). - Add `insert_instance_addresses`: one `push_values` INSERT for the set. - Rows are `api-model`'s `InstanceAddress` (its `prefix` slot was already reserved there), now shared by the table's readers and this batched write. - Gather rows across all interfaces, write once after the loops. - A query-count test guards it: N addresses persist in one statement. - Restore the NVIDIA#3387 review fixes its merge predated: the drain predicate's second-`EXISTS`-arm regression test, the redundant `::uuid` cast removal, and the drain log's `segment_id` key. Column set, types, and hostname derivation are unchanged. This supports NVIDIA#3215 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
✅ Action performedFull review finished. |
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.
Instance-address allocation holds an
ACCESS EXCLUSIVElock oninstance_addresseswhile it writes the newly assigned addresses -- one INSERT at a time, up to ~18 sequential statements for a dual-stack host with nine interfaces, serializing concurrent allocations across every round-trip.allocatenow gathers every interface's addresses and writes them with a single multi-rowpush_valuesINSERT (chunked well above any real allocation size), so the write side of the lock window is one statement regardless of address count:insert_instance_addresses-- one INSERT for the whole set,execute()instead offetch_all()(the INSERT returns nothing). Rows areapi-model'sInstanceAddress(itsprefixslot was already reserved there), now shared by the table's readers and this write.EXISTS-arm regression test, the redundant::uuidcast removal, and the drain log'ssegment_idkey.This supports #3215