From 98934939af06f965182e56bab18049c6c1a68b8d Mon Sep 17 00:00:00 2001 From: Dag Wieers Date: Sat, 25 Feb 2017 03:10:09 +0100 Subject: [PATCH] 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 --- lib/ansible/modules/windows/win_copy.ps1 | 67 +++++++++++------------- lib/ansible/modules/windows/win_copy.py | 23 +++++--- lib/ansible/plugins/action/__init__.py | 4 +- lib/ansible/plugins/action/copy.py | 30 +++++------ 4 files changed, 63 insertions(+), 61 deletions(-) diff --git a/lib/ansible/modules/windows/win_copy.ps1 b/lib/ansible/modules/windows/win_copy.ps1 index 50024f4bf94..79ef1b72de3 100644 --- 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 diff --git a/lib/ansible/modules/windows/win_copy.py b/lib/ansible/modules/windows/win_copy.py index 90a49f03f7d..4e7384f7132 100644 --- 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 ''' diff --git a/lib/ansible/plugins/action/__init__.py b/lib/ansible/plugins/action/__init__.py index 4f161a3f64e..83a73be381b 100644 --- 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) diff --git a/lib/ansible/plugins/action/copy.py b/lib/ansible/plugins/action/copy.py index dd4d337071e..3395ec39f81 100644 --- 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