-
Notifications
You must be signed in to change notification settings - Fork 176
Run e2e tests with modern probe #967
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
Run e2e tests with modern probe #967
Conversation
Signed-off-by: Mauro Ezequiel Moltrasio <[email protected]>
Signed-off-by: Mauro Ezequiel Moltrasio <[email protected]>
|
This is truly amazing @Molter73 😍 |
incertum
left a comment
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.
@Molter73 🚀 nice!
Does this PR aim to only update the CI or could we also get an update re how to use it on localhost? Currently getting IndexError: list index out of range errors ...
| sinsp.generate_id(sinsp_example) for sinsp_example in sinsp_examples | ||
| ] | ||
|
|
||
| # modern probe doesn't support EXE_WRITABLE flag yet |
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.
@LucaGuerra is this in your queue for Falco 0.35?
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.
yep, I've in on the roadmap, in Falco 0.35 the modern probe should reach feature parity with other drivers 🎉
| ids = [sinsp.generate_id(sinsp_example) for sinsp_example in sinsp_examples] | ||
|
|
||
| # For some reason, the modern probe gives a longer proc.exe than the legacy | ||
| # drivers, needs further investigation. |
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.
- 1 test is failing because the test expects
proc.exeto benginx: master proces, but the modern probe sets it tonginx: master process nginx -g daemon off;. This is not a major problem IMO, but some extra investigation is needed to understand why this happens before changing the test to pass.
Checked w/ bpf_printk statements while running the e2e tests locally ... not a truncation error, this is all we got in the old eBPF raw_syscalls tracepoints for argv[0] aka exe ... @Andreagit97 would you know some good docs stating why? Clearly the modern_bpf driver has the complete arg.
This demonstrates in an excellent way how much we need these e2e tests 🎉 .
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.
is there a way to reproduce this test locally by running it with just the modern bpf probe?
|
Thank you @Molter73 ! Sorry for the silly question but Is there a way to run locally a single test, with just the modern bpf? |
|
Hey everyone!
@incertum , this should work both locally and on CI, the problem comes from me doing hardcoded index access like this one:
I'll get it fixed in a commit in a minute, sorry about that.
@Andreagit97 I haven't planned to get the e2e tests to be run with just a single type of driver, since they are pretty fast on their own. What you can do is change this script and point it to the directory/file that has the test you want to run. You can find instructions on how to run the e2e tests in the readme: Side note, if you want the tests to stop at the first failure, you can pass |
Signed-off-by: Mauro Ezequiel Moltrasio <[email protected]>
|
For some reason, on my host the e2e tests never end, I've to type CTRL+C to stop them :/ I used a python env to run them, and I launch them with the following command: |
Yeah, there's a weird threading error were this method causes a lock when the last test fails (or xfails) in this case: libs/test/e2e/tests/commons/sinspqa/sinsp.py Lines 103 to 110 in acf239f
I believe it's because the inner for loop is waiting to read a line from I'm aware of it and will try to get it sorted, but AFAICT it doesn't break anything major, the tests do end and the HTML report is correctly created. |
yep, I can confirm it 👍 |
|
Think I have a fix for the threading thing, looks like some Python non-sense that causes the |
@Molter73 above (either explicitly adding to README or supplying elegant options to toggle both the driver used and which tests to run) and fixing the threading thing would make a perfect touch-up PR, but agreed let's first merge this one! |
incertum
left a comment
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.
Tests now run again locally, what's missing is also an updated cmake setup in the README aka add -DBUILD_LIBSCAP_MODERN_BPF=ON
Can be done all together in a possible touch up PR.
/approve
|
LGTM label has been added. DetailsGit tree hash: 166d97c48c15c64381cfceaa0700b57219aaea8c |
|
yep we can merge this, with @FedeDP we found the reason behind this #967 (comment) we will try to come out with a fix in the next few days :) |
Andreagit97
left a comment
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.
/approve
FedeDP
left a comment
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.
/approve
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Andreagit97, FedeDP, incertum, Molter73 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/milestone 0.11.0 |
What type of PR is this?
/kind feature
Any specific area of the project related to this PR?
/area CI
/area tests
Does this PR require a change in the driver versions?
What this PR does / why we need it:
With the modern BPF probe quickly reaching a production ready state, I think it's time we start running e2e tests with it. This PR achieves this by adding a new set of
sinsp-examplebinaries to be run using this new driver to the tests. A small check was added to prevent the tests from being run on systems that are not supported by the driver.There are currently 3 tests that are failing:
exe_writableflag not being supported yet on the modern probe.proc.exeto benginx: master proces, but the modern probe sets it tonginx: master process nginx -g daemon off;. This is not a major problem IMO, but some extra investigation is needed to understand why this happens before changing the test to pass.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
I couldn't get
sinsp-exampleto compile with the system libbpf in CI, so I set-DUSE_BUNDLED_LIBBPF=ONwhen compiling the tests.Does this PR introduce a user-facing change?: