From 5521945bd2e9a601fa834dad8075824af4e74ed9 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Tue, 11 Sep 2018 03:44:17 +0100 Subject: [PATCH] ansible: temporary files take 5. --- ansible_mitogen/connection.py | 57 +++++++++------- ansible_mitogen/mixins.py | 36 ++++++---- ansible_mitogen/planner.py | 3 +- ansible_mitogen/runner.py | 40 +++++++++-- ansible_mitogen/target.py | 67 ++++++------------- docs/ansible.rst | 22 ++++-- .../integration/action/make_tmp_path.yml | 56 ++++------------ 7 files changed, 142 insertions(+), 139 deletions(-) diff --git a/ansible_mitogen/connection.py b/ansible_mitogen/connection.py index 60724fa3..ae6268de 100644 --- a/ansible_mitogen/connection.py +++ b/ansible_mitogen/connection.py @@ -31,6 +31,7 @@ from __future__ import unicode_literals import logging import os +import random import stat import time @@ -474,26 +475,19 @@ class Connection(ansible.plugins.connection.ConnectionBase): #: Only sudo, su, and doas are supported for now. become_methods = ['sudo', 'su', 'doas'] - #: Dict containing init_child() return vaue as recorded at startup by + #: Dict containing init_child() return value as recorded at startup by #: ContextService. Contains: #: #: fork_context: Context connected to the fork parent : process in the #: target account. #: home_dir: Target context's home directory. - #: temp_dir: A writeable temporary directory managed by the - #: target, automatically destroyed at shutdown. + #: good_temp_dir: A writeable directory where new temporary directories + #: can be created. init_child_result = None - #: A private temporary directory destroyed during :meth:`close`, or - #: automatically during shutdown if :meth:`close` failed or was never - #: called. - temp_dir = None - - #: A :class:`mitogen.parent.CallChain` to use for calls made to the target - #: account, to ensure subsequent calls fail if pipelined directory creation - #: or file transfer fails. This eliminates roundtrips when a call is likely - #: to succeed, and ensures subsequent actions will fail with the original - #: exception if the pipelined call failed. + #: A :class:`mitogen.parent.CallChain` for calls made to the target + #: account, to ensure subsequent calls fail with the original exception if + #: pipelined directory creation or file transfer fails. chain = None # @@ -695,14 +689,24 @@ class Connection(ansible.plugins.connection.ConnectionBase): self.init_child_result = dct['init_child_result'] - def _init_temp_dir(self): - """ - """ - self.temp_dir = os.path.join( - self.init_child_result['temp_dir'], - 'worker-%d-%x' % (os.getpid(), id(self)) + def get_good_temp_dir(self): + 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), + ) ) - self.get_chain().call_no_reply(os.mkdir, self.temp_dir) + + 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 _connect(self): """ @@ -721,7 +725,6 @@ class Connection(ansible.plugins.connection.ConnectionBase): self._connect_broker() stack = self._build_stack() self._connect_stack(stack) - self._init_temp_dir() def close(self, new_task=False): """ @@ -729,18 +732,22 @@ class Connection(ansible.plugins.connection.ConnectionBase): gracefully shut down, and wait for shutdown to complete. Safe to call multiple times. """ + if getattr(self._shell, 'tmpdir', None) is not None: + # Avoid CallChain to ensure exception is logged on failure. + self.context.call_no_reply( + ansible_mitogen.target.prune_tree, + self._shell.tmpdir, + ) + self._shell.tmpdir = None + if self.context: self.chain.reset() - # No pipelining to ensure exception is logged on failure. - self.context.call_no_reply(ansible_mitogen.target.prune_tree, - self.temp_dir) self.parent.call_service( service_name='ansible_mitogen.services.ContextService', method_name='put', context=self.context ) - self.temp_dir = None self.context = None self.login_context = None self.init_child_result = None diff --git a/ansible_mitogen/mixins.py b/ansible_mitogen/mixins.py index faadbc15..d4fcbd0d 100644 --- a/ansible_mitogen/mixins.py +++ b/ansible_mitogen/mixins.py @@ -180,12 +180,7 @@ class ActionModuleMixin(ansible.plugins.action.ActionBase): connection. """ LOG.debug('_make_tmp_path(remote_user=%r)', remote_user) - self._connection._connect() - # _make_tmp_path() is basically a global stashed away as Shell.tmpdir. - self._connection._shell.tmpdir = self._connection.temp_dir - LOG.debug('Temporary directory: %r', self._connection._shell.tmpdir) - self._cleanup_remote_tmp = True - return self._connection._shell.tmpdir + return self._connection._make_tmp_path() def _remove_tmp_path(self, tmp_path): """ @@ -193,6 +188,7 @@ class ActionModuleMixin(ansible.plugins.action.ActionBase): with nothing, as the persistent interpreter automatically cleans up after itself without introducing roundtrips. """ + # The actual removal is pipelined by Connection.close(). LOG.debug('_remove_tmp_path(%r)', tmp_path) self._connection._shell.tmpdir = None @@ -293,6 +289,25 @@ class ActionModuleMixin(ansible.plugins.action.ActionBase): except AttributeError: return getattr(self._task, 'async') + def _temp_file_gibberish(self, module_args, wrap_async): + # Ansible>2.5 module_utils reuses the action's temporary directory if + # one exists. Older versions error if this key is present. + if ansible.__version__ > '2.5': + if wrap_async: + # Sharing is not possible with async tasks, as in that case, + # the directory must outlive the action plug-in. + module_args['_ansible_tmpdir'] = None + else: + module_args['_ansible_tmpdir'] = self._connection._shell.tmpdir + + # If _ansible_tmpdir is unset, Ansible>2.6 module_utils will use + # _ansible_remote_tmp as the location to create the module's temporary + # directory. Older versions error if this key is present. + if ansible.__version__ > '2.6': + module_args['_ansible_remote_tmp'] = ( + self._connection.get_good_temp_dir() + ) + def _execute_module(self, module_name=None, module_args=None, tmp=None, task_vars=None, persist_files=False, delete_remote_tmp=True, wrap_async=False): @@ -311,16 +326,9 @@ class ActionModuleMixin(ansible.plugins.action.ActionBase): self._update_module_args(module_name, module_args, task_vars) env = {} self._compute_environment_string(env) + self._temp_file_gibberish(module_args, wrap_async) - # Always set _ansible_tmpdir regardless of whether _make_remote_tmp() - # has ever been called. This short-circuits all the .tmpdir logic in - # module_common and ensures no second temporary directory or atexit - # handler is installed. self._connection._connect() - - if ansible.__version__ > '2.5': - module_args['_ansible_tmpdir'] = self._connection.temp_dir - return ansible_mitogen.planner.invoke( ansible_mitogen.planner.Invocation( action=self, diff --git a/ansible_mitogen/planner.py b/ansible_mitogen/planner.py index 5b3e5547..caf40af3 100644 --- a/ansible_mitogen/planner.py +++ b/ansible_mitogen/planner.py @@ -149,7 +149,8 @@ class Planner(object): """ new = dict((mitogen.core.UnicodeType(k), kwargs[k]) for k in kwargs) - new.setdefault('temp_dir', self._inv.connection.temp_dir) + new.setdefault('good_temp_dir', + self._inv.connection.get_good_temp_dir()) new.setdefault('cwd', self._inv.connection.get_default_cwd()) new.setdefault('extra_env', self._inv.connection.get_default_env()) new.setdefault('emulate_tty', True) diff --git a/ansible_mitogen/runner.py b/ansible_mitogen/runner.py index fe5f4c46..44780aa2 100644 --- a/ansible_mitogen/runner.py +++ b/ansible_mitogen/runner.py @@ -230,6 +230,11 @@ class Runner(object): This is passed as a string rather than a dict in order to mimic the implicit bytes/str conversion behaviour of a 2.x controller running against a 3.x target. + :param str good_temp_dir: + The writeable temporary directory for this user account reported by + :func:`ansible_mitogen.target.init_child` passed via the controller. + This is specified explicitly to remain compatible with Ansible<2.5, and + for forked tasks where init_child never runs. :param dict env: Additional environment variables to set during the run. Keys with :data:`None` are unset if present. @@ -242,7 +247,7 @@ class Runner(object): When :data:`True`, indicate the runner should detach the context from its parent after setup has completed successfully. """ - def __init__(self, module, service_context, json_args, temp_dir, + def __init__(self, module, service_context, json_args, good_temp_dir, extra_env=None, cwd=None, env=None, econtext=None, detach=False): self.module = module @@ -250,10 +255,32 @@ class Runner(object): self.econtext = econtext self.detach = detach self.args = json.loads(json_args) - self.temp_dir = temp_dir + self.good_temp_dir = good_temp_dir self.extra_env = extra_env self.env = env self.cwd = cwd + #: If not :data:`None`, :meth:`get_temp_dir` had to create a temporary + #: directory for this run, because we're in an asynchronous task, or + #: because the originating action did not create a directory. + self._temp_dir = None + + def get_temp_dir(self): + path = self.args.get('_ansible_tmpdir') + if path is not None: + return path + + if self._temp_dir is None: + self._temp_dir = tempfile.mkdtemp( + prefix='ansible_mitogen_runner_', + dir=self.good_temp_dir, + ) + + return self._temp_dir + + def revert_temp_dir(self): + if self._temp_dir is not None: + ansible_mitogen.target.prune_tree(self._temp_dir) + self._temp_dir = None def setup(self): """ @@ -291,6 +318,7 @@ class Runner(object): implementation simply restores the original environment. """ self._env.revert() + self.revert_temp_dir() def _run(self): """ @@ -466,7 +494,7 @@ class ProgramRunner(Runner): fetched via :meth:`_get_program`. """ filename = self._get_program_filename() - path = os.path.join(self.temp_dir, filename) + path = os.path.join(self.get_temp_dir(), filename) self.program_fp = open(path, 'wb') self.program_fp.write(self._get_program()) self.program_fp.flush() @@ -546,7 +574,7 @@ class ArgsFileRunner(Runner): self.args_fp = tempfile.NamedTemporaryFile( prefix='ansible_mitogen', suffix='-args', - dir=self.temp_dir, + dir=self.get_temp_dir(), ) self.args_fp.write(utf8(self._get_args_contents())) self.args_fp.flush() @@ -661,7 +689,7 @@ class NewStyleRunner(ScriptRunner): def setup(self): super(NewStyleRunner, self).setup() - self._stdio = NewStyleStdio(self.args, self.temp_dir) + self._stdio = NewStyleStdio(self.args, self.get_temp_dir()) # It is possible that not supplying the script filename will break some # module, but this has never been a bug report. Instead act like an # interpreter that had its script piped on stdin. @@ -739,7 +767,7 @@ class NewStyleRunner(ScriptRunner): # don't want to pointlessly write the module to disk when it never # actually needs to exist. So just pass the filename as it would exist. mod.__file__ = os.path.join( - self.temp_dir, + self.get_temp_dir(), 'ansible_module_' + os.path.basename(self.path), ) diff --git a/ansible_mitogen/target.py b/ansible_mitogen/target.py index 35863cb2..ae7990a9 100644 --- a/ansible_mitogen/target.py +++ b/ansible_mitogen/target.py @@ -85,13 +85,9 @@ MAKE_TEMP_FAILED_MSG = ( #: the target Python interpreter before it executes any code or imports. _fork_parent = None -#: Set by init_child() to a list of candidate $variable-expanded and -#: tilde-expanded directory paths that may be usable as a temporary directory. -_candidate_temp_dirs = None - -#: Set by reset_temp_dir() to the single temporary directory that will exist -#: for the duration of the process. -temp_dir = None +#: Set by :func:`init_child` to the name of a writeable and executable +#: temporary directory accessible by the active user account. +good_temp_dir = None def get_small_file(context, path): @@ -206,15 +202,19 @@ def _on_broker_shutdown(): prune_tree(temp_dir) -def find_good_temp_dir(): +def find_good_temp_dir(candidate_temp_dirs): """ - Given a list of candidate temp directories extracted from ``ansible.cfg`` - and stored in _candidate_temp_dirs, combine it with the Python-builtin list - of candidate directories used by :mod:`tempfile`, then iteratively try each - in turn until one is found that is both writeable and executable. + Given a list of candidate temp directories extracted from ``ansible.cfg``, + combine it with the Python-builtin list of candidate directories used by + :mod:`tempfile`, then iteratively try each until one is found that is both + writeable and executable. + + :param list candidate_temp_dirs: + List of candidate $variable-expanded and tilde-expanded directory paths + that may be usable as a temporary directory. """ paths = [os.path.expandvars(os.path.expanduser(p)) - for p in _candidate_temp_dirs] + for p in candidate_temp_dirs] paths.extend(tempfile._candidate_tempdir_list()) for path in paths: @@ -253,29 +253,6 @@ def find_good_temp_dir(): }) -@mitogen.core.takes_econtext -def reset_temp_dir(econtext): - """ - Create one temporary directory to be reused by all runner.py invocations - for the lifetime of the process. The temporary directory is changed for - each forked job, and emptied as necessary by runner.py::_cleanup_temp() - after each module invocation. - - The result is that a context need only create and delete one directory - during startup and shutdown, and no further filesystem writes need occur - assuming no modules execute that create temporary files. - """ - global temp_dir - # https://github.com/dw/mitogen/issues/239 - - basedir = find_good_temp_dir() - temp_dir = tempfile.mkdtemp(prefix='ansible_mitogen_', dir=basedir) - - # This must be reinstalled in forked children too, since the Broker - # instance from the parent process does not carry over to the new child. - mitogen.core.listen(econtext.broker, 'shutdown', _on_broker_shutdown) - - @mitogen.core.takes_econtext def init_child(econtext, log_level, candidate_temp_dirs): """ @@ -306,24 +283,23 @@ def init_child(econtext, log_level, candidate_temp_dirs): the controller will use to start forked jobs, and `home_dir` is the home directory for the active user account. """ - global _candidate_temp_dirs - _candidate_temp_dirs = candidate_temp_dirs - - global _fork_parent - mitogen.parent.upgrade_router(econtext) - _fork_parent = econtext.router.fork() - reset_temp_dir(econtext) - # Copying the master's log level causes log messages to be filtered before # they reach LogForwarder, thus reducing an influx of tiny messges waking # the connection multiplexer process in the master. LOG.setLevel(log_level) logging.getLogger('ansible_mitogen').setLevel(log_level) + global _fork_parent + mitogen.parent.upgrade_router(econtext) + _fork_parent = econtext.router.fork() + + global good_temp_dir + good_temp_dir = find_good_temp_dir(candidate_temp_dirs) + return { 'fork_context': _fork_parent, 'home_dir': mitogen.core.to_text(os.path.expanduser('~')), - 'temp_dir': temp_dir, + 'good_temp_dir': good_temp_dir, } @@ -336,7 +312,6 @@ def create_fork_child(econtext): """ mitogen.parent.upgrade_router(econtext) context = econtext.router.fork() - context.call(reset_temp_dir) LOG.debug('create_fork_child() -> %r', context) return context diff --git a/docs/ansible.rst b/docs/ansible.rst index e4619ef9..d2138f21 100644 --- a/docs/ansible.rst +++ b/docs/ansible.rst @@ -425,6 +425,9 @@ specific variables with a particular linefeed style. Temporary Files ~~~~~~~~~~~~~~~ +Temporary file handling in Ansible is incredibly tricky business, and the exact +behaviour varies across major releases. + Ansible creates a variety of temporary files and directories depending on its operating mode. @@ -462,11 +465,20 @@ In summary, for each task Ansible may create one or more of: * ``$TMPDIR/ansible__payload_.../`` owned by the become user, * ``$TMPDIR/ansible-module-tmp-.../`` owned by the become user. -A directory must exist to maintain compatibility with Ansible, as many modules -introspect :data:`sys.argv` to find a directory where they may write files, -however only one directory exists for the lifetime of each interpreter, its -location is consistent for each target account, and it is always privately -owned by that account. + +Mitogen for Ansible +^^^^^^^^^^^^^^^^^^^ + +Temporary h +Temporary directory handling is fiddly and varies across major Ansible +releases. + + +Temporary directories must exist to maintain compatibility with Ansible, as +many modules introspect :data:`sys.argv` to find a directory where they may +write files, however only one directory exists for the lifetime of each +interpreter, its location is consistent for each target account, and it is +always privately owned by that account. The paths below are tried until one is found that is writeable and lives on a filesystem with ``noexec`` disabled: diff --git a/tests/ansible/integration/action/make_tmp_path.yml b/tests/ansible/integration/action/make_tmp_path.yml index 97da070d..dc713c31 100644 --- a/tests/ansible/integration/action/make_tmp_path.yml +++ b/tests/ansible/integration/action/make_tmp_path.yml @@ -28,18 +28,18 @@ method: _make_tmp_path register: tmp_path2 - - name: "Find parent temp path" + - name: "Find good temp path" set_fact: - parent_temp_path: "{{tmp_path.result|dirname}}" + good_temp_path: "{{tmp_path.result|dirname}}" - - name: "Find parent temp path (new task)" + - name: "Find good temp path (new task)" set_fact: - parent_temp_path2: "{{tmp_path2.result|dirname}}" + good_temp_path2: "{{tmp_path2.result|dirname}}" - name: "Verify common base path for both tasks" assert: that: - - parent_temp_path == parent_temp_path2 + - good_temp_path == good_temp_path2 - name: "Verify different subdir for both tasks" assert: @@ -60,6 +60,8 @@ path: "{{tmp_path2.result}}" register: stat2 + - debug: msg={{stat1}} + - name: "Verify neither subdir exists any more" assert: that: @@ -67,15 +69,15 @@ - not stat2.stat.exists # - # Verify parent directory persistence. + # Verify good directory persistence. # - - name: Stat parent temp path (new task) + - name: Stat good temp path (new task) stat: - path: "{{parent_temp_path}}" + path: "{{good_temp_path}}" register: stat - - name: "Verify parent temp path is persistent" + - name: "Verify good temp path is persistent" assert: that: - stat.stat.exists @@ -102,36 +104,6 @@ that: - not out.stat.exists - # - # - # - - - name: "Verify temp path changes across connection reset" - mitogen_shutdown_all: - - - name: "Verify temp path changes across connection reset" - action_passthrough: - method: _make_tmp_path - register: tmp_path2 - - - name: "Verify temp path changes across connection reset" - set_fact: - parent_temp_path2: "{{tmp_path2.result|dirname}}" - - - name: "Verify temp path changes across connection reset" - assert: - that: - - parent_temp_path != parent_temp_path2 - - - name: "Verify old path disappears across connection reset" - stat: path={{parent_temp_path}} - register: junk_stat - - - name: "Verify old path disappears across connection reset" - assert: - that: - - not junk_stat.stat.exists - # # root # @@ -175,12 +147,12 @@ when: ansible_version.full < '2.5' assert: that: - - out.module_path.startswith(parent_temp_path2) + - out.module_path.startswith(good_temp_path2) - out.module_tmpdir == None - name: "Verify modules get the same tmpdir as the action plugin (>2.5)" when: ansible_version.full > '2.5' assert: that: - - out.module_path.startswith(parent_temp_path2) - - out.module_tmpdir.startswith(parent_temp_path2) + - out.module_path.startswith(good_temp_path2) + - out.module_tmpdir.startswith(good_temp_path2)