-
Notifications
You must be signed in to change notification settings - Fork 410
tests: filter unit tests by PID + fix pidSet bugs #997
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
Conversation
b37a0bc
to
a4b6ade
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I think the PR makes sense but I believe we need to figure out the details.
a4b6ade
to
bfea44d
Compare
✅ Deploy Preview for tetragon ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
7f58f57
to
1a12bc2
Compare
A recent commit introduced a new binary to the tester-progs directory, but it was not added to the gitignore. Let's do so here. Also remove an unnecessary entry from the top-level gitignore which is covered by the tester-progs gitignore. Signed-off-by: William Findlay <[email protected]>
1a12bc2
to
55842a6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline.
Let's try doubling the map if we run out of space.
Worst case scenario, let's just add a loud warning that things might not work as expected if we hit the limit.
This commit adds some additional documentation around the pid_set filter explaining how it works and that it should not be used in production. Signed-off-by: William Findlay <[email protected]>
55842a6
to
6f2498d
Compare
The existing pidSet filter was broken in a few ways. First, we were using a single shared global variable to track PIDs. This meant that having multiple pidSet filters active would cause them to pollute each other's state. The existing implementation also did not properly track children of children, meaning that we would only pay attention to nodes of depth < 2 from the parent. This patch introduces refactors to the pidSet filters that resolve the above issues. Since we are maintaining a separate PID cache per filter, we implement the cache as an LRU of size 8192 to prevent unbounded memory usage. This should be more than enough for most practical contexts. Signed-off-by: William Findlay <[email protected]>
pidSet filters are not intended for production use and may not work as intended over extended periods of time. Disable them by default in ParseFilterList() and provide a command line flag to enable them. Signed-off-by: William Findlay <[email protected]>
Add options to the observer_test_helper package that enable test authors to configure export filters on their tests. We introduce three new options: - WithMyPid() adds a PidSet filter to the export allowlist that matches the current test process and any of its children - WithAllowList() adds a generic export allowlist to the test - WithDenyList() adds a generic export denylist to the test We also modify the observer creation helpers to support passing arbitrary options as variadic parameters. Signed-off-by: William Findlay <[email protected]>
Filtering unit tests with a PidSet can significantly reduce noise in the event checker logs, making it easier to debug tests. While it is possible that this could obfuscate events that are missing process information by preventing them from being written to the export log, I think the trade-off is probably worth it here. Signed-off-by: William Findlay <[email protected]>
6f2498d
to
deb0dac
Compare
Fix pidSet filter so that it properly accounts for children of children. Update observer_test_helper to support adding export filters on protojson and update existing tests to filter by PID with a pidSet filter.