Skip to content

exec_ssh_cmd: only retry commands that failed due to SSH errors #433

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
petrutlucian94 wants to merge 1 commit into
base: master
Choose a base branch
from
+60 −19
Open

exec_ssh_cmd: only retry commands that failed due to SSH errors #433

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions coriolis/exception.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,6 +509,10 @@ class MinionMachineCommandTimeout(CoriolisException):
pass


class SSHCommandFailed(CoriolisException):
pass


class SSHCommandNotFoundException(CoriolisException):
pass

Expand Down
35 changes: 23 additions & 12 deletions coriolis/tests/test_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,26 @@ def test_retry_on_error_exception_keyboard_interrupt(self):

self.assertEqual(self.mock_func.call_count, 1)

def test_retry_on_error_not_in_list_of_retried(self):
self.mock_func.side_effect = ValueError

self.assertRaises(ValueError, utils.retry_on_error(
max_attempts=5, sleep_seconds=0,
terminal_exceptions=[],
retried_exceptions=(CoriolisTestException, ))(self.mock_func))

self.assertEqual(self.mock_func.call_count, 1)

def test_retry_on_error_in_list_of_retried(self):
self.mock_func.side_effect = CoriolisTestException

self.assertRaises(CoriolisTestException, utils.retry_on_error(
max_attempts=5, sleep_seconds=0,
terminal_exceptions=[],
retried_exceptions=(CoriolisTestException, ))(self.mock_func))

self.assertEqual(self.mock_func.call_count, 5)

def test_retry_on_error_terminal_exception(self):
self.mock_func.side_effect = CoriolisTestException

Expand Down Expand Up @@ -383,10 +403,7 @@ def test_exec_ssh_cmd_exception(self):
self.mock_ssh.exec_command.return_value = (None, self.mock_stdout,
self.mock_stdout)

original_exec_ssh_cmd = testutils.get_wrapped_function(
utils.exec_ssh_cmd)

self.assertRaises(exception.CoriolisException, original_exec_ssh_cmd,
self.assertRaises(exception.SSHCommandFailed, utils.exec_ssh_cmd,
self.mock_ssh, "command")

self.mock_ssh.exec_command.assert_called_once_with(
Expand All @@ -398,23 +415,17 @@ def test_exec_ssh_cmd_command_not_found_in_stdout(self):
self.mock_ssh.exec_command.return_value = (None, self.mock_stdout,
self.mock_stdout)

original_exec_ssh_cmd = testutils.get_wrapped_function(
utils.exec_ssh_cmd)

self.assertRaises(exception.SSHCommandNotFoundException,
original_exec_ssh_cmd, self.mock_ssh, "command")
utils.exec_ssh_cmd, self.mock_ssh, "command")

def test_exec_ssh_cmd_exit_code_127(self):
self.mock_stdout.read.return_value = b''
self.mock_stdout.channel.recv_exit_status.return_value = 127
self.mock_ssh.exec_command.return_value = (None, self.mock_stdout,
self.mock_stdout)

original_exec_ssh_cmd = testutils.get_wrapped_function(
utils.exec_ssh_cmd)

self.assertRaises(exception.SSHCommandNotFoundException,
original_exec_ssh_cmd, self.mock_ssh, "command")
utils.exec_ssh_cmd, self.mock_ssh, "command")

def test_exec_ssh_cmd_chroot(self):
self.mock_stdout.read.return_value = b'output\n'
Expand Down
40 changes: 33 additions & 7 deletions coriolis/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,8 @@ def get_single_result(lis):


def retry_on_error(max_attempts=5, sleep_seconds=1,
terminal_exceptions=[]):
terminal_exceptions=[],
retried_exceptions=(Exception, )):
def _retry_on_error(func):
@functools.wraps(func)
def _exec_retry(*args, **kwargs):
Expand All @@ -180,7 +181,7 @@ def _exec_retry(*args, **kwargs):
LOG.debug("Got a KeyboardInterrupt, skip retrying")
LOG.exception(ex)
raise
except Exception as ex:
except retried_exceptions as ex:
Copy link
Copy Markdown
Contributor

@Dany9966 Dany9966 May 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it makes sense to keep both terminal_exceptions and retried_exceptions for this method. We should go with one of them.
I also think that this is unnecessary. Why not just add SSHCommandFailed to terminal_exceptions for _exec_ssh_cmd?

Copy link
Copy Markdown
Member Author

@petrutlucian94 petrutlucian94 May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's meant to be flexible, allowing us to set a broader class of exceptions through retried_exceptions and a few potential exception subclasses that shouldn't be retried, specified through terminal_exceptions.

There's a comment saying that:

# By default, we'll perform retires only in case of SSH failures.
#
# Add "SSHCommandFailed" to the list of retried exceptions if failed
# commands should be retried as well.

The default list of terminal exceptions ensures that we won't retry missing commands for example.

Copy link
Copy Markdown
Member Author

@petrutlucian94 petrutlucian94 May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just realized that there's a typo, fixing it :).

if any([isinstance(ex, tex)
for tex in terminal_exceptions]):
raise
Expand Down Expand Up @@ -314,10 +315,7 @@ def list_ssh_dir(ssh, remote_path):
return [f for f in output.splitlines() if f.strip()]


@retry_on_error(terminal_exceptions=[
exception.MinionMachineCommandTimeout,
exception.SSHCommandNotFoundException])
def exec_ssh_cmd(ssh, cmd, environment=None, get_pty=False, timeout=None):
def _exec_ssh_cmd(ssh, cmd, environment=None, get_pty=False, timeout=None):
remote_str = "<undeterminable>"
if timeout is not None:
timeout = float(timeout)
Expand Down Expand Up @@ -350,7 +348,7 @@ def exec_ssh_cmd(ssh, cmd, environment=None, get_pty=False, timeout=None):
"command not found" in stdout_str or
"command not found" in stderr_str):
raise exception.SSHCommandNotFoundException(msg)
raise exception.CoriolisException(msg)
raise exception.SSHCommandFailed(msg)
# Most of the commands will use pseudo-terminal which unfortunately will
# include a '\r' to every newline. This will affect all plugins too, so
# best we can do now is replace them.
Expand All @@ -360,6 +358,34 @@ def exec_ssh_cmd(ssh, cmd, environment=None, get_pty=False, timeout=None):
return std_out.decode('utf-8', errors='replace')


def exec_ssh_cmd(
ssh,
cmd,
environment=None,
get_pty=False,
timeout=None,
max_attempts=5,
retry_interval=1,
terminal_exceptions=(
exception.MinionMachineCommandTimeout,
exception.SSHCommandNotFoundException,
),
retried_exceptions=(paramiko.ssh_exception.SSHException,),
):
# By default, we'll perform retries only in case of SSH failures.
#
# Add "SSHCommandFailed" to the list of retried exceptions if failed
# commands should be retried as well.

@retry_on_error(max_attempts=max_attempts, sleep_seconds=retry_interval,
terminal_exceptions=terminal_exceptions,
retried_exceptions=retried_exceptions)
def wrapper():
return _exec_ssh_cmd(ssh, cmd, environment, get_pty, timeout)

return wrapper()


def exec_ssh_cmd_chroot(ssh, chroot_dir, cmd, environment=None, get_pty=False,
timeout=None):
return exec_ssh_cmd(ssh, "sudo -E chroot %s %s" % (chroot_dir, cmd),
Expand Down
Loading