plugins/logs: fixed reconfiguration race condition #2948
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tsandall
reviewed
Nov 25, 2020
kubaj
force-pushed
the
fix-log-plugin-race-condition
branch
from
fb16bb6 to
551bb2a
Compare
|
I updated the PR. |
srenatus
previously approved these changes
Nov 27, 2020
kubaj
force-pushed
the
fix-log-plugin-race-condition
branch
from
551bb2a to
12dc58d
Compare
|
@srenatus, I'm glad I could help. I squashed commits according to instructions, can you reapprove please so I can merge? |
|
@kubaj Can you please squash the commits and update the commit message ? The sign-off line is duplicated. |
kubaj
force-pushed
the
fix-log-plugin-race-condition
branch
from
44a4906 to
3cfa8d8
Compare
|
@ashutosh-narkar I updated the commit message to remove duplicate sign-off. I hope it's all you need. |
|
@kubaj thanks for updating your commit message. I think you'll need to rebase from master and then push your single commit to your fork as currently it's showing that 11 commits need to be merged. |
Masking part of the logging plugin had incorrectly placed mutex -- only for writing the variable p.mask, but not for reading it. This caused race condition in the situation when bundle update calls plugin.Reconfigure() method between setting the p.mask variable and then evaluating it. Reconfiguration of the plugin sets p.mask to nil and therefore calling p.mask.Eval() results in panic. Signed-off-by: Jakub Kulich <jakub.kulich@exponea.com>
kubaj
force-pushed
the
fix-log-plugin-race-condition
branch
from
3cfa8d8 to
a8fd154
Compare
|
@ashutosh-narkar sorry about that. I fixed that, it should be ok now. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Bug summary
Logging plugin had race condition while it was masking the log and plugin was reconfigured at the same time. This caused panic of the OPA. Fixed issue for us and also may fix #2835 .
Details
Masking part of the logging plugin had incorrectly placed mutex -- only
for writing the variable p.mask, but not for reading it. This caused
race condition in the situation when bundle update calls plugin.Reconfigure()
method between setting the p.mask variable and then evaluating it.
Reconfiguration of the plugin sets p.mask to nil and therefore calling
p.mask.Eval() results in panic.
Tests covering functionality shouldn't change, test covering the fixed issue is not delivered, because it's difficult to write a test simulating the race condition (if you know about some efficient way, feel free to guide me, I'll happily learn).