pkg/bpf: fix bug where second value can be skipped. #26583
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
maintainer-s-little-helper
bot
added
the
dont-merge/needs-release-note-label
tommyp1ckles
added
kind/bug
maintainer-s-little-helper
bot
removed
the
dont-merge/needs-release-note-label
tommyp1ckles
added
needs-backport/1.14
|
/test |
tommyp1ckles
force-pushed
the
pr/tp/fix-dump-reliably-skipping-value
branch
from
bd9c7cd
to
1b1db9a
Compare
|
/test |
tommyp1ckles
force-pushed
the
pr/tp/fix-dump-reliably-skipping-value
branch
2 times, most recently
from
369bebe
to
e51df1c
Compare
|
Minor nit: This is marked as a |
|
@joestringer Yeah makes sense - I'll make the user impact clearer. |
tommyp1ckles
force-pushed
the
pr/tp/fix-dump-reliably-skipping-value
branch
from
e51df1c
to
74f7184
Compare
|
/test |
rgo3
reviewed
Jul 7, 2023
tommyp1ckles
force-pushed
the
pr/tp/fix-dump-reliably-skipping-value
branch
from
fae0fb2
to
632bc09
Compare
DumpReliablyWithCallback will skip a value callback in some situations. This may result in incorrect cilium map dumps or garbage collection. In situations where the initial key is deleted just after being retrieved, there is no previous key to fallback on. The reliable dump will attempt to use the current nextKey (that was based on the deleted current key). The local currentKey and nextKey Key types are being passed to NextKey which eventually writes the nextKey output to the nextKeys pointer location (via the bpf syscall). The currentValue was simply being assinged by the equals operator, which was copying the underlying interface pointer. Thus in this situation, the next iteration attempt was passing the same pointer twice to NextKey causing the currentKey to be set to the next key a second time - skipping a map element. Fixes cilium#26491 Signed-off-by: Tom Hadlaw <tom.hadlaw@isovalent.com>
tommyp1ckles
force-pushed
the
pr/tp/fix-dump-reliably-skipping-value
branch
from
632bc09
to
f6f862d
Compare
|
/test |
tommyp1ckles
force-pushed
the
pr/tp/fix-dump-reliably-skipping-value
branch
2 times, most recently
from
09fffa1
to
b38237d
Compare
|
/test |
rgo3
requested changes
Jul 10, 2023
tommyp1ckles
force-pushed
the
pr/tp/fix-dump-reliably-skipping-value
branch
from
b38237d
to
2eb6b98
Compare
|
Thanks for the review @rgo3 😄, ptal |
This test is an improvement on the previous TestDumpReliablyWithCallback test.
The goal of this one is to provide more robust testing of the reliable dump mechanism.
Specifically, it does the following:
1. Creates a map with a small number of entries, populate it with [1, maxEntries)
2. Start a goroutine that continuously dumps the map and checks that the dump contains
all odd elements in the range [1, maxEntries).
3. At the same time, start another goroutine that continuously deletes and re-adds even
elements in the map.
The motivation here is to provide a test that better catches regressions in code that is inherently prone to race-condition.
This creates a situation where we have interleaved updates and dumps, we're interested
in ensuring that each dump contains all odd elements in the range [1, maxEntries).
This will catch bugs and regressions related where elements are skipped in the dump.
For example, while running this without the fix: 74f71841e9c037ddd10bedc3128f3b28cb023597 this will fail a majority of the time.
Following this fix, this test should always pass.
This was tested locally by running this several times with a million iterations - both with the fix and without.
For practical purposes we will lower the number of iterations to 1000 to avoid slowing down the test suite too much.
Signed-off-by: Tom Hadlaw <tom.hadlaw@isovalent.com>
tommyp1ckles
force-pushed
the
pr/tp/fix-dump-reliably-skipping-value
branch
from
2eb6b98
to
84ce10e
Compare
|
/tesrt |
|
/test |
rgo3
approved these changes
Jul 12, 2023
aanm
added
backport-pending/1.14
gandro
added
backport-done/1.14
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.
DumpReliablyWithCallback will skip a value callback in some situations.
In cases where the initial key is deleted just after being retrieved, there is no previous key to fallback on. The reliable dump will attempt to use the current nextKey (that was based on the deleted current key).
The local currentKey and nextKey Key types are being passed to NextKey which eventually writes the nextKey output to the nextKeys pointer location (via the bpf syscall).
The currentValue was simply being assinged by the equals operator, which was copying the underlying interface pointer.
Thus in this situation, the next iteration attempt was passing the same pointer twice to NextKey causing the currentKey to be set to the next key a second time - skipping a map element.
Fixes #26491
Validating the fix
Because the test has 256 possible keys the particular start condition for this failure seems to be quite rare in CI, by reducing this to 5 it happens much more often (presumably 20% of the time?) - making this much easier to reproduce.
I tested this change by continuously running the DumpReliablyWithCallback (modified with maxSize=5) test to reproduce, and after the fix to validate (ran it 1000 times post fix without failure).