From a65e34ce7744048af8f3cb04595a1c748cac92c5 Mon Sep 17 00:00:00 2001 From: Clark Boylan Date: Mon, 12 Dec 2016 13:33:30 -0800 Subject: [PATCH] Fix synchronize retries (#18535) * Fix synchronize retries The synchronize module munges its task args on every invocation of run(). This was problematic because the munged data was not fit for use by a second pass of the synchronize module. Correct this by using a copy of the task args on every invocation of run() so that the original args are not affected. Local testing using this playbook seems to confirm that things work as expected: - hosts: all tasks: - delay: 2 register: task_result retries: 1 until: task_result.rc == 0 synchronize: dest: /tmp/out mode: pull src: /tmp/nonexistent/ fixes #18281 * Update synchroncization fixture assertions When we started operating on a copy of the task args the test assertions were no longer asserting things about the munged state but of the pristine state. Convert the copy of task args to a class member so that it can be compared against later in testing and update the assertions to check this munged copy. * Shuffle objects around for cleaner testing Attach the temporary args dict to the task rather than the action as this makes updating the existing tests cleaner. --- lib/ansible/plugins/action/synchronize.py | 45 ++++++++++--------- .../fixtures/synchronize/basic/meta.yaml | 6 +-- .../synchronize/basic_become/meta.yaml | 8 ++-- .../synchronize/basic_become_cli/meta.yaml | 8 ++-- .../synchronize/basic_vagrant/meta.yaml | 8 ++-- .../basic_vagrant_become_cli/meta.yaml | 8 ++-- .../synchronize/basic_vagrant_sudo/meta.yaml | 8 ++-- .../synchronize/delegate_remote/meta.yaml | 6 +-- .../synchronize/delegate_remote_su/meta.yaml | 6 +-- test/units/plugins/action/test_synchronize.py | 5 ++- 10 files changed, 57 insertions(+), 51 deletions(-) diff --git a/lib/ansible/plugins/action/synchronize.py b/lib/ansible/plugins/action/synchronize.py index 2ff84b4f8d2..0dc98bfe39c 100644 --- a/lib/ansible/plugins/action/synchronize.py +++ b/lib/ansible/plugins/action/synchronize.py @@ -76,7 +76,7 @@ class ActionModule(ActionBase): path = self._get_absolute_path(path=path) return path - def _process_remote(self, host, path, user, port_matches_localhost_port): + def _process_remote(self, task_args, host, path, user, port_matches_localhost_port): """ :arg host: hostname for the path :arg path: file path @@ -99,7 +99,7 @@ class ActionModule(ActionBase): # fix up the rsync path to use the controller's public DNS/IP # instead of "localhost" if port_matches_localhost_port and host in C.LOCALHOST: - self._task.args['_substitute_controller'] = True + task_args['_substitute_controller'] = True return self._format_rsync_rsh_target(host, path, user) if ':' not in path and not path.startswith('/'): @@ -157,6 +157,11 @@ class ActionModule(ActionBase): if task_vars is None: task_vars = dict() + # We make a copy of the args here because we may fail and be asked to + # retry. If that happens we don't want to pass the munged args through + # to our next invocation. Munged args are single use only. + _tmp_args = self._task.args.copy() + result = super(ActionModule, self).run(tmp, task_vars) # Store remote connection type @@ -189,10 +194,10 @@ class ActionModule(ActionBase): result['msg'] = "synchronize uses rsync to function. rsync needs to connect to the remote host via ssh, docker client or a direct filesystem copy. This remote host is being accessed via %s instead so it cannot work." % self._connection.transport return result - use_ssh_args = self._task.args.pop('use_ssh_args', None) + use_ssh_args = _tmp_args.pop('use_ssh_args', None) # Parameter name needed by the ansible module - self._task.args['_local_rsync_path'] = task_vars.get('ansible_rsync_path') or 'rsync' + _tmp_args['_local_rsync_path'] = task_vars.get('ansible_rsync_path') or 'rsync' # rsync thinks that one end of the connection is localhost and the # other is the host we're running the task for (Note: We use @@ -229,9 +234,9 @@ class ActionModule(ActionBase): # CHECK FOR NON-DEFAULT SSH PORT inv_port = task_vars.get('ansible_ssh_port', None) or C.DEFAULT_REMOTE_PORT - if self._task.args.get('dest_port', None) is None: + if _tmp_args.get('dest_port', None) is None: if inv_port is not None: - self._task.args['dest_port'] = inv_port + _tmp_args['dest_port'] = inv_port # Set use_delegate if we are going to run rsync on a delegated host # instead of localhost @@ -273,12 +278,12 @@ class ActionModule(ActionBase): self._override_module_replaced_vars(task_vars) # SWITCH SRC AND DEST HOST PER MODE - if self._task.args.get('mode', 'push') == 'pull': + if _tmp_args.get('mode', 'push') == 'pull': (dest_host, src_host) = (src_host, dest_host) # MUNGE SRC AND DEST PER REMOTE_HOST INFO - src = self._task.args.get('src', None) - dest = self._task.args.get('dest', None) + src = _tmp_args.get('src', None) + dest = _tmp_args.get('dest', None) if src is None or dest is None: return dict(failed=True, msg="synchronize requires both src and dest parameters are set") @@ -289,12 +294,12 @@ class ActionModule(ActionBase): if private_key is not None: private_key = os.path.expanduser(private_key) - self._task.args['private_key'] = private_key + _tmp_args['private_key'] = private_key # Src and dest rsync "path" handling # Determine if we need a user@ user = None - if boolean(self._task.args.get('set_remote_user', 'yes')): + if boolean(_tmp_args.get('set_remote_user', 'yes')): if use_delegate: user = task_vars.get('ansible_delegated_vars', dict()).get('ansible_ssh_user', None) if not user: @@ -304,14 +309,14 @@ class ActionModule(ActionBase): user = task_vars.get('ansible_ssh_user') or self._play_context.remote_user # use the mode to define src and dest's url - if self._task.args.get('mode', 'push') == 'pull': + if _tmp_args.get('mode', 'push') == 'pull': # src is a remote path: @, dest is a local path - src = self._process_remote(src_host, src, user, inv_port in localhost_ports) + src = self._process_remote(_tmp_args, src_host, src, user, inv_port in localhost_ports) dest = self._process_origin(dest_host, dest, user) else: # src is a local path, dest is a remote path: @ src = self._process_origin(src_host, src, user) - dest = self._process_remote(dest_host, dest, user, inv_port in localhost_ports) + dest = self._process_remote(_tmp_args, dest_host, dest, user, inv_port in localhost_ports) else: # Still need to munge paths (to account for roles) even if we aren't # copying files between hosts @@ -320,11 +325,11 @@ class ActionModule(ActionBase): if not dest.startswith('/'): dest = self._get_absolute_path(path=dest) - self._task.args['src'] = src - self._task.args['dest'] = dest + _tmp_args['src'] = src + _tmp_args['dest'] = dest # Allow custom rsync path argument - rsync_path = self._task.args.get('rsync_path', None) + rsync_path = _tmp_args.get('rsync_path', None) if not dest_is_local: if self._play_context.become and not rsync_path: @@ -341,7 +346,7 @@ class ActionModule(ActionBase): # make sure rsync path is quoted. if rsync_path: - self._task.args['rsync_path'] = '"%s"' % rsync_path + _tmp_args['rsync_path'] = '"%s"' % rsync_path if use_ssh_args: ssh_args = [ @@ -349,7 +354,7 @@ class ActionModule(ActionBase): getattr(self._play_context, 'ssh_common_args', ''), getattr(self._play_context, 'ssh_extra_args', ''), ] - self._task.args['ssh_args'] = ' '.join([a for a in ssh_args if a]) + _tmp_args['ssh_args'] = ' '.join([a for a in ssh_args if a]) # If launching synchronize against docker container # use rsync_opts to support container to override rsh options @@ -364,7 +369,7 @@ class ActionModule(ActionBase): self._task.args['rsync_opts'].append("--rsh='%s exec -i'" % self._docker_cmd) # run the module and store the result - result.update(self._execute_module('synchronize', task_vars=task_vars)) + result.update(self._execute_module('synchronize', module_args=_tmp_args, task_vars=task_vars)) if 'SyntaxError' in result.get('exception', result.get('msg', '')): # Emit a warning about using python3 because synchronize is diff --git a/test/units/plugins/action/fixtures/synchronize/basic/meta.yaml b/test/units/plugins/action/fixtures/synchronize/basic/meta.yaml index d508bb373de..7608ebfaf32 100644 --- a/test/units/plugins/action/fixtures/synchronize/basic/meta.yaml +++ b/test/units/plugins/action/fixtures/synchronize/basic/meta.yaml @@ -12,6 +12,6 @@ asserts: - "SAM._connection.transport == 'local'" - "self._play_context.shell == 'sh'" - "self.execute_called" - - "self.task.args['_local_rsync_path'] == 'rsync'" - - "self.task.args['src'] == '/tmp/deleteme'" - - "self.task.args['dest'] == 'root@el6host:/tmp/deleteme'" + - "self.final_module_args['_local_rsync_path'] == 'rsync'" + - "self.final_module_args['src'] == '/tmp/deleteme'" + - "self.final_module_args['dest'] == 'root@el6host:/tmp/deleteme'" diff --git a/test/units/plugins/action/fixtures/synchronize/basic_become/meta.yaml b/test/units/plugins/action/fixtures/synchronize/basic_become/meta.yaml index 24daa108cc9..c507b3271af 100644 --- a/test/units/plugins/action/fixtures/synchronize/basic_become/meta.yaml +++ b/test/units/plugins/action/fixtures/synchronize/basic_become/meta.yaml @@ -23,11 +23,11 @@ asserts: - "hasattr(SAM._connection, 'ismock')" - "SAM._connection.transport == 'local'" - "self.execute_called" - - "self.task.args['_local_rsync_path'] == 'rsync'" + - "self.final_module_args['_local_rsync_path'] == 'rsync'" # this is a crucial aspect of this scenario ... - - "self.task.args['rsync_path'] == '\"sudo rsync\"'" - - "self.task.args['src'] == '/tmp/deleteme'" - - "self.task.args['dest'] == 'root@el6host:/tmp/deleteme'" + - "self.final_module_args['rsync_path'] == '\"sudo rsync\"'" + - "self.final_module_args['src'] == '/tmp/deleteme'" + - "self.final_module_args['dest'] == 'root@el6host:/tmp/deleteme'" - "self.task.become == True" - "self.task.become_user == None" - "self._play_context.shell == 'sh'" diff --git a/test/units/plugins/action/fixtures/synchronize/basic_become_cli/meta.yaml b/test/units/plugins/action/fixtures/synchronize/basic_become_cli/meta.yaml index f4399be30c1..ae8b295df29 100644 --- a/test/units/plugins/action/fixtures/synchronize/basic_become_cli/meta.yaml +++ b/test/units/plugins/action/fixtures/synchronize/basic_become_cli/meta.yaml @@ -23,11 +23,11 @@ asserts: - "hasattr(SAM._connection, 'ismock')" - "SAM._connection.transport == 'local'" - "self.execute_called" - - "self.task.args['_local_rsync_path'] == 'rsync'" + - "self.final_module_args['_local_rsync_path'] == 'rsync'" # this is a crucial aspect of this scenario ... - - "self.task.args['rsync_path'] == '\"sudo rsync\"'" - - "self.task.args['src'] == '/tmp/deleteme'" - - "self.task.args['dest'] == 'root@el6host:/tmp/deleteme'" + - "self.final_module_args['rsync_path'] == '\"sudo rsync\"'" + - "self.final_module_args['src'] == '/tmp/deleteme'" + - "self.final_module_args['dest'] == 'root@el6host:/tmp/deleteme'" - "self.task.become == None" - "self.task.become_user == None" - "self._play_context.shell == 'sh'" diff --git a/test/units/plugins/action/fixtures/synchronize/basic_vagrant/meta.yaml b/test/units/plugins/action/fixtures/synchronize/basic_vagrant/meta.yaml index 15a3a1a06fc..7654cc6d77e 100644 --- a/test/units/plugins/action/fixtures/synchronize/basic_vagrant/meta.yaml +++ b/test/units/plugins/action/fixtures/synchronize/basic_vagrant/meta.yaml @@ -17,10 +17,10 @@ asserts: - "hasattr(SAM._connection, 'ismock')" - "SAM._connection.transport == 'local'" - "self.execute_called" - - "self.task.args['_local_rsync_path'] == 'rsync'" - - "self.task.args['dest_port'] == 2202" - - "self.task.args['src'] == '/tmp/deleteme'" - - "self.task.args['dest'] == 'vagrant@127.0.0.1:/tmp/deleteme'" + - "self.final_module_args['_local_rsync_path'] == 'rsync'" + - "self.final_module_args['dest_port'] == 2202" + - "self.final_module_args['src'] == '/tmp/deleteme'" + - "self.final_module_args['dest'] == 'vagrant@127.0.0.1:/tmp/deleteme'" - "self._play_context.shell == 'sh'" - "self._play_context.remote_addr == '127.0.0.1'" - "self._play_context.remote_user == 'vagrant'" diff --git a/test/units/plugins/action/fixtures/synchronize/basic_vagrant_become_cli/meta.yaml b/test/units/plugins/action/fixtures/synchronize/basic_vagrant_become_cli/meta.yaml index 728147ec218..242de9208c7 100644 --- a/test/units/plugins/action/fixtures/synchronize/basic_vagrant_become_cli/meta.yaml +++ b/test/units/plugins/action/fixtures/synchronize/basic_vagrant_become_cli/meta.yaml @@ -20,10 +20,10 @@ asserts: - "hasattr(SAM._connection, 'ismock')" - "SAM._connection.transport == 'local'" - "self.execute_called" - - "self.task.args['_local_rsync_path'] == 'rsync'" - - "self.task.args['dest_port'] == 2202" - - "self.task.args['src'] == '/tmp/deleteme'" - - "self.task.args['dest'] == 'vagrant@127.0.0.1:/tmp/deleteme'" + - "self.final_module_args['_local_rsync_path'] == 'rsync'" + - "self.final_module_args['dest_port'] == 2202" + - "self.final_module_args['src'] == '/tmp/deleteme'" + - "self.final_module_args['dest'] == 'vagrant@127.0.0.1:/tmp/deleteme'" - "self._play_context.shell == 'sh'" - "self._play_context.remote_addr == '127.0.0.1'" - "self._play_context.remote_user == 'vagrant'" diff --git a/test/units/plugins/action/fixtures/synchronize/basic_vagrant_sudo/meta.yaml b/test/units/plugins/action/fixtures/synchronize/basic_vagrant_sudo/meta.yaml index 15a3a1a06fc..7654cc6d77e 100644 --- a/test/units/plugins/action/fixtures/synchronize/basic_vagrant_sudo/meta.yaml +++ b/test/units/plugins/action/fixtures/synchronize/basic_vagrant_sudo/meta.yaml @@ -17,10 +17,10 @@ asserts: - "hasattr(SAM._connection, 'ismock')" - "SAM._connection.transport == 'local'" - "self.execute_called" - - "self.task.args['_local_rsync_path'] == 'rsync'" - - "self.task.args['dest_port'] == 2202" - - "self.task.args['src'] == '/tmp/deleteme'" - - "self.task.args['dest'] == 'vagrant@127.0.0.1:/tmp/deleteme'" + - "self.final_module_args['_local_rsync_path'] == 'rsync'" + - "self.final_module_args['dest_port'] == 2202" + - "self.final_module_args['src'] == '/tmp/deleteme'" + - "self.final_module_args['dest'] == 'vagrant@127.0.0.1:/tmp/deleteme'" - "self._play_context.shell == 'sh'" - "self._play_context.remote_addr == '127.0.0.1'" - "self._play_context.remote_user == 'vagrant'" diff --git a/test/units/plugins/action/fixtures/synchronize/delegate_remote/meta.yaml b/test/units/plugins/action/fixtures/synchronize/delegate_remote/meta.yaml index 49c96786ff2..06a08e6ae54 100644 --- a/test/units/plugins/action/fixtures/synchronize/delegate_remote/meta.yaml +++ b/test/units/plugins/action/fixtures/synchronize/delegate_remote/meta.yaml @@ -21,6 +21,6 @@ asserts: - "SAM._connection.transport == 'ssh'" - "self._play_context.shell == None" - "self.execute_called" - - "self.task.args['_local_rsync_path'] == 'rsync'" - - "self.task.args['src'] == '/tmp/deleteme'" - - "self.task.args['dest'] == 'el6host:/tmp/deleteme'" + - "self.final_module_args['_local_rsync_path'] == 'rsync'" + - "self.final_module_args['src'] == '/tmp/deleteme'" + - "self.final_module_args['dest'] == 'el6host:/tmp/deleteme'" diff --git a/test/units/plugins/action/fixtures/synchronize/delegate_remote_su/meta.yaml b/test/units/plugins/action/fixtures/synchronize/delegate_remote_su/meta.yaml index 0abc8bf6c46..47ae6e1d613 100644 --- a/test/units/plugins/action/fixtures/synchronize/delegate_remote_su/meta.yaml +++ b/test/units/plugins/action/fixtures/synchronize/delegate_remote_su/meta.yaml @@ -28,6 +28,6 @@ asserts: - "not self._play_context.become" - "self._play_context.become_method == 'su'" - "self.execute_called" - - "self.task.args['_local_rsync_path'] == 'rsync'" - - "self.task.args['src'] == '/tmp/deleteme'" - - "self.task.args['dest'] == 'el6host:/tmp/deleteme'" + - "self.final_module_args['_local_rsync_path'] == 'rsync'" + - "self.final_module_args['src'] == '/tmp/deleteme'" + - "self.final_module_args['dest'] == 'el6host:/tmp/deleteme'" diff --git a/test/units/plugins/action/test_synchronize.py b/test/units/plugins/action/test_synchronize.py index a22107d0a80..8072bd04440 100644 --- a/test/units/plugins/action/test_synchronize.py +++ b/test/units/plugins/action/test_synchronize.py @@ -99,11 +99,12 @@ class SynchronizeTester(object): execute_called = False - def _execute_module(self, module_name, task_vars=None): + def _execute_module(self, module_name, module_args=None, task_vars=None): self.execute_called = True + self.final_module_args = module_args self.final_task_vars = task_vars return {} - + def runtest(self, fixturepath='fixtures/synchronize/basic'): metapath = os.path.join(fixturepath, 'meta.yaml')