remove any pipe files before zip/unzipping #1133
Conversation
ansible_runner/utils/streaming.py
Outdated
| if stat.S_ISFIFO(os.stat(full_path).st_mode): | ||
| # must remove any pipes before reading | ||
| # i.e. ssh_key_data that was never cleaned up | ||
| os.remove(full_path) |
There was a problem hiding this comment.
Any reason we need to actually delete the fifo instead of just skipping it for the zip? Neither the stream nor unstream directories are guaranteed to be exclusively "ours", so who knows what might be laying around in there that something else is relying on...
There was a problem hiding this comment.
changed so that it skips instead of removes. We still remove in the unstream_dir though.
| if stat.S_ISFIFO(os.stat(out_path).st_mode): | ||
| # must remove any pipes before reading | ||
| # i.e. ssh_key_data that was never cleaned up | ||
| os.remove(out_path) |
There was a problem hiding this comment.
This one makes more sense to delete as a safety, esp since it will only trigger when the archive contains a file with the same name/path- seems a lot less likely to stomp an arbitrary socket file laying around that might be in use...
4f4fa04 to
d21629d
Compare
ansible_runner/utils/streaming.py
Outdated
| @@ -22,6 +22,11 @@ def stream_dir(source_directory, stream): | |||
| relpath = "" | |||
| for fname in files + dirs: | |||
| full_path = os.path.join(dirpath, fname) | |||
| if stat.S_ISFIFO(os.stat(full_path).st_mode): | |||
There was a problem hiding this comment.
Could you please add a test for this change?
|
I am a little worried about performance of running python stat on every file in a large repo, and it might be good to characterize the time cost of doing that compared to the total cost of extracting the zip data. |
|
to-do: move logic below the symlink checks, otherwise we get a file not found error |
|
https://gist.github.com/AlanCoding/90708be38abab46d594ee18a729e86f2 Initial data: Idea here is that this PR adds the "stat" computation. The symlink check (in addition to being necessary for this work) is pre-existing. The added time from the stat comes to Compared to the total streaming time, that's like |
|
Here are 2 proposed tests for the cases being fixed here. Obviously, I confirmed that they fail in devel, those are the failures I give in that link. I have not confirmed that your work here makes them pass, but obviously, if the fix works, it should. |
- Prevents hangs when zipping, unzipping artifacts dir. If python attempts to open a fifo pipe, it will block indefinitely until it can read data from the pipe.
fd0edfa to
df3582d
Compare
Add tests that fail in devel on timeout due to pipe hang
|
Looks good, we did integration testing with AWX as well and that came back looking fine. |
* Write test to demonstrate job hang on pipes * Remove any pipe files before zip/unzipping - Prevents hangs when zipping, unzipping artifacts dir. If python attempts to open a fifo pipe, it will block indefinitely until it can read data from the pipe. * move fifo check below symlink Co-authored-by: Alan Rominger <arominge@redhat.com> (cherry picked from commit 8f39752)
* Write test to demonstrate job hang on pipes * Remove any pipe files before zip/unzipping - Prevents hangs when zipping, unzipping artifacts dir. If python attempts to open a fifo pipe, it will block indefinitely until it can read data from the pipe. * move fifo check below symlink Co-authored-by: Alan Rominger <arominge@redhat.com> (cherry picked from commit 8f39752) Co-authored-by: Seth Foster <fosterseth@users.noreply.github.com>
We are seeing cases where unstream_dir() is hanging when attempting to read
ssh_key_data. This file is supposed to be cleaned up by this point, but maybe not if things go awry.This change just looks for files that are type "pipe" and removes them.
To test I changed the "rm" command here to "ls", so the pipe is never removed.
https://github.com/ansible/ansible-runner/blob/devel/ansible_runner/config/_base.py#L612
Project updates can still succeed, no hangs.