Skip to content
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

Added run_in_ns_wrapper to only run in namespace when root is detected. #265

Merged
merged 3 commits into from
Jan 29, 2025

Conversation

pcarella1
Copy link
Contributor

@pcarella1 pcarella1 commented Dec 5, 2024

This PR adds support for running gprofiler without root/sudo as discussed in issue 905 in the gprofiler repo: intel/gprofiler#905.

It creates run_in_ns_wrapper function which bypasses the code to enter name spaces when not root (as we assume we're always in the correct namespace for the processes being profiled).

This commit is required for PR 936 in the gprofiler repo intel/gprofiler#936.

I have tested this on x86 using scripts/build_x86_64_executable.sh script. Centos 9 Stream w/ kernel 6.6, though with some limitations mentioned in the gprofiler PR.

The code is linted.

One issue is when running on one system, I receive the below warning (though gprofiler completes and produces valid results)

[2024-12-02 19:59:55,557] WARNING: gprofiler.profilers.java: Failed to enable proc_events listener for exited Java processes (this does not prevent Java profiling)
Traceback (most recent call last):
File "granulate_utils/linux/proc_events.py", line 222, in start
PermissionError: [Errno 1] Operation not permitted

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
File "gprofiler/profilers/java.py", line 1395, in start
proc_events.register_exit_callback(self._proc_exit_callback)
File "granulate_utils/linux/proc_events.py", line 272, in wrapper
File "granulate_utils/linux/ns.py", line 305, in run_in_ns_wrapper
File "granulate_utils/linux/ns.py", line 299, in run_in_ns_wrapper
File "granulate_utils/linux/proc_events.py", line 260, in _start_listener
File "granulate_utils/linux/proc_events.py", line 225, in start
PermissionError: This process doesn't have permissions to bind/connect to the process events connector

Issue seems to be that this bind call in the threading library requires root. Is this because the read we're trying to bind to is owned by root or does the function just inherently require root? I haven't been able to diagnose this.
https://github.com/Granulate/granulate-utils/blob/0cb9b70f4eda85f0a14e86ec41ecc7b57a133755/granulate_utils/linux/proc_events.py#L222

@Jongy Jongy marked this pull request as ready for review December 10, 2024 16:33
Jongy
Jongy previously requested changes Dec 10, 2024
granulate_utils/linux/proc_events.py Outdated Show resolved Hide resolved
granulate_utils/linux/ns.py Outdated Show resolved Hide resolved
granulate_utils/linux/ns.py Outdated Show resolved Hide resolved
granulate_utils/linux/ns.py Outdated Show resolved Hide resolved
…. Simplified run_in_ns_wrapper and removed it from proc_events.
@pcarella1
Copy link
Contributor Author

I added a new commit that addresses your comments. Biggest change is moving is_root into granulate-utils. Is ns.py the right place to add it? That's where it gets used.

granulate_utils/linux/ns.py Outdated Show resolved Hide resolved
@sobolron
Copy link

I added a new commit that addresses your comments. Biggest change is moving is_root into granulate-utils. Is ns.py the right place to add it? That's where it gets used.

Wouldn't create another file just for this and I don't think any other file fits better. Leave it like this.

@sobolron sobolron dismissed Jongy’s stale review January 19, 2025 12:00

Replacing him in this PR.

@sobolron
Copy link

@pcarella1 When it's ready please also move to ready to review so CI will run and we'll see tests.

@sobolron sobolron merged commit dd8aa29 into intel:master Jan 29, 2025
7 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.

3 participants