Merge pull request #1327 from dbungert/basic-part-numbering

filesystem: non-overlapping partition numbering

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),
                     )

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,

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,18 +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.preserve:
-            return self.number
-        else:
-            return self.device._partitions.index(self) + 1
-
     def _path(self):
-        return partition_kname(self.device.path, self._number)
+        return partition_kname(self.device.path, self.number)
 
     @property
     def boot(self):
@@ -1261,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:

subiquity/models/tests/test_filesystem.py

@@ -176,8 +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:
-        partition.number = len(device._partitions)
     model._actions.append(partition)
     return partition
 
@@ -692,3 +690,42 @@ 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)

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')
 

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)
+            [sda] = add_resp['disks']
-            sda2 = first(sda['partitions'], 'number', 2)
+            [root] = match(sda['partitions'], mount='/')
-            self.assertEqual('ext3', sda2['format'])
+            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_sda] = add_resp['disks']
-            add_sda2 = first(add_sda['partitions'], 'number', 2)
+            [add_root] = match(add_sda['partitions'], mount='/')
 
-            edit_sda = first(edit_resp['disks'], 'id', disk_id)
+            [edit_sda] = edit_resp['disks']
-            edit_sda2 = first(edit_sda['partitions'], 'number', 2)
+            [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(add_root[key], edit_root[key], key)
-            self.assertEqual('ext4', edit_sda2['format'])
+            self.assertEqual('ext4', edit_root['format'])
 
             del_resp = await inst.post('/storage/v2/delete_partition', data)
-            sda = first(del_resp['disks'], 'id', disk_id)
+            [sda] = del_resp['disks']
-            self.assertEqual(2, len(sda['partitions']))
+            [p, g] = sda['partitions']
-
+            self.assertEqual('Partition', p['$type'])
-            for type in 'Partition', 'Gap':
+            self.assertEqual('Gap', g['$type'])
-                pgs = [pg for pg in sda['partitions'] if pg['$type'] == type]
-                self.assertEqual(len(pgs), 1)
 
             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
+            # these manual steps are expected to be mostly equivalent to just
-            # the single partition and getting the automatic boot partition
+            # 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')
+            [sda] = resp['disks']
-            boot = first(sda['partitions'], 'number', 1)
+            [boot] = match(sda['partitions'], mount='/boot/efi')
-            gap = sda['partitions'][2]
+            [p1, p2, gap] = sda['partitions']
-            expected = (10 << 30) - boot['size'] - (4 << 30) - (2 << 20)
+            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)
+            [sda] = resp['disks']
-            sda2 = first(sda['partitions'], 'number', 2)
+            [part] = match(sda['partitions'], mount='/foo')
-            sda2.update({'format': 'ext3', 'mount': '/bar'})
+            part.update({'format': 'ext3', 'mount': '/bar'})
-            data['partition'] = sda2
+            data['partition'] = part
             data.pop('gap')
             await inst.post('/storage/v2/edit_partition', data)
             # should not throw an exception complaining about boot