From 5eba140cbb2c399c1d7fdc1183bed30b1e28f0ca Mon Sep 17 00:00:00 2001 From: Olivier Gayot Date: Thu, 14 Sep 2023 11:52:53 +0200 Subject: [PATCH 1/3] codecs: skip installation when running an offline install ubuntu-restricted-addons is a multiverse package and is not included in the pool. Therefore, trying to get it installed when offline leads to an obvious error. Instead of making the whole Ubuntu installation fail, we now warn and skip installation of the package when performing an offline install. In a perfect world, we should not have offered to install the package in the first place, but in practice, we can run an offline installation as the result of failed mirror testing (bad network for instance). Signed-off-by: Olivier Gayot (cherry picked from commit 01ec1da86f5b811c6b59459a67ecac9af3209bb5) --- examples/autoinstall/fallback-offline.yaml | 21 ++++++++++++++++++ scripts/runtests.sh | 15 +++++++++++++ subiquity/common/pkg.py | 25 ++++++++++++++++++++++ subiquity/models/ad.py | 11 +++++++--- subiquity/models/codecs.py | 10 +++++++-- subiquity/models/locale.py | 7 ++++-- subiquity/models/network.py | 10 +++++---- subiquity/models/ssh.py | 10 +++++---- subiquity/models/subiquity.py | 12 ++++++++--- subiquity/server/controllers/install.py | 22 ++++++++++++++----- subiquity/server/controllers/package.py | 5 +++-- subiquity/tests/api/test_api.py | 2 +- 12 files changed, 124 insertions(+), 26 deletions(-) create mode 100644 examples/autoinstall/fallback-offline.yaml create mode 100644 subiquity/common/pkg.py diff --git a/examples/autoinstall/fallback-offline.yaml b/examples/autoinstall/fallback-offline.yaml new file mode 100644 index 000000000..d92b2d3e2 --- /dev/null +++ b/examples/autoinstall/fallback-offline.yaml @@ -0,0 +1,21 @@ +version: 1 +locale: en_GB.UTF-8 +network: + version: 2 + ethernets: + all-eth: + match: + name: "en*" + dhcp6: yes +apt: + mirror-selection: + primary: + - uri: http://localhost/failed + fallback: offline-install +codecs: + install: true +identity: + realname: '' + username: ubuntu + password: '$6$wdAcoXrU039hKYPd$508Qvbe7ObUnxoj15DRCkzC3qO7edjH0VV7BPNRDYK4QR8ofJaEEF2heacn0QgD.f8pO8SNp83XNdWG6tocBM1' + hostname: ubuntu diff --git a/scripts/runtests.sh b/scripts/runtests.sh index a44d37825..159c51e92 100755 --- a/scripts/runtests.sh +++ b/scripts/runtests.sh @@ -68,6 +68,9 @@ validate () { apt.security[1].uri='"http://ports.ubuntu.com/ubuntu-ports"' ;; esac + if [ "$testname" == autoinstall-fallback-offline ]; then + grep -F -- 'skipping installation of package ubuntu-restricted-addons' "$tmpdir"/subiquity-server-debug.log + fi netplan generate --root $tmpdir elif [ "${mode}" = "system_setup" ]; then setup_mode="$2" @@ -319,6 +322,18 @@ LANG=C.UTF-8 timeout --foreground 60 \ --source-catalog examples/sources/install.yaml validate install +clean +testname=autoinstall-fallback-offline +LANG=C.UTF-8 timeout --foreground 60 \ + python3 -m subiquity.cmd.tui \ + --dry-run \ + --output-base "$tmpdir" \ + --machine-config examples/machines/simple.json \ + --autoinstall examples/autoinstall/fallback-offline.yaml \ + --kernel-cmdline autoinstall \ + --source-catalog examples/sources/install.yaml +validate install + # The OOBE doesn't exist in WSL < 20.04 if [ "${RELEASE%.*}" -ge 20 ]; then # Test TCP connectivity (system_setup only) diff --git a/subiquity/common/pkg.py b/subiquity/common/pkg.py new file mode 100644 index 000000000..4efa3aea6 --- /dev/null +++ b/subiquity/common/pkg.py @@ -0,0 +1,25 @@ +# 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 . + +import attr + + +@attr.s(auto_attribs=True) +class TargetPkg: + name: str + # Some packages are not present in the pool and require a working network + # connection to be downloaded. By marking them with "skip_when_offline", we + # can skip them when running an offline install. + skip_when_offline: bool diff --git a/subiquity/models/ad.py b/subiquity/models/ad.py index f727f1d6d..a76f8af39 100644 --- a/subiquity/models/ad.py +++ b/subiquity/models/ad.py @@ -14,8 +14,9 @@ # along with this program. If not, see . import logging -from typing import Optional +from typing import List, Optional +from subiquity.common.pkg import TargetPkg from subiquity.common.types import AdConnectionInfo log = logging.getLogger("subiquity.models.ad") @@ -42,10 +43,14 @@ def set_domain(self, domain: str): else: self.conn_info = AdConnectionInfo(domain_name=domain) - async def target_packages(self): + async def target_packages(self) -> List[TargetPkg]: # NOTE Those packages must be present in the target system to allow # joining to a domain. if self.do_join: - return ["adcli", "realmd", "sssd"] + return [ + TargetPkg(name="adcli", skip_when_offline=False), + TargetPkg(name="realmd", skip_when_offline=False), + TargetPkg(name="sssd", skip_when_offline=False), + ] return [] diff --git a/subiquity/models/codecs.py b/subiquity/models/codecs.py index 326369cb0..4f7eafa2e 100644 --- a/subiquity/models/codecs.py +++ b/subiquity/models/codecs.py @@ -14,6 +14,9 @@ # along with this program. If not, see . import logging +from typing import List + +from subiquity.common.pkg import TargetPkg log = logging.getLogger("subiquity.models.codecs") @@ -21,9 +24,12 @@ class CodecsModel: do_install = False - async def target_packages(self): + async def target_packages(self) -> List[TargetPkg]: # NOTE currently, ubuntu-restricted-addons is an empty package that # pulls relevant packages through Recommends: Ideally, we should make # sure to run the APT command for this package with the # --install-recommends option. - return ["ubuntu-restricted-addons"] if self.do_install else [] + if not self.do_install: + return [] + + return [TargetPkg(name="ubuntu-restricted-addons", skip_when_offline=True)] diff --git a/subiquity/models/locale.py b/subiquity/models/locale.py index 9ca41e8a1..4e4ee07a0 100644 --- a/subiquity/models/locale.py +++ b/subiquity/models/locale.py @@ -16,7 +16,9 @@ import locale import logging import subprocess +from typing import List +from subiquity.common.pkg import TargetPkg from subiquitycore.utils import arun_command, split_cmd_output log = logging.getLogger("subiquity.models.locale") @@ -60,7 +62,7 @@ def make_cloudconfig(self): locale += ".UTF-8" return {"locale": locale} - async def target_packages(self): + async def target_packages(self) -> List[TargetPkg]: if self.selected_language is None: return [] if self.locale_support != "langpack": @@ -69,6 +71,7 @@ async def target_packages(self): if lang == "C": return [] - return await split_cmd_output( + pkgs = await split_cmd_output( self.chroot_prefix + ["check-language-support", "-l", lang], None ) + return [TargetPkg(name=pkg, skip_when_offline=False) for pkg in pkgs] diff --git a/subiquity/models/network.py b/subiquity/models/network.py index 19cc2d6fb..504b9c462 100644 --- a/subiquity/models/network.py +++ b/subiquity/models/network.py @@ -15,8 +15,10 @@ import logging import subprocess +from typing import List from subiquity import cloudinit +from subiquity.common.pkg import TargetPkg from subiquitycore.models.network import NetworkModel as CoreNetworkModel from subiquitycore.utils import arun_command @@ -93,12 +95,12 @@ def render(self): } return r - async def target_packages(self): - if self.needs_wpasupplicant: - return ["wpasupplicant"] - else: + async def target_packages(self) -> List[TargetPkg]: + if not self.needs_wpasupplicant: return [] + return [TargetPkg(name="wpasupplicant", skip_when_offline=False)] + async def is_nm_enabled(self): try: cp = await arun_command(("nmcli", "networking"), check=True) diff --git a/subiquity/models/ssh.py b/subiquity/models/ssh.py index e6081256e..b5dd527a8 100644 --- a/subiquity/models/ssh.py +++ b/subiquity/models/ssh.py @@ -16,6 +16,8 @@ import logging from typing import List +from subiquity.common.pkg import TargetPkg + log = logging.getLogger("subiquity.models.ssh") @@ -29,8 +31,8 @@ def __init__(self): # we go back to it. self.ssh_import_id = "" - async def target_packages(self): - if self.install_server: - return ["openssh-server"] - else: + async def target_packages(self) -> List[TargetPkg]: + if not self.install_server: return [] + + return [TargetPkg(name="openssh-server", skip_when_offline=False)] diff --git a/subiquity/models/subiquity.py b/subiquity/models/subiquity.py index 0ac822f2e..6c3991d12 100644 --- a/subiquity/models/subiquity.py +++ b/subiquity/models/subiquity.py @@ -20,7 +20,7 @@ import os import uuid from collections import OrderedDict -from typing import Any, Dict, Set, Tuple +from typing import Any, Dict, List, Set, Tuple import yaml from cloudinit.config.schema import ( @@ -39,6 +39,7 @@ def SchemaProblem(x, y): from curtin.config import merge_config +from subiquity.common.pkg import TargetPkg from subiquity.common.resources import get_users_and_groups from subiquity.server.types import InstallerChannels from subiquitycore.file_util import generate_timestamped_header, write_file @@ -177,6 +178,9 @@ def __init__(self, root, hub, install_model_names, postinstall_model_names): self.chroot_prefix = [] self.active_directory = AdModel() + # List of packages that will be installed using cloud-init on first + # boot. + self.cloud_init_packages: List[str] = [] self.codecs = CodecsModel() self.debconf_selections = DebconfSelectionsModel() self.drivers = DriversModel() @@ -189,7 +193,7 @@ def __init__(self, root, hub, install_model_names, postinstall_model_names): self.mirror = MirrorModel() self.network = NetworkModel() self.oem = OEMModel() - self.packages = [] + self.packages: List[TargetPkg] = [] self.proxy = ProxyModel() self.snaplist = SnapListModel() self.ssh = SSHModel() @@ -376,13 +380,15 @@ def _cloud_init_config(self): model = getattr(self, model_name) if getattr(model, "make_cloudconfig", None): merge_config(config, model.make_cloudconfig()) + for package in self.cloud_init_packages: + merge_config(config, {"packages": list(self.cloud_init_packages)}) merge_cloud_init_config(config, self.userdata) if lsb_release()["release"] not in ("20.04", "22.04"): config.setdefault("write_files", []).append(CLOUDINIT_DISABLE_AFTER_INSTALL) self.validate_cloudconfig_schema(data=config, data_source="system install") return config - async def target_packages(self): + async def target_packages(self) -> List[TargetPkg]: packages = list(self.packages) for model_name in self._postinstall_model_names.all(): meth = getattr(getattr(self, model_name), "target_packages", None) diff --git a/subiquity/server/controllers/install.py b/subiquity/server/controllers/install.py index 26dde35e9..d61a91eeb 100644 --- a/subiquity/server/controllers/install.py +++ b/subiquity/server/controllers/install.py @@ -34,6 +34,7 @@ from curtin.util import get_efibootmgr, is_uefi_bootable from subiquity.common.errorreport import ErrorReportKind +from subiquity.common.pkg import TargetPkg from subiquity.common.types import ApplicationState, PackageInstallState from subiquity.journald import journald_listen from subiquity.models.filesystem import ActionRenderMode, Partition @@ -689,11 +690,22 @@ async def postinstall(self, *, context): {"autoinstall": self.app.make_autoinstall()} ) write_file(autoinstall_path, autoinstall_config) - await self.configure_cloud_init(context=context) + try: + if self.supports_apt(): + packages = await self.get_target_packages(context=context) + for package in packages: + if package.skip_when_offline and not self.model.network.has_network: + log.warning( + "skipping installation of package %s when" + " performing an offline install.", + package.name, + ) + continue + await self.install_package(context=context, package=package.name) + finally: + await self.configure_cloud_init(context=context) + if self.supports_apt(): - packages = await self.get_target_packages(context=context) - for package in packages: - await self.install_package(context=context, package=package) if self.model.drivers.do_install: with context.child( "ubuntu-drivers-install", "installing third-party drivers" @@ -720,7 +732,7 @@ async def configure_cloud_init(self, context): await run_in_thread(self.model.configure_cloud_init) @with_context(description="calculating extra packages to install") - async def get_target_packages(self, context): + async def get_target_packages(self, context) -> List[TargetPkg]: return await self.app.base_model.target_packages() @with_context(name="install_{package}", description="installing {package}") diff --git a/subiquity/server/controllers/package.py b/subiquity/server/controllers/package.py index 61fe07f2b..feff2d56b 100644 --- a/subiquity/server/controllers/package.py +++ b/subiquity/server/controllers/package.py @@ -13,6 +13,7 @@ # You should have received a copy of the GNU Affero General Public License # along with this program. If not, see . +from subiquity.common.pkg import TargetPkg from subiquity.server.controller import NonInteractiveController @@ -25,7 +26,7 @@ class PackageController(NonInteractiveController): } def load_autoinstall_data(self, data): - self.model[:] = data + self.model[:] = [TargetPkg(name=pkg, skip_when_offline=False) for pkg in data] def make_autoinstall(self): - return self.model + return [pkg.name for pkg in self.model] diff --git a/subiquity/tests/api/test_api.py b/subiquity/tests/api/test_api.py index c4206e0b9..8bce77fb0 100644 --- a/subiquity/tests/api/test_api.py +++ b/subiquity/tests/api/test_api.py @@ -2133,7 +2133,7 @@ async def packages_lookup(self, log_dir: str) -> Dict[str, bool]: to be installed in the target system and whether they were referred to or not in the server log.""" expected_packages = await self.target_packages() - packages_lookup = {p: False for p in expected_packages} + packages_lookup = {p.name: False for p in expected_packages} log_path = os.path.join(log_dir, "subiquity-server-debug.log") find_start = "finish: subiquity/Install/install/postinstall/install_{}:" log_status = " SUCCESS: installing {}" From ecf56e14141a9b0887419892d67e84b45c087a80 Mon Sep 17 00:00:00 2001 From: Olivier Gayot Date: Mon, 2 Oct 2023 11:38:35 +0200 Subject: [PATCH 2/3] shutdown: do not try to unmount /target if install was not started If we ask for reboot before the installation has started (i.e., if curtin install was not invoked at least once), the following call fails and prevents the system from rebooting. $ umount --recursive /target Make sure we check that /target exists and is mounted before calling umount. Another approach would be to check the return value of umount but the values are not documented. Signed-off-by: Olivier Gayot (cherry picked from commit abef05178c4e60bbe259a7371e2a97e3821669fc) --- subiquity/server/controllers/filesystem.py | 12 +++++++- .../controllers/tests/test_filesystem.py | 28 +++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/subiquity/server/controllers/filesystem.py b/subiquity/server/controllers/filesystem.py index b9f091ddd..b792426c0 100644 --- a/subiquity/server/controllers/filesystem.py +++ b/subiquity/server/controllers/filesystem.py @@ -20,6 +20,7 @@ import logging import os import pathlib +import subprocess import time from typing import Any, Callable, Dict, List, Optional, Union @@ -1532,6 +1533,15 @@ def make_autoinstall(self): async def _pre_shutdown(self): if not self.reset_partition_only: - await self.app.command_runner.run(["umount", "--recursive", "/target"]) + # /target is mounted only if the installation was actually started. + try: + await self.app.command_runner.run(["mountpoint", "/target"]) + except subprocess.CalledProcessError: + log.debug( + "/target does not exist or is not mounted," + " skipping call to umount --recursive" + ) + else: + await self.app.command_runner.run(["umount", "--recursive", "/target"]) if len(self.model._all(type="zpool")) > 0: await self.app.command_runner.run(["zpool", "export", "-a"]) diff --git a/subiquity/server/controllers/tests/test_filesystem.py b/subiquity/server/controllers/tests/test_filesystem.py index cf6cb6bba..a48e82d84 100644 --- a/subiquity/server/controllers/tests/test_filesystem.py +++ b/subiquity/server/controllers/tests/test_filesystem.py @@ -14,6 +14,7 @@ # along with this program. If not, see . import copy +import subprocess import uuid from unittest import IsolatedAsyncioTestCase, mock @@ -89,6 +90,7 @@ class TestSubiquityControllerFilesystem(IsolatedAsyncioTestCase): def setUp(self): self.app = make_app() self.app.opts.bootloader = "UEFI" + self.app.command_runner = mock.AsyncMock() self.app.report_start_event = mock.Mock() self.app.report_finish_event = mock.Mock() self.app.prober = mock.Mock() @@ -343,6 +345,32 @@ async def test_v2_edit_partition_POST(self): self.assertTrue(self.fsc.locked_probe_data) handler.assert_called_once() + async def test__pre_shutdown_install_started(self): + self.fsc.reset_partition_only = False + run = mock.patch.object(self.app.command_runner, "run") + _all = mock.patch.object(self.fsc.model, "_all") + with run as mock_run, _all: + await self.fsc._pre_shutdown() + mock_run.assert_has_calls( + [ + mock.call(["mountpoint", "/target"]), + mock.call(["umount", "--recursive", "/target"]), + ] + ) + self.assertEqual(len(mock_run.mock_calls), 2) + + async def test__pre_shutdown_install_not_started(self): + async def fake_run(cmd, **kwargs): + if cmd == ["mountpoint", "/target"]: + raise subprocess.CalledProcessError(cmd=cmd, returncode=1) + + self.fsc.reset_partition_only = False + run = mock.patch.object(self.app.command_runner, "run", side_effect=fake_run) + _all = mock.patch.object(self.fsc.model, "_all") + with run as mock_run, _all: + await self.fsc._pre_shutdown() + mock_run.assert_called_once_with(["mountpoint", "/target"]) + class TestGuided(IsolatedAsyncioTestCase): boot_expectations = [ From 3dfce34240e249623c43255c19e5db4abdc5a9b6 Mon Sep 17 00:00:00 2001 From: Olivier Gayot Date: Mon, 2 Oct 2023 14:37:52 +0200 Subject: [PATCH 3/3] workflows: differentiate CI and Snap workflows The workflows defined respectively in build.yaml and snap.yaml were both called "CI". On the Github web interface, it resulted in two menus called "CI" with no easy way to know which is which. To make things clearer, we now: * rename build.yaml -> ci.yaml * call "Snap" the workflow defined by snap.yaml Signed-off-by: Olivier Gayot (cherry picked from commit a34bce470fc2f15d4a892cd19e0685c53a6d4060) --- .github/workflows/{build.yaml => ci.yaml} | 0 .github/workflows/snap.yaml | 2 +- 2 files changed, 1 insertion(+), 1 deletion(-) rename .github/workflows/{build.yaml => ci.yaml} (100%) diff --git a/.github/workflows/build.yaml b/.github/workflows/ci.yaml similarity index 100% rename from .github/workflows/build.yaml rename to .github/workflows/ci.yaml diff --git a/.github/workflows/snap.yaml b/.github/workflows/snap.yaml index 87fc71249..b92c3aec2 100644 --- a/.github/workflows/snap.yaml +++ b/.github/workflows/snap.yaml @@ -1,4 +1,4 @@ -name: CI +name: Snap on: [push, pull_request]