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

Conversation

mpozniak95
Copy link
Collaborator

Description

Test checks if py-spy will go down if gprofiler processes are brutally killed.

Related Issue

#674

@mpozniak95 mpozniak95 requested a review from Jongy April 24, 2023 10:52
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.

@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.

Comment on lines +192 to +194
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()
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.

Copy link
Contributor

@Jongy Jongy left a 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.

@Jongy
Copy link
Contributor

Jongy commented May 25, 2023

Marking this draft until #790 is merged.

@Jongy Jongy marked this pull request as draft May 25, 2023 16:51
@Jongy
Copy link
Contributor

Jongy commented Aug 11, 2023

With #790 merged, this can be revived @mpozniak95 if you have the time for it.

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.

2 participants