[client, management] Support per-peer lazy connection state and default proxy peers to lazy#6762
[client, management] Support per-peer lazy connection state and default proxy peers to lazy#6762lixmal wants to merge 5 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughManagement now emits per-peer lazy states, while ChangesLazy connection policy
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Management
participant Engine
participant ConnMgr
participant LazyConnectionManager
Management->>Engine: emit RemotePeerConfig with LazyState
Engine->>Engine: compute forwarding rules and permanent peers
Engine->>ConnMgr: AddPeerConn with permanence
ConnMgr->>LazyConnectionManager: route eligible connections
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 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 `@client/internal/engine.go`:
- Line 1782: Update the AddPeerConn call sites in the connection-handling flow
and the corresponding later path to classify permanence from the incoming peer
configuration’s allowed IPs, rather than calling isPermanentPeer against the
not-yet-populated peer store. Ensure forward targets are registered as permanent
when their configured allowed IPs indicate forwarding, while preserving existing
behavior for other peers.
🪄 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: Pro
Run ID: 46ec3772-d86e-4d61-99a8-3163f66be388
⛔ Files ignored due to path filters (1)
shared/management/proto/management.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (6)
client/internal/conn_mgr.goclient/internal/conn_mgr_test.goclient/internal/engine.goclient/internal/engine_lazy_exclude_test.gomanagement/internals/shared/grpc/conversion.goshared/management/proto/management.proto
Release artifactsBuilt for PR head
GHCR images (amd64)
This comment is updated by the Release workflow. Artifact links expire according to the workflow retention policy. |
| // Force a lazy (on-demand) connection regardless of the account flag. | ||
| LazyStateLazy = 1; | ||
| // Force an always-active connection regardless of the account flag. | ||
| LazyStateEager = 2; |
There was a problem hiding this comment.
It's used on the consumer (client), it's not produced by mgmt yet
|
|
||
| e.initLazyManager(ctx) | ||
| e.statusRecorder.UpdateLazyConnection(true) | ||
| e.statusRecorder.UpdateLazyConnection(e.PeerLazyDefault(mgmProto.LazyState_LazyStateDefault)) |
There was a problem hiding this comment.
is this capturing the fact that proxy peers are lazy when account flag is not set?
There was a problem hiding this comment.
Nope, right now it reflects the account wide setting
| return | ||
| } | ||
|
|
||
| e.initLazyManager(ctx) |
There was a problem hiding this comment.
We are starting the lazy manager even when account and local lazy flag say to not use lazy. (majority of customers today?)
By doing so, we are running toExcludedLazyPeers on every sync update potentially on a long slice of RemotePeerConfigs, even though the result will exactly the entire map of all peers.
There was a problem hiding this comment.
I added a check to avoid running the expensive part when nothing has changed
There was a problem hiding this comment.
GetPeerConfig can be nil ( from line 1393 ). If so GetLazyConnectionEnabled is defaulted to false. Is it ok?
|
2 participants
Describe your changes
Lazy connections were an all-or-nothing account setting. This makes laziness controllable per peer and runs the lazy connection manager continuously, so ephemeral proxy peers connect on-demand by default even when the account-wide flag is off.
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
No new user-facing configuration is introduced; the per-peer eager override has no admin-facing setting yet and the proxy-peer default is internal behavior.
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