perf(http,processtree): pool HTTP readers and eliminate redundant process tree allocations - #936
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup EffectivenessNo data available. |
78b4baa to
d3b43db
Compare
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup EffectivenessNo data available. |
d3b43db to
e56a044
Compare
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup EffectivenessNo data available. |
e56a044 to
66891ec
Compare
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup EffectivenessNo data available. |
66891ec to
c2a4253
Compare
c2a4253 to
c7f9a6f
Compare
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup EffectivenessNo data available. |
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Reviewed the diff in detail, focusing on the riskiest parts of a perf-motivated PR — pooled-object lifetime, cache-key correctness, and the hand-rolled heap. (Note: GitHub won't let me submit a formal APPROVE since this PR's author and this reviewing account are the same — matthyx. No blockers found; treat this as a green light to merge once CI finishes.)
httpparse.go (sync.Pool for bufio.Reader): verified there's no use-after-return-to-pool hazard. ParseHttpRequest/ParseHttpResponse only feed the pooled reader the header bytes and unconditionally overwrite req.Body/resp.Body with a bytes.Reader-backed NopCloser before returning, so the lazy body reader that would alias the pooled bufio.Reader is never actually read. fallbackReadRequest/readResponse go further and eagerly drain the body into bodyBytes before the defer putBufioReader runs, so those paths are safe too.
ordered_event_queue.go: the hand-rolled slice min-heap (up/down/pushHeap/popHeap) matches the standard container/heap sift algorithm, and the new sync.Mutex correctly protects the slice where the previous lane.PriorityQueue presumably did this internally. PopBatch reusing cw.batchBuf is safe since it's only ever touched from the single-goroutine eventProcessingLoop.
process_tree_manager.go: confirmed the removed GetProcessNode existence-check is genuinely redundant — GetPidBranch (in container_processtree.go) does its own fullTree.Load and returns a proper error on a missing PID, it doesn't silently succeed. Also confirmed the two deleted error types (ProcessNotFoundError, GetProcessNodeError) have no remaining references anywhere in the repo (no compile break), and the sole caller of GetContainerProcessTree (EventEnricher.EnrichEvents) already discards the error entirely, so neither that change nor the new empty-containerID guard alters observable behavior.
Cache keys (httpEventKey, treeCacheKey): struct keys are a correctness improvement over the old unseparated string concatenation in GetUniqueIdentifier (which could theoretically collide, e.g. inode 1 + sockFd 23 vs inode 12 + sockFd 3), not just a perf win.
Two non-blocking nits, feel free to take or leave:
GetUniqueIdentifierinhttpparse.gois now dead code (superseded bygetHttpEventKey) — could be removed.dedup-bench.sh's new--set nodeAgent.env[0].name=ENABLE_PROFILER/env[0].value=1targets a fixed array index; if the chart's default values ever populatenodeAgent.env[0]for something else, this would silently clobber it instead of appending. Low risk for benchmark-only tooling.
Benchmark results across the reruns are consistently favorable (roughly -15 to -19% memory, flat-to-negative CPU), and the docs added in docs/features/benchmark-ci-gate.md explain the p95-gating rationale well. LGTM.
c7f9a6f to
9f2af73
Compare
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup EffectivenessNo data available. |
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup EffectivenessNo data available. |
| return append(data, []byte("\r\n\r\n")...) | ||
| } | ||
|
|
||
| func detachBody(body io.ReadCloser) (io.ReadCloser, error) { |
There was a problem hiding this comment.
Correctness (high): detachBody turns a previously-successful fallback parse into a hard error, dropping the whole HTTP event.
Detaching the body is genuinely required now that the bufio.Reader is pooled (otherwise req.Body/resp.Body would read from a recycled reader — pkg/utils/cel.go's body field getter does read req.Body). But propagating the io.ReadAll error is a behavior regression, and it fires on exactly the input this fallback path exists to handle.
Concrete scenario: a captured POST is truncated by the 4 KiB BPF ring buffer, so ParseHttpRequest finds no \r\n\r\n and falls through to fallbackReadRequest. cleanCorrupted + PatchHTTPPacket produce a header-only packet in which Content-Length: 500 survived. http.ReadRequest succeeds (verified locally), then io.ReadAll(req.Body) returns io.ErrUnexpectedEOF because 0 of the 500 declared bytes are present. New code returns an error → CreateEventFromRequest fails → GroupEvents returns nil → the request event is dropped entirely, and because nothing is stored in eventsMap the matching response is dropped too. Before this PR the request was returned with its headers intact and the event was emitted (the CEL body getter prefers GetBuf(), so req.Body was usually never even read).
readResponse (line 274) has the identical problem via fallbackReadResponse, plus truncated chunked framing: io.ReadAll over a cut-off chunk returns the partial bytes and ErrUnexpectedEOF, which the current code discards.
Keeping the partial bytes preserves the old behavior and still fixes the pooled-reader lifetime issue:
| func detachBody(body io.ReadCloser) (io.ReadCloser, error) { | |
| func detachBody(body io.ReadCloser) (io.ReadCloser, error) { | |
| if body == nil { | |
| return nil, nil | |
| } | |
| // Truncated ring-buffer captures routinely yield io.ErrUnexpectedEOF here; | |
| // keep whatever was read rather than dropping the whole event. | |
| bodyBytes, _ := io.ReadAll(body) | |
| _ = body.Close() | |
| return io.NopCloser(bytes.NewReader(bodyBytes)), nil | |
| } |
| return nil, err | ||
| } | ||
| if resp.Body != nil { | ||
| detached, err := detachBody(resp.Body) |
There was a problem hiding this comment.
Correctness (high): same drop-the-event regression on the response side.
readResponse is only reached from fallbackReadResponse, i.e. when the captured response bytes were already truncated/corrupted. If the surviving headers declare a Content-Length (or a Transfer-Encoding: chunked frame is cut mid-chunk), detachBody returns io.ErrUnexpectedEOF and this now returns an error instead of the header-parsed response.
Concrete scenario: HTTP/1.1 200 OK\r\nContent-Length: 500\r\n\r\n with no body bytes captured — http.ReadResponse succeeds, io.ReadAll(resp.Body) fails. ParseHttpResponse → nil, err → GroupEvents returns nil, so the already-stored request never gets paired and is only released later by the orphan-timeout path. Chunked case verified locally: io.ReadAll returns 3 partial bytes and ErrUnexpectedEOF, and those 3 bytes are thrown away.
Fixing detachBody to keep partial bytes (see the comment on line 216) resolves this site too; alternatively ignore the error here and keep the response.
| import pandas as pd | ||
|
|
||
| SIGNIFICANT_THRESHOLD = 10.0 # percent change that triggers quality gate failure | ||
| SIGNIFICANT_THRESHOLD = 5.0 # percent change that triggers quality gate failure |
There was a problem hiding this comment.
Medium: halving SIGNIFICANT_THRESHOLD (10 → 5) tightens the Peak Memory gate, which is still a max-of-samples statistic — the exact bias this PR is fixing for CPU.
.github/workflows/benchmark.yaml invokes compare-metrics.py --check with no --threshold, so this default is live. Net effect of the PR on the gate:
| metric | statistic | before | after |
|---|---|---|---|
| Avg CPU | mean | 10% | 5% |
| Peak CPU | max → p95 | 10% | 10% |
| Avg Memory | mean | 10% | 5% |
| Peak Memory | max | 10% | 5% |
Peak Memory keeps the max() statistic and gets a 2× stricter threshold, so a run whose peak-RSS sample lands 6% high from a GC-timing artifact now fails the gate where it previously passed — reintroducing the flakiness the change sets out to remove. docs/features/benchmark-ci-gate.md says "Avg CPU and Avg/Peak Memory are unaffected … and remain gated on SIGNIFICANT_THRESHOLD (5%)", which reads as if 5% were the status quo; the diff shows it was 10%.
Either keep SIGNIFICANT_THRESHOLD = 10.0 (the p95 switch alone is what resolves #519's cases, per the doc's own verification table), or move Peak Memory onto p95 as well before tightening it.
| oeq.up(len(oeq.eventQueue) - 1) | ||
| } | ||
|
|
||
| func (oeq *OrderedEventQueue) popHeap() EventEntry { |
There was a problem hiding this comment.
Low (memory retention): popHeap truncates the slice but never clears the vacated slot, so the backing array pins every popped event.
oeq.eventQueue = oeq.eventQueue[:n] leaves the popped EventEntry — including its Event utils.K8sEvent reference — live in eventQueue[n], which is still within cap. The heap array only ever grows (orderedEventQueue.size defaults to 100000, pkg/config/config.go:209), so after any burst the array holds up to 100k stale EventEntry values, each keeping a released/pooled eBPF event object reachable until that slot happens to be overwritten by a future push. During an idle period following a burst, none of them are.
Verified with a standalone replica of this heap: after pushing 8 entries and draining, eventQueue[:cap] still contains all 8 popped entries.
Same applies to ContainerWatcher.batchBuf (container_watcher.go:210), which is make([]EventEntry, 0, cfg.EventBatchSize) = 15000 entries and retains the last batch's event references indefinitely after enrichAndProcess returns.
| func (oeq *OrderedEventQueue) popHeap() EventEntry { | |
| func (oeq *OrderedEventQueue) popHeap() EventEntry { | |
| n := len(oeq.eventQueue) - 1 | |
| oeq.eventQueue[0], oeq.eventQueue[n] = oeq.eventQueue[n], oeq.eventQueue[0] | |
| oeq.down(0, n) | |
| x := oeq.eventQueue[n] | |
| oeq.eventQueue[n] = EventEntry{} // don't pin the popped event | |
| oeq.eventQueue = oeq.eventQueue[:n] | |
| return x | |
| } |
(FWIW the heap arithmetic itself is correct — I fuzzed up/down/popHeap over 2000 random 1–60 element runs with no ordering violation, and (j-1)/2 truncating to 0 at j == 0 is handled by the i == j guard.)
| (( retries-- )) | ||
| done | ||
| if [[ -z "$ds_names" ]]; then | ||
| die "No node-agent daemonsets found." |
There was a problem hiding this comment.
Low: the new daemonset-existence wait doesn't close the race it's meant to fix.
A DaemonSet object existing does not imply its pods exist yet — the DaemonSet controller creates them a moment later. The very next statement is kubectl wait --for=condition=Ready pod -l app.kubernetes.io/component=node-agent, and kubectl wait with a label selector that currently matches zero objects does not wait: it exits non-zero immediately with error: no matching resources found. So the loop can break as soon as the operator writes the DaemonSet, kubectl wait fires before any pod exists, and the script takes the diagnostics/failure branch — the same flake this block is trying to prevent.
Waiting on rollout status instead covers both (and matches what swap_image now does):
| die "No node-agent daemonsets found." | |
| die "No node-agent daemonsets found." | |
| fi | |
| for ds in $ds_names; do | |
| kubectl rollout status daemonset/"$ds" -n "$KUBESCAPE_NS" --timeout=600s | |
| done |
Also worth noting: this 13-line discovery loop is duplicated verbatim in swap_image — extracting a _wait_for_node_agent_daemonsets() helper would keep the two in sync.
jnathangreeg
left a comment
jnathangreeg
left a comment
There was a problem hiding this comment.
Reviewed at depth: built and vetted the branch clean for GOOS=linux, fuzzed the new heap in isolation (2000 random runs, no ordering violations — the arithmetic is a faithful copy of container/heap), and reproduced the HTTP body behavior with a standalone probe. Five findings.
First, the thing I went in most worried about, since it is the classic sync.Pool failure: the pooled bufio.Reader does not leak data between events. ParseHttpRequest and ParseHttpResponse always replace Body before returning on the non-fallback paths, so no pooled buffer escapes into a subsequent event. Given this is a capture path where cross-event bleed would mean mis-attributing one workload's HTTP content to another, that is worth stating explicitly rather than leaving implied.
Blocking — a truncated body now drops the whole event
pkg/containerwatcher/v2/tracers/httpparse.go:216 (detachBody) and :274 (readResponse) both propagate io.ReadAll's error. That converts a previously-successful fallback parse into a hard failure:
- On the request side, the HTTP event is dropped entirely.
- On the response side,
ParseHttpResponsefails, soGroupEventsreturns nil and the stored request is left orphaned — an unpaired request that never completes.
I verified the mechanism locally: on a truncated Content-Length body or a mid-chunk cut, io.ReadAll returns the partial bytes and ErrUnexpectedEOF. The current code discards those partial bytes and fails, where before the pooling change the parse succeeded with what was available.
Detaching the body is genuinely required now that the reader is pooled — that part is correct and necessary. Propagating the error is the part that isn't. Truncated bodies are not an edge case on a live capture path (connection resets, size caps, mid-stream attach), so this trades a real allocation win for silent capture loss on exactly the traffic that is hardest to reason about later.
Suggested shape: keep the partial bytes and treat a truncation error as a successful-but-truncated read, mirroring how the rest of this package already handles partial capture, rather than discarding the data.
Please reconsider — the memory gate got 2x stricter, not "unaffected"
benchmark/compare-metrics.py:18 — SIGNIFICANT_THRESHOLD moves 10 → 5. That tightens the Peak Memory gate by 2x while leaving it computed on max(), which is precisely the max-of-independent-draws bias the PR's own documentation describes for CPU. The workflow passes no --threshold, so the new value is live, and docs/features/benchmark-ci-gate.md states memory gating is unaffected by the change.
Either move memory off max() to a percentile the way CPU was handled, or keep memory on the old threshold and tighten only what the doc claims to tighten.
Lows
pkg/containerwatcher/v2/ordered_event_queue.go:126 — popHeap truncates the slice without zeroing the vacated slot, so up to orderedEventQueue.size (default 100k) stale EventEntry values stay reachable, each holding a live event reference. ContainerWatcher.batchBuf (cap 15000) has the same retention. A q.items[n] = EventEntry{} before the truncate releases them; on a queue sized for 100k events on a memory-optimization PR, it's worth taking.
benchmark/dedup-bench.sh:233 — the new DaemonSet-existence wait doesn't imply any pods exist yet, and kubectl wait against a zero-match selector exits immediately with "no matching resources found" rather than blocking. That's the exact flake the block was added to prevent, so it may not be doing what it looks like it's doing. The same block is duplicated verbatim in swap_image.
Also checked and cleared
Worth listing, because several of these look suspicious on a skim:
- The removed
GetProcessNodecall has no side effects (pure map lookup plusshallowCopyProcess), andGetPidBranchalready errors on a missing target PID — so dropping it introduces no negative caching and no lost nil-guard. - The
containerID == ""early return matches prior behavior, since the only caller discards the error. - The new
httpEventKeystruct doesn't just avoid an allocation, it fixes a latent collision in the old unseparated string key: inode=1/fd=23 and inode=12/fd=3 previously produced the same key. --set nodeAgent.env[0]does replace the chart's wholeenvlist, but here it only clobbers a vestigialNodeNameentry — the realNODE_NAMEis hardcoded in the template, and the template quotesvalue, so the int1renders as"1". (Flagging because the same construct is a live bug in armosec/private-node-agent#552, where the clobbered default was load-bearing. Worth keeping the two copies of this script in mind as they diverge.)- The new heap is a faithful
container/heapport — 2000 randomized runs produced no ordering violations.
Requesting changes on the two io.ReadAll sites only; the threshold change is a judgment call I'd like revisited, and the rest are follow-ups. The pooling and key-struct work is good, and the collision fix is a bonus the description undersells.
…cess tree allocations - Use sync.Pool for bufio.Reader in HTTP parsing (ParseHttpRequest, ParseHttpResponse, and fallbacks) to avoid allocating 4KB buffers on every HTTP packet. - Replace string keys in HTTP eventsMap and ProcessTreeManager cache with zero-allocation structs (httpEventKey and treeCacheKey). - Remove redundant GetProcessNode deep-copy existence check before GetPidBranch in GetContainerProcessTree. Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
9f2af73 to
1ed3532
Compare
jnathangreeg
left a comment
jnathangreeg
left a comment
There was a problem hiding this comment.
Approving 1ed353263. All five findings addressed, and the two blocking ones fixed the right way rather than the convenient way.
Both io.ReadAll sites — FIXED, and the fix keeps the data. detachBody now discards the error and retains the partial bytes:
bodyBytes, _ := io.ReadAll(body)
_ = body.Close()
return io.NopCloser(bytes.NewReader(bodyBytes))Dropping the error return entirely is cleaner than propagating-then-ignoring at the call sites. And the chunked paths at :113 / :155 now use if err == nil { bodyData = decodedBody }, so a mid-chunk cut falls back to the raw bodyData instead of failing the parse. That means a truncated body degrades to a truncated capture rather than dropping the whole event and orphaning its pair — which was the substance of the finding.
popHeap — FIXED, exactly the one-liner: oeq.eventQueue[n] = EventEntry{} before the truncate, so the vacated slot no longer pins a live event reference. Worth having on a queue sized for 100k.
dedup-bench.sh:236 — FIXED, with a pod-existence wait ahead of kubectl wait and the reason recorded inline ("which errors on 0 matches"). Recording the why is what stops someone reverting it as redundant later.
SIGNIFICANT_THRESHOLD — back to 10.0.
Two notes, neither about this PR's code
GitHub's conflict flag is stale. The PR shows mergeable=false, state=dirty, but locally the branch is a clean fast-forward:
git rev-list --left-right --count origin/main...HEAD -> 0 1
git merge-tree origin/main HEAD -> exit 0, no conflicts
Zero divergence, one commit ahead. GitHub hasn't recomputed after the force-push plus the base moving. No rebase needed — flagging so nobody goes chasing a phantom conflict.
SIGNIFICANT_THRESHOLD = 5.0 is live on main right now, independently of this PR. The benchmark commit 24a77e3f merged separately and carried the 10 -> 5 change, so the 2x stricter Peak Memory gate — still computed on max(), while docs/features/benchmark-ci-gate.md states memory gating is unaffected — is already gating CI for everyone. This PR happens to revert it, but if it stalls or gets reworked, main keeps the mis-gated value and unrelated PRs can fail on a max()-driven memory swing the docs say shouldn't gate. Worth fixing on main directly rather than relying on this PR to carry it.
For the record
The scope concern resolved itself usefully: the benchmark files went to main via 24a77e3f, so this PR is now genuinely just the perf commit.
And restating what I cleared last round, since it is the load-bearing safety property here: the pooled bufio.Reader does not leak data between events — ParseHttpRequest/ParseHttpResponse always replace Body before returning, so no pooled buffer escapes into a later event. On a capture path, bleed there would mean attributing one workload's HTTP content to another, so that is the property worth re-checking if this pooling is ever extended to another reader. Also still true: the removed GetProcessNode call had no side effects and GetPidBranch already errors on a missing PID; and the new httpEventKey struct fixes a real latent collision in the old unseparated string key (inode=1/fd=23 vs inode=12/fd=3), which the description undersells.
The merge-base changed after approval.
2 participants
Summary of Changes
This PR implements high-impact memory and CPU optimizations identified from the pprof profiles captured during the benchmark CI runs:
Pool
bufio.Readerin HTTP Parsing (pkg/containerwatcher/v2/tracers/httpparse.go):bufio.NewReaderallocations with async.Poolof*bufio.Reader.bufio.NewReaderSizeaccounted for 1.24 GB (14.03%) of total memory allocated under load.Eliminate Redundant Process Node Copy in
GetContainerProcessTree(pkg/processtree/process_tree_manager.go):GetContainerProcessTreepreviously calledptm.creator.GetProcessNode(int(pid))solely to verifyprocessNode != nil.GetProcessNodeperformed a fullshallowCopyProcesscreating maps and slices that were immediately discarded becauseGetPidBranchdoes its own lookup directly on the process map.Zero-Allocation Struct Keys for LRU Caches:
eventsMapbytype httpEventKey struct { inode uint64; sockFd uint32 }instead of allocating concatenated string keys (strconv.FormatUint(...)).containerProcessTreeCachebytype treeCacheKey struct { containerID string; pid uint32 }instead of string formatting on every single event lookup.Empty Container Guard in
GetContainerProcessTree(pkg/processtree/process_tree_manager.go):armotypes.Process{}, nilfor host events (containerID == ""), avoiding map lookups, mutex acquisitions, and missing-container error formatting.Typed Slice Min-Heap with Zero Interface Boxing for
OrderedEventQueue(pkg/containerwatcher/v2/ordered_event_queue.go) & Batch Extraction (pkg/containerwatcher/v2/container_watcher.go):lane.PriorityQueuewith a typed slice min-heap ([]EventEntry), eliminating heap wrapper allocations on push/pop.PopBatchand reusable bufferbatchBufinContainerWatcher.processQueueBatch, popping full batches under a single lock and eliminating per-event mutex lock acquisitions.Verification
pkg/containerwatcher/v2/...andpkg/processtree/...pass.