Add ability to mock kernel feature prober and expand BPF map tests #14876
Conversation
christarazi
added
release-note/misc
christarazi
changed the title
christarazi
changed the title
christarazi
force-pushed
the
pr/christarazi/bpftool-mock
branch
from
4794053
to
a382184
Compare
christarazi
changed the base branch from
pr/christarazi/fix-bpf-maps-preallocate-flags-fatal
to
master
christarazi
force-pushed
the
pr/christarazi/bpftool-mock
branch
from
a382184
to
eea840e
Compare
|
test-me-please |
pkg/datapath/linux/probes/probes.go
Outdated
| // Prober abstracts the notion of a kernel feature prober. This is useful for | ||
| // testing purposes as it allows us to mock out the kernel, enabling control | ||
| // control over what features are returned. | ||
| type Prober interface { | ||
| // Probe returns the kernel feaures available on machine. | ||
| Probe() Features | ||
| } | ||
|
|
There was a problem hiding this comment.
Usually we define the interface at the place where it will be consumed, I think this is in pkg/bpf instead since that's where the function is that receives this type. Otherwise we are forced to pull in this package with the concrete implementation at the same time that we pull in the interface. Is there a motivation for defining this interface in the probes package?
There was a problem hiding this comment.
Good point. I initially thought to provide the interface inside the probes package simply because proximity and that pkg/bpf was already importing probes anyway. However, that doesn't preclude the pattern of providing the interface at the consumer site, so I've made that change.
| c.Assert(err, IsNil) | ||
|
|
||
| upgrade := upgradeMap.CheckAndUpgrade(&upgradeMap.MapInfo) | ||
| c.Assert(upgrade, Equals, false) |
There was a problem hiding this comment.
If I follow right, the upgrade == false case here is basically saying that:
- Typically, LRU requires pre-allocation.
- Given the underlying lack of LRU support from the "kernel" prober, this map type would actually use hashmap
- Since the map type is switched to hashmap, now pre-allocation can be disabled
- When we try to upgrade the map, defining that its type should be LRU, there's no intermediate state where we decide that the map should be upgraded because the desired type is LRU (or the preallocation flags are mismatched)
- Instead, every single time the map info is evaluated, the type & flags are evaluated first and then the upgrade decision is made based on the attributes afterwards.
At a glance I found this a little bit cryptic in the reading of the test, you might consider stashing a comment something like this in the code. But not such a big deal.
There was a problem hiding this comment.
Thanks, I'll add this snippet as a comment. It can indeed be hard to follow at first glance, as it's very subtle. Also amended the commit to include a reference to the commit that fixed this to establish a link between this regression test and the fix.
christarazi
force-pushed
the
pr/christarazi/bpftool-mock
branch
2 times, most recently
from
143fb44
to
cf24464
Compare
|
test-me-please |
pkg/bpf/map.go
Outdated
| // SetMapTypesFromProber initializes the supported map types from the given | ||
| // prober. This function is useful for testing purposes, as we require | ||
| // injecting our own mocked prober. | ||
| func SetMapTypesFromProber(prober prober) { |
There was a problem hiding this comment.
Why is this method exported if it's only being used from the test file which is in the same package?
There was a problem hiding this comment.
I figured it might be useful in the future, but I can unexport it for now. If it needs to be exported at some point, we can make that change.
This is a preparatory commit to allow mocking of the probe manager for testing purposes. Signed-off-by: Chris Tarazi <chris@isovalent.com>
This commit allows us to mock out kernel probing so that unit tests aren't constrained to the underlying kernel of the running system. Note, special care was taken for bpf.GetMapType() to keep the function API untouched. The main motivation is to avoid having to modify each call site. The logic relies on the fact that if no supported map types have been loaded into memory, we will default to probing the underlying kernel. The unit tests will leverage this in order to inject their own supported may types by calling bpf.SetMapTypesFromProber(), and thus bypassing this check. Signed-off-by: Chris Tarazi <chris@isovalent.com>
This commit preparatively refactors the test to be table-driven for later commits to expand on. Signed-off-by: Chris Tarazi <chris@isovalent.com>
This commit builds upon the previous commit by expanding the table-driven test and leveraging the ability to mock the kernel prober to assert against 4.9 kernel supported maps. The test is a regression test against the bpf.(*Map).CheckAndUpgrade() bug that was fixed in 7e5a97f ("bpf: Fix wrong map type when fetching preallocate map flags"). Signed-off-by: Chris Tarazi <chris@isovalent.com>
christarazi
force-pushed
the
pr/christarazi/bpftool-mock
branch
from
cf24464
to
a971fc9
Compare
See commit msgs.
Followup from #14853