new(userspace/libsinsp): expose threadinfo cgroups in plugins table api #2107

FedeDP · 2024-10-10T09:16:54Z

What type of PR is this?

/kind feature

Any specific area of the project related to this PR?

/area libsinsp
/area tests

Does this PR require a change in the driver versions?

What this PR does / why we need it:

This is needed to ensure k8smeta plugin can improve its perf.
Also, it is mandatory for the possible container engine as a plugin refactor because we will need to access thread cgroups from the plugin.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Most of the lines changes come from the new test. Most of real code is under table_adapters.h for the new std::pair adapter.

Does this PR introduce a user-facing change?:

new(userspace/libsinsp): expose threadinfo cgroups in plugins table api

…able API. Signed-off-by: Federico Di Pierro <[email protected]>

…pair_t values. Signed-off-by: Federico Di Pierro <[email protected]>

poiana · 2024-10-10T09:17:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP

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~~ [FedeDP]

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

Instead, rely on a new table entry adapter, `pair_table_entry_adapter`. Signed-off-by: Federico Di Pierro <[email protected]>

userspace/libsinsp/state/table_adapters.h

FedeDP · 2024-10-10T09:22:43Z

userspace/libsinsp/threadinfo.h

@@ -443,10 +443,10 @@ class SINSP_PUBLIC sinsp_threadinfo : public libsinsp::state::table_entry {
 	bool m_exe_lower_layer;  ///< True if the executable file belongs to lower layer in overlayfs
 	bool m_exe_from_memfd;   ///< True if the executable is stored in fileless memory referenced by
 	                         ///< memfd
-	std::vector<std::string> m_args;       ///< Command line arguments (e.g. "-d1")
-	std::vector<std::string> m_env;        ///< Environment variables
-	std::unique_ptr<cgroups_t> m_cgroups;  ///< subsystem-cgroup pairs


Had to revert this change... (see #473 for the initial changeset)

FedeDP · 2024-10-10T09:23:06Z

userspace/libsinsp/threadinfo.h

@@ -659,6 +660,10 @@ class SINSP_PUBLIC sinsp_threadinfo : public libsinsp::state::table_entry {
 	bool m_parent_loop_detected;
 	libsinsp::state::stl_container_table_adapter<decltype(m_args)> m_args_table_adapter;
 	libsinsp::state::stl_container_table_adapter<decltype(m_env)> m_env_table_adapter;
+	libsinsp::state::stl_container_table_adapter<


This is a bit ugly because we need to enforce the specific value unwrapping class (the second entry).

FedeDP · 2024-10-10T09:24:01Z

/milestone 0.19.0
/cc @jasondellaluce @mrgian

for a review :) i want to gather feedback asap (eg: change field names, or anything else i got wrong)

poiana · 2024-10-10T09:24:05Z

@FedeDP: GitHub didn't allow me to request PR reviews from the following users: mrgian.

Note that only falcosecurity members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/milestone 0.19.0
/cc @jasondellaluce @mrgian

for a review :) even if this is still wip, i want to gather feedback asap (eg: change field names, or anything else i got wrong)

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-sigs/prow repository.

github-actions · 2024-10-10T09:28:06Z

Perf diff from master - unit tests

     6.07%     +1.01%  [.] sinsp::next
     0.59%     +0.98%  [.] std::_Hashtable<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, libsinsp::state::dynamic_struct::field_info>, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, libsinsp::state::dynamic_struct::field_info> >, std::__detail::_Select1st, std::equal_to<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::hash<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<true, false, true> >::find
     1.63%     +0.89%  [.] scap_event_decode_params
     4.97%     -0.72%  [.] gzfile_read
     1.64%     -0.56%  [.] sinsp::fetch_next_event
     0.32%     +0.54%  [.] std::_Hashtable<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, libsinsp::state::dynamic_struct::field_info>, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, libsinsp::state::dynamic_struct::field_info> >, std::__detail::_Select1st, std::equal_to<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::hash<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<true, false, true> >::_M_find_before_node
     0.76%     +0.53%  [.] std::_Hashtable<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::__detail::_Identity, std::equal_to<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::hash<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<true, true, true> >::find
     5.14%     -0.53%  [.] next
     1.68%     -0.51%  [.] std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char const*>
     2.45%     +0.45%  [.] sinsp_thread_manager::find_thread

Heap diff from master - unit tests

peak heap memory consumption: -6.56M
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

Heap diff from master - scap file

peak heap memory consumption: -117.47K
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

Benchmarks diff from master

Comparing gbench_data.json to /root/actions-runner/_work/libs/libs/build/gbench_data.json
Benchmark                                                         Time             CPU      Time Old      Time New       CPU Old       CPU New
----------------------------------------------------------------------------------------------------------------------------------------------
BM_sinsp_split_mean                                            +0.0300         +0.0300           153           158           153           158
BM_sinsp_split_median                                          +0.0300         +0.0300           153           158           153           158
BM_sinsp_split_stddev                                          +0.9625         +0.9627             1             1             1             1
BM_sinsp_split_cv                                              +0.9053         +0.9055             0             0             0             0
BM_sinsp_concatenate_paths_relative_path_mean                  +0.0145         +0.0145            56            57            56            57
BM_sinsp_concatenate_paths_relative_path_median                +0.0095         +0.0095            56            57            56            57
BM_sinsp_concatenate_paths_relative_path_stddev                +2.3723         +2.3738             0             1             0             1
BM_sinsp_concatenate_paths_relative_path_cv                    +2.3240         +2.3254             0             0             0             0
BM_sinsp_concatenate_paths_empty_path_mean                     -0.0452         -0.0452            25            24            25            24
BM_sinsp_concatenate_paths_empty_path_median                   -0.0429         -0.0429            25            24            25            24
BM_sinsp_concatenate_paths_empty_path_stddev                   -0.5688         -0.5685             0             0             0             0
BM_sinsp_concatenate_paths_empty_path_cv                       -0.5483         -0.5481             0             0             0             0
BM_sinsp_concatenate_paths_absolute_path_mean                  +0.0086         +0.0086            56            56            56            56
BM_sinsp_concatenate_paths_absolute_path_median                +0.0084         +0.0084            56            56            56            56
BM_sinsp_concatenate_paths_absolute_path_stddev                +0.0151         +0.0132             0             0             0             0
BM_sinsp_concatenate_paths_absolute_path_cv                    +0.0065         +0.0046             0             0             0             0
BM_sinsp_split_container_image_mean                            +0.0100         +0.0100           390           394           390           394
BM_sinsp_split_container_image_median                          +0.0110         +0.0110           390           394           390           394
BM_sinsp_split_container_image_stddev                          +0.6310         +0.6263             1             2             1             2
BM_sinsp_split_container_image_cv                              +0.6149         +0.6102             0             0             0             0

codecov · 2024-10-10T09:32:45Z

Codecov Report

Attention: Patch coverage is 74.81752% with 69 lines in your changes missing coverage. Please review.

Project coverage is 74.46%. Comparing base (2b1e402) to head (538ea70).
Report is 50 commits behind head on master.

Files with missing lines	Patch %	Lines
...insp/test/plugins/syscall_subtables_array_pair.cpp	64.36%	62 Missing ⚠️
userspace/libsinsp/state/table_adapters.h	82.75%	5 Missing ⚠️
userspace/libsinsp/threadinfo.cpp	81.81%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2107      +/-   ##
==========================================
+ Coverage   73.69%   74.46%   +0.76%     
==========================================
  Files         253      254       +1     
  Lines       31914    33316    +1402     
  Branches     5642     5718      +76     
==========================================
+ Hits        23519    24808    +1289     
- Misses       8395     8481      +86     
- Partials        0       27      +27

Flag	Coverage Δ
libsinsp	`74.46% <74.81%> (+0.76%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

FedeDP · 2024-10-10T11:46:59Z

/hold
For a review by Jason!

FedeDP · 2024-10-15T06:16:36Z

Also, should i bump plugin api version?

…` and `second`. Signed-off-by: Federico Di Pierro <[email protected]>

FedeDP · 2024-10-21T14:38:39Z

/unhold

FedeDP added 2 commits October 9, 2024 16:32

new(userspace,test): expose threadinfo::m_cgroups to plugin state t…

5886211

…able API. Signed-off-by: Federico Di Pierro <[email protected]>

chore(userspace/libsinsp): do not return pointers to stack allocated …

9795c39

…pair_t values. Signed-off-by: Federico Di Pierro <[email protected]>

poiana added release-note do-not-merge/work-in-progress dco-signoff: yes kind/feature New feature or request area/libsinsp area/tests size/XXL labels Oct 10, 2024

poiana added the approved label Oct 10, 2024

poiana requested review from hbrueckner and jasondellaluce October 10, 2024 09:17

chore(userspace/libsinsp): avoid defining new value type.

2364176

Instead, rely on a new table entry adapter, `pair_table_entry_adapter`. Signed-off-by: Federico Di Pierro <[email protected]>

FedeDP force-pushed the new/expose_cgroups_plugins_table_api branch from dceb0b7 to 2364176 Compare October 10, 2024 09:21

FedeDP commented Oct 10, 2024

View reviewed changes

userspace/libsinsp/state/table_adapters.h Show resolved Hide resolved

FedeDP commented Oct 10, 2024

View reviewed changes

poiana added this to the 0.19.0 milestone Oct 10, 2024

FedeDP changed the title ~~wip: new(userspace/libsinsp): expose threadinfo cgroups in plugins table api~~ new(userspace/libsinsp): expose threadinfo cgroups in plugins table api Oct 10, 2024

poiana removed the do-not-merge/work-in-progress label Oct 10, 2024

poiana added the do-not-merge/hold label Oct 10, 2024

chore(userpace/libsinsp): rename pair.{first,second} to just `first…

538ea70

…` and `second`. Signed-off-by: Federico Di Pierro <[email protected]>

jasondellaluce approved these changes Oct 21, 2024

View reviewed changes

poiana assigned jasondellaluce Oct 21, 2024

poiana added the lgtm label Oct 21, 2024

poiana removed the do-not-merge/hold label Oct 21, 2024

LucaGuerra approved these changes Oct 22, 2024

View reviewed changes

poiana assigned LucaGuerra Oct 22, 2024

poiana merged commit d8ca259 into master Oct 22, 2024
49 checks passed

poiana deleted the new/expose_cgroups_plugins_table_api branch October 22, 2024 10:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new(userspace/libsinsp): expose threadinfo cgroups in plugins table api #2107

new(userspace/libsinsp): expose threadinfo cgroups in plugins table api #2107

FedeDP commented Oct 10, 2024 •

edited

Loading

poiana commented Oct 10, 2024

FedeDP Oct 10, 2024 •

edited

Loading

FedeDP Oct 10, 2024

FedeDP commented Oct 10, 2024 •

edited

Loading

poiana commented Oct 10, 2024

github-actions bot commented Oct 10, 2024 •

edited

Loading

codecov bot commented Oct 10, 2024 •

edited

Loading

FedeDP commented Oct 10, 2024

FedeDP commented Oct 15, 2024

FedeDP commented Oct 21, 2024

new(userspace/libsinsp): expose threadinfo cgroups in plugins table api #2107

new(userspace/libsinsp): expose threadinfo cgroups in plugins table api #2107

Conversation

FedeDP commented Oct 10, 2024 • edited Loading

poiana commented Oct 10, 2024

FedeDP Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

FedeDP Oct 10, 2024

Choose a reason for hiding this comment

FedeDP commented Oct 10, 2024 • edited Loading

poiana commented Oct 10, 2024

github-actions bot commented Oct 10, 2024 • edited Loading

Perf diff from master - unit tests

Heap diff from master - unit tests

Heap diff from master - scap file

Benchmarks diff from master

codecov bot commented Oct 10, 2024 • edited Loading

Codecov Report

FedeDP commented Oct 10, 2024

FedeDP commented Oct 15, 2024

FedeDP commented Oct 21, 2024

FedeDP commented Oct 10, 2024 •

edited

Loading

FedeDP Oct 10, 2024 •

edited

Loading

FedeDP commented Oct 10, 2024 •

edited

Loading

github-actions bot commented Oct 10, 2024 •

edited

Loading

codecov bot commented Oct 10, 2024 •

edited

Loading