From 3edac559d3b74848fb7b61879592338a890c9958 Mon Sep 17 00:00:00 2001 From: Toshio Kuratomi Date: Sat, 12 Aug 2017 22:33:01 -0700 Subject: [PATCH] the smart transport is broken by ssh retry code 1fe67f9 introduced retries to the ssh connection put file and fetch file. Unfortunately, that change broke the smart transport because it started raising exceptions instead of returning from _run(). This breakage is documented in #23711. An attempt to fix it was made at #23717 but the first attempt was objected to as needing to touch too much code. The second attmept was objected to as smart was forced to encapsulate retries (thus retrying a sftp "rety" times before trying scp "retry" times and then finally moving onto piped). This third attempt has retries encapsulate smart. So each sub-transport is tried once and if all three fail, another retry attempt is made which tries each of the three again. Fixes #23711 Fixes #23717 --- lib/ansible/plugins/connection/ssh.py | 14 +++++-- test/units/plugins/connection/test_ssh.py | 46 +++++++++++------------ 2 files changed, 33 insertions(+), 27 deletions(-) diff --git a/lib/ansible/plugins/connection/ssh.py b/lib/ansible/plugins/connection/ssh.py index 1339af7449..b526905ff3 100644 --- a/lib/ansible/plugins/connection/ssh.py +++ b/lib/ansible/plugins/connection/ssh.py @@ -491,8 +491,7 @@ class Connection(ConnectionBase): return b''.join(output), remainder - @_ssh_retry - def _run(self, cmd, in_data, sudoable=True, checkrc=True): + def _bare_run(self, cmd, in_data, sudoable=True, checkrc=True): ''' Starts the command and communicates with it until it ends. ''' @@ -767,6 +766,13 @@ class Connection(ConnectionBase): return (p.returncode, b_stdout, b_stderr) + @_ssh_retry + def _run(self, cmd, in_data, sudoable=True, checkrc=True): + """Wrapper around _bare_run that retries the connection + """ + return self._bare_run(cmd, in_data, sudoable, checkrc) + + @_ssh_retry def _file_transport_command(self, in_path, out_path, sftp_action): # scp and sftp require square brackets for IPv6 addresses, but # accept them for hostnames and IPv4 addresses too. @@ -807,14 +813,14 @@ class Connection(ConnectionBase): cmd = self._build_command('sftp', to_bytes(host)) in_data = u"{0} {1} {2}\n".format(sftp_action, shlex_quote(in_path), shlex_quote(out_path)) in_data = to_bytes(in_data, nonstring='passthru') - (returncode, stdout, stderr) = self._run(cmd, in_data, checkrc=False) + (returncode, stdout, stderr) = self._bare_run(cmd, in_data, checkrc=False) elif method == 'scp': if sftp_action == 'get': cmd = self._build_command('scp', u'{0}:{1}'.format(host, shlex_quote(in_path)), out_path) else: cmd = self._build_command('scp', in_path, u'{0}:{1}'.format(host, shlex_quote(out_path))) in_data = None - (returncode, stdout, stderr) = self._run(cmd, in_data, checkrc=False) + (returncode, stdout, stderr) = self._bare_run(cmd, in_data, checkrc=False) elif method == 'piped': if sftp_action == 'get': # we pass sudoable=False to disable pty allocation, which diff --git a/test/units/plugins/connection/test_ssh.py b/test/units/plugins/connection/test_ssh.py index ef7632d579..2ca4d93a8e 100644 --- a/test/units/plugins/connection/test_ssh.py +++ b/test/units/plugins/connection/test_ssh.py @@ -198,11 +198,11 @@ class TestConnectionBaseClass(unittest.TestCase): new_stdin = StringIO() conn = ssh.Connection(pc, new_stdin) conn._build_command = MagicMock() - conn._run = MagicMock() + conn._bare_run = MagicMock() mock_ospe.return_value = True conn._build_command.return_value = 'some command to run' - conn._run.return_value = (0, '', '') + conn._bare_run.return_value = (0, '', '') conn.host = "some_host" C.ANSIBLE_SSH_RETRIES = 9 @@ -212,41 +212,41 @@ class TestConnectionBaseClass(unittest.TestCase): C.DEFAULT_SCP_IF_SSH = 'smart' expected_in_data = b' '.join((b'put', to_bytes(shlex_quote('/path/to/in/file')), to_bytes(shlex_quote('/path/to/dest/file')))) + b'\n' conn.put_file('/path/to/in/file', '/path/to/dest/file') - conn._run.assert_called_with('some command to run', expected_in_data, checkrc=False) + conn._bare_run.assert_called_with('some command to run', expected_in_data, checkrc=False) # Test when SFTP doesn't work but SCP does - conn._run.side_effect = [(1, 'stdout', 'some errors'), (0, '', '')] + conn._bare_run.side_effect = [(1, 'stdout', 'some errors'), (0, '', '')] conn.put_file('/path/to/in/file', '/path/to/dest/file') - conn._run.assert_called_with('some command to run', None, checkrc=False) - conn._run.side_effect = None + conn._bare_run.assert_called_with('some command to run', None, checkrc=False) + conn._bare_run.side_effect = None # test with C.DEFAULT_SCP_IF_SSH enabled C.DEFAULT_SCP_IF_SSH = True conn.put_file('/path/to/in/file', '/path/to/dest/file') - conn._run.assert_called_with('some command to run', None, checkrc=False) + conn._bare_run.assert_called_with('some command to run', None, checkrc=False) conn.put_file(u'/path/to/in/file/with/unicode-fö〩', u'/path/to/dest/file/with/unicode-fö〩') - conn._run.assert_called_with('some command to run', None, checkrc=False) + conn._bare_run.assert_called_with('some command to run', None, checkrc=False) # test with C.DEFAULT_SCP_IF_SSH disabled C.DEFAULT_SCP_IF_SSH = False expected_in_data = b' '.join((b'put', to_bytes(shlex_quote('/path/to/in/file')), to_bytes(shlex_quote('/path/to/dest/file')))) + b'\n' conn.put_file('/path/to/in/file', '/path/to/dest/file') - conn._run.assert_called_with('some command to run', expected_in_data, checkrc=False) + conn._bare_run.assert_called_with('some command to run', expected_in_data, checkrc=False) expected_in_data = b' '.join((b'put', to_bytes(shlex_quote('/path/to/in/file/with/unicode-fö〩')), to_bytes(shlex_quote('/path/to/dest/file/with/unicode-fö〩')))) + b'\n' conn.put_file(u'/path/to/in/file/with/unicode-fö〩', u'/path/to/dest/file/with/unicode-fö〩') - conn._run.assert_called_with('some command to run', expected_in_data, checkrc=False) + conn._bare_run.assert_called_with('some command to run', expected_in_data, checkrc=False) # test that a non-zero rc raises an error - conn._run.return_value = (1, 'stdout', 'some errors') + conn._bare_run.return_value = (1, 'stdout', 'some errors') self.assertRaises(AnsibleError, conn.put_file, '/path/to/bad/file', '/remote/path/to/file') # test that a not-found path raises an error mock_ospe.return_value = False - conn._run.return_value = (0, 'stdout', '') + conn._bare_run.return_value = (0, 'stdout', '') self.assertRaises(AnsibleFileNotFound, conn.put_file, '/path/to/bad/file', '/remote/path/to/file') @patch('time.sleep') @@ -255,10 +255,10 @@ class TestConnectionBaseClass(unittest.TestCase): new_stdin = StringIO() conn = ssh.Connection(pc, new_stdin) conn._build_command = MagicMock() - conn._run = MagicMock() + conn._bare_run = MagicMock() conn._build_command.return_value = 'some command to run' - conn._run.return_value = (0, '', '') + conn._bare_run.return_value = (0, '', '') conn.host = "some_host" C.ANSIBLE_SSH_RETRIES = 9 @@ -268,36 +268,36 @@ class TestConnectionBaseClass(unittest.TestCase): C.DEFAULT_SCP_IF_SSH = 'smart' expected_in_data = b' '.join((b'get', to_bytes(shlex_quote('/path/to/in/file')), to_bytes(shlex_quote('/path/to/dest/file')))) + b'\n' conn.fetch_file('/path/to/in/file', '/path/to/dest/file') - conn._run.assert_called_with('some command to run', expected_in_data, checkrc=False) + conn._bare_run.assert_called_with('some command to run', expected_in_data, checkrc=False) # Test when SFTP doesn't work but SCP does - conn._run.side_effect = [(1, 'stdout', 'some errors'), (0, '', '')] + conn._bare_run.side_effect = [(1, 'stdout', 'some errors'), (0, '', '')] conn.fetch_file('/path/to/in/file', '/path/to/dest/file') - conn._run.assert_called_with('some command to run', None, checkrc=False) - conn._run.side_effect = None + conn._bare_run.assert_called_with('some command to run', None, checkrc=False) + conn._bare_run.side_effect = None # test with C.DEFAULT_SCP_IF_SSH enabled C.DEFAULT_SCP_IF_SSH = True conn.fetch_file('/path/to/in/file', '/path/to/dest/file') - conn._run.assert_called_with('some command to run', None, checkrc=False) + conn._bare_run.assert_called_with('some command to run', None, checkrc=False) conn.fetch_file(u'/path/to/in/file/with/unicode-fö〩', u'/path/to/dest/file/with/unicode-fö〩') - conn._run.assert_called_with('some command to run', None, checkrc=False) + conn._bare_run.assert_called_with('some command to run', None, checkrc=False) # test with C.DEFAULT_SCP_IF_SSH disabled C.DEFAULT_SCP_IF_SSH = False expected_in_data = b' '.join((b'get', to_bytes(shlex_quote('/path/to/in/file')), to_bytes(shlex_quote('/path/to/dest/file')))) + b'\n' conn.fetch_file('/path/to/in/file', '/path/to/dest/file') - conn._run.assert_called_with('some command to run', expected_in_data, checkrc=False) + conn._bare_run.assert_called_with('some command to run', expected_in_data, checkrc=False) expected_in_data = b' '.join((b'get', to_bytes(shlex_quote('/path/to/in/file/with/unicode-fö〩')), to_bytes(shlex_quote('/path/to/dest/file/with/unicode-fö〩')))) + b'\n' conn.fetch_file(u'/path/to/in/file/with/unicode-fö〩', u'/path/to/dest/file/with/unicode-fö〩') - conn._run.assert_called_with('some command to run', expected_in_data, checkrc=False) + conn._bare_run.assert_called_with('some command to run', expected_in_data, checkrc=False) # test that a non-zero rc raises an error - conn._run.return_value = (1, 'stdout', 'some errors') + conn._bare_run.return_value = (1, 'stdout', 'some errors') self.assertRaises(AnsibleError, conn.fetch_file, '/path/to/bad/file', '/remote/path/to/file')