-
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
new(scap, sinsp)!: linux_hostinfo platform for use with non-syscall source plugins #1969
Conversation
90f8d8b
to
af5f096
Compare
Perf diff from master - unit tests
Perf diff from master - scap file
Heap diff from master - unit tests
Heap diff from master - scap file
|
af5f096
to
51e01cd
Compare
Perf diff from master - unit tests
Perf diff from master - scap file
Heap diff from master - unit tests
Heap diff from master - scap file
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1969 +/- ##
==========================================
+ Coverage 50.98% 50.99% +0.01%
==========================================
Files 310 311 +1
Lines 39612 39636 +24
Branches 17793 17415 -378
==========================================
+ Hits 20197 20214 +17
+ Misses 14350 14328 -22
- Partials 5065 5094 +29 ☔ View full report in Codecov by Sentry. |
51e01cd
to
7d27878
Compare
Perf diff from master - unit tests
Perf diff from master - scap file
Heap diff from master - unit tests
Heap diff from master - scap file
|
7d27878
to
ee8829e
Compare
Perf diff from master - unit tests
Perf diff from master - scap file
Heap diff from master - unit tests
Heap diff from master - scap file
|
1fb6fe1
to
7850607
Compare
Perf diff from master - unit tests
Perf diff from master - scap file
Heap diff from master - unit tests
Heap diff from master - scap file
|
1 similar comment
Perf diff from master - unit tests
Perf diff from master - scap file
Heap diff from master - unit tests
Heap diff from master - scap file
|
Just need to decide on the naming of the enum items, else LGTM ty! |
userspace/libsinsp/sinsp.h
Outdated
enum class sinsp_plugin_platform | ||
{ | ||
GENERIC, //!< generic platform, no system information collected | ||
LINUX_HOSTINFO, //!< basic host information collected, for non-syscall source plugins | ||
LINUX, //!< full system information collected, for syscall source plugins | ||
}; |
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.
enum class sinsp_plugin_platform | |
{ | |
GENERIC, //!< generic platform, no system information collected | |
LINUX_HOSTINFO, //!< basic host information collected, for non-syscall source plugins | |
LINUX, //!< full system information collected, for syscall source plugins | |
}; | |
enum class sinsp_plugin_platform | |
{ | |
SINSP_PLATFORM_GENERIC, //!< generic platform, no system information collected | |
SINSP_PLATFORM_HOSTINFO, //!< basic host information collected, for non-syscall source plugins | |
SINSP_PLATFORM_FULL, //!< full system information collected, for syscall source plugins | |
}; |
Are we ok with this naming?
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 sure am, though I thought we wanted to hide the platform concept from users?
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.
Personally, I'd leave the thing as is (modulo naming), including the breaking API change but I'll accept whatever we decide.
I went this road :)
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.
ok if it's now ok to say platform
works for me for sure.
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.
Why is it that I always push first, commit second? :D the enum variants should be renamed now
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.
If the end result works, no problem :) Where are we at with this PR @gnosek? Ready for final review? Doesn't seem like any more folks posted on the PR when Fede asked #1969 (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.
Ready for final review, or at least looks like it
This is a minimal Linux platformm intended to be useful with source plugins that do not handle syscall data but still want access to some info about the machine they're running on. Currently collected data includes: - machine info - agent info - interface list Signed-off-by: Grzegorz Nosek <[email protected]>
7850607
to
29e59f5
Compare
Rather than passing the mode directly, introduce a new enum that describes both the mode and the platform to use. Fixes: #2281 Signed-off-by: Grzegorz Nosek <[email protected]>
29e59f5
to
2118fcd
Compare
Perf diff from master - unit tests
Perf diff from master - scap file
Heap diff from master - unit tests
Heap diff from master - scap file
|
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.
LGTM!
Also cc @mrgian
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 to decide on when to merge it as we need to bump falco with these changes and do not want to block other updates in the meantime.
LGTM label has been added. Git tree hash: d941eacc10bd46effbe92bebac9768bf5cb3d679
|
@FedeDP bump since you are back. Also when would we want to unhold this and sync with a falco libs bump? |
I'd say asap :) we have the libs bump PR opened since ages on Falco, it's time to sync with latest libs! Let's say next week :) |
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: FedeDP, gnosek, incertum 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 |
Also, since this is a breaking change, cc @Molter73 |
/unhold |
The following PR in the libs falcosecurity/libs#1969 introduces a new platform for plugins that requires access to the proc filesystem. Signed-off-by: Aldo Lacuku <[email protected]>
The following PR in the libs falcosecurity/libs#1969 introduces a new platform for plugins that requires access to the proc filesystem. Signed-off-by: Aldo Lacuku <[email protected]>
What type of PR is this?
/kind bug
Any specific area of the project related to this PR?
/area libscap
/area libsinsp
Does this PR require a change in the driver versions?
What this PR does / why we need it:
Introduce a new lightweight Linux platform type and expose it for use with (non-syscall) source plugins
Which issue(s) this PR fixes:
Fixes #2821
Special notes for your reviewer:
Does this PR introduce a user-facing change?: