feat(userspace/libsinsp)!: isolate sinsp_threadinfo from sinsp
#2335
Conversation
8e69c08 to
2c3e142
Compare
Perf diff from master - unit testsHeap diff from master - unit testsHeap diff from master - scap fileBenchmarks diff from master |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2335 +/- ##
==========================================
- Coverage 77.03% 77.02% -0.01%
==========================================
Files 228 228
Lines 30270 30266 -4
Branches 4636 4635 -1
==========================================
- Hits 23319 23313 -6
- Misses 6951 6953 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2c3e142 to
5d3a9ce
Compare
userspace/libsinsp/threadinfo.cpp
Outdated
| @@ -381,14 +388,13 @@ void sinsp_threadinfo::add_fd_from_scap(scap_fdinfo* fdi) { | |||
| tevt.set_info(&(g_infotables.m_event_info[PPME_SYSCALL_READ_X])); | |||
| tevt.set_cpuid(0); | |||
| tevt.set_num(0); | |||
| tevt.set_inspector(m_inspector); | |||
There was a problem hiding this comment.
I removed the following line as the inspector is not anymore available in threadinfo. By inspecting the code, and by looking at the tests results, I couldn't find evidence this line was needed, but I'd be happier if someone could confirm that we don't actually need it.
There was a problem hiding this comment.
Just tried to run sinsp-example in capture mode, configuring a filter to trigger that code path. It seems to work fine. I ran it using the following command:
sudo ./libsinsp/examples/sinsp-example -s ./scap_files/kexec_arm64.scap -f fd.type=ipv4|
/milestone 0.21.0 |
d74f7d0 to
cf37cf5
Compare
cf37cf5 to
64dce56
Compare
|
I broke down the big commit in 3 commits. 2 of them are still big, but I hope reviewers can better reason about the changes in this way. |
userspace/libsinsp/threadinfo.cpp
Outdated
| fdi.m_name = ipv4tuple_to_string(&fdi.m_sockinfo.m_ipv4info, | ||
| m_hostname_and_port_resolution_enabled); |
There was a problem hiding this comment.
m_hostname_and_port_resolution_enabled looks like a global thing (at least a per-thread-manager thing), so maybe we can pass it as a parameter from sinsp_thread_manager::fix_sockets_coming_from_proc
userspace/libsinsp/threadinfo.cpp
Outdated
| m_inspector->is_hostname_and_port_resolution_enabled()); | ||
| m_network_interfaces.update_fd(*newfdi); | ||
| newfdi->m_name = ipv4tuple_to_string(&newfdi->m_sockinfo.m_ipv4info, | ||
| m_hostname_and_port_resolution_enabled); |
There was a problem hiding this comment.
this only gets called from sinsp::on_new_entry_from_proc AFAICT, so we can pass m_hostname_and_port_resolution_enabled from there as an argument
userspace/libsinsp/threadinfo.cpp
Outdated
| @@ -356,7 +360,7 @@ void sinsp_threadinfo::add_fd_from_scap(scap_fdinfo* fdi) { | |||
| } | |||
|
|
|||
| auto addedfdi = m_fdtable.add(fdi->fd, std::move(newfdi)); | |||
| if(m_inspector->m_filter != nullptr && m_inspector->is_capture()) { | |||
| if(m_inspector->m_filter != nullptr && m_sinsp_mode.is_capture()) { | |||
There was a problem hiding this comment.
This whole if can live in the caller, just return addedfdi rather than void
userspace/libsinsp/threadinfo.cpp
Outdated
| @@ -540,7 +544,7 @@ void sinsp_threadinfo::set_user(uint32_t uid) { | |||
| m_uid = uid; | |||
| if(const scap_userinfo* user = m_inspector->m_usergroup_manager->get_user(container_id, uid); | |||
| !user) { | |||
| const auto notify = m_inspector->is_live() || m_inspector->is_syscall_plugin(); | |||
| const auto notify = m_sinsp_mode.is_live() || is_syscall_plugin_enabled(); | |||
There was a problem hiding this comment.
This (and set_group) could get a bool notify passed in; the ultimate callers of this are:
- sinsp::on_new_entry_from_proc
- sinsp_thread_manager::get_thread_ref
- sinsp_parser::parse_*_exit
The latter two (still) have a sinsp* m_sinsp, so could call a method on it to figure out whether we need to notify.
Same for the usergroup_manager, it could get passed in.
(the usergroup_updater could take the same two parameters in its constructor)
userspace/libsinsp/threadinfo.cpp
Outdated
| @@ -630,7 +634,7 @@ void sinsp_threadinfo::set_args(const std::vector<std::string>& args) { | |||
| } | |||
|
|
|||
| void sinsp_threadinfo::set_env(const char* env, size_t len) { | |||
| if(len == SCAP_MAX_ENV_SIZE && m_inspector->large_envs_enabled()) { | |||
| if(len == SCAP_MAX_ENV_SIZE && is_large_envs_enabled()) { | |||
There was a problem hiding this comment.
We can pass this flag from the caller (same story as for set_user, they all have a sinsp* available, though sinsp_threadinfo::init might become a bit cluttered)
userspace/libsinsp/threadinfo.cpp
Outdated
| if(m_bound_server_ports.find(fdi.m_sockinfo.m_ipv4info.m_fields.m_sport) != | ||
| m_bound_server_ports.end()) { |
There was a problem hiding this comment.
bound_server_ports could be a parameter too
userspace/libsinsp/threadinfo.cpp
Outdated
| @@ -360,7 +363,7 @@ void sinsp_threadinfo::add_fd_from_scap(scap_fdinfo* fdi) { | |||
| } | |||
|
|
|||
| auto addedfdi = m_fdtable.add(fdi->fd, std::move(newfdi)); | |||
| if(m_inspector->m_filter != nullptr && m_sinsp_mode.is_capture()) { | |||
| if(m_filter != nullptr && m_sinsp_mode.is_capture()) { | |||
There was a problem hiding this comment.
As I mentioned in the previous commit's comments, this could all live in the caller
userspace/libsinsp/threadinfo.cpp
Outdated
| const auto accessor = | ||
| m_thread_manager->get_field_accessor(sinsp_thread_manager::s_container_id_field_name); |
There was a problem hiding this comment.
Ouch. All the dynamic table stuff should be cached somewhere. I don't have a checkout of libs fresh enough to see how get_container_id is used, but maybe we can pass some sort of proxy to the method that would have all this cached?
userspace/libsinsp/threadinfo.cpp
Outdated
| @@ -510,8 +511,7 @@ std::string sinsp_threadinfo::get_container_user() { | |||
|
|
|||
| const auto container_id = get_container_id(); | |||
| if(!container_id.empty()) { | |||
| auto table = m_inspector->m_thread_manager->get_table( | |||
| sinsp_thread_manager::s_containers_table_name); | |||
| auto table = m_thread_manager->get_table(sinsp_thread_manager::s_containers_table_name); | |||
There was a problem hiding this comment.
Umm a thread manager is a table, not sure its responsibility is to provide info about other tables... (not your code, I know)
|
As the number of requested changes is high, I decided to split this work in multiple PRs. I'm gonna keep track of all these related works in #2343. |
b3f5a39 to
4cba8fb
Compare
4cba8fb to
382547d
Compare
|
As the other related works have been merged (refer to #2343), I rebased this one to align it with those changes. |
Remove `sinsp_threadinfo` dependency on `sinsp` fields that are immutable from the `sinsp_threadinfo` perspective by selectively providing each single dependency the component needs. In order to avoid code duplication in the event processor, add `sinsp_threadinfo_factory`'s `create_unique_attorney` inner class following the attorney-client idiom to limit access to `sinsp_threadinfo_factory::create_unique` private method. BREAKING CHANGE: update `sinsp_threadinfo` constructor Signed-off-by: Leonardo Di Giovanna <leonardodigiovanna1@gmail.com>
Remove `sinsp_threadinfo` dependency on `sinsp` fields that are mutable from the `sinsp_threadinfo` perspective by selectively providing each single dependency the component needs. In order to solve a cyclic dependency problem with the thread manager and the thread info factory, add `sinsp_threadinfo_factory`'s `set_thread_manager_attorney` inner class following the attorney-client idiom to limit access to `sinsp_threadinfo_factory::set_thread_manager` private method. BREAKING CHANGE: update `sinsp_threadinfo` constructor Signed-off-by: Leonardo Di Giovanna <leonardodigiovanna1@gmail.com>
Remove unused `sinsp::get_fdinfo_factory()` and `sinsp::get_fdtable_factory()` from `sinsp` public API. BREAKING CHANGE: remove `sinsp::get_fdinfo_factory()` and `sinsp::get_fdtable_factory()` Signed-off-by: Leonardo Di Giovanna <leonardodigiovanna1@gmail.com>
Reduce threadinfo's params resources waste by moving them into a separate struct provided at construction phase and shared among all threadinfo instances. BREAKING CHANGE: update `sinsp_threadinfo` constructor Signed-off-by: Leonardo Di Giovanna <leonardodigiovanna1@gmail.com>
382547d to
138dc83
Compare
|
LGTM label has been added. Git tree hash: 83d0704f37b253247c11c6b4a87dcf28004d63b6
|
|
cc @mstemm PTAL 🙏 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ekoops, 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:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind cleanup
/kind feature
Any specific area of the project related to this PR?
/area libsinsp
Does this PR require a change in the driver versions?
What this PR does / why we need it:
This PR is part of a series #2343.
It removes
sinsp_threadinfodependency onsinspby selectively providing each single dependency the component needs.In order to avoid code duplication in the event processor, this PR adds
sinsp_thread_info::create_unique_attorneysinsp_threadinfo_factory's inner classes following the attorney-client idiom (this helps to limit access tosinsp_threadinfo_factoryprivate methods).Moreover, this PR also removes some related and unused sinsp APIs:
sinsp::get_fdinfo_factory()andsinsp::get_fdtable_factory().Finally, it changes the
sinsp_threadinfoconstructor to accept the dependencies.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: