Skip to content

Commit 33535cd

Browse files
umagoumago
committed
Get root device hints from the node object
In order to support a more complex syntax for root device hints (e.g operators: greater than, less than, in, etc...) we need to stop relying on the kernel command line for passing the root device hints. This patch changes this approach by getting the root device hints from a cached node object that was set in the hardware module. Two new functions: "cache_node" and "get_cached_node" were added to the hardware module. The idea is to facilitate the access to a node object representation from the hardware extension methods without changing method signatures, which would break compatibility with out-of-tree hardware managers. Note that the new "get_cached_node" is just a guard function to facilitate the tests for the code. The function parse_root_device_hints() and its tests were removed since it's not used/needed anymore. Partial-Bug: #1561137 Change-Id: I830fe7da1a59b46e348213b6f451c2ee55f6008c
1 parent 962ee1a commit 33535cd

7 files changed

Lines changed: 57 additions & 90 deletions

File tree

‎ironic_python_agent/agent.py‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -303,6 +303,7 @@ def run(self):
303303
node_uuid=uuid)
304304

305305
self.node = content['node']
306+
hardware.cache_node(self.node)
306307
self.heartbeat_timeout = content['heartbeat_timeout']
307308

308309
wsgi = simple_server.make_server(

‎ironic_python_agent/extensions/clean.py‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ def get_clean_steps(self, node, ports):
3636
"""
3737
LOG.debug('Getting clean steps, called with node: %(node)s, '
3838
'ports: %(ports)s', {'node': node, 'ports': ports})
39+
hardware.cache_node(node)
3940
# Results should be a dict, not a list
4041
candidate_steps = hardware.dispatch_to_all_managers('get_clean_steps',
4142
node, ports)
@@ -65,6 +66,7 @@ def execute_clean_step(self, step, node, ports, clean_version=None,
6566
"""
6667
# Ensure the agent is still the same version, or raise an exception
6768
LOG.debug('Executing clean step %s', step)
69+
hardware.cache_node(node)
6870
_check_clean_version(clean_version)
6971

7072
if 'step' not in step:

‎ironic_python_agent/hardware.py‎

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,8 @@
4242
_DISK_WAIT_ATTEMPTS = 10
4343
_DISK_WAIT_DELAY = 3
4444

45+
NODE = None
46+
4547

4648
def _get_device_vendor(dev):
4749
"""Get the vendor name of a given device."""
@@ -520,9 +522,12 @@ def list_block_devices(self):
520522
return list_all_block_devices()
521523

522524
def get_os_install_device(self):
523-
block_devices = self.list_block_devices()
524-
root_device_hints = utils.parse_root_device_hints()
525+
cached_node = get_cached_node()
526+
root_device_hints = None
527+
if cached_node is not None:
528+
root_device_hints = cached_node['properties'].get('root_device')
525529

530+
block_devices = self.list_block_devices()
526531
if not root_device_hints:
527532
return utils.guess_root_disk(block_devices).name
528533
else:
@@ -917,3 +922,20 @@ def load_managers():
917922
:raises HardwareManagerNotFound: if no valid hardware managers found
918923
"""
919924
_get_managers()
925+
926+
927+
def cache_node(node):
928+
"""Store the node object in the hardware module.
929+
930+
Stores the node object in the hardware module to facilitate the
931+
access of a node information in the hardware extensions.
932+
933+
:param node: Ironic node object
934+
"""
935+
global NODE
936+
NODE = node
937+
938+
939+
def get_cached_node():
940+
"""Guard function around the module variable NODE."""
941+
return NODE

‎ironic_python_agent/tests/unit/test_hardware.py‎

Lines changed: 24 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -321,31 +321,38 @@ def test_list_network_interfaces_no_carrier(self,
321321
self.assertEqual('192.168.1.2', interfaces[0].ipv4_address)
322322
self.assertFalse(interfaces[0].has_carrier)
323323

324+
@mock.patch.object(hardware, 'get_cached_node')
324325
@mock.patch.object(utils, 'execute')
325-
def test_get_os_install_device(self, mocked_execute):
326+
def test_get_os_install_device(self, mocked_execute, mock_cached_node):
327+
mock_cached_node.return_value = None
326328
mocked_execute.return_value = (BLK_DEVICE_TEMPLATE, '')
327329
self.assertEqual('/dev/sdb', self.hardware.get_os_install_device())
328330
mocked_execute.assert_called_once_with(
329331
'lsblk', '-Pbdi', '-oKNAME,MODEL,SIZE,ROTA,TYPE',
330332
check_exit_code=[0])
333+
mock_cached_node.assert_called_once_with()
331334

335+
@mock.patch.object(hardware, 'get_cached_node')
332336
@mock.patch.object(utils, 'execute')
333-
def test_get_os_install_device_fails(self, mocked_execute):
337+
def test_get_os_install_device_fails(self, mocked_execute,
338+
mock_cached_node):
334339
"""Fail to find device >=4GB w/o root device hints"""
340+
mock_cached_node.return_value = None
335341
mocked_execute.return_value = (BLK_DEVICE_TEMPLATE_SMALL, '')
336342
ex = self.assertRaises(errors.DeviceNotFound,
337343
self.hardware.get_os_install_device)
338344
mocked_execute.assert_called_once_with(
339345
'lsblk', '-Pbdi', '-oKNAME,MODEL,SIZE,ROTA,TYPE',
340346
check_exit_code=[0])
341347
self.assertIn(str(4 * units.Gi), ex.details)
348+
mock_cached_node.assert_called_once_with()
342349

343350
@mock.patch.object(hardware, 'list_all_block_devices')
344-
@mock.patch.object(utils, 'parse_root_device_hints')
351+
@mock.patch.object(hardware, 'get_cached_node')
345352
def _get_os_install_device_root_device_hints(self, hints, expected_device,
346-
mock_root_device, mock_dev):
353+
mock_cached_node, mock_dev):
354+
mock_cached_node.return_value = {'properties': {'root_device': hints}}
347355
model = 'fastable sd131 7'
348-
mock_root_device.return_value = hints
349356
mock_dev.return_value = [
350357
hardware.BlockDevice(name='/dev/sda',
351358
model='TinyUSB Drive',
@@ -369,7 +376,7 @@ def _get_os_install_device_root_device_hints(self, hints, expected_device,
369376

370377
self.assertEqual(expected_device,
371378
self.hardware.get_os_install_device())
372-
mock_root_device.assert_called_once_with()
379+
mock_cached_node.assert_called_once_with()
373380
mock_dev.assert_called_once_with()
374381

375382
def test_get_os_install_device_root_device_hints_model(self):
@@ -397,15 +404,18 @@ def test_get_os_install_device_root_device_hints_name(self):
397404
{'name': '/dev/sdb'}, '/dev/sdb')
398405

399406
@mock.patch.object(hardware, 'list_all_block_devices')
400-
@mock.patch.object(utils, 'parse_root_device_hints')
407+
@mock.patch.object(hardware, 'get_cached_node')
401408
def test_get_os_install_device_root_device_hints_no_device_found(
402-
self, mock_root_device, mock_dev):
409+
self, mock_cached_node, mock_dev):
403410
model = 'fastable sd131 7'
404-
mock_root_device.return_value = {'model': model,
405-
'wwn': 'fake-wwn',
406-
'serial': 'fake-serial',
407-
'vendor': 'fake-vendor',
408-
'size': 10}
411+
mock_cached_node.return_value = {
412+
'properties': {
413+
'root_device': {
414+
'model': model,
415+
'wwn': 'fake-wwn',
416+
'serial': 'fake-serial',
417+
'vendor': 'fake-vendor',
418+
'size': 10}}}
409419
# Model is different here
410420
mock_dev.return_value = [
411421
hardware.BlockDevice(name='/dev/sda',
@@ -425,7 +435,7 @@ def test_get_os_install_device_root_device_hints_no_device_found(
425435
]
426436
self.assertRaises(errors.DeviceNotFound,
427437
self.hardware.get_os_install_device)
428-
mock_root_device.assert_called_once_with()
438+
mock_cached_node.assert_called_once_with()
429439
mock_dev.assert_called_once_with()
430440

431441
def test__get_device_vendor(self):

‎ironic_python_agent/tests/unit/test_utils.py‎

Lines changed: 0 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -403,42 +403,6 @@ def test__get_vmedia_params_rmtree_fails(self, execute_mock, mkdir_mock,
403403
mkdtemp_mock.assert_called_once_with()
404404
rmtree_mock.assert_called_once_with("/tempdir")
405405

406-
@mock.patch.object(utils, 'get_agent_params')
407-
def test_parse_root_device_hints(self, mock_get_params):
408-
mock_get_params.return_value = {
409-
'root_device': 'vendor=SpongeBob,model=Square%20Pants',
410-
'ipa-api-url': 'http://1.2.3.4:1234'
411-
}
412-
expected = {'vendor': 'spongebob', 'model': 'square pants'}
413-
result = utils.parse_root_device_hints()
414-
self.assertEqual(expected, result)
415-
416-
@mock.patch.object(utils, 'get_agent_params')
417-
def test_parse_root_device_hints_no_hints(self, mock_get_params):
418-
mock_get_params.return_value = {
419-
'ipa-api-url': 'http://1.2.3.4:1234'
420-
}
421-
result = utils.parse_root_device_hints()
422-
self.assertEqual({}, result)
423-
424-
@mock.patch.object(utils, 'get_agent_params')
425-
def test_parse_root_device_size(self, mock_get_params):
426-
mock_get_params.return_value = {
427-
'root_device': 'size=12345',
428-
'ipa-api-url': 'http://1.2.3.4:1234'
429-
}
430-
result = utils.parse_root_device_hints()
431-
self.assertEqual(12345, result['size'])
432-
433-
@mock.patch.object(utils, 'get_agent_params')
434-
def test_parse_root_device_not_supported(self, mock_get_params):
435-
mock_get_params.return_value = {
436-
'root_device': 'foo=bar,size=12345',
437-
'ipa-api-url': 'http://1.2.3.4:1234'
438-
}
439-
self.assertRaises(errors.DeviceNotFound,
440-
utils.parse_root_device_hints)
441-
442406

443407
class TestFailures(testtools.TestCase):
444408
def test_get_error(self):

‎ironic_python_agent/utils.py‎

Lines changed: 0 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -233,44 +233,6 @@ def normalize(string):
233233
return parse.unquote(string).lower().strip()
234234

235235

236-
def parse_root_device_hints():
237-
"""Parse the root device hints.
238-
239-
Parse the root device hints given by Ironic via kernel cmdline
240-
or vmedia.
241-
242-
:returns: A dict with the hints or an empty dict if no hints are
243-
passed.
244-
:raises: DeviceNotFound if there are unsupported hints.
245-
246-
"""
247-
root_device = get_agent_params().get('root_device')
248-
if not root_device:
249-
return {}
250-
251-
hints = dict((item.split('=') for item in root_device.split(',')))
252-
253-
# Find invalid hints for logging
254-
not_supported = set(hints) - SUPPORTED_ROOT_DEVICE_HINTS
255-
if not_supported:
256-
error_msg = ('No device can be found because the following hints: '
257-
'"%(not_supported)s" are not supported by this version '
258-
'of IPA. Supported hints are: "%(supported)s"',
259-
{'not_supported': ', '.join(not_supported),
260-
'supported': ', '.join(SUPPORTED_ROOT_DEVICE_HINTS)})
261-
raise errors.DeviceNotFound(error_msg)
262-
263-
# Normalise the values
264-
hints = {k: normalize(v) for k, v in hints.items()}
265-
266-
if 'size' in hints:
267-
# NOTE(lucasagomes): Ironic should validate before passing to
268-
# the deploy ramdisk
269-
hints['size'] = int(hints['size'])
270-
271-
return hints
272-
273-
274236
class AccumulatedFailures(object):
275237
"""Object to accumulate failures without raising exception."""
276238

‎releasenotes/notes/device-hints-from-node-object-9a689f5a4175a1a6.yaml‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
features:
3+
- IPA now gets the root device hints directly from the node object
4+
representation instead of the kernel command line. This is a required
5+
work in order to support a more complex syntax for the hints in
6+
the future.

0 commit comments

Comments
 (0)