From e241081cae51f7abcfeb5de547bf5fb16ea19cc6 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Sun, 9 Sep 2018 23:39:08 +0100 Subject: [PATCH] ansible: stop sharing target temp_dir in runner. This cannot work with delegate_to, since delegate_to permits multiple concurrent tasks to be executing on the same target. --- ansible_mitogen/connection.py | 34 ++++++++++------------ ansible_mitogen/mixins.py | 9 +++--- ansible_mitogen/planner.py | 1 + ansible_mitogen/runner.py | 55 ++++++++++++----------------------- docs/ansible.rst | 7 +++-- mitogen/parent.py | 2 +- 6 files changed, 44 insertions(+), 64 deletions(-) diff --git a/ansible_mitogen/connection.py b/ansible_mitogen/connection.py index 52a61b77..6c2ef4b0 100644 --- a/ansible_mitogen/connection.py +++ b/ansible_mitogen/connection.py @@ -463,10 +463,10 @@ class Connection(ansible.plugins.connection.ConnectionBase): #: target, automatically destroyed at shutdown. init_child_result = None - #: After :meth:`get_temp_dir` is called, a private temporary directory, - #: destroyed during :meth:`close`, or automatically during shutdown if - #: :meth:`close` failed or was never called. - _temp_dir = 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 @@ -674,17 +674,14 @@ class Connection(ansible.plugins.connection.ConnectionBase): self.init_child_result = dct['init_child_result'] - def get_temp_dir(self): + def _init_temp_dir(self): """ """ - if self._temp_dir is None: - self._temp_dir = os.path.join( - self.init_child_result['temp_dir'], - 'worker-%d-%x' % (os.getpid(), id(self)) - ) - self.get_chain().call_no_reply(os.mkdir, self._temp_dir) - - return self._temp_dir + self.temp_dir = os.path.join( + self.init_child_result['temp_dir'], + 'worker-%d-%x' % (os.getpid(), id(self)) + ) + self.get_chain().call_no_reply(os.mkdir, self.temp_dir) def _connect(self): """ @@ -703,6 +700,7 @@ 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): """ @@ -712,18 +710,16 @@ class Connection(ansible.plugins.connection.ConnectionBase): """ if self.context: self.chain.reset() - if self._temp_dir: - # Don't pipeline here to ensure exception is dumped into - # logging framework on failure. - self.context.call_no_reply(ansible_mitogen.target.prune_tree, - self._temp_dir) - self._temp_dir = None + # 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 6ab8fec7..2ebffb84 100644 --- a/ansible_mitogen/mixins.py +++ b/ansible_mitogen/mixins.py @@ -176,12 +176,13 @@ class ActionModuleMixin(ansible.plugins.action.ActionBase): def _make_tmp_path(self, remote_user=None): """ - Return the temporary directory created by the persistent interpreter at - startup. + Return the directory created by the Connection instance during + 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.get_temp_dir() + 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 @@ -318,7 +319,7 @@ class ActionModuleMixin(ansible.plugins.action.ActionBase): self._connection._connect() if ansible.__version__ > '2.5': - module_args['_ansible_tmpdir'] = self._connection.get_temp_dir() + module_args['_ansible_tmpdir'] = self._connection.temp_dir return ansible_mitogen.planner.invoke( ansible_mitogen.planner.Invocation( diff --git a/ansible_mitogen/planner.py b/ansible_mitogen/planner.py index c709bf7d..5b3e5547 100644 --- a/ansible_mitogen/planner.py +++ b/ansible_mitogen/planner.py @@ -149,6 +149,7 @@ 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('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 97a3cb36..fe5f4c46 100644 --- a/ansible_mitogen/runner.py +++ b/ansible_mitogen/runner.py @@ -66,9 +66,6 @@ except ImportError: # Prevent accidental import of an Ansible module from hanging on stdin read. import ansible.module_utils.basic ansible.module_utils.basic._ANSIBLE_ARGS = '{}' -ansible.module_utils.basic.get_module_path = lambda: ( - ansible_mitogen.target.temp_dir -) # For tasks that modify /etc/resolv.conf, non-Debian derivative glibcs cache # resolv.conf at startup and never implicitly reload it. Cope with that via an @@ -245,13 +242,15 @@ 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, extra_env=None, - cwd=None, env=None, econtext=None, detach=False): + def __init__(self, module, service_context, json_args, temp_dir, + extra_env=None, cwd=None, env=None, econtext=None, + detach=False): self.module = module self.service_context = service_context self.econtext = econtext self.detach = detach self.args = json.loads(json_args) + self.temp_dir = temp_dir self.extra_env = extra_env self.env = env self.cwd = cwd @@ -292,33 +291,6 @@ class Runner(object): implementation simply restores the original environment. """ self._env.revert() - self._try_cleanup_temp() - - def _cleanup_temp(self): - """ - Empty temp_dir in time for the next module invocation. - """ - for name in os.listdir(ansible_mitogen.target.temp_dir): - if name in ('.', '..'): - continue - - path = os.path.join(ansible_mitogen.target.temp_dir, name) - LOG.debug('Deleting %r', path) - ansible_mitogen.target.prune_tree(path) - - def _try_cleanup_temp(self): - """ - During broker shutdown triggered by async task timeout or loss of - connection to the parent, it is possible for prune_tree() in - target.py::_on_broker_shutdown() to run before _cleanup_temp(), so skip - cleanup if the directory or a file disappears from beneath us. - """ - try: - self._cleanup_temp() - except (IOError, OSError) as e: - if e.args[0] == errno.ENOENT: - return - raise def _run(self): """ @@ -431,7 +403,8 @@ class NewStyleStdio(object): """ Patch ansible.module_utils.basic argument globals. """ - def __init__(self, args): + def __init__(self, args, temp_dir): + self.temp_dir = temp_dir self.original_stdout = sys.stdout self.original_stderr = sys.stderr self.original_stdin = sys.stdin @@ -441,7 +414,15 @@ class NewStyleStdio(object): ansible.module_utils.basic._ANSIBLE_ARGS = utf8(encoded) sys.stdin = StringIO(mitogen.core.to_text(encoded)) + self.original_get_path = getattr(ansible.module_utils.basic, + 'get_module_path', None) + ansible.module_utils.basic.get_module_path = self._get_path + + def _get_path(self): + return self.temp_dir + def revert(self): + ansible.module_utils.basic.get_module_path = self.original_get_path sys.stdout = self.original_stdout sys.stderr = self.original_stderr sys.stdin = self.original_stdin @@ -485,7 +466,7 @@ class ProgramRunner(Runner): fetched via :meth:`_get_program`. """ filename = self._get_program_filename() - path = os.path.join(ansible_mitogen.target.temp_dir, filename) + path = os.path.join(self.temp_dir, filename) self.program_fp = open(path, 'wb') self.program_fp.write(self._get_program()) self.program_fp.flush() @@ -565,7 +546,7 @@ class ArgsFileRunner(Runner): self.args_fp = tempfile.NamedTemporaryFile( prefix='ansible_mitogen', suffix='-args', - dir=ansible_mitogen.target.temp_dir, + dir=self.temp_dir, ) self.args_fp.write(utf8(self._get_args_contents())) self.args_fp.flush() @@ -680,7 +661,7 @@ class NewStyleRunner(ScriptRunner): def setup(self): super(NewStyleRunner, self).setup() - self._stdio = NewStyleStdio(self.args) + self._stdio = NewStyleStdio(self.args, self.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. @@ -758,7 +739,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( - ansible_mitogen.target.temp_dir, + self.temp_dir, 'ansible_module_' + os.path.basename(self.path), ) diff --git a/docs/ansible.rst b/docs/ansible.rst index 14f6e553..7a06c120 100644 --- a/docs/ansible.rst +++ b/docs/ansible.rst @@ -464,9 +464,10 @@ filesystem with ``noexec`` disabled: 8. ``/usr/tmp`` 9. Current working directory -As the directory is created once at startup, and its content is managed by code -running remotely, no additional network roundtrips are required to manage it -for each task requiring temporary storage. +The directory is created once at startup, and subdirectories are automatically +created and destroyed for every new task. Management of subdirectories happens +on the controller, but management of the parent directory happens entirely on +the target. .. _ansible_process_env: diff --git a/mitogen/parent.py b/mitogen/parent.py index b291447c..1bda68a2 100644 --- a/mitogen/parent.py +++ b/mitogen/parent.py @@ -1193,7 +1193,7 @@ class CallChain(object): @classmethod def make_chain_id(cls): - return '%s-%s-%s-%s' % ( + return '%s-%s-%x-%x' % ( socket.gethostname(), os.getpid(), threading.currentThread().ident,