win_copy: Add force parameter and check-mode support (#20405)

* win_copy: Add force parameter and check-mode support The rationale behind this is that if you're working with +3GB files, creating the checksum takes a lot of time, which we can avoid by simply testing if the file exists. I also took the liberty to put the various parameters together. It probably takes a (neglible) performance hit but makes the code a bit easier to inspect/work with, as its closer to all other windows modules. On a normal run, the action plugin does a local checksum of the source and a remote checksum of the destination. And afterwards, the module will do another remote checksum of the copied source, a remote checksum of the original destination, and another remote checksum of the copied destination. On a very huge file (think 4GB) that means 5x reading the complete file (if you have a large cache you may get away with it, otherwise you're doomed !). This patch will ensure with `force: no` that not checksums are being performed. * Moving presence check before remote checksum * Adapted to wishes * Even more performance improvements
8 years ago · 98934939af
parent ce08b4165d
commit 98934939af
4 changed files with 63 additions and 61 deletions
--- a/lib/ansible/modules/windows/win_copy.ps1
+++ b/lib/ansible/modules/windows/win_copy.ps1
@ -17,47 +17,43 @@
 # WANT_JSON
 # POWERSHELL_COMMON

-$params = Parse-Args $args
+$params = Parse-Args $args -supports_check_mode $true

-$src = Get-Attr $params "src" $FALSE -type "path"
-If ($src -eq $FALSE)
-{
-    Fail-Json (New-Object psobject) "missing required argument: src"
-}
+$check_mode = Get-AnsibleParam -obj $params -name "_ansible_check_mode" -type "bool" -default $false

-$dest = Get-Attr $params "dest" $FALSE -type "path"
-If ($dest -eq $FALSE)
-{
-    Fail-Json (New-Object psobject) "missing required argument: dest"
-}
-
-$original_basename = Get-Attr $params "original_basename" $FALSE
-If ($original_basename -eq $FALSE)
-{
-    Fail-Json (New-Object psobject) "missing required argument: original_basename "
-}
+$src = Get-AnsibleParam -obj $params -name "src" -type "path" -failifempty $true
+$dest = Get-AnsibleParam -obj $params -name "dest" -type "path" -failifempty $true
+$force = Get-AnsibleParam -obj $params -name "force" -type "bool" -default $true
+$original_basename = Get-AnsibleParam -obj $params -name "original_basename" -type "str" -failifempty $true

-$result = New-Object psobject @{
+$result = @{
    changed = $FALSE
    original_basename = $original_basename
+    src = $src
+    dest = $dest
 }

 # original_basename gets set if src and dest are dirs
 # but includes subdir if the source folder contains sub folders
-# e.g. you could get subdir/foo.txt 
+# e.g. you could get subdir/foo.txt
+
+if (($force -eq $false) -and (Test-Path -Path $dest)) {
+    $result.msg = "file already exists"
+    Exit-Json $result
+}

 # detect if doing recursive folder copy and create any non-existent destination sub folder
 $parent = Split-Path -Path $original_basename -Parent
-if ($parent.length -gt 0) 
+if ($parent.length -gt 0)
 {
    $dest_folder = Join-Path $dest $parent
-    New-Item -Force $dest_folder -Type directory
+    New-Item -Path $dest_folder -Type Directory -Force -WhatIf:$check_mode
 }

 # if $dest is a dir, append $original_basename so the file gets copied with its intended name.
-if (Test-Path $dest -PathType Container)
+if (Test-Path -Path $dest -PathType Container)
 {
-    $dest = Join-Path $dest $original_basename
+    $dest = Join-Path -Path $dest -ChildPath $original_basename
 }

 $orig_checksum = Get-FileChecksum ($dest)
@ -69,8 +65,9 @@ If ($src_checksum.Equals($orig_checksum))
    If ($src_checksum.Equals("3"))
    {
       # New-Item -Force creates subdirs for recursive copies
-       New-Item -Force $dest -Type file
-       Copy-Item -Path $src -Destination $dest -Force
+       New-Item -Path $dest -Type File -Force -WhatIf:$check_mode
+       Copy-Item -Path $src -Destination $dest -Force -WhatIf:$check_mode
+       $result.changed = $true
       $result.operation = "folder_copy"
    }

@ -79,24 +76,20 @@ ElseIf (-Not $src_checksum.Equals($orig_checksum))
 {
    If ($src_checksum.Equals("3"))
    {
-       Fail-Json (New-Object psobject) "If src is a folder, dest must also be a folder"
+        Fail-Json $result "If src is a folder, dest must also be a folder"
    }
    # The checksums don't match, there's something to do
-    Copy-Item -Path $src -Destination $dest -Force
+    Copy-Item -Path $src -Destination $dest -Force -WhatIf:$check_mode
+
+    $result.changed = $true
    $result.operation = "file_copy"
 }

-# verify before we return that the file has changed
-$dest_checksum = Get-FileChecksum ($dest)
-If ($src_checksum.Equals($dest_checksum))
-{
-    If (-Not $orig_checksum.Equals($dest_checksum)) {
-        $result.changed = $TRUE
-    }
-}
-Else
+# Verify before we return that the file has changed
+$dest_checksum = Get-FileChecksum($dest)
+If (-Not $src_checksum.Equals($dest_checksum) -And -Not $check_mode)
 {
-    Fail-Json (New-Object psobject) "src checksum $src_checksum did not match dest_checksum $dest_checksum  Failed to place file $original_basename in $dest"
+    Fail-Json $result "src checksum $src_checksum did not match dest_checksum $dest_checksum, failed to place file $original_basename in $dest"
 }

 $info = Get-Item $dest
--- a/lib/ansible/modules/windows/win_copy.py
+++ b/lib/ansible/modules/windows/win_copy.py
@ -44,6 +44,15 @@ options:
      - Remote absolute path where the file should be copied to. If src is a directory,
        this must be a directory too. Use \\ for path separators.
    required: true
+  force:
+    version_added: "2.3"
+    description:
+      - If set to C(yes), the remote file will be replaced when content is different than the source.
+      - If set to C(no), the remote file will only be transferred if the destination does not exist.
+    default: True
+    choices:
+    - yes
+    - no
 author: "Jon Hawkesworth (@jhawkesworth)"
 '''

@ -51,29 +60,29 @@ EXAMPLES = r'''
 - name: Copy a single file
  win_copy:
    src: /srv/myfiles/foo.conf
-    dest: c:\Temp\foo.conf
+    dest: C:\Temp\foo.conf

 - name: Copy files/temp_files to c:\temp
  win_copy:
    src: files/temp_files/
-    dest: c:\Temp
+    dest: C:\Temp\
 '''
 RETURN = r'''
 dest:
    description: destination file/path
    returned: changed
    type: string
-    sample: c:\Temp
+    sample: C:\Temp\
 src:
    description: source file used for the copy on the target machine
    returned: changed
    type: string
-    sample: "/home/httpd/.ansible/tmp/ansible-tmp-1423796390.97-147729857856000/source"
+    sample: /home/httpd/.ansible/tmp/ansible-tmp-1423796390.97-147729857856000/source
 checksum:
    description: sha1 checksum of the file after running copy
    returned: success
    type: string
-    sample: "6e642bb8dd5c2e027bf21dd923337cbb4214f827"
+    sample: 6e642bb8dd5c2e027bf21dd923337cbb4214f827
 size:
    description: size of the target, after execution
    returned: changed (single files only)
@ -83,11 +92,11 @@ operation:
    description: whether a single file copy took place or a folder copy
    returned: changed (single files only)
    type: string
-    sample: "file_copy"
+    sample: file_copy
 original_basename:
    description: basename of the copied file
    returned: changed (single files only)
    type: string
-    sample: "foo.txt"
+    sample: foo.txt
 '''

--- a/lib/ansible/plugins/action/init.py
+++ b/lib/ansible/plugins/action/init.py
@ -471,7 +471,7 @@ class ActionBase(with_metaclass(ABCMeta, object)):
        res = self._low_level_execute_command(cmd, sudoable=sudoable)
        return res

-    def _execute_remote_stat(self, path, all_vars, follow, tmp=None):
+    def _execute_remote_stat(self, path, all_vars, follow, tmp=None, checksum=True):
        '''
        Get information from remote file.
        '''
@ -479,7 +479,7 @@ class ActionBase(with_metaclass(ABCMeta, object)):
            path=path,
            follow=follow,
            get_md5=False,
-            get_checksum=True,
+            get_checksum=checksum,
            checksum_algo='sha1',
        )
        mystat = self._execute_module(module_name='stat', module_args=module_args, task_vars=all_vars, tmp=tmp, delete_remote_tmp=(tmp is None), wrap_async=False)
--- a/lib/ansible/plugins/action/copy.py
+++ b/lib/ansible/plugins/action/copy.py
@ -25,7 +25,7 @@ import stat
 import tempfile

 from ansible.constants import mk_boolean as boolean
-from ansible.errors import AnsibleError
+from ansible.errors import AnsibleError, AnsibleFileNotFound
 from ansible.module_utils._text import to_bytes, to_native, to_text
 from ansible.plugins.action import ActionBase
 from ansible.utils.hashing import checksum
@ -155,10 +155,14 @@ class ActionModule(ActionBase):
        diffs = []
        for source_full, source_rel in source_files:

-            source_full = self._loader.get_real_file(source_full)
-
-            # Generate a hash of the local file.
-            local_checksum = checksum(source_full)
+            # If the local file does not exist, get_real_file() raises AnsibleFileNotFound
+            try:
+                source_full = self._loader.get_real_file(source_full)
+            except AnsibleFileNotFound as e:
+                result['failed'] = True
+                result['msg'] = "could not find src=%s, %s" % (source_full, e)
+                self._remove_tmp_path(tmp)
+                return result

            # Get the local mode and set if user wanted it preserved
            # https://github.com/ansible/ansible-modules-core/issues/1124
@ -166,13 +170,6 @@ class ActionModule(ActionBase):
                lmode = '0%03o' % stat.S_IMODE(os.stat(source_full).st_mode)
                self._task.args['mode'] = lmode

-            # If local_checksum is not defined we can't find the file so we should fail out.
-            if local_checksum is None:
-                result['failed'] = True
-                result['msg'] = "could not find src=%s" % source_full
-                self._remove_tmp_path(tmp)
-                return result
-
            # This is kind of optimization - if user told us destination is
            # dir, do path manipulation right away, otherwise we still check
            # for dest being a dir via remote call below.
@ -182,7 +179,7 @@ class ActionModule(ActionBase):
                dest_file = self._connection._shell.join_path(dest)

            # Attempt to get remote file info
-            dest_status = self._execute_remote_stat(dest_file, all_vars=task_vars, follow=follow, tmp=tmp)
+            dest_status = self._execute_remote_stat(dest_file, all_vars=task_vars, follow=follow, tmp=tmp, checksum=force)

            if dest_status['exists'] and dest_status['isdir']:
                # The dest is a directory.
@ -196,12 +193,15 @@ class ActionModule(ActionBase):
                else:
                    # Append the relative source location to the destination and get remote stats again
                    dest_file = self._connection._shell.join_path(dest, source_rel)
-                    dest_status = self._execute_remote_stat(dest_file, all_vars=task_vars, follow=follow, tmp=tmp)
+                    dest_status = self._execute_remote_stat(dest_file, all_vars=task_vars, follow=follow, tmp=tmp, checksum=force)

            if dest_status['exists'] and not force:
-                # remote_file does not exist so continue to next iteration.
+                # remote_file exists so continue to next iteration.
                continue

+            # Generate a hash of the local file.
+            local_checksum = checksum(source_full)
+
            if local_checksum != dest_status['checksum']:
                # The checksums don't match and we will change or error out.
                changed = True