[proxy, client] Bound embed client WireGuard per-Device memory#5962
Open
[proxy, client] Bound embed client WireGuard per-Device memory#5962
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds runtime WireGuard tuning: new WGTuning APIs in embed/engine, package-level WG defaults, CLI/debug endpoints to get/set tuning and view runtime stats, environment variable overrides on startup, and debug handlers to apply per-client tuning and report runtime/process metrics. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant DebugClient
participant DebugHTTP as Debug HTTP Handler
participant Provider as Proxy/Embed Manager
participant Engine
participant WGDevice as WireGuard Device
CLI->>DebugClient: WGTuneSet(value)
DebugClient->>DebugHTTP: GET /debug/wgtune?value=<v>
DebugHTTP->>Provider: ListClientsForStartup()
loop per client
Provider->>Engine: Engine.SetWGTuning({PreallocatedBuffersPerPool:v})
Engine->>WGDevice: SetPreallocatedBuffersPerPool(v)
WGDevice-->>Engine: ack / error
Engine-->>Provider: result
end
DebugHTTP-->>DebugClient: JSON {default,batch_size,applied,failed}
DebugClient-->>CLI: formatted output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
lixmal
changed the title
lixmal
force-pushed
the
reduce-embed-wg-pool
branch
from
f8e7023 to
d217e35
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
proxy/cmd/proxy/cmd/root.go (1)
159-174: Consider warning when pool cap is below the eager-allocation floor.The PR notes that each bind receive goroutine (~3/device) plus
RoutineReadFromTUNeagerly reservemax_batch_sizebuffers, and a pool cap below this floor will block startup. Operators who set both env vars together (e.g.NB_WG_MAX_BATCH_SIZE=128+NB_WG_PREALLOCATED_BUFFERS=256) will hang with no log clue. A startup warning whenenvWGPreallocatedBuffers > 0 && envWGPreallocatedBuffers < eagerFloor(envWGMaxBatchSize)would make the knob far less foot-gunny.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/cmd/proxy/cmd/root.go` around lines 159 - 174, When parsing envWGPreallocatedBuffers and envWGMaxBatchSize, add a startup warning if preallocatedBuffers > 0 and is less than the eager-allocation floor derived from maxBatchSize so operators aren't blocked silently; compute the floor (the same logic used for eager allocation / RoutineReadFromTUN) from the parsed max batch value and compare it after calling embed.SetWGDefaultMaxBatchSize and embed.SetWGDefaultPreallocatedBuffersPerPool, and use logger.Warnf to emit a clear message referencing envWGPreallocatedBuffers and envWGMaxBatchSize when preallocated < eagerFloor to indicate startup may block and recommend increasing the pool cap.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@proxy/internal/debug/client.go`:
- Around line 286-304: The printer printWGTuneSet currently ignores server-side
errors returned in the response body; update printWGTuneSet to check
data["error"] (e.g., as a string) first and, if present, write that error to
c.out (preferably as "Error: <message>") and return early so the silent "Default
set to: 0" / "Applied to 0" output is not shown; keep printWGTuneGet unchanged
but ensure printWGTuneSet handles both the error case and the existing success
fields (default, applied, failed).
In `@proxy/internal/debug/handler.go`:
- Around line 642-679: In handleWGTune, change the parse-failure branch to
return HTTP 400 and a JSON body that matches sibling handlers (include
"success": false and an "error" string) instead of writing a 200 with only
"error"; after parsing succeeds keep setting
nbembed.SetWGDefaultPreallocatedBuffersPerPool(uint32(n)) and applying to
clients via h.provider.ListClientsForStartup() as before, but ensure the success
response always includes "success": true, "default": uint32(n), "batch_size":
nbembed.WGDefaultMaxBatchSize(), and "applied": applied, and if any client
errors occur include "failed" map; use http.Error or your existing write helper
with w.WriteHeader(http.StatusBadRequest) to set the 400 status on parse error
so the CLI treats it as an error.
---
Nitpick comments:
In `@proxy/cmd/proxy/cmd/root.go`:
- Around line 159-174: When parsing envWGPreallocatedBuffers and
envWGMaxBatchSize, add a startup warning if preallocatedBuffers > 0 and is less
than the eager-allocation floor derived from maxBatchSize so operators aren't
blocked silently; compute the floor (the same logic used for eager allocation /
RoutineReadFromTUN) from the parsed max batch value and compare it after calling
embed.SetWGDefaultMaxBatchSize and embed.SetWGDefaultPreallocatedBuffersPerPool,
and use logger.Warnf to emit a clear message referencing
envWGPreallocatedBuffers and envWGMaxBatchSize when preallocated < eagerFloor to
indicate startup may block and recommend increasing the pool cap.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a8d22a9-98da-490d-9747-7737a0209673
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
client/embed/embed.goclient/internal/engine.gogo.modproxy/cmd/proxy/cmd/debug.goproxy/cmd/proxy/cmd/root.goproxy/internal/debug/client.goproxy/internal/debug/handler.go
lixmal
force-pushed
the
reduce-embed-wg-pool
branch
from
d217e35 to
8b4ec36
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@proxy/internal/debug/handler.go`:
- Around line 642-655: The handler handleWGTune currently treats missing and
explicitly empty query parameters identically because it only uses
r.URL.Query().Get("value"); change the logic to first check presence with
r.URL.Query()["value"] (or lookup the map) to distinguish missing vs
present-empty: if the "value" key is absent keep the existing default-response
path, but if the key is present and the string is empty return
http.StatusBadRequest with an error about an empty value; otherwise parse the
non-empty value as before using strconv.ParseUint. Ensure you update only
handleWGTune to follow the same explicit-empty validation pattern as ping and
setloglevel.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8e59ad59-8860-4a2f-bdd3-5128d9971130
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
client/embed/embed.goclient/internal/engine.gogo.modproxy/cmd/proxy/cmd/debug.goproxy/cmd/proxy/cmd/root.goproxy/internal/debug/client.goproxy/internal/debug/handler.go
✅ Files skipped from review due to trivial changes (1)
- go.mod
🚧 Files skipped from review as they are similar to previous changes (3)
- proxy/cmd/proxy/cmd/root.go
- client/internal/engine.go
- client/embed/embed.go
lixmal
force-pushed
the
reduce-embed-wg-pool
branch
from
8b4ec36 to
e81ce81
Compare
|
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.
Describe your changes
Bounds memory used by per-account WireGuard Devices in the reverse proxy by exposing the wireguard-go buffer-pool and batch-size knobs through the embed client and new debug surfaces.
client/embedso the proxy does not reach into wireguard-go internals:SetWGDefaultPreallocatedBuffersPerPool,SetWGDefaultMaxBatchSize, and live-tune viaClient.SetWGTuning(WGTuning{...}).NB_WG_PREALLOCATED_BUFFERS— per-Device pool cap. 0 = unbounded (unchanged default).NB_WG_MAX_BATCH_SIZE— per-Device batch size. Controls how many buffers each receive/TUN goroutine eagerly allocates for its lifetime. 0 = use bind/tun default./debug/wgtune(GET shows defaults; GET with?value=sets the pool cap globally and on all live clients) plusnetbird-proxy debug wgtune get|set <n>. Batch size is startup-only sosetonly touches the pool cap./debug/runtime(MemStats, goroutine count,/proc/self/statusVmRSS/VmSize/VmData, client counts) plusnetbird-proxy debug runtime. Cheap to call, no pprof.Depends on netbirdio/wireguard-go#14 (bumps fork replace in
go.mod).Tuning matrix
Each bind receive goroutine (IPv4 + IPv6 + ICE relay ≈ 3) plus
RoutineReadFromTUNeagerly reservesbatchmessage buffers for its lifetime. Pool caps below this eager floor will block the pipeline at startup.NB_WG_MAX_BATCH_SIZENB_WG_PREALLOCATED_BUFFERSLeaving both at 0 keeps upstream behavior unchanged.
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Operator-only tuning knobs for the internal reverse proxy; no user-facing surface change.
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