Use sync.Map to prevent race on map access in osquery-perf
#24501
Conversation
dantecatalfamo
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24501 +/- ##
==========================================
+ Coverage 63.43% 63.49% +0.05%
==========================================
Files 1586 1597 +11
Lines 150594 151450 +856
Branches 3867 3867
==========================================
+ Hits 95534 96156 +622
- Misses 47437 47635 +198
- Partials 7623 7659 +36
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -2401,16 +2411,27 @@ func (a *agent) submitLogs(results []resultLog) error { | |||
| return fmt.Errorf("/version check failed: %w", err) | |||
| } | |||
|
|
|||
| var resultLogs []byte | |||
There was a problem hiding this comment.
This was an attempt to reduce memory/CPU footprint on a previous load test where we had to simulate Fleet server being down and a lot of logs buffered in osquery perf. Is this causing any issues?
dantecatalfamo
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
|
@lucasmrod There's a screenshot in the linked issue. Essentially there are two goroutines on separate timers that read from the same map concurrently, and if things line up just right it can cause a panic |
|
I've actually never run into it myself, but victor was running a load test and hit it. I suspect it's super rare |
@dantecatalfamo No need to include a change file unless it's a product change. |
cmd/osquery-perf/agent.go
Outdated
| @@ -777,28 +776,32 @@ func (a *agent) runLoop(i int, onlyAlreadyEnrolled bool) { | |||
| // check if we have any scheduled queries that should be returning results | |||
| var results []resultLog | |||
| now := time.Now().Unix() | |||
| a.scheduledQueriesMu.Lock() | |||
| // a.scheduledQueriesMu.Lock() | |||
cmd/osquery-perf/agent.go
Outdated
| } | ||
| scheduledQueryData[scheduledQueryName] = q | ||
| a.scheduledQueryData.Store(scheduledQueryName, q) |
There was a problem hiding this comment.
logger goroutine could send an incomplete number of results (if it runs after a.scheduledQueryData.Clear), right?
There was a problem hiding this comment.
I've replaced the sync.Map with a pointer
dantecatalfamo
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
dantecatalfamo
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
cmd/osquery-perf/agent.go
Outdated
| v.lastRun = now | ||
| a.scheduledQueryData[queryName] = v | ||
| query.lastRun = now | ||
| a.scheduledQueryData.Store(queryName, query) |
There was a problem hiding this comment.
This still might end up with a map with inconsitent values, right? (if this gets executed at same time as the assignment to the map on the other goroutine).
Could we have a mutex to do mutual exclusion between the "logger" and "config" goroutines? (to simplify and reduce risk of races/invalid-data)
There was a problem hiding this comment.
I took the reference to the scheduledQueryData from the agent before iterating over it. If you still think I should add a muted after I can
dantecatalfamo
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
#24381
Do we add
changes/forosquery-perf?