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

map: Fix MapBatch test for BatchLookupAndDelete case #1259

Closed
wants to merge 1 commit into from

Conversation

alxn
Copy link
Contributor

@alxn alxn commented Dec 6, 2023

I've been seeing this test flake on main:

$ go test -exec sudo  . -count 100
--- FAIL: TestMapBatch (0.00s)
    --- FAIL: TestMapBatch/Hash (0.00s)
        quicktest.go:12:
            error:
              values are not equal
            got:
              int(1)
            want:
              int(2)
            stack:
              /Users/alun/Code/github.com/cilium/ebpf.git/map_test.go:175
                qt.Assert(t, qt.Equals(count, len(lookupKeys)))

--- FAIL: TestMapBatch (0.00s)
    --- FAIL: TestMapBatch/Hash (0.00s)
        quicktest.go:12:
            error:
              values are not equal
            got:
              int(1)
            want:
              int(2)
            stack:
              /Users/alun/Code/github.com/cilium/ebpf.git/map_test.go:175
                qt.Assert(t, qt.Equals(count, len(lookupKeys)))

--- FAIL: TestMapBatch (0.01s)
    --- FAIL: TestMapBatch/Hash (0.01s)
        quicktest.go:12:
            error:
              values are not equal
            got:
              int(1)
            want:
              int(2)
            stack:
              /Users/alun/Code/github.com/cilium/ebpf.git/map_test.go:175
                qt.Assert(t, qt.Equals(count, len(lookupKeys)))

FAIL
FAIL	github.com/cilium/ebpf	75.904s
FAIL

It seems that BatchLookupAndDelete() can actually sometimes return 1 element (verified by iterating over the map afterwards, and there were 3 items remaining).

@alxn alxn requested a review from a team as a code owner December 6, 2023 23:20
@lmb lmb closed this Dec 7, 2023
@lmb lmb force-pushed the alun/hacking-tests branch from 8e1cfee to 447288c Compare December 7, 2023 09:40
@lmb
Copy link
Collaborator

lmb commented Dec 7, 2023

Something very strange happened. I checked out your PR via the GH VScode extension and then amended the commit. For some reason the checkout only did a shallow clone of your repo (how does that even work?!). Can you push you original change again? I can't push to the branch anymore.

@lmb
Copy link
Collaborator

lmb commented Dec 7, 2023

NVM: #1260

@alxn alxn deleted the alun/hacking-tests branch December 7, 2023 17:42
@alxn
Copy link
Contributor Author

alxn commented Dec 7, 2023

https://github.com/torvalds/linux/blob/master/kernel/bpf/syscall.c#L1771

I guess it could be the retry case after getting 1 element, but it was a surprise to me that it returned only 1, after being asked for 2.

@lmb
Copy link
Collaborator

lmb commented Dec 7, 2023

This is because the batch lookup semantics are really complicated for hash tables. It doesn't use generic_map_lookup_batch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants