-
Notifications
You must be signed in to change notification settings - Fork 58
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
Run gprofiler without root/sudo #936
base: master
Are you sure you want to change the base?
Conversation
…exit. Added run_in_ns_wrapper to only run in namespace when root is detected. Updated pgrep_maps to provide parameter that ignores permission errors when not root.
…er_tmp directory. Added pids_to_process to discover_appropriate_perf_event(), so it will not error out on perf record -a while not root. Changed TEMPORARY_STORAGE_PATH to the resources directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file defines is_root
, that you've moved to granulate-utils. Remove it here and use the copy from granulate-utils? We avoid maintaining 2 copies.
gprofiler/utils/__init__.py
Outdated
@@ -82,6 +77,13 @@ def resource_path(relative_path: str = "") -> str: | |||
raise Exception(f"Resource {relative_path!r} not found!") from e | |||
|
|||
|
|||
TEMPORARY_STORAGE_PATH = ( | |||
f"{resource_path(GPROFILER_DIRECTORY_NAME)}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/tmp
is writable for non-root users, and it's best practice to keep our temporary files there. Plus, this path is used as the prefix for async-profiler paths, for which we should keep the path constant across runs of gProfiler - and by using resource_path
, you cause it to change.
You might've hit permission errors because you've already ran gProfiler as root, so /tmp/gprofiler_tmp
is owned by root now. If you remove that directory, then for a non-root process it would go fine to create a /tmp/gprofiler_tmp
directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that mkdir_owned_user expects the parent directory to be owned by the user, which /tmp is not. I can alter mkdir_owned_user to expect the parent to be owned by either the user or root.
Ahh I see this is made irrelevant by your later comment that mkdir_owned_user is not necessary.
I've changed this and it now works fine in /tmp as non-root
gprofiler/utils/__init__.py
Outdated
and ( | ||
line.endswith(b"/maps: No such file or directory") | ||
or line.endswith(b"/maps: No such process") | ||
or b"Permission denied" in line |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Please make the check more strict, e.g
b"/maps: Permission denied" in line
, like previous checks - You're not handling
ignore_permission_errors
here?
gprofiler/profilers/ruby.py
Outdated
if is_root(): | ||
ignore_permission_errors = False | ||
else: | ||
ignore_permission_errors = True | ||
return pgrep_maps(self.DETECTED_RUBY_PROCESSES_REGEX, ignore_permission_errors) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much more concise:
if is_root(): | |
ignore_permission_errors = False | |
else: | |
ignore_permission_errors = True | |
return pgrep_maps(self.DETECTED_RUBY_PROCESSES_REGEX, ignore_permission_errors) | |
return pgrep_maps(self.DETECTED_RUBY_PROCESSES_REGEX, ignore_permission_errors=not is_root()) |
gprofiler/utils/__init__.py
Outdated
@@ -351,7 +353,7 @@ def pgrep_exe(match: str) -> List[Process]: | |||
return procs | |||
|
|||
|
|||
def pgrep_maps(match: str) -> List[Process]: | |||
def pgrep_maps(match: str, ignore_permission_errors: bool = False) -> List[Process]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove this parameter, and instead - call is_root
here to decide (instead of having all callers forced to call this and pass as an argument)
gprofiler/utils/fs.py
Outdated
return statbuf.st_uid == os.geteuid() and statbuf.st_gid == os.getegid() | ||
|
||
|
||
def mkdir_owned_user(path: Union[str, Path], mode: int = 0o755) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This behavior is not needed when we're not root. So - either add a wrapper, which checks if we're root and calls it OR simply mkdir
s, or if there's 1-2 calls sites, add such if
directly:
if is_root():
mkdir_owned_root(...)
else:
mkdir(...)
@@ -95,7 +99,7 @@ def discover_appropriate_perf_event(tmp_dir: Path, stop_event: Event) -> Support | |||
is_dwarf=False, | |||
inject_jit=False, | |||
extra_args=current_extra_args, | |||
processes_to_profile=None, | |||
processes_to_profile=pids, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it mean anything if we pass only specific PIDs here, permission wise?
I prefer that the rootless mode will be opt-in, and not a default in case of non-root user (which allows misconfigurations to continue misusing the profiler).
I left some comments on the PR about this topic.
Not sure I got you here.
Yeah, it's fine - as I commented on granulate-utils, proc_events are expected to fail in rootless. |
… is_root function (now in granulate-utils). Added mkdir_owned_root_wrapper. Moved TEMPORARY_STORAGE_PATH back to /tmp. pgrep_maps root check now inside function
I added a commit that should address all of your comments, but let me know if there is anything I should change. In particular with the new --rootless option and in mkdir_owned_root_wrapper. |
Description
This PR adds support for running gprofiler without root/sudo as discussed in issue 905. There are several assumptions and components that I will mention here.
This PR requires a change in the granulate-utils repo that defines the run_in_ns_wrapper function found in PR 265 on that repo Granulate/granulate-utils#265.
Assumptions when running without root:
Components:
Potential issues:
Related Issue
#905
Motivation and Context
Users of this gprofiler have requested this feature as some cloud instances do not have root access, but still want to profile user owned processes.
How Has This Been Tested?
I ran stress-ng and targeted gprofiler to the stress-ng pids without sudo. It successfully produced flamegraphs
Sample command line: ./build/x86_64/gprofiler --pids 1421864 -o results/ -d 15 --log-file ./gprofiler.log --pid-file ./gprofiler.pid
I have tested this on x86 using scripts/build_x86_64_executable.sh script. Centos 9 Stream w/ kernel 6.6
Also tested using sudo targeting specific pid(s) and system-wide, and it still works.
Was not able to run tests/test.sh as it required apt-get/debian environment.
Screenshots
Checklist:
The code is linted.
I have not updated the README.md doc here. Might need some guidance.