From 638b196a4538926c125b9258db68c537a3098ee3 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Mon, 10 Sep 2018 19:05:27 +0100 Subject: [PATCH] ansible: fix put_file() for large temporary files. Reverts 49736b3a, large file copies can't avoid the RTT. The parent stack must be blocked while FileService progresses, as unlike the small file path, it does not make a snapshot of the (possibly temporary) file passed by the action plug-in. So we need to keep that file alive while the service runs. Add a new integration test and a new soak test to cover both. --- ansible_mitogen/connection.py | 12 ++--- docs/changelog.rst | 5 -- tests/ansible/integration/action/all.yml | 1 + tests/ansible/integration/action/copy.yml | 66 +++++++++++++++++++++++ tests/ansible/soak/_file_service_loop.yml | 6 +++ tests/ansible/soak/file_service.yml | 6 +++ 6 files changed, 84 insertions(+), 12 deletions(-) create mode 100644 tests/ansible/integration/action/copy.yml create mode 100644 tests/ansible/soak/_file_service_loop.yml create mode 100644 tests/ansible/soak/file_service.yml diff --git a/ansible_mitogen/connection.py b/ansible_mitogen/connection.py index c5c96f24..12626485 100644 --- a/ansible_mitogen/connection.py +++ b/ansible_mitogen/connection.py @@ -880,13 +880,11 @@ class Connection(ansible.plugins.connection.ConnectionBase): path=mitogen.utils.cast(in_path) ) - # A roundtrip is always necessary for the target to request the file - # from FileService, however, by pipelining the transfer function, the - # subsequent step (probably a module invocation) can get its - # dependencies and function call in-flight before the transfer is - # complete. This saves at least 1 RTT between the transfer completing - # and the start of the follow-up task. - self.get_chain().call_no_reply( + # For now this must remain synchronous, as the action plug-in may have + # passed us a temporary file to transfer. A future FileService could + # maintain an LRU list of open file descriptors to keep the temporary + # file alive, but that requires more work. + self.get_chain().call( ansible_mitogen.target.transfer_file, context=self.parent, in_path=in_path, diff --git a/docs/changelog.rst b/docs/changelog.rst index f4c46976..162a7c48 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -53,11 +53,6 @@ Enhancements a 250 ms link from 30 seconds to 10 seconds compared to v0.2.2, down from 120 seconds compared to vanilla. -* `49736b3a `_: avoid a - roundtrip when transferring files larger than 124KiB, removing a delay - between waiting for the transfer to complete and start of the follow-up - action. - * `#337 `_: To avoid a scaling limitation, a PTY is no longer allocated for an SSH connection unless the configuration specifies a password. diff --git a/tests/ansible/integration/action/all.yml b/tests/ansible/integration/action/all.yml index c5cb80d7..75f77243 100644 --- a/tests/ansible/integration/action/all.yml +++ b/tests/ansible/integration/action/all.yml @@ -1,3 +1,4 @@ +- import_playbook: copy.yml - import_playbook: fixup_perms2__copy.yml - import_playbook: low_level_execute_command.yml - import_playbook: make_tmp_path.yml diff --git a/tests/ansible/integration/action/copy.yml b/tests/ansible/integration/action/copy.yml new file mode 100644 index 00000000..e3fca87f --- /dev/null +++ b/tests/ansible/integration/action/copy.yml @@ -0,0 +1,66 @@ +# Verify copy module for small and large files, and inline content. + +- name: integration/action/synchronize.yml + hosts: test-targets + any_errors_fatal: true + tasks: + - copy: + dest: /tmp/copy-tiny-file + content: + this is a tiny file. + connection: local + + - copy: + dest: /tmp/copy-large-file + # Must be larger than Connection.SMALL_SIZE_LIMIT. + content: "{% for x in range(200000) %}x{% endfor %}" + connection: local + + # end of making files + + - file: + state: absent + path: "{{item}}" + with_items: + - /tmp/copy-tiny-file.out + - /tmp/copy-large-file.out + - /tmp/copy-tiny-inline-file.out + - /tmp/copy-large-inline-file.out + + # end of cleaning out files + + - copy: + dest: /tmp/copy-large-file.out + src: /tmp/copy-large-file + + - copy: + dest: /tmp/copy-tiny-file.out + src: /tmp/copy-tiny-file + + - copy: + dest: /tmp/copy-tiny-inline-file.out + content: "tiny inline content" + + - copy: + dest: /tmp/copy-large-inline-file.out + content: | + {% for x in range(200000) %}y{% endfor %} + + # stat results + + - stat: + path: "{{item}}" + with_items: + - /tmp/copy-tiny-file.out + - /tmp/copy-large-file.out + - /tmp/copy-tiny-inline-file.out + - /tmp/copy-large-inline-file.out + register: stat + + - assert: + that: + - stat.results[0].stat.checksum == "f29faa9a6f19a700a941bf2aa5b281643c4ec8a0" + - stat.results[1].stat.checksum == "62951f943c41cdd326e5ce2b53a779e7916a820d" + - stat.results[2].stat.checksum == "b26dd6444595e2bdb342aa0a91721b57478b5029" + - stat.results[3].stat.checksum == "d675f47e467eae19e49032a2cc39118e12a6ee72" + diff --git a/tests/ansible/soak/_file_service_loop.yml b/tests/ansible/soak/_file_service_loop.yml new file mode 100644 index 00000000..96111b3c --- /dev/null +++ b/tests/ansible/soak/_file_service_loop.yml @@ -0,0 +1,6 @@ + - file: + path: /tmp/foo-{{inventory_hostname}} + state: absent + - copy: + dest: /tmp/foo-{{inventory_hostname}} + content: "{{content}}" diff --git a/tests/ansible/soak/file_service.yml b/tests/ansible/soak/file_service.yml new file mode 100644 index 00000000..3b338b3c --- /dev/null +++ b/tests/ansible/soak/file_service.yml @@ -0,0 +1,6 @@ +- hosts: all + tasks: + - set_fact: + content: "{% for x in range(126977) %}x{% endfor %}" + - include_tasks: _file_service_loop.yml + with_sequence: start=1 end=100