From 7a9916422a843ab94cad0d7c37d45d9b4fe54260 Mon Sep 17 00:00:00 2001 From: James Cammarata Date: Wed, 22 Jul 2015 14:25:47 -0400 Subject: [PATCH] Fixing up error handling for fetch_file ops in connection plugins * enable batch mode (configurable with a config option, on by default) for sftp transfers, so we can catch errors more easily * general cleanup in the local connection plugin and fetch action plugin Fixes #11612 --- examples/ansible.cfg | 5 +++++ lib/ansible/constants.py | 1 + lib/ansible/plugins/action/fetch.py | 9 ++++++--- lib/ansible/plugins/connections/local.py | 6 ++---- lib/ansible/plugins/connections/ssh.py | 6 ++++++ 5 files changed, 20 insertions(+), 7 deletions(-) diff --git a/examples/ansible.cfg b/examples/ansible.cfg index 2b68ad9e7ea..b6845c07033 100644 --- a/examples/ansible.cfg +++ b/examples/ansible.cfg @@ -220,6 +220,11 @@ fact_caching = memory # (default is sftp) #scp_if_ssh = True +# if False, sftp will not use batch mode to transfer files. This may cause some +# types of file transfer failures impossible to catch however, and should +# only be disabled if your sftp version has problems with batch mode +#sftp_batch_mode = False + [accelerate] accelerate_port = 5099 accelerate_timeout = 30 diff --git a/lib/ansible/constants.py b/lib/ansible/constants.py index 43ae782e195..1ff33205242 100644 --- a/lib/ansible/constants.py +++ b/lib/ansible/constants.py @@ -130,6 +130,7 @@ DEFAULT_ASK_VAULT_PASS = get_config(p, DEFAULTS, 'ask_vault_pass', 'ANSIBL DEFAULT_VAULT_PASSWORD_FILE = shell_expand_path(get_config(p, DEFAULTS, 'vault_password_file', 'ANSIBLE_VAULT_PASSWORD_FILE', None)) DEFAULT_TRANSPORT = get_config(p, DEFAULTS, 'transport', 'ANSIBLE_TRANSPORT', 'smart') DEFAULT_SCP_IF_SSH = get_config(p, 'ssh_connection', 'scp_if_ssh', 'ANSIBLE_SCP_IF_SSH', False, boolean=True) +DEFAULT_SFTP_BATCH_MODE = get_config(p, 'ssh_connection', 'sftp_batch_mode', 'ANSIBLE_SFTP_BATCH_MODE', True, boolean=True) DEFAULT_MANAGED_STR = get_config(p, DEFAULTS, 'ansible_managed', None, 'Ansible managed: {file} modified on %Y-%m-%d %H:%M:%S by {uid} on {host}') DEFAULT_SYSLOG_FACILITY = get_config(p, DEFAULTS, 'syslog_facility', 'ANSIBLE_SYSLOG_FACILITY', 'LOG_USER') DEFAULT_KEEP_REMOTE_FILES = get_config(p, DEFAULTS, 'keep_remote_files', 'ANSIBLE_KEEP_REMOTE_FILES', False, boolean=True) diff --git a/lib/ansible/plugins/action/fetch.py b/lib/ansible/plugins/action/fetch.py index a239e252acd..9d62a7b9784 100644 --- a/lib/ansible/plugins/action/fetch.py +++ b/lib/ansible/plugins/action/fetch.py @@ -131,9 +131,12 @@ class ActionModule(ActionBase): if remote_data is None: self._connection.fetch_file(source, dest) else: - f = open(dest, 'w') - f.write(remote_data) - f.close() + try: + f = open(dest, 'w') + f.write(remote_data) + f.close() + except (IOError, OSError) as e: + raise AnsibleError("Failed to fetch the file: %s" % e) new_checksum = secure_hash(dest) # For backwards compatibility. We'll return None on FIPS enabled # systems diff --git a/lib/ansible/plugins/connections/local.py b/lib/ansible/plugins/connections/local.py index d77d9484a97..4bd076ccd65 100644 --- a/lib/ansible/plugins/connections/local.py +++ b/lib/ansible/plugins/connections/local.py @@ -113,11 +113,9 @@ class Connection(ConnectionBase): try: shutil.copyfile(in_path, out_path) except shutil.Error: - traceback.print_exc() raise AnsibleError("failed to copy: {0} and {1} are the same".format(in_path, out_path)) - except IOError: - traceback.print_exc() - raise AnsibleError("failed to transfer file to {0}".format(out_path)) + except IOError as e: + raise AnsibleError("failed to transfer file to {0}: {1}".format(out_path, e)) def fetch_file(self, in_path, out_path): ''' fetch a file from local to local -- for copatibility ''' diff --git a/lib/ansible/plugins/connections/ssh.py b/lib/ansible/plugins/connections/ssh.py index 4cf7d569a47..f0852e0ea55 100644 --- a/lib/ansible/plugins/connections/ssh.py +++ b/lib/ansible/plugins/connections/ssh.py @@ -472,6 +472,12 @@ class Connection(ConnectionBase): indata = None else: cmd.append('sftp') + # sftp batch mode allows us to correctly catch failed transfers, + # but can be disabled if for some reason the client side doesn't + # support the option + if C.DEFAULT_SFTP_BATCH_MODE: + cmd.append('-b') + cmd.append('-') cmd.extend(self._common_args) cmd.append(self.host) indata = "get {0} {1}\n".format(in_path, out_path)