Skip to content

Cgroup #164

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

Merged
merged 2 commits into from
Jun 11, 2025
Merged

Cgroup #164

merged 2 commits into from
Jun 11, 2025

Conversation

haesbaert
Copy link
Collaborator

@haesbaert haesbaert commented May 6, 2025

Add cgroups to quark_process{}
Only for EBPF atm, basically the same dance we do other paths.
Currently we replace the existing cgroup with the new one for every event, but
likely this is only needed at fork, consider changing this in the future.

+++

Signal root cgroup as /, not NUL
Before this change if a process was in the root cgroup, we would get a zero
length string. If an error ocurred it would also return a zero length string.

Change it so that a successful case of a root process actually sends "/" and not
a zero length string, this way quark can know if it should signal it knows the
cgroup path or not.

@haesbaert haesbaert force-pushed the cgroup branch 2 times, most recently from b5fa6c6 to ca49311 Compare June 10, 2025 16:57
@haesbaert haesbaert marked this pull request as ready for review June 10, 2025 16:59
@haesbaert haesbaert requested a review from a team as a code owner June 10, 2025 16:59
@haesbaert haesbaert marked this pull request as draft June 10, 2025 17:09
@haesbaert haesbaert force-pushed the cgroup branch 2 times, most recently from 34f4add to 59522b4 Compare June 10, 2025 17:39
Before this change if a process was in the root cgroup, we would get a zero
length string. If an error ocurred it would also return a zero length string.

Change it so that a successful case of a root process actually sends "/" and not
a zero length string, this way quark can know if it should signal it knows the
cgroup path or not.
@haesbaert haesbaert force-pushed the cgroup branch 2 times, most recently from fcc8407 to f0bcbfa Compare June 10, 2025 18:15
@haesbaert haesbaert marked this pull request as ready for review June 10, 2025 20:46
@nicholasberlin nicholasberlin requested a review from Copilot June 11, 2025 13:07
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for cgroups in quark_process for EBPF, ensuring that the root cgroup is signaled as "/" rather than as a zero-length string.

  • Updates the data structures (quark_process, raw_task) to include a new cgroup field.
  • Implements and integrates a new cgroup processing function (sproc_cgroup) and updates event handling and tests accordingly.
  • Adjusts documentation and helper functions (e.g., in PathResolver.h and bpf_queue.c) to support proper cgroup resolution.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
qutil.c Added a helper function comment for load_file_nostat
quark_queue_get_event.3 Documented the new cgroup field in the event format
quark.h Updated structs and flag definitions with cgroup support
quark.c Added cgroup processing logic, updated free routines, and event flag handling for cgroup
quark-test.c Updated tests and introduced an openfmt helper for cgroup testing
elastic-ebpf/GPL/Events/PathResolver.h Modified path resolution to signal root cgroup as "/"
bpf_queue.c Integrated cgroup extraction from EBPF events into the task structure
Comments suppressed due to low confidence (1)

quark-test.c:602

  • [nitpick] Consider using the same variable name (e.g., 'cwd') for the current working directory as used elsewhere in the code to maintain consistency and avoid potential confusion.
char path[PATH_MAX];

Only for EBPF atm, basically the same dance we do other paths.
Currently we replace the existing cgroup with the new one for every event, but
likely this is only needed at fork, consider changing this in the future.

Co-authored-by: Nicholas Berlin <[email protected]>
@haesbaert haesbaert merged commit 414cd3f into main Jun 11, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants