fix(ebpf): synchronize perfUsageCollector.Collect with Manager.Start - #54952
fix(ebpf): synchronize perfUsageCollector.Collect with Manager.Start#54952pgimalac wants to merge 1 commit into
Conversation
perfUsageCollector.Collect reads PerfMap/RingBuffer telemetry fields (e.g. BufferSize) during Prometheus Gather ticks, while ebpf.Manager.Start writes those same fields during eBPF program startup. The collector's mutex protected its own data structures but not the shared PerfMap/ RingBuffer objects. Change the collector mutex from sync.Mutex to sync.RWMutex so Collect takes RLock (reader), and export LockForWrite/UnlockForWrite so Manager.Start can take the write lock around m.Manager.Start(). This prevents Collect from reading telemetry fields while Start is writing them. Found via a race-detector-enabled build in staging (see #54333).
pgimalac
added
qa/done
|
@codex review |
github-actions
Bot
added
the
short review
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ac15534c1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ebpftelemetry.LockForWrite() | ||
| defer ebpftelemetry.UnlockForWrite() |
There was a problem hiding this comment.
When CWS internal telemetry is enabled, pkg/security/probe/eventstream/{reorderer/perfmap,ringbuffer/ringbuffer}.go registers buffers with this collector, but pkg/security/probe/probe_ebpf.go:494 starts its raw *ebpf-manager.Manager directly. That path therefore never acquires this new lock and can still race with a /telemetry collection while Start initializes the registered buffer fields; the synchronization needs to cover these direct manager starts as well, or be centralized below the wrapper.
Useful? React with 👍 / 👎.
| p.mtx.RLock() | ||
| defer p.mtx.RUnlock() |
There was a problem hiding this comment.
When two /telemetry requests gather concurrently, this read lock lets both collections invoke the registered channel-length callbacks and mutate the shared metric vectors at once. The callbacks installed in pkg/ebpf/perf/event.go:371-372 and 408-409 use Swap(0), so one collection can consume the value while the other overwrites the gauge with zero before either snapshot is emitted, making channel-length telemetry nondeterministic. Keep collection serialized and use a separate lock or mechanism to synchronize it with manager startup.
Useful? React with 👍 / 👎.
| case <-time.After(100 * time.Millisecond): | ||
| t.Fatal("Collect did not complete after write lock was released") |
There was a problem hiding this comment.
Under a sufficiently loaded CI worker, the collector goroutine can take longer than 100 ms to be scheduled after the lock is released, causing this test to fail even though the locking implementation is correct. The earlier 20 ms wait can likewise pass without proving that the goroutine reached RLock; use a readiness barrier to establish that the collection attempt has started and a deterministic completion signal instead of fixed wall-clock deadlines.
Useful? React with 👍 / 👎.
|
🎯 Code Coverage (details) 🔗 Commit SHA: 6ac1553 | Docs | Datadog PR Page | Give us feedback! |
1 participant
What does this PR do?
Fixes a data race on
PerfMap/RingBuffertelemetry fields:perfUsageCollector.CollectreadsBufferSize()(which readsbufferSizewithout a lock) during Prometheus Gather ticks, whileebpf.Manager.Startwrites those same fields during eBPF program startup. Changes the collector mutex fromsync.Mutextosync.RWMutexsoCollecttakesRLock(reader), and exportsLockForWrite/UnlockForWritesoManager.Startcan take the write lock aroundm.Manager.Start().Motivation
Fix a race, found via a race-detector-enabled build in staging (see #54333 for context).
Describe how you validated your changes
Added
TestLockForWriteBlocksCollectandTestCollectConcurrentReaderswhich validate the lock mechanism:LockForWriteblocksCollectuntilUnlockForWriteis called, and multipleCollectcalls can run concurrently underRLock. Tests pass under-race(dda inv test --targets=./pkg/ebpf/telemetry/... --race). The actual race requires eBPF support to trigger (Manager.Start writes to PerfMap/RingBuffer internal fields), so the test validates the synchronization mechanism rather than the full race.