diff --git a/ansible_mitogen/connection.py b/ansible_mitogen/connection.py index 30d23a4f..966f86c1 100644 --- a/ansible_mitogen/connection.py +++ b/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 - ansible_mitogen.target function in the target. + Implement put_file() by caling the corresponding ansible_mitogen.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)) diff --git a/ansible_mitogen/mixins.py b/ansible_mitogen/mixins.py index 7d8f5518..7be5444f 100644 --- a/ansible_mitogen/mixins.py +++ b/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() diff --git a/ansible_mitogen/process.py b/ansible_mitogen/process.py index 5ce1b8be..e7ceb2ce 100644 --- a/ansible_mitogen/process.py +++ b/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) diff --git a/docs/ansible.rst b/docs/ansible.rst index 790d0369..514708bd 100644 --- a/docs/ansible.rst +++ b/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 -are embedded in the initiating RPC. For tools operating via SSH multiplexing, 4 -roundtrips are required to configure the IO channel, in addition to the time to -start the local and remote processes. +One roundtrip initiates a transfer larger than 124 KiB, while smaller transfers +are embedded in a 0-roundtrip remote call. For tools operating via SSH +multiplexing, 4 roundtrips are required to configure the IO channel, in +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 diff --git a/docs/changelog.rst b/docs/changelog.rst index 64dfd471..2a094476 100644 --- a/docs/changelog.rst +++ b/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 `_: Mitogen for Ansible is - supported under Ansible 2.6. +* `#315 `_: Ansible 2.6 is now + supported. * `#321 `_, `#336 `_: temporary file handling - has been simplified and additional network roundtrips have been removed, - undoing earlier damage caused by compatibility fixes, and improving 2.6 - compatibility. One directory is created at startup for each persistent - interpreter. See :ref:`ansible_tempfiles` for a complete description. - -* `#324 `_: plays with a custom - ``module_utils`` would fail due to fallout from the Python 3 port and related - tests being disabled. + was simplified, undoing earlier damage caused by compatibility fixes, + improving 2.6 compatibility, and avoiding two network roundtrips for every + related action + (`assemble `_, + `aws_s3 `_, + `copy `_, + `patch `_, + `script `_, + `template `_, + `unarchive `_, + `uri `_). See + :ref:`ansible_tempfiles` for a complete description. + +* `#324 `_: plays with a + `custom module_utils `_ + would fail due to fallout from the Python 3 port and related tests being + disabled. * `#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 - prompt when ``-vvv`` is enabled. + process, ensuring logs generated by it do not overwrite the user's prompt + when ``-vvv`` is enabled. * `#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 `_: 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 `_: connections no longer - fail when the parent machine's logged in username contains slashes. + fail when the controller's login username contains slashes. * `#345 `_: the ``IdentitiesOnly - yes`` option is no longer supplied to OpenSSH by default, more closely - mimicking Ansible's default behaviour. - -* Runs with many targets executed the module dependency scanner redundantly - due to missing synchronization, causing significant wasted computation in the - connection multiplexer subprocess. For one real-world playbook the scanner - runtime was reduced by 95%, which may manifest as shorter runs. + yes`` option is no longer supplied to OpenSSH by default, better matching + Ansible's behaviour. + +* `084c0ac0 `_: avoid a + roundtrip in + `copy `_ and + `template `_ + due to an unfortunate default. + +* `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 `_: 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 diff --git a/tests/ansible/bench/loop-20-templates.yml b/tests/ansible/bench/loop-20-templates.yml new file mode 100644 index 00000000..df994bd8 --- /dev/null +++ b/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 diff --git a/tests/ansible/integration/action/all.yml b/tests/ansible/integration/action/all.yml index ebbff26a..22904c23 100644 --- a/tests/ansible/integration/action/all.yml +++ b/tests/ansible/integration/action/all.yml @@ -1,5 +1,6 @@ -- import_playbook: remote_file_exists.yml -- import_playbook: remote_expand_user.yml +- import_playbook: fixup_perms2__copy.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 diff --git a/tests/ansible/integration/action/fixup_perms2__copy.yml b/tests/ansible/integration/action/fixup_perms2__copy.yml new file mode 100644 index 00000000..17bfbed9 --- /dev/null +++ b/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"