From 17548d1e49ab548379d4579c8db6ddec823aba33 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yannig=20Perr=C3=A9?= Date: Thu, 20 Sep 2018 21:20:39 +0200 Subject: [PATCH 01/28] [Enhancement] handle kubectl vars from Ansible connector. This change allows the kubectl connector to support the same options as Ansible's original connector. The playbook sample comes with an example of a pod containing two containers and checking that moving from one container to another, the version of Python changes as expected. --- ansible_mitogen/connection.py | 8 +- .../plugins/connection/mitogen_kubectl.py | 11 +++ mitogen/kubectl.py | 34 +++----- tests/ansible/test-kubectl.yml | 79 ++++++++++++++++--- 4 files changed, 97 insertions(+), 35 deletions(-) diff --git a/ansible_mitogen/connection.py b/ansible_mitogen/connection.py index 60724fa3..ce36bc53 100644 --- a/ansible_mitogen/connection.py +++ b/ansible_mitogen/connection.py @@ -132,11 +132,10 @@ def _connect_kubectl(spec): return { 'method': 'kubectl', 'kwargs': { - 'username': spec['remote_user'], 'pod': spec['remote_addr'], - #'container': spec['container'], 'python_path': spec['python_path'], 'connect_timeout': spec['ansible_ssh_timeout'] or spec['timeout'], + 'additional_parameters': spec['additional_parameters'], } } @@ -392,6 +391,8 @@ def config_from_play_context(transport, inventory_name, connection): connection.get_task_var('mitogen_machinectl_path'), 'mitogen_ssh_debug_level': connection.get_task_var('mitogen_ssh_debug_level'), + 'additional_parameters': + connection.get_additional_parameters(), } @@ -571,6 +572,9 @@ class Connection(ansible.plugins.connection.ConnectionBase): def connected(self): return self.context is not None + def get_additional_parameters(self): + return [] + def _config_from_via(self, via_spec): """ Produce a dict connection specifiction given a string `via_spec`, of diff --git a/ansible_mitogen/plugins/connection/mitogen_kubectl.py b/ansible_mitogen/plugins/connection/mitogen_kubectl.py index 43d162f8..acbe9a39 100644 --- a/ansible_mitogen/plugins/connection/mitogen_kubectl.py +++ b/ansible_mitogen/plugins/connection/mitogen_kubectl.py @@ -31,6 +31,8 @@ from __future__ import absolute_import import os.path import sys +iteritems = getattr(dict, 'iteritems', dict.items) + try: import ansible_mitogen except ImportError: @@ -40,6 +42,15 @@ except ImportError: import ansible_mitogen.connection +import ansible.plugins.connection.kubectl class Connection(ansible_mitogen.connection.Connection): transport = 'kubectl' + + def get_additional_parameters(self): + parameters = [] + for key, option in iteritems(ansible.plugins.connection.kubectl.CONNECTION_OPTIONS): + if self.get_task_var('ansible_' + key) is not None: + parameters += [ option, self.get_task_var('ansible_' + key) ] + + return parameters diff --git a/mitogen/kubectl.py b/mitogen/kubectl.py index 2dfaa232..d9a70927 100644 --- a/mitogen/kubectl.py +++ b/mitogen/kubectl.py @@ -32,6 +32,9 @@ import logging import mitogen.core import mitogen.parent +import ansible.plugins.connection.kubectl + +from ansible.module_utils.six import iteritems LOG = logging.getLogger(__name__) @@ -42,37 +45,26 @@ class Stream(mitogen.parent.Stream): container = None username = None kubectl_path = 'kubectl' + kubectl_options = [] # TODO: better way of capturing errors such as "No such container." create_child_args = { 'merge_stdio': True } - def construct(self, pod = None, container=None, - kubectl_path=None, username=None, - **kwargs): - assert pod + def construct(self, pod = None, kubectl_path=None, + additional_parameters=None, **kwargs): + assert(pod) super(Stream, self).construct(**kwargs) - if pod: - self.pod = pod - if container: - self.container = container - if kubectl_path: - self.kubectl_path = kubectl_path - if username: - self.username = username + + self.kubectl_options = additional_parameters + self.pod = pod def connect(self): super(Stream, self).connect() - self.name = u'kubectl.' + (self.pod) + str(self.container) + self.name = u'kubectl.' + (self.pod) + str(self.container) + str(self.kubectl_options) def get_boot_command(self): - args = ['exec', '-it', self.pod] - if self.username: - args += ['--username=' + self.username] - - if self.container: - args += ['--container=' + self.container] - bits = [self.kubectl_path] + bits = [self.kubectl_path] + self.kubectl_options + ['exec', '-it', self.pod] - return bits + args + [ "--" ] + super(Stream, self).get_boot_command() + return bits + [ "--" ] + super(Stream, self).get_boot_command() diff --git a/tests/ansible/test-kubectl.yml b/tests/ansible/test-kubectl.yml index 05fc6517..d2be9ba5 100644 --- a/tests/ansible/test-kubectl.yml +++ b/tests/ansible/test-kubectl.yml @@ -1,8 +1,11 @@ --- - name: "Create pod" - tags: always + tags: create hosts: localhost + vars: + pod_count: 10 + loop_count: 5 gather_facts: no tasks: - name: Create a test pod @@ -19,7 +22,10 @@ - name: python2 image: python:2 args: [ "sleep", "100000" ] - loop: "{{ range(10)|list }}" + - name: python3 + image: python:3 + args: [ "sleep", "100000" ] + loop: "{{ range(pod_count|int)|list }}" - name: "Wait pod to be running" debug: { msg: "pod is running" } @@ -30,7 +36,7 @@ delay: 2 vars: pod_def: "{{lookup('k8s', kind='Pod', namespace='default', resource_name='test-pod-' ~ item)}}" - loop: "{{ range(10)|list }}" + loop: "{{ range(pod_count|int)|list }}" - name: "Add pod to pods group" add_host: @@ -39,45 +45,95 @@ ansible_connection: "kubectl" changed_when: no tags: "always" - loop: "{{ range(10)|list }}" + loop: "{{ range(pod_count|int)|list }}" - name: "Test kubectl connection (default strategy)" tags: default hosts: pods strategy: "linear" + vars: + pod_count: 10 + loop_count: 5 gather_facts: no tasks: - name: "Simple shell with linear" shell: ls /tmp - loop: [ 1, 2, 3, 4, 5 ] + loop: "{{ range(loop_count|int)|list }}" - name: "Simple file with linear" file: path: "/etc" state: directory - loop: [ 1, 2, 3, 4, 5 ] + loop: "{{ range(loop_count|int)|list }}" + + - block: + - name: "Check python version on python3 container" + command: python --version + vars: + ansible_kubectl_container: python3 + register: _ + + - assert: { that: "'Python 3' in _.stdout" } + + - debug: var=_.stdout,_.stderr + run_once: yes + + - name: "Check python version on default container" + command: python --version + register: _ + + - assert: { that: "'Python 2' in _.stderr" } + + - debug: var=_.stdout,_.stderr + run_once: yes - name: "Test kubectl connection (mitogen strategy)" tags: mitogen hosts: pods strategy: "mitogen_linear" + vars: + pod_count: 10 + loop_count: 5 gather_facts: no tasks: - name: "Simple shell with mitogen" shell: ls /tmp - loop: [ 1, 2, 3, 4, 5 ] + loop: "{{ range(loop_count|int)|list }}" - name: "Simple file with mitogen" file: path: "/etc" state: directory - loop: [ 1, 2, 3, 4, 5 ] - register: _ + loop: "{{ range(loop_count|int)|list }}" + + - block: + - name: "Check python version on python3 container" + command: python --version + vars: + ansible_kubectl_container: python3 + register: _ + + - assert: { that: "'Python 3' in _.stdout" } + + - debug: var=_.stdout,_.stderr + run_once: yes + + - name: "Check python version on default container" + command: python --version + register: _ + + - assert: { that: "'Python 2' in _.stderr" } + + - debug: var=_.stdout,_.stderr + run_once: yes + tags: check - name: "Destroy pod" tags: cleanup - hosts: localhost + hosts: pods gather_facts: no + vars: + ansible_connection: "local" tasks: - name: Destroy pod k8s: @@ -86,6 +142,5 @@ apiVersion: v1 kind: Pod metadata: - name: test-pod-{{item}} + name: "{{inventory_hostname}}" namespace: default - loop: "{{ range(10)|list }}" From 3660febeb239dedbc09ffc234484fe5f9f10ff0d Mon Sep 17 00:00:00 2001 From: David Wilson Date: Mon, 10 Sep 2018 19:23:37 +0100 Subject: [PATCH 02/28] docs: add inline subscribe form to installation instructions --- docs/ansible.rst | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/docs/ansible.rst b/docs/ansible.rst index 485c24dc..e4619ef9 100644 --- a/docs/ansible.rst +++ b/docs/ansible.rst @@ -78,9 +78,27 @@ Installation deploy = (ALL) NOPASSWD:/usr/bin/python -c* -5. Subscribe to the `mitogen-announce mailing list - `_ to stay updated with new - releases and important bug fixes. +5. + + .. raw:: html + +
+ Releases occur frequently and often include important fixes. Subscribe + to the mitogen-announce + mailing list be notified of new releases. + +

+ + + + + +

+
+ Demo From 43ad23946e45ca6cfd482254386a82713647f93e Mon Sep 17 00:00:00 2001 From: David Wilson Date: Mon, 10 Sep 2018 19:25:31 +0100 Subject: [PATCH 03/28] docs: tidy up wording. --- docs/changelog.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 162a7c48..2c219c4d 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -71,11 +71,11 @@ Enhancements * The `faulthandler `_ module is automatically activated if it is installed, simplifying debugging of hangs. - See :ref:`diagnosing-hangs` for more information. + See :ref:`diagnosing-hangs` for details. * The ``MITOGEN_DUMP_THREAD_STACKS`` environment variable's value now indicates the number of seconds between stack dumps. See :ref:`diagnosing-hangs` for - more information. + details. Fixes From 638e473ff1233a198a2bf0bb4e9e961fb74c1ec6 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Mon, 10 Sep 2018 19:43:32 +0100 Subject: [PATCH 04/28] tests: hacksmash synchronize test to work Avoid password typing idiocy. --- .../integration/action/synchronize.yml | 26 ++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/tests/ansible/integration/action/synchronize.yml b/tests/ansible/integration/action/synchronize.yml index f9a8cb2c..2672b08e 100644 --- a/tests/ansible/integration/action/synchronize.yml +++ b/tests/ansible/integration/action/synchronize.yml @@ -3,7 +3,22 @@ - name: integration/action/synchronize.yml hosts: test-targets any_errors_fatal: true + vars: + ansible_user: mitogen__has_sudo_pubkey + ansible_ssh_private_key_file: /tmp/synchronize-action-key tasks: + # must copy git file to set proper file mode. + - copy: + dest: /tmp/synchronize-action-key + src: ../../../data/docker/mitogen__has_sudo_pubkey.key + mode: u=rw,go= + connection: local + + - file: + path: /tmp/sync-test + state: absent + connection: local + - file: path: /tmp/sync-test state: directory @@ -14,12 +29,17 @@ content: "item!" connection: local + - file: + path: /tmp/sync-test.out + state: absent + - synchronize: - dest: /tmp/sync-test - src: /tmp/sync-test + private_key: /tmp/synchronize-action-key + dest: /tmp/sync-test.out + src: /tmp/sync-test/ - slurp: - src: /tmp/sync-test/item + src: /tmp/sync-test.out/item register: out - set_fact: outout="{{out.content|b64decode}}" From 0cf908661ea2f53f26aa940a08d5d91ea88cfa3b Mon Sep 17 00:00:00 2001 From: David Wilson Date: Tue, 11 Sep 2018 03:36:32 +0000 Subject: [PATCH 05/28] tests: set Docker hostname for more readable exceptions --- .travis/ansible_tests.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.travis/ansible_tests.py b/.travis/ansible_tests.py index 0c47ab27..c7ddb989 100755 --- a/.travis/ansible_tests.py +++ b/.travis/ansible_tests.py @@ -24,9 +24,10 @@ with ci_lib.Fold('docker_setup'): --rm --detach --publish 0.0.0.0:%s:22/tcp + --hostname=target-%s --name=target-%s mitogen/%s-test - """, BASE_PORT + i, distro, distro,) + """, BASE_PORT + i, distro, distro, distro) with ci_lib.Fold('job_setup'): From e85760477bbdf91e1f554c2afc167fa6ca43f318 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Tue, 11 Sep 2018 03:39:39 +0000 Subject: [PATCH 06/28] tests: fix connection/_put_file.yml Was statting wrong destination path, and comparing floats that don't roundtrip serialization reliably. --- tests/ansible/integration/connection/_put_file.yml | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/ansible/integration/connection/_put_file.yml b/tests/ansible/integration/connection/_put_file.yml index e93a1e68..a0fea4ed 100644 --- a/tests/ansible/integration/connection/_put_file.yml +++ b/tests/ansible/integration/connection/_put_file.yml @@ -13,11 +13,10 @@ register: original connection: local -- stat: path=/tmp/{{file_name}} +- stat: path=/tmp/{{file_name}}.out register: copied - assert: that: - original.stat.checksum == copied.stat.checksum - #- original.stat.atime == copied.stat.atime - - original.stat.mtime == copied.stat.mtime + - original.stat.mtime|int == copied.stat.mtime|int From f6b74992e1861caf738c228ff36fa9a4cff44be9 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Tue, 11 Sep 2018 05:22:49 +0100 Subject: [PATCH 07/28] tests: fix apparently erroneous localhost delegation. The stack delegates to localhost, which has ansible_python_interpreter set. --- tests/ansible/integration/delegation/stack_construction.yml | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/ansible/integration/delegation/stack_construction.yml b/tests/ansible/integration/delegation/stack_construction.yml index beb9a9d1..4d9c75f4 100644 --- a/tests/ansible/integration/delegation/stack_construction.yml +++ b/tests/ansible/integration/delegation/stack_construction.yml @@ -331,9 +331,7 @@ out.result == [ { 'kwargs': { - 'python_path': [ - hostvars['cd-normal'].local_env.sys_executable - ], + 'python_path': None }, 'method': 'local', }, From 5521945bd2e9a601fa834dad8075824af4e74ed9 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Tue, 11 Sep 2018 03:44:17 +0100 Subject: [PATCH 08/28] ansible: temporary files take 5. --- ansible_mitogen/connection.py | 57 +++++++++------- ansible_mitogen/mixins.py | 36 ++++++---- ansible_mitogen/planner.py | 3 +- ansible_mitogen/runner.py | 40 +++++++++-- ansible_mitogen/target.py | 67 ++++++------------- docs/ansible.rst | 22 ++++-- .../integration/action/make_tmp_path.yml | 56 ++++------------ 7 files changed, 142 insertions(+), 139 deletions(-) diff --git a/ansible_mitogen/connection.py b/ansible_mitogen/connection.py index 60724fa3..ae6268de 100644 --- a/ansible_mitogen/connection.py +++ b/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 diff --git a/ansible_mitogen/mixins.py b/ansible_mitogen/mixins.py index faadbc15..d4fcbd0d 100644 --- a/ansible_mitogen/mixins.py +++ b/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, diff --git a/ansible_mitogen/planner.py b/ansible_mitogen/planner.py index 5b3e5547..caf40af3 100644 --- a/ansible_mitogen/planner.py +++ b/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) diff --git a/ansible_mitogen/runner.py b/ansible_mitogen/runner.py index fe5f4c46..44780aa2 100644 --- a/ansible_mitogen/runner.py +++ b/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), ) diff --git a/ansible_mitogen/target.py b/ansible_mitogen/target.py index 35863cb2..ae7990a9 100644 --- a/ansible_mitogen/target.py +++ b/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 diff --git a/docs/ansible.rst b/docs/ansible.rst index e4619ef9..d2138f21 100644 --- a/docs/ansible.rst +++ b/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__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: diff --git a/tests/ansible/integration/action/make_tmp_path.yml b/tests/ansible/integration/action/make_tmp_path.yml index 97da070d..dc713c31 100644 --- a/tests/ansible/integration/action/make_tmp_path.yml +++ b/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) From e58b6a8f056eb8f202fb9d543ba823dabf99384d Mon Sep 17 00:00:00 2001 From: David Wilson Date: Tue, 11 Sep 2018 05:14:30 +0000 Subject: [PATCH 09/28] tests: correct path for common-hosts --- .travis/ansible_tests.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis/ansible_tests.py b/.travis/ansible_tests.py index c7ddb989..723f9c1a 100755 --- a/.travis/ansible_tests.py +++ b/.travis/ansible_tests.py @@ -38,7 +38,7 @@ with ci_lib.Fold('job_setup'): run("pip install -q ansible==%s", ci_lib.ANSIBLE_VERSION) run("mkdir %s", HOSTS_DIR) - run("ln -s %s/common-hosts %s", TESTS_DIR, HOSTS_DIR) + run("ln -s %s/hosts/common-hosts %s", TESTS_DIR, HOSTS_DIR) with open(os.path.join(HOSTS_DIR, 'target'), 'w') as fp: fp.write('[test-targets]\n') From 564113874be90ead2394efbd0a5a1a3d4a4aa008 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Tue, 11 Sep 2018 06:21:16 +0100 Subject: [PATCH 10/28] tests: explicitly define localhost in common-hosts --- tests/ansible/hosts/common-hosts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/ansible/hosts/common-hosts b/tests/ansible/hosts/common-hosts index 449442f6..cf84d2d1 100644 --- a/tests/ansible/hosts/common-hosts +++ b/tests/ansible/hosts/common-hosts @@ -1,5 +1,11 @@ # vim: syntax=dosini + +# This must be defined explicitly, otherwise _create_implicit_localhost() +# generates its own copy, which includes an ansible_python_interpreter that +# varies according to host machine. +localhost + [connection-delegation-test] cd-bastion cd-rack11 mitogen_via=ssh-user@cd-bastion From dfb4930fce82cc0f59e3907c995ffbae3d8a116a Mon Sep 17 00:00:00 2001 From: David Wilson Date: Tue, 11 Sep 2018 06:40:56 +0100 Subject: [PATCH 11/28] tests: import custom binaries for tests It means Linux<->OS X runs work fine without manual hackery. --- .travis/ansible_tests.py | 2 +- tests/ansible/Makefile | 10 +++++--- .../runner/custom_binary_producing_json.yml | 21 +++++++++++++--- .../runner/custom_binary_producing_junk.yml | 23 ++++++++++++++---- .../custom_binary_producing_json_Darwin | Bin 0 -> 8540 bytes .../custom_binary_producing_json_Linux | Bin 0 -> 8400 bytes .../custom_binary_producing_junk_Darwin | Bin 0 -> 8540 bytes .../custom_binary_producing_junk_Linux | Bin 0 -> 8400 bytes 8 files changed, 42 insertions(+), 14 deletions(-) create mode 100755 tests/ansible/lib/modules/custom_binary_producing_json_Darwin create mode 100755 tests/ansible/lib/modules/custom_binary_producing_json_Linux create mode 100755 tests/ansible/lib/modules/custom_binary_producing_junk_Darwin create mode 100755 tests/ansible/lib/modules/custom_binary_producing_junk_Linux diff --git a/.travis/ansible_tests.py b/.travis/ansible_tests.py index 723f9c1a..3b5e40db 100755 --- a/.travis/ansible_tests.py +++ b/.travis/ansible_tests.py @@ -55,7 +55,7 @@ with ci_lib.Fold('job_setup'): )) # Build the binaries. - run("make -C %s", TESTS_DIR) + # run("make -C %s", TESTS_DIR) if not ci_lib.exists_in_path('sshpass'): run("sudo apt-get update") run("sudo apt-get install -y sshpass") diff --git a/tests/ansible/Makefile b/tests/ansible/Makefile index 00d6a8ab..1d4ab1dd 100644 --- a/tests/ansible/Makefile +++ b/tests/ansible/Makefile @@ -1,13 +1,15 @@ -TARGETS+=lib/modules/custom_binary_producing_junk -TARGETS+=lib/modules/custom_binary_producing_json +SYSTEM=$(shell uname -s) + +TARGETS+=lib/modules/custom_binary_producing_junk_$(SYSTEM) +TARGETS+=lib/modules/custom_binary_producing_json_$(SYSTEM) all: clean $(TARGETS) -lib/modules/custom_binary_producing_junk: lib/modules.src/custom_binary_producing_junk.c +lib/modules/custom_binary_producing_junk_$(SYSTEM): lib/modules.src/custom_binary_producing_junk.c $(CC) -o $@ $< -lib/modules/custom_binary_producing_json: lib/modules.src/custom_binary_producing_json.c +lib/modules/custom_binary_producing_json_$(SYSTEM): lib/modules.src/custom_binary_producing_json.c $(CC) -o $@ $< clean: diff --git a/tests/ansible/integration/runner/custom_binary_producing_json.yml b/tests/ansible/integration/runner/custom_binary_producing_json.yml index 00f03f07..4fe09f0d 100644 --- a/tests/ansible/integration/runner/custom_binary_producing_json.yml +++ b/tests/ansible/integration/runner/custom_binary_producing_json.yml @@ -1,12 +1,25 @@ - name: integration/runner/custom_binary_producing_json.yml hosts: test-targets any_errors_fatal: true + gather_facts: true tasks: - - custom_binary_producing_json: - foo: true - with_sequence: start=1 end={{end|default(1)}} - register: out + - block: + - custom_binary_producing_json_Darwin: + foo: true + with_sequence: start=1 end={{end|default(1)}} + register: out_darwin + - set_fact: out={{out_darwin}} + when: ansible_system == "Darwin" + - block: + - custom_binary_producing_json_Linux: + foo: true + with_sequence: start=1 end={{end|default(1)}} + register: out_linux + - set_fact: out={{out_linux}} + when: ansible_system == "Linux" + + - debug: msg={{out}} - assert: that: | out.changed and diff --git a/tests/ansible/integration/runner/custom_binary_producing_junk.yml b/tests/ansible/integration/runner/custom_binary_producing_junk.yml index 93d98065..b1672ad9 100644 --- a/tests/ansible/integration/runner/custom_binary_producing_junk.yml +++ b/tests/ansible/integration/runner/custom_binary_producing_junk.yml @@ -1,11 +1,24 @@ - name: integration/runner/custom_binary_producing_junk.yml hosts: test-targets + gather_facts: true tasks: - - custom_binary_producing_junk: - foo: true - with_sequence: start=1 end={{end|default(1)}} - ignore_errors: true - register: out + - block: + - custom_binary_producing_junk_Darwin: + foo: true + with_sequence: start=1 end={{end|default(1)}} + ignore_errors: true + register: out_darwin + - set_fact: out={{out_darwin}} + when: ansible_system == "Darwin" + + - block: + - custom_binary_producing_junk_Linux: + foo: true + with_sequence: start=1 end={{end|default(1)}} + ignore_errors: true + register: out_linux + - set_fact: out={{out_linux}} + when: ansible_system == "Linux" - hosts: test-targets diff --git a/tests/ansible/lib/modules/custom_binary_producing_json_Darwin b/tests/ansible/lib/modules/custom_binary_producing_json_Darwin new file mode 100755 index 0000000000000000000000000000000000000000..69de2fea2ef06ce88f94230d7f0eb94e2da4b653 GIT binary patch literal 8540 zcmeHMQEL=e6uxU?8&c6ILQ`6C+%}?;W>u0l1cgL1bVac-MhIfL>}F@PE0dXJW)=;l z5D~#3f&2ttee=P8pkn*fzO?w@gAY{<#J=c*6zTeXckXOvC&Bl?J?zF%Nx6b#ecEtH)&2bw}0ADuWkk0q5jwdGXZP`%YY#0S~Z%zkf^Udgd zS9CvdS~_y0L&~Oit?pH1-K+Wvch1KHosV-)Axi1yiSg<2R1fBlye0FP zwVYQxb4nQ|<(!|IR^ZL^bu*ZP{2ZSM`A^p=rRU0UVZQ9UvJnLv@^$O_^kXK7v-#3k zO1Jc}e0`aG#Od+LbDY$>zOK%lIe*rkIFqaiE9UKrA`j8Plms(R(nWtB>-H!cmJsBo z8}<-vB1f$_#Uw1_$Wb5kHZVEsIv~Uh?B~!)D24eGWqil_VL(b=SwgU;tYF!(B(-h| z%`hmq^>U%I;8t|tSFo((?A_C&$N&7ny*IGuyYibG|8QM^It=VldL+U0L2z%29LIH? z7Ou|`DEnCRwiAD&?3`#{8`rQ7uhczZYMhpXn0mFb{ zz%XDKFbo(5wjKkQic2qwH`jWL%foe4U%a!u2d}nI?DxEt;+>KE;?;h(mQP-VS+d{3 zu@|jlzc1NumwqeWeAN5%!puLvOz5xUN57UGoFCGMN9hN4=YW&1*u^mhZiMFCK!W@>`X{Horx7U}UcvP49X` zq0HZe{ti5^ZKm5m!+>GHFkl!k3>XFs1BL;^fMLKeU>GnA7zVZw1H1EmQ!ID)nB>uQ z#R-CjxDbeyF&@^b!*Pkd4EV(g>lbhYZPEe`@qlLoo_s52YhmJF)_y9Q#_#^-T literal 0 HcmV?d00001 diff --git a/tests/ansible/lib/modules/custom_binary_producing_json_Linux b/tests/ansible/lib/modules/custom_binary_producing_json_Linux new file mode 100755 index 0000000000000000000000000000000000000000..16e6d046516121ce619146739eb42eacdab12d0c GIT binary patch literal 8400 zcmeHMYit}>6~60@6X)Ue#!Xu10U1mpk~W?=FULt6;$1&7rp|-ZE>zT+tasP;mVH>e zvyB}eNe5I8s#8u23m5rO~_P*a?$6{e3Ufa2wxJNL6^ zX4e!X#IM}d%(>^g=bU@)bI!f{TzgkXh0iCD{Nkg6x$#OHX;B4dSEvk;7SSvg()$Bq zwU|%*Jc-%mEjFQ2JKa^LL8}z*C(3%w6*l0dtCbumJY=d|Qxa~{ywK)Bh0r9xdhF}@ zYRMASWCrwz@?%s{=rPNB<4SK_>48ou$3W3OZW}+{%AS|DQ5h#tmgRKtmKEdty9*>+ zLLakX0Sdj3lOFr|pQfM^Jf!NIt6vT(|3OuA#inGUuW8$+WUL{XNau$ehMSujnzre= zjJ`#-o9(mg-u*qYCLV8eE&IHT9>%!*EB7wE-tzKaPF+b~uRU_oS3g|&g;!aY`x4_O zGs*YfWa-@$N`pk~IfXnY7Y0ZPBuV>|BK$K^_@9 z$Cyw4gur~tuT0PitPp#+kP_Ax+BL2w=|#1UfR8G?%F_?vZ`5eg9*gmuK)UI`c|IXc zI&iEJE}L@T&h>KFfvcq_O<`Pk&w;O8@N*^4vY6M5Uv=QleaaYnv&uMGnO~#{C)74ug+5I}n6qnywsEK!L*0gcS?iFO9$2eWNl&G=uu2HyU ztsvK~kPN$4-CMlbuh*Hn4mPjCMvksf2iH0c|cru{}?%D6PS$MC&k zT${G)$lw>1!6j^PE_%(A1m|C*{6)%$-ig_! zNqeDBgr4jy{3g^BqP|>rX1?sI2BuGh3imn-ce*y*l4E)4o`3QqZ1TigmbUqKXkS<1 zA6Z7%t z9WjTpnOHuWNDr9LjzNtlvgY70FexoJy?wG(C&R&mD|~0>S4iTC3-0$suA{ z=d<)QQhA)H2nT{MRD=U{XZ-B}?ex5GVEw5|BhdK#d?V02wqReNB^ziC1sX$v_2Ga< zawH!PRLOnqGO2%(>^Rpso4lXy_rU!gxZeZ+&mKVhgm{U+aGXGw*sK^-!G}7wh~p}# z!7&bT*(xQEy>+GH5ueFDnlO~VH(kuI$T_tcBE~tVf-l5W4WRzY!#a?}=)T;5eL9vvLIZk$E-_zi-$qom=gM`Z+G=1i60}?eT28oV~ZTb(gljr!Swj z^4bo4tKQhKxhXH%ts89MeJt_0&Np+ecMAI{w!t~rjr)a#fp!%5=_HAH=77n5rNI1h zcxJu1@g-uk%y{@kwK!iE56s-Vxb4); ztY0@C6lXEv9R+?mQR4o<0aL%#ibhOyM}c2FFmq0Gz$Cv+U>|Vf%jrHub3!uj7Y_=o zTW6u4SE)MJx$)U^uN%iMHH-3knWE zrJd68`lFJ^cR;fAdRt~n=T9X$>MJw8s|c^9{vTEI5X<-x!ZjFm(9cV}bUy5rb{v8# z`xN0pnm<9${8=aEOV`DbIqW>A?0DzlDZ;hVYQ*l-?46TorOYVbI_2obDV8w@l9|3p z(u`S|Y|f12heb4#8cN2kcue2Xw6m$a5Z`wt%t$sH88PE&D?1|kvyoKXjOA0Q5mIq* zCP`W@(P%zrWm0C@V>P*InjMEi``XR+{cR>)gSqu%!fbnLe`sH4tGh(rw-H2lm}a}7 zEEsKvgt@otKseN89_Z+JqP^Sf4u!kg*{Zyyi{|pOCI9wbj_>?hT3oky@mR!)z{u=X zpGV4MDf>Q;WZjp0W-OO6>DC~|7k`}xNGg^{oB3Rv8aV44cOCJ4xtua1FBI*IM{i|x zmxwOoOUAM{hwi&dw*~uF(~Su|Hp<5+P*Jab8{u~- zhDQB}|3IOS&%HOC*-!d>#(@5~%7Ee=z}qPI6ZCN&c#<%V-Jy?o5)|hc5|N2*IMZQb z_>2I3#GRn{?*aZp4)oKc&u0$sh)+Q^Rhid+1&d>p;XZ}_5!Jz zSApUj#`+J^J29-GVXW~s&0qTJP=z*W2 z3}dKYQ*mQ(4t))ZioQpb>u>bn&|mApS(cNNV(!@`_PbftU#B{bzcU=O!hh_aJip<) gt!cjx@@7FP>@kjd?NW{@%j*}v>U|sRf2`WX( zOS!x9uKm5Klk^f+xQf5tAC`~1Erp(*cqIk_-etufS;dntTrJ49!ytYt0 z;$VOOJ@(UI*jGb?Cywg_gZ`e-T;9$*2Ianq6$@6(%p_x} zl9>XgY=kn8*}2(ePrn^`a`)o82XAig{KS0$>d>*p(Y-#V4T8Cq*^m1=C>*B;$~NZQ zaOf@4jx3Cg@kdGp+nDM++KD_eNjO1vK{;MobNSojVe+FyF`yVw3@8Q^1BwB~fMVeP zXW&eH`a}HsmuP&ZJA>KAZ#BI~@k0N_smE{i+!ym(S(`aLiDFv+0mE9<_P?6ezfZr6 zUwhz{{q2i#x!$v=jam_0OO{%IGPz)#r6a$I@#eiZ!F`yVw3@8Q^1BwB~z(2-7 z>-M(OOu4mB^6ENeSXN$~vP7kimvzqNxWw56?4m+Hs|}tLdE7|b6R=nsU=7hBBIQ(C z8dr_+qHW+FJi)H(c`UJ{RJOQg-q#<39@P`P?sx7S*F>C~K<6BS_j)*w;E{wQJMawR zju7T~N3fk|AH2t8BF+Fp^!YZJa6h~_>i{FQDf^RM#Ez9nS-r7X_rd*L-CYM5Qv3l+ CsOnPy literal 0 HcmV?d00001 diff --git a/tests/ansible/lib/modules/custom_binary_producing_junk_Linux b/tests/ansible/lib/modules/custom_binary_producing_junk_Linux new file mode 100755 index 0000000000000000000000000000000000000000..4aadc9c1d36a55b07f12a215347b8067891dba00 GIT binary patch literal 8400 zcmeHMU2I%O6`u9RiIdptO`5d%DdbWT;UswD{5ejWQ1ALD*VIW&VwYCbxmkPH_O|;I z?cPo72&qO%DpvU+AXEh*9uN|ON{GNqBObuPrG!2uARq`30TJX>Etr-@0TeIi%srod z_wJg4!~+i*Ywwx!%{gbzoS8fK%=pEQ?#?QoPoenL#}soD)eh1s4Hs8Sg-EMvQH$yM z0rjX_Nc;kgIqj_up;o8d6VsqIg8PZGUQ3k&crIH>gy?}zi({Y|ACHZHy<*Qx+o_EUD9du`xoyX|er=Iv z%jg~lR-n-P1nIG#|7i(|;34U6zPR*=|DZBlwI!JtXx_0U8EZ@?()rQG(U#`M<{hD2 zCbUhDo9(mgz5{)_C!TMFmVI79KgPKIl|7NQo4$2?a`V*To@>*wFP-~yLkG)pEHQ7o zA|@E${mxk|4dTknQv@01uv$vk@(Ot4Joqr-Mg{pt=8-=>kNgJ-f2@KXj?*%=N*O^z zw_^P$itiwYvsoR>+c}l9WASWO4V}m)?6|V5!O^HSlt@RDiIdE-w3V}?S=&lQ6KS=t zyDQRWZ3}G+?NnA*?|zH4y9*gmsK)L0@c|D;_ zxp2HA+&1mP-S5ji7cQ@!HidcNJqNyW!!?m-S*&Zuue)&fK4qSItHwNAonK-o^UQ=@ zRlIJVdaY(s6?bkUsJLMVJ?q!CQjPf$Hg)G#k;;Zm%=3`%T-SVr_@Rp|z-8;(U4FqvW{R+vN=j*m>viDCUQ(Sp>v7!D}zpkBI_O2!aedhVWtqkOq*Ayo-}>0 zn>S|cRb=q{f_pv+U z?AW`LWRk|i;{A7qNP=tB>r*6V7OsA89SMD(!$ux|o5<96XNpCtPjdZbs$Zds>Ytn& znzk4IWccZ>!Y{*pVT$F33k!9q8krsr7w&fz?sjjyt>^OUegD))*yNeFZDZ@NF~07? zKe`Kd+Y5gQ7gzkoJT>VvcmFp3CeQV!`oqtK`@=`V*5t*xPTwVev^~8Vcx^kYX@K7C z?3i^dn~CKI6X{{=`F#3?$BoR0kbA9L}YJfwW1LEqZiMKu?Fi-L^vMtg~D zY-Rx|mF1OMPwgz4}+8w?WpY=~GQ4SHp5rSob;l zZKKfcVk{g8zEss#w=hc%5z9KCrr$Hv9w(|If#Bz=B7s#G{2c+~{DMfJ;as&DXnJX( z8E84RXn&wJ8)yj!n!=0vsP^oS%Ma%@qjC8@zN4td#Hk;fjqMtJ0BydQHx`+GCR4D**{(?pJQNs=$* zR1?y^goUXfVOi?1l<_)crBD#$aF->I!rrXEB_xFJjtoa}9?qpj907iGfrG>En+{9o zR;QqU&da$#?w>_xd=ys=(dR*rw^i)XCsXr?`?T;4(*AehF%HN2|L%!%r`O`kGXAe) zLc9X+!z0h$*Vgv9(a<-LPuqE8S7>{vsc~y_UbEXbI>7r_;&YvE_FV54_ET#G!BsIU%8@kI*rQ;OFp%xfvWSYiE?;!9@NSt-6$;e9K` zYt?v#`S7bcb-5xQn7wx?wNpR){+8lFbrB2R)!?TSCGHPgFvV@Ts=-2cHTcy-v*$Dy zO!6xf_JLA-CEbS@Zb;Yt>S2ZV*4^mmw^Yt`rTE;rw-m=NHHY&1wm|*0IEOej-tVeZ zKuvq%84$b)8dS;L zt9nFXUxdgv7$=YL-pRJFUL~C45R^E?Md1d~Ag!w=kN;;x9`~#`Tm6`DmiLbLXCnWG ztY==YTyARIsk@(V5zhX5?eIOU;TDwoa+Ap&dv`Fg$C8! zos(s!tUfT%G%d`FcR*z4t91P?&!68!;$U|wyJOG26K5m zl7D+I$9I0MttGd3@mSQ3!pPiJpGV4KDd#?qWJ@pitXM8%jYQKizWD1pNK&yx+REqR zG{8CEICR7h5GMP1cPWQP|H%J9p^wkKH=Q{^`h3QK{)ALOaSq^Zl;Z?_ zoClsJjB|JBBcBAtIfg`ZVH@spgcv>}Kp%N0DE{{V{~-tZ0_pRa13dCmP(wQN`Y*6L zNfnMM^p8q_K{0;x?;ZbX(T|8hoU1@_4rBd?>F3qIKp4kFK2x)KG4f;j@|P5GZx$<1 z&8?6885I3PAt)8lFM9NmXM@6D=z|CSibo&cXV4#dCLI03-q$?(ICq13(>d?>zbX2d zKjayp&FDxd{096Tk3P-;pnD)F)Z70LJo-2<%u>(zz5V}$^f~v&`4{;m{=evz2M_vl z(ye#thJbQSvpc3&IN54)h=aiN8zaSmVKv1Yx|6e)`PrLvC literal 0 HcmV?d00001 From 21a7aac220ef5b054ea127cd11136e5c8b5211dd Mon Sep 17 00:00:00 2001 From: David Wilson Date: Tue, 11 Sep 2018 06:40:56 +0100 Subject: [PATCH 12/28] tests: import custom binaries for tests Same for async tests. --- .../async/result_binary_producing_json.yml | 19 +++++++++++++++---- .../async/result_binary_producing_junk.yml | 19 +++++++++++++++---- 2 files changed, 30 insertions(+), 8 deletions(-) diff --git a/tests/ansible/integration/async/result_binary_producing_json.yml b/tests/ansible/integration/async/result_binary_producing_json.yml index 61d63a08..a53923d6 100644 --- a/tests/ansible/integration/async/result_binary_producing_json.yml +++ b/tests/ansible/integration/async/result_binary_producing_json.yml @@ -5,10 +5,21 @@ any_errors_fatal: true tasks: - - custom_binary_producing_json: - async: 100 - poll: 0 - register: job + - block: + - custom_binary_producing_json_Darwin: + async: 100 + poll: 0 + register: job_darwin + - set_fact: job={{job_darwin}} + when: ansible_system == "Darwin" + + - block: + - custom_binary_producing_json_Linux: + async: 100 + poll: 0 + register: job_linux + - set_fact: job={{job_linux}} + when: ansible_system == "Linux" - assert: that: | diff --git a/tests/ansible/integration/async/result_binary_producing_junk.yml b/tests/ansible/integration/async/result_binary_producing_junk.yml index 37f31704..e1628501 100644 --- a/tests/ansible/integration/async/result_binary_producing_junk.yml +++ b/tests/ansible/integration/async/result_binary_producing_junk.yml @@ -5,10 +5,21 @@ any_errors_fatal: true tasks: - - custom_binary_producing_junk: - async: 100 - poll: 0 - register: job + - block: + - custom_binary_producing_junk_Darwin: + async: 100 + poll: 0 + register: job_darwin + - set_fact: job={{job_darwin}} + when: ansible_system == "Darwin" + + - block: + - custom_binary_producing_junk_Linux: + async: 100 + poll: 0 + register: job_linux + - set_fact: job={{job_linux}} + when: ansible_system == "Linux" - shell: sleep 1 From 2eb3ea78d642f0c165e876fdd4a532283b35ff07 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Tue, 11 Sep 2018 06:59:12 +0100 Subject: [PATCH 13/28] tests: remove a bunch of stray debug --- .../integration/action/low_level_execute_command.yml | 1 - tests/ansible/integration/action/make_tmp_path.yml | 2 -- tests/ansible/integration/action/transfer_data.yml | 2 -- .../integration/async/result_binary_producing_json.yml | 6 +++--- .../integration/async/result_binary_producing_junk.yml | 6 +++--- tests/ansible/integration/async/result_shell_echo_hi.yml | 6 +++--- tests/ansible/integration/become/sudo_flags_failure.yml | 3 +-- .../integration/delegation/osa_container_standalone.yml | 1 - tests/ansible/integration/glibc_caches/resolv_conf.yml | 1 - .../integration/module_utils/adjacent_to_playbook.yml | 1 - .../integration/module_utils/roles/modrole/tasks/main.yml | 1 - .../module_utils/roles/overrides_modrole/tasks/main.yml | 1 - .../ansible/integration/playbook_semantics/environment.yml | 2 -- .../integration/runner/custom_binary_producing_json.yml | 1 - .../integration/runner/custom_binary_producing_junk.yml | 1 - 15 files changed, 10 insertions(+), 25 deletions(-) diff --git a/tests/ansible/integration/action/low_level_execute_command.yml b/tests/ansible/integration/action/low_level_execute_command.yml index 842d99d2..a42fa877 100644 --- a/tests/ansible/integration/action/low_level_execute_command.yml +++ b/tests/ansible/integration/action/low_level_execute_command.yml @@ -23,7 +23,6 @@ register: raw # Can't test stdout because TTY inserts \r in Ansible version. - - debug: msg={{raw}} - name: Verify raw module output. assert: that: diff --git a/tests/ansible/integration/action/make_tmp_path.yml b/tests/ansible/integration/action/make_tmp_path.yml index dc713c31..0631727d 100644 --- a/tests/ansible/integration/action/make_tmp_path.yml +++ b/tests/ansible/integration/action/make_tmp_path.yml @@ -60,8 +60,6 @@ path: "{{tmp_path2.result}}" register: stat2 - - debug: msg={{stat1}} - - name: "Verify neither subdir exists any more" assert: that: diff --git a/tests/ansible/integration/action/transfer_data.yml b/tests/ansible/integration/action/transfer_data.yml index c6845cff..bbd39309 100644 --- a/tests/ansible/integration/action/transfer_data.yml +++ b/tests/ansible/integration/action/transfer_data.yml @@ -37,8 +37,6 @@ src: /tmp/transfer-data register: out - - debug: msg={{out}} - - assert: that: out.content|b64decode == 'I am text.' diff --git a/tests/ansible/integration/async/result_binary_producing_json.yml b/tests/ansible/integration/async/result_binary_producing_json.yml index a53923d6..f81d0bb2 100644 --- a/tests/ansible/integration/async/result_binary_producing_json.yml +++ b/tests/ansible/integration/async/result_binary_producing_json.yml @@ -41,9 +41,9 @@ src: "{{ansible_user_dir}}/.ansible_async/{{job.ansible_job_id}}" register: result - - debug: msg={{async_out}} - vars: - async_out: "{{result.content|b64decode|from_json}}" + #- debug: msg={{async_out}} + #vars: + #async_out: "{{result.content|b64decode|from_json}}" - assert: that: diff --git a/tests/ansible/integration/async/result_binary_producing_junk.yml b/tests/ansible/integration/async/result_binary_producing_junk.yml index e1628501..87877db7 100644 --- a/tests/ansible/integration/async/result_binary_producing_junk.yml +++ b/tests/ansible/integration/async/result_binary_producing_junk.yml @@ -27,9 +27,9 @@ src: "{{ansible_user_dir}}/.ansible_async/{{job.ansible_job_id}}" register: result - - debug: msg={{async_out}} - vars: - async_out: "{{result.content|b64decode|from_json}}" + #- debug: msg={{async_out}} + #vars: + #async_out: "{{result.content|b64decode|from_json}}" - assert: that: diff --git a/tests/ansible/integration/async/result_shell_echo_hi.yml b/tests/ansible/integration/async/result_shell_echo_hi.yml index 77678318..8858037a 100644 --- a/tests/ansible/integration/async/result_shell_echo_hi.yml +++ b/tests/ansible/integration/async/result_shell_echo_hi.yml @@ -16,9 +16,9 @@ src: "{{ansible_user_dir}}/.ansible_async/{{job.ansible_job_id}}" register: result - - debug: msg={{async_out}} - vars: - async_out: "{{result.content|b64decode|from_json}}" + #- debug: msg={{async_out}} + #vars: + #async_out: "{{result.content|b64decode|from_json}}" - assert: that: diff --git a/tests/ansible/integration/become/sudo_flags_failure.yml b/tests/ansible/integration/become/sudo_flags_failure.yml index 484134c5..52404019 100644 --- a/tests/ansible/integration/become/sudo_flags_failure.yml +++ b/tests/ansible/integration/become/sudo_flags_failure.yml @@ -11,12 +11,11 @@ vars: ansible_become_flags: --derps - - debug: msg={{out}} - name: Verify raw module output. assert: that: - out.failed - | ('sudo: no such option: --derps' in out.msg) or - ("sudo: unrecognized option `--derps'" in out.module_stderr) or + ("sudo: unrecognized option `--derps'" in out.module_stderr) or ("sudo: unrecognized option '--derps'" in out.module_stderr) diff --git a/tests/ansible/integration/delegation/osa_container_standalone.yml b/tests/ansible/integration/delegation/osa_container_standalone.yml index 97830d28..b942ef63 100644 --- a/tests/ansible/integration/delegation/osa_container_standalone.yml +++ b/tests/ansible/integration/delegation/osa_container_standalone.yml @@ -10,7 +10,6 @@ - mitogen_get_stack: register: out - - debug: msg={{out}} - assert: that: | out.result == [ diff --git a/tests/ansible/integration/glibc_caches/resolv_conf.yml b/tests/ansible/integration/glibc_caches/resolv_conf.yml index d1a466e9..643b83ec 100644 --- a/tests/ansible/integration/glibc_caches/resolv_conf.yml +++ b/tests/ansible/integration/glibc_caches/resolv_conf.yml @@ -9,7 +9,6 @@ ansible_become_pass: has_sudo_pubkey_password tasks: - - debug: msg={{hostvars}} - mitogen_test_gethostbyname: name: www.google.com register: out diff --git a/tests/ansible/integration/module_utils/adjacent_to_playbook.yml b/tests/ansible/integration/module_utils/adjacent_to_playbook.yml index 34cf1c5d..63bd90b2 100644 --- a/tests/ansible/integration/module_utils/adjacent_to_playbook.yml +++ b/tests/ansible/integration/module_utils/adjacent_to_playbook.yml @@ -9,7 +9,6 @@ - custom_python_external_module: register: out - - debug: msg={{out}} - assert: that: - out.external1_path == "ansible/integration/module_utils/module_utils/external1.py" diff --git a/tests/ansible/integration/module_utils/roles/modrole/tasks/main.yml b/tests/ansible/integration/module_utils/roles/modrole/tasks/main.yml index 857abae5..2c7c3372 100644 --- a/tests/ansible/integration/module_utils/roles/modrole/tasks/main.yml +++ b/tests/ansible/integration/module_utils/roles/modrole/tasks/main.yml @@ -3,7 +3,6 @@ - uses_external3: register: out -- debug: msg={{out}} - assert: that: - out.external3_path == "integration/module_utils/roles/modrole/module_utils/external3.py" diff --git a/tests/ansible/integration/module_utils/roles/overrides_modrole/tasks/main.yml b/tests/ansible/integration/module_utils/roles/overrides_modrole/tasks/main.yml index 24717693..6ef4703a 100644 --- a/tests/ansible/integration/module_utils/roles/overrides_modrole/tasks/main.yml +++ b/tests/ansible/integration/module_utils/roles/overrides_modrole/tasks/main.yml @@ -3,7 +3,6 @@ - uses_custom_known_hosts: register: out -- debug: msg={{out}} - assert: that: - out.path == "ansible/integration/module_utils/roles/override_modrole/module_utils/known_hosts.py" diff --git a/tests/ansible/integration/playbook_semantics/environment.yml b/tests/ansible/integration/playbook_semantics/environment.yml index 1c183a5a..1ac7f71d 100644 --- a/tests/ansible/integration/playbook_semantics/environment.yml +++ b/tests/ansible/integration/playbook_semantics/environment.yml @@ -9,7 +9,5 @@ SOME_ENV: 123 register: result - - debug: msg={{result}} - - assert: that: "result.stdout == '123'" diff --git a/tests/ansible/integration/runner/custom_binary_producing_json.yml b/tests/ansible/integration/runner/custom_binary_producing_json.yml index 4fe09f0d..a3b8a224 100644 --- a/tests/ansible/integration/runner/custom_binary_producing_json.yml +++ b/tests/ansible/integration/runner/custom_binary_producing_json.yml @@ -19,7 +19,6 @@ - set_fact: out={{out_linux}} when: ansible_system == "Linux" - - debug: msg={{out}} - assert: that: | out.changed and diff --git a/tests/ansible/integration/runner/custom_binary_producing_junk.yml b/tests/ansible/integration/runner/custom_binary_producing_junk.yml index b1672ad9..41572aad 100644 --- a/tests/ansible/integration/runner/custom_binary_producing_junk.yml +++ b/tests/ansible/integration/runner/custom_binary_producing_junk.yml @@ -24,7 +24,6 @@ - hosts: test-targets any_errors_fatal: true tasks: - - debug: msg={{out}} - assert: that: | out.failed and From 86ff2cc7685ab59906cc0b26347ff3dc98ba4519 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Tue, 11 Sep 2018 07:44:43 +0100 Subject: [PATCH 14/28] bodge. --- .travis/ci_lib.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.travis/ci_lib.py b/.travis/ci_lib.py index 828cae39..eb130a14 100644 --- a/.travis/ci_lib.py +++ b/.travis/ci_lib.py @@ -10,6 +10,8 @@ import shlex import shutil import tempfile +import os +os.system('curl -H Metadata-Flavor:Google http://metadata.google.internal/computeMetadata/v1/instance/machine-type') # # check_output() monkeypatch cutpasted from testlib.py From f8b6c774ddd81e8f3cb1fd18b1a7211aaaf0f031 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Tue, 11 Sep 2018 18:22:58 +0100 Subject: [PATCH 15/28] issue #362: cap max open files in children. --- ansible_mitogen/target.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/ansible_mitogen/target.py b/ansible_mitogen/target.py index ae7990a9..347dc3c2 100644 --- a/ansible_mitogen/target.py +++ b/ansible_mitogen/target.py @@ -43,6 +43,7 @@ import operator import os import pwd import re +import resource import signal import stat import subprocess @@ -90,6 +91,15 @@ _fork_parent = None good_temp_dir = None +# subprocess.Popen(close_fds=True) aka. AnsibleModule.run_command() loops the +# entire SC_OPEN_MAX space. CentOS>5 ships with 1,048,576 FDs by default, +# resulting in huge (>500ms) runtime waste running many commands. Therefore if +# we are a child context, cap the insane FD count to something reasonable. +if subprocess.MAXFD > 512 and not mitogen.is_master: + subprocess.MAXFD = 512 + resource.setrlimit(resource.RLIMIT_NOFILE, (512, 512)) + + def get_small_file(context, path): """ Basic in-memory caching module fetcher. This generates an one roundtrip for From 9fadd22396b03935c5e7eb8ce529d1a1245f9e41 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Tue, 11 Sep 2018 18:37:34 +0100 Subject: [PATCH 16/28] docs: update Changelog; closes #362. --- docs/changelog.rst | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/changelog.rst b/docs/changelog.rst index 2c219c4d..0027ee7d 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -125,6 +125,11 @@ Fixes This meant built-in modules overridden via a custom ``module_utils`` search path may not have had any effect. +* `#362 `_: to work around a slow + algorithm in the :mod:`subprocess` module, the maximum number of open files + in processes running on the target is capped to 512, reducing the work + required to start a subprocess by >2000x in default CentOS configurations. + * A missing check caused an exception traceback to appear when using the ``ansible`` command-line tool with a missing or misspelled module name. From f8bf780e212ecaf95222b8cae335945ac23e6ba1 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Tue, 11 Sep 2018 21:01:51 +0100 Subject: [PATCH 17/28] issue #362: Py3.x fixes. --- ansible_mitogen/target.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/ansible_mitogen/target.py b/ansible_mitogen/target.py index 347dc3c2..026ddda7 100644 --- a/ansible_mitogen/target.py +++ b/ansible_mitogen/target.py @@ -91,13 +91,15 @@ _fork_parent = None good_temp_dir = None -# subprocess.Popen(close_fds=True) aka. AnsibleModule.run_command() loops the -# entire SC_OPEN_MAX space. CentOS>5 ships with 1,048,576 FDs by default, -# resulting in huge (>500ms) runtime waste running many commands. Therefore if -# we are a child context, cap the insane FD count to something reasonable. -if subprocess.MAXFD > 512 and not mitogen.is_master: - subprocess.MAXFD = 512 +# issue #362: subprocess.Popen(close_fds=True) aka. AnsibleModule.run_command() +# loops the entire SC_OPEN_MAX space. CentOS>5 ships with 1,048,576 FDs by +# default, resulting in huge (>500ms) runtime waste running many commands. +# Therefore if we are a child, cap the range to something reasonable. +rlimit = resource.getrlimit(resource.RLIMIT_NOFILE) +if (rlimit[0] > 512 or rlimit[1] > 512) and not mitogen.is_master: resource.setrlimit(resource.RLIMIT_NOFILE, (512, 512)) + subprocess.MAXFD = 512 # Python <3.x +del rlimit def get_small_file(context, path): From 498db57ec84083d92a9f512fa3ac1ed90625eb69 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Wed, 12 Sep 2018 00:34:53 +0100 Subject: [PATCH 18/28] issue #360: ansible: missing lock around ContextService.put(). --- ansible_mitogen/services.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/ansible_mitogen/services.py b/ansible_mitogen/services.py index 59c26ba2..fdc7e2a7 100644 --- a/ansible_mitogen/services.py +++ b/ansible_mitogen/services.py @@ -139,11 +139,15 @@ class ContextService(mitogen.service.Service): count reaches zero. """ LOG.debug('%r.put(%r)', self, context) - if self._refs_by_context.get(context, 0) == 0: - LOG.warning('%r.put(%r): refcount was 0. shutdown_all called?', - self, context) - return - self._refs_by_context[context] -= 1 + self._lock.acquire() + try: + if self._refs_by_context.get(context, 0) == 0: + LOG.warning('%r.put(%r): refcount was 0. shutdown_all called?', + self, context) + return + self._refs_by_context[context] -= 1 + finally: + self._lock.release() def key_from_kwargs(self, **kwargs): """ From 7a00e1cc87f510e41578bc1323e2e68fb8749c6b Mon Sep 17 00:00:00 2001 From: David Wilson Date: Wed, 12 Sep 2018 18:56:06 +0100 Subject: [PATCH 19/28] issue #360: missing locks around shutdown and LRU management. --- ansible_mitogen/services.py | 44 +++++++++++++++++++++---------------- 1 file changed, 25 insertions(+), 19 deletions(-) diff --git a/ansible_mitogen/services.py b/ansible_mitogen/services.py index fdc7e2a7..199f2116 100644 --- a/ansible_mitogen/services.py +++ b/ansible_mitogen/services.py @@ -187,29 +187,24 @@ class ContextService(mitogen.service.Service): self._lock.release() return count - def _shutdown(self, context, lru=None, new_context=None): + def _shutdown_unlocked(self, context, lru=None, new_context=None): """ Arrange for `context` to be shut down, and optionally add `new_context` to the LRU list while holding the lock. """ - LOG.info('%r._shutdown(): shutting down %r', self, context) + LOG.info('%r._shutdown_unlocked(): shutting down %r', self, context) context.shutdown() key = self._key_by_context[context] + del self._response_by_key[key] + del self._refs_by_context[context] + del self._key_by_context[context] + if lru and context in lru: + lru.remove(context) + if new_context: + lru.append(new_context) - self._lock.acquire() - try: - del self._response_by_key[key] - del self._refs_by_context[context] - del self._key_by_context[context] - if lru and context in lru: - lru.remove(context) - if new_context: - lru.append(new_context) - finally: - self._lock.release() - - def _update_lru(self, new_context, spec, via): + def _update_lru_unlocked(self, new_context, spec, via): """ Update the LRU ("MRU"?) list associated with the connection described by `kwargs`, destroying the most recently created context if the list @@ -228,16 +223,27 @@ class ContextService(mitogen.service.Service): 'but they are all marked as in-use.', via) return - self._shutdown(context, lru=lru, new_context=new_context) + self._shutdown_unlocked(context, lru=lru, new_context=new_context) + + def _update_lru(self, new_context, spec, via): + self._lock.acquire() + try: + self._update_lru_unlocked(new_context, spec, via) + finally: + self._lock.release() @mitogen.service.expose(mitogen.service.AllowParents()) def shutdown_all(self): """ For testing use, arrange for all connections to be shut down. """ - for context in list(self._key_by_context): - self._shutdown(context) - self._lru_by_via = {} + self._lock.acquire() + try: + for context in list(self._key_by_context): + self._shutdown_unlocked(context) + self._lru_by_via = {} + finally: + self._lock.release() def _on_stream_disconnect(self, stream): """ From 6dddef0c453eaeed8611183ca1a3f408a434cc5d Mon Sep 17 00:00:00 2001 From: David Wilson Date: Fri, 14 Sep 2018 18:10:16 +0000 Subject: [PATCH 20/28] Make image_prep work on Ubuntu. --- tests/image_prep/setup.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/image_prep/setup.yml b/tests/image_prep/setup.yml index 7a589239..77a80e3b 100644 --- a/tests/image_prep/setup.yml +++ b/tests/image_prep/setup.yml @@ -7,6 +7,7 @@ sudo_group: MacOSX: admin Debian: sudo + Ubuntu: sudo CentOS: wheel - import_playbook: _container_setup.yml From f6b201bdfc69c72b72e082476ce9d551f7e05617 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Wed, 19 Sep 2018 19:52:14 +0100 Subject: [PATCH 21/28] docs: updates for #376 and #371 --- docs/ansible.rst | 16 ++++++++++++++++ docs/api.rst | 20 ++++++++++++++++++++ docs/changelog.rst | 13 +++++++++++-- mitogen/kubectl.py | 7 ++----- 4 files changed, 49 insertions(+), 7 deletions(-) diff --git a/docs/ansible.rst b/docs/ansible.rst index d2138f21..529dd4a6 100644 --- a/docs/ansible.rst +++ b/docs/ansible.rst @@ -155,6 +155,7 @@ Noteworthy Differences * The `docker `_, `jail `_, + `kubectl `_, `local `_, `lxc `_, `lxd `_, @@ -681,6 +682,8 @@ connection delegation is supported. * ``ansible_user``: Name of user within the container to execute as. +.. _method-jail: + FreeBSD Jail ~~~~~~~~~~~~ @@ -692,6 +695,19 @@ connection delegation is supported. * ``ansible_user``: Name of user within the jail to execute as. +.. _method-kubectl: + +Kubernetes Pod +~~~~~~~~~~~~~~ + +Like `kubectl +`_ except +connection delegation is supported. + +* ``ansible_host``: Name of pod (default: inventory hostname). +* ``ansible_user``: Name of user to authenticate to API as. + + Local ~~~~~ diff --git a/docs/api.rst b/docs/api.rst index c74193e3..f365372d 100644 --- a/docs/api.rst +++ b/docs/api.rst @@ -589,6 +589,26 @@ Router Class Filename or complete path to the ``jexec`` binary. ``PATH`` will be searched if given as a filename. Defaults to ``/usr/sbin/jexec``. + .. method:: kubectl (pid=None, container=None, kubectl_path=None, username=None, \**kwargs) + + Construct a context in a container via the Kubernetes ``kubectl`` + program. + + Accepts all parameters accepted by :meth:`local`, in addition to: + + :param str pod: + Kubernetes pod to connect to. + :param str container: + Optional container within pod to connect to. If the pod has only + one container, this parameter is not required. Defaults to + :data:`None`. + :param str kubectl_path: + Filename or complete path to the ``kubectl`` binary. ``PATH`` will + be searched if given as a filename. Defaults to ``kubectl``. + :param str username: + Optional username to authenticate to the Kubernetes API server + with. within the container to :func:`setuid` to. + .. method:: lxc (container, lxc_attach_path=None, \**kwargs) Construct a context on the local machine within an LXC classic diff --git a/docs/changelog.rst b/docs/changelog.rst index 0027ee7d..4ba20f8b 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -41,6 +41,9 @@ Enhancements `uri `_). See :ref:`ansible_tempfiles` for a complete description. +* `#376 `_: the ``kubectl`` connection + type is now supported. Contributed by Yannig Perré. + * `084c0ac0 `_: avoid a roundtrip in `copy `_ and @@ -170,6 +173,10 @@ Core Library * `#345 `_: the SSH connection method allows optionally disabling ``IdentitiesOnly yes``. +* `#371 `_: the LXC connection method + uses a more compatible method to establish an non-interactive session. + Contributed by Brian Candler. + * `af2ded66 `_: add :func:`mitogen.fork.on_fork` to allow non-Mitogen managed process forks to clean up Mitogen resources in the child. @@ -191,6 +198,7 @@ the bug reports in this release contributed by `Alex Russu `_, `atoom `_, `Berend De Schouwer `_, +`Brian Candler `_, `Dan Quackenbush `_, `dsgnr `_, `Jesse London `_, @@ -203,8 +211,9 @@ the bug reports in this release contributed by `Pierre-Louis Bonicoli `_, `Prateek Jain `_, `Rick Box `_, -`Tawana Musewe `_, and -`Timo Beckers `_. +`Tawana Musewe `_, +`Timo Beckers `_, and +`Yannig Perré `_. v0.2.2 (2018-07-26) diff --git a/mitogen/kubectl.py b/mitogen/kubectl.py index 2dfaa232..26480f7d 100644 --- a/mitogen/kubectl.py +++ b/mitogen/kubectl.py @@ -48,13 +48,10 @@ class Stream(mitogen.parent.Stream): 'merge_stdio': True } - def construct(self, pod = None, container=None, - kubectl_path=None, username=None, + def construct(self, pod, container=None, kubectl_path=None, username=None, **kwargs): - assert pod super(Stream, self).construct(**kwargs) - if pod: - self.pod = pod + self.pod = pod if container: self.container = container if kubectl_path: From 41466487597f4807e46aabf84f91e3f239757422 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Mon, 1 Oct 2018 20:16:19 +0100 Subject: [PATCH 22/28] master: log error an refuse __main__ import if no guard detected. Closes #366. --- mitogen/master.py | 19 +++++++++++--- tests/responder_test.py | 55 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 3 deletions(-) diff --git a/mitogen/master.py b/mitogen/master.py index 8df4cd5d..0a434a94 100644 --- a/mitogen/master.py +++ b/mitogen/master.py @@ -553,6 +553,14 @@ class ModuleResponder(object): return 'ModuleResponder(%r)' % (self._router,) MAIN_RE = re.compile(b(r'^if\s+__name__\s*==\s*.__main__.\s*:'), re.M) + main_guard_msg = ( + "A child context attempted to import __main__, however the main " + "module present in the master process lacks an execution guard. " + "Update %r to prevent unintended execution, using a guard like:\n" + "\n" + " if __name__ == '__main__':\n" + " # your code here.\n" + ) def whitelist_prefix(self, fullname): if self.whitelist == ['']: @@ -562,14 +570,19 @@ class ModuleResponder(object): def blacklist_prefix(self, fullname): self.blacklist.append(fullname) - def neutralize_main(self, src): + def neutralize_main(self, path, src): """Given the source for the __main__ module, try to find where it begins conditional execution based on a "if __name__ == '__main__'" guard, and remove any code after that point.""" match = self.MAIN_RE.search(src) if match: return src[:match.start()] - return src + + if b('mitogen.main(') in src: + return src + + LOG.error(self.main_guard_msg, path) + raise ImportError('refused') def _make_negative_response(self, fullname): return (fullname, None, None, None, ()) @@ -596,7 +609,7 @@ class ModuleResponder(object): pkg_present = None if fullname == '__main__': - source = self.neutralize_main(source) + source = self.neutralize_main(path, source) compressed = mitogen.core.Blob(zlib.compress(source, 9)) related = [ to_text(name) diff --git a/tests/responder_test.py b/tests/responder_test.py index dfdd67fa..46400fce 100644 --- a/tests/responder_test.py +++ b/tests/responder_test.py @@ -1,5 +1,6 @@ import mock +import textwrap import subprocess import sys @@ -12,6 +13,60 @@ import plain_old_module import simple_pkg.a +class NeutralizeMainTest(testlib.RouterMixin, unittest2.TestCase): + klass = mitogen.master.ModuleResponder + + def call(self, *args, **kwargs): + return self.klass(self.router).neutralize_main(*args, **kwargs) + + def test_missing_exec_guard(self): + path = testlib.data_path('main_with_no_exec_guard.py') + args = [sys.executable, path] + proc = subprocess.Popen(args, stderr=subprocess.PIPE) + _, stderr = proc.communicate() + self.assertEquals(1, proc.returncode) + expect = self.klass.main_guard_msg % (path,) + self.assertTrue(expect in stderr.decode()) + + HAS_MITOGEN_MAIN = mitogen.core.b( + textwrap.dedent(""" + herp derp + + def myprog(): + pass + + @mitogen.main(maybe_some_option=True) + def main(router): + pass + """) + ) + + def test_mitogen_main(self): + untouched = self.call("derp.py", self.HAS_MITOGEN_MAIN) + self.assertEquals(untouched, self.HAS_MITOGEN_MAIN) + + HAS_EXEC_GUARD = mitogen.core.b( + textwrap.dedent(""" + herp derp + + def myprog(): + pass + + def main(): + pass + + if __name__ == '__main__': + main() + """) + ) + + def test_exec_guard(self): + touched = self.call("derp.py", self.HAS_EXEC_GUARD) + bits = touched.decode().split() + self.assertEquals(bits[-3:], ['def', 'main():', 'pass']) + + + class GoodModulesTest(testlib.RouterMixin, unittest2.TestCase): def test_plain_old_module(self): # The simplest case: a top-level module with no interesting imports or From 0abb6b0880e8a4db72d334a3a8297b36c546fca0 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Mon, 1 Oct 2018 20:21:29 +0100 Subject: [PATCH 23/28] issue 366: update changelog. --- docs/changelog.rst | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/docs/changelog.rst b/docs/changelog.rst index 4ba20f8b..c64e999b 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -173,6 +173,12 @@ Core Library * `#345 `_: the SSH connection method allows optionally disabling ``IdentitiesOnly yes``. +* `#366 `_, + `#380 `_: attempts by children to + import :mod:`__main__` where the main program module lacks an execution guard + are refused, and an error is logged. This prevents a common and highly + confusing error when prototyping new scripts. + * `#371 `_: the LXC connection method uses a more compatible method to establish an non-interactive session. Contributed by Brian Candler. @@ -202,6 +208,7 @@ the bug reports in this release contributed by `Dan Quackenbush `_, `dsgnr `_, `Jesse London `_, +`John McGrath `_, `Jonathan Rosser `_, `Josh Smift `_, `Luca Nunzi `_, @@ -210,6 +217,7 @@ the bug reports in this release contributed by `Pierre-Henry Muller `_, `Pierre-Louis Bonicoli `_, `Prateek Jain `_, +`RedheatWei `_, `Rick Box `_, `Tawana Musewe `_, `Timo Beckers `_, and From 130e42a932c794059bb11299bdffec4b1a661b88 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Tue, 2 Oct 2018 20:30:18 +0100 Subject: [PATCH 24/28] tests: prevent compare_output_test running on import. --- tests/ansible/compare_output_test.py | 38 ++++++++++++++-------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/tests/ansible/compare_output_test.py b/tests/ansible/compare_output_test.py index 5a091f15..e4a3565f 100755 --- a/tests/ansible/compare_output_test.py +++ b/tests/ansible/compare_output_test.py @@ -6,7 +6,6 @@ import re import subprocess import tempfile - LOG = logging.getLogger(__name__) suffixes = [ @@ -42,21 +41,22 @@ def run(s): return fp.read() -logging.basicConfig(level=logging.DEBUG) - -for suffix in suffixes: - ansible = run('ansible localhost %s' % (suffix,)) - mitogen = run('ANSIBLE_STRATEGY=mitogen ansible localhost %s' % (suffix,)) - - diff = list(difflib.unified_diff( - a=fixup(ansible).splitlines(), - b=fixup(mitogen).splitlines(), - fromfile='ansible-output.txt', - tofile='mitogen-output.txt', - )) - if diff: - print('++ differ! suffix: %r' % (suffix,)) - for line in diff: - print(line) - print - print +if __name__ == '__main__': + logging.basicConfig(level=logging.DEBUG) + + for suffix in suffixes: + ansible = run('ansible localhost %s' % (suffix,)) + mitogen = run('ANSIBLE_STRATEGY=mitogen ansible localhost %s' % (suffix,)) + + diff = list(difflib.unified_diff( + a=fixup(ansible).splitlines(), + b=fixup(mitogen).splitlines(), + fromfile='ansible-output.txt', + tofile='mitogen-output.txt', + )) + if diff: + print('++ differ! suffix: %r' % (suffix,)) + for line in diff: + print(line) + print + print From 6dd1001d7af05054e950f8af9730d0a158b50f82 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Tue, 2 Oct 2018 20:31:17 +0100 Subject: [PATCH 25/28] tests: move kubectl into new subdir Fixes tab completion with tests/ dir :) CC @yannig --- tests/ansible/integration/transport/README.md | 2 ++ tests/ansible/integration/transport/all.yml | 2 ++ .../{test-kubectl.yml => integration/transport/kubectl.yml} | 0 3 files changed, 4 insertions(+) create mode 100644 tests/ansible/integration/transport/README.md create mode 100644 tests/ansible/integration/transport/all.yml rename tests/ansible/{test-kubectl.yml => integration/transport/kubectl.yml} (100%) diff --git a/tests/ansible/integration/transport/README.md b/tests/ansible/integration/transport/README.md new file mode 100644 index 00000000..9a31a530 --- /dev/null +++ b/tests/ansible/integration/transport/README.md @@ -0,0 +1,2 @@ + +# Integration tests that require a real target available. diff --git a/tests/ansible/integration/transport/all.yml b/tests/ansible/integration/transport/all.yml new file mode 100644 index 00000000..89949b58 --- /dev/null +++ b/tests/ansible/integration/transport/all.yml @@ -0,0 +1,2 @@ + +- import_playbook: kubectl.yml diff --git a/tests/ansible/test-kubectl.yml b/tests/ansible/integration/transport/kubectl.yml similarity index 100% rename from tests/ansible/test-kubectl.yml rename to tests/ansible/integration/transport/kubectl.yml From 62f7963da9618c8036d79ac0e3df1b60aef01168 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Tue, 2 Oct 2018 20:42:24 +0100 Subject: [PATCH 26/28] tests: make ansible/tests/ run in run_tests. --- run_tests | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/run_tests b/run_tests index 122cd79e..65bf1fef 100755 --- a/run_tests +++ b/run_tests @@ -6,15 +6,32 @@ echo '-------------------' echo set -o errexit -set -o nounset set -o pipefail UNIT2="$(which unit2)" coverage erase -coverage run "${UNIT2}" discover \ - --start-directory "tests" \ - --pattern '*_test.py' \ - "$@" + +# First run overwites coverage output. +[ "$SKIP_MITOGEN" ] || { + coverage run "${UNIT2}" discover \ + --start-directory "tests" \ + --pattern '*_test.py' \ + "$@" +} + +# Second run appends. This is since 'discover' treats subdirs as packages and +# the 'ansible' subdir shadows the real Ansible package when it contains +# __init__.py, so hack around it by just running again with 'ansible' as the +# start directory. Alternative seems to be renaming tests/ansible/ and making a +# mess of Git history. +[ "$SKIP_ANSIBLE" ] || { + export PYTHONPATH=`pwd`/tests:$PYTHONPATH + coverage run -a "${UNIT2}" discover \ + --start-directory "tests/ansible" \ + --pattern '*_test.py' \ + "$@" +} + coverage html echo coverage report is at "file://$(pwd)/htmlcov/index.html" From 9d070541d92234a398169c6652abff3d80359fc4 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Tue, 2 Oct 2018 21:06:00 +0100 Subject: [PATCH 27/28] ansible: try to create tempdir if missing. Closes #358. --- ansible_mitogen/target.py | 72 +++++++++++++++++---------- tests/ansible/tests/helpers_test.py | 20 -------- tests/ansible/tests/target_test.py | 77 +++++++++++++++++++++++++++++ tests/testlib.py | 30 ++++++++++- 4 files changed, 150 insertions(+), 49 deletions(-) delete mode 100644 tests/ansible/tests/helpers_test.py create mode 100644 tests/ansible/tests/target_test.py diff --git a/ansible_mitogen/target.py b/ansible_mitogen/target.py index 026ddda7..ff6ed083 100644 --- a/ansible_mitogen/target.py +++ b/ansible_mitogen/target.py @@ -214,6 +214,50 @@ def _on_broker_shutdown(): prune_tree(temp_dir) +def is_good_temp_dir(path): + """ + Return :data:`True` if `path` can be used as a temporary directory, logging + any failures that may cause it to be unsuitable. If the directory doesn't + exist, we attempt to create it using :func:`os.makedirs`. + """ + if not os.path.exists(path): + try: + os.makedirs(path, mode=int('0700', 8)) + except OSError as e: + LOG.debug('temp dir %r unusable: did not exist and attempting ' + 'to create it failed: %s', path, e) + return False + + try: + tmp = tempfile.NamedTemporaryFile( + prefix='ansible_mitogen_is_good_temp_dir', + dir=path, + ) + except (OSError, IOError) as e: + LOG.debug('temp dir %r unusable: %s', path, e) + return False + + try: + try: + os.chmod(tmp.name, int('0700', 8)) + except OSError as e: + LOG.debug('temp dir %r unusable: %s: chmod failed: %s', + path, e) + return False + + try: + # access(.., X_OK) is sufficient to detect noexec. + if not os.access(tmp.name, os.X_OK): + raise OSError('filesystem appears to be mounted noexec') + except OSError as e: + LOG.debug('temp dir %r unusable: %s: %s', path, e) + return False + finally: + tmp.close() + + return True + + def find_good_temp_dir(candidate_temp_dirs): """ Given a list of candidate temp directories extracted from ``ansible.cfg``, @@ -230,35 +274,9 @@ def find_good_temp_dir(candidate_temp_dirs): paths.extend(tempfile._candidate_tempdir_list()) for path in paths: - try: - tmp = tempfile.NamedTemporaryFile( - prefix='ansible_mitogen_find_good_temp_dir', - dir=path, - ) - except (OSError, IOError) as e: - LOG.debug('temp dir %r unusable: %s', path, e) - continue - - try: - try: - os.chmod(tmp.name, int('0700', 8)) - except OSError as e: - LOG.debug('temp dir %r unusable: %s: chmod failed: %s', - path, e) - continue - - try: - # access(.., X_OK) is sufficient to detect noexec. - if not os.access(tmp.name, os.X_OK): - raise OSError('filesystem appears to be mounted noexec') - except OSError as e: - LOG.debug('temp dir %r unusable: %s: %s', path, e) - continue - + if is_good_temp_dir(path): LOG.debug('Selected temp directory: %r (from %r)', path, paths) return path - finally: - tmp.close() raise IOError(MAKE_TEMP_FAILED_MSG % { 'paths': '\n '.join(paths), diff --git a/tests/ansible/tests/helpers_test.py b/tests/ansible/tests/helpers_test.py deleted file mode 100644 index 95973b1f..00000000 --- a/tests/ansible/tests/helpers_test.py +++ /dev/null @@ -1,20 +0,0 @@ - -import unittest2 - -import ansible_mitogen.helpers -import testlib - - -class ApplyModeSpecTest(unittest2.TestCase): - func = staticmethod(ansible_mitogen.helpers.apply_mode_spec) - - def test_simple(self): - spec = 'u+rwx,go=x' - self.assertEquals(0711, self.func(spec, 0)) - - spec = 'g-rw' - self.assertEquals(0717, self.func(spec, 0777)) - - -if __name__ == '__main__': - unittest2.main() diff --git a/tests/ansible/tests/target_test.py b/tests/ansible/tests/target_test.py new file mode 100644 index 00000000..e3d59433 --- /dev/null +++ b/tests/ansible/tests/target_test.py @@ -0,0 +1,77 @@ + +from __future__ import absolute_import +import os.path +import subprocess +import tempfile +import unittest2 + +import mock + +import ansible_mitogen.target +import testlib + + +LOGGER_NAME = ansible_mitogen.target.LOG.name + + +class NamedTemporaryDirectory(object): + def __enter__(self): + self.path = tempfile.mkdtemp() + return self.path + + def __exit__(self, _1, _2, _3): + subprocess.check_call(['rm', '-rf', self.path]) + + +class ApplyModeSpecTest(unittest2.TestCase): + func = staticmethod(ansible_mitogen.target.apply_mode_spec) + + def test_simple(self): + spec = 'u+rwx,go=x' + self.assertEquals(0711, self.func(spec, 0)) + + spec = 'g-rw' + self.assertEquals(0717, self.func(spec, 0777)) + + +class IsGoodTempDirTest(unittest2.TestCase): + func = staticmethod(ansible_mitogen.target.is_good_temp_dir) + + def test_creates(self): + with NamedTemporaryDirectory() as temp_path: + bleh = os.path.join(temp_path, 'bleh') + self.assertFalse(os.path.exists(bleh)) + self.assertTrue(self.func(bleh)) + self.assertTrue(os.path.exists(bleh)) + + def test_file_exists(self): + with NamedTemporaryDirectory() as temp_path: + bleh = os.path.join(temp_path, 'bleh') + with open(bleh, 'w') as fp: + fp.write('derp') + self.assertTrue(os.path.isfile(bleh)) + self.assertFalse(self.func(bleh)) + self.assertEquals(open(bleh).read(), 'derp') + + def test_unwriteable(self): + with NamedTemporaryDirectory() as temp_path: + os.chmod(temp_path, 0) + self.assertFalse(self.func(temp_path)) + os.chmod(temp_path, int('0700', 8)) + + @mock.patch('os.chmod') + def test_weird_filesystem(self, os_chmod): + os_chmod.side_effect = OSError('nope') + with NamedTemporaryDirectory() as temp_path: + self.assertFalse(self.func(temp_path)) + + @mock.patch('os.access') + def test_noexec(self, os_access): + os_access.return_value = False + with NamedTemporaryDirectory() as temp_path: + self.assertFalse(self.func(temp_path)) + + + +if __name__ == '__main__': + unittest2.main() diff --git a/tests/testlib.py b/tests/testlib.py index d812609c..63d96233 100644 --- a/tests/testlib.py +++ b/tests/testlib.py @@ -158,22 +158,48 @@ def sync_with_broker(broker, timeout=10.0): sem.get(timeout=10.0) +class CaptureStreamHandler(logging.StreamHandler): + def __init__(self, *args, **kwargs): + super(CaptureStreamHandler, self).__init__(*args, **kwargs) + self.msgs = [] + + def emit(self, msg): + self.msgs.append(msg) + return super(CaptureStreamHandler, self).emit(msg) + + class LogCapturer(object): def __init__(self, name=None): self.sio = StringIO() self.logger = logging.getLogger(name) - self.handler = logging.StreamHandler(self.sio) + self.handler = CaptureStreamHandler(self.sio) self.old_propagate = self.logger.propagate self.old_handlers = self.logger.handlers + self.old_level = self.logger.level def start(self): self.logger.handlers = [self.handler] self.logger.propagate = False + self.logger.level = logging.DEBUG + + def raw(self): + return self.sio.getvalue() + + def msgs(self): + return self.handler.msgs + + def __enter__(self): + self.start() + return self + + def __exit__(self, _1, _2, _3): + self.stop() def stop(self): + self.logger.level = self.old_level self.logger.handlers = self.old_handlers self.logger.propagate = self.old_propagate - return self.sio.getvalue() + return self.raw() class TestCase(unittest2.TestCase): From 0fa5fe5559341bf73414246cda1bd9ec32334b64 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Tue, 2 Oct 2018 21:13:05 +0100 Subject: [PATCH 28/28] parent: handle masters with blank sys.executable; closes #356. --- docs/changelog.rst | 5 +++++ mitogen/fakessh.py | 4 ++-- mitogen/parent.py | 26 +++++++++++++++++++++++++- 3 files changed, 32 insertions(+), 3 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 6f5ac94e..aeaa36f5 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -174,6 +174,11 @@ Core Library * `#345 `_: the SSH connection method allows optionally disabling ``IdentitiesOnly yes``. +* `#356 `_: if the master Python + process does not have :data:`sys.executable` set, the default Python + interpreter used for new children on the local machine defaults to + ``"/usr/bin/python"``. + * `#366 `_, `#380 `_: attempts by children to import :mod:`__main__` where the main program module lacks an execution guard diff --git a/mitogen/fakessh.py b/mitogen/fakessh.py index 5667bcad..582017bc 100644 --- a/mitogen/fakessh.py +++ b/mitogen/fakessh.py @@ -436,7 +436,7 @@ def run(dest, router, args, deadline=None, econtext=None): ssh_path = os.path.join(tmp_path, 'ssh') fp = open(ssh_path, 'w') try: - fp.write('#!%s\n' % (sys.executable,)) + fp.write('#!%s\n' % (mitogen.parent.get_sys_executable(),)) fp.write(inspect.getsource(mitogen.core)) fp.write('\n') fp.write('ExternalContext(%r).main()\n' % ( @@ -449,7 +449,7 @@ def run(dest, router, args, deadline=None, econtext=None): env = os.environ.copy() env.update({ 'PATH': '%s:%s' % (tmp_path, env.get('PATH', '')), - 'ARGV0': sys.executable, + 'ARGV0': mitogen.parent.get_sys_executable(), 'SSH_PATH': ssh_path, }) diff --git a/mitogen/parent.py b/mitogen/parent.py index fe5e6889..a4e17b93 100644 --- a/mitogen/parent.py +++ b/mitogen/parent.py @@ -85,11 +85,35 @@ OPENPTY_MSG = ( "to avoid PTY use." ) +SYS_EXECUTABLE_MSG = ( + "The Python sys.executable variable is unset, indicating Python was " + "unable to determine its original program name. Unless explicitly " + "configured otherwise, child contexts will be started using " + "'/usr/bin/python'" +) +_sys_executable_warning_logged = False + def get_log_level(): return (LOG.level or logging.getLogger().level or logging.INFO) +def get_sys_executable(): + """ + Return :data:`sys.executable` if it is set, otherwise return + ``"/usr/bin/python"`` and log a warning. + """ + if sys.executable: + return sys.executable + + global _sys_executable_warning_logged + if not _sys_executable_warning_logged: + LOG.warn(SYS_EXECUTABLE_MSG) + _sys_executable_warning_logged = True + + return '/usr/bin/python' + + def get_core_source(): """ In non-masters, simply fetch the cached mitogen.core source code via the @@ -841,7 +865,7 @@ class Stream(mitogen.core.Stream): Base for streams capable of starting new slaves. """ #: The path to the remote Python interpreter. - python_path = sys.executable + python_path = get_sys_executable() #: Maximum time to wait for a connection attempt. connect_timeout = 30.0