[EBPF-595] Create a common process consumer for eventmonitor #30559

gjulianm · 2024-10-28T17:00:33Z

What does this PR do?

This PR adds a ProcessConsumer type that can be directly used by callers interested just in exec/exit callbacks without having to manually implement all the necessary interfaces.

Motivation

Removing coupling between UprobeAttacher and ProcessMonitor, which caused problems with the process monitor initialization being called in multiple places. This PR allows GPU and DI module to use a consumer for the event monitor stream quickly, without having to copy-paste existing code.

Describe how to test/QA your changes

Possible Drawbacks / Trade-offs

Additional Notes

Related PR: #30554

cit-pr-commenter · 2024-10-28T19:45:25Z

Regression Detector

agent-platform-auto-pr · 2024-10-29T14:10:26Z

[Fast Unit Tests Report]

On pipeline 48141503 (CI Visibility). The following jobs did not run any unit tests:

Jobs:

tests_deb-arm64-py3
tests_deb-x64-py3
tests_flavor_dogstatsd_deb-x64
tests_flavor_heroku_deb-x64
tests_flavor_iot_deb-x64
tests_rpm-arm64-py3
tests_rpm-x64-py3
tests_windows-x64

If you modified Go files and expected unit tests to run in these jobs, please double check the job logs. If you think tests should have been executed reach out to #agent-devx-help

val06

reviewed

pkg/eventmonitor/consumers/doc.go

pkg/eventmonitor/consumers/process.go

val06 · 2024-10-29T14:18:16Z

pkg/eventmonitor/consumers/process.go

+	callbacks map[*ProcessCallback]struct{}
+
+	// mutex is the mutex that protects the callbacks map
+	mutex sync.RWMutex


what is the common pattern to use this struct?
in some cases rwmutex perform worse than the regular mutex

I took the same pattern as there is in process-monitor, I assumed it was tested to be the best option.

cc @guyarb @vitkyrka

common use case - adding all callbacks (write lock), and then calling them (read lock)

normally, all callbacks are being added (or removed) before (or after) the operation.
We can use regular mutex here, but, if we do intend to make the change - it should be tested to show it improves the usage of rwmutex

me and @brycekahle discussed rwmutex in some other context. and in some cases the rwmutex actually creates slight overhead (even when just using the readerlock without writer lock).

I don't think we can do that, we're always going to (potentially) have two goroutines accessing the callback map concurrently, one from the event stream and another one from the subscription.

do we have any subscriptions happening in "execution" phase or rather we can limit the subscription during the "init" phase before the handler go-routine starts?

also these benchmarks for reference

(maybe sync.Map is a better choice)

It might not always be possible to limit the subscriptions, specially considering that the current way of adding event stream consumers requires having a global variable to share the consumer, and that could probably change later. Also, it'd require changing the SubscribeX signatures to be able to return an error.

I think it's a good idea to research this a bit more, but I think we should have a better view of dependencies in system-probe modules first to see how that fits with those phases.

i agree that we can leave it out of scope for this PR. Resolving the comment

pkg/eventmonitor/consumers/process.go

agent-platform-auto-pr · 2024-10-29T14:52:26Z

Test changes on VM

Use this command from test-infra-definitions to manually test this PR changes on a VM:

inv create-vm --pipeline-id=48141503 --os-family=ubuntu

Note: This applies to commit d632825

pkg/eventmonitor/consumers/testutil/testutil.go

val06

reviewed

pkg/eventmonitor/consumers/process.go

val06

reviewed

pkg/eventmonitor/consumers/process.go

gjulianm · 2024-11-04T16:01:56Z

/merge

dd-devflow · 2024-11-04T16:02:12Z

🚂 MergeQueue: pull request added to the queue

The median merge time in main is 22m.

Use /merge -c to cancel this operation!

gjulianm force-pushed the guillermo.julian/eventmonitor-common-process-consumer branch 2 times, most recently from ed95e39 to fd3481e Compare October 28, 2024 18:10

Base automatically changed from valeri.pliskin/eventmonitor-rename-interfaces to main October 29, 2024 12:38

gjulianm force-pushed the guillermo.julian/eventmonitor-common-process-consumer branch from fd3481e to 0b2b370 Compare October 29, 2024 13:41

github-actions bot added component/system-probe team/ebpf-platform team/agent-security medium review PR review might take time labels Oct 29, 2024

gjulianm force-pushed the guillermo.julian/eventmonitor-common-process-consumer branch from 0b2b370 to 1a38ab4 Compare October 29, 2024 13:52

gjulianm marked this pull request as ready for review October 29, 2024 13:52

gjulianm requested review from a team as code owners October 29, 2024 13:52

gjulianm added changelog/no-changelog qa/done Skip QA week as QA was done before merge and regressions are covered by tests labels Oct 29, 2024

Add common process consumer

52219d7

gjulianm force-pushed the guillermo.julian/eventmonitor-common-process-consumer branch from 1a38ab4 to 52219d7 Compare October 29, 2024 14:08

val06 requested changes Oct 29, 2024

View reviewed changes

pkg/eventmonitor/consumers/doc.go Outdated Show resolved Hide resolved

pkg/eventmonitor/consumers/doc.go Show resolved Hide resolved

pkg/eventmonitor/consumers/process.go Show resolved Hide resolved

pkg/eventmonitor/consumers/process.go Show resolved Hide resolved

val06 reviewed Oct 29, 2024

View reviewed changes

Fix build tags

1165d41

val06 reviewed Oct 29, 2024

View reviewed changes

pkg/eventmonitor/consumers/process.go Show resolved Hide resolved

github-actions bot added long review PR is complex, plan time to review it and removed medium review PR review might take time labels Oct 29, 2024

val06 reviewed Oct 30, 2024

View reviewed changes

pkg/eventmonitor/consumers/testutil/testutil.go Outdated Show resolved Hide resolved

gjulianm added 3 commits October 30, 2024 11:38

Rename consumers -> consumer

a1d8614

ADd support for fork events

bf5dc6a

Revert rename

8b07947

paulcacheux approved these changes Oct 30, 2024

View reviewed changes

val06 requested changes Oct 31, 2024

View reviewed changes

pkg/eventmonitor/consumers/process.go Outdated Show resolved Hide resolved

pkg/eventmonitor/consumers/process.go Outdated Show resolved Hide resolved

pkg/eventmonitor/consumers/process.go Outdated Show resolved Hide resolved

gjulianm added 4 commits October 31, 2024 12:50

Fix package name

26768ee

Allow generic event types

e6ef65f

Fix testutil

35be299

Enable callback maps only if the type is enabled

99baa4e

gjulianm changed the title ~~[EBPF-594] Create a common process consumer for eventmonitor~~ [EBPF-595] Create a common process consumer for eventmonitor Nov 4, 2024

val06 requested changes Nov 4, 2024

View reviewed changes

pkg/eventmonitor/consumers/process.go Outdated Show resolved Hide resolved

val06 reviewed Nov 4, 2024

View reviewed changes

pkg/eventmonitor/consumers/process.go Outdated Show resolved Hide resolved

val06 reviewed Nov 4, 2024

View reviewed changes

pkg/eventmonitor/consumers/process.go Outdated Show resolved Hide resolved

val06 reviewed Nov 4, 2024

View reviewed changes

pkg/eventmonitor/consumers/process.go Outdated Show resolved Hide resolved

subscriptors -> subscribers

d632825

val06 approved these changes Nov 4, 2024

View reviewed changes

dd-mergequeue bot merged commit 8803d71 into main Nov 4, 2024
221 checks passed

dd-mergequeue bot deleted the guillermo.julian/eventmonitor-common-process-consumer branch November 4, 2024 16:21

github-actions bot added this to the 7.61.0 milestone Nov 4, 2024

gjulianm mentioned this pull request Nov 8, 2024

[EBPF-616] gpu: Use event consumer for processs monitoring #30755

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EBPF-595] Create a common process consumer for eventmonitor #30559

[EBPF-595] Create a common process consumer for eventmonitor #30559

gjulianm commented Oct 28, 2024 •

edited

Loading

cit-pr-commenter bot commented Oct 28, 2024 •

edited

Loading

agent-platform-auto-pr bot commented Oct 29, 2024 •

edited

Loading

val06 left a comment

val06 Oct 29, 2024

gjulianm Oct 29, 2024

val06 Oct 30, 2024

guyarb Oct 30, 2024

val06 Oct 30, 2024

gjulianm Oct 31, 2024

val06 Oct 31, 2024 •

edited

Loading

val06 Oct 31, 2024 •

edited

Loading

gjulianm Oct 31, 2024

val06 Nov 4, 2024

agent-platform-auto-pr bot commented Oct 29, 2024 •

edited

Loading

val06 left a comment

val06 left a comment

gjulianm commented Nov 4, 2024

dd-devflow bot commented Nov 4, 2024

[EBPF-595] Create a common process consumer for eventmonitor #30559

[EBPF-595] Create a common process consumer for eventmonitor #30559

Conversation

gjulianm commented Oct 28, 2024 • edited Loading

What does this PR do?

Motivation

Describe how to test/QA your changes

Possible Drawbacks / Trade-offs

Additional Notes

cit-pr-commenter bot commented Oct 28, 2024 • edited Loading

Regression Detector

agent-platform-auto-pr bot commented Oct 29, 2024 • edited Loading

val06 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

val06 Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

val06 Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agent-platform-auto-pr bot commented Oct 29, 2024 • edited Loading

Test changes on VM

val06 left a comment

Choose a reason for hiding this comment

val06 left a comment

Choose a reason for hiding this comment

gjulianm commented Nov 4, 2024

dd-devflow bot commented Nov 4, 2024

gjulianm commented Oct 28, 2024 •

edited

Loading

cit-pr-commenter bot commented Oct 28, 2024 •

edited

Loading

agent-platform-auto-pr bot commented Oct 29, 2024 •

edited

Loading

val06 Oct 31, 2024 •

edited

Loading

val06 Oct 31, 2024 •

edited

Loading

agent-platform-auto-pr bot commented Oct 29, 2024 •

edited

Loading