From b2d5fffcc0a63a00ddfa46cb31bb8f73299b660c Mon Sep 17 00:00:00 2001 From: Dan Bungert Date: Thu, 16 Jun 2022 17:27:37 -0600 Subject: [PATCH 1/3] filesystem: guided_method refactoring Update the guided_methods, particularly guided_direct. It now does a direct call to create_partition, bypassing the increasingly complicated partition_disk_handler, making it more similar to guided_lvm, and making way for guided partitioning into a gap. --- subiquity/common/filesystem/manipulator.py | 10 ++++--- subiquity/server/controllers/filesystem.py | 34 ++++++++++------------ 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/subiquity/common/filesystem/manipulator.py b/subiquity/common/filesystem/manipulator.py index 8a171cde..2daa9213 100644 --- a/subiquity/common/filesystem/manipulator.py +++ b/subiquity/common/filesystem/manipulator.py @@ -152,20 +152,22 @@ class FilesystemManipulator: return getattr(self, 'delete_' + obj.type)(obj) - def clear(self, obj): + def clear(self, obj, wipe=None): if obj.type == "disk": obj.preserve = False - obj.wipe = 'superblock' + if wipe is None: + wipe = 'superblock' + obj.wipe = wipe for subobj in obj.fs(), obj.constructed_device(): self.delete(subobj) - def reformat(self, disk, ptable=None): + def reformat(self, disk, ptable=None, wipe=None): disk.grub_device = False if ptable is not None: disk.ptable = ptable for p in list(disk.partitions()): self.delete_partition(p, True) - self.clear(disk) + self.clear(disk, wipe) def can_resize_partition(self, partition): if not partition.preserve: diff --git a/subiquity/server/controllers/filesystem.py b/subiquity/server/controllers/filesystem.py index 53927442..c9d77b3d 100644 --- a/subiquity/server/controllers/filesystem.py +++ b/subiquity/server/controllers/filesystem.py @@ -133,28 +133,26 @@ class FilesystemController(SubiquityController, FilesystemManipulator): "autoinstall config did not create needed bootloader " "partition") - def guided_direct(self, disk): - self.reformat(disk) - gap = gaps.largest_gap(disk) - spec = { - "size": gap.size, - "fstype": "ext4", - "mount": "/", - } - self.partition_disk_handler(disk, spec, gap=gap) - - def guided_lvm(self, disk, lvm_options=None): - self.reformat(disk) + def setup_disk_for_guided(self, disk): + self.reformat(disk, wipe='superblock-recursive') if DeviceAction.TOGGLE_BOOT in DeviceAction.supported(disk): self.add_boot_disk(disk) - gap = gaps.largest_gap(disk) - size = sizes.get_bootfs_size(gap.size) - gap_boot, gap_rest = gap.split(size) - spec = dict(size=size, fstype="ext4", mount='/boot') + return gaps.largest_gap(disk) + + def guided_direct(self, disk): + gap = self.setup_disk_for_guided(disk) + spec = dict(fstype="ext4", mount="/") + self.create_partition(device=disk, gap=gap, spec=spec) + + def guided_lvm(self, disk, lvm_options=None): + gap = self.setup_disk_for_guided(disk) + gap_boot, gap_rest = gap.split(sizes.get_bootfs_size(gap.size)) + spec = dict(fstype="ext4", mount='/boot') self.create_partition(device=disk, gap=gap_boot, spec=spec) - spec = dict(size=gap_rest.size, fstype=None) - part = self.create_partition(device=disk, gap=gap_rest, spec=spec) + part = self.create_partition( + device=disk, gap=gap_rest, spec=dict(fstype=None)) + vg_name = 'ubuntu-vg' i = 0 while self.model._one(type='lvm_volgroup', name=vg_name) is not None: From d622c383f65d06522615039b1a885947eab7959b Mon Sep 17 00:00:00 2001 From: Dan Bungert Date: Thu, 16 Jun 2022 17:30:09 -0600 Subject: [PATCH 2/3] filesystem: offer autoinstall use_gap Accept a new 'mode' parameter when using autoinstall guided storage. The default mode is 'reformat_disk', which acts as today. A new mode of 'use_gap' is available, which does not reformat the disk. --- subiquity/server/controllers/filesystem.py | 50 +++++++++++++------ .../controllers/tests/test_filesystem.py | 20 ++++++++ 2 files changed, 54 insertions(+), 16 deletions(-) diff --git a/subiquity/server/controllers/filesystem.py b/subiquity/server/controllers/filesystem.py index c9d77b3d..24df42f2 100644 --- a/subiquity/server/controllers/filesystem.py +++ b/subiquity/server/controllers/filesystem.py @@ -133,19 +133,20 @@ class FilesystemController(SubiquityController, FilesystemManipulator): "autoinstall config did not create needed bootloader " "partition") - def setup_disk_for_guided(self, disk): - self.reformat(disk, wipe='superblock-recursive') + def setup_disk_for_guided(self, disk, mode): + if mode is None or mode == 'reformat_disk': + self.reformat(disk, wipe='superblock-recursive') if DeviceAction.TOGGLE_BOOT in DeviceAction.supported(disk): self.add_boot_disk(disk) return gaps.largest_gap(disk) - def guided_direct(self, disk): - gap = self.setup_disk_for_guided(disk) + def guided_direct(self, disk, mode=None): + gap = self.setup_disk_for_guided(disk, mode) spec = dict(fstype="ext4", mount="/") self.create_partition(device=disk, gap=gap, spec=spec) - def guided_lvm(self, disk, lvm_options=None): - gap = self.setup_disk_for_guided(disk) + def guided_lvm(self, disk, mode=None, lvm_options=None): + gap = self.setup_disk_for_guided(disk, mode) gap_boot, gap_rest = gap.split(sizes.get_bootfs_size(gap.size)) spec = dict(fstype="ext4", mount='/boot') self.create_partition(device=disk, gap=gap_boot, spec=spec) @@ -198,7 +199,7 @@ class FilesystemController(SubiquityController, FilesystemManipulator): 'password': choice.password, }, } - self.guided_lvm(disk, lvm_options) + self.guided_lvm(disk, lvm_options=lvm_options) else: self.guided_direct(disk) @@ -459,19 +460,36 @@ class FilesystemController(SubiquityController, FilesystemManipulator): continue break + def run_guided(self, layout): + guided_method = getattr(self, "guided_" + layout['name']) + mode = layout.get('mode', 'reformat_disk') + self.validate_layout_mode(mode) + + if mode == 'reformat_disk': + match = layout.get("match", {'size': 'largest'}) + disk = self.model.disk_for_match(self.model.all_disks(), match) + if not disk: + raise Exception("autoinstall cannot configure storage " + "- no disk found large enough for install") + elif mode == 'use_gap': + gap = gaps.largest_gap(self.model.all_disks()) + if not gap: + raise Exception("autoinstall cannot configure storage " + "- no gap found large enough for install") + # This is not necessarily the exact gap to be used, as the gap size + # may change once add_boot_disk has sorted things out. + disk = gap.device + guided_method(disk=disk, mode=mode) + + def validate_layout_mode(self, mode): + if mode not in ('reformat_disk', 'use_gap'): + raise ValueError(f'Unknown layout mode {mode}') + @with_context() def convert_autoinstall_config(self, context=None): log.debug("self.ai_data = %s", self.ai_data) if 'layout' in self.ai_data: - layout = self.ai_data['layout'] - meth = getattr(self, "guided_" + layout['name']) - disk = self.model.disk_for_match( - self.model.all_disks(), - layout.get("match", {'size': 'largest'})) - if not disk: - raise Exception("autoinstall cannot configure storage " - "- no disk found large enough for install") - meth(disk) + self.run_guided(self.ai_data['layout']) elif 'config' in self.ai_data: self.model.apply_autoinstall_config(self.ai_data['config']) self.model.grub = self.ai_data.get('grub') diff --git a/subiquity/server/controllers/tests/test_filesystem.py b/subiquity/server/controllers/tests/test_filesystem.py index 41307a92..f73e33c4 100644 --- a/subiquity/server/controllers/tests/test_filesystem.py +++ b/subiquity/server/controllers/tests/test_filesystem.py @@ -15,6 +15,8 @@ from unittest import mock, TestCase +from parameterized import parameterized + from subiquity.server.controllers.filesystem import FilesystemController from subiquitycore.tests.util import run_coro @@ -103,3 +105,21 @@ class TestGuided(TestCase): self.assertEqual(None, d1p1.mount) self.assertEqual('/', d1p2.mount) self.assertEqual(d1p1.size + d1p1.offset, d1p2.offset) + + +class TestLayout(TestCase): + def setUp(self): + self.app = make_app() + self.app.opts.bootloader = 'UEFI' + self.app.report_start_event = mock.Mock() + self.app.report_finish_event = mock.Mock() + self.fsc = FilesystemController(app=self.app) + + @parameterized.expand([('reformat_disk',), ('use_gap',)]) + def test_good_modes(self, mode): + self.fsc.validate_layout_mode(mode) + + @parameterized.expand([('resize_biggest',), ('use_free',)]) + def test_bad_modes(self, mode): + with self.assertRaises(ValueError): + self.fsc.validate_layout_mode(mode) From 8e529ca1ad4df5af9799f496d58d0895496827d2 Mon Sep 17 00:00:00 2001 From: Dan Bungert Date: Fri, 17 Jun 2022 09:38:29 -0600 Subject: [PATCH 3/3] filesystem: check use_gap devices can be boot --- subiquity/server/controllers/filesystem.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/subiquity/server/controllers/filesystem.py b/subiquity/server/controllers/filesystem.py index 24df42f2..0e00abec 100644 --- a/subiquity/server/controllers/filesystem.py +++ b/subiquity/server/controllers/filesystem.py @@ -472,7 +472,9 @@ class FilesystemController(SubiquityController, FilesystemManipulator): raise Exception("autoinstall cannot configure storage " "- no disk found large enough for install") elif mode == 'use_gap': - gap = gaps.largest_gap(self.model.all_disks()) + bootable = [d for d in self.model.all_disks() + if boot.can_be_boot_device(d, with_reformatting=False)] + gap = gaps.largest_gap(bootable) if not gap: raise Exception("autoinstall cannot configure storage " "- no gap found large enough for install")