ansible: temporary files take 5.

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

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,

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)

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),
         )
 

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
 

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_<modname>_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:

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)