-
Notifications
You must be signed in to change notification settings - Fork 63
Add test to ensure all profiler-spawned processes go down if gProfiler is brutally killed #780
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
base: master
Are you sure you want to change the base?
Changes from all commits
32afe79
0ab781b
0c6dbf1
b5bd37b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,15 +4,18 @@ | |
# | ||
import os | ||
import shutil | ||
import signal | ||
from pathlib import Path | ||
from subprocess import Popen | ||
from threading import Event | ||
from typing import Callable, List, Mapping, Optional | ||
|
||
import pytest | ||
from docker import DockerClient | ||
from docker.models.containers import Container | ||
from docker.models.images import Image | ||
|
||
from gprofiler.utils import wait_event | ||
from gprofiler.utils.collapsed_format import parse_one_collapsed | ||
from tests.utils import RUNTIME_PROFILERS, _no_errors, is_aarch64, run_gprofiler_in_container | ||
|
||
|
@@ -137,3 +140,55 @@ def test_executable_not_privileged( | |
|
||
collapsed = parse_one_collapsed(Path(output_directory / "last_profile.col").read_text()) | ||
assert_collapsed(collapsed) | ||
|
||
|
||
@pytest.mark.parametrize( | ||
"runtime,profiler_type", | ||
[ | ||
("python", "py-spy"), | ||
], | ||
) | ||
@pytest.mark.parametrize("in_container", [True]) | ||
@pytest.mark.parametrize("application_docker_mount", [True]) | ||
@pytest.mark.parametrize("application_docker_capabilities", ["SYS_PTRACE"]) | ||
def test_killing_spawned_processes( | ||
gprofiler_exe: Path, | ||
application_docker_container: Container, | ||
runtime_specific_args: List[str], | ||
output_directory: Path, | ||
profiler_flags: List[str], | ||
application_docker_mount: bool, | ||
) -> None: | ||
"""Tests if killing gprofiler with -9 results in killing py-spy""" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd give a link to the issue #674 here which explains a bit more about the reasoning. |
||
os.makedirs(str(output_directory), mode=0o755, exist_ok=True) | ||
|
||
mount_gprofiler_exe = str(output_directory / "gprofiler") | ||
if not os.path.exists(mount_gprofiler_exe): | ||
shutil.copy(str(gprofiler_exe), mount_gprofiler_exe) | ||
|
||
command = ( | ||
[ | ||
mount_gprofiler_exe, | ||
"-v", | ||
"--output-dir", | ||
str(output_directory), | ||
"--disable-pidns-check", | ||
"--no-perf", | ||
] | ||
+ runtime_specific_args | ||
+ profiler_flags | ||
) | ||
application_docker_container.exec_run(cmd=command, privileged=True, user="root:root", detach=True) | ||
wait_event(30, Event(), lambda: "py-spy record" in str(application_docker_container.top().get("Processes"))) | ||
processes_in_container = application_docker_container.top().get("Processes") | ||
gprofiler_pids = [process[1] for process in processes_in_container if "disable-pidns-check" in process[-1]] | ||
for pid in gprofiler_pids: | ||
os.kill(int(pid), signal.SIGKILL) | ||
processes_in_container = application_docker_container.top().get("Processes") | ||
processes_in_container = [process for process in processes_in_container if "<defunct>" not in process[-1]] | ||
print(f"Processes left in container: {processes_in_container}") | ||
assert len(processes_in_container) == 1 | ||
assert "py-spy record" not in str(processes_in_container) | ||
command = ["ls", "/tmp/gprofiler_tmp"] | ||
e, ls_output = application_docker_container.exec_run(cmd=command, privileged=True, user="root:root", detach=False) | ||
assert "tmp" in ls_output.decode() | ||
Comment on lines
+192
to
+194
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This means to ensure that part?
Please add a comment, the purpose isn't clear. Also the test can be made more robust, this is not enough - even if the directory is missing, output will be Should check either for the exact 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.
Note that
test_executable.py
runs in a matrix for different images, while this one we actually need only one time (liketest_sanity.py
f.e).The problem is, the workflow of executable tests has the exe, and the workflow of the other tests (like
test_sanity.py
) don't have it. We can do the same testing with the container version, but it's more work.I think, since we're planning on doing #582 soon, it makes more sense to merge it nad then it'll be easier to write this test.