From 6cb0d9915f5e12724d623f92ebf97b3bc3ae3e3b Mon Sep 17 00:00:00 2001 From: Morten Stephansen Date: Fri, 31 Oct 2025 13:28:16 +0000 Subject: [PATCH] Fix for matching hints with lists of strings Added logic for matching hints with lists of WWN/Serial. These lists appear when both lsblk and udev are used to fetch the information about a device. One consequence of this is that it allows a device on the skip list to be used as root device, thus overwriting the protected data. This has previously been handled before matching the hints, e.g. the removed section in hardware.py. This patch aims to fix the problem globally by handling the issue inside the find_devices_by_hints function. Closes-bug: #2130410 Change-Id: I28129f2ededb37474025f35164d5dc9ece21ec8e Signed-off-by: Morten Stephansen Signed-off-by: Jakub Jelinek (cherry picked from commit bb4b4fdb3887c1fbe0e8536869ac68e9ed1dbe96) --- ironic_python_agent/device_hints.py | 63 ++++++++++++++++--- ironic_python_agent/hardware.py | 16 ----- .../tests/unit/test_device_hints.py | 38 ++++++++++- ...matching-hints-lists-b62254ff8eb4a2c1.yaml | 6 ++ 4 files changed, 94 insertions(+), 29 deletions(-) create mode 100644 releasenotes/notes/fix-matching-hints-lists-b62254ff8eb4a2c1.yaml diff --git a/ironic_python_agent/device_hints.py b/ironic_python_agent/device_hints.py index 5d6160409..47d70842c 100644 --- a/ironic_python_agent/device_hints.py +++ b/ironic_python_agent/device_hints.py @@ -191,11 +191,12 @@ def find_devices_by_hints(devices, root_device_hints): :size: (Integer) Size of the device in *bytes* :model: (String) Device model :vendor: (String) Device vendor name - :serial: (String) Device serial number - :wwn: (String) Unique storage identifier - :wwn_with_extension: (String): Unique storage identifier with - the vendor extension appended - :wwn_vendor_extension: (String): United vendor storage identifier + :serial: (String or List[String]) Device serial number(s) + :wwn: (String or List[String]) Unique storage identifier(s) + :wwn_with_extension: (String or List[String]): Unique storage + identifier(s) with the vendor extension appended + :wwn_vendor_extension: (String or List[String]): United vendor + storage identifier(s) :rotational: (Boolean) Whether it's a rotational device or not. Useful to distinguish HDDs (rotational) and SSDs (not rotational). @@ -221,6 +222,47 @@ def find_devices_by_hints(devices, root_device_hints): device_value = dev.get(hint) hint_value = parsed_hints[hint] + # Handle device attributes that are a list of strings + # (serial, wwn, wwn_with_extension, and wwn_vendor_extension). + if hint_type is str and isinstance(device_value, list): + # NOTE(mostepha): Device value is a list. Consider it matched + # if any value in the list matches the hint. + device_values = [v for v in device_value if v is not None] + if not device_values: + LOG.warning( + 'The attribute "%(attr)s" of the device "%(dev)s" ' + 'has an empty value. Skipping device.', + {'attr': hint, 'dev': device_name}) + break + + matched = False + for val in device_values: + try: + normalized_val = _normalize_hint_expression(val, hint) + if specs_matcher.match(normalized_val, hint_value): + LOG.info('The attribute "%(attr)s" of device ' + '"%(dev)s" matched hint %(hint)s with ' + 'value "%(value)s"', + {'attr': hint, 'dev': device_name, + 'hint': hint_value, 'value': val}) + matched = True + break + except ValueError: + # Skip invalid/empty values in the list + continue + + # Continue to next hint if any value matched. Otherwise, break + # and try the next device. + if matched: + continue + else: + LOG.info('None of the values %(values)s for attribute ' + '"%(attr)s" of device "%(dev)s" match the hint ' + '%(hint)s', + {'values': device_values, 'attr': hint, + 'dev': device_name, 'hint': hint_value}) + break + if hint_type is str: try: device_value = _normalize_hint_expression(device_value, @@ -286,11 +328,12 @@ def match_root_device_hints(devices, root_device_hints): :size: (Integer) Size of the device in *bytes* :model: (String) Device model :vendor: (String) Device vendor name - :serial: (String) Device serial number - :wwn: (String) Unique storage identifier - :wwn_with_extension: (String): Unique storage identifier with - the vendor extension appended - :wwn_vendor_extension: (String): United vendor storage identifier + :serial: (String or List[String]) Device serial number(s) + :wwn: (String or List[String]) Unique storage identifier(s) + :wwn_with_extension: (String or List[String]): Unique storage + identifier(s) with the vendor extension appended + :wwn_vendor_extension: (String or List[String]): United vendor + storage identifier(s) :rotational: (Boolean) Whether it's a rotational device or not. Useful to distinguish HDDs (rotational) and SSDs (not rotational). diff --git a/ironic_python_agent/hardware.py b/ironic_python_agent/hardware.py index 36e7178f4..01cc43c8e 100644 --- a/ironic_python_agent/hardware.py +++ b/ironic_python_agent/hardware.py @@ -1765,22 +1765,6 @@ class GenericHardwareManager(HardwareManager): dev_name = utils.guess_root_disk(block_devices).name else: serialized_devs = [dev.serialize() for dev in block_devices] - orig_size = len(serialized_devs) - for dev_idx in range(orig_size): - ser_dev = serialized_devs.pop(0) - serials = ser_dev.get('serial') - wwns = ser_dev.get('wwn') - # (rozzi) static serial and static wwn are used to avoid - # reundancy in the number of wwns and serials, if the code - # would just loop over both serials and wwns it could be that - # there would be an uncesarry duplication of the first wwn - # number - for serial in serials: - for wwn in wwns: - tmp_ser_dev = ser_dev.copy() - tmp_ser_dev['wwn'] = wwn - tmp_ser_dev['serial'] = serial - serialized_devs.append(tmp_ser_dev) try: device = device_hints.match_root_device_hints( serialized_devs, root_device_hints) diff --git a/ironic_python_agent/tests/unit/test_device_hints.py b/ironic_python_agent/tests/unit/test_device_hints.py index 50c67e0b7..7595182e8 100644 --- a/ironic_python_agent/tests/unit/test_device_hints.py +++ b/ironic_python_agent/tests/unit/test_device_hints.py @@ -245,11 +245,13 @@ class MatchRootDeviceTestCase(base.IronicAgentTest): super(MatchRootDeviceTestCase, self).setUp() self.devices = [ {'name': '/dev/sda', 'size': 64424509440, 'model': 'ok model', - 'serial': 'fakeserial'}, + 'serial': 'fakeserial', 'wwn': 'wwn_1'}, {'name': '/dev/sdb', 'size': 128849018880, 'model': 'big model', - 'serial': 'veryfakeserial', 'rotational': 'yes'}, + 'serial': ['veryfakeserial', 'alsoveryfakeserial'], + 'rotational': 'yes', 'wwn': ['wwn_2', 'wwn_2_ext']}, {'name': '/dev/sdc', 'size': 10737418240, 'model': 'small model', - 'serial': 'veryveryfakeserial', 'rotational': False}, + 'serial': 'veryveryfakeserial', 'rotational': False, + 'wwn': 'wwn_3'}, ] def test_match_root_device_hints_one_hint(self): @@ -292,6 +294,12 @@ class MatchRootDeviceTestCase(base.IronicAgentTest): self.devices, root_device_hints) self.assertEqual('/dev/sdc', dev['name']) + def test_match_root_device_hints_list_of_wwns(self): + root_device_hints = {'wwn': 'wwn_2_ext'} + dev = device_hints.match_root_device_hints( + self.devices, root_device_hints) + self.assertEqual('/dev/sdb', dev['name']) + def test_match_root_device_hints_no_operators(self): root_device_hints = {'size': '120', 'model': 'big model', 'serial': 'veryfakeserial'} @@ -328,3 +336,27 @@ class MatchRootDeviceTestCase(base.IronicAgentTest): devs = list(device_hints.find_devices_by_hints(self.devices, root_device_hints)) self.assertEqual([self.devices[0]], devs) + + def test_find_devices_single_serial(self): + root_device_hints = {'serial': 's== fakeserial'} + devs = list(device_hints.find_devices_by_hints(self.devices, + root_device_hints)) + self.assertEqual([self.devices[0]], devs) + + def test_find_devices_multiple_serials(self): + root_device_hints = {'serial': 's== alsoveryfakeserial'} + devs = list(device_hints.find_devices_by_hints(self.devices, + root_device_hints)) + self.assertEqual([self.devices[1]], devs) + + def test_find_devices_single_wwn(self): + root_device_hints = {'wwn': 's== wwn_1'} + devs = list(device_hints.find_devices_by_hints(self.devices, + root_device_hints)) + self.assertEqual([self.devices[0]], devs) + + def test_find_devices_multiple_wwns(self): + root_device_hints = {'wwn': 's== wwn_2'} + devs = list(device_hints.find_devices_by_hints(self.devices, + root_device_hints)) + self.assertEqual([self.devices[1]], devs) diff --git a/releasenotes/notes/fix-matching-hints-lists-b62254ff8eb4a2c1.yaml b/releasenotes/notes/fix-matching-hints-lists-b62254ff8eb4a2c1.yaml new file mode 100644 index 000000000..b5c3f8263 --- /dev/null +++ b/releasenotes/notes/fix-matching-hints-lists-b62254ff8eb4a2c1.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + Fixes matching hints with lists of WWN/Serial which was only handled + in some cases - due to the issue, it was possible to choose a device + listed in the skip_block_devices property as a root device.