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

Add test to ensure all profiler-spawned processes go down if gProfiler is brutally killed #780

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions tests/test_executable.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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(
Copy link
Contributor

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 (like test_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.

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"""
Copy link
Contributor

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

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

This means to ensure that part?

The test should ensure that gProfiler was indeed brutally killed - i.e, no cleanups were performed, the process exited with SIGKILL exit code, the temporary directory at /tmp/gprofiler_tmp/tmpxxxxx remains on the filesystem.

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 ls: cannot access '/tmp/....': no such file... and your assert still passes, won't it?

Should check either for the exact directory /tmp/gprofiler_tmp/tmpxxxx OR for the unpacked staticx executable (which is removed after gprofiler exits gracefully, but not when terminated). I think checking the directory is nicer, but the exact one. You can find it by running ls earlier and finding it.