From 4b1f3b0d23232aa9e0262b46b0e866556f7f7b8b Mon Sep 17 00:00:00 2001 From: Chris Patterson Date: Mon, 2 Aug 2021 07:17:15 -0400 Subject: [PATCH 1/2] charmcraft: perform LXD checks before use (CRAFT-420) Invoke ensure_lxd_is_ready() interface before using LXD provider. This consolidates a couple of existing checks done elsewhere as well as a new one for ensuring the user has the necessary permissions to use LXD. Minor simplification of imports used in providers module to improve and update mocks accordingly. Example error: ``` $ /snap/bin/charmcraft pack LXD requires additional permissions. Please ensure that the user is in the 'lxd' group. (full execution logs in '/home/test1/snap/charmcraft/common/charmcraft-log-0xgcist7') ``` Signed-off-by: Chris Patterson --- charmcraft/providers.py | 51 +++++++++++++--------------- requirements-dev.txt | 8 ++--- requirements.txt | 6 ++-- tests/test_providers.py | 75 +++++++++++++++++++++++++++-------------- 4 files changed, 81 insertions(+), 59 deletions(-) diff --git a/charmcraft/providers.py b/charmcraft/providers.py index ce2c50e8d..19f06c997 100644 --- a/charmcraft/providers.py +++ b/charmcraft/providers.py @@ -27,16 +27,10 @@ from craft_providers import Executor, bases, lxd from craft_providers.actions import snap_installer -from craft_providers.lxd import LXDInstallationError -from craft_providers.lxd import installer as lxd_installer -from craft_providers.lxd.remotes import configure_buildd_image_remote from charmcraft.cmdbase import CommandError from charmcraft.config import Base -from charmcraft.env import ( - get_managed_environment_log_path, - get_managed_environment_project_path, -) +from charmcraft.env import get_managed_environment_log_path, get_managed_environment_project_path from charmcraft.utils import confirm_with_user, get_host_architecture logger = logging.getLogger(__name__) @@ -112,26 +106,29 @@ def ensure_provider_is_available() -> None: :raises CommandError: if provider is not available. """ - if is_provider_available(): - return - - if confirm_with_user( - "LXD is required, but not installed. Do you wish to install LXD " - "and configure it with the defaults?", - default=False, - ): - try: - lxd_installer.install() - except LXDInstallationError as error: + if not is_provider_available(): + if confirm_with_user( + "LXD is required, but not installed. Do you wish to install LXD " + "and configure it with the defaults?", + default=False, + ): + try: + lxd.install() + except lxd.LXDInstallationError as error: + raise CommandError( + "Failed to install LXD. Please visit https://snapcraft.io/lxd for " + "instructions on how to install the LXD snap for your distribution" + ) from error + else: raise CommandError( - "Failed to install LXD. Please visit https://snapcraft.io/lxd for " + "LXD is required, but not installed. Please visit https://snapcraft.io/lxd for " "instructions on how to install the LXD snap for your distribution" - ) from error - else: - raise CommandError( - "LXD is required, but not installed. Please visit https://snapcraft.io/lxd for " - "instructions on how to install the LXD snap for your distribution" - ) + ) + + try: + lxd.ensure_lxd_is_ready() + except lxd.LXDError as error: + raise CommandError(str(error)) def is_base_providable(base: Base) -> Tuple[bool, Union[str, None]]: @@ -216,7 +213,7 @@ def is_provider_available() -> bool: :returns: True if installed. """ - return lxd_installer.is_installed() + return lxd.is_installed() @contextlib.contextmanager @@ -250,7 +247,7 @@ def launched_environment( ) environment = get_command_environment() - image_remote = configure_buildd_image_remote() + image_remote = lxd.configure_buildd_image_remote() base_configuration = CharmcraftBuilddBaseConfiguration( alias=alias, environment=environment, hostname=instance_name ) diff --git a/requirements-dev.txt b/requirements-dev.txt index 7b2e681fd..bce8f01da 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -3,13 +3,13 @@ attrs==21.2.0 black==21.7b0 certifi==2021.5.30 cffi==1.14.6 -charset-normalizer==2.0.3 +charset-normalizer==2.0.4 click==8.0.1 coverage==5.5 craft-parts @ https://github.com/canonical/craft-parts/archive/refs/tags/v1.0-alpha.3.tar.gz -craft-providers==1.0.1 +craft-providers==1.0.2 flake8==3.9.2 -humanize==3.10.0 +humanize==3.11.0 idna==3.2 iniconfig==1.1.1 Jinja2==3.0.1 @@ -51,6 +51,6 @@ six==1.16.0 snowballstemmer==2.1.0 tabulate==0.8.9 toml==0.10.2 -tomli==1.1.0 +tomli==1.2.0 typing-extensions==3.10.0.0 urllib3==1.26.6 diff --git a/requirements.txt b/requirements.txt index 12b605059..c7c86e653 100644 --- a/requirements.txt +++ b/requirements.txt @@ -2,10 +2,10 @@ appdirs==1.4.4 attrs==21.2.0 certifi==2021.5.30 cffi==1.14.6 -charset-normalizer==2.0.3 +charset-normalizer==2.0.4 craft-parts @ https://github.com/canonical/craft-parts/archive/refs/tags/v1.0-alpha.3.tar.gz -craft-providers==1.0.1 -humanize==3.10.0 +craft-providers==1.0.2 +humanize==3.11.0 idna==3.2 Jinja2==3.0.1 jsonschema==3.2.0 diff --git a/tests/test_providers.py b/tests/test_providers.py index 4ae3a35f5..fc88aaf49 100644 --- a/tests/test_providers.py +++ b/tests/test_providers.py @@ -24,7 +24,7 @@ import pytest from craft_providers import Executor, bases from craft_providers.actions import snap_installer -from craft_providers.lxd import LXDInstallationError +from craft_providers.lxd import LXDError, LXDInstallationError from charmcraft import providers from charmcraft.cmdbase import CommandError @@ -40,7 +40,7 @@ def bypass_buildd_base_setup(monkeypatch): @pytest.fixture() def mock_configure_buildd_image_remote(): with mock.patch( - "charmcraft.providers.configure_buildd_image_remote", + "charmcraft.providers.lxd.configure_buildd_image_remote", return_value="buildd-remote", ) as mock_remote: yield mock_remote @@ -73,22 +73,30 @@ def mock_lxc(monkeypatch): @pytest.fixture -def mock_lxd(monkeypatch): - with mock.patch("charmcraft.providers.lxd", autospec=True) as mock_lxd: - yield mock_lxd +def mock_lxd_launch(monkeypatch): + with mock.patch("charmcraft.providers.lxd.launch", autospec=True) as mock_lxd_launch: + yield mock_lxd_launch + + +@pytest.fixture(autouse=True) +def mock_lxd_ensure_lxd_is_ready(): + with mock.patch( + "charmcraft.providers.lxd.ensure_lxd_is_ready", return_value=None + ) as mock_is_ready: + yield mock_is_ready @pytest.fixture(autouse=True) def mock_lxd_is_installed(): with mock.patch( - "charmcraft.providers.lxd_installer.is_installed", return_value=True + "charmcraft.providers.lxd.is_installed", return_value=True ) as mock_is_installed: yield mock_is_installed @pytest.fixture() def mock_lxd_install(): - with mock.patch("charmcraft.providers.lxd_installer.install") as mock_install: + with mock.patch("charmcraft.providers.lxd.install") as mock_install: yield mock_install @@ -354,6 +362,24 @@ def test_ensure_provider_is_available_errors_when_lxd_install_fails( assert exc_info.value.__cause__ == mock_lxd_install.side_effect +def test_ensure_provider_is_available_errors_when_lxd_not_ready( + mock_confirm_with_user, mock_lxd_is_installed, mock_lxd_install, mock_lxd_ensure_lxd_is_ready +): + mock_confirm_with_user.return_value = True + mock_lxd_is_installed.return_value = True + mock_lxd_ensure_lxd_is_ready.side_effect = LXDError( + brief="some error", details="some details", resolution="some resolution" + ) + + with pytest.raises( + CommandError, + match=re.escape("some error\nsome details\nsome resolution"), + ) as exc_info: + providers.ensure_provider_is_available() + + assert exc_info.value.__cause__ == mock_lxd_install.side_effect + + def test_get_command_environment_minimal(monkeypatch): monkeypatch.setenv("IGNORE_ME", "or-im-failing") monkeypatch.setenv("PATH", "not-using-host-path") @@ -453,9 +479,10 @@ def test_is_base_providable( @pytest.mark.parametrize("is_installed", [True, False]) -def test_is_provider_available(is_installed): - with mock.patch("charmcraft.providers.lxd_installer.is_installed", return_value=is_installed): - assert providers.is_provider_available() == is_installed +def test_is_provider_available(is_installed, mock_lxd_is_installed): + mock_lxd_is_installed.return_value = is_installed + + assert providers.is_provider_available() == is_installed @pytest.mark.parametrize( @@ -466,7 +493,7 @@ def test_launched_environment( channel, alias, mock_configure_buildd_image_remote, - mock_lxd, + mock_lxd_launch, monkeypatch, tmp_path, mock_path, @@ -491,8 +518,8 @@ def test_launched_environment( ) as instance: assert instance is not None assert mock_configure_buildd_image_remote.mock_calls == [mock.call()] - assert mock_lxd.mock_calls == [ - mock.call.launch( + assert mock_lxd_launch.mock_calls == [ + mock.call( name="charmcraft-test-charm-445566-1-2-host-arch", base_configuration=mock_base_config.return_value, image_name=channel, @@ -504,9 +531,7 @@ def test_launched_environment( project="charmcraft", remote="local", ), - mock.call.launch().mount( - host_source=mock_path, target=pathlib.Path("/root/project") - ), + mock.call().mount(host_source=mock_path, target=pathlib.Path("/root/project")), ] assert mock_base_config.mock_calls == [ call( @@ -516,16 +541,16 @@ def test_launched_environment( ) ] - mock_lxd.reset_mock() + mock_lxd_launch.reset_mock() - assert mock_lxd.mock_calls == [ - mock.call.launch().unmount_all(), - mock.call.launch().stop(), + assert mock_lxd_launch.mock_calls == [ + mock.call().unmount_all(), + mock.call().stop(), ] def test_launched_environment_unmounts_and_stops_after_error( - mock_configure_buildd_image_remote, mock_lxd, monkeypatch, tmp_path + mock_configure_buildd_image_remote, mock_lxd_launch, monkeypatch, tmp_path ): monkeypatch.setattr(providers, "get_host_architecture", lambda: "host-arch") base = Base(name="ubuntu", channel="20.04", architectures=["host-arch"]) @@ -541,10 +566,10 @@ def test_launched_environment_unmounts_and_stops_after_error( lxd_project="charmcraft", lxd_remote="local", ): - mock_lxd.reset_mock() + mock_lxd_launch.reset_mock() raise RuntimeError("this is a test") - assert mock_lxd.mock_calls == [ - mock.call.launch().unmount_all(), - mock.call.launch().stop(), + assert mock_lxd_launch.mock_calls == [ + mock.call().unmount_all(), + mock.call().stop(), ] From d7f75bb8f1d475d7fec159732def3a79aba741c3 Mon Sep 17 00:00:00 2001 From: Chris Patterson Date: Mon, 2 Aug 2021 19:01:14 -0400 Subject: [PATCH 2/2] providers: update error message wording Signed-off-by: Chris Patterson --- charmcraft/providers.py | 4 ++-- tests/test_providers.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/charmcraft/providers.py b/charmcraft/providers.py index 19f06c997..5453e1a86 100644 --- a/charmcraft/providers.py +++ b/charmcraft/providers.py @@ -116,12 +116,12 @@ def ensure_provider_is_available() -> None: lxd.install() except lxd.LXDInstallationError as error: raise CommandError( - "Failed to install LXD. Please visit https://snapcraft.io/lxd for " + "Failed to install LXD. Visit https://snapcraft.io/lxd for " "instructions on how to install the LXD snap for your distribution" ) from error else: raise CommandError( - "LXD is required, but not installed. Please visit https://snapcraft.io/lxd for " + "LXD is required, but not installed. Visit https://snapcraft.io/lxd for " "instructions on how to install the LXD snap for your distribution" ) diff --git a/tests/test_providers.py b/tests/test_providers.py index fc88aaf49..033fb0c76 100644 --- a/tests/test_providers.py +++ b/tests/test_providers.py @@ -321,7 +321,7 @@ def test_ensure_provider_is_available_errors_when_user_declines( with pytest.raises( CommandError, match=re.escape( - "LXD is required, but not installed. Please visit https://snapcraft.io/lxd for " + "LXD is required, but not installed. Visit https://snapcraft.io/lxd for " "instructions on how to install the LXD snap for your distribution" ), ): @@ -346,7 +346,7 @@ def test_ensure_provider_is_available_errors_when_lxd_install_fails( with pytest.raises( CommandError, match=re.escape( - "Failed to install LXD. Please visit https://snapcraft.io/lxd for " + "Failed to install LXD. Visit https://snapcraft.io/lxd for " "instructions on how to install the LXD snap for your distribution" ), ) as exc_info: