Skip to content

cleanup(sinsp): remove threadinfo dependency on thread_manager/usergroup_manager#2689

Merged
poiana merged 11 commits intofalcosecurity:masterfrom
gnosek:threadinfo-api-cleanup
Nov 11, 2025
Merged

cleanup(sinsp): remove threadinfo dependency on thread_manager/usergroup_manager#2689
poiana merged 11 commits intofalcosecurity:masterfrom
gnosek:threadinfo-api-cleanup

Conversation

@gnosek
Copy link
Copy Markdown
Contributor

@gnosek gnosek commented Oct 14, 2025 •
edited by ekoops
Loading

What type of PR is this?

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

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind 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 CI

/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-savefile

/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:

This PR removes references to thread_manager/usergroup_manager from threadinfo. The references are cyclic dependencies involving code that's not even in sinsp any more (the container plugin), and make reasoning about the code harder than needed.

Summary of changes in threadinfo methods:

  • get_container_id, get_container_user, get_container_ip -- replaced with a method on sinsp->m_plugin_tables
  • get_user, get_group, get_loginuser and corresponding set_* methods -- removed (inlined into filterchecks)
  • get_parent_thread -- removed (inlined into callers)
  • get_ancestor_process, traverse_parent_state, get_ancestor_field_as_string -- moved to sinsp_thread_manager

The following user-facing changes require some action to be taken:

  • sinsp_threadinfo::get_parent_thread is removed (replace with sinsp_thread_manager::find_thread(tinfo->m_ptid)
  • sinsp_threadinfo::get_ancestor_process is replaced by sinsp_thread_manager::get_ancestor_process
  • sinsp_threadinfo::traverse_parent_state is replaced by sinsp_thread_manager::traverse_parent_state
  • sinsp_threadinfo::remove_child_from_parent is removed from the public API (no replacement)
  • sinsp_threadinfo::get_ancestor_field_as_string is replaced by sinsp_thread_manager::get_ancestor_field_as_string
  • sinsp_threadinfo::get_container_ip is replaced by plugin_tables::get_container_ip
  • sinsp_threadinfo::get_container_user is replaced by plugin_tables::get_container_user
  • sinsp_threadinfo::get_user, sinsp_threadinfo::get_group, and sinsp_threadinfo::get_loginuser are removed (replace with explicit calls to the sinsp_usergroup_manager::get_user and sinsp_usergroup_manager::get_group
  • sinsp_threadinfo::set_user, sinsp_threadinfo::set_group, and sinsp_threadinfo::set_loginuser are removed (replace with setting fields directly and explicit calls to sinsp_usergroup_manager::add_user and sinsp_usergroup_manager::add_group)
  • sinsp_user_group_manager::add_user and sinsp_user_group_manager::add_group gained new overloads and now require string literals to be replaced with explicit std::string_view objects
  • sinsp_threadinfo::get_container_id is replaced with plugin_tables::get_container_id

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

cleanup(sinsp)!: remove sinsp_threadinfo::get_parent_thread
cleanup(sinsp)!: move get_ancestor_process to the thread_manager
cleanup(sinsp)!: move traverse_parent_state to thread_manager
cleanup(sinsp)!: move remove_child_from_parent to thread_manager
cleanup(sinsp)!: move get_ancestor_field_as_string to thread_manager
cleanup(sinsp)!: remove sinsp_threadinfo->get_container_ip
cleanup(sinsp)!: remove users/groups handling from threadinfo
cleanup(sinsp)!: remove the last use of tinfo->get_container_id

@poiana poiana requested review from irozzo-1A and terror96 October 14, 2025 13:44
@poiana
Copy link
Copy Markdown
Contributor

poiana commented Oct 14, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gnosek

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@codecov
Copy link
Copy Markdown

codecov bot commented Oct 14, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 64.17910% with 120 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.90%. Comparing base (580267d) to head (2e3421e).
⚠️ Report is 63 commits behind head on master.

Files with missing lines Patch % Lines
userspace/libsinsp/plugin_tables.cpp 16.66% 35 Missing ⚠️
userspace/libsinsp/sinsp_filtercheck_thread.cpp 64.86% 13 Missing ⚠️
userspace/libsinsp/thread_manager.cpp 78.68% 13 Missing ⚠️
userspace/libsinsp/ifinfo.cpp 18.18% 9 Missing ⚠️
userspace/libsinsp/sinsp.cpp 78.78% 7 Missing ⚠️
userspace/libsinsp/sinsp_debug/sinsp_debug.cpp 0.00% 7 Missing ⚠️
userspace/libsinsp/sinsp_filtercheck_group.cpp 0.00% 7 Missing ⚠️
userspace/libsinsp/threadinfo.cpp 50.00% 7 Missing ⚠️
userspace/libsinsp/plugin_tables.h 14.28% 6 Missing ⚠️
userspace/libsinsp/sinsp_filtercheck_fd.cpp 45.45% 6 Missing ⚠️
... and 3 more
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #2689      +/-   ##
==========================================
- Coverage   76.92%   76.90%   -0.02%     
==========================================
  Files         294      296       +2     
  Lines       30856    30875      +19     
  Branches     4683     4693      +10     
==========================================
+ Hits        23736    23745       +9     
- Misses       7120     7130      +10
Flag Coverage Δ
libsinsp 76.90% <64.17%> (-0.02%) ⬇️

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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ekoops
Copy link
Copy Markdown
Contributor

ekoops commented Oct 14, 2025

Great! Thank you. I'll review it after 0.22.0 is released 😄
/milestone 0.23.0

@poiana poiana added this to the 0.23.0 milestone Oct 14, 2025
@gnosek
Copy link
Copy Markdown
Contributor Author

gnosek commented Oct 14, 2025

/hold until I figure out what's going on with the tests

mstemm
mstemm requested changes Oct 14, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@mstemm mstemm left a comment

Choose a reason for hiding this comment

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

Overall looks good. Just a question/comment about removing the setter methods.

@github-project-automation github-project-automation bot moved this from Todo to In progress in Falco Roadmap Oct 14, 2025
@gnosek gnosek force-pushed the threadinfo-api-cleanup branch from 05dcb6f to da1dce2 Compare October 16, 2025 09:01
@gnosek
Copy link
Copy Markdown
Contributor Author

gnosek commented Oct 16, 2025

Note: this now includes the other PR (#2694) but if we decide against merging that, I can take it out. I'd just rather resolve the merge conflicts once.

@gnosek gnosek force-pushed the threadinfo-api-cleanup branch 4 times, most recently from 53d19e0 to 45bba36 Compare October 17, 2025 10:07
@gnosek gnosek force-pushed the threadinfo-api-cleanup branch from 45bba36 to 9caa558 Compare October 24, 2025 08:33
@irozzo-1A irozzo-1A self-assigned this Oct 30, 2025
irozzo-1A
irozzo-1A reviewed Oct 31, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@irozzo-1A irozzo-1A left a comment

Choose a reason for hiding this comment

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

LGTM overall, just a few comments. I addressed some of those myself as we discussed offline.

@gnosek gnosek force-pushed the threadinfo-api-cleanup branch from b0e00fa to e492c73 Compare November 5, 2025 14:47
irozzo-1A
irozzo-1A reviewed Nov 6, 2025
View reviewed changes
irozzo-1A
irozzo-1A reviewed Nov 6, 2025
View reviewed changes
@irozzo-1A
Copy link
Copy Markdown
Contributor

irozzo-1A commented Nov 6, 2025

I would suggest to squash e492c73 with 1c5fdf6 too

@irozzo-1A irozzo-1A requested review from irozzo-1A and mstemm November 6, 2025 09:34
@irozzo-1A irozzo-1A force-pushed the threadinfo-api-cleanup branch from e492c73 to 2120c62 Compare November 7, 2025 14:14
irozzo-1A
irozzo-1A approved these changes Nov 7, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@irozzo-1A irozzo-1A left a comment

Choose a reason for hiding this comment

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

LGTM

@poiana poiana added the lgtm label Nov 7, 2025
@poiana
Copy link
Copy Markdown
Contributor

poiana commented Nov 7, 2025

LGTM label has been added.

DetailsGit tree hash: 5a18d8ec828ca270176699532f704062dc0874db

@gnosek
Copy link
Copy Markdown
Contributor Author

gnosek commented Nov 11, 2025

/hold

gnosek and others added 10 commits November 11, 2025 13:55
@gnosek
cleanup(sinsp)!: remove sinsp_threadinfo::get_parent_thread
19e9d3f 
An individual thread should not know or care about the existence
of a thread manager.

Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>
@gnosek
cleanup(sinsp)!: move get_ancestor_process to the thread_manager
a31aa5a 
This code is used only by filterchecks and does not belong
in the threadinfo itself.

Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>
@gnosek
cleanup(sinsp)!: move traverse_parent_state to thread_manager
137b489 
A threadinfo should not need to know its position in the hierarchy

Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>
@gnosek
cleanup(sinsp)!: move remove_child_from_parent to thread_manager
8ce4ea8 
This is effectively a helper for internal thread_manager code

Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>
@gnosek
cleanup(sinsp)!: move get_ancestor_field_as_string to thread_manager
a4e86f4 
A threadinfo should not need to know its place in the hierarchy

Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>
@gnosek @irozzo-1A
new(sinsp): introduce plugin_tables class
91cddc6 
This class holds all data related to accessing plugin-defined fields
and tables from libsinsp itself. None of this is the responsibility
of e.g. the thread_manager or individual threadinfos, but it has to live
somewhere. Rather than put it in the `sinsp` kitchen sink, move it all
to a separate class.

Note: at this point we still keep the accessors available through
thread_manager/threadinfo, they will be cleaned up later.

Co-Authored-By: Iacopo Rozzo <iacopo@sysdig.com>
Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>
@gnosek
cleanup(sinsp)!: remove sinsp_threadinfo->get_container_ip
2978409 
Instead, use plugin_tables to access the plugin-defined field

Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>
@gnosek @irozzo-1A
cleanup(sinsp)!: remove users/groups handling from threadinfo
4da2537 
Instead, the threadinfo is now responsible only for storing uids
and gids. User/group name handling is now done only in filterchecks.

Co-Authored-By: Iacopo Rozzo <iacopo@sysdig.com>
Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>
@gnosek
cleanup(sinsp)!: remove the last use of tinfo->get_container_id
0e63cd1 
Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>
@gnosek
cleanup(sinsp): remove old plugin table access code
2e3421e 
Also, finally remove all traces of usergroup_manager and thread_manager
from threadinfo

Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>
@gnosek gnosek force-pushed the threadinfo-api-cleanup branch from 2120c62 to 2e3421e Compare November 11, 2025 13:35
@gnosek
Copy link
Copy Markdown
Contributor Author

gnosek commented Nov 11, 2025

/unhold

@poiana poiana merged commit a8fb3d2 into falcosecurity:master Nov 11, 2025
44 of 47 checks passed
@github-project-automation github-project-automation bot moved this from In progress to Done in Falco Roadmap Nov 11, 2025
@BrewTestBot BrewTestBot mentioned this pull request Dec 23, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mstemm mstemm mstemm approved these changes

@irozzo-1A irozzo-1A irozzo-1A approved these changes

@terror96 terror96 Awaiting requested review from terror96

Assignees

@mstemm mstemm

@irozzo-1A irozzo-1A

Labels

approved area/libsinsp dco-signoff: yes kind/cleanup lgtm release-note size/XXL

Projects

Falco Roadmap
Status: Done

Milestone

0.23.0

Development

Successfully merging this pull request may close these issues.

5 participants

@gnosek @poiana @ekoops @irozzo-1A @mstemm