win_copy: Add force parameter and check-mode support

[dagwieers]

ISSUE TYPE

• Feature Pull Request

COMPONENT NAME

win_copy

ANSIBLE VERSION

v2.3

SUMMARY

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 no less than 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 no checksums are being
performed by the action plugin and the module. We do this by delaying
the checksum calls until we have determined whether the destination in
fact exists. More optimisations are still possible, especially in the case the
destination does not exist (we don't need a checksum in that case anyhow).

[ansibot]

cc @jhawkesworth
click here for bot help

[nitzmahone]

creates/removes is ... weird for win_copy IMO. For the use case you mentioned, the force arg as implemented by copy makes more sense to me.

[dagwieers]

I knew you were going to say that :-)

The force parameter is weird. In my case I want to skip the checksum on huge files, which is why creates: does make sense (it shorts the normal idempotency based on presence or absence of a file). I know it sounds pretty weird as win_copy' does not really remove something, but I figured it could be useful to base your decision to copy something on something else. (e.g. only install inkscape templates if the inkscape-directory exists, aka. removes`)

So I will have to say force: no to skip the checksum. These parameters are so wrong on so many levels it hurts :-/

I guess the name of all these parameters was a bad choice (since its name does make sense for all use-cases, quite the opposite sometimes)...

[dagwieers]

Have it your way :-)

As a bonus, to please the gods, I also added check-mode support !

[dagwieers]

After careful testing, it appears we have another problem. The checksumming is done first by the action plugin at the source (4GB), then again with a remote stat at the destination. And then again by the module itself on the copied source, old destination and copied destination. So in fact it is being done 5 times (once at the controlling master, and four times at the target node). Fine for small files, but it explains why it takes this long even if the file is already there and identical :-(

Maybe we also need a dedicated option to disable any checksums.

Update: I refactored some of the logic to delay the checksumming so we can avoid it for some codepaths.

[dagwieers]

@nitzmahone @jhawkesworth Is this ready to be merged ?

[bcoca]

@jborean93 @dagwieers the force parameter came 'late' to copy, it was added to allow 'not copying' a file, I agree it is not well named but the default behaviour (which force=true reflects) was always 'copy when we detect it is needed'. The force=no was added to allow for not copying when file existed even if it is different.

[dagwieers]

@bcoca Can you sign this PR off ? I'd like to leave this one behind me now :-)

[bcoca]

@dagwieers you are not making the local checksum conditional on force?

[dagwieers]

@bcoca Yes, we do. We only skip it when the file does not exist remotely.

           if dest_status['exists'] and not force:
                # remote_file exists so continue to next iteration.
                continue
  
            # Generate a hash of the local file.
            local_checksum = checksum(source_full)

This was the original code BTW. I only corrected the comment.

In your proposal it was not attempted when force: no, but we do need it in case the remote file exists, otherwise it will fail because local_checksum was not defined.

[bcoca]

which i avoided with if force prepended, you don't need it to decide a copy when force=no

[bcoca]

just use as e, controller supports >=2.6 so no need for this work around

[dagwieers]

@bcoca The very next line compares local_checksum with the remote checksum in case force: yes or the remote file does not exist. So strictly it is not needed, but the code still expects it. We could of course initialize it to None to avoid it. Let me know what you prefer.

[bcoca]

in my paste i had 'if force and ...` to avoid that, no preference either way.

[dagwieers]

You are right, I missed that. Damnit, you win !

[bcoca]

I always win, <voice_of_tree_creature>I AM ROOT</voice_of_tree_creature>.

[mattclay]

CI failure in integration tests due to:

2017-02-24 19:25:05 An exception occurred during task execution. The full traceback is:
2017-02-24 19:25:05 Traceback (most recent call last):
2017-02-24 19:25:05   File "/private/var/root/ansible/lib/ansible/executor/task_executor.py", line 126, in run
2017-02-24 19:25:05     res = self._execute()
2017-02-24 19:25:05   File "/private/var/root/ansible/lib/ansible/executor/task_executor.py", line 512, in _execute
2017-02-24 19:25:05     result = self._handler.run(task_vars=variables)
2017-02-24 19:25:05   File "/private/var/root/ansible/lib/ansible/plugins/action/copy.py", line 295, in run
2017-02-24 19:25:05     module_return['checksum'] = local_checksum
2017-02-24 19:25:05 UnboundLocalError: local variable 'local_checksum' referenced before assignment
2017-02-24 19:25:05 
2017-02-24 19:25:05 fatal: [testhost]: FAILED! => {
2017-02-24 19:25:05     "failed": true, 
2017-02-24 19:25:05     "msg": "Unexpected failure during module execution.", 
2017-02-24 19:25:05     "stdout": ""
2017-02-24 19:25:05 }

[nitzmahone]

(single failure was the flaky win_service test, unrelated to anything about this)

Thanks all for the input to get this one done!

[jhawkesworth]

Yay!!!!