From a0dfa8616a8e7b00ce8c6f344ea721a387434721 Mon Sep 17 00:00:00 2001 From: Toshio Kuratomi Date: Mon, 17 Apr 2017 10:55:56 -0700 Subject: [PATCH] Make fetch default to fail on errors Fixes #23501 --- CHANGELOG.md | 3 ++ lib/ansible/modules/files/fetch.py | 16 ++++++---- lib/ansible/plugins/action/__init__.py | 3 ++ lib/ansible/plugins/action/fetch.py | 29 ++++++++++++------- test/integration/targets/fetch/tasks/main.yml | 15 ++++++++-- 5 files changed, 48 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d1a76189b9..6a7f3fdfbf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,6 +45,9 @@ Ansible Changes By Release The new behaviour mirrors how the variables would appear if there was no hash mark in the string. +- As of 2.4.0, the fetch module fails if there are errors reading the remote + file. Use ignore_errors or failed_when in playbooks if you wish to ignore + errors. #### New Inventory scripts: - lxd diff --git a/lib/ansible/modules/files/fetch.py b/lib/ansible/modules/files/fetch.py index b4f5680735..c3fc43d3e8 100644 --- a/lib/ansible/modules/files/fetch.py +++ b/lib/ansible/modules/files/fetch.py @@ -27,9 +27,7 @@ short_description: Fetches a file from remote nodes description: - This module works like M(copy), but in reverse. It is used for fetching files from remote machines and storing them locally in a file tree, - organized by hostname. Note that this module is written to transfer - log files that might not be present, so a missing remote file won't - be an error unless fail_on_missing is set to 'yes'. + organized by hostname. version_added: "0.2" options: src: @@ -50,10 +48,13 @@ options: fail_on_missing: version_added: "1.1" description: - - When set to 'yes', the task will fail if the source file is missing. + - When set to 'yes', the task will fail if the remote file cannot be + read for any reason. Prior to Ansible-2.4, setting this would only fail + if the source file was missing. + - The default was changed to "yes" in Ansible-2.4. required: false choices: [ "yes", "no" ] - default: "no" + default: "yes" validate_checksum: version_added: "1.4" description: @@ -80,6 +81,11 @@ notes: depending on the file size can consume all available memory on the remote or local hosts causing a C(MemoryError). Due to this it is advisable to run this module without C(become) whenever possible. + - Prior to Ansible-2.4 this module would not fail if reading the remote + file was impossible unless fail_on_missing was set. In Ansible-2.4+, + playbook authors are encouraged to use fail_when or ignore_errors to + get this ability. They may also explicitly set fail_on_missing to False + to get the non-failing behaviour. ''' EXAMPLES = ''' diff --git a/lib/ansible/plugins/action/__init__.py b/lib/ansible/plugins/action/__init__.py index b3df22061e..bed9b48d22 100644 --- a/lib/ansible/plugins/action/__init__.py +++ b/lib/ansible/plugins/action/__init__.py @@ -518,6 +518,7 @@ class ActionBase(with_metaclass(ABCMeta, object)): 2 = permissions issue 3 = its a directory, not a file 4 = stat module failed, likely due to not finding python + 5 = appropriate json module not found ''' x = "0" # unknown error has occurred try: @@ -532,6 +533,8 @@ class ActionBase(with_metaclass(ABCMeta, object)): x = "2" # cannot read file elif errormsg.endswith(u'MODULE FAILURE'): x = "4" # python not found or module uncaught exception + elif 'json' in errormsg or 'simplejson' in errormsg: + x = "5" # json or simplejson modules needed finally: return x diff --git a/lib/ansible/plugins/action/fetch.py b/lib/ansible/plugins/action/fetch.py index 6fda9070a3..db418ecad2 100644 --- a/lib/ansible/plugins/action/fetch.py +++ b/lib/ansible/plugins/action/fetch.py @@ -122,26 +122,31 @@ class ActionModule(ActionBase): dest = dest.replace("//","/") - if remote_checksum in ('0', '1', '2', '3', '4'): - # these don't fail because you may want to transfer a log file that - # possibly MAY exist but keep going to fetch other log files + if remote_checksum in ('0', '1', '2', '3', '4', '5'): result['changed'] = False result['file'] = source if remote_checksum == '0': result['msg'] = "unable to calculate the checksum of the remote file" elif remote_checksum == '1': - if fail_on_missing: - result['failed'] = True - del result['changed'] - result['msg'] = "the remote file does not exist" - else: - result['msg'] = "the remote file does not exist, not transferring, ignored" + result['msg'] = "the remote file does not exist" elif remote_checksum == '2': - result['msg'] = "no read permission on remote file, not transferring, ignored" + result['msg'] = "no read permission on remote file" elif remote_checksum == '3': result['msg'] = "remote file is a directory, fetch cannot work on directories" elif remote_checksum == '4': result['msg'] = "python isn't present on the system. Unable to compute checksum" + elif remote_checksum == '5': + result['msg'] = "stdlib json or simplejson was not found on the remote machine. Only the raw module can work without those installed" + # Historically, these don't fail because you may want to transfer + # a log file that possibly MAY exist but keep going to fetch other + # log files. Today, this is better achieved by adding + # ignore_errors or failed_when to the task. Control the behaviour + # via fail_when_missing + if fail_on_missing: + result['failed'] = True + del result['changed'] + else: + result['msg'] += ", not transferring, ignored" return result # calculate checksum for the local file @@ -173,7 +178,9 @@ class ActionModule(ActionBase): msg="checksum mismatch", file=source, dest=dest, remote_md5sum=None, checksum=new_checksum, remote_checksum=remote_checksum)) else: - result.update(dict(changed=True, md5sum=new_md5, dest=dest, remote_md5sum=None, checksum=new_checksum, remote_checksum=remote_checksum)) + result.update({'changed': True, 'md5sum': new_md5, 'dest': dest, + 'remote_md5sum': None, 'checksum': new_checksum, + 'remote_checksum': remote_checksum}) else: # For backwards compatibility. We'll return None on FIPS enabled systems try: diff --git a/test/integration/targets/fetch/tasks/main.yml b/test/integration/targets/fetch/tasks/main.yml index ed249f7568..7fb34b385e 100644 --- a/test/integration/targets/fetch/tasks/main.yml +++ b/test/integration/targets/fetch/tasks/main.yml @@ -56,7 +56,7 @@ - 'fetched["checksum"] == "a94a8fe5ccb19ba61c4c0873d391e987982fbbd3"' - name: attempt to fetch a non-existent file - do not fail on missing - fetch: src={{ output_dir }}/doesnotexist dest={{ output_dir }}/fetched + fetch: src={{ output_dir }}/doesnotexist dest={{ output_dir }}/fetched fail_on_missing=False register: fetch_missing_nofail - name: check fetch missing no fail result @@ -78,7 +78,7 @@ - "not fetch_missing|changed" - name: attempt to fetch a directory - should not fail but return a message - fetch: src={{ output_dir }} dest={{ output_dir }}/somedir + fetch: src={{ output_dir }} dest={{ output_dir }}/somedir fail_on_missing=False register: fetch_dir - name: check fetch directory result @@ -87,6 +87,17 @@ - "not fetch_dir|changed" - "fetch_dir.msg" +- name: attempt to fetch a directory - should fail + fetch: src={{ output_dir }} dest={{ output_dir }}/somedir fail_on_missing=True + register: failed_fetch_dir + ignore_errors: true + +- name: check fetch directory result + assert: + that: + - "failed_fetch_dir['failed']" + - "fetch_dir.msg" + - name: create symlink to a file that we can fetch file: path: "{{ output_dir }}/link"