Merge "Refactor vendor detection and add Redfish implementation" into stable/train

ironic/conductor/manager.py

@@ -1537,6 +1537,9 @@ class ConductorManager(base_manager.BaseConductorManager):
                          {'node': node.uuid, 'msg': e})
 
         if error is None:
+            # Cache the vendor if possible
+            utils.node_cache_vendor(task)
+
             if power_state != node.power_state:
                 old_power_state = node.power_state
                 node.power_state = power_state
@@ -4157,6 +4160,10 @@ def do_sync_power_state(task, count):
                          'retries': max_retries, 'err': e})
         return count
 
+    # Make sure we have the vendor cached (if for some reason it failed during
+    # the transition to manageable or a really old API version was used).
+    utils.node_cache_vendor(task)
+
     if node.power_state and node.power_state == power_state:
         # No action is needed
         return 0

ironic/conductor/utils.py

@@ -928,3 +928,34 @@ def remove_agent_url(node):
     info = node.driver_internal_info
     info.pop('agent_url', None)
     node.driver_internal_info = info
+
+
+def node_cache_vendor(task):
+    """Cache the vendor if it can be detected."""
+    properties = task.node.properties
+    if properties.get('vendor'):
+        return  # assume that vendors don't change on fly
+
+    try:
+        # We have no vendor stored, so we'll go ahead and
+        # call to store it.
+        vendor = task.driver.management.detect_vendor(task)
+        if not vendor:
+            return
+
+        # This function may be called without an exclusive lock, so get one
+        task.upgrade_lock(purpose='caching node vendor')
+    except exception.UnsupportedDriverExtension:
+        return
+    except Exception as exc:
+        LOG.warning('Unexpected exception when trying to detect vendor '
+                    'for node %(node)s. %(class)s: %(exc)s',
+                    {'node': task.node.uuid,
+                     'class': type(exc).__name__, 'exc': exc},
+                    exc_info=not isinstance(exc, exception.IronicException))
+        return
+
+    props = task.node.properties
+    props['vendor'] = vendor
+    task.node.properties = props
+    task.node.save()

ironic/drivers/modules/ipmitool.py

@@ -1234,12 +1234,7 @@ class IPMIManagement(base.ManagementInterface):
     @task_manager.require_exclusive_lock
     @METRICS.timer('IPMIManagement.detect_vendor')
     def detect_vendor(self, task):
-        """Detects, stores, and returns the hardware vendor.
+        """Detects and returns the hardware vendor.
-
-        If the Node object ``properties`` field does not already contain
-        a ``vendor`` field, then this method is intended to query
-        Detects the BMC hardware vendor and stores the returned value
-        with-in the Node object ``properties`` field if detected.
 
         :param task: A task from TaskManager.
         :raises: InvalidParameterValue if an invalid component, indicator
@@ -1248,20 +1243,11 @@ class IPMIManagement(base.ManagementInterface):
         :returns: String representing the BMC reported Vendor or
                   Manufacturer, otherwise returns None.
         """
-        try:
+        driver_info = _parse_driver_info(task.node)
-            driver_info = _parse_driver_info(task.node)
+        out, err = _exec_ipmitool(driver_info, "mc info")
-            out, err = _exec_ipmitool(driver_info, "mc info")
+        re_obj = re.search("Manufacturer Name *: (.+)", out)
-            re_obj = re.search("Manufacturer Name .*: (.+)", out)
+        if re_obj:
-            if re_obj:
+            return re_obj.group(1).lower()
-                bmc_vendor = str(re_obj.groups('')[0]).lower().split(':')
-                # Pull unparsed data and save the vendor
-                return bmc_vendor[-1]
-        except (exception.PasswordFileFailedToCreate,
-                processutils.ProcessExecutionError) as e:
-            LOG.warning('IPMI get boot device failed to detect vendor '
-                        'of bmc for %(node)s. Error %(err)s',
-                        {'node': task.node.uuid,
-                         'err': e})
 
     @METRICS.timer('IPMIManagement.get_sensors_data')
     def get_sensors_data(self, task):

ironic/drivers/modules/redfish/management.py

@@ -503,3 +503,18 @@ class RedfishManagement(base.ManagementInterface):
                                                   'error': e})
             LOG.error(error_msg)
             raise exception.RedfishError(error=error_msg)
+
+    def detect_vendor(self, task):
+        """Detects and returns the hardware vendor.
+
+        Uses the System's Manufacturer field.
+
+        :param task: A task from TaskManager.
+        :raises: InvalidParameterValue if an invalid component, indicator
+            or state is specified.
+        :raises: MissingParameterValue if a required parameter is missing
+        :raises: RedfishError on driver-specific problems.
+        :returns: String representing the BMC reported Vendor or
+                  Manufacturer, otherwise returns None.
+        """
+        return redfish_utils.get_system(task.node).manufacturer

ironic/tests/unit/conductor/test_manager.py

@@ -5050,11 +5050,12 @@ class DoNodeRescueTestCase(mgr_utils.CommonMixIn, mgr_utils.ServiceSetUpMixin,
 
 @mgr_utils.mock_record_keepalive
 class DoNodeVerifyTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase):
+    @mock.patch.object(conductor_utils, 'node_cache_vendor', autospec=True)
     @mock.patch('ironic.objects.node.NodeCorrectedPowerStateNotification')
     @mock.patch('ironic.drivers.modules.fake.FakePower.get_power_state')
     @mock.patch('ironic.drivers.modules.fake.FakePower.validate')
     def test__do_node_verify(self, mock_validate, mock_get_power_state,
-                             mock_notif):
+                             mock_notif, mock_cache_vendor):
         self._start_service()
         mock_get_power_state.return_value = states.POWER_OFF
         # Required for exception handling
@@ -5069,6 +5070,7 @@ class DoNodeVerifyTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase):
         with task_manager.acquire(
                 self.context, node['id'], shared=False) as task:
             self.service._do_node_verify(task)
+            mock_cache_vendor.assert_called_once_with(task)
 
         self._stop_service()
 
@@ -6643,6 +6645,8 @@ class ManagerDoSyncPowerStateTestCase(db_base.DbTestCase):
         super(ManagerDoSyncPowerStateTestCase, self).setUp()
         self.service = manager.ConductorManager('hostname', 'test-topic')
         self.driver = mock.Mock(spec_set=drivers_base.BareDriver)
+        self.driver.management.detect_vendor.side_effect = \
+            exception.UnsupportedDriverExtension
         self.power = self.driver.power
         self.node = obj_utils.create_test_node(
             self.context, driver='fake-hardware', maintenance=False,
@@ -6938,6 +6942,24 @@ class ManagerDoSyncPowerStateTestCase(db_base.DbTestCase):
         self.assertFalse(node_power_action.called)
         self.task.upgrade_lock.assert_called_once_with()
 
+    @mock.patch.object(nova, 'power_update', autospec=True)
+    def test_vendor_detection(self, mock_power_update, node_power_action):
+        self.driver.management.detect_vendor.side_effect = [
+            "Fake Inc."
+        ]
+        self._do_sync_power_state(states.POWER_ON, states.POWER_OFF)
+
+        self.assertFalse(self.power.validate.called)
+        self.power.get_power_state.assert_called_once_with(self.task)
+        self.assertFalse(node_power_action.called)
+        self.assertEqual(states.POWER_OFF, self.node.power_state)
+        # node_cache_vendor calls upgrade_lock, then power update does it once
+        # more (which is safe because TaskManager checks its state)
+        self.assertEqual(2, self.task.upgrade_lock.call_count)
+        mock_power_update.assert_called_once_with(
+            self.task.context, self.node.instance_uuid, states.POWER_OFF)
+        self.assertEqual("Fake Inc.", self.node.properties['vendor'])
+
 
 @mock.patch.object(waiters, 'wait_for_all',
                    new=mock.MagicMock(return_value=(0, 0)))

ironic/tests/unit/conductor/test_utils.py

@@ -1915,3 +1915,76 @@ class FastTrackTestCase(db_base.DbTestCase):
         with task_manager.acquire(
                 self.context, self.node.uuid, shared=False) as task:
             self.assertFalse(conductor_utils.is_fast_track(task))
+
+
+@mock.patch.object(fake.FakeManagement, 'detect_vendor', autospec=True,
+                   return_value="Fake Inc.")
+class CacheVendorTestCase(db_base.DbTestCase):
+
+    def setUp(self):
+        super(CacheVendorTestCase, self).setUp()
+        self.node = obj_utils.create_test_node(self.context,
+                                               driver='fake-hardware',
+                                               properties={})
+
+    def test_ok(self, mock_detect):
+        with task_manager.acquire(self.context, self.node.id,
+                                  shared=True) as task:
+            conductor_utils.node_cache_vendor(task)
+            self.assertFalse(task.shared)
+            mock_detect.assert_called_once_with(task.driver.management, task)
+
+        self.node.refresh()
+        self.assertEqual("Fake Inc.", self.node.properties['vendor'])
+
+    def test_already_present(self, mock_detect):
+        self.node.properties = {'vendor': "Fake GmbH"}
+        self.node.save()
+
+        with task_manager.acquire(self.context, self.node.id,
+                                  shared=True) as task:
+            conductor_utils.node_cache_vendor(task)
+            self.assertTrue(task.shared)
+
+        self.node.refresh()
+        self.assertEqual("Fake GmbH", self.node.properties['vendor'])
+        self.assertFalse(mock_detect.called)
+
+    def test_empty(self, mock_detect):
+        mock_detect.return_value = None
+        with task_manager.acquire(self.context, self.node.id,
+                                  shared=True) as task:
+            conductor_utils.node_cache_vendor(task)
+            self.assertTrue(task.shared)
+            mock_detect.assert_called_once_with(task.driver.management, task)
+
+        self.node.refresh()
+        self.assertNotIn('vendor', self.node.properties)
+
+    @mock.patch.object(conductor_utils.LOG, 'warning', autospec=True)
+    def test_unsupported(self, mock_log, mock_detect):
+        mock_detect.side_effect = exception.UnsupportedDriverExtension
+
+        with task_manager.acquire(self.context, self.node.id,
+                                  shared=True) as task:
+            conductor_utils.node_cache_vendor(task)
+            self.assertTrue(task.shared)
+            mock_detect.assert_called_once_with(task.driver.management, task)
+
+        self.node.refresh()
+        self.assertNotIn('vendor', self.node.properties)
+        self.assertFalse(mock_log.called)
+
+    @mock.patch.object(conductor_utils.LOG, 'warning', autospec=True)
+    def test_failed(self, mock_log, mock_detect):
+        mock_detect.side_effect = RuntimeError
+
+        with task_manager.acquire(self.context, self.node.id,
+                                  shared=True) as task:
+            conductor_utils.node_cache_vendor(task)
+            self.assertTrue(task.shared)
+            mock_detect.assert_called_once_with(task.driver.management, task)
+
+        self.node.refresh()
+        self.assertNotIn('vendor', self.node.properties)
+        self.assertTrue(mock_log.called)

ironic/tests/unit/drivers/modules/redfish/test_management.py

@@ -664,3 +664,11 @@ class RedfishManagementTestCase(db_base.DbTestCase):
             fake_system.reset_system.assert_called_once_with(
                 sushy.RESET_NMI)
             mock_get_system.assert_called_once_with(task.node)
+
+    @mock.patch.object(redfish_utils, 'get_system', autospec=True)
+    def test_detect_vendor(self, mock_get_system):
+        mock_get_system.return_value.manufacturer = "Fake GmbH"
+        with task_manager.acquire(self.context, self.node.uuid,
+                                  shared=True) as task:
+            response = task.driver.management.detect_vendor(task)
+            self.assertEqual("Fake GmbH", response)