Merge branch 'dmw'

ansible_mitogen/connection.py

@@ -692,15 +692,29 @@ class Connection(ansible.plugins.connection.ConnectionBase):
         :param bool use_login_context:
             If present and :data:`True`, send the call to the login account
             context rather than the optional become user context.
+
+        :param bool no_reply:
+            If present and :data:`True`, send the call with no ``reply_to``
+            header, causing the context to execute it entirely asynchronously,
+            and to log any exception thrown. This allows avoiding a roundtrip
+            in places where the outcome of a call is highly likely to succeed,
+            and subsequent actions will fail regardless with a meaningful
+            exception if the no_reply call failed.
+
         :returns:
-            mitogen.core.Receiver that receives the function call result.
+            :class:`mitogen.core.Receiver` that receives the function call result.
         """
         self._connect()
+
         if kwargs.pop('use_login_context', None):
             call_context = self.login_context
         else:
             call_context = self.context
-        return call_context.call_async(func, *args, **kwargs)
+
+        if kwargs.pop('no_reply', None):
+            return call_context.call_no_reply(func, *args, **kwargs)
+        else:
+            return call_context.call_async(func, *args, **kwargs)
 
     def call(self, func, *args, **kwargs):
         """
@@ -713,7 +727,10 @@ class Connection(ansible.plugins.connection.ConnectionBase):
         """
         t0 = time.time()
         try:
-            return self.call_async(func, *args, **kwargs).get().unpickle()
+            recv = self.call_async(func, *args, **kwargs)
+            if recv is None:  # no_reply=True
+                return None
+            return recv.get().unpickle()
         finally:
             LOG.debug('Call took %d ms: %r', 1000 * (time.time() - t0),
                       mitogen.parent.CallSpec(func, args, kwargs))
@@ -786,19 +803,33 @@ class Connection(ansible.plugins.connection.ConnectionBase):
 
     def put_data(self, out_path, data, mode=None, utimes=None):
         """
-        Implement put_file() by caling the corresponding
+        Implement put_file() by caling the corresponding ansible_mitogen.target
-        ansible_mitogen.target function in the target.
+        function in the target, transferring small files inline.
 
         :param str out_path:
             Remote filesystem path to write.
         :param byte data:
             File contents to put.
         """
+        # no_reply=True here avoids a roundrip that 99% of the time will report
+        # a successful response. If the file transfer fails, the target context
+        # will dump an exception into the logging framework, which will appear
+        # on console, and the missing file will cause the subsequent task step
+        # to fail regardless. This is safe since CALL_FUNCTION is presently
+        # single-threaded for each target, so subsequent steps cannot execute
+        # until the transfer RPC has completed.
         self.call(ansible_mitogen.target.write_path,
                   mitogen.utils.cast(out_path),
                   mitogen.core.Blob(data),
                   mode=mode,
-                  utimes=utimes)
+                  utimes=utimes,
+                  no_reply=True)
+
+    #: Maximum size of a small file before switching to streaming file
+    #: transfer. This should really be the same as
+    #: mitogen.services.FileService.IO_SIZE, however the message format has
+    #: slightly more overhead, so just randomly subtract 4KiB.
+    SMALL_FILE_LIMIT = mitogen.core.CHUNK_SIZE - 4096
 
     def put_file(self, in_path, out_path):
         """
@@ -817,14 +848,14 @@ class Connection(ansible.plugins.connection.ConnectionBase):
         # If the file is sufficiently small, just ship it in the argument list
         # rather than introducing an extra RTT for the child to request it from
         # FileService.
-        if st.st_size <= 32768:
+        if st.st_size <= self.SMALL_FILE_LIMIT:
             fp = open(in_path, 'rb')
             try:
-                s = fp.read(32769)
+                s = fp.read(self.SMALL_FILE_LIMIT + 1)
             finally:
                 fp.close()
 
-            # Ensure file was not growing during call.
+            # Ensure did not grow during read.
             if len(s) == st.st_size:
                 return self.put_data(out_path, s, mode=st.st_mode,
                                      utimes=(st.st_atime, st.st_mtime))

ansible_mitogen/mixins.py

@@ -216,6 +216,11 @@ class ActionModuleMixin(ansible.plugins.action.ActionBase):
         self._connection.put_data(remote_path, data)
         return remote_path
 
+    #: Actions listed here cause :func:`_fixup_perms2` to avoid a needless
+    #: roundtrip, as they modify file modes separately afterwards. This is due
+    #: to the method prototype having a default of `execute=True`.
+    FIXUP_PERMS_RED_HERRING = set(['copy'])
+
     def _fixup_perms2(self, remote_paths, remote_user=None, execute=True):
         """
         Mitogen always executes ActionBase helper methods in the context of the
@@ -224,7 +229,7 @@ class ActionModuleMixin(ansible.plugins.action.ActionBase):
         """
         LOG.debug('_fixup_perms2(%r, remote_user=%r, execute=%r)',
                   remote_paths, remote_user, execute)
-        if execute:
+        if execute and self._load_name not in self.FIXUP_PERMS_RED_HERRING:
             return self._remote_chmod(remote_paths, mode='u+x')
         return self.COMMAND_RESULT.copy()
 

ansible_mitogen/process.py

@@ -133,7 +133,7 @@ class MuxProcess(object):
         mitogen.core.set_cloexec(cls.worker_sock.fileno())
         mitogen.core.set_cloexec(cls.child_sock.fileno())
 
-        if os.environ.get('MITOGEN_PROFILING', '1'):
+        if os.environ.get('MITOGEN_PROFILING'):
             mitogen.core.enable_profiling()
 
         cls.original_env = dict(os.environ)

docs/ansible.rst

@@ -312,10 +312,10 @@ where readers may observe inconsistent file contents.
 Performance
 ^^^^^^^^^^^
 
-One roundtrip initiates a transfer larger than 32KiB, while smaller transfers
+One roundtrip initiates a transfer larger than 124 KiB, while smaller transfers
-are embedded in the initiating RPC. For tools operating via SSH multiplexing, 4
+are embedded in a 0-roundtrip remote call. For tools operating via SSH
-roundtrips are required to configure the IO channel, in addition to the time to
+multiplexing, 4 roundtrips are required to configure the IO channel, in
-start the local and remote processes.
+addition to the time to start the local and remote processes.
 
 An invocation of ``scp`` with an empty ``.profile`` over a 30 ms link takes
 ~140 ms, wasting 110 ms per invocation, rising to ~2,000 ms over a 400 ms

docs/changelog.rst

@@ -32,27 +32,36 @@ Mitogen for Ansible
   extension that had been installed using the documented steps. Now the bundled
   library always overrides over any system-installed copy.
 
-* `#315 <https://github.com/dw/mitogen/pull/315>`_: Mitogen for Ansible is
+* `#315 <https://github.com/dw/mitogen/pull/315>`_: Ansible 2.6 is now
-  supported under Ansible 2.6.
+  supported.
 
 * `#321 <https://github.com/dw/mitogen/issues/321>`_,
   `#336 <https://github.com/dw/mitogen/issues/336>`_: temporary file handling
-  has been simplified and additional network roundtrips have been removed,
+  was simplified, undoing earlier damage caused by compatibility fixes,
-  undoing earlier damage caused by compatibility fixes, and improving 2.6
+  improving 2.6 compatibility, and avoiding two network roundtrips for every
-  compatibility. One directory is created at startup for each persistent
+  related action
-  interpreter. See :ref:`ansible_tempfiles` for a complete description.
+  (`assemble <http://docs.ansible.com/ansible/latest/modules/assemble_module.html>`_,
-
+  `aws_s3 <http://docs.ansible.com/ansible/latest/modules/aws_s3_module.html>`_,
-* `#324 <https://github.com/dw/mitogen/issues/324>`_: plays with a custom
+  `copy <http://docs.ansible.com/ansible/latest/modules/copy_module.html>`_,
-  ``module_utils`` would fail due to fallout from the Python 3 port and related
+  `patch <http://docs.ansible.com/ansible/latest/modules/patch_module.html>`_,
-  tests being disabled.
+  `script <http://docs.ansible.com/ansible/latest/modules/script_module.html>`_,
+  `template <http://docs.ansible.com/ansible/latest/modules/template_module.html>`_,
+  `unarchive <http://docs.ansible.com/ansible/latest/modules/unarchive_module.html>`_,
+  `uri <http://docs.ansible.com/ansible/latest/modules/uri_module.html>`_). See
+  :ref:`ansible_tempfiles` for a complete description.
+
+* `#324 <https://github.com/dw/mitogen/issues/324>`_: plays with a
+  `custom module_utils <https://docs.ansible.com/ansible/latest/reference_appendices/config.html#default-module-utils-path>`_
+  would fail due to fallout from the Python 3 port and related tests being
+  disabled.
 
 * `#331 <https://github.com/dw/mitogen/issues/331>`_: fixed known issue: the
   connection multiplexer subprocess always exits before the main Ansible
-  process exits, ensuring logs generated by it do not overwrite the user's
+  process, ensuring logs generated by it do not overwrite the user's prompt
-  prompt when ``-vvv`` is enabled.
+  when ``-vvv`` is enabled.
 
 * `#332 <https://github.com/dw/mitogen/issues/332>`_: support a new
-  :data:`sys.excepthook`-based module exit mechanism added in Ansible 2.6.
+  :func:`sys.excepthook`-based module exit mechanism added in Ansible 2.6.
 
 * `#338 <https://github.com/dw/mitogen/issues/338>`_: compatibility: changes to
   ``/etc/environment`` and ``~/.pam_environment`` made by a task are reflected
@@ -63,21 +72,33 @@ Mitogen for Ansible
   option is supported.
 
 * `#344 <https://github.com/dw/mitogen/issues/344>`_: connections no longer
-  fail when the parent machine's logged in username contains slashes.
+  fail when the controller's login username contains slashes.
 
 * `#345 <https://github.com/dw/mitogen/issues/345>`_: the ``IdentitiesOnly
-  yes`` option is no longer supplied to OpenSSH by default, more closely
+  yes`` option is no longer supplied to OpenSSH by default, better matching
-  mimicking Ansible's default behaviour.
+  Ansible's behaviour.
-
+
-* Runs with many targets executed the module dependency scanner redundantly
+* `084c0ac0 <https://github.com/dw/mitogen/commit/084c0ac0>`_: avoid a
-  due to missing synchronization, causing significant wasted computation in the
+  roundtrip in
-  connection multiplexer subprocess. For one real-world playbook the scanner
+  `copy <http://docs.ansible.com/ansible/latest/modules/copy_module.html>`_ and
-  runtime was reduced by 95%, which may manifest as shorter runs.
+  `template <http://docs.ansible.com/ansible/latest/modules/template_module.html>`_
+  due to an unfortunate default.
+
+* `7458dfae <https://github.com/dw/mitogen/commit/7458dfae>`_: avoid a
+  roundtrip when transferring files smaller than 124KiB. Copy and template
+  actions are now 2-RTT, reducing runtime for a 20-iteration template loop over
+  a 250 ms link from 30 seconds to 10 seconds compared to v0.2.2, down from 120
+  seconds compared to vanilla.
+
+* `d62e6e2a <https://github.com/dw/mitogen/commit/d62e6e2a>`_: many-target
+  runs executed the dependency scanner redundantly due to missing
+  synchronization, wasting significant runtime in the connection multiplexer.
+  In one case work was reduced by 95%, which may manifest as faster runs.
 
 * A missing check caused an exception traceback to appear when using the
   ``ansible`` command-line tool with a missing or misspelled module name.
 
-* Ansible since >2.6 began importing ``__main__`` from
+* Ansible since >=2.7 began importing ``__main__`` from
   ``ansible.module_utils.basic``, causing an error during execution, due to the
   controller being configured to refuse network imports outside the
   ``ansible.*`` namespace. Update the target implementation to construct a stub

tests/ansible/bench/loop-20-templates.yml

@@ -0,0 +1,14 @@
+
+- hosts: all
+  tasks:
+  - file:
+      dest: /tmp/templates
+      state: "{{item}}"
+    with_items: ["absent", "directory"]
+
+  - copy:
+      dest: /tmp/templates/{{item}}
+      mode: 0755
+      content:
+        Hello from {{item}}
+    with_sequence: start=1 end=20

tests/ansible/integration/action/all.yml

@@ -1,5 +1,6 @@
-- import_playbook: remote_file_exists.yml
+- import_playbook: fixup_perms2__copy.yml
-- import_playbook: remote_expand_user.yml
 - import_playbook: low_level_execute_command.yml
 - import_playbook: make_tmp_path.yml
+- import_playbook: remote_expand_user.yml
+- import_playbook: remote_file_exists.yml
 - import_playbook: transfer_data.yml

tests/ansible/integration/action/fixup_perms2__copy.yml

@@ -0,0 +1,104 @@
+# Verify action plugins still set file modes correctly even though
+# fixup_perms2() avoids setting execute bit despite being asked to.
+
+- name: integration/action/fixup_perms2__copy.yml
+  hosts: test-targets
+  any_errors_fatal: true
+  tasks:
+    - name: Get default remote file mode
+      shell: python -c 'import os; print("%04o" % (int("0666", 8) & ~os.umask(0)))'
+      register: py_umask
+
+    - name: Set default file mode
+      set_fact:
+        mode: "{{py_umask.stdout}}"
+
+    #
+    # copy module (no mode).
+    #
+
+    - name: "Copy files (no mode)"
+      copy:
+        content: ""
+        dest: /tmp/copy-no-mode
+
+    - stat: path=/tmp/copy-no-mode
+      register: out
+    - assert:
+        that:
+        -  out.stat.mode == mode
+
+    #
+    # copy module (explicit mode).
+    #
+
+    - name: "Copy files from content: arg"
+      copy:
+        content: ""
+        mode: 0400
+        dest: /tmp/copy-with-mode
+
+    - stat: path=/tmp/copy-with-mode
+      register: out
+    - assert:
+        that:
+        -  out.stat.mode == "0400"
+
+    #
+    # copy module (existing disk files, no mode).
+    #
+
+    - file:
+        path: /tmp/weird-mode
+        state: absent
+
+    - name: Create local test file.
+      connection: local
+      copy:
+        content: "weird mode"
+        dest: "/tmp/weird-mode"
+        mode: "1462"
+
+    - copy:
+        src: "/tmp/weird-mode"
+        dest: "/tmp/weird-mode"
+
+    - stat:
+        path: "/tmp/weird-mode"
+      register: out
+    - assert:
+        that:
+        - out.stat.mode == mode
+
+    #
+    # copy module (existing disk files, preserve mode).
+    #
+
+    - copy:
+        src: "/tmp/weird-mode"
+        dest: "/tmp/weird-mode"
+        mode: preserve
+
+    - stat:
+        path: "/tmp/weird-mode"
+      register: out
+    - assert:
+        that:
+        - out.stat.mode == "1462"
+
+    #
+    # copy module (existing disk files, explicit mode).
+    #
+
+    - copy:
+        src: "/tmp/weird-mode"
+        dest: "/tmp/weird-mode"
+        mode: "1461"
+
+    - stat:
+        path: "/tmp/weird-mode"
+      register: out
+
+    - assert:
+        that:
+        - out.stat.mode == "1461"