nat/allocator: extract persistent-lease reuse from allocate_translation (#4409 increment 1)#4643
Merged
Merged
Conversation
Increment 1 of #4409 (Rust NAT modules — nat/allocator.rs PortAllocator god-struct / god-functions). Pure code-motion: the 57-line #2397 persistent-NAT lease-reuse block is lifted out of the allocate_translation god-function into a new private helper. allocate_translation opened by handling the "a persistent lease already exists for this source key" case inline: reuse a still-valid lease (return its translated tuple), or tear down an expired one and fall through to a fresh allocation. That block mixed lease-state mutation, the expiry-index bookkeeping, the live-flow insert, and an early Ok-return into the middle of the already-long allocator, obscuring the fresh-allocation path that follows. Move it verbatim into fn reuse_existing_lease_locked(&self, live, key, flow, persistent_nat_timeout_ns, now_ns) -> Option<TranslatedTuple> Some(tuple) means a lease was reused (the caller returns Ok(tuple)); None means no lease existed or an expired one was reclaimed and the caller should proceed to a fresh allocation. The inner `return Ok(translated)` maps to the helper's Some; the call site is now a four-line `if let Some(t) = self.reuse_existing_lease_locked(...) { return Ok(t); }`. This is behaviour-preserving. Mutation order and side effects are identical (reuses_total bump, live_by_flow insert, lease-expiration removal, release_translated_locked, persistent_by_source.remove), and the lock scope is unchanged: allocate_translation still holds its Mutex<PortAllocatorLiveState> guard across the whole call and passes it to the helper by &mut, so nothing executes outside the existing critical section. The only textual change beyond the move is `&mut live` -> `live` at the internal helper calls, because inside the helper `live` is already `&mut PortAllocatorLiveState`. This is NOT the #2852 Phase-1 lock-free contention refactor — that is a behaviour change and is tracked separately (needs full triple-review). This increment is a plain extract-function to shrink the god-function; further increments (separating the hot bitmap/allocation state from the cold config/stats/GC state) and the lock-free change remain open on #4409. Validation: full cargo test 3830/0 (nat:: subset 205/0), build clean, rustfmt-clean on the changed lines. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
Contributor
There was a problem hiding this comment.
Pull request overview
This PR is a small refactor in the Rust userspace dataplane NAT allocator: it extracts the persistent-NAT “reuse an existing lease if still valid” block out of PortAllocator::allocate_translation into a private helper, reducing the size/complexity of the allocation function while keeping behavior and lock scope the same.
Changes:
- Extracted persistent-lease reuse logic into
reuse_existing_lease_locked(...) -> Option<TranslatedTuple>. - Replaced the inline reuse block in
allocate_translationwith a shortif let Some(...) { return Ok(...) }call. - Logged the refactor increment in
_Log.md.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| userspace-dp/src/nat/allocator.rs | Extracts the persistent-lease reuse block into a new private helper and updates the call site. |
| _Log.md | Adds an entry documenting the refactor increment and touched files. |
Comment on lines
+561
to
+563
| if !live.persistent_by_source.contains_key(&key) { | ||
| return None; | ||
| } |
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
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.
Addresses #4409 (increment 1: extract
reuse_existing_lease_lockedfrom theallocate_translationgod-function; further increments / the Phase-1 lock-free change tracked).What
Pure code-motion. The 57-line #2397 persistent-NAT lease-reuse block is lifted out of
PortAllocator::allocate_translation(userspace-dp/src/nat/allocator.rs) into a new private helper:Some(tuple)= a still-valid lease was reused (caller returnsOk(tuple));None= no lease existed, or an expired one was reclaimed and the caller proceeds to a fresh allocation. The innerreturn Ok(translated)maps to the helper'sSome; the call site is now a four-lineif let Some(t) = self.reuse_existing_lease_locked(...) { return Ok(t); }.Why this is pure code-motion (behaviour + lock scope identical)
reuses_totalbump,live_by_flowinsert, lease-expiration index removal,release_translated_locked,persistent_by_source.remove— all in the same order.allocate_translationstill holds itsMutex<PortAllocatorLiveState>guard across the whole call and passes it to the helper by&mut. Nothing runs outside the existing critical section. The only textual change beyond the move is&mut live->liveat the internal helper calls (inside the helperliveis already&mut PortAllocatorLiveState).Remaining on #4409
Validation
cargo test: 3830 passed / 0 failed (2 pre-existing ignored). NAT subset (nat::): 205 / 0.cargo buildclean (warnings all pre-existing).use-ordering at line 42, untouched — repo is not fmt-clean under local rustfmt).🤖 Generated with Claude Code
https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi