From 630a0fbe740de30f4058c5d9e1713f33d3f18ce7 Mon Sep 17 00:00:00 2001 From: Redouane Kachach Date: Fri, 31 Mar 2023 09:04:10 +0200 Subject: [PATCH] mgr/cephadm: show meaningful messages when failing to execute cmds Fixes: https://tracker.ceph.com/issues/59254 Signed-off-by: Redouane Kachach (cherry picked from commit a66bdaafbc26098faf6a105c2e0109816d63ad35) --- src/pybind/mgr/cephadm/ssh.py | 37 ++++++++++++------ src/pybind/mgr/cephadm/tests/test_ssh.py | 49 ++++++++++++++++++++++++ src/pybind/mgr/tox.ini | 2 +- 3 files changed, 75 insertions(+), 13 deletions(-) diff --git a/src/pybind/mgr/cephadm/ssh.py b/src/pybind/mgr/cephadm/ssh.py index 560b8b826d5..c202bb00a4a 100644 --- a/src/pybind/mgr/cephadm/ssh.py +++ b/src/pybind/mgr/cephadm/ssh.py @@ -123,7 +123,7 @@ class SSHManager: except OSError as e: self.mgr.offline_hosts.add(host) log_content = log_string.getvalue() - msg = f"Can't communicate with remote host `{addr}`, possibly because python3 is not installed there or you are missing NOPASSWD in sudoers. {str(e)}" + msg = f"Can't communicate with remote host `{addr}`, possibly because the host is not reachable or python3 is not installed on the host. {str(e)}" logger.exception(msg) raise HostConnectionError(msg, host, addr) except asyncssh.Error as e: @@ -151,31 +151,44 @@ class SSHManager: async def _execute_command(self, host: str, - cmd: List[str], + cmd_components: List[str], stdin: Optional[str] = None, addr: Optional[str] = None, log_command: Optional[bool] = True, ) -> Tuple[str, str, int]: + conn = await self._remote_connection(host, addr) sudo_prefix = "sudo " if self.mgr.ssh_user != 'root' else "" - cmd = sudo_prefix + " ".join(quote(x) for x in cmd) + cmd = sudo_prefix + " ".join(quote(x) for x in cmd_components) + try: + address = addr or self.mgr.inventory.get_addr(host) + except Exception: + address = host if log_command: logger.debug(f'Running command: {cmd}') try: - r = await conn.run(f'{sudo_prefix}true', check=True, timeout=5) + r = await conn.run(f'{sudo_prefix}true', check=True, timeout=5) # host quick check r = await conn.run(cmd, input=stdin) - # handle these Exceptions otherwise you might get a weird error like TypeError: __init__() missing 1 required positional argument: 'reason' (due to the asyncssh error interacting with raise_if_exception) - except (asyncssh.ChannelOpenError, asyncssh.ProcessError, Exception) as e: + # handle these Exceptions otherwise you might get a weird error like + # TypeError: __init__() missing 1 required positional argument: 'reason' (due to the asyncssh error interacting with raise_if_exception) + except asyncssh.ChannelOpenError as e: # SSH connection closed or broken, will create new connection next call logger.debug(f'Connection to {host} failed. {str(e)}') await self._reset_con(host) self.mgr.offline_hosts.add(host) - if not addr: - try: - addr = self.mgr.inventory.get_addr(host) - except Exception: - addr = host - raise HostConnectionError(f'Unable to reach remote host {host}. {str(e)}', host, addr) + raise HostConnectionError(f'Unable to reach remote host {host}. {str(e)}', host, address) + except asyncssh.ProcessError as e: + msg = f"Cannot execute the command '{cmd}' on the {host}. {str(e.stderr)}." + logger.debug(msg) + await self._reset_con(host) + self.mgr.offline_hosts.add(host) + raise HostConnectionError(msg, host, address) + except Exception as e: + msg = f"Generic error while executing command '{cmd}' on the host {host}. {str(e)}." + logger.debug(msg) + await self._reset_con(host) + self.mgr.offline_hosts.add(host) + raise HostConnectionError(msg, host, address) def _rstrip(v: Union[bytes, str, None]) -> str: if not v: diff --git a/src/pybind/mgr/cephadm/tests/test_ssh.py b/src/pybind/mgr/cephadm/tests/test_ssh.py index 4197d8d7ef0..29f01b6c797 100644 --- a/src/pybind/mgr/cephadm/tests/test_ssh.py +++ b/src/pybind/mgr/cephadm/tests/test_ssh.py @@ -1,3 +1,5 @@ +import asyncssh +from asyncssh.process import SSHCompletedProcess from unittest import mock try: # AsyncMock was not added until python 3.8 @@ -19,6 +21,7 @@ from ceph.deployment.hostspec import HostSpec from cephadm import CephadmOrchestrator from cephadm.serve import CephadmServe from cephadm.tests.fixtures import with_host, wait, async_side_effect +from orchestrator import OrchestratorError @pytest.mark.skipif(ConnectionLost is None, reason='no asyncssh') @@ -49,6 +52,52 @@ class TestWithSSH: out = wait(cephadm_module, cephadm_module.get_hosts())[0].to_json() assert out == HostSpec('test', '1::4').to_json() + def test_ssh_remote_cmds_execution(self, cephadm_module): + + if not AsyncMock: + # can't run this test if we could not import AsyncMock + return + + class FakeConn: + def __init__(self, exception=None, returncode=0): + self.exception = exception + self.returncode = returncode + + async def run(self, *args, **kwargs): + if self.exception: + raise self.exception + else: + return SSHCompletedProcess(returncode=self.returncode, stdout="", stderr="") + + async def close(self): + pass + + def run_test(host, conn, expected_error): + mock_connect = AsyncMock(return_value=conn) + with pytest.raises(OrchestratorError, match=expected_error): + with mock.patch("asyncssh.connect", new=mock_connect): + with with_host(cephadm_module, host): + CephadmServe(cephadm_module)._check_host(host) + + # Test case 1: command failure + run_test('test1', FakeConn(returncode=1), "Command .+ failed") + + # Test case 2: connection error + run_test('test2', FakeConn(exception=asyncssh.ChannelOpenError(1, "", "")), "Unable to reach remote host test2.") + + # Test case 3: asyncssh ProcessError + stderr = "my-process-stderr" + run_test('test3', FakeConn(exception=asyncssh.ProcessError(returncode=3, + env="", + command="", + subsystem="", + exit_status="", + exit_signal="", + stderr=stderr, + stdout="")), f"Cannot execute the command.+{stderr}") + # Test case 4: generic error + run_test('test4', FakeConn(exception=Exception), "Generic error while executing command.+") + @pytest.mark.skipif(ConnectionLost is not None, reason='asyncssh') class TestWithoutSSH: diff --git a/src/pybind/mgr/tox.ini b/src/pybind/mgr/tox.ini index b781d1be67f..0f2fe777f05 100644 --- a/src/pybind/mgr/tox.ini +++ b/src/pybind/mgr/tox.ini @@ -59,7 +59,7 @@ setenv = UNITTEST = true PYTHONPATH = $PYTHONPATH:.. deps = - -rrequirements-required.txt + -rrequirements.txt commands = pytest {posargs:cephadm/tests/test_ssh.py} -- 2.39.5