diff --git a/ironic_python_agent/agent.py b/ironic_python_agent/agent.py index 75a7e8aab..9044721ef 100644 --- a/ironic_python_agent/agent.py +++ b/ironic_python_agent/agent.py @@ -303,6 +303,7 @@ class IronicPythonAgent(base.ExecuteCommandMixin): node_uuid=uuid) self.node = content['node'] + hardware.cache_node(self.node) self.heartbeat_timeout = content['heartbeat_timeout'] wsgi = simple_server.make_server( diff --git a/ironic_python_agent/extensions/clean.py b/ironic_python_agent/extensions/clean.py index 83631825c..2b9da5db5 100644 --- a/ironic_python_agent/extensions/clean.py +++ b/ironic_python_agent/extensions/clean.py @@ -36,6 +36,7 @@ class CleanExtension(base.BaseAgentExtension): """ LOG.debug('Getting clean steps, called with node: %(node)s, ' 'ports: %(ports)s', {'node': node, 'ports': ports}) + hardware.cache_node(node) # Results should be a dict, not a list candidate_steps = hardware.dispatch_to_all_managers('get_clean_steps', node, ports) @@ -65,6 +66,7 @@ class CleanExtension(base.BaseAgentExtension): """ # Ensure the agent is still the same version, or raise an exception LOG.debug('Executing clean step %s', step) + hardware.cache_node(node) _check_clean_version(clean_version) if 'step' not in step: diff --git a/ironic_python_agent/hardware.py b/ironic_python_agent/hardware.py index ac7e25d3f..7ad335e2a 100644 --- a/ironic_python_agent/hardware.py +++ b/ironic_python_agent/hardware.py @@ -42,6 +42,8 @@ UNIT_CONVERTER.define('GB = 1024 MB') _DISK_WAIT_ATTEMPTS = 10 _DISK_WAIT_DELAY = 3 +NODE = None + def _get_device_vendor(dev): """Get the vendor name of a given device.""" @@ -520,9 +522,12 @@ class GenericHardwareManager(HardwareManager): return list_all_block_devices() def get_os_install_device(self): - block_devices = self.list_block_devices() - root_device_hints = utils.parse_root_device_hints() + cached_node = get_cached_node() + root_device_hints = None + if cached_node is not None: + root_device_hints = cached_node['properties'].get('root_device') + block_devices = self.list_block_devices() if not root_device_hints: return utils.guess_root_disk(block_devices).name else: @@ -917,3 +922,20 @@ def load_managers(): :raises HardwareManagerNotFound: if no valid hardware managers found """ _get_managers() + + +def cache_node(node): + """Store the node object in the hardware module. + + Stores the node object in the hardware module to facilitate the + access of a node information in the hardware extensions. + + :param node: Ironic node object + """ + global NODE + NODE = node + + +def get_cached_node(): + """Guard function around the module variable NODE.""" + return NODE diff --git a/ironic_python_agent/tests/unit/test_hardware.py b/ironic_python_agent/tests/unit/test_hardware.py index 1aa04c624..43d30fe67 100644 --- a/ironic_python_agent/tests/unit/test_hardware.py +++ b/ironic_python_agent/tests/unit/test_hardware.py @@ -321,17 +321,23 @@ class TestGenericHardwareManager(test_base.BaseTestCase): self.assertEqual('192.168.1.2', interfaces[0].ipv4_address) self.assertFalse(interfaces[0].has_carrier) + @mock.patch.object(hardware, 'get_cached_node') @mock.patch.object(utils, 'execute') - def test_get_os_install_device(self, mocked_execute): + def test_get_os_install_device(self, mocked_execute, mock_cached_node): + mock_cached_node.return_value = None mocked_execute.return_value = (BLK_DEVICE_TEMPLATE, '') self.assertEqual('/dev/sdb', self.hardware.get_os_install_device()) mocked_execute.assert_called_once_with( 'lsblk', '-Pbdi', '-oKNAME,MODEL,SIZE,ROTA,TYPE', check_exit_code=[0]) + mock_cached_node.assert_called_once_with() + @mock.patch.object(hardware, 'get_cached_node') @mock.patch.object(utils, 'execute') - def test_get_os_install_device_fails(self, mocked_execute): + def test_get_os_install_device_fails(self, mocked_execute, + mock_cached_node): """Fail to find device >=4GB w/o root device hints""" + mock_cached_node.return_value = None mocked_execute.return_value = (BLK_DEVICE_TEMPLATE_SMALL, '') ex = self.assertRaises(errors.DeviceNotFound, self.hardware.get_os_install_device) @@ -339,13 +345,14 @@ class TestGenericHardwareManager(test_base.BaseTestCase): 'lsblk', '-Pbdi', '-oKNAME,MODEL,SIZE,ROTA,TYPE', check_exit_code=[0]) self.assertIn(str(4 * units.Gi), ex.details) + mock_cached_node.assert_called_once_with() @mock.patch.object(hardware, 'list_all_block_devices') - @mock.patch.object(utils, 'parse_root_device_hints') + @mock.patch.object(hardware, 'get_cached_node') def _get_os_install_device_root_device_hints(self, hints, expected_device, - mock_root_device, mock_dev): + mock_cached_node, mock_dev): + mock_cached_node.return_value = {'properties': {'root_device': hints}} model = 'fastable sd131 7' - mock_root_device.return_value = hints mock_dev.return_value = [ hardware.BlockDevice(name='/dev/sda', model='TinyUSB Drive', @@ -369,7 +376,7 @@ class TestGenericHardwareManager(test_base.BaseTestCase): self.assertEqual(expected_device, self.hardware.get_os_install_device()) - mock_root_device.assert_called_once_with() + mock_cached_node.assert_called_once_with() mock_dev.assert_called_once_with() def test_get_os_install_device_root_device_hints_model(self): @@ -397,15 +404,18 @@ class TestGenericHardwareManager(test_base.BaseTestCase): {'name': '/dev/sdb'}, '/dev/sdb') @mock.patch.object(hardware, 'list_all_block_devices') - @mock.patch.object(utils, 'parse_root_device_hints') + @mock.patch.object(hardware, 'get_cached_node') def test_get_os_install_device_root_device_hints_no_device_found( - self, mock_root_device, mock_dev): + self, mock_cached_node, mock_dev): model = 'fastable sd131 7' - mock_root_device.return_value = {'model': model, - 'wwn': 'fake-wwn', - 'serial': 'fake-serial', - 'vendor': 'fake-vendor', - 'size': 10} + mock_cached_node.return_value = { + 'properties': { + 'root_device': { + 'model': model, + 'wwn': 'fake-wwn', + 'serial': 'fake-serial', + 'vendor': 'fake-vendor', + 'size': 10}}} # Model is different here mock_dev.return_value = [ hardware.BlockDevice(name='/dev/sda', @@ -425,7 +435,7 @@ class TestGenericHardwareManager(test_base.BaseTestCase): ] self.assertRaises(errors.DeviceNotFound, self.hardware.get_os_install_device) - mock_root_device.assert_called_once_with() + mock_cached_node.assert_called_once_with() mock_dev.assert_called_once_with() def test__get_device_vendor(self): diff --git a/ironic_python_agent/tests/unit/test_utils.py b/ironic_python_agent/tests/unit/test_utils.py index fd1cd5d69..eff1a0912 100644 --- a/ironic_python_agent/tests/unit/test_utils.py +++ b/ironic_python_agent/tests/unit/test_utils.py @@ -403,42 +403,6 @@ class GetAgentParamsTestCase(test_base.BaseTestCase): mkdtemp_mock.assert_called_once_with() rmtree_mock.assert_called_once_with("/tempdir") - @mock.patch.object(utils, 'get_agent_params') - def test_parse_root_device_hints(self, mock_get_params): - mock_get_params.return_value = { - 'root_device': 'vendor=SpongeBob,model=Square%20Pants', - 'ipa-api-url': 'http://1.2.3.4:1234' - } - expected = {'vendor': 'spongebob', 'model': 'square pants'} - result = utils.parse_root_device_hints() - self.assertEqual(expected, result) - - @mock.patch.object(utils, 'get_agent_params') - def test_parse_root_device_hints_no_hints(self, mock_get_params): - mock_get_params.return_value = { - 'ipa-api-url': 'http://1.2.3.4:1234' - } - result = utils.parse_root_device_hints() - self.assertEqual({}, result) - - @mock.patch.object(utils, 'get_agent_params') - def test_parse_root_device_size(self, mock_get_params): - mock_get_params.return_value = { - 'root_device': 'size=12345', - 'ipa-api-url': 'http://1.2.3.4:1234' - } - result = utils.parse_root_device_hints() - self.assertEqual(12345, result['size']) - - @mock.patch.object(utils, 'get_agent_params') - def test_parse_root_device_not_supported(self, mock_get_params): - mock_get_params.return_value = { - 'root_device': 'foo=bar,size=12345', - 'ipa-api-url': 'http://1.2.3.4:1234' - } - self.assertRaises(errors.DeviceNotFound, - utils.parse_root_device_hints) - class TestFailures(testtools.TestCase): def test_get_error(self): diff --git a/ironic_python_agent/utils.py b/ironic_python_agent/utils.py index d647e2dd0..df5d7c67a 100644 --- a/ironic_python_agent/utils.py +++ b/ironic_python_agent/utils.py @@ -233,44 +233,6 @@ def normalize(string): return parse.unquote(string).lower().strip() -def parse_root_device_hints(): - """Parse the root device hints. - - Parse the root device hints given by Ironic via kernel cmdline - or vmedia. - - :returns: A dict with the hints or an empty dict if no hints are - passed. - :raises: DeviceNotFound if there are unsupported hints. - - """ - root_device = get_agent_params().get('root_device') - if not root_device: - return {} - - hints = dict((item.split('=') for item in root_device.split(','))) - - # Find invalid hints for logging - not_supported = set(hints) - SUPPORTED_ROOT_DEVICE_HINTS - if not_supported: - error_msg = ('No device can be found because the following hints: ' - '"%(not_supported)s" are not supported by this version ' - 'of IPA. Supported hints are: "%(supported)s"', - {'not_supported': ', '.join(not_supported), - 'supported': ', '.join(SUPPORTED_ROOT_DEVICE_HINTS)}) - raise errors.DeviceNotFound(error_msg) - - # Normalise the values - hints = {k: normalize(v) for k, v in hints.items()} - - if 'size' in hints: - # NOTE(lucasagomes): Ironic should validate before passing to - # the deploy ramdisk - hints['size'] = int(hints['size']) - - return hints - - class AccumulatedFailures(object): """Object to accumulate failures without raising exception.""" diff --git a/releasenotes/notes/device-hints-from-node-object-9a689f5a4175a1a6.yaml b/releasenotes/notes/device-hints-from-node-object-9a689f5a4175a1a6.yaml new file mode 100644 index 000000000..b5e755bd0 --- /dev/null +++ b/releasenotes/notes/device-hints-from-node-object-9a689f5a4175a1a6.yaml @@ -0,0 +1,6 @@ +--- +features: + - IPA now gets the root device hints directly from the node object + representation instead of the kernel command line. This is a required + work in order to support a more complex syntax for the hints in + the future.