Merge "Get root device hints from the node object"

ironic_python_agent/agent.py

@@ -311,6 +311,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.