ansible/community.general
1
0
Fork 0
mirror of https://github.com/ansible-collections/community.general.git synced 2024-09-14 20:13:21 +02:00
Code Releases Activity

the smart transport is broken by ssh retry code

Browse source 
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
This commit is contained in:
Toshio Kuratomi 2017-08-12 22:33:01 -07:00
parent ab81c6c0f3
commit 3edac559d3
2 changed files with 33 additions and 27 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

14
lib/ansible/plugins/connection/ssh.py
View file

@ -491,8 +491,7 @@ class Connection(ConnectionBase):
return b''.join(output), remainder return b''.join(output), remainder
@_ssh_retry def _bare_run(self, cmd, in_data, sudoable=True, checkrc=True):
def _run(self, cmd, in_data, sudoable=True, checkrc=True):
''' '''
Starts the command and communicates with it until it ends. Starts the command and communicates with it until it ends.
''' '''
@ -767,6 +766,13 @@ class Connection(ConnectionBase):
return (p.returncode, b_stdout, b_stderr) 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): def _file_transport_command(self, in_path, out_path, sftp_action):
# scp and sftp require square brackets for IPv6 addresses, but # scp and sftp require square brackets for IPv6 addresses, but
# accept them for hostnames and IPv4 addresses too. # accept them for hostnames and IPv4 addresses too.
@ -807,14 +813,14 @@ class Connection(ConnectionBase):
cmd = self._build_command('sftp', to_bytes(host)) 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 = u"{0} {1} {2}\n".format(sftp_action, shlex_quote(in_path), shlex_quote(out_path))
in_data = to_bytes(in_data, nonstring='passthru') 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': elif method == 'scp':
if sftp_action == 'get': if sftp_action == 'get':
cmd = self._build_command('scp', u'{0}:{1}'.format(host, shlex_quote(in_path)), out_path) cmd = self._build_command('scp', u'{0}:{1}'.format(host, shlex_quote(in_path)), out_path)
else: else:
cmd = self._build_command('scp', in_path, u'{0}:{1}'.format(host, shlex_quote(out_path))) cmd = self._build_command('scp', in_path, u'{0}:{1}'.format(host, shlex_quote(out_path)))
in_data = None 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': elif method == 'piped':
if sftp_action == 'get': if sftp_action == 'get':
# we pass sudoable=False to disable pty allocation, which # we pass sudoable=False to disable pty allocation, which

46
test/units/plugins/connection/test_ssh.py
View file

@ -198,11 +198,11 @@ class TestConnectionBaseClass(unittest.TestCase):
new_stdin = StringIO() new_stdin = StringIO()
conn = ssh.Connection(pc, new_stdin) conn = ssh.Connection(pc, new_stdin)
conn._build_command = MagicMock() conn._build_command = MagicMock()
conn._run = MagicMock() conn._bare_run = MagicMock()
mock_ospe.return_value = True mock_ospe.return_value = True
conn._build_command.return_value = 'some command to run' conn._build_command.return_value = 'some command to run'
conn._run.return_value = (0, '', '') conn._bare_run.return_value = (0, '', '')
conn.host = "some_host" conn.host = "some_host"
C.ANSIBLE_SSH_RETRIES = 9 C.ANSIBLE_SSH_RETRIES = 9
@ -212,41 +212,41 @@ class TestConnectionBaseClass(unittest.TestCase):
C.DEFAULT_SCP_IF_SSH = 'smart' 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' 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.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 # 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.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._run.side_effect = None conn._bare_run.side_effect = None
# test with C.DEFAULT_SCP_IF_SSH enabled # test with C.DEFAULT_SCP_IF_SSH enabled
C.DEFAULT_SCP_IF_SSH = True C.DEFAULT_SCP_IF_SSH = True
conn.put_file('/path/to/in/file', '/path/to/dest/file') 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.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 # test with C.DEFAULT_SCP_IF_SSH disabled
C.DEFAULT_SCP_IF_SSH = False 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' 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.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', expected_in_data = b' '.join((b'put',
to_bytes(shlex_quote('/path/to/in/file/with/unicode-fö〩')), to_bytes(shlex_quote('/path/to/in/file/with/unicode-fö〩')),
to_bytes(shlex_quote('/path/to/dest/file/with/unicode-fö〩')))) + b'\n' 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.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 # 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') self.assertRaises(AnsibleError, conn.put_file, '/path/to/bad/file', '/remote/path/to/file')
# test that a not-found path raises an error # test that a not-found path raises an error
mock_ospe.return_value = False 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') self.assertRaises(AnsibleFileNotFound, conn.put_file, '/path/to/bad/file', '/remote/path/to/file')
@patch('time.sleep') @patch('time.sleep')
@ -255,10 +255,10 @@ class TestConnectionBaseClass(unittest.TestCase):
new_stdin = StringIO() new_stdin = StringIO()
conn = ssh.Connection(pc, new_stdin) conn = ssh.Connection(pc, new_stdin)
conn._build_command = MagicMock() conn._build_command = MagicMock()
conn._run = MagicMock() conn._bare_run = MagicMock()
conn._build_command.return_value = 'some command to run' conn._build_command.return_value = 'some command to run'
conn._run.return_value = (0, '', '') conn._bare_run.return_value = (0, '', '')
conn.host = "some_host" conn.host = "some_host"
C.ANSIBLE_SSH_RETRIES = 9 C.ANSIBLE_SSH_RETRIES = 9
@ -268,36 +268,36 @@ class TestConnectionBaseClass(unittest.TestCase):
C.DEFAULT_SCP_IF_SSH = 'smart' 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' 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.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 # 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.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._run.side_effect = None conn._bare_run.side_effect = None
# test with C.DEFAULT_SCP_IF_SSH enabled # test with C.DEFAULT_SCP_IF_SSH enabled
C.DEFAULT_SCP_IF_SSH = True C.DEFAULT_SCP_IF_SSH = True
conn.fetch_file('/path/to/in/file', '/path/to/dest/file') 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.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 # test with C.DEFAULT_SCP_IF_SSH disabled
C.DEFAULT_SCP_IF_SSH = False 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' 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.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', expected_in_data = b' '.join((b'get',
to_bytes(shlex_quote('/path/to/in/file/with/unicode-fö〩')), to_bytes(shlex_quote('/path/to/in/file/with/unicode-fö〩')),
to_bytes(shlex_quote('/path/to/dest/file/with/unicode-fö〩')))) + b'\n' 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.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 # 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') self.assertRaises(AnsibleError, conn.fetch_file, '/path/to/bad/file', '/remote/path/to/file')