Skip to content

Commit

Permalink
filesystem: handle sorting disks without required metadata
Browse files Browse the repository at this point in the history
If there are at least two disks and one of them is missing any metadata used
during sorting installer will crash. Issue can be reproduced using already
available machine .json file: examples/machines/existing-partitions.json
in which /dev/vda does not have serial value.

This patch makes sure that all values used in sorting are initialized

2025-01-21 15:41:33,340 ERROR root:38 finish: subiquity/apply_autoinstall_config: FAIL: '<' not supported between instances of 'str' and 'NoneType'
2025-01-21 15:41:33,340 ERROR subiquity.server.server:503 top level error
Traceback (most recent call last):
  File "/home/rmalz/repos/subiquity/subiquity/server/server.py", line 1021, in start
    await self.apply_autoinstall_config()
  File "/home/rmalz/repos/subiquity/subiquitycore/context.py", line 165, in decorated_async
    return await meth(self, **kw)
  File "/home/rmalz/repos/subiquity/subiquity/server/server.py", line 575, in apply_autoinstall_config
    await controller.apply_autoinstall_config()
  File "/home/rmalz/repos/subiquity/subiquitycore/context.py", line 165, in decorated_async
    return await meth(self, **kw)
  File "/home/rmalz/repos/subiquity/subiquity/server/controllers/filesystem.py", line 484, in apply_autoinstall_config
    await self.convert_autoinstall_config(context=context)
  File "/home/rmalz/repos/subiquity/subiquitycore/context.py", line 165, in decorated_async
    return await meth(self, **kw)
  File "/home/rmalz/repos/subiquity/subiquity/server/controllers/filesystem.py", line 1745, in convert_autoinstall_config
    await self.run_autoinstall_guided(self.ai_data["layout"])
  File "/home/rmalz/repos/subiquity/subiquity/server/controllers/filesystem.py", line 1684, in run_autoinstall_guided
    disk = self.get_bootable_matching_disk(match)
  File "/home/rmalz/repos/subiquity/subiquity/server/controllers/filesystem.py", line 1594, in get_bootable_matching_disk
    matching_disks = self.get_bootable_matching_disks(match)
  File "/home/rmalz/repos/subiquity/subiquity/server/controllers/filesystem.py", line 1582, in get_bootable_matching_disks
    matching_disks = self.model.disks_for_match(disks, match)
  File "/home/rmalz/repos/subiquity/subiquity/models/filesystem.py", line 1800, in disks_for_match
    candidates, _ = self._matching_disks(disks, match)
  File "/home/rmalz/repos/subiquity/subiquity/models/filesystem.py", line 1791, in _matching_disks
    candidates = self._sorted_matches(candidates, m)
  File "/home/rmalz/repos/subiquity/subiquity/models/filesystem.py", line 1769, in _sorted_matches
    disks.sort(key=lambda d: d.sort_key)
TypeError: '<' not supported between instances of 'str' and 'NoneType'

Fixes: #2050
  • Loading branch information
rmalz-c committed Jan 22, 2025
1 parent 782dbb6 commit c77f8ca
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 12 deletions.
25 changes: 13 additions & 12 deletions subiquity/models/filesystem.py
Original file line number Diff line number Diff line change
Expand Up @@ -653,10 +653,11 @@ def size(self):
@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.
"""return a value that is usable for sorting and does not contain
any None values. 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

Expand Down Expand Up @@ -799,8 +800,8 @@ class Disk(_Device):
_has_in_use_partition: bool = attributes.for_api(default=False)

@property
def sort_key(self) -> int:
return (self.wwn, self.serial, self.path)
def sort_key(self) -> Tuple:
return (self.wwn or "", self.serial or "", self.path or "")

@property
def available_for_partitions(self):
Expand Down Expand Up @@ -944,8 +945,8 @@ def __post_init__(self):
raise Exception("Exceeded number of available partitions")

@property
def sort_key(self) -> int:
return (self.device.sort_key(), self.number)
def sort_key(self) -> Tuple:
return (self.device.sort_key(), self.number or 0)

def available(self):
if self.flag in ["bios_grub", "prep"] or self.grub_device:
Expand Down Expand Up @@ -1039,7 +1040,7 @@ class Raid(_Device):
_has_in_use_partition = False

@property
def sort_key(self) -> int:
def sort_key(self) -> Tuple:
return tuple([dev.sort_key for dev in raid_device_sort(self.devices)])

def serialize_devices(self):
Expand Down Expand Up @@ -1138,7 +1139,7 @@ def size(self):
return get_lvm_size(self.devices)

@property
def sort_key(self) -> int:
def sort_key(self) -> Tuple:
return tuple([dev.sort_key for dev in self.devices])

@property
Expand Down Expand Up @@ -1305,8 +1306,8 @@ def size(self):
return 0

@property
def sort_key(self) -> int:
return (self.path,)
def sort_key(self) -> Tuple:
return (self.path or "",)

ok_for_raid = False
ok_for_lvm_vg = False
Expand Down
10 changes: 10 additions & 0 deletions subiquity/models/tests/test_filesystem.py
Original file line number Diff line number Diff line change
Expand Up @@ -2075,6 +2075,16 @@ def test_sort_path_wins(self, sort_criteria: str):
self.assertEqual(d1, m.disk_for_match([d2, d1], {"size": sort_criteria}))
self.assertEqual([d1, d2], m.disks_for_match([d2, d1], {"size": sort_criteria}))

def test_sort_disks_none_values(self):
m = make_model()
d1 = make_disk(m, wwn=None, serial="s", path="/dev/d1")
d2 = make_disk(m, wwn="w", serial=None, path="/dev/d2")
d3 = make_disk(m, wwn="w", serial="s", path=None)
self.assertEqual(d1, m.disk_for_match([d3, d2, d1], {"size": "largest"}))
self.assertEqual(
[d1, d2, d3], m.disks_for_match([d3, d2, d1], {"size": "largest"})
)

def test_sort_raid(self):
m = make_model()
d1_1 = make_disk(m, size=100 << 30)
Expand Down

0 comments on commit c77f8ca

Please sign in to comment.