From 2226c23c91f4445a39e6d11810c0d60ce499d427 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Sat, 23 Feb 2019 16:21:17 +0000 Subject: [PATCH 1/8] issue #550: update Changelog. --- docs/changelog.rst | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index ea52ef29..3c865ff7 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -147,6 +147,10 @@ Fixes * `#548 `_: `mitogen_via=` could fail when the selected transport was set to ``smart``. +* `#550 `_: avoid some broken + TTY-related `ioctl()` calls on Windows Subsystem for Linux 2016 Anniversary + Update. + Core Library ~~~~~~~~~~~~ @@ -178,8 +182,9 @@ Mitogen would not be possible without the support of users. A huge thanks for bug reports, testing, features and fixes in this release contributed by `Fabian Arrotin `_, `Percy Grunwald `_, -`Petr Enkov `_, and -`@elbunda `_. +`Petr Enkov `_, +`@elbunda `_, and +`@zyphermonkey `_. From 7dacb68eeb242e74022da9e71d891356ab731cd3 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Mon, 25 Feb 2019 16:33:03 +0000 Subject: [PATCH 2/8] issue #552: include process identity in log messages. --- ansible_mitogen/logging.py | 26 +++++++++++++++++++++++++- ansible_mitogen/process.py | 6 ++++-- ansible_mitogen/strategy.py | 1 + 3 files changed, 30 insertions(+), 3 deletions(-) diff --git a/ansible_mitogen/logging.py b/ansible_mitogen/logging.py index 1c439be8..ce6f1659 100644 --- a/ansible_mitogen/logging.py +++ b/ansible_mitogen/logging.py @@ -40,6 +40,25 @@ except ImportError: display = Display() +#: The process name set via :func:`set_process_name`. +_process_name = None + +#: The PID of the process that last called :func:`set_process_name`, so its +#: value can be ignored in unknown fork children. +_process_pid = None + + +def set_process_name(name): + """ + Set a name to adorn log messages with. + """ + global _process_name + _process_name = name + + global _process_pid + _process_pid = os.getpid() + + class Handler(logging.Handler): """ Use Mitogen's log format, but send the result to a Display method. @@ -65,7 +84,12 @@ class Handler(logging.Handler): if mitogen_name in self.NOISY_LOGGERS and record.levelno >= logging.WARNING: record.levelno = logging.DEBUG - s = '[pid %d] %s' % (os.getpid(), self.format(record)) + if _process_pid == os.getpid(): + process_name = _process_name + else: + process_name = '?' + + s = '[%-4s %d] %s' % (process_name, os.getpid(), self.format(record)) if record.levelno >= logging.ERROR: display.error(s, wrap_text=False) elif record.levelno >= logging.WARNING: diff --git a/ansible_mitogen/process.py b/ansible_mitogen/process.py index d7f36496..e4e61e8b 100644 --- a/ansible_mitogen/process.py +++ b/ansible_mitogen/process.py @@ -185,19 +185,21 @@ class MuxProcess(object): cls.profiling = os.environ.get('MITOGEN_PROFILING') is not None if cls.profiling: mitogen.core.enable_profiling() + if _init_logging: + ansible_mitogen.logging.setup() cls.original_env = dict(os.environ) cls.child_pid = os.fork() - if _init_logging: - ansible_mitogen.logging.setup() if cls.child_pid: save_pid('controller') + ansible_mitogen.logging.set_process_name('top') ansible_mitogen.affinity.policy.assign_controller() cls.child_sock.close() cls.child_sock = None mitogen.core.io_op(cls.worker_sock.recv, 1) else: save_pid('mux') + ansible_mitogen.logging.set_process_name('mux') ansible_mitogen.affinity.policy.assign_muxprocess() cls.worker_sock.close() cls.worker_sock = None diff --git a/ansible_mitogen/strategy.py b/ansible_mitogen/strategy.py index 50486841..ba0ff525 100644 --- a/ansible_mitogen/strategy.py +++ b/ansible_mitogen/strategy.py @@ -108,6 +108,7 @@ def wrap_worker__run(*args, **kwargs): if mitogen.core._profile_hook.__name__ != '_profile_hook': signal.signal(signal.SIGTERM, signal.SIG_IGN) + ansible_mitogen.logging.set_process_name('task') ansible_mitogen.affinity.policy.assign_worker() return mitogen.core._profile_hook('WorkerProcess', lambda: worker__run(*args, **kwargs) From ffae035584a281e78ac7bd3cfc8c65194d324306 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Thu, 28 Feb 2019 06:33:12 +0000 Subject: [PATCH 3/8] docs: drastically simplify install/changelog. --- docs/ansible.rst | 105 ++++++++++++++++++++++++++++++------------- docs/changelog.rst | 110 --------------------------------------------- 2 files changed, 74 insertions(+), 141 deletions(-) diff --git a/docs/ansible.rst b/docs/ansible.rst index 1c376a24..22b1433f 100644 --- a/docs/ansible.rst +++ b/docs/ansible.rst @@ -57,7 +57,7 @@ write files. Installation ------------ -1. Thoroughly review :ref:`noteworthy_differences` and :ref:`known_issues`. +1. Review :ref:`noteworthy_differences`. 2. Download and extract |mitogen_url|. 3. Modify ``ansible.cfg``: @@ -143,13 +143,29 @@ Testimonials Noteworthy Differences ---------------------- -* Ansible 2.3-2.7 are supported along with Python 2.6, 2.7 or 3.6. Verify your - installation is running one of these versions by checking ``ansible +* Ansible 2.3-2.7 are supported along with Python 2.6, 2.7, 3.6 and 3.7. Verify + your installation is running one of these versions by checking ``ansible --version`` output. -* The Ansible ``raw`` action executes as a regular Mitogen connection, - precluding its use for installing Python on a target. This will be addressed - soon. +* The ``raw`` action executes as a regular Mitogen connection, which requires + Python on the target, precluding its use for installing Python. This will be + addressed in a future release. For now, simply mix Mitogen and vanilla + Ansible strategies: + + .. code-block:: yaml + + - hosts: web-servers + strategy: linear + tasks: + - name: Install Python if necessary. + raw: test -e /usr/bin/python || apt install -y python-minimal + + - hosts: web-servers + strategy: mitogen_linear + roles: + - nginx + - initech_app + - y2k_fix * The ``doas``, ``su`` and ``sudo`` become methods are available. File bugs to register interest in more. @@ -166,43 +182,70 @@ Noteworthy Differences :ref:`mitogen_su `, :ref:`mitogen_sudo `, and :ref:`setns ` types. File bugs to register interest in others. -* Local commands execute in a reuseable interpreter created identically to - interpreters on targets. Presently one interpreter per ``become_user`` - exists, and so only one local action may execute simultaneously. +* Actions are single-threaded for each `(host, user account)` combination, + including actions that execute on the local machine. Playbooks may experience + slowdown compared to vanilla Ansible if they employ long-running + ``local_action`` or ``delegate_to`` tasks delegating many target hosts to a + single machine and user account. Ansible usually permits up to ``forks`` simultaneous local actions. Any long-running local actions that execute for every target will experience artificial serialization, causing slowdown equivalent to `task_duration * - num_targets`. This will be fixed soon. + num_targets`. This will be addressed soon. -* "Module Replacer" style modules are not supported. These rarely appear in - practice, and light web searches failed to reveal many examples of them. +* The Ansible 2.7 `reboot + `_ module + may require a ``pre_reboot_delay`` on systemd hosts, as insufficient time + exists for the reboot command's exit status to be reported before necessary + processes are torn down. + +* On OS X when a SSH password is specified and the default connection type of + ``smart`` is used, Ansible may select the Paramiko plug-in rather than + Mitogen. If you specify a password on OS X, ensure ``connection: ssh`` + appears in your playbook, ``ansible.cfg``, or as ``-c ssh`` on the + command-line. * Ansible permits up to ``forks`` connections to be setup in parallel, whereas in Mitogen this is handled by a fixed-size thread pool. Up to 32 connections may be established in parallel by default, this can be modified by setting the ``MITOGEN_POOL_SIZE`` environment variable. -* The ``ansible_python_interpreter`` variable is parsed using a restrictive - :mod:`shell-like ` syntax, permitting values such as ``/usr/bin/env - FOO=bar python``, which occur in practice. Ansible `documents this - `_ - as an absolute path, however the implementation passes it unquoted through - the shell, permitting arbitrary code to be injected. - -* Performance does not scale linearly with target count. This will improve over +* Performance does not scale cleanly with target count. This will improve over time. -* SSH and ``become`` are treated distinctly when applying timeouts, and - timeouts apply up to the point when the new interpreter is ready to accept - messages. Ansible has two timeouts: ``ConnectTimeout`` for SSH, applying up - to when authentication completes, and a separate parallel timeout up to when - ``become`` authentication completes. +* Performance on Python 3 is significantly worse than on Python 2. While this + has not yet been investigated, at least some of the regression appears to be + part of the core library, and should therefore be straightforward to fix as + part of 0.2.x. - For busy targets, Ansible may successfully execute a module where Mitogen - would fail without increasing the timeout. For sick targets, Ansible may hang - indefinitely after authentication without executing a command, for example - due to a stuck filesystem IO appearing in ``$HOME/.profile``. +.. + * SSH and ``become`` are treated distinctly when applying timeouts, and + timeouts apply up to the point when the new interpreter is ready to accept + messages. Ansible has two timeouts: ``ConnectTimeout`` for SSH, applying up + to when authentication completes, and a separate parallel timeout up to + when ``become`` authentication completes. + For busy targets, Ansible may successfully execute a module where Mitogen + would fail without increasing the timeout. For sick targets, Ansible may + hang indefinitely after authentication without executing a command, for + example due to a stuck filesystem IO appearing in ``$HOME/.profile``. + +.. + * "Module Replacer" style modules are not supported. These rarely appear in + practice, and light web searches failed to reveal many examples of them. + +.. + * The ``ansible_python_interpreter`` variable is parsed using a restrictive + :mod:`shell-like ` syntax, permitting values such as ``/usr/bin/env + FOO=bar python``, which occur in practice. Ansible `documents this + `_ + as an absolute path, however the implementation passes it unquoted through + the shell, permitting arbitrary code to be injected. + +.. + * Configurations will break that rely on the `hashbang argument splitting + behaviour `_ of the + ``ansible_python_interpreter`` setting, contrary to the Ansible + documentation. This will be addressed in a future 0.2 release. New Features & Notes @@ -253,8 +296,8 @@ container. ``ansible_password``, or ``ansible_become_pass`` inventory variables. * Automatic tunnelling of SSH-dependent actions, such as the - ``synchronize`` module, is not yet supported. This will be added in the - 0.3 series. + ``synchronize`` module, is not yet supported. This will be addressed in a + future release. To enable connection delegation, set ``mitogen_via=`` on the command line, or as host and group variables. diff --git a/docs/changelog.rst b/docs/changelog.rst index 3c865ff7..d5edd05e 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -15,116 +15,6 @@ Release Notes -.. _known_issues: - -Known Issues ------------- - -Mitogen For Ansible -~~~~~~~~~~~~~~~~~~~ - -* The Ansible 2.7 `reboot - `_ module - may require a ``pre_reboot_delay`` on systemd hosts, as insufficient time - exists for the reboot command's exit status to be reported before necessary - processes are torn down. - -* On OS X when a SSH password is specified and the default connection type of - ``smart`` is used, Ansible may select the Paramiko plug-in rather than - Mitogen. If you specify a password on OS X, ensure ``connection: ssh`` - appears in your playbook, ``ansible.cfg``, or as ``-c ssh`` on the - command-line. - -* The ``raw`` action executes as a regular Mitogen connection, which requires - Python on the target, precluding its use for installing Python. This will be - addressed in a future 0.2 release. For now, simply mix Mitogen and vanilla - Ansible strategies in your playbook: - - .. code-block:: yaml - - - hosts: web-servers - strategy: linear - tasks: - - name: Install Python if necessary. - raw: test -e /usr/bin/python || apt install -y python-minimal - - - hosts: web-servers - strategy: mitogen_linear - roles: - - nginx - - initech_app - - y2k_fix - -.. * When running with ``-vvv``, log messages will be printed to the console - *after* the Ansible run completes, as connection multiplexer shutdown only - begins after Ansible exits. This is due to a lack of suitable shutdown hook - in Ansible, and is fairly harmless, albeit cosmetically annoying. A future - release may include a solution. - -.. * Configurations will break that rely on the `hashbang argument splitting - behaviour `_ of the - ``ansible_python_interpreter`` setting, contrary to the Ansible - documentation. This will be addressed in a future 0.2 release. - -* Performance does not scale linearly with target count. This requires - significant additional work, as major bottlenecks exist in the surrounding - Ansible code. Performance-related bug reports for any scenario remain - welcome with open arms. - -* Performance on Python 3 is significantly worse than on Python 2. While this - has not yet been investigated, at least some of the regression appears to be - part of the core library, and should therefore be straightforward to fix as - part of 0.2.x. - -* *Module Replacer* style Ansible modules are not supported. - -* Actions are single-threaded for each `(host, user account)` combination, - including actions that execute on the local machine. Playbooks may experience - slowdown compared to vanilla Ansible if they employ long-running - ``local_action`` or ``delegate_to`` tasks delegating many target hosts to a - single machine and user account. - -* Connection Delegation remains in preview and has bugs around how it infers - connections. Connection establishment will remain single-threaded for the 0.2 - series, however connection inference bugs will be addressed in a future 0.2 - release. - -* Connection Delegation does not support automatic tunnelling of SSH-dependent - actions, such as the ``synchronize`` module. This will be addressed in the - 0.3 series. - - -Core Library -~~~~~~~~~~~~ - -* Serialization is still based on :mod:`pickle`. While there is high confidence - remote code execution is impossible in Mitogen's configuration, an untrusted - context may at least trigger disproportionately high memory usage injecting - small messages (*"billion laughs attack"*). Replacement is an important - future priority, but not critical for an initial release. - -* Child processes are not reliably reaped, leading to a pileup of zombie - processes when a program makes many short-lived connections in a single - invocation. This does not impact Mitogen for Ansible, however it limits the - usefulness of the core library. A future 0.2 release will address it. - -* Some races remain around :class:`mitogen.core.Broker ` destruction, - disconnection and corresponding file descriptor closure. These are only - problematic in situations where child process reaping is also problematic. - -* The `fakessh` component does not shut down correctly and requires flow - control added to the design. While minimal fixes are possible, due to the - absence of flow control the original design is functionally incomplete. - -* The multi-threaded :ref:`service` remains in a state of design flux and - should be considered obsolete, despite heavy use in Mitogen for Ansible. A - future replacement may be integrated more tightly with, or entirely replace - the RPC dispatcher on the main thread. - -* Documentation is in a state of disrepair. This will be improved over the 0.2 - series. - - v0.2.6 (unreleased) ------------------- From d1ba077f0e53be13496400913c4cf7a751523983 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Thu, 28 Feb 2019 06:35:44 +0000 Subject: [PATCH 4/8] docs: update Changelog. --- docs/changelog.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/changelog.rst b/docs/changelog.rst index d5edd05e..bf6d9088 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -41,6 +41,9 @@ Fixes TTY-related `ioctl()` calls on Windows Subsystem for Linux 2016 Anniversary Update. +* `ffae0355 `_: needless + information was removed from the documentation and installation procedure. + Core Library ~~~~~~~~~~~~ @@ -71,6 +74,7 @@ Thanks! Mitogen would not be possible without the support of users. A huge thanks for bug reports, testing, features and fixes in this release contributed by `Fabian Arrotin `_, +`Matt Layman `_, `Percy Grunwald `_, `Petr Enkov `_, `@elbunda `_, and From 7743e57ff3a6a706d41d063b2845fd3df04f7466 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Thu, 28 Feb 2019 06:51:14 +0000 Subject: [PATCH 5/8] issue #554: track and remove multiple make_tmp_path() calls. --- ansible_mitogen/connection.py | 31 ------------- ansible_mitogen/mixins.py | 45 ++++++++++++++----- docs/changelog.rst | 5 +++ tests/ansible/integration/action/all.yml | 1 + .../action/make_tmp_path__double.yml | 20 +++++++++ 5 files changed, 60 insertions(+), 42 deletions(-) create mode 100644 tests/ansible/integration/action/make_tmp_path__double.yml diff --git a/ansible_mitogen/connection.py b/ansible_mitogen/connection.py index fe07f44f..bd4330ff 100644 --- a/ansible_mitogen/connection.py +++ b/ansible_mitogen/connection.py @@ -33,7 +33,6 @@ import errno import logging import os import pprint -import random import stat import sys import time @@ -703,35 +702,6 @@ class Connection(ansible.plugins.connection.ConnectionBase): self._connect() return self.init_child_result['good_temp_dir'] - def _generate_tmp_path(self): - return os.path.join( - self.get_good_temp_dir(), - 'ansible_mitogen_action_%016x' % ( - random.getrandbits(8*8), - ) - ) - - def _make_tmp_path(self): - assert getattr(self._shell, 'tmpdir', None) is None - self._shell.tmpdir = self._generate_tmp_path() - LOG.debug('Temporary directory: %r', self._shell.tmpdir) - self.get_chain().call_no_reply(os.mkdir, self._shell.tmpdir) - return self._shell.tmpdir - - def _reset_tmp_path(self): - """ - Called by _mitogen_reset(); ask the remote context to delete any - temporary directory created for the action. CallChain is not used here - to ensure exception is logged by the context on failure, since the - CallChain itself is about to be destructed. - """ - if getattr(self._shell, 'tmpdir', None) is not None: - self.context.call_no_reply( - ansible_mitogen.target.prune_tree, - self._shell.tmpdir, - ) - self._shell.tmpdir = None - def _connect(self): """ Establish a connection to the master process's UNIX listener socket, @@ -763,7 +733,6 @@ class Connection(ansible.plugins.connection.ConnectionBase): if not self.context: return - self._reset_tmp_path() self.chain.reset() self.parent.call_service( service_name='ansible_mitogen.services.ContextService', diff --git a/ansible_mitogen/mixins.py b/ansible_mitogen/mixins.py index 5f51cc6f..6aa020fb 100644 --- a/ansible_mitogen/mixins.py +++ b/ansible_mitogen/mixins.py @@ -30,6 +30,7 @@ from __future__ import absolute_import import logging import os import pwd +import random import traceback try: @@ -173,26 +174,48 @@ class ActionModuleMixin(ansible.plugins.action.ActionBase): """ assert False, "_is_pipelining_enabled() should never be called." + def _generate_tmp_path(self): + return os.path.join( + self._connection.get_good_temp_dir(), + 'ansible_mitogen_action_%016x' % ( + random.getrandbits(8*8), + ) + ) + + def _generate_tmp_path(self): + return os.path.join( + self._connection.get_good_temp_dir(), + 'ansible_mitogen_action_%016x' % ( + random.getrandbits(8*8), + ) + ) + def _make_tmp_path(self, remote_user=None): """ - Return the directory created by the Connection instance during - connection. + Create a temporary subdirectory as a child of the temporary directory + managed by the remote interpreter. """ LOG.debug('_make_tmp_path(remote_user=%r)', remote_user) - return self._connection._make_tmp_path() + path = self._generate_tmp_path() + LOG.debug('Temporary directory: %r', path) + self._connection.get_chain().call_no_reply(os.mkdir, path) + self._connection._shell.tmpdir = path + return path def _remove_tmp_path(self, tmp_path): """ - Stub out the base implementation's invocation of rm -rf, replacing it - with nothing, as the persistent interpreter automatically cleans up - after itself without introducing roundtrips. + Replace the base implementation's invocation of rm -rf, replacing it + with a pipelined call to :func:`ansible_mitogen.target.prune_tree`. """ - # The actual removal is pipelined by Connection.close(). LOG.debug('_remove_tmp_path(%r)', tmp_path) - # Upstream _remove_tmp_path resets shell.tmpdir here, however - # connection.py uses that as the sole location of the temporary - # directory, if one exists. - # self._connection._shell.tmpdir = None + if tmp_path is None and ansible.__version__ > '2.6': + tmp_path = self._connection._shell.tmpdir # 06f73ad578d + if tmp_path is not None: + self._connection.get_chain().call_no_reply( + ansible_mitogen.target.prune_tree, + tmp_path, + ) + self._connection._shell.tmpdir = None def _transfer_data(self, remote_path, data): """ diff --git a/docs/changelog.rst b/docs/changelog.rst index bf6d9088..165f8a45 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -41,6 +41,10 @@ Fixes TTY-related `ioctl()` calls on Windows Subsystem for Linux 2016 Anniversary Update. +* `#554 `_: third party Ansible + action plug-ins that invoked :func:`_make_tmp_path` repeatedly could trigger + an assertion failure. + * `ffae0355 `_: needless information was removed from the documentation and installation procedure. @@ -77,6 +81,7 @@ bug reports, testing, features and fixes in this release contributed by `Matt Layman `_, `Percy Grunwald `_, `Petr Enkov `_, +`Tony Finch `_, `@elbunda `_, and `@zyphermonkey `_. diff --git a/tests/ansible/integration/action/all.yml b/tests/ansible/integration/action/all.yml index 461c742b..c43d5cc7 100644 --- a/tests/ansible/integration/action/all.yml +++ b/tests/ansible/integration/action/all.yml @@ -2,6 +2,7 @@ - include: fixup_perms2__copy.yml - include: low_level_execute_command.yml - include: make_tmp_path.yml +- include: make_tmp_path__double.yml - include: remote_expand_user.yml - include: remote_file_exists.yml - include: remove_tmp_path.yml diff --git a/tests/ansible/integration/action/make_tmp_path__double.yml b/tests/ansible/integration/action/make_tmp_path__double.yml new file mode 100644 index 00000000..8b24d322 --- /dev/null +++ b/tests/ansible/integration/action/make_tmp_path__double.yml @@ -0,0 +1,20 @@ +# issue #554: double calls to make_tmp_path() fail with assertion error. Ensure +# they succeed and are cleaned up correctly. + +- hosts: target + tasks: + - mitogen_action_script: + script: | + result['t1'] = self._make_tmp_path() + result['t2'] = self._make_tmp_path() + assert result['t1'] != result['t2'] + assert self._remote_file_exists(result['t1']) + assert self._remote_file_exists(result['t2']) + self._remove_tmp_path(result['t1']) + self._remove_tmp_path(result['t2']) + register: out + + - mitogen_action_script: + script: | + assert not self._remote_file_exists("{{ out.t1 }}") + assert not self._remote_file_exists("{{ out.t2 }}") From f36b4b47bf6e73f0c2c87b0ad8264ba1d98311c5 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Wed, 6 Mar 2019 11:22:40 +0000 Subject: [PATCH 6/8] issue #554: don't rely on tmp_path autoremoval in test. Ansible doesn't do this, so we shouldn't either. --- .../integration/action/make_tmp_path.yml | 31 ++++++++++++------- .../lib/action/mitogen_action_script.py | 18 ++++++++--- 2 files changed, 32 insertions(+), 17 deletions(-) diff --git a/tests/ansible/integration/action/make_tmp_path.yml b/tests/ansible/integration/action/make_tmp_path.yml index 0631727d..73aa1187 100644 --- a/tests/ansible/integration/action/make_tmp_path.yml +++ b/tests/ansible/integration/action/make_tmp_path.yml @@ -19,22 +19,26 @@ # - name: "Find regular temp path" - action_passthrough: - method: _make_tmp_path + mitogen_action_script: + script: | + path = self._make_tmp_path() + self._remove_tmp_path(path) register: tmp_path - name: "Find regular temp path (new task)" - action_passthrough: - method: _make_tmp_path + mitogen_action_script: + script: | + path = self._make_tmp_path() + self._remove_tmp_path(path) register: tmp_path2 - name: "Find good temp path" set_fact: - good_temp_path: "{{tmp_path.result|dirname}}" + good_temp_path: "{{tmp_path.path|dirname}}" - name: "Find good temp path (new task)" set_fact: - good_temp_path2: "{{tmp_path2.result|dirname}}" + good_temp_path2: "{{tmp_path2.path|dirname}}" - name: "Verify common base path for both tasks" assert: @@ -44,7 +48,7 @@ - name: "Verify different subdir for both tasks" assert: that: - - tmp_path.result != tmp_path2.result + - tmp_path.path != tmp_path2.path # # Verify subdirectory removal. @@ -52,12 +56,12 @@ - name: Stat temp path stat: - path: "{{tmp_path.result}}" + path: "{{tmp_path.path}}" register: stat1 - name: Stat temp path (new task) stat: - path: "{{tmp_path2.result}}" + path: "{{tmp_path2.path}}" register: stat2 - name: "Verify neither subdir exists any more" @@ -108,14 +112,17 @@ - name: "Find root temp path" become: true - action_passthrough: - method: _make_tmp_path + mitogen_action_script: + script: | + path = self._make_tmp_path() + self._remove_tmp_path(path) register: tmp_path_root - name: "Verify root temp path differs from regular path" assert: that: - - tmp_path2.result != tmp_path_root.result + - tmp_path2.path != tmp_path_root.path + - tmp_path2.path|dirname != tmp_path_root.path|dirname # # readonly homedir diff --git a/tests/ansible/lib/action/mitogen_action_script.py b/tests/ansible/lib/action/mitogen_action_script.py index e034345c..a8077fb4 100644 --- a/tests/ansible/lib/action/mitogen_action_script.py +++ b/tests/ansible/lib/action/mitogen_action_script.py @@ -16,12 +16,20 @@ def execute(s, gbls, lcls): class ActionModule(ActionBase): def run(self, tmp=None, task_vars=None): super(ActionModule, self).run(tmp=tmp, task_vars=task_vars) - lcls = { - 'self': self, - 'result': {} - } + + lcls = {} + # Capture keys to remove later, including any crap Python adds. + execute('pass', globals(), lcls) + lcls['self'] = self + # Old mitogen_action_script used explicit result dict. + lcls['result'] = lcls + + pre_keys = list(lcls) execute(self._task.args['script'], globals(), lcls) - return lcls['result'] + + for key in pre_keys: + del lcls[key] + return lcls if __name__ == '__main__': From 6309774be26e00f59823937bef5d1ffc2cd9e07f Mon Sep 17 00:00:00 2001 From: David Wilson Date: Wed, 6 Mar 2019 12:06:55 +0000 Subject: [PATCH 7/8] issue #554: fix Ansible 2.4 compatibility --- ansible_mitogen/mixins.py | 10 +++++++++- ansible_mitogen/target.py | 8 -------- tests/ansible/integration/action/remove_tmp_path.yml | 3 --- 3 files changed, 9 insertions(+), 12 deletions(-) diff --git a/ansible_mitogen/mixins.py b/ansible_mitogen/mixins.py index 6aa020fb..890467fd 100644 --- a/ansible_mitogen/mixins.py +++ b/ansible_mitogen/mixins.py @@ -354,7 +354,7 @@ class ActionModuleMixin(ansible.plugins.action.ActionBase): self._temp_file_gibberish(module_args, wrap_async) self._connection._connect() - return ansible_mitogen.planner.invoke( + result = ansible_mitogen.planner.invoke( ansible_mitogen.planner.Invocation( action=self, connection=self._connection, @@ -368,6 +368,14 @@ class ActionModuleMixin(ansible.plugins.action.ActionBase): ) ) + if ansible.__version__ < '2.5' and delete_remote_tmp and \ + getattr(self._connection._shell, 'tmpdir', None) is not None: + # Built-in actions expected tmpdir to be cleaned up automatically + # on _execute_module(). + self._remove_tmp_path(self._connection._shell.tmpdir) + + return result + def _postprocess_response(self, result): """ Apply fixups mimicking ActionBase._execute_module(); this is copied diff --git a/ansible_mitogen/target.py b/ansible_mitogen/target.py index 809165da..40e5c57b 100644 --- a/ansible_mitogen/target.py +++ b/ansible_mitogen/target.py @@ -260,14 +260,6 @@ def prune_tree(path): LOG.error('prune_tree(%r): %s', path, e) -def _on_broker_shutdown(): - """ - Respond to broker shutdown (graceful termination by parent, or loss of - connection to parent) by deleting our sole temporary directory. - """ - prune_tree(temp_dir) - - def is_good_temp_dir(path): """ Return :data:`True` if `path` can be used as a temporary directory, logging diff --git a/tests/ansible/integration/action/remove_tmp_path.yml b/tests/ansible/integration/action/remove_tmp_path.yml index 566e4f3f..7a0c6c25 100644 --- a/tests/ansible/integration/action/remove_tmp_path.yml +++ b/tests/ansible/integration/action/remove_tmp_path.yml @@ -6,9 +6,6 @@ hosts: test-targets any_errors_fatal: true tasks: - - meta: end_play - when: not is_mitogen - # # Use the copy module to cause a temporary directory to be created, and # return a result with a 'src' attribute pointing into that directory. From 30b8172573faa665737c60300104109922bda73e Mon Sep 17 00:00:00 2001 From: David Wilson Date: Wed, 6 Mar 2019 12:06:55 +0000 Subject: [PATCH 8/8] issue #554: mitogen_action_script fix --- .../lib/action/mitogen_action_script.py | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/ansible/lib/action/mitogen_action_script.py b/tests/ansible/lib/action/mitogen_action_script.py index a8077fb4..8a784c61 100644 --- a/tests/ansible/lib/action/mitogen_action_script.py +++ b/tests/ansible/lib/action/mitogen_action_script.py @@ -5,6 +5,21 @@ import sys from ansible.plugins.action import ActionBase +try: + long +except NameError: + long = int + +try: + unicode +except NameError: + unicode = str + +try: + bytes +except NameError: + bytes = str + def execute(s, gbls, lcls): if sys.version_info > (3,): @@ -29,6 +44,11 @@ class ActionModule(ActionBase): for key in pre_keys: del lcls[key] + for key in list(lcls): + if not isinstance(lcls[key], + (unicode, bytes, int, long, dict, list, tuple, + bool)): + del lcls[key] return lcls