Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: allow use of interceptable mknod in ProviderService #607

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions craft_application/services/provider.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,14 @@
class ProviderService(base.ProjectService):
"""Manager for craft_providers in an application.

:param app: Metadata about this application.
:param app: Metadata about this application
:param services: Factory class for lazy-loading service classes
:param project: The project model
:param work_dir: The working directory for parts processing
:param build_plan: The filtered build plan of platforms that are valid for
:param provider_name: The provider to use - "lxd" or "multipass"
:param install_snap: Whether to install this app's snap from the host (default True)
:param intercept_mknod: If the host can, tell LXD instance to intercept mknod
"""

managed_mode_env_var = platforms.ENVIRONMENT_CRAFT_MANAGED_MODE
Expand All @@ -68,13 +73,16 @@ def __init__(
build_plan: list[models.BuildInfo],
provider_name: str | None = None,
install_snap: bool = True,
intercept_mknod: bool = True,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and in craft-providers this is set to True by default. But according to the discussion in canonical/craft-providers#191 I understand the proposition was to set it to False by default. Is that a mistake or did we change our mind?

) -> None:
super().__init__(app, services, project=project)
self._provider: craft_providers.Provider | None = None
self._work_dir = work_dir
self._build_plan = build_plan
self.snaps: list[Snap] = []
self._install_snap = install_snap
self._intercept_mknod = intercept_mknod

self.environment: dict[str, str | None] = {self.managed_mode_env_var: "1"}
self.packages: list[str] = []
# this is a private attribute because it may not reflect the actual
Expand Down Expand Up @@ -309,7 +317,11 @@ def _get_provider_by_name(self, name: str) -> craft_providers.Provider:
def _get_lxd_provider(self) -> LXDProvider:
"""Get the LXD provider for this manager."""
lxd_remote = self._services.config.get("lxd_remote")
return LXDProvider(lxd_project=self._app.name, lxd_remote=lxd_remote)
return LXDProvider(
lxd_project=self._app.name,
lxd_remote=lxd_remote,
intercept_mknod=self._intercept_mknod,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick?: I feel like this is an implementation detail, or at least something living at a deeper abstraction level than the other arguments given to the LXDProvider class.

I do not have a good solution and I do not really know if we will ever need to gather other configuration values of this kind at some point in the future but maybe this should be in a "lxd_tuning" object?

)

def _get_multipass_provider(self) -> MultipassProvider:
"""Get the Multipass provider for this manager."""
Expand Down
3 changes: 2 additions & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ dependencies = [
"craft-grammar>=2.0.0",
"craft-parts>=2.1.1",
"craft-platforms>=0.5.0",
"craft-providers>=2.1.0",
"craft-providers @ git+https://github.com/canonical/craft-providers.git@work/CRAFT-2568-settable-mknod-intercept",
#"craft-providers>=2.1.0",
Comment on lines +12 to +13
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reminder to future self: update this once I release a new craft-providers.

"Jinja2~=3.1",
"snap-helpers>=0.4.2",
"platformdirs>=3.10",
Expand Down
2 changes: 1 addition & 1 deletion tests/unit/services/test_provider.py
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ def test_get_lxd_provider(monkeypatch, provider_service, lxd_remote, check):
check.equal(actual, mock_provider.return_value)
with check:
mock_provider.assert_called_once_with(
lxd_project="testcraft", lxd_remote=lxd_remote
lxd_project="testcraft", lxd_remote=lxd_remote, intercept_mknod=True
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you exercise the default value and setting ProviderService(intercept_mknod)?

A quick way to do this would be to instantiate a ProviderService like some of the other tests and add something along the lines of @pytest.param("intercept_mknod", [None, True, False])

)


Expand Down
Loading