-
Notifications
You must be signed in to change notification settings - Fork 549
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix panic in the metrics-generator when using multiple tenants with default overrides #2786
Conversation
@@ -151,8 +152,11 @@ func (i *instance) updateSubprocessors(desiredProcessors map[string]struct{}, de | |||
_, latencyOk := desiredProcessors[spanmetrics.Latency.String()] | |||
_, sizeOk := desiredProcessors[spanmetrics.Size.String()] | |||
|
|||
newDesiredProcessors := map[string]struct{}{} | |||
maps.Copy(newDesiredProcessors, desiredProcessors) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
…ocessors calls
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 --continue If 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 --web Or, 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.2 Unless you've used the GitHub CLI above, now create a pull request where the |
…efault overrides (#2786) (#2806) * Modify concurrency test to consider multitenant / concurrent updateProcessors calls * Copy desiredProcessors map before modifying it * Update CHANGELOG.md (cherry picked from commit 9af6bb2) Co-authored-by: Koenraad Verheyden <[email protected]>
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.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]