From f555d6800e740e7d3968a915599b0056bc10bc37 Mon Sep 17 00:00:00 2001 From: Olivier Gayot Date: Thu, 18 Jan 2024 12:33:44 +0100 Subject: [PATCH] apport: show name of curtin stage when an error occurs When an error occurs during the execution of a "$ curtin install" invocation, the associated bug report would be titled: "install failed crashed with CalledProcessError" This is not very user friendly and it does not immediately give any insight on: With the number of reports that we receive these days, I think classifying these reports would make our life a bit easier, and possibly make the users understand better what's going on. This change adds a wat to detect if a bug report is the result of a "$ curtin install" invocation failing. If it is, subiquity sets the title of the bug report accordingly. Examples: * "partitioning crashed with CurtinInstallError" * "extract crashed with CurtinInstallError" * "curthooks crashed with CurtinInstallError" Signed-off-by: Olivier Gayot --- subiquity/server/controllers/install.py | 48 +++++++++++++------ .../server/controllers/tests/test_install.py | 43 ++++++++++++++++- 2 files changed, 76 insertions(+), 15 deletions(-) diff --git a/subiquity/server/controllers/install.py b/subiquity/server/controllers/install.py index 87dc1b59..3344e037 100644 --- a/subiquity/server/controllers/install.py +++ b/subiquity/server/controllers/install.py @@ -55,6 +55,12 @@ from subiquitycore.utils import arun_command, log_process_streams log = logging.getLogger("subiquity.server.controllers.install") +class CurtinInstallError(Exception): + def __init__(self, *, stages: List[str]) -> None: + super().__init__() + self.stages = stages + + class TracebackExtractor: start_marker = re.compile(r"^Traceback \(most recent call last\):") end_marker = re.compile(r"\S") @@ -195,6 +201,17 @@ class InstallController(SubiquityController): keyboard = self.app.controllers.Keyboard await keyboard.setup_target(context=context) + @staticmethod + def error_in_curtin_invocation(exc: Exception) -> Optional[str]: + """If the exception passed as an argument corresponds to an error + during the invocation of a single curtin stage, return the name of the + stage. Otherwise, return None.""" + if not isinstance(exc, CurtinInstallError): + return None + if len(exc.stages) != 1: + return None + return exc.stages[0] + @with_context(description="executing curtin install {name} step") async def run_curtin_step( self, @@ -226,16 +243,19 @@ class InstallController(SubiquityController): else: source_args = () - await run_curtin_command( - self.app, - context, - "install", - "--set", - f"json:stages={json.dumps(stages)}", - *source_args, - config=str(config_file), - private_mounts=False, - ) + try: + await run_curtin_command( + self.app, + context, + "install", + "--set", + f"json:stages={json.dumps(stages)}", + *source_args, + config=str(config_file), + private_mounts=False, + ) + except subprocess.CalledProcessError: + raise CurtinInstallError(stages=stages) device_map_path = config.get("storage", {}).get("device_map_path") if device_map_path is not None: @@ -673,13 +693,13 @@ class InstallController(SubiquityController): await self.postinstall(context=context) self.app.update_state(ApplicationState.DONE) - except Exception: + except Exception as exc: kw = {} if self.tb_extractor.traceback: kw["Traceback"] = "\n".join(self.tb_extractor.traceback) - self.app.make_apport_report( - ErrorReportKind.INSTALL_FAIL, "install failed", **kw - ) + text = self.error_in_curtin_invocation(exc) or "install failed" + + self.app.make_apport_report(ErrorReportKind.INSTALL_FAIL, text, **kw) raise async def platform_postinstall(self): diff --git a/subiquity/server/controllers/tests/test_install.py b/subiquity/server/controllers/tests/test_install.py index e0048b44..7810819a 100644 --- a/subiquity/server/controllers/tests/test_install.py +++ b/subiquity/server/controllers/tests/test_install.py @@ -25,7 +25,7 @@ from curtin.util import EFIBootEntry, EFIBootState from subiquity.common.types import PackageInstallState from subiquity.models.tests.test_filesystem import make_model_and_partition -from subiquity.server.controllers.install import InstallController +from subiquity.server.controllers.install import CurtinInstallError, InstallController from subiquitycore.tests.mocks import make_app @@ -88,6 +88,31 @@ class TestWriteConfig(unittest.IsolatedAsyncioTestCase): private_mounts=False, ) + @patch("subiquity.server.controllers.install.open", mock_open()) + async def test_run_curtin_install_step_failed(self): + cmd = ["curtin", "install", "--set", 'json:stages=["partitioning"]'] + stages = ["partitioning"] + + async def fake_run_curtin_command(*args, **kwargs): + raise subprocess.CalledProcessError(returncode=1, cmd=cmd) + + with patch( + "subiquity.server.controllers.install.run_curtin_command", + fake_run_curtin_command, + ): + with self.assertRaises(CurtinInstallError) as exc_cm: + await self.controller.run_curtin_step( + name="MyStep", + stages=stages, + config_file=Path("/config.yaml"), + source=None, + config=self.controller.base_config( + logs_dir=Path("/"), resume_data_file=Path("resume-data") + ), + ) + self.assertEqual(stages, exc_cm.exception.stages) + self.assertEqual(cmd, exc_cm.exception.__context__.cmd) + def test_base_config(self): config = self.controller.base_config( logs_dir=Path("/logs"), resume_data_file=Path("resume-data") @@ -397,3 +422,19 @@ class TestInstallController(unittest.IsolatedAsyncioTestCase): "https://canonical-subiquity.readthedocs-hosted.com/en/latest/reference/autoinstall-reference.html", # noqa: E501 data, ) + + def test_error_in_curtin_invocation(self): + method = self.controller.error_in_curtin_invocation + + self.assertIsNone(method(Exception())) + self.assertIsNone(method(RuntimeError())) + + self.assertIsNone(method(CurtinInstallError(stages=[]))) + # Running multiple stages in one "curtin step" is not something that + # currently happens in practice. + self.assertIsNone( + method(CurtinInstallError(stages=["extract", "partitioning"])) + ) + + self.assertEqual("extract", method(CurtinInstallError(stages=["extract"]))) + self.assertEqual("curthooks", method(CurtinInstallError(stages=["curthooks"])))