From c09a32c5bbdb06b9c922513d458c66e51640e9b4 Mon Sep 17 00:00:00 2001 From: Olivier Gayot Date: Mon, 4 Sep 2023 11:28:49 +0200 Subject: [PATCH 1/2] network: fix Wi-Fi interfaces not listed in dry-run When a Wi-Fi interface is present in the machine configuration (e.g., mwhudson.json), the GUI seemingly ignores it. This happens because there is a filter on the server side which only returns Wi-Fi interfaces if the wlan_support_install_state() function returns PackageInstallState.DONE. However, calling the /network endpoint shows that the state is set to the wrong value: {"wlan_support_install_state": "NOT_NEEDED"} This turns out to be inconsistent because: * we lean on a PackageInstaller instance to tell if wpasupplicant is installed (this is what the wlan_support_install_state() function reflects) ; but * in dry-run mode, we pretend to install wpasupplicant without actually relying on the PackageInstaller instance. Fixed by using the PackageInstaller instance to install the wpasupplicant package - with a special implementation that only pretends to install it. This is enough to make the PackageInstaller instance think the package is installed. Signed-off-by: Olivier Gayot --- subiquity/server/controllers/network.py | 22 +++-- subiquity/server/pkghelper.py | 23 ++++-- subiquity/server/tests/test_pkghelper.py | 101 +++++++++++++++++++++++ 3 files changed, 134 insertions(+), 12 deletions(-) create mode 100644 subiquity/server/tests/test_pkghelper.py diff --git a/subiquity/server/controllers/network.py b/subiquity/server/controllers/network.py index 463c8131..455d6337 100644 --- a/subiquity/server/controllers/network.py +++ b/subiquity/server/controllers/network.py @@ -109,14 +109,22 @@ class NetworkController(BaseNetworkController, SubiquityController): async def _install_wpasupplicant(self): if self.opts.dry_run: - await asyncio.sleep(10 / self.app.scale_factor) - a = "DONE" - for k in self.app.debug_flags: - if k.startswith("wlan_install="): - a = k.split("=", 2)[1] - r = getattr(PackageInstallState, a) + + async def fake_install(pkgname: str) -> PackageInstallState: + await asyncio.sleep(10 / self.app.scale_factor) + a = "DONE" + for k in self.app.debug_flags: + if k.startswith("wlan_install="): + a = k.split("=", 2)[1] + return getattr(PackageInstallState, a) + + install_coro = fake_install else: - r = await self.app.package_installer.install_pkg("wpasupplicant") + install_coro = None + + r = await self.app.package_installer.install_pkg( + "wpasupplicant", install_coro=install_coro + ) log.debug("wlan_support_install_finished %s", r) self._call_clients("wlan_support_install_finished", r) if r == PackageInstallState.DONE: diff --git a/subiquity/server/pkghelper.py b/subiquity/server/pkghelper.py index d27521d5..5e5828e4 100644 --- a/subiquity/server/pkghelper.py +++ b/subiquity/server/pkghelper.py @@ -16,7 +16,7 @@ import asyncio import logging import os -from typing import Dict, Optional +from typing import Callable, Dict, Optional import apt @@ -26,6 +26,9 @@ from subiquitycore.utils import arun_command log = logging.getLogger("subiquity.server.pkghelper") +InstallCoroutine = Optional[Callable[[str], PackageInstallState]] + + class PackageInstaller: """Install packages from the pool on the ISO in the live session. @@ -54,12 +57,22 @@ class PackageInstaller: else: return PackageInstallState.INSTALLING - def start_installing_pkg(self, pkgname: str) -> None: + def start_installing_pkg( + self, pkgname: str, *, install_coro: InstallCoroutine = None + ) -> None: if pkgname not in self.pkgs: - self.pkgs[pkgname] = asyncio.create_task(self._install_pkg(pkgname)) + if install_coro is None: + install_coro = self._install_pkg + self.pkgs[pkgname] = asyncio.create_task(install_coro(pkgname)) - async def install_pkg(self, pkgname) -> PackageInstallState: - self.start_installing_pkg(pkgname) + async def install_pkg( + self, pkgname: str, *, install_coro: InstallCoroutine = None + ) -> PackageInstallState: + """Install the requested package. The default implementation runs + apt-get (or will consider the package installed after two seconds in + dry-run mode). If a different implementation is wanted, one can specify + an alternative install coroutine""" + self.start_installing_pkg(pkgname, install_coro=install_coro) return await self.pkgs[pkgname] async def _install_pkg(self, pkgname: str) -> PackageInstallState: diff --git a/subiquity/server/tests/test_pkghelper.py b/subiquity/server/tests/test_pkghelper.py new file mode 100644 index 00000000..faa2d91e --- /dev/null +++ b/subiquity/server/tests/test_pkghelper.py @@ -0,0 +1,101 @@ +# Copyright 2023 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 . + +from typing import Optional +from unittest.mock import Mock, patch + +import attr + +from subiquity.server.pkghelper import PackageInstaller, PackageInstallState +from subiquitycore.tests import SubiTestCase +from subiquitycore.tests.mocks import make_app + + +@patch("apt.Cache", Mock(return_value={})) +class TestPackageInstaller(SubiTestCase): + class Package: + @attr.s(auto_attribs=True) + class Candidate: + uri: str + + def __init__( + self, installed: bool, name: str, candidate_uri: Optional[str] = None + ) -> None: + self.installed = installed + self.name = name + if candidate_uri is None: + self.candidate = None + else: + self.candidate = self.Candidate(uri=candidate_uri) + + def setUp(self): + self.pkginstaller = PackageInstaller(make_app()) + + async def test_install_pkg_not_found(self): + self.assertEqual( + await self.pkginstaller.install_pkg("sysvinit-core"), + PackageInstallState.NOT_AVAILABLE, + ) + + async def test_install_pkg_already_installed(self): + with patch.dict( + self.pkginstaller.cache, + {"util-linux": self.Package(installed=True, name="util-linux")}, + ): + self.assertEqual( + await self.pkginstaller.install_pkg("util-linux"), + PackageInstallState.DONE, + ) + + async def test_install_pkg_dr_install(self): + with patch.dict( + self.pkginstaller.cache, + {"python3-attr": self.Package(installed=False, name="python3-attr")}, + ): + with patch("subiquity.server.pkghelper.asyncio.sleep") as sleep: + self.assertEqual( + await self.pkginstaller.install_pkg("python3-attr"), + PackageInstallState.DONE, + ) + sleep.assert_called_once() + + async def test_install_pkg_not_from_cdrom(self): + with patch.dict( + self.pkginstaller.cache, + { + "python3-attr": self.Package( + installed=False, + name="python3-attr", + candidate_uri="http://archive.ubuntu.com", + ) + }, + ): + with patch.object(self.pkginstaller.app.opts, "dry_run", False): + self.assertEqual( + await self.pkginstaller.install_pkg("python3-attr"), + PackageInstallState.NOT_AVAILABLE, + ) + + async def test_install_pkg_alternative_impl(self): + async def impl(pkgname: str) -> PackageInstallState: + return PackageInstallState.FAILED + + with patch.object(self.pkginstaller, "_install_pkg") as default_impl: + self.assertEqual( + await self.pkginstaller.install_pkg("python3-attr", install_coro=impl), + PackageInstallState.FAILED, + ) + + default_impl.assert_not_called() From 1670d0711fb0c8d86d5faf41c517e018a5f6eb34 Mon Sep 17 00:00:00 2001 From: Olivier Gayot Date: Tue, 5 Sep 2023 12:59:33 +0200 Subject: [PATCH 2/2] pkghelper: have alternative dry-run implementation Signed-off-by: Olivier Gayot --- subiquity/server/controllers/network.py | 18 +--- subiquity/server/pkghelper.py | 73 +++++++++----- subiquity/server/server.py | 4 +- subiquity/server/tests/test_pkghelper.py | 119 ++++++++++++++--------- 4 files changed, 126 insertions(+), 88 deletions(-) diff --git a/subiquity/server/controllers/network.py b/subiquity/server/controllers/network.py index 455d6337..a82ca466 100644 --- a/subiquity/server/controllers/network.py +++ b/subiquity/server/controllers/network.py @@ -108,23 +108,7 @@ class NetworkController(BaseNetworkController, SubiquityController): return self.app.package_installer.state_for_pkg("wpasupplicant") async def _install_wpasupplicant(self): - if self.opts.dry_run: - - async def fake_install(pkgname: str) -> PackageInstallState: - await asyncio.sleep(10 / self.app.scale_factor) - a = "DONE" - for k in self.app.debug_flags: - if k.startswith("wlan_install="): - a = k.split("=", 2)[1] - return getattr(PackageInstallState, a) - - install_coro = fake_install - else: - install_coro = None - - r = await self.app.package_installer.install_pkg( - "wpasupplicant", install_coro=install_coro - ) + r = await self.app.package_installer.install_pkg("wpasupplicant") log.debug("wlan_support_install_finished %s", r) self._call_clients("wlan_support_install_finished", r) if r == PackageInstallState.DONE: diff --git a/subiquity/server/pkghelper.py b/subiquity/server/pkghelper.py index 5e5828e4..6af1f239 100644 --- a/subiquity/server/pkghelper.py +++ b/subiquity/server/pkghelper.py @@ -16,7 +16,7 @@ import asyncio import logging import os -from typing import Callable, Dict, Optional +from typing import Dict, List, Optional import apt @@ -26,9 +26,6 @@ from subiquitycore.utils import arun_command log = logging.getLogger("subiquity.server.pkghelper") -InstallCoroutine = Optional[Callable[[str], PackageInstallState]] - - class PackageInstaller: """Install packages from the pool on the ISO in the live session. @@ -37,10 +34,9 @@ class PackageInstaller: by the server installer. """ - def __init__(self, app): + def __init__(self): self.pkgs: Dict[str, asyncio.Task] = {} self._cache: Optional[apt.Cache] = None - self.app = app @property def cache(self): @@ -57,22 +53,12 @@ class PackageInstaller: else: return PackageInstallState.INSTALLING - def start_installing_pkg( - self, pkgname: str, *, install_coro: InstallCoroutine = None - ) -> None: + def start_installing_pkg(self, pkgname: str) -> None: if pkgname not in self.pkgs: - if install_coro is None: - install_coro = self._install_pkg - self.pkgs[pkgname] = asyncio.create_task(install_coro(pkgname)) + self.pkgs[pkgname] = asyncio.create_task(self._install_pkg(pkgname)) - async def install_pkg( - self, pkgname: str, *, install_coro: InstallCoroutine = None - ) -> PackageInstallState: - """Install the requested package. The default implementation runs - apt-get (or will consider the package installed after two seconds in - dry-run mode). If a different implementation is wanted, one can specify - an alternative install coroutine""" - self.start_installing_pkg(pkgname, install_coro=install_coro) + async def install_pkg(self, pkgname: str) -> PackageInstallState: + self.start_installing_pkg(pkgname) return await self.pkgs[pkgname] async def _install_pkg(self, pkgname: str) -> PackageInstallState: @@ -84,10 +70,6 @@ class PackageInstaller: if binpkg.installed: log.debug("%s already installed", pkgname) return PackageInstallState.DONE - if self.app.opts.dry_run: - log.debug("dry-run apt-get install %s", pkgname) - await asyncio.sleep(2 / self.app.scale_factor) - return PackageInstallState.DONE if not binpkg.candidate.uri.startswith("cdrom:"): log.debug( "%s not available from cdrom (rather %s)", pkgname, binpkg.candidate.uri @@ -107,3 +89,46 @@ class PackageInstaller: return PackageInstallState.DONE else: return PackageInstallState.FAILED + + +class DryRunPackageInstaller(PackageInstaller): + def __init__(self, app) -> None: + super().__init__() + self.scale_factor: float = app.scale_factor + self.debug_flags: List[str] = app.debug_flags + self.package_specific_impl = { + "wpasupplicant": self._install_wpa_supplicant, + } + + async def _install_wpa_supplicant(self) -> PackageInstallState: + """Special implementation for wpasupplicant (used by code related to + Wi-Fi interfaces).""" + await asyncio.sleep(10 / self.scale_factor) + status = "DONE" + for flag in self.debug_flags: + if flag.startswith("wlan_install="): + status = flag.split("=", 2)[1] + return getattr(PackageInstallState, status) + + async def _install_pkg(self, pkgname: str) -> PackageInstallState: + if pkgname in self.package_specific_impl: + return await self.package_specific_impl[pkgname]() + + log.debug("checking if %s is available", pkgname) + binpkg = self.cache.get(pkgname) + if not binpkg: + log.debug("%s not found", pkgname) + return PackageInstallState.NOT_AVAILABLE + if binpkg.installed: + log.debug("%s already installed", pkgname) + return PackageInstallState.DONE + log.debug("dry-run apt-get install %s", pkgname) + await asyncio.sleep(2 / self.scale_factor) + return PackageInstallState.DONE + + +def get_package_installer(app): + if app.opts.dry_run: + return DryRunPackageInstaller(app) + else: + return PackageInstaller() diff --git a/subiquity/server/server.py b/subiquity/server/server.py index 5550b8e5..e08a71a3 100644 --- a/subiquity/server/server.py +++ b/subiquity/server/server.py @@ -44,7 +44,7 @@ from subiquity.server.controller import SubiquityController from subiquity.server.dryrun import DRConfig from subiquity.server.errors import ErrorController from subiquity.server.geoip import DryRunGeoIPStrategy, GeoIP, HTTPGeoIPStrategy -from subiquity.server.pkghelper import PackageInstaller +from subiquity.server.pkghelper import get_package_installer from subiquity.server.runner import get_command_runner from subiquity.server.snapdapi import make_api_client from subiquity.server.types import InstallerChannels @@ -291,7 +291,7 @@ class SubiquityServer(Application): self.event_syslog_id = "subiquity_event.{}".format(os.getpid()) self.log_syslog_id = "subiquity_log.{}".format(os.getpid()) self.command_runner = get_command_runner(self) - self.package_installer = PackageInstaller(self) + self.package_installer = get_package_installer(self) self.error_reporter = ErrorReporter( self.context.child("ErrorReporter"), self.opts.dry_run, self.root diff --git a/subiquity/server/tests/test_pkghelper.py b/subiquity/server/tests/test_pkghelper.py index faa2d91e..822d05f7 100644 --- a/subiquity/server/tests/test_pkghelper.py +++ b/subiquity/server/tests/test_pkghelper.py @@ -18,30 +18,36 @@ from unittest.mock import Mock, patch import attr -from subiquity.server.pkghelper import PackageInstaller, PackageInstallState +from subiquity.server.pkghelper import ( + DryRunPackageInstaller, + PackageInstaller, + PackageInstallState, +) +from subiquity.server.pkghelper import log as PkgHelperLogger from subiquitycore.tests import SubiTestCase from subiquitycore.tests.mocks import make_app +class MockPackage: + @attr.s(auto_attribs=True) + class Candidate: + uri: str + + def __init__( + self, installed: bool, name: str, candidate_uri: Optional[str] = None + ) -> None: + self.installed = installed + self.name = name + if candidate_uri is None: + self.candidate = None + else: + self.candidate = self.Candidate(uri=candidate_uri) + + @patch("apt.Cache", Mock(return_value={})) class TestPackageInstaller(SubiTestCase): - class Package: - @attr.s(auto_attribs=True) - class Candidate: - uri: str - - def __init__( - self, installed: bool, name: str, candidate_uri: Optional[str] = None - ) -> None: - self.installed = installed - self.name = name - if candidate_uri is None: - self.candidate = None - else: - self.candidate = self.Candidate(uri=candidate_uri) - def setUp(self): - self.pkginstaller = PackageInstaller(make_app()) + self.pkginstaller = PackageInstaller() async def test_install_pkg_not_found(self): self.assertEqual( @@ -52,50 +58,73 @@ class TestPackageInstaller(SubiTestCase): async def test_install_pkg_already_installed(self): with patch.dict( self.pkginstaller.cache, - {"util-linux": self.Package(installed=True, name="util-linux")}, + {"util-linux": MockPackage(installed=True, name="util-linux")}, ): self.assertEqual( await self.pkginstaller.install_pkg("util-linux"), PackageInstallState.DONE, ) - async def test_install_pkg_dr_install(self): - with patch.dict( - self.pkginstaller.cache, - {"python3-attr": self.Package(installed=False, name="python3-attr")}, - ): - with patch("subiquity.server.pkghelper.asyncio.sleep") as sleep: - self.assertEqual( - await self.pkginstaller.install_pkg("python3-attr"), - PackageInstallState.DONE, - ) - sleep.assert_called_once() - async def test_install_pkg_not_from_cdrom(self): with patch.dict( self.pkginstaller.cache, { - "python3-attr": self.Package( + "python3-attr": MockPackage( installed=False, name="python3-attr", candidate_uri="http://archive.ubuntu.com", ) }, ): - with patch.object(self.pkginstaller.app.opts, "dry_run", False): - self.assertEqual( - await self.pkginstaller.install_pkg("python3-attr"), - PackageInstallState.NOT_AVAILABLE, - ) - - async def test_install_pkg_alternative_impl(self): - async def impl(pkgname: str) -> PackageInstallState: - return PackageInstallState.FAILED - - with patch.object(self.pkginstaller, "_install_pkg") as default_impl: self.assertEqual( - await self.pkginstaller.install_pkg("python3-attr", install_coro=impl), - PackageInstallState.FAILED, + await self.pkginstaller.install_pkg("python3-attr"), + PackageInstallState.NOT_AVAILABLE, ) - default_impl.assert_not_called() + +@patch("apt.Cache", Mock(return_value={})) +@patch("subiquity.server.pkghelper.asyncio.sleep") +class TestDryRunPackageInstaller(SubiTestCase): + def setUp(self): + app = make_app() + app.debug_flags = [] + self.pkginstaller = DryRunPackageInstaller(app) + + async def test_install_pkg(self, sleep): + with patch.dict( + self.pkginstaller.cache, + {"python3-attr": MockPackage(installed=False, name="python3-attr")}, + ): + with self.assertLogs(PkgHelperLogger, "DEBUG") as debug: + self.assertEqual( + await self.pkginstaller.install_pkg("python3-attr"), + PackageInstallState.DONE, + ) + sleep.assert_called_once() + self.assertIn( + "dry-run apt-get install %s", [record.msg for record in debug.records] + ) + + async def test_install_pkg_wpasupplicant_default_impl(self, sleep): + with patch.object(self.pkginstaller, "debug_flags", []): + self.assertEqual( + await self.pkginstaller.install_pkg("wpasupplicant"), + PackageInstallState.DONE, + ) + sleep.assert_called_once() + + async def test_install_pkg_wpasupplicant_done(self, sleep): + with patch.object(self.pkginstaller, "debug_flags", ["wlan_install=DONE"]): + self.assertEqual( + await self.pkginstaller.install_pkg("wpasupplicant"), + PackageInstallState.DONE, + ) + sleep.assert_called_once() + + async def test_install_pkg_wpasupplicant_failed(self, sleep): + with patch.object(self.pkginstaller, "debug_flags", ["wlan_install=FAILED"]): + self.assertEqual( + await self.pkginstaller.install_pkg("wpasupplicant"), + PackageInstallState.FAILED, + ) + sleep.assert_called_once()