issue #321: simplify temp directory handling.

ansible_mitogen/connection.py

@@ -456,6 +456,9 @@ class Connection(ansible.plugins.connection.ConnectionBase):
     #: Set after connection to the target context's home directory.
     home_dir = None
 
+    #: Set after connection to the target context's home directory.
+    _temp_dir = None
+
     def __init__(self, play_context, new_stdin, **kwargs):
         assert ansible_mitogen.process.MuxProcess.unix_listener_path, (
             'Mitogen connection types may only be instantiated '
@@ -635,6 +638,11 @@ class Connection(ansible.plugins.connection.ConnectionBase):
 
         self.fork_context = dct['init_child_result']['fork_context']
         self.home_dir = dct['init_child_result']['home_dir']
+        self._temp_dir = dct['init_child_result']['temp_dir']
+
+    def get_temp_dir(self):
+        self._connect()
+        return self._temp_dir
 
     def _connect(self):
         """

ansible_mitogen/mixins.py

@@ -180,48 +180,26 @@ class ActionModuleMixin(ansible.plugins.action.ActionBase):
         """
         assert False, "_is_pipelining_enabled() should never be called."
 
-    def _get_remote_tmp(self):
-        """
-        Mitogen-only: return the 'remote_tmp' setting.
-        """
-        try:
-            s = self._connection._shell.get_option('remote_tmp')
-        except AttributeError:
-            s = ansible.constants.DEFAULT_REMOTE_TMP  # <=2.4.x
-
-        return self._remote_expand_user(s, sudoable=False)
-
     def _make_tmp_path(self, remote_user=None):
         """
-        Replace the base implementation's use of shell to implement mkdtemp()
+        Return the temporary directory created by the persistent interpreter at
-        with an actual call to mkdtemp(). Like vanilla, the directory is always
+        startup.
-        created in the login account context.
         """
         LOG.debug('_make_tmp_path(remote_user=%r)', remote_user)
-
         # _make_tmp_path() is basically a global stashed away as Shell.tmpdir.
-        # The copy action plugin violates layering and grabs this attribute
+        self._connection._shell.tmpdir = self._connection.get_temp_dir()
-        # directly.
-        self._connection._shell.tmpdir = self._connection.call(
-            ansible_mitogen.target.make_temp_directory,
-            base_dir=self._get_remote_tmp(),
-            use_login_context=True,
-        )
         LOG.debug('Temporary directory: %r', self._connection._shell.tmpdir)
         self._cleanup_remote_tmp = True
         return self._connection._shell.tmpdir
 
     def _remove_tmp_path(self, tmp_path):
         """
-        Replace the base implementation's invocation of rm -rf with a call to
+        Stub out the base implementation's invocation of rm -rf, replacing it
-        shutil.rmtree().
+        with nothing, as the persistent interpreter automatically cleans up
+        after itself without introducing roundtrips.
         """
         LOG.debug('_remove_tmp_path(%r)', tmp_path)
-        if tmp_path is None:
+        self._connection._shell.tmpdir = None
-            tmp_path = self._connection._shell.tmpdir
-        if self._should_remove_tmp_path(tmp_path):
-            self.call(shutil.rmtree, tmp_path)
-            self._connection._shell.tmpdir = None
 
     def _transfer_data(self, remote_path, data):
         """
@@ -332,7 +310,13 @@ class ActionModuleMixin(ansible.plugins.action.ActionBase):
         env = {}
         self._compute_environment_string(env)
 
+        # 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()
+        module_args['_ansible_tmpdir'] = self._connection.get_temp_dir()
+
         return ansible_mitogen.planner.invoke(
             ansible_mitogen.planner.Invocation(
                 action=self,

ansible_mitogen/target.py

@@ -69,6 +69,19 @@ import ansible_mitogen.runner
 
 LOG = logging.getLogger(__name__)
 
+MAKE_TEMP_FAILED_MSG = (
+    "Unable to create a temporary directory for the persistent interpreter.\n"
+    "This likely means no system-supplied TMP directory can be written to.\n"
+    "\n"
+    "The following paths were tried:\n"
+    "    %(namelist)s\n"
+    "\n"
+    "The original exception was:\n"
+    "\n"
+    "%(exception)s"
+)
+
+
 #: Set by init_child() to the single temporary directory that will exist for
 #: the duration of the process.
 temp_dir = None
@@ -204,7 +217,14 @@ def reset_temp_dir(econtext):
     """
     global temp_dir
     # https://github.com/dw/mitogen/issues/239
-    temp_dir = tempfile.mkdtemp(prefix='ansible_mitogen_')
+
+    try:
+        temp_dir = tempfile.mkdtemp(prefix='ansible_mitogen_')
+    except IOError:
+        raise IOError(MAKE_TEMP_FAILED_MSG % {
+            'namelist': '\n    '.join(tempfile._candidate_tempdir_list()),
+            'exception': traceback.format_exc()
+        })
 
     # This must be reinstalled in forked children too, since the Broker
     # instance from the parent process does not carry over to the new child.
@@ -252,6 +272,7 @@ def init_child(econtext, log_level):
     return {
         'fork_context': _fork_parent,
         'home_dir': mitogen.core.to_text(os.path.expanduser('~')),
+        'temp_dir': temp_dir,
     }
 
 
@@ -416,27 +437,6 @@ def run_module_async(kwargs, job_id, timeout_secs, econtext):
     arunner.run()
 
 
-def make_temp_directory(base_dir):
-    """
-    Handle creation of `base_dir` if it is absent, in addition to a unique
-    temporary directory within `base_dir`. This is the temporary directory that
-    becomes 'remote_tmp', not the one used by Ansiballz. It always uses the
-    system temporary directory.
-
-    :returns:
-        Newly created temporary directory.
-    """
-    # issue #301: remote_tmp may contain $vars.
-    base_dir = os.path.expandvars(base_dir)
-
-    if not os.path.exists(base_dir):
-        os.makedirs(base_dir, mode=int('0700', 8))
-    return tempfile.mkdtemp(
-        dir=base_dir,
-        prefix='ansible-mitogen-tmp-',
-    )
-
-
 def get_user_shell():
     """
     For commands executed directly via an SSH command-line, SSH looks up the

docs/ansible.rst

@@ -271,8 +271,8 @@ command line, or as host and group variables.
 File Transfer
 ~~~~~~~~~~~~~
 
-Normally `sftp <https://linux.die.net/man/1/sftp>`_ or
+Normally `sftp(1) <https://linux.die.net/man/1/sftp>`_ or
-`scp <https://linux.die.net/man/1/scp>`_ are used to copy files by the
+`scp(1) <https://linux.die.net/man/1/scp>`_ are used to copy files by the
 `assemble <http://docs.ansible.com/ansible/latest/modules/assemble_module.html>`_,
 `copy <http://docs.ansible.com/ansible/latest/modules/copy_module.html>`_,
 `patch <http://docs.ansible.com/ansible/latest/modules/patch_module.html>`_,
@@ -302,7 +302,7 @@ to rename over any existing file. This ensures the file remains consistent at
 all times, in the event of a crash, or when overlapping `ansible-playbook` runs
 deploy differing file contents.
 
-The `sftp <https://linux.die.net/man/1/sftp>`_ and `scp
+The `sftp(1) <https://linux.die.net/man/1/sftp>`_ and `scp(1)
 <https://linux.die.net/man/1/sftp>`_ tools may cause undetected data corruption
 in the form of truncated files, or files containing intermingled data segments
 from overlapping runs. As part of normal operation, both tools expose a window
@@ -401,6 +401,67 @@ this precisely, to avoid breaking playbooks that expect text to appear in
 specific variables with a particular linefeed style.
 
 
+.. _ansible_tempfiles:
+
+Temporary Files
+~~~~~~~~~~~~~~~
+
+Ansible creates a variety of temporary files and directories depending on its
+operating mode.
+
+In the best case when pipelining is enabled and no temporary uploads are
+required, for each task Ansible will create one directory below a
+system-supplied temporary directory returned by :func:`tempfile.mkdtemp`, owned
+by the target user account a new-style module intends to execute in.
+
+In other cases depending on the task type, whether become is active, whether
+the target become user is privileged, whether the associated action plugin
+needs to upload files, and whether the associated module needs to store files,
+Ansible may:
+
+* Create a directory owned by the SSH user either under ``remote_tmp``, or a
+  system-default directory,
+* Upload action dependencies such as non-new style modules or rendered
+  templates to that directory via `sftp(1) <https://linux.die.net/man/1/sftp>`_
+  or `scp(1) <https://linux.die.net/man/1/scp>`_.
+* Attempt to modify the directory's access control list to grant access to the
+  target user using `setfacl(1) <https://linux.die.net/man/1/setfacl>`_,
+  requiring that tool to be installed and a supported filesystem to be in use,
+  or for the ``allow_world_readable_tmpfiles`` setting to be  :data:`True`.
+* Create a directory owned by the target user either under ``remote_tmp``, or
+  a system-default directory, if a new-style module needs a temporary directory
+  and one was not previously created for a supporting file earlier in the
+  invocation.
+
+In summary, for each task Ansible may create one or more of:
+
+* ``~ssh_user/<remote_tmp>/...`` owned by the login user,
+* ``$TMPDIR/ansible-tmp-...`` owned by the login user,
+* ``$TMPDIR/ansible-tmp-...`` owned by the login user with ACLs permitting
+  write access by the become user,
+* ``~become_user/<remote_tmp>/...`` owned by the become user,
+* ``$TMPDIR/ansible_<modname>_payload_.../`` owned by the become user,
+* ``$TMPDIR/ansible-module-tmp-.../`` owned by the become user.
+
+The extension must create a temporary directory to maintain compatibility with
+Ansible, since many modules introspect :data:`sys.argv` in order to find a
+directory where they may write temporary files, however for simplicity only
+one such directory exists for the lifetime of each interpreter, stored in a
+system-supplied temporary directory, and always privately owned by the target
+user account.
+
+The ``remote_tmp`` path is unused, since Ansible does not make exclusive use of
+it, existing semantics are untenable, environments exist with read-only home
+directories where the default ``remote_tmp`` path (``~/.ansible/tmp``) cannot
+be used, and new-style modules always depended on the existence of a
+system-supplied directory anyway, so no requirement is introduced by simply
+ignoring ``remote_tmp``.
+
+As the directory is created once at startup, and its content is managed by code
+running remotely, no additional network roundtrips are required to create and
+destroy it for each task requiring temporary file storage.
+
+
 .. _ansible_process_env:
 
 Process Environment Emulation
@@ -425,7 +486,7 @@ Modifications to ``/etc/resolv.conf`` cause the glibc resolver configuration to
 be reloaded via `res_init(3) <https://linux.die.net/man/3/res_init>`_. This
 isn't necessary on some Linux distributions carrying glibc patches to
 automatically check ``/etc/resolv.conf`` periodically, however it is necessary
-on at least Debian and the BSD derivatives.
+on at least Debian and BSD derivatives.
 
 
 ``/etc/environment``
@@ -433,7 +494,7 @@ on at least Debian and the BSD derivatives.
 
 When ``become: true`` is active or SSH multiplexing is disabled, modifications
 by previous tasks to ``/etc/environment`` and ``$HOME/.pam_environment`` are
-reflected, since the content of those files is reapplied by `PAM
+normally reflected, since the content of those files is reapplied by `PAM
 <https://en.wikipedia.org/wiki/Pluggable_authentication_module>`_ via `pam_env`
 on each authentication of ``sudo`` or ``sshd``.
 

docs/changelog.rst

@@ -44,6 +44,13 @@ Mitogen for Ansible
   extension that had been installed using the documented steps. Now the bundled
   library always overrides over any system-installed copy.
 
+* `#321 <https://github.com/dw/mitogen/issues/321>`_: temporary file handling
+  has been simplified and additional network roundtrips have been removed,
+  undoing earlier damage caused by compatibility bug fixes. A single directory
+  is created once at startup for each persistent interpreter, and the
+  ``remote_tmp`` setting is always ignored. See :ref:`ansible_tempfiles` for a
+  complete description.
+
 * `#324 <https://github.com/dw/mitogen/issues/324>`_: plays with a custom
   ``module_utils`` would fail due to fallout from the Python 3 port and related
   tests being disabled.

tests/ansible/ansible.cfg

@@ -17,10 +17,6 @@ timeout = 10
 # On Travis, paramiko check fails due to host key checking enabled.
 host_key_checking = False
 
-# "mitogen-tests" required by integration/runner/remote_tmp.yml
-# "$HOME" required by integration/action/make_tmp_path.yml
-remote_tmp = $HOME/.ansible/mitogen-tests/
-
 [ssh_connection]
 ssh_args = -o ForwardAgent=yes -o ControlMaster=auto -o ControlPersist=60s
 pipelining = True

tests/ansible/integration/action/make_tmp_path.yml

@@ -1,63 +1,115 @@
+#
+# Ensure _make_tmp_path returns the same result across invocations for a single
+# user account, and that the path returned cleans itself up on connection
+# termination.
+#
+# Related bugs prior to the new-style handling:
+#   https://github.com/dw/mitogen/issues/239
+#   https://github.com/dw/mitogen/issues/301
 
 - name: integration/action/make_tmp_path.yml
   hosts: test-targets
   any_errors_fatal: true
   tasks:
-    - name: "Find out root's homedir."
-      # Runs first because it blats regular Ansible facts with junk, so
-      # non-become run fixes that up.
-      setup: gather_subset=min
-      become: true
-      register: root_facts
-
-    - name: "Find regular homedir"
-      setup: gather_subset=min
-      register: user_facts
-
     #
-    # non-become
+    # non-root
     #
 
-    - action_passthrough:
+    - name: "Find regular temp path"
+      action_passthrough:
         method: _make_tmp_path
-      register: out
+      register: tmp_path
+
+    - name: "Write some junk in regular temp path"
+      shell: hostname > {{tmp_path.result}}/hostname
+
+    - name: "Verify junk did not persist across tasks"
+      stat: path={{tmp_path.result}}/hostname
+      register: junk_stat
+
+    - name: "Verify junk did not persist across tasks"
+      assert:
+        that:
+        - not junk_stat.stat.exists
+
+    - name: "Verify temp path hasn't changed since start"
+      action_passthrough:
+        method: _make_tmp_path
+      register: tmp_path2
+
+    - name: "Verify temp path hasn't changed since start"
+      assert:
+        that:
+        - tmp_path2.result == tmp_path.result
+
+    - 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
 
-    - assert:
+    - name: "Verify temp path changes across connection reset"
-        # This string must match ansible.cfg::remote_tmp
+      assert:
-        that: out.result.startswith("{{user_facts.ansible_facts.ansible_user_dir}}/.ansible/mitogen-tests/")
+        that:
+        - tmp_path2.result != tmp_path.result
 
-    - stat:
+    - name: "Verify old path disappears across connection reset"
-        path: "{{out.result}}"
+      stat: path={{tmp_path.result}}
-      register: st
+      register: junk_stat
 
-    - assert:
+    - name: "Verify old path disappears across connection reset"
-        that: st.stat.exists and st.stat.isdir and st.stat.mode == "0700"
+      assert:
+        that:
+        - not junk_stat.stat.exists
 
-    - file:
+    #
-        path: "{{out.result}}"
+    # root
-        state: absent
+    #
+
+    - name: "Find root temp path"
+      become: true
+      action_passthrough:
+        method: _make_tmp_path
+      register: tmp_path_root
+
+    - name: "Verify root temp path differs from regular path"
+      assert:
+        that:
+        - tmp_path2.result != tmp_path_root.result
 
     #
-    # become. make_tmp_path() must evaluate HOME in the context of the SSH
+    # readonly homedir
-    # user, not the become user.
     #
 
-    - action_passthrough:
+    - name: "Try writing to temp directory for the readonly_homedir user"
+      become: true
+      become_user: mitogen__readonly_homedir
+      action_passthrough:
         method: _make_tmp_path
-      register: out
+      register: tmp_path
+
+    - name: "Try writing to temp directory for the readonly_homedir user"
       become: true
+      become_user: mitogen__readonly_homedir
+      shell: hostname > {{tmp_path.result}}/hostname
 
-    - assert:
+    #
-        # This string must match ansible.cfg::remote_tmp
+    # modules get the same temp dir
-        that: out.result.startswith("{{user_facts.ansible_facts.ansible_user_dir}}/.ansible/mitogen-tests/")
+    #
 
-    - stat:
+    - name: "Verify modules get the same tmpdir as the action plugin"
-        path: "{{out.result}}"
+      action_passthrough:
-      register: st
+        method: _make_tmp_path
+      register: tmp_path
 
-    - assert:
+    - name: "Verify modules get the same tmpdir as the action plugin"
-        that: st.stat.exists and st.stat.isdir and st.stat.mode == "0700"
+      custom_python_detect_environment:
+      register: out
 
-    - file:
+    # v2.6 related: https://github.com/ansible/ansible/pull/39833
-        path: "{{out.result}}"
+    - name: "Verify modules get the same tmpdir as the action plugin"
-        state: absent
+      assert:
+        that:
+        - out.module_tmpdir == tmp_path.result

tests/ansible/integration/all.yml

@@ -13,7 +13,6 @@
 - import_playbook: local/all.yml
 - import_playbook: module_utils/all.yml
 - import_playbook: playbook_semantics/all.yml
-- import_playbook: remote_tmp/all.yml
 - import_playbook: runner/all.yml
 - import_playbook: ssh/all.yml
 - import_playbook: strategy/all.yml

tests/ansible/integration/remote_tmp/all.yml

@@ -1,2 +0,0 @@
-
-- import_playbook: readonly_homedir.yml

tests/ansible/integration/remote_tmp/readonly_homedir.yml

@@ -1,20 +0,0 @@
-# https://github.com/dw/mitogen/issues/239
-# While remote_tmp is used in the context of the SSH user by action code
-# running on the controller, Ansiballz ignores it and uses the system default
-# instead.
-
-- name: integration/remote_tmp/readonly_homedir.yml
-  hosts: test-targets
-  any_errors_fatal: true
-  tasks:
-    - custom_python_detect_environment:
-      become: true
-      become_user: mitogen__readonly_homedir
-      register: out
-      vars:
-        ansible_become_pass: readonly_homedir_password
-
-    - name: Verify system temp directory was used.
-      assert:
-        that:
-          - out.__file__.startswith("/tmp/ansible_")

tests/ansible/lib/modules/custom_python_detect_environment.py

@@ -29,6 +29,7 @@ def main():
         mitogen_loaded='mitogen.core' in sys.modules,
         hostname=socket.gethostname(),
         username=pwd.getpwuid(os.getuid()).pw_name,
+        module_tmpdir=getattr(module, 'tmpdir', None),
     )
 
 if __name__ == '__main__':