Merge pull request #2050 from dbungert/predictably-arbitrary

predictably arbitrary disk selection

subiquity/models/filesystem.py

@@ -617,6 +617,16 @@ class _Device(_Formattable, ABC):
     def size(self):
         pass
 
+    @property
+    @abstractmethod
+    def sort_key(self) -> Tuple:
+        """return a value that is usable for sorting.  The intent here is
+        that two runs of a specific version of subiquity must produce an
+        identical sort result for _Devices, so that while Subiquity might make
+        an arbitrary choice of disk, it's a consistent arbitrary choice.
+        """
+        pass
+
     # [Partition]
     _partitions: List["Partition"] = attributes.backlink(default=attr.Factory(list))
 
@@ -739,6 +749,10 @@ class Disk(_Device):
     _info: StorageInfo = attributes.for_api()
     _has_in_use_partition: bool = attributes.for_api(default=False)
 
+    @property
+    def sort_key(self) -> int:
+        return (self.wwn, self.serial, self.path)
+
     @property
     def available_for_partitions(self):
         margin_before = self.alignment_data().min_start_offset
@@ -880,6 +894,10 @@ def __post_init__(self):
                 return
         raise Exception("Exceeded number of available partitions")
 
+    @property
+    def sort_key(self) -> int:
+        return (self.device.sort_key(), self.number)
+
     def available(self):
         if self.flag in ["bios_grub", "prep"] or self.grub_device:
             return False
@@ -966,6 +984,10 @@ class Raid(_Device):
     _info: Optional[StorageInfo] = attributes.for_api(default=None)
     _has_in_use_partition = False
 
+    @property
+    def sort_key(self) -> int:
+        return tuple([dev.sort_key for dev in raid_device_sort(self.devices)])
+
     def serialize_devices(self):
         # Surprisingly, the order of devices passed to mdadm --create
         # matters (see get_raid_size) so we sort devices here the same
@@ -1061,6 +1083,10 @@ def size(self):
         # Should probably query actual size somehow for an existing VG!
         return get_lvm_size(self.devices)
 
+    @property
+    def sort_key(self) -> int:
+        return tuple([dev.sort_key for dev in self.devices])
+
     @property
     def available_for_partitions(self):
         return self.size
@@ -1224,6 +1250,10 @@ class ArbitraryDevice(_Device):
     def size(self):
         return 0
 
+    @property
+    def sort_key(self) -> int:
+        return (self.path,)
+
     ok_for_raid = False
     ok_for_lvm_vg = False
 
@@ -1680,6 +1710,13 @@ def match_nonzero_size(disk):
         return matchers
 
     def _sorted_matches(self, disks: Sequence[_Device], match: dict):
+        # sort first on the sort_key.  Objective here is that if we are falling
+        # back to arbitrary disk selection, we're at least consistent in what
+        # disk we arbitrarily select across runs
+        disks.sort(key=lambda d: d.sort_key)
+
+        # then sort on size, if requested.  Thanks to stable sort, if disks
+        # (or raids) have the same size, the sort_key will tiebreak.
         if match.get("size") == "smallest":
             disks.sort(key=lambda d: d.size)
         elif match.get("size") == "largest":

subiquity/models/tests/test_filesystem.py

@@ -209,9 +209,11 @@ def make_model_and_partition(bootloader=None):
     return model, make_partition(model, disk)
 
 
-def make_raid(model, **kw):
+def make_raid(model, disks=None, **kw):
     name = "md%s" % len(model._actions)
-    r = model.add_raid(name, "raid1", {make_disk(model), make_disk(model)}, set())
+    if disks is None:
+        disks = {make_disk(model), make_disk(model)}
+    r = model.add_raid(name, "raid1", disks, set())
     size = r.size
     for k, v in kw.items():
         setattr(r, k, v)
@@ -1860,17 +1862,23 @@ def test_lvm_logical_volume(self):
 
 
 class TestDiskForMatch(SubiTestCase):
+    match_sort_criteria = (["smallest"], ["largest"])
+
     def test_empty_match_directive(self):
         m = make_model()
         d1 = make_disk(m)
         d2 = make_disk(m)
+
+        # this test relies heavily on the assumptions in make_disk
         actual = m.disk_for_match([d1, d2], {})
         self.assertEqual(d1, actual)
+        actual = m.disk_for_match([d2, d1], {})
+        self.assertEqual(d1, actual)
 
     def test_sort_largest(self):
         m = make_model()
-        d100 = make_disk(m, size=100 << 30)
-        d200 = make_disk(m, size=200 << 30)
+        d100 = make_disk(m, size=100 << 30, serial="s1", path="/dev/d1")
+        d200 = make_disk(m, size=200 << 30, serial="s2", path="/dev/d2")
         actual = m.disk_for_match([d100, d200], {"size": "largest"})
         self.assertEqual(d200, actual)
 
@@ -1881,6 +1889,79 @@ def test_sort_smallest(self):
         actual = m.disk_for_match([d200, d100], {"size": "smallest"})
         self.assertEqual(d100, actual)
 
+    @parameterized.expand(match_sort_criteria)
+    def test_sort_serial(self, sort_criteria: str):
+        m = make_model()
+        d1 = make_disk(m, serial="s1", path=None, wwn=None)
+        d2 = make_disk(m, serial="s2", path=None, wwn=None)
+        # while the size sort is reversed when doing largest,
+        # we pre-sort on the other criteria, and stable sort helps out
+        actual = m.disk_for_match([d2, d1], {"size": sort_criteria})
+        self.assertEqual(d1, actual)
+
+    @parameterized.expand(match_sort_criteria)
+    def test_sort_path(self, sort_criteria: str):
+        m = make_model()
+        d1 = make_disk(m, serial=None, path="/dev/d1", wwn=None)
+        d2 = make_disk(m, serial=None, path="/dev/d2", wwn=None)
+        actual = m.disk_for_match([d2, d1], {"size": sort_criteria})
+        self.assertEqual(d1, actual)
+
+    @parameterized.expand(match_sort_criteria)
+    def test_sort_wwn(self, sort_criteria: str):
+        m = make_model()
+        d1 = make_disk(m, serial=None, path=None, wwn="w1")
+        d2 = make_disk(m, serial=None, path=None, wwn="w2")
+        actual = m.disk_for_match([d2, d1], {"size": sort_criteria})
+        self.assertEqual(d1, actual)
+
+    @parameterized.expand(match_sort_criteria)
+    def test_sort_wwn_wins(self, sort_criteria: str):
+        m = make_model()
+        d1 = make_disk(m, serial="s2", path="/dev/d2", wwn="w1")
+        d2 = make_disk(m, serial="s1", path="/dev/d1", wwn="w2")
+        actual = m.disk_for_match([d2, d1], {"size": sort_criteria})
+        self.assertEqual(d1, actual)
+
+    @parameterized.expand(match_sort_criteria)
+    def test_sort_serial_wins(self, sort_criteria: str):
+        m = make_model()
+        d1 = make_disk(m, serial="s1", path="/dev/d2", wwn="w")
+        d2 = make_disk(m, serial="s2", path="/dev/d1", wwn="w")
+        actual = m.disk_for_match([d2, d1], {"size": sort_criteria})
+        self.assertEqual(d1, actual)
+
+    @parameterized.expand(match_sort_criteria)
+    def test_sort_path_wins(self, sort_criteria: str):
+        m = make_model()
+        d1 = make_disk(m, serial="s", path="/dev/d1", wwn="w")
+        d2 = make_disk(m, serial="s", path="/dev/d2", wwn="w")
+        actual = m.disk_for_match([d2, d1], {"size": sort_criteria})
+        self.assertEqual(d1, actual)
+
+    def test_sort_raid(self):
+        m = make_model()
+        d1_1 = make_disk(m, size=100 << 30)
+        d1_2 = make_disk(m, size=100 << 30)
+        d2_1 = make_disk(m, size=200 << 30)
+        d2_2 = make_disk(m, size=200 << 30)
+        r1 = make_raid(m, disks={d1_1, d1_2})
+        r2 = make_raid(m, disks={d2_1, d2_2})
+        actual = m.disk_for_match([r1, r2], {"size": "largest"})
+        self.assertEqual(r2, actual)
+
+    @parameterized.expand(match_sort_criteria)
+    def test_sort_raid_on_disks(self, sort_criteria: str):
+        m = make_model()
+        d1_1 = make_disk(m, serial=None, path=None, wwn="w1_1")
+        d1_2 = make_disk(m, serial=None, path=None, wwn="w1_2")
+        d2_1 = make_disk(m, serial=None, path=None, wwn="w2_1")
+        d2_2 = make_disk(m, serial=None, path=None, wwn="w2_2")
+        r1 = make_raid(m, disks={d1_1, d1_2})
+        r2 = make_raid(m, disks={d2_1, d2_2})
+        actual = m.disk_for_match([r1, r2], {"size": sort_criteria})
+        self.assertEqual(r1, actual)
+
     def test_skip_empty(self):
         m = make_model()
         d0 = make_disk(m, size=0)