From 418d2355ee8b5c5bb131a40a1f811d264075387b Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Wed, 20 Jan 2021 12:11:54 +0100 Subject: [PATCH] Refactor vendor detection and add Redfish implementation Get rid of the TODO in the code and prepare for more management interfaces supporting detect_vendor(). Vendor detecting now runs during transition to manageable and on power state sync (essentially same as before but for all drivers not only IPMI). Update the IPMI implementation to no longer hide exceptions since they're not handled on the upper level. Simplify the regex and fix the docstring. Add the Redfish implementation as a foundation for future vendor-specific changes. Change-Id: Ie521cf2295613dde5842cbf9a053540a40be4b9c (cherry picked from commit 121b3348c813eb075ca38bd264a9315ee3acc2fe) (cherry picked from commit 4df73e47e1042dcd3cc693520cf63712df0bb875) --- ironic/conductor/manager.py | 7 ++ ironic/conductor/utils.py | 31 ++++++++ ironic/drivers/modules/ipmitool.py | 26 ++----- ironic/drivers/modules/redfish/management.py | 15 ++++ ironic/tests/unit/conductor/test_manager.py | 24 +++++- ironic/tests/unit/conductor/test_utils.py | 73 +++++++++++++++++++ .../modules/redfish/test_management.py | 8 ++ 7 files changed, 163 insertions(+), 21 deletions(-) diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py index 5607bd658d..9fed188218 100644 --- a/ironic/conductor/manager.py +++ b/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 diff --git a/ironic/conductor/utils.py b/ironic/conductor/utils.py index 43aaeee7e1..2f188a4377 100644 --- a/ironic/conductor/utils.py +++ b/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() diff --git a/ironic/drivers/modules/ipmitool.py b/ironic/drivers/modules/ipmitool.py index da73549e79..9bead2d796 100644 --- a/ironic/drivers/modules/ipmitool.py +++ b/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. - - 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. + """Detects and returns the hardware vendor. :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) - out, err = _exec_ipmitool(driver_info, "mc info") - re_obj = re.search("Manufacturer Name .*: (.+)", out) - if re_obj: - 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}) + driver_info = _parse_driver_info(task.node) + out, err = _exec_ipmitool(driver_info, "mc info") + re_obj = re.search("Manufacturer Name *: (.+)", out) + if re_obj: + return re_obj.group(1).lower() @METRICS.timer('IPMIManagement.get_sensors_data') def get_sensors_data(self, task): diff --git a/ironic/drivers/modules/redfish/management.py b/ironic/drivers/modules/redfish/management.py index 04e3d96b0c..a018cd8f89 100644 --- a/ironic/drivers/modules/redfish/management.py +++ b/ironic/drivers/modules/redfish/management.py @@ -490,3 +490,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 diff --git a/ironic/tests/unit/conductor/test_manager.py b/ironic/tests/unit/conductor/test_manager.py index 2a2bc68416..38720ade2e 100644 --- a/ironic/tests/unit/conductor/test_manager.py +++ b/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))) diff --git a/ironic/tests/unit/conductor/test_utils.py b/ironic/tests/unit/conductor/test_utils.py index 86d51b7055..2bc935dde3 100644 --- a/ironic/tests/unit/conductor/test_utils.py +++ b/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) diff --git a/ironic/tests/unit/drivers/modules/redfish/test_management.py b/ironic/tests/unit/drivers/modules/redfish/test_management.py index 5bc5aaf418..ddc88ff3f7 100644 --- a/ironic/tests/unit/drivers/modules/redfish/test_management.py +++ b/ironic/tests/unit/drivers/modules/redfish/test_management.py @@ -632,3 +632,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)