diff --git a/subiquity/models/filesystem.py b/subiquity/models/filesystem.py index c1db6351f..5689c1560 100644 --- a/subiquity/models/filesystem.py +++ b/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": diff --git a/subiquity/models/tests/test_filesystem.py b/subiquity/models/tests/test_filesystem.py index 50f14dace..f3c781fe7 100644 --- a/subiquity/models/tests/test_filesystem.py +++ b/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)