[management] implement separate method for getaccount without users #4963
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaced the full-account backpressure path with a new "light" account path: added Store.GetAccountLight and SqlStore implementations, extended RequestBuffer with GetAccountLightWithBackpressure and batched processing for light requests, and updated network_map controller call sites to use the light backpressure method. Changes
Sequence Diagram(s)sequenceDiagram
participant Controller
participant RequestBuffer
participant BatchProcessor
participant Store
Controller->>RequestBuffer: GetAccountLightWithBackpressure(ctx, accountID)
RequestBuffer->>RequestBuffer: enqueue request (per-account map)
Note right of RequestBuffer: requests buffered until batch trigger
BatchProcessor->>RequestBuffer: collect batched requests for accountID
BatchProcessor->>Store: GetAccountLight(ctx, accountID)
Store-->>BatchProcessor: Account (light)
BatchProcessor->>RequestBuffer: broadcast account to queued requesters
RequestBuffer-->>Controller: return Account / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
management/server/account_request_buffer.go (1)
27-35: Consider refactoring to reduce code duplication.The struct now has parallel fields for both paths (
getAccountRequests/getAccountWithoutUsersRequests,getAccountRequestCh/getAccountWithoutUsersRequestCh). The processing logic inprocessGetAccountWithoutUsersBatchandprocessGetAccountWithoutUsersRequestsis nearly identical to the existing methods.Consider extracting the common batching logic into a generic helper that accepts a fetch function parameter. This would reduce maintenance burden and ensure both paths stay in sync.
Example approach:
type fetchFunc func(ctx context.Context, accountID string) (*types.Account, error) func (ac *AccountRequestBuffer) processBatch(ctx context.Context, accountID string, requests []*AccountRequest, fetch fetchFunc, logPrefix string) { // shared batch processing logic }management/server/store/sql_store.go (1)
907-993: LGTM but consider refactoring to reduce duplication.The
getAccountWithoutUsersGormimplementation correctly:
- Omits
UsersGandUsersG.PATsGfrom preloads- Initializes an empty
Usersmap (line 956) to prevent nil pointer issues- Processes all other account data identically to
getAccountGormThe substantial code duplication (~85 lines) between
getAccountGormandgetAccountWithoutUsersGormis a maintenance concern. Consider extracting common post-processing logic.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
management/internals/controllers/network_map/controller/controller.go(7 hunks)management/server/account/request_buffer.go(1 hunks)management/server/account_request_buffer.go(4 hunks)management/server/store/sql_store.go(3 hunks)management/server/store/store.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-28T12:20:47.254Z
Learnt from: bcmmbaga
Repo: netbirdio/netbird PR: 4849
File: management/internals/modules/zones/manager/manager.go:55-86
Timestamp: 2025-11-28T12:20:47.254Z
Learning: In the NetBird management server, DNS zones without records are automatically filtered out in network map generation (filterPeerAppliedZones in management/internals/controllers/network_map/controller/controller.go checks `len(zone.Records) == 0`). Therefore, CreateZone operations don't need to call UpdateAccountPeers since empty zones don't affect the network map.
Applied to files:
management/internals/controllers/network_map/controller/controller.go
🧬 Code graph analysis (3)
management/server/store/store.go (1)
management/server/types/account.go (1)
Account(71-106)
management/server/store/sql_store.go (6)
management/server/types/account.go (1)
Account(71-106)management/server/types/setupkey.go (1)
SetupKey(33-59)route/route.go (2)
ID(48-48)Route(91-112)management/server/types/group.go (2)
Group(15-36)GroupPeer(38-42)management/server/types/resource.go (1)
Resource(16-19)management/server/types/policyrule.go (1)
PolicyRule(41-83)
management/server/account_request_buffer.go (1)
management/server/store/store.go (1)
Store(50-208)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Management / Benchmark (API) (amd64, postgres)
- GitHub Check: Management / Integration (amd64, postgres)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Client / Unit (386)
- GitHub Check: Management / Integration (amd64, sqlite)
- GitHub Check: Management / Benchmark (amd64, sqlite)
- GitHub Check: Management / Benchmark (API) (amd64, sqlite)
- GitHub Check: Signal / Unit (386)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Management / Benchmark (amd64, postgres)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: CodeQL analysis (go)
- GitHub Check: Agent
- GitHub Check: Client / Unit
- GitHub Check: Android / Build
- GitHub Check: release
- GitHub Check: release_ui_darwin
- GitHub Check: release_ui
- GitHub Check: Windows
- GitHub Check: iOS / Build
- GitHub Check: Darwin
- GitHub Check: Client / Unit
- GitHub Check: Linux
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
🔇 Additional comments (9)
management/server/store/store.go (1)
54-54: LGTM - Interface addition is consistent with existing patterns.The new
GetAccountWithoutUsersmethod is properly added to theStoreinterface, placed logically next toGetAccount, and follows the same signature pattern.management/server/account_request_buffer.go (2)
78-91: LGTM - New backpressure method follows established pattern.The
GetAccountWithoutUsersWithBackpressureimplementation correctly mirrors the existingGetAccountWithBackpressuremethod, using a separate channel and request map. The trace logging includes appropriate context to distinguish between the two paths.
133-171: LGTM - Batch processing implementation is correct.Both
processGetAccountWithoutUsersBatchandprocessGetAccountWithoutUsersRequestscorrectly implement the batching pattern:
- Proper mutex usage around map operations
- Store call happens outside the lock
- Results are broadcast to all waiting requesters
- Channels are properly closed after sending results
management/server/store/sql_store.go (2)
799-804: LGTM - Dispatcher correctly routes to appropriate implementation.The
GetAccountWithoutUsersmethod properly checks for pool availability and dispatches to either the pgx or GORM implementation, matching the pattern used byGetAccount.
1278-1516: LGTM - Pgx implementation correctly excludes user data.The
getAccountWithoutUsersPgximplementation properly:
- Removes the
getUsersgoroutine (first phase: 11 goroutines vs 12)- Removes the
getPersonalAccessTokensgoroutine (second phase: 2 goroutines vs 3)- Skips PAT-to-user association logic
- Initializes empty
Usersmap (line 1477)Error channel capacities correctly match goroutine counts (11 and 2 respectively).
management/internals/controllers/network_map/controller/controller.go (3)
147-147: LGTM - Consistent migration to "without users" variant.The network map controller operations don't require user data, making this optimization appropriate. The account is used for:
- Peer iteration and network map generation
- Group and policy evaluation
- DNS configuration
None of these operations require the
Usersmap.
551-551: LGTM - LoadOrStoreFunc updated consistently.The
LoadOrStoreFunccall correctly passesGetAccountWithoutUsersWithBackpressureas the fetch function for the holder cache initialization. This ensures the cached account also excludes user data, maintaining consistency.
303-303: All remaining call sites consistently updated.The following methods have been migrated to use
GetAccountWithoutUsersWithBackpressure:
UpdateAccountPeer(line 303)GetValidatedPeerWithMap(line 417)RecalculateNetworkMapCache(line 509)OnPeersAdded(line 718)OnPeersDeleted(line 764)All changes are consistent and appropriate for network map operations.
Also applies to: 417-417, 509-509, 718-718, 764-764
management/server/account/request_buffer.go (1)
9-11: No action needed. The interface correctly exposes onlyGetAccountWithoutUsersWithBackpressurefor its callers. Thepeer.gocode uses the concrete type*AccountRequestBuffer, not the interface, so it continues to accessGetAccountWithBackpressuredirectly. This design intentionally limits the interface to the "without users" variant while keeping both methods available in the implementation.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Pull request overview
This PR implements a performance optimization by introducing a new GetAccountWithoutUsers method that retrieves account data without loading user information. This reduces database query overhead for operations that don't require user data, particularly in network map operations.
Key Changes:
- Added
GetAccountWithoutUsersmethod to the store interface with both GORM and PostgreSQL implementations - Created parallel request buffering infrastructure for the new method to handle concurrent requests efficiently
- Updated all network map controller calls to use the new method instead of the full account retrieval
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| management/server/store/store.go | Added GetAccountWithoutUsers method to Store interface |
| management/server/store/sql_store.go | Implemented GetAccountWithoutUsers with both GORM and PostgreSQL backends, excluding user data loading |
| management/server/account/request_buffer.go | Updated RequestBuffer interface to expose GetAccountWithoutUsersWithBackpressure method |
| management/server/account_request_buffer.go | Added buffering infrastructure for GetAccountWithoutUsers requests with separate channel and processing logic |
| management/internals/controllers/network_map/controller/controller.go | Updated all calls to use GetAccountWithoutUsersWithBackpressure instead of GetAccountWithBackpressure |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
management/server/store/sql_store.go (1)
1267-1480: Missing defensive nil-checks in PGX variant (same as GORM).Similar to
getAccountLightGorm, this implementation is missing the same nil-checks forGroups.ResourcesandNameServerGroupsfields. Since both the GORM and PGX code paths should behave identically, apply the same nil-checks here as suggested for the GORM variant.
🧹 Nitpick comments (2)
management/server/store/sql_store.go (1)
908-982: Consider refactoring to reduce code duplication.There's significant duplication between the full account loading methods (
getAccountGorm,getAccountPgx) and their light variants (getAccountLightGorm,getAccountLightPgx). The main differences are:
- Which preloads/queries are executed
- Which maps are initialized vs populated
Consider extracting the common mapping logic into shared helper functions or using a builder pattern that can be configured to load full vs light data. This would reduce maintenance burden and ensure consistency between variants.
Also applies to: 1267-1480
management/server/account_request_buffer.go (1)
134-172: Light request processing duplicates existing pattern correctly.The
processGetAccountLightBatchandprocessGetAccountLightRequestsmethods correctly implement the buffering and batching logic, callingstore.GetAccountLightinstead ofstore.GetAccount. The implementation is consistent with the existing full-account path.The duplication between the full and light request processing paths (lines 94-132 vs 134-172) could be reduced by extracting a generic batch processor that takes the store method as a parameter. This would eliminate ~40 lines of duplicated logic while maintaining the same behavior.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
management/internals/controllers/network_map/controller/controller.go(7 hunks)management/server/account/request_buffer.go(1 hunks)management/server/account_request_buffer.go(4 hunks)management/server/store/sql_store.go(3 hunks)management/server/store/store.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
management/server/store/store.go (1)
management/server/types/account.go (1)
Account(71-106)
management/server/store/sql_store.go (2)
management/server/types/account.go (1)
Account(71-106)shared/management/status/error.go (4)
Error(54-57)Errorf(70-75)NewAccountNotFoundError(95-97)NewGetAccountFromStoreError(144-146)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (26)
- GitHub Check: Management / Integration (amd64, sqlite)
- GitHub Check: Management / Benchmark (amd64, postgres)
- GitHub Check: Management / Integration (amd64, postgres)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Management / Benchmark (API) (amd64, postgres)
- GitHub Check: Signal / Unit (386)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Benchmark (API) (amd64, sqlite)
- GitHub Check: Management / Benchmark (amd64, sqlite)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Client / Unit (386)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Relay / Unit (386)
- GitHub Check: release_ui
- GitHub Check: release
- GitHub Check: release_ui_darwin
- GitHub Check: Client / Unit
- GitHub Check: Darwin
- GitHub Check: Windows
- GitHub Check: Linux
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
🔇 Additional comments (7)
management/server/store/store.go (1)
54-55: Interface addition looks good.The new
GetAccountLightmethod is clearly documented and follows the existing interface patterns. The comment effectively communicates that this returns a lighter account payload excluding users, setup keys, and onboarding data.management/server/store/sql_store.go (2)
799-805: Implementation delegates correctly to light variants.The
GetAccountLightmethod properly routes to the appropriate implementation based on the connection pool availability.
908-982: Missing defensive nil-checks for nested structures.The
getAccountLightGormimplementation correctly omits loading users, setup keys, and onboarding data. However, it's missing nil-checks that are present in the fullgetAccountGormimplementation:
Groups.Resources: The full version checks
if group.Resources == nil(line 878) and initializes to empty slice. The light version should include this check at line 953.NameServerGroups fields: The full version checks and initializes nil slices for
NameServers,Groups, andDomains(lines 892-900). The light version should include similar checks at lines 968-976.These checks prevent potential nil pointer issues when consuming code iterates over these fields.
Apply this pattern from the full implementation:
account.Groups = make(map[string]*types.Group, len(account.GroupsG)) for _, group := range account.GroupsG { group.Peers = make([]string, len(group.GroupPeers)) for i, gp := range group.GroupPeers { group.Peers[i] = gp.PeerID } + if group.Resources == nil { + group.Resources = []types.Resource{} + } account.Groups[group.ID] = group }And for NameServerGroups:
account.NameServerGroups = make(map[string]*nbdns.NameServerGroup, len(account.NameServerGroupsG)) for _, ns := range account.NameServerGroupsG { ns.AccountID = "" + if ns.NameServers == nil { + ns.NameServers = []nbdns.NameServer{} + } + if ns.Groups == nil { + ns.Groups = []string{} + } + if ns.Domains == nil { + ns.Domains = []string{} + } account.NameServerGroups[ns.ID] = &ns }Likely an incorrect or invalid review comment.
management/server/account_request_buffer.go (2)
28-35: Light request tracking fields initialized correctly.The new fields for managing light account requests (
getAccountLightRequests,getAccountLightRequestCh) are properly initialized and the processing goroutine is started. The parallel structure mirrors the existing full-account request path.Also applies to: 49-62
78-92: GetAccountLightWithBackpressure implementation follows established pattern.The new method correctly implements the request buffering pattern, matching the behavior of
GetAccountWithBackpressure. It properly:
- Creates a request with result channel
- Sends to the dedicated light request channel
- Waits for and returns the batched result
management/internals/controllers/network_map/controller/controller.go (1)
147-147: Consistent migration to light account API across all call sites.All usages of
GetAccountWithBackpressurehave been consistently updated toGetAccountLightWithBackpressureacross seven functions. Verification confirms these code paths only access fields available in light accounts (Peers, Groups, Policies, Settings, Id) and do not require user data or setup keys.management/server/account/request_buffer.go (1)
9-12: No action needed. The interface removal ofGetAccountWithBackpressuredoes not constitute a breaking change. The method remains in theAccountRequestBufferimplementation (line 63), and existing callers inmanagement/server/peer.go(lines 82, 1042) use the concrete type*AccountRequestBuffer, not theRequestBufferinterface. These callers will continue to function correctly.Likely an incorrect or invalid review comment.
Describe your changes
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
small performance improvement
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.