From e18396d54d9e24f9815bbc42d0764683383cddb7 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Mon, 20 Aug 2018 12:48:52 +0100 Subject: [PATCH 1/6] ansible: enable profiling by default! Thankfully this never made it into a release --- ansible_mitogen/process.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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) From 084c0ac06513196d6524add5ce4021b156d06f19 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Mon, 20 Aug 2018 13:24:09 +0100 Subject: [PATCH 2/6] ansible: avoid roundtrip in copy action due to fixup_perms2(). On top of existing temporary files work, this reduces the number of roundtrips required for "copy" and "template" actions from 6 to 3. --- ansible_mitogen/mixins.py | 7 +- tests/ansible/integration/action/all.yml | 5 +- .../integration/action/fixup_perms2__copy.yml | 104 ++++++++++++++++++ 3 files changed, 113 insertions(+), 3 deletions(-) create mode 100644 tests/ansible/integration/action/fixup_perms2__copy.yml 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/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" From 84521b714fa79404b6d7b3d9e397eb0bd956431d Mon Sep 17 00:00:00 2001 From: David Wilson Date: Mon, 20 Aug 2018 13:40:48 +0100 Subject: [PATCH 3/6] docs: update changelog. --- docs/changelog.rst | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 64dfd471..ea44f73c 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -37,10 +37,18 @@ Mitogen for Ansible * `#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. + 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 @@ -48,8 +56,8 @@ Mitogen for Ansible * `#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. @@ -69,6 +77,12 @@ Mitogen for Ansible yes`` option is no longer supplied to OpenSSH by default, more closely mimicking Ansible's default behaviour. +* `084c0ac0 `_: avoid a + needless roundtrip for each invocation of the + `copy `_ and + `template `_ + actions, due to an unfortunate default parameter. + * 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 @@ -77,7 +91,7 @@ Mitogen for Ansible * 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 From 8e9b5ad5766fec79aa34badab3f95dedd7310163 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Mon, 20 Aug 2018 13:44:33 +0100 Subject: [PATCH 4/6] tests: import template benchmark script. --- tests/ansible/bench/loop-20-templates.yml | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 tests/ansible/bench/loop-20-templates.yml 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 From 7458dfae855d86512e85c2a1068ec80eac925ad7 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Mon, 20 Aug 2018 14:11:46 +0100 Subject: [PATCH 5/6] ansible: avoid roundtrip for small file transfers. Calls to connect.put_file() where the file is sufficiently small enough to fit in a single RPC proceed without waiting for an RPC response. If the write fails the target context will log an exception, and any subsequent step depending on the written file will fail. I verified every built-in action plugin for file transfer calls, and they all depend on the transferred file in the following step, so this should be safe. Reduces template/copy actions to 2-RTT, loop-20-templates.yml runtime reduced from 30 seconds to 10 seconds over a 250ms link compared to v0.2.2, and from 123 seconds compared to vanilla with pipelining enabled. --- ansible_mitogen/connection.py | 49 ++++++++++++++++++++++++++++------- docs/ansible.rst | 8 +++--- 2 files changed, 44 insertions(+), 13 deletions(-) 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/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 From 52cd7fddc12cdabd0eded3d637a5e4b849a7b975 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Mon, 20 Aug 2018 14:23:30 +0100 Subject: [PATCH 6/6] docs: update changelog. --- docs/changelog.rst | 39 +++++++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 16 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index ea44f73c..2a094476 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -32,8 +32,8 @@ 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 @@ -50,9 +50,10 @@ Mitogen for Ansible `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. +* `#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 @@ -60,7 +61,7 @@ Mitogen for Ansible 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 @@ -71,22 +72,28 @@ 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. + yes`` option is no longer supplied to OpenSSH by default, better matching + Ansible's behaviour. * `084c0ac0 `_: avoid a - needless roundtrip for each invocation of the + roundtrip in `copy `_ and `template `_ - actions, due to an unfortunate default parameter. - -* 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. + 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.