diff --git a/.github/workflows/tests.yaml b/.github/workflows/tests.yaml index 33f2ae18f..ac9498234 100644 --- a/.github/workflows/tests.yaml +++ b/.github/workflows/tests.yaml @@ -80,6 +80,11 @@ jobs: run: | sudo apt update sudo apt install -y python3-pip python3-setuptools python3-wheel python3-venv libapt-pkg-dev + - name: Setup LXD + uses: canonical/setup-lxd@v0.1.1 + with: + channel: latest/stable + if: ${{ runner.os == 'Linux' }} - name: Install skopeo (mac) # This is only necessary for Linux until skopeo >= 1.11 is in repos. # Once we're running on Noble, we can get skopeo from apt. diff --git a/charmcraft/services/provider.py b/charmcraft/services/provider.py index cefcb9ad9..951cca117 100644 --- a/charmcraft/services/provider.py +++ b/charmcraft/services/provider.py @@ -17,11 +17,19 @@ """Service class for creating providers.""" from __future__ import annotations +import atexit + +try: + import fcntl +except ModuleNotFoundError: # Not available on Windows. + fcntl = None # type: ignore[assignment] import os import pathlib +from typing import cast import craft_providers from craft_application import services +from craft_cli import emit from craft_providers import bases from charmcraft import env @@ -56,12 +64,42 @@ def get_base( If no cache_path is included, adds one. """ + cache_path = cast(pathlib.Path, kwargs.get("cache_path", env.get_host_shared_cache_path())) + our_lock = _maybe_lock_cache(cache_path) + # Forward the shared cache path. - if "cache_path" not in kwargs: - kwargs["cache_path"] = env.get_host_shared_cache_path() + kwargs["cache_path"] = cache_path if our_lock else None return super().get_base( base_name, instance_name=instance_name, # craft-application annotation is incorrect **kwargs, # type: ignore[arg-type] ) + + +def _maybe_lock_cache(path: pathlib.Path) -> bool: + """Lock the cache so we only have one copy of Charmcraft using it at a time.""" + if fcntl is None: # Don't lock on Windows - just don't cache. + return False + cache_lock_path = path / "charmcraft.lock" + + emit.trace("Attempting to lock the cache path") + lock_file = cache_lock_path.open("w+") + try: + # Exclusive lock, but non-blocking. + fcntl.flock(lock_file, fcntl.LOCK_EX | fcntl.LOCK_NB) + except OSError: + emit.progress( + "Shared cache locked by another process; running without cache.", permanent=True + ) + return False + else: + pid = str(os.getpid()) + lock_file.write(pid) + lock_file.flush() + os.fsync(lock_file.fileno()) + emit.trace(f"Cache path locked by this process ({pid})") + atexit.register(fcntl.flock, lock_file, fcntl.LOCK_UN) + atexit.register(cache_lock_path.unlink, missing_ok=True) + + return True diff --git a/tests/conftest.py b/tests/conftest.py index 1eeecf33d..8b66dd28f 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -109,16 +109,19 @@ def service_factory( @pytest.fixture -def default_build_plan(): +def default_build_info() -> models.BuildInfo: arch = util.get_host_architecture() - return [ - models.BuildInfo( - base=bases.BaseName("ubuntu", "22.04"), - build_on=arch, - build_for="arm64", - platform="distro-1-test64", - ) - ] + return models.BuildInfo( + base=bases.BaseName("ubuntu", "22.04"), + build_on=arch, + build_for="arm64", + platform="distro-1-test64", + ) + + +@pytest.fixture +def default_build_plan(default_build_info: models.BuildInfo): + return [default_build_info] @pytest.fixture diff --git a/tests/integration/services/conftest.py b/tests/integration/services/conftest.py index 04704da92..96f232f06 100644 --- a/tests/integration/services/conftest.py +++ b/tests/integration/services/conftest.py @@ -14,10 +14,7 @@ # # For further info, check https://github.com/canonical/charmcraft """Configuration for services integration tests.""" -import contextlib -import sys -import pyfakefs.fake_filesystem import pytest from charmcraft import services @@ -25,20 +22,10 @@ @pytest.fixture -def service_factory( - fs: pyfakefs.fake_filesystem.FakeFilesystem, fake_path, simple_charm -) -> services.CharmcraftServiceFactory: - fake_project_dir = fake_path / "project" +def service_factory(simple_charm, new_path) -> services.CharmcraftServiceFactory: + fake_project_dir = new_path / "project" fake_project_dir.mkdir() - # Allow access to the real venv library path. - # This is necessary because certifi lazy-loads the certificate file. - for python_path in sys.path: - if not python_path: - continue - with contextlib.suppress(OSError): - fs.add_real_directory(python_path) - factory = services.CharmcraftServiceFactory(app=APP_METADATA) app = Charmcraft(app=APP_METADATA, services=factory) diff --git a/tests/integration/services/test_package.py b/tests/integration/services/test_package.py index cfe9fc0b8..7f21f98b5 100644 --- a/tests/integration/services/test_package.py +++ b/tests/integration/services/test_package.py @@ -14,7 +14,6 @@ # # For further info, check https://github.com/canonical/charmcraft """Tests for package service.""" -import contextlib import datetime import pathlib @@ -28,8 +27,8 @@ @pytest.fixture -def package_service(fake_path, service_factory, default_build_plan): - fake_project_dir = fake_path +def package_service(new_path: pathlib.Path, service_factory, default_build_plan): + fake_project_dir = new_path svc = services.PackageService( app=APP_METADATA, project=service_factory.project, @@ -49,12 +48,10 @@ def package_service(fake_path, service_factory, default_build_plan): ], ) @freezegun.freeze_time(datetime.datetime(2020, 3, 14, 0, 0, 0, tzinfo=datetime.timezone.utc)) -def test_write_metadata(monkeypatch, fs, package_service, project_path): +def test_write_metadata(monkeypatch, new_path, package_service, project_path): monkeypatch.setattr(charmcraft, "__version__", "3.0-test-version") - with contextlib.suppress(FileExistsError): - fs.add_real_directory(project_path) - test_prime_dir = pathlib.Path("/prime") - fs.create_dir(test_prime_dir) + test_prime_dir = new_path / "prime" + test_prime_dir.mkdir() expected_prime_dir = project_path / "prime" project = models.CharmcraftProject.from_yaml_file(project_path / "project" / "charmcraft.yaml") @@ -75,23 +72,21 @@ def test_write_metadata(monkeypatch, fs, package_service, project_path): ], ) @freezegun.freeze_time(datetime.datetime(2020, 3, 14, 0, 0, 0, tzinfo=datetime.timezone.utc)) -def test_overwrite_metadata(monkeypatch, fs, package_service, project_path): +def test_overwrite_metadata(monkeypatch, new_path, package_service, project_path): """Test that the metadata file gets rewritten for a charm. Regression test for https://github.com/canonical/charmcraft/issues/1654 """ monkeypatch.setattr(charmcraft, "__version__", "3.0-test-version") - with contextlib.suppress(FileExistsError): - fs.add_real_directory(project_path) - test_prime_dir = pathlib.Path("/prime") - fs.create_dir(test_prime_dir) + test_prime_dir = new_path / "prime" + test_prime_dir.mkdir() expected_prime_dir = project_path / "prime" project = models.CharmcraftProject.from_yaml_file(project_path / "project" / "charmcraft.yaml") project._started_at = datetime.datetime.now(tz=datetime.timezone.utc) package_service._project = project - fs.create_file(test_prime_dir / const.METADATA_FILENAME, contents="INVALID!!") + (test_prime_dir / const.METADATA_FILENAME).write_text("INVALID!!") package_service.write_metadata(test_prime_dir) @@ -100,20 +95,18 @@ def test_overwrite_metadata(monkeypatch, fs, package_service, project_path): @freezegun.freeze_time(datetime.datetime(2020, 3, 14, 0, 0, 0, tzinfo=datetime.timezone.utc)) -def test_no_overwrite_reactive_metadata(monkeypatch, fs, package_service): +def test_no_overwrite_reactive_metadata(monkeypatch, new_path, package_service): """Test that the metadata file doesn't get overwritten for a reactive charm.. Regression test for https://github.com/canonical/charmcraft/issues/1654 """ monkeypatch.setattr(charmcraft, "__version__", "3.0-test-version") project_path = pathlib.Path(__file__).parent / "sample_projects" / "basic-reactive" - with contextlib.suppress(FileExistsError): - fs.add_real_directory(project_path) - test_prime_dir = pathlib.Path("/prime") - fs.create_dir(test_prime_dir) - test_stage_dir = pathlib.Path("/stage") - fs.create_dir(test_stage_dir) - fs.create_file(test_stage_dir / const.METADATA_FILENAME, contents="INVALID!!") + test_prime_dir = new_path / "prime" + test_prime_dir.mkdir() + test_stage_dir = new_path / "stage" + test_stage_dir.mkdir() + (test_stage_dir / const.METADATA_FILENAME).write_text("INVALID!!") project = models.CharmcraftProject.from_yaml_file(project_path / "project" / "charmcraft.yaml") project._started_at = datetime.datetime.now(tz=datetime.timezone.utc) diff --git a/tests/integration/services/test_provider.py b/tests/integration/services/test_provider.py new file mode 100644 index 000000000..31fbcb0f5 --- /dev/null +++ b/tests/integration/services/test_provider.py @@ -0,0 +1,52 @@ +# Copyright 2024 Canonical Ltd. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +# For further info, check https://github.com/canonical/charmcraft +"""Integration tests for the provider service.""" + +import pathlib +import sys + +import pytest +from craft_application.models import BuildInfo +from craft_cli.pytest_plugin import RecordingEmitter + +from charmcraft import services +from charmcraft.services.provider import _maybe_lock_cache + + +@pytest.mark.skipif(sys.platform == "win32", reason="no cache on windows") +def test_locks_cache( + service_factory: services.CharmcraftServiceFactory, + tmp_path: pathlib.Path, + default_build_info: BuildInfo, + emitter: RecordingEmitter, +): + _maybe_lock_cache(tmp_path) + assert (tmp_path / "charmcraft.lock").exists() + provider = service_factory.provider + provider_kwargs = { + "build_info": default_build_info, + "work_dir": pathlib.Path(__file__).parent, + "cache_path": tmp_path, + } + + with provider.instance(**provider_kwargs) as instance: + # Because we've already locked the cache, we shouldn't see the lockfile. + lock_test = instance.execute_run(["test", "-f", "/root/.cache/charmcraft.lock"]) + assert lock_test.returncode == 1 + + # Create a file in the cache and ensure it's not visible in the outer fs + instance.execute_run(["touch", "/root/.cache/cache_cached"]) + assert not (tmp_path / "cache_cached").exists() diff --git a/tests/unit/services/test_provider.py b/tests/unit/services/test_provider.py index b55f6c48d..a4963725c 100644 --- a/tests/unit/services/test_provider.py +++ b/tests/unit/services/test_provider.py @@ -15,12 +15,23 @@ # For further info, check https://github.com/canonical/charmcraft """Unit tests for the provider service.""" +try: + import fcntl +except ModuleNotFoundError: # Windows + fcntl = None +import functools import pathlib +import sys +from collections.abc import Iterator +from unittest import mock import pytest +from craft_cli.pytest_plugin import RecordingEmitter from craft_providers import bases from charmcraft import models, services +from charmcraft.application.main import APP_METADATA +from charmcraft.services.provider import _maybe_lock_cache @pytest.fixture @@ -42,6 +53,17 @@ def provider_service( return service_factory.provider +@pytest.fixture +def mock_register(monkeypatch) -> Iterator[mock.Mock]: + register = mock.Mock() + monkeypatch.setattr("atexit.register", register) + yield register + + # Call the exit hooks as if exiting the application. + for hook in register.mock_calls: + functools.partial(*hook.args)() + + @pytest.mark.parametrize( "base_name", [ @@ -62,6 +84,7 @@ def provider_service( bases.BaseName("almalinux", "9"), ], ) +@pytest.mark.skipif(sys.platform == "win32", reason="no cache on windows") def test_get_base_forwards_cache( monkeypatch, provider_service: services.ProviderService, @@ -76,3 +99,68 @@ def test_get_base_forwards_cache( ) assert base._cache_path == fake_path / "cache" + + +@pytest.mark.parametrize( + "base_name", + [ + bases.BaseName("ubuntu", "20.04"), + bases.BaseName("ubuntu", "22.04"), + bases.BaseName("ubuntu", "24.04"), + bases.BaseName("ubuntu", "devel"), + bases.BaseName("almalinux", "9"), + ], +) +@pytest.mark.skipif(sys.platform == "win32", reason="no cache on windows") +def test_get_base_no_cache_if_locked( + monkeypatch, + mock_register, + tmp_path: pathlib.Path, + base_name: bases.BaseName, + emitter: RecordingEmitter, +): + cache_path = tmp_path / "cache" + cache_path.mkdir(exist_ok=True, parents=True) + + # Make a new path object to work around caching the paths and thus getting the + # same file descriptor. + locked = _maybe_lock_cache(cache_path) + assert locked + new_cache_path = pathlib.Path(str(cache_path)) + monkeypatch.setattr("charmcraft.env.get_host_shared_cache_path", lambda: new_cache_path) + + # Can't use the fixture as pyfakefs doesn't handle locks. + provider_service = services.ProviderService( + app=APP_METADATA, + services=None, # pyright: ignore[reportArgumentType] + project=None, # pyright: ignore[reportArgumentType] + work_dir=tmp_path, + build_plan=[], + ) + + base = provider_service.get_base( + base_name=base_name, + instance_name="charmcraft-test-instance", + ) + + assert base._cache_path is None + emitter.assert_progress( + "Shared cache locked by another process; running without cache.", + permanent=True, + ) + + +@pytest.mark.skipif(sys.platform == "win32", reason="no cache on windows") +def test_maybe_lock_cache_locks_single_lock(tmp_path: pathlib.Path, mock_register) -> None: + assert _maybe_lock_cache(tmp_path) is True + mock_register.assert_has_calls([mock.call(fcntl.flock, mock.ANY, fcntl.LOCK_UN)]) + + +@pytest.mark.skipif(sys.platform == "win32", reason="no cache on windows") +def test_maybe_lock_cache_with_another_lock(tmp_path: pathlib.Path, mock_register) -> None: + assert _maybe_lock_cache(tmp_path) is True + assert _maybe_lock_cache(tmp_path) is False + assert mock_register.mock_calls == [ + mock.call(fcntl.flock, mock.ANY, fcntl.LOCK_UN), + mock.call(mock.ANY, missing_ok=True), + ]