From 24de248cec9c4687c3fea856ed030fcddb860b75 Mon Sep 17 00:00:00 2001 From: Chris Peterson Date: Thu, 1 Feb 2024 10:32:00 -0800 Subject: [PATCH] AutoinstallError: Disable apport reporting Autoinstall related failures are more likely than not going to be user caused, so we shouldn't immediately generate a crash report for these types of failures. This should hopefully allow the user to debug their autoinstall data much faster and reduce the number of autoinstall-related bugs reported. --- subiquity/server/autoinstall.py | 31 +++++++++++++++++++++++ subiquity/server/server.py | 11 ++++---- subiquity/server/tests/test_controller.py | 7 ++++- subiquity/server/tests/test_server.py | 15 +++++++++++ subiquitycore/tests/mocks.py | 1 + 5 files changed, 59 insertions(+), 6 deletions(-) create mode 100644 subiquity/server/autoinstall.py diff --git a/subiquity/server/autoinstall.py b/subiquity/server/autoinstall.py new file mode 100644 index 00000000..0abc3dd3 --- /dev/null +++ b/subiquity/server/autoinstall.py @@ -0,0 +1,31 @@ +# Copyright 2024 Canonical, Ltd. +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU Affero General Public License as +# published by the Free Software Foundation, either version 3 of the +# License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU Affero General Public License for more details. +# +# You should have received a copy of the GNU Affero General Public License +# along with this program. If not, see . +import logging + +log = logging.getLogger("subiquity.server.autoinstall") + + +class AutoinstallError(Exception): + pass + + +class AutoinstallValidationError(AutoinstallError): + def __init__( + self, + owner: str, + ): + self.message = f"Malformed autoinstall in {owner!r} section" + self.owner = owner + super().__init__(self.message) diff --git a/subiquity/server/server.py b/subiquity/server/server.py index 1aa949be..75831878 100644 --- a/subiquity/server/server.py +++ b/subiquity/server/server.py @@ -30,7 +30,7 @@ from systemd import journal from subiquity.cloudinit import get_host_combined_cloud_config from subiquity.common.api.server import bind, controller_for_request from subiquity.common.apidef import API -from subiquity.common.errorreport import ErrorReporter, ErrorReportKind +from subiquity.common.errorreport import ErrorReport, ErrorReporter, ErrorReportKind from subiquity.common.serialize import to_json from subiquity.common.types import ( ApplicationState, @@ -41,7 +41,7 @@ from subiquity.common.types import ( PasswordKind, ) from subiquity.models.subiquity import ModelNames, SubiquityModel -from subiquity.server.autoinstall import AutoinstallValidationError +from subiquity.server.autoinstall import AutoinstallError, AutoinstallValidationError from subiquity.server.controller import SubiquityController from subiquity.server.dryrun import DRConfig from subiquity.server.errors import ErrorController @@ -403,8 +403,9 @@ class SubiquityServer(Application): def make_apport_report(self, kind, thing, *, wait=False, **kw): return self.error_reporter.make_apport_report(kind, thing, wait=wait, **kw) - async def _run_error_cmds(self, report): - await report._info_task + async def _run_error_cmds(self, report: Optional[ErrorReport] = None) -> None: + if report is not None and report._info_task is not None: + await report._info_task Error = getattr(self.controllers, "Error", None) if Error is not None and Error.cmds: try: @@ -421,7 +422,7 @@ class SubiquityServer(Application): return report = self.error_reporter.report_for_exc(exc) log.error("top level error", exc_info=exc) - if not report: + if not isinstance(exc, AutoinstallError) and not report: report = self.make_apport_report( ErrorReportKind.UNKNOWN, "unknown error", exc=exc ) diff --git a/subiquity/server/tests/test_controller.py b/subiquity/server/tests/test_controller.py index bf73c453..89592aa7 100644 --- a/subiquity/server/tests/test_controller.py +++ b/subiquity/server/tests/test_controller.py @@ -61,7 +61,7 @@ class TestController(SubiTestCase): mock_load.assert_called_once_with("default-data") def test_autoinstall_validation(self): - """Test validation error type""" + """Test validation error type and no apport reporting""" self.controller.autoinstall_schema = { "type": "object", @@ -84,3 +84,8 @@ class TestController(SubiTestCase): # Assert error section is based on autoinstall_key self.assertEquals(exception.owner, "some-key") + + # Assert apport report is not created + # This only checks that controllers do not manually create an apport + # report on validation. Should also be tested in Server + self.controller.app.make_apport_report.assert_not_called() diff --git a/subiquity/server/tests/test_server.py b/subiquity/server/tests/test_server.py index ae380bf2..ce1eb1b0 100644 --- a/subiquity/server/tests/test_server.py +++ b/subiquity/server/tests/test_server.py @@ -160,6 +160,7 @@ class TestAutoinstallValidation(SubiTestCase): }, }, } + self.server.make_apport_report = Mock() def test_valid_schema(self): """Test that the expected autoinstall JSON schema is valid""" @@ -179,6 +180,20 @@ class TestAutoinstallValidation(SubiTestCase): with self.assertRaises(AutoinstallValidationError): self.server.validate_autoinstall() + async def test_autoinstall_validation__no_error_report(self): + """Test no apport reporting""" + + exception = AutoinstallValidationError("Mock") + + loop = Mock() + context = {"exception": exception} + + with patch("subiquity.server.server.log"): + with patch.object(self.server, "_run_error_cmds"): + self.server._exception_handler(loop, context) + + self.server.make_apport_report.assert_not_called() + class TestMetaController(SubiTestCase): async def test_interactive_sections_not_present(self): diff --git a/subiquitycore/tests/mocks.py b/subiquitycore/tests/mocks.py index 89456ac6..748b97b5 100644 --- a/subiquitycore/tests/mocks.py +++ b/subiquitycore/tests/mocks.py @@ -50,5 +50,6 @@ def make_app(model=None): app.log_syslog_id = None app.report_start_event = mock.Mock() app.report_finish_event = mock.Mock() + app.make_apport_report = mock.Mock() return app