-
Notifications
You must be signed in to change notification settings - Fork 60
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
base: master
Are you sure you want to change the base?
Conversation
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 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.
@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( |
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 (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.
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() |
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 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.
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.
Gave some comments but see the main one - I think we should wait with it.
Marking this draft until #790 is merged. |
With #790 merged, this can be revived @mpozniak95 if you have the time for it. |
Description
Test checks if py-spy will go down if gprofiler processes are brutally killed.
Related Issue
#674