From 72c7e8df430712410cbcaa9b426a88f49939a847 Mon Sep 17 00:00:00 2001 From: Michael Hudson-Doyle Date: Thu, 17 Nov 2022 12:39:44 +1300 Subject: [PATCH 1/4] reuse an existing partition if possible in apply_system This required a bit of a refactor which lead to rewriting tests as well as adding a new one. --- subiquity/server/controllers/filesystem.py | 69 +++++++----- .../controllers/tests/test_filesystem.py | 101 ++++++++++++------ subiquity/server/snapdapi.py | 5 +- 3 files changed, 118 insertions(+), 57 deletions(-) diff --git a/subiquity/server/controllers/filesystem.py b/subiquity/server/controllers/filesystem.py index 94c5e8ac..930b1800 100644 --- a/subiquity/server/controllers/filesystem.py +++ b/subiquity/server/controllers/filesystem.py @@ -452,43 +452,60 @@ class FilesystemController(SubiquityController, FilesystemManipulator): for structure in volume.structure: if structure.role == snapdapi.Role.MBR: continue - if ',' not in structure.type: - continue if structure.offset is not None: offset = structure.offset yield (structure, offset, structure.size) offset = offset + structure.size - def _add_structure(self, *, disk: Disk, offset: int, size: int, - is_last: bool, structure: snapdapi.VolumeStructure): - if structure.role == snapdapi.Role.SYSTEM_DATA and is_last: - size = gaps.largest_gap(disk).size - flag = ptable_uuid_to_flag_entry(structure.gpt_part_uuid())[0] - part = self.model.add_partition( - disk, offset=offset, size=size, flag=flag, - partition_name=structure.name) - if structure.filesystem: - part.wipe = 'superblock' - fs = self.model.add_filesystem( - part, structure.filesystem, label=structure.label) - if structure.role == snapdapi.Role.SYSTEM_DATA: - self.model.add_mount(fs, '/') - elif structure.role == snapdapi.Role.SYSTEM_BOOT: - self.model.add_mount(fs, '/boot') - elif flag == 'boot': - self.model.add_mount(fs, '/boot/efi') - self._role_to_device[structure.role] = part - def apply_system(self, disk_id): disk = self.model._one(id=disk_id) - self.reformat(disk) [volume] = self._system.volumes.values() + preserved_parts = set() + + if volume.schema != disk.ptable: + self.reformat(disk) + disk.ptable = volume.schema + parts_by_offset_size = {} + else: + parts_by_offset_size = { + (part.offset, part.size): part for part in disk.partitions() + } + + for _struct, offset, size in self._offsets_and_sizes_for_system(): + if (offset, size) in parts_by_offset_size: + preserved_parts.add(parts_by_offset_size[(offset, size)]) + + for part in disk.partitions(): + if part not in preserved_parts: + self.delete_partition(part) + del parts_by_offset_size[(part.offset, part.size)] + for structure, offset, size in self._offsets_and_sizes_for_system(): - self._add_structure( - disk=disk, offset=offset, size=size, structure=structure, - is_last=structure == volume.structure[-1]) + if (offset, size) in parts_by_offset_size: + part = parts_by_offset_size[(offset, size)] + else: + if structure.role == snapdapi.Role.SYSTEM_DATA and \ + structure == volume.structure[-1]: + gap = gaps.largest_gap(disk) + size = gap.size - (offset - gap.offset) + part = self.model.add_partition(disk, offset=offset, size=size) + part.flag = ptable_uuid_to_flag_entry(structure.gpt_part_uuid())[0] + if structure.name: + part.partition_name = structure.name + if structure.filesystem: + part.wipe = 'superblock' + fs = self.model.add_filesystem( + part, structure.filesystem, label=structure.label) + if structure.role == snapdapi.Role.SYSTEM_DATA: + self.model.add_mount(fs, '/') + elif structure.role == snapdapi.Role.SYSTEM_BOOT: + self.model.add_mount(fs, '/boot') + elif part.flag == 'boot': + self.model.add_mount(fs, '/boot/efi') + self._role_to_device[structure.role] = part + disk._partitions.sort(key=lambda p: p.number) def _on_volumes(self) -> Dict[str, snapdapi.OnVolume]: diff --git a/subiquity/server/controllers/tests/test_filesystem.py b/subiquity/server/controllers/tests/test_filesystem.py index 7786c561..ef95dba3 100644 --- a/subiquity/server/controllers/tests/test_filesystem.py +++ b/subiquity/server/controllers/tests/test_filesystem.py @@ -476,45 +476,86 @@ class TestCoreBootInstallMethods(IsolatedAsyncioTestCase): self.fsc._configured = True self.fsc.model = make_model(Bootloader.UEFI) - def test_add_structure(self): + def _details_for_structures(self, structures): + return snapdapi.SystemDetails( + volumes={'pc': snapdapi.Volume(schema='gpt', structure=structures)} + ) + + def test_apply_system(self): disk = make_disk(self.fsc.model) - self.fsc._add_structure( - disk=disk, - offset=100, - size=2 << 20, - is_last=False, - structure=snapdapi.VolumeStructure( - name='ptname', + self.fsc._system = self._details_for_structures([ + snapdapi.VolumeStructure( type="83,0FC63DAF-8483-4772-8E79-3D69D8477DE4", - label='label', - size=1 << 20, - role=snapdapi.Role.SYSTEM_DATA, - filesystem='ext4')) + offset=1 << 20, + size=1 << 30, + filesystem='ext4'), + ]) + self.fsc.apply_system(disk.id) [part] = disk.partitions() - self.assertEqual(part.offset, 100) + self.assertEqual(part.offset, 1 << 20) + self.assertEqual(part.size, 1 << 30) + self.assertEqual(part.fs().fstype, 'ext4') + + def test_apply_system_reuse(self): + disk = make_disk(self.fsc.model) + # Add a partition that matches one in the volume structure + reused_part = make_partition( + self.fsc.model, disk, offset=1 << 20, size=1 << 30, preserve=True) + # And one that does not. + make_partition( + self.fsc.model, disk, offset=2 << 30, size=1 << 30, preserve=True) + self.fsc._system = self._details_for_structures([ + snapdapi.VolumeStructure( + type="0FC63DAF-8483-4772-8E79-3D69D8477DE4", + offset=1 << 20, + size=1 << 30, + filesystem='ext4'), + ]) + self.fsc.apply_system(disk.id) + [part] = disk.partitions() + self.assertEqual(reused_part, part) + self.assertEqual(reused_part.wipe, 'superblock') + self.assertEqual(part.fs().fstype, 'ext4') + + def test_apply_system_reuse_no_format(self): + disk = make_disk(self.fsc.model) + existing_part = make_partition( + self.fsc.model, disk, offset=1 << 20, size=1 << 30, preserve=True) + self.fsc._system = self._details_for_structures([ + snapdapi.VolumeStructure( + type="0FC63DAF-8483-4772-8E79-3D69D8477DE4", + offset=1 << 20, + size=1 << 30, + filesystem=None), + ]) + self.fsc.apply_system(disk.id) + [part] = disk.partitions() + self.assertEqual(existing_part, part) + self.assertEqual(existing_part.wipe, None) + + def test_apply_system_system_data(self): + disk = make_disk(self.fsc.model) + self.fsc._system = self._details_for_structures([ + snapdapi.VolumeStructure( + type="0FC63DAF-8483-4772-8E79-3D69D8477DE4", + offset=2 << 20, + name='ptname', + size=2 << 30, + role=snapdapi.Role.SYSTEM_DATA, + filesystem='ext4'), + ]) + self.fsc.apply_system(disk.id) + [part] = disk.partitions() + self.assertEqual(part.offset, 2 << 20) self.assertEqual(part.partition_name, 'ptname') self.assertEqual(part.flag, 'linux') - self.assertEqual(part.size, 2 << 20) + self.assertEqual( + part.size, + disk.size - (2 << 20) - disk.alignment_data().min_end_offset) self.assertEqual(part.fs().fstype, 'ext4') self.assertEqual(part.fs().mount().path, '/') self.assertEqual(part.wipe, 'superblock') - def test_add_structure_no_fs(self): - disk = make_disk(self.fsc.model) - self.fsc._add_structure( - disk=disk, - offset=100, - size=2 << 20, - is_last=False, - structure=snapdapi.VolumeStructure( - type="83,0FC63DAF-8483-4772-8E79-3D69D8477DE4", - size=1 << 20, - filesystem=None)) - [part] = disk.partitions() - self.assertEqual(part.size, 2 << 20) - self.assertEqual(part.fs(), None) - self.assertEqual(part.wipe, None) - async def test_from_sample_data(self): # calling this a unit test is definitely questionable. but it # runs much more quickly than the integration test! diff --git a/subiquity/server/snapdapi.py b/subiquity/server/snapdapi.py index 14549955..07da716f 100644 --- a/subiquity/server/snapdapi.py +++ b/subiquity/server/snapdapi.py @@ -142,7 +142,10 @@ class VolumeStructure: update: VolumeUpdate = attr.Factory(VolumeUpdate) def gpt_part_uuid(self): - return self.type.split(',', 1)[1].upper() + if ',' in self.type: + return self.type.split(',', 1)[1].upper() + else: + return self.type @attr.s(auto_attribs=True) From f13084169f335655c56b3f768eff13d2871cd3f9 Mon Sep 17 00:00:00 2001 From: Michael Hudson-Doyle Date: Thu, 17 Nov 2022 14:24:46 +1300 Subject: [PATCH 2/4] fix reusing a formatted partition --- subiquity/server/controllers/filesystem.py | 1 + subiquity/server/controllers/tests/test_filesystem.py | 1 + 2 files changed, 2 insertions(+) diff --git a/subiquity/server/controllers/filesystem.py b/subiquity/server/controllers/filesystem.py index 930b1800..776940eb 100644 --- a/subiquity/server/controllers/filesystem.py +++ b/subiquity/server/controllers/filesystem.py @@ -496,6 +496,7 @@ class FilesystemController(SubiquityController, FilesystemManipulator): part.partition_name = structure.name if structure.filesystem: part.wipe = 'superblock' + self.delete_filesystem(part.fs()) fs = self.model.add_filesystem( part, structure.filesystem, label=structure.label) if structure.role == snapdapi.Role.SYSTEM_DATA: diff --git a/subiquity/server/controllers/tests/test_filesystem.py b/subiquity/server/controllers/tests/test_filesystem.py index ef95dba3..9887b849 100644 --- a/subiquity/server/controllers/tests/test_filesystem.py +++ b/subiquity/server/controllers/tests/test_filesystem.py @@ -501,6 +501,7 @@ class TestCoreBootInstallMethods(IsolatedAsyncioTestCase): # Add a partition that matches one in the volume structure reused_part = make_partition( self.fsc.model, disk, offset=1 << 20, size=1 << 30, preserve=True) + self.fsc.model.add_filesystem(reused_part, 'ext4') # And one that does not. make_partition( self.fsc.model, disk, offset=2 << 30, size=1 << 30, preserve=True) From 368d49cca1825a06b38c9469f66a160b830a853d Mon Sep 17 00:00:00 2001 From: Michael Hudson-Doyle Date: Thu, 17 Nov 2022 14:40:24 +1300 Subject: [PATCH 3/4] FilesystemController.on_devices should always refer to underlying device --- subiquity/server/controllers/filesystem.py | 1 - .../server/controllers/tests/test_filesystem.py | 13 ------------- 2 files changed, 14 deletions(-) diff --git a/subiquity/server/controllers/filesystem.py b/subiquity/server/controllers/filesystem.py index 776940eb..cf5164cf 100644 --- a/subiquity/server/controllers/filesystem.py +++ b/subiquity/server/controllers/filesystem.py @@ -543,7 +543,6 @@ class FilesystemController(SubiquityController, FilesystemManipulator): for fs in self.model._all(type='format'): if fs.volume == part: fs.volume = arb_device - self._role_to_device[role] = arb_device @with_context(description="making system bootable") async def finish_install(self, context): diff --git a/subiquity/server/controllers/tests/test_filesystem.py b/subiquity/server/controllers/tests/test_filesystem.py index 9887b849..b6344d3d 100644 --- a/subiquity/server/controllers/tests/test_filesystem.py +++ b/subiquity/server/controllers/tests/test_filesystem.py @@ -607,16 +607,3 @@ class TestCoreBootInstallMethods(IsolatedAsyncioTestCase): request = call.args[2] self.assertEqual(request.action, snapdapi.SystemAction.INSTALL) self.assertEqual(request.step, snapdapi.SystemActionStep.FINISH) - [on_volume] = request.on_volumes.values() - role_to_path = {} - for s in on_volume.structure: - if s.role in [snapdapi.Role.SYSTEM_DATA, - snapdapi.Role.SYSTEM_BOOT, - snapdapi.Role.SYSTEM_SEED]: - role_to_path[s.role] = s.device - expected_role_to_path = { - snapdapi.Role.SYSTEM_DATA: mounts['/'].path, - snapdapi.Role.SYSTEM_BOOT: mounts['/boot'].path, - snapdapi.Role.SYSTEM_SEED: mounts['/boot/efi'].path, - } - self.assertEqual(expected_role_to_path, role_to_path) From 97901a78a254ff3aa55e97a578dceae4ea2e3574 Mon Sep 17 00:00:00 2001 From: Michael Hudson-Doyle Date: Thu, 17 Nov 2022 22:24:22 +1300 Subject: [PATCH 4/4] fix answers race (maybe) --- subiquity/client/controllers/filesystem.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/subiquity/client/controllers/filesystem.py b/subiquity/client/controllers/filesystem.py index 32facffe..08ea373c 100644 --- a/subiquity/client/controllers/filesystem.py +++ b/subiquity/client/controllers/filesystem.py @@ -107,8 +107,8 @@ class FilesystemController(SubiquityTuiController, FilesystemManipulator): await asyncio.sleep(0.1) if self.answers['tpm-default']: - self.ui.body.done(self.ui.body.form) await self.app.confirm_install() + self.ui.body.done(self.ui.body.form) if self.answers['guided']: if 'guided-index' in self.answers: disk = self.ui.body.form.disks[self.answers['guided-index']]