Skip to content
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

pkg/eventqueue: fix concurrent access of waitConsumeOffQueue #10137

Merged
merged 1 commit into from Feb 12, 2020

Conversation

aanm
Copy link
Member

@aanm aanm commented Feb 11, 2020

spanStart of waitConsumeOffQueue could be read before being written in
case the buffered event was executed before the execution of
waitConsumeOffQueue.Start() in the modified lines of this commit. To fix
this we should execute waitConsumeOffQueue.Start() even before the event
is put into the queue. Although it does not give the correct span stat,
the developer or user can derieve it by subtracting the waitEnqueue span
to retrieve the real waitConsumeOffQueue span.

Fixes: add0d65 ("add eventqueue package")
Signed-off-by: André Martins andre@cilium.io

Fix concurrent access of a variable used for metrics

This change is Reviewable

@aanm aanm added kind/bug This is a bug in the Cilium logic. pending-review release-note/bug This PR fixes an issue in a previous release of Cilium. labels Feb 11, 2020
@aanm aanm requested a review from a team as a code owner February 11, 2020 11:46
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 Feb 11, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.6.7 Feb 11, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.7.0-rc4 Feb 11, 2020
@aanm aanm force-pushed the pr/fix-concurrent-access-of-wait branch from 6ccefe3 to f336373 Compare February 11, 2020 11:47
spanStart of waitConsumeOffQueue could be read before being written in
case the buffered event was executed before the execution of
waitConsumeOffQueue.Start() in the modified lines of this commit. To fix
this we should execute waitConsumeOffQueue.Start() even before the event
is put into the queue. Although it does not give the correct span stat,
the developer or user can derive it by subtracting the waitEnqueue span
to retrieve the real waitConsumeOffQueue span.

Fixes: add0d65 ("add eventqueue package")
Signed-off-by: André Martins <andre@cilium.io>
@aanm aanm force-pushed the pr/fix-concurrent-access-of-wait branch from f336373 to 2a67492 Compare February 11, 2020 11:47
@aanm
Copy link
Member Author

aanm commented Feb 11, 2020

test-me-please

@coveralls
Copy link

coveralls commented Feb 11, 2020

Coverage Status

Coverage decreased (-0.02%) to 44.594% when pulling 2a67492 on pr/fix-concurrent-access-of-wait into 54d9254 on master.

Copy link
Member

@ianvernon ianvernon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch !

Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

If we intend to backport this, please consider adding a more user-friendly release-note describing briefly the impact. At a glance this looks like just a statistics fixup, but the current PR title suggests potentially a lot more.

@aanm aanm merged commit da2e652 into master Feb 12, 2020
1.8.0 automation moved this from In progress to Merged Feb 12, 2020
@aanm aanm deleted the pr/fix-concurrent-access-of-wait branch February 12, 2020 13:09
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.7 in 1.7.0-rc4 Feb 17, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.7 in 1.7.0-rc4 Feb 17, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.6 in 1.6.7 Feb 17, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.6 in 1.6.7 Feb 17, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.7 to Backport done to v1.7 in 1.7.0-rc4 Feb 18, 2020
@joestringer joestringer moved this from Backport pending to v1.6 to Backport done to v1.6 in 1.6.7 Feb 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
No open projects
1.6.7
Backport done to v1.6
1.7.0-rc4
Backport done to v1.7
1.8.0
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

6 participants