From 16c4a22c326f67a4dc3fc9b22b0ddc31909571d5 Mon Sep 17 00:00:00 2001 From: Matt Martz Date: Wed, 4 Dec 2019 10:55:17 -0600 Subject: [PATCH 1/5] Add method to automatically clean up after an action plugin --- lib/ansible/executor/task_executor.py | 4 ++++ lib/ansible/plugins/action/__init__.py | 12 ++++++++++++ 2 files changed, 16 insertions(+) diff --git a/lib/ansible/executor/task_executor.py b/lib/ansible/executor/task_executor.py index 0516479adc2107..1bcdbc1e60f1f1 100644 --- a/lib/ansible/executor/task_executor.py +++ b/lib/ansible/executor/task_executor.py @@ -649,6 +649,8 @@ def _execute(self, variables=None): return dict(failed=True, msg=to_text(e)) except AnsibleConnectionFailure as e: return dict(unreachable=True, msg=to_text(e)) + finally: + self._handler.cleanup() display.debug("handler run complete") # preserve no log @@ -846,6 +848,8 @@ def _poll_async_result(self, result, templar, task_vars=None): raise else: time_left -= self._task.poll + finally: + self._handler.cleanup(force=True) if int(async_result.get('finished', 0)) != 1: if async_result.get('_ansible_parsed'): diff --git a/lib/ansible/plugins/action/__init__.py b/lib/ansible/plugins/action/__init__.py index 8b70d46cffaa2d..dc72e6c56c94fa 100644 --- a/lib/ansible/plugins/action/__init__.py +++ b/lib/ansible/plugins/action/__init__.py @@ -115,6 +115,18 @@ def run(self, tmp=None, task_vars=None): return result + def cleanup(self, force=False): + """Method to perform a clean up at the end of an action plugin execution + + By default this is designed to clean up the shell tmpdir, and is toggled based on whether + async is in use + + Action plugins may override this if they deem necessary, but should still call this method + via super + """ + if force or not self._task.async_val: + self._remove_tmp_path(self._connection._shell.tmpdir) + def get_plugin_option(self, plugin, option, default=None): """Helper to get an option from a plugin without having to use the try/except dance everywhere to set a default From 3fe9fd930683eacff5a6711e85dfd4d545d23222 Mon Sep 17 00:00:00 2001 From: Matt Martz Date: Wed, 4 Dec 2019 11:56:23 -0600 Subject: [PATCH 2/5] Use correct var, move cleanup for async --- lib/ansible/executor/task_executor.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/ansible/executor/task_executor.py b/lib/ansible/executor/task_executor.py index 1bcdbc1e60f1f1..eb32af7897a6a5 100644 --- a/lib/ansible/executor/task_executor.py +++ b/lib/ansible/executor/task_executor.py @@ -848,8 +848,6 @@ def _poll_async_result(self, result, templar, task_vars=None): raise else: time_left -= self._task.poll - finally: - self._handler.cleanup(force=True) if int(async_result.get('finished', 0)) != 1: if async_result.get('_ansible_parsed'): @@ -857,6 +855,7 @@ def _poll_async_result(self, result, templar, task_vars=None): else: return dict(failed=True, msg="async task produced unparseable results", async_result=async_result) else: + async_handler.cleanup(force=True) return async_result def _get_become(self, name): From 65c227923dc84ff026299d7dc1c5e892f96bd8fc Mon Sep 17 00:00:00 2001 From: Matt Martz Date: Thu, 5 Dec 2019 17:15:28 -0600 Subject: [PATCH 3/5] Add changelog and tests. Fixes #65393. Fixes #65277. --- .../action-plugin-always-cleanup.yml | 7 +++++ .../targets/remote_tmp/playbook.yml | 29 +++++++++++++++++++ test/integration/targets/remote_tmp/runme.sh | 2 +- 3 files changed, 37 insertions(+), 1 deletion(-) create mode 100644 changelogs/fragments/action-plugin-always-cleanup.yml diff --git a/changelogs/fragments/action-plugin-always-cleanup.yml b/changelogs/fragments/action-plugin-always-cleanup.yml new file mode 100644 index 00000000000000..15776974e1fe00 --- /dev/null +++ b/changelogs/fragments/action-plugin-always-cleanup.yml @@ -0,0 +1,7 @@ +bugfixes: +- ActionBase - Add new ``cleanup`` method that is explicitly run by the + ``TaskExecutor`` to ensure that the shell plugins ``tmpdir`` is always + removed. This change means that individual action plugins need not be + responsible for removing the temporary directory, which ensures that + we don't have code paths that accidentally leave behind the temporary + directory. diff --git a/test/integration/targets/remote_tmp/playbook.yml b/test/integration/targets/remote_tmp/playbook.yml index 46a2846590c22f..cfd44b75e6e707 100644 --- a/test/integration/targets/remote_tmp/playbook.yml +++ b/test/integration/targets/remote_tmp/playbook.yml @@ -24,3 +24,32 @@ - name: clean up test user user: name=tmptest state=absent become_user: root + +- name: Test tempdir is removed + hosts: testhost + gather_facts: false + tasks: + - file: + state: touch + path: "/{{ output_dir }}/65393" + + - copy: + src: "/{{ output_dir }}/65393" + dest: "/{{ output_dir }}/65393.2" + remote_src: true + + - find: + path: "~/.ansible/tmp" + use_regex: yes + patterns: 'AnsiballZ_.+\.py' + recurse: true + register: result + + - debug: + var: result + + - assert: + that: + # Should only be AnsiballZ_find.py because find is actively running + - result.files|length == 1 + - result.files[0].path.endswith('/AnsiballZ_find.py') diff --git a/test/integration/targets/remote_tmp/runme.sh b/test/integration/targets/remote_tmp/runme.sh index 69efd6e01644c2..5f2edc2f10f573 100755 --- a/test/integration/targets/remote_tmp/runme.sh +++ b/test/integration/targets/remote_tmp/runme.sh @@ -2,4 +2,4 @@ set -ux -ansible-playbook -i ../../inventory playbook.yml -v "$@" +ansible-playbook -i ../../inventory playbook.yml -e output_dir=${OUTPUT_DIR} -v "$@" From 9f50579293b33954b5233f13429d90e346c89daf Mon Sep 17 00:00:00 2001 From: Matt Martz Date: Fri, 6 Dec 2019 08:31:42 -0600 Subject: [PATCH 4/5] Kill off all long running async tasks from listen_ports_facts --- .../integration/targets/listen_ports_facts/tasks/main.yml | 8 ++++++++ test/integration/targets/remote_tmp/runme.sh | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/test/integration/targets/listen_ports_facts/tasks/main.yml b/test/integration/targets/listen_ports_facts/tasks/main.yml index 2ed69e00023828..f0e5787067c7b7 100644 --- a/test/integration/targets/listen_ports_facts/tasks/main.yml +++ b/test/integration/targets/listen_ports_facts/tasks/main.yml @@ -77,3 +77,11 @@ assert: that: 5555 in ansible_facts.udp_listen | map(attribute='port') | sort | list when: (ansible_os_family == "RedHat" and ansible_distribution_major_version|int >= 7) or ansible_os_family == "Debian" + +- name: kill all async commands + command: "kill -9 {{ item }}" + loop: "{{ [tcp_pids, udp_pids]|flatten }}" + vars: + tcp_pids: "{{ tcp_listen|selectattr('name', 'match', 'nc$')|map(attribute='pid')|list }}" + udp_pids: "{{ udp_listen|selectattr('name', 'match', 'nc$')|map(attribute='pid')|list }}" + ignore_errors: true diff --git a/test/integration/targets/remote_tmp/runme.sh b/test/integration/targets/remote_tmp/runme.sh index 5f2edc2f10f573..8d1eebd6b14623 100755 --- a/test/integration/targets/remote_tmp/runme.sh +++ b/test/integration/targets/remote_tmp/runme.sh @@ -2,4 +2,4 @@ set -ux -ansible-playbook -i ../../inventory playbook.yml -e output_dir=${OUTPUT_DIR} -v "$@" +ansible-playbook -i ../../inventory playbook.yml -e "output_dir=${OUTPUT_DIR}" -v "$@" From f7bf64f8c0ea6b52ccb7e50738bb783d8b58fa03 Mon Sep 17 00:00:00 2001 From: Matt Martz Date: Fri, 6 Dec 2019 09:52:34 -0600 Subject: [PATCH 5/5] Update task to work with older jinja2 --- .../integration/targets/listen_ports_facts/tasks/main.yml | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/test/integration/targets/listen_ports_facts/tasks/main.yml b/test/integration/targets/listen_ports_facts/tasks/main.yml index f0e5787067c7b7..02d6ed57ca8e62 100644 --- a/test/integration/targets/listen_ports_facts/tasks/main.yml +++ b/test/integration/targets/listen_ports_facts/tasks/main.yml @@ -79,9 +79,7 @@ when: (ansible_os_family == "RedHat" and ansible_distribution_major_version|int >= 7) or ansible_os_family == "Debian" - name: kill all async commands - command: "kill -9 {{ item }}" - loop: "{{ [tcp_pids, udp_pids]|flatten }}" - vars: - tcp_pids: "{{ tcp_listen|selectattr('name', 'match', 'nc$')|map(attribute='pid')|list }}" - udp_pids: "{{ udp_listen|selectattr('name', 'match', 'nc$')|map(attribute='pid')|list }}" + command: "kill -9 {{ item.pid }}" + loop: "{{ [tcp_listen, udp_listen]|flatten }}" + when: item.name == 'nc' ignore_errors: true