Skip to content

Commit bb4b4fd

Browse files
TheMipmapTheMipmap
committed
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 <morten.kaastrup.stephansen@cern.ch> Signed-off-by: Jakub Jelinek <jakub.jelinek@cern.ch>
1 parent 6fdb13d commit bb4b4fd

File tree

4 files changed

+94
-29
lines changed

4 files changed

+94
-29
lines changed

‎ironic_python_agent/device_hints.py‎

Lines changed: 53 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -191,11 +191,12 @@ def find_devices_by_hints(devices, root_device_hints):
191191
:size: (Integer) Size of the device in *bytes*
192192
:model: (String) Device model
193193
:vendor: (String) Device vendor name
194-
:serial: (String) Device serial number
195-
:wwn: (String) Unique storage identifier
196-
:wwn_with_extension: (String): Unique storage identifier with
197-
the vendor extension appended
198-
:wwn_vendor_extension: (String): United vendor storage identifier
194+
:serial: (String or List[String]) Device serial number(s)
195+
:wwn: (String or List[String]) Unique storage identifier(s)
196+
:wwn_with_extension: (String or List[String]): Unique storage
197+
identifier(s) with the vendor extension appended
198+
:wwn_vendor_extension: (String or List[String]): United vendor
199+
storage identifier(s)
199200
:rotational: (Boolean) Whether it's a rotational device or
200201
not. Useful to distinguish HDDs (rotational) and SSDs
201202
(not rotational).
@@ -221,6 +222,47 @@ def find_devices_by_hints(devices, root_device_hints):
221222
device_value = dev.get(hint)
222223
hint_value = parsed_hints[hint]
223224

225+
# Handle device attributes that are a list of strings
226+
# (serial, wwn, wwn_with_extension, and wwn_vendor_extension).
227+
if hint_type is str and isinstance(device_value, list):
228+
# NOTE(mostepha): Device value is a list. Consider it matched
229+
# if any value in the list matches the hint.
230+
device_values = [v for v in device_value if v is not None]
231+
if not device_values:
232+
LOG.warning(
233+
'The attribute "%(attr)s" of the device "%(dev)s" '
234+
'has an empty value. Skipping device.',
235+
{'attr': hint, 'dev': device_name})
236+
break
237+
238+
matched = False
239+
for val in device_values:
240+
try:
241+
normalized_val = _normalize_hint_expression(val, hint)
242+
if specs_matcher.match(normalized_val, hint_value):
243+
LOG.info('The attribute "%(attr)s" of device '
244+
'"%(dev)s" matched hint %(hint)s with '
245+
'value "%(value)s"',
246+
{'attr': hint, 'dev': device_name,
247+
'hint': hint_value, 'value': val})
248+
matched = True
249+
break
250+
except ValueError:
251+
# Skip invalid/empty values in the list
252+
continue
253+
254+
# Continue to next hint if any value matched. Otherwise, break
255+
# and try the next device.
256+
if matched:
257+
continue
258+
else:
259+
LOG.info('None of the values %(values)s for attribute '
260+
'"%(attr)s" of device "%(dev)s" match the hint '
261+
'%(hint)s',
262+
{'values': device_values, 'attr': hint,
263+
'dev': device_name, 'hint': hint_value})
264+
break
265+
224266
if hint_type is str:
225267
try:
226268
device_value = _normalize_hint_expression(device_value,
@@ -286,11 +328,12 @@ def match_root_device_hints(devices, root_device_hints):
286328
:size: (Integer) Size of the device in *bytes*
287329
:model: (String) Device model
288330
:vendor: (String) Device vendor name
289-
:serial: (String) Device serial number
290-
:wwn: (String) Unique storage identifier
291-
:wwn_with_extension: (String): Unique storage identifier with
292-
the vendor extension appended
293-
:wwn_vendor_extension: (String): United vendor storage identifier
331+
:serial: (String or List[String]) Device serial number(s)
332+
:wwn: (String or List[String]) Unique storage identifier(s)
333+
:wwn_with_extension: (String or List[String]): Unique storage
334+
identifier(s) with the vendor extension appended
335+
:wwn_vendor_extension: (String or List[String]): United vendor
336+
storage identifier(s)
294337
:rotational: (Boolean) Whether it's a rotational device or
295338
not. Useful to distinguish HDDs (rotational) and SSDs
296339
(not rotational).

‎ironic_python_agent/hardware.py‎

Lines changed: 0 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1859,22 +1859,6 @@ def get_os_install_device(self, permit_refresh=False):
18591859
dev_name = utils.guess_root_disk(block_devices).name
18601860
else:
18611861
serialized_devs = [dev.serialize() for dev in block_devices]
1862-
orig_size = len(serialized_devs)
1863-
for dev_idx in range(orig_size):
1864-
ser_dev = serialized_devs.pop(0)
1865-
serials = ser_dev.get('serial')
1866-
wwns = ser_dev.get('wwn')
1867-
# (rozzi) static serial and static wwn are used to avoid
1868-
# reundancy in the number of wwns and serials, if the code
1869-
# would just loop over both serials and wwns it could be that
1870-
# there would be an uncesarry duplication of the first wwn
1871-
# number
1872-
for serial in serials:
1873-
for wwn in wwns:
1874-
tmp_ser_dev = ser_dev.copy()
1875-
tmp_ser_dev['wwn'] = wwn
1876-
tmp_ser_dev['serial'] = serial
1877-
serialized_devs.append(tmp_ser_dev)
18781862
try:
18791863
device = device_hints.match_root_device_hints(
18801864
serialized_devs, root_device_hints)

‎ironic_python_agent/tests/unit/test_device_hints.py‎

Lines changed: 35 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -245,11 +245,13 @@ def setUp(self):
245245
super(MatchRootDeviceTestCase, self).setUp()
246246
self.devices = [
247247
{'name': '/dev/sda', 'size': 64424509440, 'model': 'ok model',
248-
'serial': 'fakeserial'},
248+
'serial': 'fakeserial', 'wwn': 'wwn_1'},
249249
{'name': '/dev/sdb', 'size': 128849018880, 'model': 'big model',
250-
'serial': 'veryfakeserial', 'rotational': 'yes'},
250+
'serial': ['veryfakeserial', 'alsoveryfakeserial'],
251+
'rotational': 'yes', 'wwn': ['wwn_2', 'wwn_2_ext']},
251252
{'name': '/dev/sdc', 'size': 10737418240, 'model': 'small model',
252-
'serial': 'veryveryfakeserial', 'rotational': False},
253+
'serial': 'veryveryfakeserial', 'rotational': False,
254+
'wwn': 'wwn_3'},
253255
]
254256

255257
def test_match_root_device_hints_one_hint(self):
@@ -292,6 +294,12 @@ def test_match_root_device_hints_multiple_hints3(self):
292294
self.devices, root_device_hints)
293295
self.assertEqual('/dev/sdc', dev['name'])
294296

297+
def test_match_root_device_hints_list_of_wwns(self):
298+
root_device_hints = {'wwn': 'wwn_2_ext'}
299+
dev = device_hints.match_root_device_hints(
300+
self.devices, root_device_hints)
301+
self.assertEqual('/dev/sdb', dev['name'])
302+
295303
def test_match_root_device_hints_no_operators(self):
296304
root_device_hints = {'size': '120', 'model': 'big model',
297305
'serial': 'veryfakeserial'}
@@ -328,3 +336,27 @@ def test_find_devices_name(self):
328336
devs = list(device_hints.find_devices_by_hints(self.devices,
329337
root_device_hints))
330338
self.assertEqual([self.devices[0]], devs)
339+
340+
def test_find_devices_single_serial(self):
341+
root_device_hints = {'serial': 's== fakeserial'}
342+
devs = list(device_hints.find_devices_by_hints(self.devices,
343+
root_device_hints))
344+
self.assertEqual([self.devices[0]], devs)
345+
346+
def test_find_devices_multiple_serials(self):
347+
root_device_hints = {'serial': 's== alsoveryfakeserial'}
348+
devs = list(device_hints.find_devices_by_hints(self.devices,
349+
root_device_hints))
350+
self.assertEqual([self.devices[1]], devs)
351+
352+
def test_find_devices_single_wwn(self):
353+
root_device_hints = {'wwn': 's== wwn_1'}
354+
devs = list(device_hints.find_devices_by_hints(self.devices,
355+
root_device_hints))
356+
self.assertEqual([self.devices[0]], devs)
357+
358+
def test_find_devices_multiple_wwns(self):
359+
root_device_hints = {'wwn': 's== wwn_2'}
360+
devs = list(device_hints.find_devices_by_hints(self.devices,
361+
root_device_hints))
362+
self.assertEqual([self.devices[1]], devs)

‎releasenotes/notes/fix-matching-hints-lists-b62254ff8eb4a2c1.yaml‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
fixes:
3+
- |
4+
Fixes matching hints with lists of WWN/Serial which was only handled
5+
in some cases - due to the issue, it was possible to choose a device
6+
listed in the skip_block_devices property as a root device.

0 commit comments

Comments
 (0)