From 47dc10c0c609882d74ce82d56259549cf4628f9a Mon Sep 17 00:00:00 2001 From: Michael Hudson-Doyle Date: Thu, 10 Aug 2023 12:50:47 +1200 Subject: [PATCH 1/7] several fixes around reset partition handling 1. adjust the RP to use a different casper uuid, to avoid confusing when install media is still present 2. record the partuuid of the RP on the kernel command line when booted from there so subiquity knows to add a grub boot entry pointing to it in this case 3. fix generating the grub boot entry pointing to the RP as this was broken in a few ways 4. get the logic the right way around to only set BootNext in reset-partition-only case --- subiquity/models/filesystem.py | 11 ++ subiquity/server/controllers/install.py | 128 ++++++++++++++---- .../server/controllers/tests/test_install.py | 66 +++++++-- 3 files changed, 172 insertions(+), 33 deletions(-) diff --git a/subiquity/models/filesystem.py b/subiquity/models/filesystem.py index 7a903c92..65611297 100644 --- a/subiquity/models/filesystem.py +++ b/subiquity/models/filesystem.py @@ -1820,6 +1820,17 @@ class FilesystemModel(object): def all_volgroups(self): return self._all(type="lvm_volgroup") + def partition_by_partuuid(self, partuuid: str) -> Optional[Partition]: + # This can be simplified when + # https://code.launchpad.net/~mwhudson/curtin/+git/curtin/+merge/448842 + # lands. + for part in self.app.base_model.filesystem._all(type="partition"): + if part._info is None: + continue + if part._info.raw.get("ID_PART_ENTRY_UUID") == partuuid: + return part + return None + def _remove(self, obj): _remove_backlinks(obj) self._actions.remove(obj) diff --git a/subiquity/server/controllers/install.py b/subiquity/server/controllers/install.py index f17f4e84..03ec9141 100644 --- a/subiquity/server/controllers/install.py +++ b/subiquity/server/controllers/install.py @@ -15,13 +15,16 @@ import asyncio import copy +import glob import json import logging import os import re +import shlex import shutil import subprocess import tempfile +import uuid from pathlib import Path from typing import Any, Dict, List, Optional @@ -32,11 +35,11 @@ from curtin.util import get_efibootmgr, is_uefi_bootable from subiquity.common.errorreport import ErrorReportKind from subiquity.common.types import ApplicationState, PackageInstallState from subiquity.journald import journald_listen -from subiquity.models.filesystem import ActionRenderMode +from subiquity.models.filesystem import ActionRenderMode, Partition from subiquity.server.controller import SubiquityController from subiquity.server.curtin import run_curtin_command, start_curtin_command from subiquity.server.kernel import list_installed_kernels -from subiquity.server.mounter import Mounter +from subiquity.server.mounter import Mounter, Mountpoint from subiquity.server.types import InstallerChannels from subiquitycore.async_helpers import run_bg_task, run_in_thread from subiquitycore.context import with_context @@ -421,22 +424,81 @@ class InstallController(SubiquityController): step_config=self.rp_config(logs_dir, mp.p()), source="cp:///cdrom", ) - await self.create_rp_boot_entry(context=context, rp=rp) + new_casper_uuid = await self.adjust_rp(rp, mp) + await self.configure_rp_boot( + context=context, rp=rp, casper_uuid=new_casper_uuid + ) + else: + casper_uuid = None + for casper_uuid_file in glob.glob("/cdrom/.disk/casper-uuid-*"): + with open(casper_uuid_file) as fp: + casper_uuid = fp.read().strip() + rp_partuuid = self.app.kernel_cmdline.get("rp-partuuid") + if casper_uuid is not None and rp_partuuid is not None: + rp = self.app.base_model.partition_by_partuuid(rp_partuuid) + if rp is not None: + await self.configure_rp_boot( + context=context, rp=rp, casper_uuid=casper_uuid + ) - @with_context(description="creating boot entry for reset partition") - async def create_rp_boot_entry(self, context, rp): - fs_controller = self.app.controllers.Filesystem - if not fs_controller.reset_partition_only: - cp = await self.app.command_runner.run( - ["lsblk", "-n", "-o", "UUID", rp.path], capture=True - ) - uuid = cp.stdout.decode("ascii").strip() - conf = grub_reset_conf.format( - HEADER=generate_timestamped_header(), PARTITION=rp.number, UUID=uuid - ) - with open(self.tpath("etc/grub.d/99_reset"), "w") as fp: - os.chmod(fp.fileno(), 0o755) - fp.write(conf) + async def adjust_rp(self, rp: Partition, mp: Mountpoint) -> str: + if self.app.opts.dry_run: + return + # Once the installer has been copied to the RP, we need to make two + # adjustments: + # + # 1. set a new "casper uuid" so that booting from the install + # media again, or booting from the RP but with the install + # media still attached, does not get confused about which + # device to use as /cdrom. + # + # 2. add "rp-partuuid" to the kernel command line in grub.cfg + # so that subiquity can identify when it is running from + # the recovery partition and add a reference to it to + # grub.cfg on the target system in that case. + grub_cfg_path = mp.p("boot/grub/grub.cfg") + new_cfg = [] + new_casper_uuid = str(uuid.uuid4()) + cp = await self.app.command_runner.run( + ["lsblk", "-n", "-o", "PARTUUID", rp.path], capture=True + ) + rp_uuid = cp.stdout.decode("ascii").strip() + with open(grub_cfg_path) as fp: + for line in fp: + words = shlex.split(line) + if words and words[0] == "linux" and "---" in words: + index = words.index("---") + words[index - 1 : index - 1] = [ + "uuid=" + new_casper_uuid, + "rp-partuuid=" + rp_uuid, + ] + new_cfg.append(shlex.join(words) + "\n") + else: + new_cfg.append(line) + with open(grub_cfg_path, "w") as fp: + fp.write("".join(new_cfg)) + for casper_uuid_file in glob.glob(mp.p(".disk/casper-uuid-*")): + with open(casper_uuid_file, "w") as fp: + fp.write(new_casper_uuid + "\n") + return new_casper_uuid + + @with_context(description="configuring grub menu entry for factory reset") + async def configure_rp_boot_grub(self, context, rp: Partition, casper_uuid: str): + # Add a grub menu entry to boot from the RP + cp = await self.app.command_runner.run( + ["lsblk", "-n", "-o", "UUID,PARTUUID", rp.path], capture=True + ) + fs_uuid, rp_uuid = cp.stdout.decode("ascii").strip().split() + conf = grub_reset_conf.format( + HEADER=generate_timestamped_header(), + PARTITION=rp.number, + FS_UUID=fs_uuid, + CASPER_UUID=casper_uuid, + RP_UUID=rp_uuid, + ) + with open(self.tpath("etc/grub.d/99_reset"), "w") as fp: + os.chmod(fp.fileno(), 0o755) + fp.write(conf) await run_curtin_command( self.app, context, @@ -447,9 +509,14 @@ class InstallController(SubiquityController): "update-grub", private_mounts=False, ) - if self.app.opts.dry_run and not is_uefi_bootable(): - # Can't even run efibootmgr in this case. - return + + @with_context(description="configuring UEFI menu entry for factory reset") + async def configure_rp_boot_uefi(self, context, rp: Partition): + # Add an UEFI boot entry to point at the RP + # Details: + # 1. Do not leave duplicate entries + # 2. Do not leave the boot entry in BootOrder + # 3. Set BootNext to boot from RP in the reset-partition-only case. state = await self.app.package_installer.install_pkg("efibootmgr") if state != PackageInstallState.DONE: raise RuntimeError("could not install efibootmgr") @@ -470,6 +537,7 @@ class InstallController(SubiquityController): efi_state_after = get_efibootmgr("/") new_bootnums = set(efi_state_after.entries) - set(efi_state_before.entries) if not new_bootnums: + # Will probably only happen in dry-run mode. return new_bootnum = new_bootnums.pop() new_entry = efi_state_after.entries[new_bootnum] @@ -493,7 +561,7 @@ class InstallController(SubiquityController): ] rp_bootnum = new_bootnum await self.app.command_runner.run(cmd) - if not fs_controller.reset_partition_only: + if self.model.target is None: cmd = [ "efibootmgr", "--bootnext", @@ -501,6 +569,16 @@ class InstallController(SubiquityController): ] await self.app.command_runner.run(cmd) + async def configure_rp_boot(self, context, rp: Partition, casper_uuid: str): + if self.model.target is not None and not self.opts.dry_run: + await self.configure_rp_boot_grub( + context=context, rp=rp, casper_uuid=casper_uuid + ) + if self.app.opts.dry_run and not is_uefi_bootable(): + # Can't even run efibootmgr in this case. + return + await self.configure_rp_boot_uefi(context=context, rp=rp) + @with_context(description="creating fstab") async def create_core_boot_classic_fstab(self, *, context): with open(self.tpath("etc/fstab"), "w") as fp: @@ -739,10 +817,10 @@ grub_reset_conf = """\ set -e cat << EOF -menuentry "Restore Ubuntu to factory state" { - search --no-floppy --hint '(hd0,{PARTITION})' --set --fs-uuid {UUID} - linux /casper/vmlinuz uuid={UUID} nopersistent +menuentry "Restore Ubuntu to factory state" {{ + search --no-floppy --hint '(hd0,{PARTITION})' --set --fs-uuid {FS_UUID} + linux /casper/vmlinuz uuid={CASPER_UUID} rp-partuuid={RP_UUID} nopersistent initrd /casper/initrd -} +}} EOF """ diff --git a/subiquity/server/controllers/tests/test_install.py b/subiquity/server/controllers/tests/test_install.py index 5c042046..0b142d90 100644 --- a/subiquity/server/controllers/tests/test_install.py +++ b/subiquity/server/controllers/tests/test_install.py @@ -13,7 +13,10 @@ # You should have received a copy of the GNU Affero General Public License # along with this program. If not, see . +import os +import shutil import subprocess +import tempfile import unittest from pathlib import Path from unittest.mock import ANY, AsyncMock, Mock, call, mock_open, patch @@ -198,7 +201,9 @@ class TestInstallController(unittest.IsolatedAsyncioTestCase): self.controller = InstallController(make_app()) self.controller.app.report_start_event = Mock() self.controller.app.report_finish_event = Mock() - self.controller.model.target = "/target" + self.controller.model.target = tempfile.mkdtemp() + os.makedirs(os.path.join(self.controller.model.target, "etc/grub.d")) + self.addCleanup(shutil.rmtree, self.controller.model.target) @patch("asyncio.sleep") async def test_install_package(self, m_sleep): @@ -221,23 +226,25 @@ class TestInstallController(unittest.IsolatedAsyncioTestCase): with self.assertRaises(subprocess.CalledProcessError): await self.controller.install_package(package="git") - def setup_rp_test(self): + def setup_rp_test(self, lsblk_output=b"lsblk_output"): app = self.controller.app app.opts.dry_run = False fsc = app.controllers.Filesystem fsc.reset_partition_only = True app.package_installer = Mock() - app.command_runner = Mock() - self.run = app.command_runner.run = AsyncMock() + app.command_runner = AsyncMock() + self.run = app.command_runner.run = AsyncMock( + return_value=subprocess.CompletedProcess((), 0, stdout=lsblk_output) + ) app.package_installer.install_pkg = AsyncMock() app.package_installer.install_pkg.return_value = PackageInstallState.DONE fsm, self.part = make_model_and_partition() @patch("subiquity.server.controllers.install.get_efibootmgr") - async def test_create_rp_boot_entry_add(self, m_get_efibootmgr): + async def test_configure_rp_boot_uefi_add(self, m_get_efibootmgr): m_get_efibootmgr.side_effect = iter([efi_state_no_rp, efi_state_with_rp]) self.setup_rp_test() - await self.controller.create_rp_boot_entry(rp=self.part) + await self.controller.configure_rp_boot_uefi(rp=self.part) calls = [ call( [ @@ -264,10 +271,41 @@ class TestInstallController(unittest.IsolatedAsyncioTestCase): self.run.assert_has_awaits(calls) @patch("subiquity.server.controllers.install.get_efibootmgr") - async def test_create_rp_boot_entry_dup(self, m_get_efibootmgr): + async def test_configure_rp_boot_uefi_bootnext(self, m_get_efibootmgr): + m_get_efibootmgr.side_effect = iter([efi_state_no_rp, efi_state_with_rp]) + self.setup_rp_test() + self.controller.app.base_model.target = None + await self.controller.configure_rp_boot_uefi(rp=self.part) + calls = [ + call( + [ + "efibootmgr", + "--create", + "--loader", + "\\EFI\\boot\\shimx64.efi", + "--disk", + self.part.device.path, + "--part", + str(self.part.number), + "--label", + "Restore Ubuntu to factory state", + ] + ), + call( + [ + "efibootmgr", + "--bootorder", + "0000,0002", + ] + ), + ] + self.run.assert_has_awaits(calls) + + @patch("subiquity.server.controllers.install.get_efibootmgr") + async def test_configure_rp_boot_uefi_dup(self, m_get_efibootmgr): m_get_efibootmgr.side_effect = iter([efi_state_with_rp, efi_state_with_dup_rp]) self.setup_rp_test() - await self.controller.create_rp_boot_entry(rp=self.part) + await self.controller.configure_rp_boot_uefi(rp=self.part) calls = [ call( [ @@ -293,3 +331,15 @@ class TestInstallController(unittest.IsolatedAsyncioTestCase): ), ] self.run.assert_has_awaits(calls) + + async def test_configure_rp_boot_grub(self): + fsuuid, partuuid = "fsuuid", "partuuid" + self.setup_rp_test(f"{fsuuid}\t{partuuid}".encode("ascii")) + await self.controller.configure_rp_boot_grub( + rp=self.part, casper_uuid="casper-uuid" + ) + with open(self.controller.tpath("etc/grub.d/99_reset")) as fp: + cfg = fp.read() + self.assertIn("--fs-uuid fsuuid", cfg) + self.assertIn("rp-partuuid=partuuid", cfg) + self.assertIn("uuid=casper-uuid", cfg) From 1ab061f8d8f1dfece48a93af8dad66d6389038d8 Mon Sep 17 00:00:00 2001 From: Michael Hudson-Doyle Date: Mon, 14 Aug 2023 09:10:37 +1200 Subject: [PATCH 2/7] fix boot entry creation to use actual removable media path --- subiquity/server/controllers/install.py | 2 +- subiquity/server/controllers/tests/test_install.py | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/subiquity/server/controllers/install.py b/subiquity/server/controllers/install.py index 03ec9141..2e0ad9c9 100644 --- a/subiquity/server/controllers/install.py +++ b/subiquity/server/controllers/install.py @@ -525,7 +525,7 @@ class InstallController(SubiquityController): "efibootmgr", "--create", "--loader", - "\\EFI\\boot\\shimx64.efi", + "\\EFI\\boot\\bootx64.efi", "--disk", rp.device.path, "--part", diff --git a/subiquity/server/controllers/tests/test_install.py b/subiquity/server/controllers/tests/test_install.py index 0b142d90..645df605 100644 --- a/subiquity/server/controllers/tests/test_install.py +++ b/subiquity/server/controllers/tests/test_install.py @@ -251,7 +251,7 @@ class TestInstallController(unittest.IsolatedAsyncioTestCase): "efibootmgr", "--create", "--loader", - "\\EFI\\boot\\shimx64.efi", + "\\EFI\\boot\\bootx64.efi", "--disk", self.part.device.path, "--part", @@ -282,7 +282,7 @@ class TestInstallController(unittest.IsolatedAsyncioTestCase): "efibootmgr", "--create", "--loader", - "\\EFI\\boot\\shimx64.efi", + "\\EFI\\boot\\bootx64.efi", "--disk", self.part.device.path, "--part", @@ -312,7 +312,7 @@ class TestInstallController(unittest.IsolatedAsyncioTestCase): "efibootmgr", "--create", "--loader", - "\\EFI\\boot\\shimx64.efi", + "\\EFI\\boot\\bootx64.efi", "--disk", self.part.device.path, "--part", From 92f793ee9ba641966132836cd6af5133958ac7bd Mon Sep 17 00:00:00 2001 From: Michael Hudson-Doyle Date: Mon, 14 Aug 2023 09:12:47 +1200 Subject: [PATCH 3/7] simplify partition_by_partuuid --- subiquity/models/filesystem.py | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/subiquity/models/filesystem.py b/subiquity/models/filesystem.py index 65611297..563ba9a3 100644 --- a/subiquity/models/filesystem.py +++ b/subiquity/models/filesystem.py @@ -1821,15 +1821,7 @@ class FilesystemModel(object): return self._all(type="lvm_volgroup") def partition_by_partuuid(self, partuuid: str) -> Optional[Partition]: - # This can be simplified when - # https://code.launchpad.net/~mwhudson/curtin/+git/curtin/+merge/448842 - # lands. - for part in self.app.base_model.filesystem._all(type="partition"): - if part._info is None: - continue - if part._info.raw.get("ID_PART_ENTRY_UUID") == partuuid: - return part - return None + return self._one(type="partition", uuid=partuuid) def _remove(self, obj): _remove_backlinks(obj) From 3716f1259ff134289d8b72a4f66bb7753274a94f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yao=20Wei=20=28=E9=AD=8F=E9=8A=98=E5=BB=B7=29?= Date: Mon, 14 Aug 2023 17:45:36 +0800 Subject: [PATCH 4/7] fix: fix calling partition_by_partuuid --- subiquity/server/controllers/install.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/subiquity/server/controllers/install.py b/subiquity/server/controllers/install.py index 2e0ad9c9..c2a80269 100644 --- a/subiquity/server/controllers/install.py +++ b/subiquity/server/controllers/install.py @@ -435,7 +435,7 @@ class InstallController(SubiquityController): casper_uuid = fp.read().strip() rp_partuuid = self.app.kernel_cmdline.get("rp-partuuid") if casper_uuid is not None and rp_partuuid is not None: - rp = self.app.base_model.partition_by_partuuid(rp_partuuid) + rp = self.app.base_model.filesystem.partition_by_partuuid(rp_partuuid) if rp is not None: await self.configure_rp_boot( context=context, rp=rp, casper_uuid=casper_uuid From e0e2515eb58bbfa3e1a70ea95b0f13b7f372ba39 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yao=20Wei=20=28=E9=AD=8F=E9=8A=98=E5=BB=B7=29?= Date: Mon, 14 Aug 2023 17:46:13 +0800 Subject: [PATCH 5/7] fix: put EFI boot entry of recovery partition last of boot order --- subiquity/server/controllers/install.py | 2 +- subiquity/server/controllers/tests/test_install.py | 11 +++++++++-- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/subiquity/server/controllers/install.py b/subiquity/server/controllers/install.py index c2a80269..8661710d 100644 --- a/subiquity/server/controllers/install.py +++ b/subiquity/server/controllers/install.py @@ -557,7 +557,7 @@ class InstallController(SubiquityController): cmd = [ "efibootmgr", "--bootorder", - ",".join(efi_state_before.order), + ",".join(efi_state_before.order + [new_bootnum]), ] rp_bootnum = new_bootnum await self.app.command_runner.run(cmd) diff --git a/subiquity/server/controllers/tests/test_install.py b/subiquity/server/controllers/tests/test_install.py index 645df605..20197584 100644 --- a/subiquity/server/controllers/tests/test_install.py +++ b/subiquity/server/controllers/tests/test_install.py @@ -264,7 +264,7 @@ class TestInstallController(unittest.IsolatedAsyncioTestCase): [ "efibootmgr", "--bootorder", - "0000,0002", + "0000,0002,0003", ] ), ] @@ -295,7 +295,14 @@ class TestInstallController(unittest.IsolatedAsyncioTestCase): [ "efibootmgr", "--bootorder", - "0000,0002", + "0000,0002,0003", + ] + ), + call( + [ + "efibootmgr", + "--bootnext", + "0003", ] ), ] From 9c37efd534f71d6a1bbae14e35b73bb6691b9e31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yao=20Wei=20=28=E9=AD=8F=E9=8A=98=E5=BB=B7=29?= Date: Mon, 14 Aug 2023 17:46:55 +0800 Subject: [PATCH 6/7] fix: call update-grub after closing 99_reset --- subiquity/server/controllers/install.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/subiquity/server/controllers/install.py b/subiquity/server/controllers/install.py index 8661710d..fb6faf11 100644 --- a/subiquity/server/controllers/install.py +++ b/subiquity/server/controllers/install.py @@ -499,16 +499,16 @@ class InstallController(SubiquityController): with open(self.tpath("etc/grub.d/99_reset"), "w") as fp: os.chmod(fp.fileno(), 0o755) fp.write(conf) - await run_curtin_command( - self.app, - context, - "in-target", - "-t", - self.tpath(), - "--", - "update-grub", - private_mounts=False, - ) + await run_curtin_command( + self.app, + context, + "in-target", + "-t", + self.tpath(), + "--", + "update-grub", + private_mounts=False, + ) @with_context(description="configuring UEFI menu entry for factory reset") async def configure_rp_boot_uefi(self, context, rp: Partition): From 051b6cba1209b6d5bc93c8d1e35feb8cb2f5aac4 Mon Sep 17 00:00:00 2001 From: Michael Hudson-Doyle Date: Thu, 17 Aug 2023 14:23:16 +1200 Subject: [PATCH 7/7] extract code into a method to improve clarify, add comments --- subiquity/server/controllers/install.py | 35 +++++++++++++++++-------- 1 file changed, 24 insertions(+), 11 deletions(-) diff --git a/subiquity/server/controllers/install.py b/subiquity/server/controllers/install.py index 2e0ad9c9..0a623474 100644 --- a/subiquity/server/controllers/install.py +++ b/subiquity/server/controllers/install.py @@ -429,17 +429,7 @@ class InstallController(SubiquityController): context=context, rp=rp, casper_uuid=new_casper_uuid ) else: - casper_uuid = None - for casper_uuid_file in glob.glob("/cdrom/.disk/casper-uuid-*"): - with open(casper_uuid_file) as fp: - casper_uuid = fp.read().strip() - rp_partuuid = self.app.kernel_cmdline.get("rp-partuuid") - if casper_uuid is not None and rp_partuuid is not None: - rp = self.app.base_model.partition_by_partuuid(rp_partuuid) - if rp is not None: - await self.configure_rp_boot( - context=context, rp=rp, casper_uuid=casper_uuid - ) + await self.maybe_configure_exiting_rp_boot(context=context) async def adjust_rp(self, rp: Partition, mp: Mountpoint) -> str: if self.app.opts.dry_run: @@ -579,6 +569,29 @@ class InstallController(SubiquityController): return await self.configure_rp_boot_uefi(context=context, rp=rp) + async def maybe_configure_exiting_rp_boot(self, context): + # We are not creating a reset partition here if we are running + # from one we still want to configure booting from it. + + # Look for the command line argument added in adjust_rp) + # above. + rp_partuuid = self.app.kernel_cmdline.get("rp-partuuid") + if rp_partuuid is None: + # Most likely case: we are not running from an reset partition + return + rp = self.app.base_model.partition_by_partuuid(rp_partuuid) + if rp is None: + # This shouldn't happen, but don't crash. + return + casper_uuid = None + for casper_uuid_file in glob.glob("/cdrom/.disk/casper-uuid-*"): + with open(casper_uuid_file) as fp: + casper_uuid = fp.read().strip() + if casper_uuid is None: + # This also shouldn't happen, but, again, don't crash. + return + await self.configure_rp_boot(context=context, rp=rp, casper_uuid=casper_uuid) + @with_context(description="creating fstab") async def create_core_boot_classic_fstab(self, *, context): with open(self.tpath("etc/fstab"), "w") as fp: