From 5aa3bd3560ec2ab6605a4e61b636b244f4f284b6 Mon Sep 17 00:00:00 2001 From: Dan Bungert Date: Tue, 21 Jun 2022 12:29:22 -0600 Subject: [PATCH 1/2] filesystem: better partition numbering With a mix of preserved and non-preserved partitions, we must not allocate a partition number already in use. --- subiquity/models/filesystem.py | 12 +++-- subiquity/models/tests/test_filesystem.py | 59 +++++++++++++++++++++- subiquity/tests/api/test_api.py | 61 +++++++++++++---------- 3 files changed, 102 insertions(+), 30 deletions(-) diff --git a/subiquity/models/filesystem.py b/subiquity/models/filesystem.py index 35cbb00b..746bf7ee 100644 --- a/subiquity/models/filesystem.py +++ b/subiquity/models/filesystem.py @@ -689,10 +689,16 @@ class Partition(_Formattable): @property def _number(self): - if self.preserve: + if self.number is not None: return self.number - else: - return self.device._partitions.index(self) + 1 + used_nums = {part.number for part in self.device._partitions + if part.number is not None} + possible_nums = {i for i in range(1, len(self.device._partitions) + 1)} + unused_nums = sorted(list(possible_nums - used_nums)) + for part in self.device._partitions: + if part.number is None: + part.number = unused_nums.pop(0) + return self.number def _path(self): return partition_kname(self.device.path, self._number) diff --git a/subiquity/models/tests/test_filesystem.py b/subiquity/models/tests/test_filesystem.py index 1d29576e..50bf248b 100644 --- a/subiquity/models/tests/test_filesystem.py +++ b/subiquity/models/tests/test_filesystem.py @@ -177,7 +177,11 @@ def make_partition(model, device=None, *, preserve=False, size=None, partition = Partition(m=model, device=device, size=size, offset=offset, preserve=preserve, **kw) if preserve: - partition.number = len(device._partitions) + number = kw.get('number') + if number is not None: + partition.number = number + else: + partition.number = len(device._partitions) model._actions.append(partition) return partition @@ -692,3 +696,56 @@ class TestAlignmentData(unittest.TestCase): # information, so gaps produces numbers that are too small by 1MiB # for ptable != 'gpt' self.assertTrue(gaps_max <= align_max, f'ptable={ptable}') + + +class TestPartitionNumbering(unittest.TestCase): + def test_basic(self): + m, d1 = make_model_and_disk(ptable='gpt') + p1 = make_partition(m, d1) + p2 = make_partition(m, d1) + p3 = make_partition(m, d1) + self.assertEqual(1, p1._number) + self.assertEqual(2, p2._number) + self.assertEqual(3, p3._number) + + def test_p1_preserved(self): + m = make_model() + m.storage_version = 2 + d1 = make_disk(m, ptable='gpt') + p1 = make_partition(m, d1, preserve=True, number=1) + p2 = make_partition(m, d1) + p3 = make_partition(m, d1) + self.assertEqual(1, p1._number) + self.assertEqual(True, p1.preserve) + self.assertEqual(2, p2._number) + self.assertEqual(False, p2.preserve) + self.assertEqual(3, p3._number) + self.assertEqual(False, p3.preserve) + + def test_p2_preserved(self): + m = make_model() + m.storage_version = 2 + d1 = make_disk(m, ptable='gpt') + p2 = make_partition(m, d1, preserve=True, number=2) + p1 = make_partition(m, d1) + p3 = make_partition(m, d1) + self.assertEqual(1, p1._number) + self.assertEqual(False, p1.preserve) + self.assertEqual(2, p2._number) + self.assertEqual(True, p2.preserve) + self.assertEqual(3, p3._number) + self.assertEqual(False, p3.preserve) + + def test_trigger_part_num_allocation_for_disk(self): + m = make_model() + d1 = make_disk(m, ptable='gpt') + p1 = make_partition(m, d1) + p2 = make_partition(m, d1) + p3 = make_partition(m, d1) + self.assertIsNone(p1.number) + self.assertIsNone(p2.number) + self.assertIsNone(p3.number) + self.assertEqual(3, p3._number) + self.assertEqual(1, p1.number) + self.assertEqual(2, p2.number) + self.assertEqual(3, p3.number) diff --git a/subiquity/tests/api/test_api.py b/subiquity/tests/api/test_api.py index 911d6756..2ce8aa95 100755 --- a/subiquity/tests/api/test_api.py +++ b/subiquity/tests/api/test_api.py @@ -299,36 +299,34 @@ class TestFlow(TestAPI): } } add_resp = await inst.post('/storage/v2/add_partition', data) - sda = first(add_resp['disks'], 'id', disk_id) - sda2 = first(sda['partitions'], 'number', 2) - self.assertEqual('ext3', sda2['format']) + [sda] = add_resp['disks'] + [root] = match(sda['partitions'], mount='/') + self.assertEqual('ext3', root['format']) data = { 'disk_id': disk_id, 'partition': { - 'number': 2, + 'number': root['number'], 'format': 'ext4', } } edit_resp = await inst.post('/storage/v2/edit_partition', data) - add_sda = first(add_resp['disks'], 'id', disk_id) - add_sda2 = first(add_sda['partitions'], 'number', 2) + [add_sda] = add_resp['disks'] + [add_root] = match(add_sda['partitions'], mount='/') - edit_sda = first(edit_resp['disks'], 'id', disk_id) - edit_sda2 = first(edit_sda['partitions'], 'number', 2) + [edit_sda] = edit_resp['disks'] + [edit_root] = match(edit_sda['partitions'], mount='/') for key in 'size', 'number', 'mount', 'boot': - self.assertEqual(add_sda2[key], edit_sda2[key], key) - self.assertEqual('ext4', edit_sda2['format']) + self.assertEqual(add_root[key], edit_root[key], key) + self.assertEqual('ext4', edit_root['format']) del_resp = await inst.post('/storage/v2/delete_partition', data) - sda = first(del_resp['disks'], 'id', disk_id) - self.assertEqual(2, len(sda['partitions'])) - - for type in 'Partition', 'Gap': - pgs = [pg for pg in sda['partitions'] if pg['$type'] == type] - self.assertEqual(len(pgs), 1) + [sda] = del_resp['disks'] + [p, g] = sda['partitions'] + self.assertEqual('Partition', p['$type']) + self.assertEqual('Gap', g['$type']) reset_resp = await inst.post('/storage/v2/reset') self.assertEqual(orig_resp, reset_resp) @@ -374,8 +372,9 @@ class TestAdd(TestAPI): await inst.post('/storage/v2/reset') - # these manual steps are expected to be equivalent to just adding - # the single partition and getting the automatic boot partition + # these manual steps are expected to be mostly equivalent to just + # adding the single partition and getting the automatic boot + # partition resp = await inst.post( '/storage/v2/add_boot_partition', disk_id=disk_id) sda = first(resp['disks'], 'id', disk_id) @@ -390,6 +389,15 @@ class TestAdd(TestAPI): } manual_add = await inst.post('/storage/v2/add_partition', data) + # the only difference is the partition number assigned - when we + # explicitly add_boot_partition, that is the first partition + # created, versus when we add_partition and get a boot partition + # implicitly + for resp in single_add, manual_add: + for part in resp['disks'][0]['partitions']: + part.pop('number') + part.pop('path') + self.assertEqual(single_add, manual_add) @timeout() @@ -978,10 +986,11 @@ class TestGap(TestAPI): } } resp = await inst.post('/storage/v2/add_partition', data) - sda = first(resp['disks'], 'id', 'disk-sda') - boot = first(sda['partitions'], 'number', 1) - gap = sda['partitions'][2] - expected = (10 << 30) - boot['size'] - (4 << 30) - (2 << 20) + [sda] = resp['disks'] + [boot] = match(sda['partitions'], mount='/boot/efi') + [p1, p2, gap] = sda['partitions'] + self.assertEqual('Gap', gap['$type']) + expected = (10 << 30) - p1['size'] - p2['size'] - (2 << 20) self.assertEqual(expected, gap['size']) async def SKIP_test_two_gaps(self): @@ -1040,10 +1049,10 @@ class TestRegression(TestAPI): } } resp = await inst.post('/storage/v2/add_partition', data) - sda = first(resp['disks'], 'id', disk_id) - sda2 = first(sda['partitions'], 'number', 2) - sda2.update({'format': 'ext3', 'mount': '/bar'}) - data['partition'] = sda2 + [sda] = resp['disks'] + [part] = match(sda['partitions'], mount='/foo') + part.update({'format': 'ext3', 'mount': '/bar'}) + data['partition'] = part data.pop('gap') await inst.post('/storage/v2/edit_partition', data) # should not throw an exception complaining about boot From 40a6ff71d921152c801526b3f7ad4c84bf6ca224 Mon Sep 17 00:00:00 2001 From: Dan Bungert Date: Wed, 22 Jun 2022 09:34:32 -0600 Subject: [PATCH 2/2] filesystem: number partition on creation --- subiquity/common/filesystem/actions.py | 2 +- subiquity/common/filesystem/labels.py | 6 ++-- subiquity/models/filesystem.py | 40 ++++++++++++---------- subiquity/models/tests/test_filesystem.py | 38 +++++--------------- subiquity/server/controllers/filesystem.py | 2 +- 5 files changed, 35 insertions(+), 53 deletions(-) diff --git a/subiquity/common/filesystem/actions.py b/subiquity/common/filesystem/actions.py index 6c55fd24..e1274d0f 100644 --- a/subiquity/common/filesystem/actions.py +++ b/subiquity/common/filesystem/actions.py @@ -304,7 +304,7 @@ def _can_delete_raid_vg(device): "Cannot delete {devicelabel} as partition {partnum} is part " "of the {cdtype} {cdname}.").format( devicelabel=labels.label(device), - partnum=p._number, + partnum=p.number, cdtype=labels.desc(cd), cdname=labels.label(cd), ) diff --git a/subiquity/common/filesystem/labels.py b/subiquity/common/filesystem/labels.py index 742f7f33..f7a7fe6b 100644 --- a/subiquity/common/filesystem/labels.py +++ b/subiquity/common/filesystem/labels.py @@ -181,10 +181,10 @@ def _label_partition(partition, *, short=False): else: p = "" if short: - return p + _("partition {number}").format(number=partition._number) + return p + _("partition {number}").format(number=partition.number) else: return p + _("partition {number} of {device}").format( - number=partition._number, device=label(partition.device)) + number=partition.number, device=label(partition.device)) @label.register(gaps.Gap) @@ -309,7 +309,7 @@ def _for_client_disk(disk, *, min_size=0): def _for_client_partition(partition, *, min_size=0): return types.Partition( size=partition.size, - number=partition._number, + number=partition.number, wipe=partition.wipe, preserve=partition.preserve, grub_device=partition.grub_device, diff --git a/subiquity/models/filesystem.py b/subiquity/models/filesystem.py index 746bf7ee..b28230a0 100644 --- a/subiquity/models/filesystem.py +++ b/subiquity/models/filesystem.py @@ -115,9 +115,18 @@ def fsobj__repr(obj): return "{}({})".format(type(obj).__name__, ", ".join(args)) +def _do_post_inits(obj): + for fn in obj._post_inits: + fn(obj) + + def fsobj(typ): def wrapper(c): - c.__attrs_post_init__ = _set_backlinks + c.__attrs_post_init__ = _do_post_inits + c._post_inits = [_set_backlinks] + class_post_init = getattr(c, '__post_init__', None) + if class_post_init is not None: + c._post_inits.append(class_post_init) c.type = attributes.const(typ) c.id = attr.ib(default=None) c._m = attr.ib(repr=None, default=None) @@ -675,6 +684,15 @@ class Partition(_Formattable): resize = attr.ib(default=None) partition_type = attr.ib(default=None) + def __post_init__(self): + if self.number is not None: + return + used_nums = {part.number for part in self.device._partitions + if part.number is not None} + possible_nums = {i for i in range(1, len(self.device._partitions) + 1)} + unused_nums = sorted(list(possible_nums - used_nums)) + self.number = unused_nums.pop(0) + def available(self): if self.flag in ['bios_grub', 'prep'] or self.grub_device: return False @@ -684,24 +702,8 @@ class Partition(_Formattable): return True return self._fs._available() - def serialize_number(self): - return {'number': self._number} - - @property - def _number(self): - if self.number is not None: - return self.number - used_nums = {part.number for part in self.device._partitions - if part.number is not None} - possible_nums = {i for i in range(1, len(self.device._partitions) + 1)} - unused_nums = sorted(list(possible_nums - used_nums)) - for part in self.device._partitions: - if part.number is None: - part.number = unused_nums.pop(0) - return self.number - def _path(self): - return partition_kname(self.device.path, self._number) + return partition_kname(self.device.path, self.number) @property def boot(self): @@ -1267,7 +1269,7 @@ class FilesystemModel(object): if obj.type == "partition": ensure_partitions(obj.device) for p in obj.device.partitions(): - if p._number < obj._number and p.id not in emitted_ids: + if p.number < obj.number and p.id not in emitted_ids: return False for dep in dependencies(obj): if dep.id not in emitted_ids: diff --git a/subiquity/models/tests/test_filesystem.py b/subiquity/models/tests/test_filesystem.py index 50bf248b..519c57ed 100644 --- a/subiquity/models/tests/test_filesystem.py +++ b/subiquity/models/tests/test_filesystem.py @@ -176,12 +176,6 @@ def make_partition(model, device=None, *, preserve=False, size=None, offset = gap.offset partition = Partition(m=model, device=device, size=size, offset=offset, preserve=preserve, **kw) - if preserve: - number = kw.get('number') - if number is not None: - partition.number = number - else: - partition.number = len(device._partitions) model._actions.append(partition) return partition @@ -704,9 +698,9 @@ class TestPartitionNumbering(unittest.TestCase): p1 = make_partition(m, d1) p2 = make_partition(m, d1) p3 = make_partition(m, d1) - self.assertEqual(1, p1._number) - self.assertEqual(2, p2._number) - self.assertEqual(3, p3._number) + self.assertEqual(1, p1.number) + self.assertEqual(2, p2.number) + self.assertEqual(3, p3.number) def test_p1_preserved(self): m = make_model() @@ -715,11 +709,11 @@ class TestPartitionNumbering(unittest.TestCase): p1 = make_partition(m, d1, preserve=True, number=1) p2 = make_partition(m, d1) p3 = make_partition(m, d1) - self.assertEqual(1, p1._number) + self.assertEqual(1, p1.number) self.assertEqual(True, p1.preserve) - self.assertEqual(2, p2._number) + self.assertEqual(2, p2.number) self.assertEqual(False, p2.preserve) - self.assertEqual(3, p3._number) + self.assertEqual(3, p3.number) self.assertEqual(False, p3.preserve) def test_p2_preserved(self): @@ -729,23 +723,9 @@ class TestPartitionNumbering(unittest.TestCase): p2 = make_partition(m, d1, preserve=True, number=2) p1 = make_partition(m, d1) p3 = make_partition(m, d1) - self.assertEqual(1, p1._number) - self.assertEqual(False, p1.preserve) - self.assertEqual(2, p2._number) - self.assertEqual(True, p2.preserve) - self.assertEqual(3, p3._number) - self.assertEqual(False, p3.preserve) - - def test_trigger_part_num_allocation_for_disk(self): - m = make_model() - d1 = make_disk(m, ptable='gpt') - p1 = make_partition(m, d1) - p2 = make_partition(m, d1) - p3 = make_partition(m, d1) - self.assertIsNone(p1.number) - self.assertIsNone(p2.number) - self.assertIsNone(p3.number) - self.assertEqual(3, p3._number) self.assertEqual(1, p1.number) + self.assertEqual(False, p1.preserve) self.assertEqual(2, p2.number) + self.assertEqual(True, p2.preserve) self.assertEqual(3, p3.number) + self.assertEqual(False, p3.preserve) diff --git a/subiquity/server/controllers/filesystem.py b/subiquity/server/controllers/filesystem.py index 0e00abec..af77bd97 100644 --- a/subiquity/server/controllers/filesystem.py +++ b/subiquity/server/controllers/filesystem.py @@ -310,7 +310,7 @@ class FilesystemController(SubiquityController, FilesystemManipulator): def get_partition(self, disk, number): for p in disk.partitions(): - if p._number == number: + if p.number == number: return p raise ValueError(f'Partition {number} on {disk.id} not found')