From c6f41daa83aaad28bf51fcfe8e0eb539fe9bc03d Mon Sep 17 00:00:00 2001 From: Callahan Date: Fri, 27 Oct 2023 20:01:45 -0500 Subject: [PATCH] fix(remote-build): parse `--launchpad-timeout` argument (#4426) Signed-off-by: Callahan Kovacs --- snapcraft/commands/remote.py | 16 ++++- snapcraft/remote/errors.py | 10 ++- snapcraft/remote/launchpad.py | 25 +++++-- snapcraft/remote/remote_builder.py | 5 +- tests/unit/commands/test_remote.py | 49 ++++++++++++++ tests/unit/remote/test_errors.py | 22 +++++-- tests/unit/remote/test_launchpad.py | 83 +++++++++++++++++++++--- tests/unit/remote/test_remote_builder.py | 6 ++ 8 files changed, 191 insertions(+), 25 deletions(-) diff --git a/snapcraft/commands/remote.py b/snapcraft/commands/remote.py index dbde5a4b61..3da9975da8 100644 --- a/snapcraft/commands/remote.py +++ b/snapcraft/commands/remote.py @@ -69,7 +69,13 @@ class RemoteBuildCommand(BaseCommand): option, followed by the build number informed when the remote build was originally dispatched. The current state of the remote build for each architecture can be checked using the - --status option.""" + --status option. + + To set a timeout on the remote-build command, use the option + ``--launchpad-timeout=``. The timeout is local, so the build on + launchpad will continue even if the local instance of snapcraft is + interrupted or times out. + """ ) @overrides @@ -101,6 +107,13 @@ def fill_parser(self, parser: argparse.ArgumentParser) -> None: action="store_true", help="acknowledge that uploaded code will be publicly available.", ) + parser.add_argument( + "--launchpad-timeout", + type=int, + default=0, + metavar="", + help="Time in seconds to wait for launchpad to build.", + ) @overrides def run(self, parsed_args: argparse.Namespace) -> None: @@ -222,6 +235,7 @@ def _run_new_remote_build(self) -> None: project_name=self._get_project_name(), architectures=self._determine_architectures(), project_dir=Path(), + timeout=self._parsed_args.launchpad_timeout, ) if self._parsed_args.status: diff --git a/snapcraft/remote/errors.py b/snapcraft/remote/errors.py index 30b2b3b97b..c8d67d46fc 100644 --- a/snapcraft/remote/errors.py +++ b/snapcraft/remote/errors.py @@ -55,10 +55,14 @@ def __init__(self, message: str) -> None: class RemoteBuildTimeoutError(RemoteBuildError): """Remote-build timed out.""" - def __init__(self) -> None: - brief = "Remote build timed out." + def __init__(self, recovery_command: str) -> None: + brief = "Remote build command timed out." + details = ( + "Build may still be running on Launchpad and can be recovered " + f"with {recovery_command!r}." + ) - super().__init__(brief=brief) + super().__init__(brief=brief, details=details) class LaunchpadHttpsError(RemoteBuildError): diff --git a/snapcraft/remote/launchpad.py b/snapcraft/remote/launchpad.py index b6bf075d0e..2d02404a84 100644 --- a/snapcraft/remote/launchpad.py +++ b/snapcraft/remote/launchpad.py @@ -79,7 +79,14 @@ def _get_url_basename(url: str): class LaunchpadClient: - """Launchpad remote builder operations.""" + """Launchpad remote builder operations. + + :param app_name: Name of the application. + :param build_id: Unique identifier for the build. + :param project_name: Name of the project. + :param architectures: List of architectures to build on. + :param timeout: Time in seconds to wait for the build to complete. + """ def __init__( self, @@ -88,7 +95,7 @@ def __init__( build_id: str, project_name: str, architectures: Sequence[str], - deadline: int = 0, + timeout: int = 0, ) -> None: self._app_name = app_name @@ -104,7 +111,11 @@ def __init__( self._lp: Launchpad = self._login() self.user = self._lp.me.name # type: ignore - self._deadline = deadline + # calculate deadline from the timeout + if timeout > 0: + self._deadline = int(time.time()) + timeout + else: + self._deadline = 0 @property def architectures(self) -> Sequence[str]: @@ -135,7 +146,11 @@ def _check_timeout_deadline(self) -> None: return if int(time.time()) >= self._deadline: - raise errors.RemoteBuildTimeoutError() + raise errors.RemoteBuildTimeoutError( + recovery_command=( + f"{self._app_name} remote-build --recover --build-id {self._build_id}" + ) + ) def _create_data_directory(self) -> Path: data_dir = Path( @@ -199,7 +214,7 @@ def _lp_load_url(self, url: str) -> Entry: return self._lp.load(url) def _wait_for_build_request_acceptance(self, build_request: Entry) -> None: - # Not be be confused with the actual build(s), this is + # Not to be confused with the actual build(s), this is # ensuring that Launchpad accepts the build request. while build_request.status == "Pending": # Check to see if we've run out of time. diff --git a/snapcraft/remote/remote_builder.py b/snapcraft/remote/remote_builder.py index dff8abde30..e32cd64775 100644 --- a/snapcraft/remote/remote_builder.py +++ b/snapcraft/remote/remote_builder.py @@ -35,19 +35,21 @@ class RemoteBuilder: :param project_name: Name of the project. :param architectures: List of architectures to build on. :param project_dir: Path of the project. + :param timeout: Time in seconds to wait for the build to complete. :raises UnsupportedArchitectureError: if any architecture is not supported for remote building. :raises LaunchpadHttpsError: If a connection to Launchpad cannot be established. """ - def __init__( + def __init__( # noqa: PLR0913 pylint: disable=too-many-arguments self, app_name: str, build_id: Optional[str], project_name: str, architectures: List[str], project_dir: Path, + timeout: int, ): self._app_name = app_name self._project_name = project_name @@ -78,6 +80,7 @@ def __init__( build_id=self._build_id, project_name=self._project_name, architectures=self._architectures, + timeout=timeout, ) @property diff --git a/tests/unit/commands/test_remote.py b/tests/unit/commands/test_remote.py index 889585bc90..bdbc36b849 100644 --- a/tests/unit/commands/test_remote.py +++ b/tests/unit/commands/test_remote.py @@ -195,6 +195,50 @@ def test_cannot_load_snapcraft_yaml(capsys): ) +@pytest.mark.parametrize( + "create_snapcraft_yaml", CURRENT_BASES | LEGACY_BASES, indirect=True +) +@pytest.mark.usefixtures( + "create_snapcraft_yaml", "mock_confirm", "use_new_remote_build", "mock_argv" +) +def test_launchpad_timeout_default(mock_remote_builder): + """Use the default timeout `0` when `--launchpad-timeout` is not provided.""" + cli.run() + + mock_remote_builder.assert_called_with( + app_name="snapcraft", + build_id=None, + project_name="mytest", + architectures=ANY, + project_dir=Path(), + timeout=0, + ) + + +@pytest.mark.parametrize( + "create_snapcraft_yaml", CURRENT_BASES | LEGACY_BASES, indirect=True +) +@pytest.mark.usefixtures( + "create_snapcraft_yaml", "mock_confirm", "use_new_remote_build" +) +def test_launchpad_timeout(mocker, mock_remote_builder): + """Pass the `--launchpad-timeout` to the remote builder.""" + mocker.patch.object( + sys, "argv", ["snapcraft", "remote-build", "--launchpad-timeout", "100"] + ) + + cli.run() + + mock_remote_builder.assert_called_with( + app_name="snapcraft", + build_id=None, + project_name="mytest", + architectures=ANY, + project_dir=Path(), + timeout=100, + ) + + ################################ # Snapcraft project base tests # ################################ @@ -537,6 +581,7 @@ def test_determine_architectures_from_snapcraft_yaml( project_name="mytest", architectures=expected_archs, project_dir=Path(), + timeout=0, ) @@ -560,6 +605,7 @@ def test_determine_architectures_host_arch(mocker, mock_remote_builder): project_name="mytest", architectures=["arm64"], project_dir=Path(), + timeout=0, ) @@ -592,6 +638,7 @@ def test_determine_architectures_provided_by_user( project_name="mytest", architectures=expected_archs, project_dir=Path(), + timeout=0, ) @@ -640,6 +687,7 @@ def test_build_id_provided(mocker, mock_remote_builder): project_name="mytest", architectures=ANY, project_dir=Path(), + timeout=0, ) @@ -660,6 +708,7 @@ def test_build_id_not_provided(mock_remote_builder): project_name="mytest", architectures=ANY, project_dir=Path(), + timeout=0, ) diff --git a/tests/unit/remote/test_errors.py b/tests/unit/remote/test_errors.py index ef65549aef..9420decaa9 100644 --- a/tests/unit/remote/test_errors.py +++ b/tests/unit/remote/test_errors.py @@ -32,14 +32,24 @@ def test_git_error(): def test_remote_build_timeout_error(): """Test RemoteBuildTimeoutError.""" - error = errors.RemoteBuildTimeoutError() + error = errors.RemoteBuildTimeoutError( + recovery_command="craftapp remote-build --recover --build-id test-id" + ) - assert str(error) == "Remote build timed out." - assert ( - repr(error) - == "RemoteBuildTimeoutError(brief='Remote build timed out.', details=None)" + assert str(error) == ( + "Remote build command timed out.\nBuild may still be running on Launchpad and " + "can be recovered with 'craftapp remote-build --recover --build-id test-id'." + ) + assert repr(error) == ( + "RemoteBuildTimeoutError(brief='Remote build command timed out.', " + 'details="Build may still be running on Launchpad and can be recovered with ' + "'craftapp remote-build --recover --build-id test-id'.\")" + ) + assert error.brief == "Remote build command timed out." + assert error.details == ( + "Build may still be running on Launchpad and can be recovered with " + "'craftapp remote-build --recover --build-id test-id'." ) - assert error.brief == "Remote build timed out." def test_launchpad_https_error(): diff --git a/tests/unit/remote/test_launchpad.py b/tests/unit/remote/test_launchpad.py index 7446477e49..42a549124f 100644 --- a/tests/unit/remote/test_launchpad.py +++ b/tests/unit/remote/test_launchpad.py @@ -353,18 +353,55 @@ def test_start_build_error(mocker, launchpad_client): assert str(raised.value) == "snapcraft.yaml not found..." -def test_start_build_error_timeout(mocker, launchpad_client): +def test_start_build_deadline_not_reached(mock_login_with, mocker): + """Do not raise an error if the deadline has not been reached.""" + + def lp_refresh(self): + """Update the status from Pending to Completed when refreshed.""" + self.status = "Completed" + mocker.patch( "tests.unit.remote.test_launchpad.SnapImpl.requestBuilds", return_value=SnapBuildReqImpl(status="Pending", error_message=""), ) + mocker.patch.object(SnapBuildReqImpl, "lp_refresh", lp_refresh) mocker.patch("time.time", return_value=500) - launchpad_client._deadline = 499 + + lpc = LaunchpadClient( + app_name="test-app", + build_id="id", + project_name="test-project", + architectures=[], + timeout=100, + ) + + lpc.start_build() + + +def test_start_build_timeout_error(mock_login_with, mocker): + """Raise an error if the build times out.""" + mocker.patch( + "tests.unit.remote.test_launchpad.SnapImpl.requestBuilds", + return_value=SnapBuildReqImpl(status="Pending", error_message=""), + ) + mocker.patch("time.time", return_value=500) + lpc = LaunchpadClient( + app_name="test-app", + build_id="id", + project_name="test-project", + architectures=[], + timeout=100, + ) + # advance 1 second past deadline + mocker.patch("time.time", return_value=601) with pytest.raises(errors.RemoteBuildTimeoutError) as raised: - launchpad_client.start_build() + lpc.start_build() - assert str(raised.value) == "Remote build timed out." + assert str(raised.value) == ( + "Remote build command timed out.\nBuild may still be running on Launchpad and " + "can be recovered with 'test-app remote-build --recover --build-id id'." + ) def test_issue_build_request_defaults(launchpad_client): @@ -422,16 +459,44 @@ def test_monitor_build_error(mock_log, mock_download_file, mocker, launchpad_cli ] -def test_monitor_build_error_timeout(mocker, launchpad_client): +def test_monitor_build_deadline_not_reached(mock_login_with, mocker): + """Do not raise an error if the deadline has not been reached.""" + mocker.patch("snapcraft.remote.LaunchpadClient._download_file") + lpc = LaunchpadClient( + app_name="test-app", + build_id="id", + project_name="test-project", + architectures=[], + timeout=100, + ) + + lpc.start_build() + lpc.monitor_build(interval=0) + + +def test_monitor_build_timeout_error(mock_login_with, mocker): + """Raise an error if the build times out.""" mocker.patch("snapcraft.remote.LaunchpadClient._download_file") mocker.patch("time.time", return_value=500) - launchpad_client._deadline = 499 - launchpad_client.start_build() + lpc = LaunchpadClient( + app_name="test-app", + build_id="id", + project_name="test-project", + architectures=[], + timeout=100, + ) + # advance 1 second past deadline + mocker.patch("time.time", return_value=601) + + lpc.start_build() with pytest.raises(errors.RemoteBuildTimeoutError) as raised: - launchpad_client.monitor_build(interval=0) + lpc.monitor_build(interval=0) - assert str(raised.value) == "Remote build timed out." + assert str(raised.value) == ( + "Remote build command timed out.\nBuild may still be running on Launchpad and " + "can be recovered with 'test-app remote-build --recover --build-id id'." + ) def test_get_build_status(launchpad_client): diff --git a/tests/unit/remote/test_remote_builder.py b/tests/unit/remote/test_remote_builder.py index d5f8590178..a78451174f 100644 --- a/tests/unit/remote/test_remote_builder.py +++ b/tests/unit/remote/test_remote_builder.py @@ -52,6 +52,7 @@ def fake_remote_builder(new_dir, mock_launchpad_client, mock_worktree): project_name="test-project", architectures=["amd64"], project_dir=Path(), + timeout=0, ) @@ -63,6 +64,7 @@ def test_remote_builder_init(mock_launchpad_client, mock_worktree): project_name="test-project", architectures=["amd64"], project_dir=Path(), + timeout=10, ) assert mock_launchpad_client.mock_calls == [ @@ -71,6 +73,7 @@ def test_remote_builder_init(mock_launchpad_client, mock_worktree): build_id="test-build-id", project_name="test-project", architectures=["amd64"], + timeout=10, ) ] assert mock_worktree.mock_calls == [ @@ -87,6 +90,7 @@ def test_build_id_computed(): project_name="test-project", architectures=["amd64"], project_dir=Path(), + timeout=0, ) assert re.match("test-app-test-project-[0-9a-f]{32}", remote_builder.build_id) @@ -101,6 +105,7 @@ def test_validate_architectures_supported(archs): project_name="test-project", architectures=archs, project_dir=Path(), + timeout=0, ) @@ -124,6 +129,7 @@ def test_validate_architectures_unsupported(archs): project_name="test-project", architectures=archs, project_dir=Path(), + timeout=0, )