From de1a2192e95dd1026302bc56d18fe8ebfa51d9f4 Mon Sep 17 00:00:00 2001 From: Nir Soffer Date: Tue, 11 Jul 2023 20:29:40 +0300 Subject: [PATCH] Better errors for missing executable When trying to run a missing executable (e.g. developer forgot to installed a required tool) we logged a generic error that makes it harder to understand and the debug the issue: $ addons/velero/start velero Deploying velero Traceback (most recent call last): File "/home/nsoffer/src/ramen/test/addons/velero/start", line 38, in deploy(cluster) File "/home/nsoffer/src/ramen/test/addons/velero/start", line 16, in deploy for line in commands.watch( File "/home/nsoffer/src/ramen/test/drenv/commands.py", line 96, in watch with subprocess.Popen( ^^^^^^^^^^^^^^^^^ File "/usr/lib64/python3.11/subprocess.py", line 1026, in __init__ self._execute_child(args, executable, preexec_fn, close_fds, File "/usr/lib64/python3.11/subprocess.py", line 1950, in _execute_child raise child_exception_type(errno_num, err_msg, err_filename) FileNotFoundError: [Errno 2] No such file or directory: 'velero' There is no information on the code code that failed inside the script, and in particular, the command we tried to run. This is caused by not handling the error when subprocess.Popen() fail to execute the child process. Now we raise the standard `commands.Error`, preserving the traceback from the underlying error. Example error with this change: $ addons/velero/start velero Deploying velero Traceback (most recent call last): File "/home/nsoffer/src/ramen/test/addons/velero/start", line 38, in deploy(cluster) File "/home/nsoffer/src/ramen/test/addons/velero/start", line 16, in deploy for line in commands.watch( File "/home/nsoffer/src/ramen/test/drenv/commands.py", line 119, in watch raise Error(args, f"Could not execute: {e}").with_exception(e) File "/home/nsoffer/src/ramen/test/drenv/commands.py", line 111, in watch p = subprocess.Popen( ^^^^^^^^^^^^^^^^^ File "/usr/lib64/python3.11/subprocess.py", line 1026, in __init__ self._execute_child(args, executable, preexec_fn, close_fds, File "/usr/lib64/python3.11/subprocess.py", line 1950, in _execute_child raise child_exception_type(errno_num, err_msg, err_filename) drenv.commands.Error: Command failed: command: ('velero', 'install', '--provider=aws', '--plugins=velero/velero-plugin-for-aws:v1.2.1', '--bucket=bucket', '--secret-file=credentials.conf', '--use-volume-snapshots=false', '--backup-location-config=region=minio,s3ForcePathStyle=true,s3Url=http://192.168.39.8:30000', '--kubecontext=velero', '--wait') error: Could not execute: [Errno 2] No such file or directory: 'velero' Signed-off-by: Nir Soffer --- test/drenv/commands.py | 65 ++++++++++++++++++++++++------------- test/drenv/commands_test.py | 34 +++++++++++++++++++ 2 files changed, 77 insertions(+), 22 deletions(-) diff --git a/test/drenv/commands.py b/test/drenv/commands.py index a0b1fa7ca..f0502cf56 100644 --- a/test/drenv/commands.py +++ b/test/drenv/commands.py @@ -20,12 +20,19 @@ class Error(Exception): INDENT = 3 * " " - def __init__(self, command, exitcode, error, output=None): + def __init__(self, command, error, exitcode=None, output=None): self.command = command - self.exitcode = exitcode self.error = error + self.exitcode = exitcode self.output = output + def with_exception(self, exc): + """ + Return a new error preserving the traceback from another excpetion. + """ + self.__cause__ = None + return self.with_traceback(exc.__traceback__) + def _indent(self, s): return textwrap.indent(s, self.INDENT) @@ -33,9 +40,11 @@ def __str__(self): lines = [ "Command failed:\n", self._indent(f"command: {self.command}\n"), - self._indent(f"exitcode: {self.exitcode}\n"), ] + if self.exitcode is not None: + lines.append(self._indent(f"exitcode: {self.exitcode}\n")) + if self.output: output = self._indent(self.output.rstrip()) lines.append(self._indent(f"output:\n{output}\n")) @@ -58,19 +67,25 @@ def run(*args, input=None): fail to raise an error about the failing command. Invalid characters will be replaced with unicode replacement character (U+FFFD). - Raises Error if the child process terminated with non-zero exit code. The - error includes all data read from the child process stdout and stderr. + Raises Error if creating a child process failed or the child process + terminated with non-zero exit code. The error includes all data read from + the child process stdout and stderr. """ - cp = subprocess.run( - args, - input=input.encode() if input else None, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - ) + try: + cp = subprocess.run( + args, + input=input.encode() if input else None, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + ) + except OSError as e: + raise Error(args, f"Could not execute: {e}").with_exception(e) + output = cp.stdout.decode() if cp.returncode != 0: error = cp.stderr.decode(errors="replace") - raise Error(args, cp.returncode, error, output=output) + raise Error(args, error, exitcode=cp.returncode, output=output) + return output @@ -86,20 +101,26 @@ def watch(*args, input=None): fail to raise an error about the failing command. Invalid characters will be replaced with unicode replacement character (U+FFFD). - Raises Error if the child process terminated with non-zero exit code. The - error includes all data read from the child process stderr. + Raises Error if creating a child process failed or the child process + terminated with non-zero exit code. The error includes all data read from + the child process stderr. """ # Avoid delays in python child process logs. env = dict(os.environ) env["PYTHONUNBUFFERED"] = "1" - with subprocess.Popen( - args, - stdin=subprocess.PIPE if input else None, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - env=env, - ) as p: + try: + p = subprocess.Popen( + args, + stdin=subprocess.PIPE if input else None, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + env=env, + ) + except OSError as e: + raise Error(args, f"Could not execute: {e}").with_exception(e) + + with p: error = bytearray() partial = bytearray() @@ -124,7 +145,7 @@ def watch(*args, input=None): if p.returncode != 0: error = error.decode(errors="replace") - raise Error(args, p.returncode, error) + raise Error(args, error, exitcode=p.returncode) def stream(proc, input=None, bufsize=32 << 10): diff --git a/test/drenv/commands_test.py b/test/drenv/commands_test.py index badfcc318..9cecae098 100644 --- a/test/drenv/commands_test.py +++ b/test/drenv/commands_test.py @@ -1,6 +1,7 @@ # SPDX-FileCopyrightText: The RamenDR authors # SPDX-License-Identifier: Apache-2.0 +import re import subprocess from contextlib import contextmanager @@ -183,6 +184,25 @@ def test_watch_no_newline(): assert output == ["first second last"] +def test_watch_error_missing_executable(): + cmd = ("no-such-executable-in-path",) + output = [] + + with pytest.raises(commands.Error) as e: + for line in commands.watch(*cmd): + output.append(line) + + assert output == [] + + assert e.value.command == cmd + assert e.value.exitcode is None + assert e.value.output is None + assert re.match( + r"Could not execute: .*'no-such-executable-in-path'.*", + e.value.error, + ) + + def test_watch_error_empty(): cmd = ("false",) output = [] @@ -248,6 +268,20 @@ def test_run_input_non_ascii(): assert output == "\u05d0" +def test_run_error_missing_executable(): + cmd = ("no-such-executable-in-path",) + with pytest.raises(commands.Error) as e: + commands.run(*cmd) + + assert e.value.command == cmd + assert e.value.exitcode is None + assert e.value.output is None + assert re.match( + r"Could not execute: .*'no-such-executable-in-path'.*", + e.value.error, + ) + + def test_run_error_empty(): cmd = ("false",) with pytest.raises(commands.Error) as e: