From c7fd905c6b03b596391dd0b80ea6cb76a48a1b0d Mon Sep 17 00:00:00 2001 From: Michael Hudson-Doyle Date: Mon, 6 May 2024 14:41:38 +0200 Subject: [PATCH 1/4] allow system definition to be in live layer Rather than always assuming it has to be mounted in from the layer to be installed (snapd will now populate the seed in the target system when its install API is called if it is empty). --- examples/snaps/v2-systems.json | 43 +++++++++++++++++++ subiquity/server/controllers/filesystem.py | 36 ++++++++++------ .../controllers/tests/test_filesystem.py | 15 ++++++- subiquity/server/snapdapi.py | 9 ++++ 4 files changed, 90 insertions(+), 13 deletions(-) create mode 100644 examples/snaps/v2-systems.json diff --git a/examples/snaps/v2-systems.json b/examples/snaps/v2-systems.json new file mode 100644 index 00000000..666861a5 --- /dev/null +++ b/examples/snaps/v2-systems.json @@ -0,0 +1,43 @@ +{ + "type": "sync", + "status-code": 200, + "status": "OK", + "result": { + "systems": [ + { + "current": true, + "label": "20240314", + "model": { + "model": "ubuntu-core-24-amd64-dangerous", + "brand-id": "canonical", + "display-name": "ubuntu-core-24-amd64-dangerous" + }, + "brand": { + "id": "canonical", + "username": "canonical", + "display-name": "Canonical", + "validation": "verified" + }, + "actions": [ + { + "title": "Reinstall", + "mode": "install" + }, + { + "title": "Recover", + "mode": "recover" + }, + { + "title": "Factory reset", + "mode": "factory-reset" + }, + { + "title": "Run normally", + "mode": "run" + } + ], + "default-recovery-system": true + } + ] + } +} diff --git a/subiquity/server/controllers/filesystem.py b/subiquity/server/controllers/filesystem.py index 1a58c0c0..84192cad 100644 --- a/subiquity/server/controllers/filesystem.py +++ b/subiquity/server/controllers/filesystem.py @@ -337,17 +337,26 @@ class FilesystemController(SubiquityController, FilesystemManipulator): self._source_handler = None async def _get_system(self, variation_name, label): - try: - await self._mount_systems_dir(variation_name) - except NoSnapdSystemsOnSource: - return None - try: - system = await self.app.snapdapi.v2.systems[label].GET() - except requests.exceptions.HTTPError as http_err: - log.warning("v2/systems/%s returned %s", label, http_err.response.text) - raise - finally: - await self._unmount_systems_dir() + systems = await self.app.snapdapi.v2.systems.GET() + labels = {system.label for system in systems.systems} + if label in labels: + try: + system = await self.app.snapdapi.v2.systems[label].GET() + except requests.exceptions.HTTPError as http_err: + log.warning("v2/systems/%s returned %s", label, http_err.response.text) + raise + else: + try: + await self._mount_systems_dir(variation_name) + except NoSnapdSystemsOnSource: + return None + try: + system = await self.app.snapdapi.v2.systems[label].GET() + except requests.exceptions.HTTPError as http_err: + log.warning("v2/systems/%s returned %s", label, http_err.response.text) + raise + finally: + await self._unmount_systems_dir() log.debug("got system %s", system) return system @@ -858,7 +867,10 @@ class FilesystemController(SubiquityController, FilesystemManipulator): async def guided_core_boot(self, disk: Disk): # Formatting for a core boot classic system relies on some curtin # features that are only available with v2 partitioning. - await self._mount_systems_dir(self._info.name) + systems = await self.app.snapdapi.v2.systems.GET() + labels = {system.label for system in systems.systems} + if self._info.label not in labels: + await self._mount_systems_dir(self._info.name) self.model.storage_version = 2 [volume] = self._info.system.volumes.values() self._on_volume = snapdapi.OnVolume.from_volume(volume) diff --git a/subiquity/server/controllers/tests/test_filesystem.py b/subiquity/server/controllers/tests/test_filesystem.py index bbad135f..0cc4ede5 100644 --- a/subiquity/server/controllers/tests/test_filesystem.py +++ b/subiquity/server/controllers/tests/test_filesystem.py @@ -432,6 +432,18 @@ class TestSubiquityControllerFilesystem(IsolatedAsyncioTestCase): }, } requests_mocker = requests_mock.Mocker() + requests_mocker.get( + "http+unix://snapd/v2/systems", + json={ + "type": "sync", + "status-code": 200, + "status": "OK", + "result": { + "systems": [], + }, + }, + status_code=200, + ) requests_mocker.get( "http+unix://snapd/v2/systems/enhanced-secureboot-desktop", json=json_body, @@ -1397,9 +1409,10 @@ class TestCoreBootInstallMethods(IsolatedAsyncioTestCase): name="foo", label="system", system=snapdapi.SystemDetails( + label="system", volumes={ "pc": snapdapi.Volume(schema="gpt", structure=structures), - } + }, ), ) diff --git a/subiquity/server/snapdapi.py b/subiquity/server/snapdapi.py index 5ae38382..2231cb61 100644 --- a/subiquity/server/snapdapi.py +++ b/subiquity/server/snapdapi.py @@ -203,6 +203,7 @@ class StorageEncryption: @attr.s(auto_attribs=True) class SystemDetails: + label: str current: bool = False volumes: Dict[str, Volume] = attr.Factory(dict) storage_encryption: Optional[StorageEncryption] = named_field( @@ -210,6 +211,11 @@ class SystemDetails: ) +@attr.s(auto_attribs=True) +class SystemsResponse: + systems: List[SystemDetails] = attr.Factory(list) + + class SystemAction(enum.Enum): INSTALL = "install" @@ -258,6 +264,9 @@ class SnapdAPI: ... class systems: + def GET() -> SystemsResponse: + ... + @path_parameter class label: def GET() -> SystemDetails: From 86282f5721e74038fb206d019e863c1aa0245421 Mon Sep 17 00:00:00 2001 From: Michael Hudson-Doyle Date: Tue, 7 May 2024 09:01:21 +0200 Subject: [PATCH 2/4] refactor to avoid checking the labels in the live layer twice --- subiquity/server/controllers/filesystem.py | 89 ++++++++++------------ 1 file changed, 40 insertions(+), 49 deletions(-) diff --git a/subiquity/server/controllers/filesystem.py b/subiquity/server/controllers/filesystem.py index 84192cad..e1f71f3c 100644 --- a/subiquity/server/controllers/filesystem.py +++ b/subiquity/server/controllers/filesystem.py @@ -22,7 +22,7 @@ import os import pathlib import subprocess import time -from typing import Any, Callable, Dict, List, Optional, Union +from typing import Any, Callable, Dict, List, Optional, Tuple, Union import attr import pyudev @@ -184,6 +184,7 @@ class VariationInfo: capability_info: CapabilityInfo = attr.Factory(CapabilityInfo) min_size: Optional[int] = None system: Optional[SystemDetails] = None + needs_systems_mount: bool = False def is_core_boot_classic(self) -> bool: return self.label is not None @@ -336,12 +337,14 @@ class FilesystemController(SubiquityController, FilesystemManipulator): self._source_handler.cleanup() self._source_handler = None - async def _get_system(self, variation_name, label): + async def _get_system( + self, variation_name, label + ) -> Tuple[Optional[SystemDetails], bool]: systems = await self.app.snapdapi.v2.systems.GET() labels = {system.label for system in systems.systems} if label in labels: try: - system = await self.app.snapdapi.v2.systems[label].GET() + return await self.app.snapdapi.v2.systems[label].GET(), True except requests.exceptions.HTTPError as http_err: log.warning("v2/systems/%s returned %s", label, http_err.response.text) raise @@ -349,16 +352,14 @@ class FilesystemController(SubiquityController, FilesystemManipulator): try: await self._mount_systems_dir(variation_name) except NoSnapdSystemsOnSource: - return None + return None, False try: - system = await self.app.snapdapi.v2.systems[label].GET() + return await self.app.snapdapi.v2.systems[label].GET(), False except requests.exceptions.HTTPError as http_err: log.warning("v2/systems/%s returned %s", label, http_err.response.text) raise finally: await self._unmount_systems_dir() - log.debug("got system %s", system) - return system def info_for_system(self, name: str, label: str, system: SystemDetails): if len(system.volumes) > 1: @@ -419,6 +420,29 @@ class FilesystemController(SubiquityController, FilesystemManipulator): return info + def _maybe_disable_encryption(self, info: VariationInfo) -> None: + if self.model.bootloader != Bootloader.UEFI: + log.debug("Disabling core boot based install options on non-UEFI " "system") + info.capability_info.disallow_if( + lambda cap: cap.is_core_boot(), + GuidedDisallowedCapabilityReason.NOT_UEFI, + _("Enhanced secure boot options only available on UEFI " "systems."), + ) + search_drivers = self.app.base_model.source.search_drivers + if search_drivers is not SEARCH_DRIVERS_AUTOINSTALL_DEFAULT and search_drivers: + log.debug( + "Disabling core boot based install options as third-party " + "drivers selected" + ) + info.capability_info.disallow_if( + lambda cap: cap.is_core_boot(), + GuidedDisallowedCapabilityReason.THIRD_PARTY_DRIVERS, + _( + "Enhanced secure boot options cannot currently install " + "third party drivers." + ), + ) + async def _examine_systems(self): self._variation_info.clear() catalog_entry = self.app.base_model.source.current @@ -426,7 +450,7 @@ class FilesystemController(SubiquityController, FilesystemManipulator): system = None label = variation.snapd_system_label if label is not None: - system = await self._get_system(name, label) + system, in_live_layer = await self._get_system(name, label) log.debug("got system %s for variation %s", system, name) if system is not None and len(system.volumes) > 0: if not self.app.opts.enhanced_secureboot: @@ -435,46 +459,15 @@ class FilesystemController(SubiquityController, FilesystemManipulator): info = self.info_for_system(name, label, system) if info is None: continue - if self.model.bootloader != Bootloader.UEFI: - log.debug( - "Disabling core boot based install options on non-UEFI " - "system" - ) - info.capability_info.disallow_if( - lambda cap: cap.is_core_boot(), - GuidedDisallowedCapabilityReason.NOT_UEFI, - _( - "Enhanced secure boot options only available on UEFI " - "systems." - ), - ) - search_drivers = self.app.base_model.source.search_drivers - if ( - search_drivers is not SEARCH_DRIVERS_AUTOINSTALL_DEFAULT - and search_drivers - ): - log.debug( - "Disabling core boot based install options as third-party " - "drivers selected" - ) - info.capability_info.disallow_if( - lambda cap: cap.is_core_boot(), - GuidedDisallowedCapabilityReason.THIRD_PARTY_DRIVERS, - _( - "Enhanced secure boot options cannot currently install " - "third party drivers." - ), - ) - self._variation_info[name] = info + if not in_live_layer: + info.needs_systems_mount = True + self._maybe_disable_encryption(info) elif catalog_entry.type.startswith("dd-"): min_size = variation.size - self._variation_info[name] = VariationInfo.dd( - name=name, min_size=min_size - ) + info = VariationInfo.dd(name=name, min_size=min_size) else: - self._variation_info[name] = VariationInfo.classic( - name=name, min_size=variation.size - ) + info = VariationInfo.classic(name=name, min_size=variation.size) + self._variation_info[name] = info @with_context() async def apply_autoinstall_config(self, context=None): @@ -865,12 +858,10 @@ class FilesystemController(SubiquityController, FilesystemManipulator): offset = offset + structure.size async def guided_core_boot(self, disk: Disk): + if self._info.needs_systems_mount: + await self._mount_systems_dir(self._info.name) # Formatting for a core boot classic system relies on some curtin # features that are only available with v2 partitioning. - systems = await self.app.snapdapi.v2.systems.GET() - labels = {system.label for system in systems.systems} - if self._info.label not in labels: - await self._mount_systems_dir(self._info.name) self.model.storage_version = 2 [volume] = self._info.system.volumes.values() self._on_volume = snapdapi.OnVolume.from_volume(volume) From d0366229ecf108de2acab9e291d4d5f9d0bc0eb6 Mon Sep 17 00:00:00 2001 From: Michael Hudson-Doyle Date: Wed, 22 May 2024 12:06:32 +1200 Subject: [PATCH 3/4] add docstring explaining _get_system return value --- subiquity/server/controllers/filesystem.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/subiquity/server/controllers/filesystem.py b/subiquity/server/controllers/filesystem.py index e1f71f3c..f6d16a38 100644 --- a/subiquity/server/controllers/filesystem.py +++ b/subiquity/server/controllers/filesystem.py @@ -340,6 +340,13 @@ class FilesystemController(SubiquityController, FilesystemManipulator): async def _get_system( self, variation_name, label ) -> Tuple[Optional[SystemDetails], bool]: + """Return system information for a given system label. + + The return value is a SystemDetails object (if any) and True if + the system was found in the layer that the installer is running + in or False if the source layer needed to be mounted to find + it. + """ systems = await self.app.snapdapi.v2.systems.GET() labels = {system.label for system in systems.systems} if label in labels: From cd3d7b1c95613c7b16fd5fcac17ef1a5473be9fc Mon Sep 17 00:00:00 2001 From: Michael Hudson-Doyle Date: Wed, 22 May 2024 12:09:06 +1200 Subject: [PATCH 4/4] tidy string literals --- subiquity/server/controllers/filesystem.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/subiquity/server/controllers/filesystem.py b/subiquity/server/controllers/filesystem.py index f6d16a38..c89a2b11 100644 --- a/subiquity/server/controllers/filesystem.py +++ b/subiquity/server/controllers/filesystem.py @@ -429,11 +429,11 @@ class FilesystemController(SubiquityController, FilesystemManipulator): def _maybe_disable_encryption(self, info: VariationInfo) -> None: if self.model.bootloader != Bootloader.UEFI: - log.debug("Disabling core boot based install options on non-UEFI " "system") + log.debug("Disabling core boot based install options on non-UEFI system") info.capability_info.disallow_if( lambda cap: cap.is_core_boot(), GuidedDisallowedCapabilityReason.NOT_UEFI, - _("Enhanced secure boot options only available on UEFI " "systems."), + _("Enhanced secure boot options only available on UEFI systems."), ) search_drivers = self.app.base_model.source.search_drivers if search_drivers is not SEARCH_DRIVERS_AUTOINSTALL_DEFAULT and search_drivers: