MapType probe API #321
MapType probe API #321
Conversation
There was a problem hiding this comment.
I'm not super familiar with the library, but I've worked on bpftool's probes and I've been following the work from Robin (as a GSoC co-mentor), so here are some comments on the PR.
In addition to the comments below, I would suggest to rework the content of the different commits (possibly squashing them all as the result is not that big), there is not point in adding probe.go to remove it in the following commit for example.
I see a test for the feature. I haven't looked at the CI so I don't know if it will integrate well with it. Ideally we need an environment with all features available so we can check we can detect them? And an environment where we know that a given set of features is missing, so we can check that there are not reported as supported? How can we validate the cache is working? It would also be interesting to be able to test with different capabilities for the process (although the policy on how to deal with -EPERM would need to be sorted out first).
|
Thanks for the review @qmonnet. I always planned on squashing the commits at the end to have a nice single commit for this work, Timo and I just agreed I should add smaller commits for now documenting my progress and making the review easier while its a draft. I'll start addressing the comments on Monday morning.
For now the test was just a way for me to check if my probes are working. Generally I had similar thoughts, I was also wondering if this feature API, or at least the testing of it needs a minimum version for featuers like we have with the FeatureTest abstraction e.g. Line 16 in ca49208
In the earlier commits I had took some time stamps in the tests to validate it, but generally isn't it ok to assume that a Go map works? |
rgo3
force-pushed
the
probemaps-poc
branch
2 times, most recently
from
619445c
to
8a73ce5
Compare
rgo3
force-pushed
the
probemaps-poc
branch
2 times, most recently
from
50f9da2
to
ef58877
Compare
With this commit we add a new subpackage `features`. `features` provides an API for calling processes to probe the kernel for available BPF features. The first public API function added with this commit is `HaveMapType(mt ebpf.MapType) error`, which allows to probe for available BPF map types. The results of an API call are stored in an internal cache, resulting in a probe being run at most once even if called repeatedly. Signed-off-by: Robin Gögge <r.goegge@outlook.com>
- Removed naked error comparison to ErrNotSupported (although valid) - Converted error handling logic to switch statement so only a single case may fire. Before, it was possible to hit multiple cases if the error from BPFMapCreate contained multiple wrapped errors. - Skipped wrapping of EPERM to avoid ending up with 'unexpected error during feature probe: operation not permitted'. Permission errors are not unexpected. Signed-off-by: Timo Beckers <timo@isovalent.com>
Signed-off-by: Timo Beckers <timo@isovalent.com>
Currently only supports default bpfMapCreateAttr values, to showcase
basic functionality. Results are cached by a go map. Tests log durations
to see cache performance. Added some open questions and missing error
interface checks to the comments in the file.
Signed-off-by: Robin Gögge r.goegge@outlook.com