Fix panic in the metrics-generator when using multiple tenants with default overrides#2786
Conversation
| _, sizeOk := desiredProcessors[spanmetrics.Size.String()] | ||
|
|
||
| newDesiredProcessors := map[string]struct{}{} | ||
| maps.Copy(newDesiredProcessors, desiredProcessors) |
There was a problem hiding this comment.
if desiredProcessors was capable of racing in the previous code, I don't see how this fixes it. it could still race here while copying to newDesiredProcessors
There was a problem hiding this comment.
This only reads the map, afaik concurrent map reads are fine. Later in this function we would modify desiredProcessors but with this change we only modify the copy newDesiredProcessors.
There was a problem hiding this comment.
Good catch. A little googling and a quick test confirms what you're saying. If multiple goroutines have access to this map though there is the threat in the future of someone writing to it in a different goroutine and breaking this assumption.
I'm fine with the fix, but let's add a comment explaining why we are copying the map and what our expectations are for other goroutines holding a reference to the map.
11c95df to
32bd393
Compare
|
The modified test fails on a data race ☝🏻 |
a3c5661 to
f956733
Compare
desiredProcessors before configuring subprocessors to avoid concurrent read and writes|
The backport to To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-2786-to-release-v2.2 origin/release-v2.2
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x 9af6bb2eccd7c04c2f06097278f6b92f8d74860b
# When the conflicts are resolved, stage and commit the changes
git add . && git cherry-pick --continueIf you have the GitHub CLI installed: # Create the PR body template
PR_BODY=$(gh pr view 2786 --json body --template 'Backport 9af6bb2eccd7c04c2f06097278f6b92f8d74860b from #2786{{ "\n\n---\n\n" }}{{ index . "body" }}')
# Push the branch to GitHub and a PR
echo "${PR_BODY}" | gh pr create --title "[release-v2.2] Fix panic in the metrics-generator when using multiple tenants with default overrides" --body-file - --label "type/bug" --label "backport" --base release-v2.2 --milestone release-v2.2 --webOr, if you don't have the GitHub CLI installed (we recommend you install it!): # If you don't have the GitHub CLI installed: Push the branch to GitHub and manually create a PR:
git push --set-upstream origin backport-2786-to-release-v2.2
# Remove the local backport branch
git switch main
git branch -D backport-2786-to-release-v2.2Unless you've used the GitHub CLI above, now create a pull request where the |
What this PR does:
We have a concurrent read/write panic while reading from
desiredProcessors, this is the only place we modify it really. Let's copy it before modifying it.Which issue(s) this PR fixes:
Fixes #2785
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]