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

@@ -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)
+        src = _tmp_args.get('src', None)
-        dest = self._task.args.get('dest', 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: <user>@<host>, 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: <user>@<host>
                 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
+        _tmp_args['src'] = src
-        self._task.args['dest'] = dest
+        _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

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.final_module_args['_local_rsync_path'] == 'rsync'"
-    - "self.task.args['src'] == '/tmp/deleteme'"
+    - "self.final_module_args['src'] == '/tmp/deleteme'"
-    - "self.task.args['dest'] == 'root@el6host:/tmp/deleteme'"
+    - "self.final_module_args['dest'] == 'root@el6host:/tmp/deleteme'"

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.final_module_args['rsync_path'] == '\"sudo rsync\"'"
-    - "self.task.args['src'] == '/tmp/deleteme'"
+    - "self.final_module_args['src'] == '/tmp/deleteme'"
-    - "self.task.args['dest'] == 'root@el6host:/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'"

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.final_module_args['rsync_path'] == '\"sudo rsync\"'"
-    - "self.task.args['src'] == '/tmp/deleteme'"
+    - "self.final_module_args['src'] == '/tmp/deleteme'"
-    - "self.task.args['dest'] == 'root@el6host:/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'"

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.final_module_args['_local_rsync_path'] == 'rsync'"
-    - "self.task.args['dest_port'] == 2202"
+    - "self.final_module_args['dest_port'] == 2202"
-    - "self.task.args['src'] == '/tmp/deleteme'"
+    - "self.final_module_args['src'] == '/tmp/deleteme'"
-    - "self.task.args['dest'] == 'vagrant@127.0.0.1:/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'"

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.final_module_args['_local_rsync_path'] == 'rsync'"
-    - "self.task.args['dest_port'] == 2202"
+    - "self.final_module_args['dest_port'] == 2202"
-    - "self.task.args['src'] == '/tmp/deleteme'"
+    - "self.final_module_args['src'] == '/tmp/deleteme'"
-    - "self.task.args['dest'] == 'vagrant@127.0.0.1:/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'"

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.final_module_args['_local_rsync_path'] == 'rsync'"
-    - "self.task.args['dest_port'] == 2202"
+    - "self.final_module_args['dest_port'] == 2202"
-    - "self.task.args['src'] == '/tmp/deleteme'"
+    - "self.final_module_args['src'] == '/tmp/deleteme'"
-    - "self.task.args['dest'] == 'vagrant@127.0.0.1:/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'"

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.final_module_args['_local_rsync_path'] == 'rsync'"
-    - "self.task.args['src'] == '/tmp/deleteme'"
+    - "self.final_module_args['src'] == '/tmp/deleteme'"
-    - "self.task.args['dest'] == 'el6host:/tmp/deleteme'"
+    - "self.final_module_args['dest'] == 'el6host:/tmp/deleteme'"

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.final_module_args['_local_rsync_path'] == 'rsync'"
-    - "self.task.args['src'] == '/tmp/deleteme'"
+    - "self.final_module_args['src'] == '/tmp/deleteme'"
-    - "self.task.args['dest'] == 'el6host:/tmp/deleteme'"
+    - "self.final_module_args['dest'] == 'el6host:/tmp/deleteme'"

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')