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

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(

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:

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()
+        cached_node = get_cached_node()
-        root_device_hints = utils.parse_root_device_hints()
+        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

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,
+        mock_cached_node.return_value = {
-                                         'wwn': 'fake-wwn',
+            'properties': {
-                                         'serial': 'fake-serial',
+                'root_device': {
-                                         'vendor': 'fake-vendor',
+                    'model': model,
-                                         'size': 10}
+                    '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):

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):

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."""
 

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.