-
Notifications
You must be signed in to change notification settings - Fork 165
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
fix(driver): extend modern bpf test framework to all drivers (part 5) #832
Conversation
Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>
Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>
Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>
Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>
Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>
Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>
Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>
Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>
Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>
Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>
352dabb
to
396c42f
Compare
/hold |
hold because we need to change the required tests on test-infra when we are ready to merge it |
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.
This LGTM, as always Andrea :D
Left a minor comment!
* workaround, since `CAPTURE_SCHED_PROC_FORK` requires `BPF_RAW_TRACEPOINTS` and so | ||
* kernel versions >= 4.17 | ||
*/ | ||
#ifndef CAPTURE_SCHED_PROC_FORK |
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.
Shouldn't we check the kernel version or if has raw tracepoints here?
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.
I would perhaps adding those kind of checks to driver/feature_gates.h
or another central place to collect those minimum requirements or even distro levels.
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.
Uhm right now we have this check at compile time in the probe bpf https://github.com/falcosecurity/libs/blob/master/driver/bpf/quirks.h#L31-L33... maybe we can do something better a build time like if you enable -DBUILD_BPF
you should have a kernel version >=4.17
on some arch like aarch64
and s390x
. BTW this doesn't seem too much related to this test PR, we can do that in another PR when we decide how to proceed, WDYT?
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.
I think this is more a follow-up as we should then review and consider the other options.
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
LGTM label has been added. Git tree hash: f77f1131923de6954cc2b3a9af42caafec0e10dd
|
Note, we need to merge falcosecurity/test-infra#959 before this PR can be merged. |
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
/hold due to @FedeDP's comment above
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Andreagit97, FedeDP, jasondellaluce 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:
Approvers can indicate their approval by writing |
Closing and reopening to trigger new prow required status checks. |
@FedeDP: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/reopen |
@FedeDP: Reopened this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/unhold |
Poiana is 😴 |
/honk |
@FedeDP: Unable to find goose. Have you checked the garden? In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
What type of PR is this?
/kind bug
/kind feature
Any specific area of the project related to this PR?
/area driver-kmod
/area driver-bpf
/area driver-modern-bpf
/area tests
Does this PR require a change in the driver versions?
No
What this PR does / why we need it:
This PR belongs to the #783 series. It fixes some differences between the 3 drivers. It tries also to simplify code paths where possible.
Which issue(s) this PR fixes:
Special notes for your reviewer:
I've to check the CI part, but I've tested it locally on the following machines:
Does this PR introduce a user-facing change?: