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

new(test): add test/vm for localhost VM-based driver kernel compatibility tests #524

Merged
merged 21 commits into from
Jul 13, 2023

Conversation

incertum
Copy link
Contributor

@incertum incertum commented Aug 2, 2022

Signed-off-by: Melissa Kilby [email protected]

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area API-version

/area build

/area driver-kmod

/area driver-bpf

/area driver-modern-bpf

/area libscap-engine-bpf

/area libscap-engine-gvisor

/area libscap-engine-kmod

/area libscap-engine-modern-bpf

/area libscap-engine-nodriver

/area libscap-engine-noop

/area libscap-engine-source-plugin

/area libscap-engine-udig

/area libscap

/area libpman

/area libsinsp

/area tests

/area proposals

Does this PR require a change in the driver versions?

/version driver-API-version-major

/version driver-API-version-minor

/version driver-API-version-patch

/version driver-SCHEMA-version-major

/version driver-SCHEMA-version-minor

/version driver-SCHEMA-version-patch

What this PR does / why we need it:

Edited:

Add test/vm (localhost automated VM based kernel - compiler version grid-search driver tests) to test targets similar to e2e sinsp tests.

CI integration is tracked here #531.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

new(test): add `test/vm` for localhost VM-based driver kernel compatibility tests 

@FedeDP
Copy link
Contributor

FedeDP commented Aug 2, 2022

Just WOW 😲
This is something we have been needing for a while!
Having something similar in the CI is super valuable indeed!
Perhaps a cmake target to run the tests would help a bit :)
I will give it a look asap, at least for some initial feedback!
Thank you very much for this contribution!

@jasondellaluce
Copy link
Contributor

This is real good Melissa! We've been looking for something like this for a while now. Thank you a lot!

@incertum
Copy link
Contributor Author

incertum commented Aug 3, 2022

Just WOW 😲 This is something we have been needing for a while! Having something similar in the CI is super valuable indeed!

localhost obviously is more tractable to gain more confidence in your code changes and spot issues you care about in your own deployment :) which typically is only a few distros and a smaller grid of kernel releases ...

Proper CI needs to care about more which is such a daunting task, that's probably why we are where we are.
#531

Perhaps a cmake target to run the tests would help a bit :) I will give it a look asap, at least for some initial feedback! Thank you very much for this contribution!

Let me look into this more as this gets a bit more polished. Will also look more into #506 to converge to a good common style for such tests.

@incertum
Copy link
Contributor Author

incertum commented Nov 5, 2022

@FedeDP: Added cmake targets as I finally got to refactoring and polishing it a bit. Thanks again for the suggestion, it helped making the driver-sanity tests cleaner and easier to use.
@Molter73: Tried to make the cmake setup similar to e2e targets. Thanks to your work on e2e it was pretty straight forward as I used the e2e cmake setup as template.
@LucaGuerra: Thanks for your feedback in the CI integration discussion. Did not forget about making sure to reboot the VM after each kmod test to be resilient against buggy kernel modules.

Removed WIP as v1 would now be considered complete. The PR is now ready for review. Aware that it's a random collection of scripts, mostly bash scripts. Happy to address any style feedback throughout in an attempt to make the scripts easier to follow and maintain, I think there is improvement potential in this regard.

Outlook: Emulation support still missing and arm64 support as well ... let's first see if v1 is useful for x86_64 for now.

Few observations: Noticed that in general ubuntu distro appears to be the most difficult one. Re-ran these tests many times and a few ubuntu kmod tests sometimes randomly just did not work. centos7 stable in comparison.
Unrelated to this PR also noticed that the eBPF verifier on ubuntu let's minor bugs pass compared to other distros (same kernel versions), hence yum based distros appear to be the better choice for dev for sure. Also kmod never seemed to work for newer ubuntu kernels (e.g. 5.19 or 6.0), while centos7 was fine.

@incertum incertum force-pushed the test-driver-sanity-test-suites branch from ca88e0b to 598755c Compare November 16, 2022 23:10
@FedeDP
Copy link
Contributor

FedeDP commented Dec 2, 2022

/milestone 0.11.0

@poiana poiana added this to the 0.11.0 milestone Dec 2, 2022
@poiana
Copy link
Contributor

poiana commented Mar 2, 2023

Issues go stale after 90d of inactivity.

Mark the issue as fresh with /remove-lifecycle stale.

Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Provide feedback via https:/falcosecurity/community.

/lifecycle stale

@incertum
Copy link
Contributor Author

incertum commented Mar 2, 2023

/remove-lifecycle stale

incertum and others added 2 commits June 29, 2023 15:18
and highlight basic system dependencies

Co-authored-by: Massimiliano Giovagnoli <[email protected]>
Signed-off-by: Melissa Kilby <[email protected]>
Co-authored-by: Massimiliano Giovagnoli <[email protected]>
Signed-off-by: Melissa Kilby <[email protected]>
@incertum incertum force-pushed the test-driver-sanity-test-suites branch from e7d1fcf to 2d29259 Compare June 29, 2023 15:19
jasondellaluce
jasondellaluce previously approved these changes Jun 30, 2023
@poiana
Copy link
Contributor

poiana commented Jun 30, 2023

LGTM label has been added.

Git tree hash: e769b66a9136d603c536cdd4838609ae7769456e

Copy link
Contributor

@Molter73 Molter73 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay on this @incertum!! I left a few more comments, but overall LGTM and unless someone else has objections I think we can go ahead and merge.

test/vm/CMakeLists.txt Outdated Show resolved Hide resolved
test/vm/README.md Outdated Show resolved Hide resolved
test/vm/kernels.jsonl Outdated Show resolved Hide resolved
test/vm/scripts/compile_drivers.sh Show resolved Hide resolved
test/vm/scripts/compile_scap_open.sh Show resolved Hide resolved
test/vm/scripts/kernel_download.sh Outdated Show resolved Hide resolved
test/vm/scripts/kernel_download.sh Outdated Show resolved Hide resolved
@incertum
Copy link
Contributor Author

incertum commented Jul 3, 2023

Thank you @Molter73 for taking another look, appreciate it 🙏 !

Also re-worked the previous commit of adding more info to the readme here 3d96343 more clearly stating how this is different and separate from the approach we will take for the new kernel driver CI-powered tests. I thought it was important to highlight as there will be no overlap in setup, scripts and design choices, because of these very different use cases. CC @LucaGuerra @therealbobo @FedeDP

minor cleanup
setting expectations for duration of tests

Co-authored-by: Mauro Ezequiel Moltrasio <[email protected]>
Signed-off-by: Melissa Kilby <[email protected]>
@incertum
Copy link
Contributor Author

incertum commented Jul 7, 2023

@Molter73 kindly asking if you have additional change requests for v1 or if we can proceed? Thanks!

@incertum
Copy link
Contributor Author

Hi everyone, kind bump, thank you!

Copy link
Contributor

@Molter73 Molter73 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

Sorry for the delay on this, LGTM!

@poiana
Copy link
Contributor

poiana commented Jul 11, 2023

LGTM label has been added.

Git tree hash: 45f77f49c9b008e201597b72bfc6bc55a2bcc778

@poiana
Copy link
Contributor

poiana commented Jul 13, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: incertum, jasondellaluce, Molter73

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [Molter73,incertum,jasondellaluce]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@poiana poiana merged commit 121d918 into falcosecurity:master Jul 13, 2023
17 checks passed
@FedeDP
Copy link
Contributor

FedeDP commented Jul 28, 2023

/milestone 5.1.0+driver

@poiana poiana modified the milestones: driver-backlog, 5.1.0+driver Jul 28, 2023
@incertum incertum deleted the test-driver-sanity-test-suites branch December 8, 2023 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants