fix(k8sprocessor): race condition when getting Pod data #938
Conversation
swiatekm
force-pushed
the
fix/k8sprocessor/update-race
branch
from
a6c6817 to
a742efd
Compare
github-actions
bot
added
the
documentation
| func (c *WatchClient) GetPodAttributes(identifier PodIdentifier) (map[string]string, bool) { | ||
| pod, ok := c.getPod(identifier) | ||
| if !ok { | ||
| return nil, false | ||
| } | ||
| attributes := make(map[string]string, len(pod.Attributes)) | ||
| for key, value := range pod.Attributes { | ||
| attributes[key] = value | ||
| } | ||
| for key, value := range c.getPodOwnerMetadataAttributes(pod) { | ||
| attributes[key] = value | ||
| } | ||
| observability.RecordIPLookupMiss() | ||
| return nil, false | ||
| return attributes, ok |
There was a problem hiding this comment.
Could we pass target map to this function so we avoid double copying?
There was a problem hiding this comment.
I'd rather not unless we can show a performance increase. Doing it this way is easier to understand, and I'd be surprised if copying like 16 strings twice makes any significant difference.
Either way, for this fix I'd rather keep things simple, we can revisit this later.
swiatekm
force-pushed
the
fix/k8sprocessor/update-race
branch
from
a742efd to
e5dac4f
Compare
swiatekm
force-pushed
the
fix/k8sprocessor/update-race
branch
from
e5dac4f to
8fa9147
Compare
|
I refactored the locking a bit and I believe it's now safe. We should rethink how it's meant to work in general though. The code acts internally as if passing around a |
| ownerAttributes := c.getPodOwnerMetadataAttributes(pod) | ||
|
|
||
| // we need to take a lock here because pod.Attributes may be modified concurrently | ||
| // TODO: clean up the locking in these functions |
There was a problem hiding this comment.
This one and the ones it calls, made that clearer in the comment.
swiatekm
force-pushed
the
fix/k8sprocessor/update-race
branch
from
8fa9147 to
b28fda2
Compare
We had a race condition where we'd update Pod metadata in
GetPodwhile only holding a read lock. In circumstances where two batches of data would arrive from the same Pod in quick succession, we'd get a panic due to concurrent map writes.I fixed this by changing the interface to only return an attributes map and explicitly making that a copy of what's in the Pod attributes.