diff options
| -rw-r--r-- | ironic/conductor/manager.py | 6 | ||||
| -rw-r--r-- | ironic/conductor/utils.py | 31 | ||||
| -rw-r--r-- | ironic/drivers/modules/ipmitool.py | 45 | ||||
| -rw-r--r-- | ironic/drivers/modules/redfish/management.py | 15 | ||||
| -rw-r--r-- | ironic/tests/unit/conductor/test_manager.py | 24 | ||||
| -rw-r--r-- | ironic/tests/unit/conductor/test_utils.py | 73 | ||||
| -rw-r--r-- | ironic/tests/unit/drivers/modules/redfish/test_management.py | 8 | ||||
| -rw-r--r-- | ironic/tests/unit/drivers/modules/test_ipmitool.py | 21 |
8 files changed, 166 insertions, 57 deletions
diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py index cacae082b..d22cd8525 100644 --- a/ironic/conductor/manager.py +++ b/ironic/conductor/manager.py @@ -1171,6 +1171,8 @@ class ConductorManager(base_manager.BaseConductorManager): if error is None: # Retrieve BIOS config settings for this node utils.node_cache_bios_settings(task, node) + # Cache the vendor if possible + utils.node_cache_vendor(task) if power_state != node.power_state: old_power_state = node.power_state @@ -3572,6 +3574,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 fccb9261c..d0e2bbd15 100644 --- a/ironic/conductor/utils.py +++ b/ironic/conductor/utils.py @@ -1359,3 +1359,34 @@ def node_cache_bios_settings(task, node): msg = (_('Caching of bios settings failed on node %(node)s.') % {'node': node.uuid}) LOG.exception(msg) + + +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 13391ef4e..9020b8234 100644 --- a/ironic/drivers/modules/ipmitool.py +++ b/ironic/drivers/modules/ipmitool.py @@ -939,25 +939,6 @@ class IPMIPower(base.PowerInterface): call). """ - # NOTE(TheJulia): Temporary until we promote detect_vendor as - # a higher level management method and able to automatically - # run detection upon either power state sync or in the enrollment - # to management step. - try: - properties = task.node.properties - if not properties.get('vendor'): - # We have no vendor stored, so we'll go ahead and - # call to store it. - vendor = task.driver.management.detect_vendor(task) - if vendor: - task.upgrade_lock() - props = task.node.properties - props['vendor'] = vendor - task.node.properties = props - task.node.save() - except exception.UnsupportedDriverExtension: - pass - driver_info = _parse_driver_info(task.node) return _power_status(driver_info) @@ -1244,12 +1225,7 @@ class IPMIManagement(base.ManagementInterface): @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 @@ -1258,20 +1234,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 2e89d0aa1..9d49a21eb 100644 --- a/ironic/drivers/modules/redfish/management.py +++ b/ironic/drivers/modules/redfish/management.py @@ -677,6 +677,21 @@ class RedfishManagement(base.ManagementInterface): 'component': component, 'uuid': task.node.uuid}) + 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 + @METRICS.timer('RedfishManagement.update_firmware') @base.clean_step(priority=0, abortable=False, argsinfo={ 'firmware_images': { diff --git a/ironic/tests/unit/conductor/test_manager.py b/ironic/tests/unit/conductor/test_manager.py index b3d3f4251..c5fd84279 100644 --- a/ironic/tests/unit/conductor/test_manager.py +++ b/ironic/tests/unit/conductor/test_manager.py @@ -2981,6 +2981,7 @@ 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', autospec=True) @mock.patch('ironic.drivers.modules.fake.FakePower.get_power_state', @@ -2988,7 +2989,7 @@ class DoNodeVerifyTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): @mock.patch('ironic.drivers.modules.fake.FakePower.validate', autospec=True) 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 @@ -3003,6 +3004,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() @@ -4672,6 +4674,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, @@ -4984,6 +4988,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 a41559898..8d0bf5f35 100644 --- a/ironic/tests/unit/conductor/test_utils.py +++ b/ironic/tests/unit/conductor/test_utils.py @@ -2195,3 +2195,76 @@ class StoreAgentCertificateTestCase(db_base.DbTestCase): self.assertEqual(self.fname, result) with open(self.fname, 'rt') as fp: self.assertEqual('cert text', fp.read()) + + +@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 a1fb4e8d0..dc67e17d6 100644 --- a/ironic/tests/unit/drivers/modules/redfish/test_management.py +++ b/ironic/tests/unit/drivers/modules/redfish/test_management.py @@ -702,6 +702,14 @@ class RedfishManagementTestCase(db_base.DbTestCase): self.assertEqual(indicator_states.ON, state) + @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) + @mock.patch.object(manager_utils, 'node_power_action', autospec=True) @mock.patch.object(deploy_utils, 'get_async_step_return_state', autospec=True) diff --git a/ironic/tests/unit/drivers/modules/test_ipmitool.py b/ironic/tests/unit/drivers/modules/test_ipmitool.py index f4d54e74c..7a172ec2d 100644 --- a/ironic/tests/unit/drivers/modules/test_ipmitool.py +++ b/ironic/tests/unit/drivers/modules/test_ipmitool.py @@ -1541,16 +1541,14 @@ class IPMIToolPrivateMethodTestCase( self.assertEqual(expected, mock_exec.call_args_list) - @mock.patch.object(ipmi.IPMIManagement, 'detect_vendor', autospec=True) @mock.patch.object(ipmi, '_exec_ipmitool', autospec=True) @mock.patch('oslo_utils.eventletutils.EventletEvent.wait', autospec=True) - def test__soft_power_off(self, sleep_mock, mock_exec, mock_vendor): + def test__soft_power_off(self, sleep_mock, mock_exec): def side_effect(driver_info, command, **kwargs): resp_dict = {"power status": ["Chassis Power is off\n", None], "power soft": [None, None]} return resp_dict.get(command, ["Bad\n", None]) - mock_vendor.return_value = None mock_exec.side_effect = side_effect expected = [mock.call(self.info, "power soft"), @@ -1561,13 +1559,10 @@ class IPMIToolPrivateMethodTestCase( self.assertEqual(expected, mock_exec.call_args_list) self.assertEqual(states.POWER_OFF, state) - self.assertTrue(mock_vendor.called) - @mock.patch.object(ipmi.IPMIManagement, 'detect_vendor', autospec=True) @mock.patch.object(ipmi, '_exec_ipmitool', autospec=True) @mock.patch('oslo_utils.eventletutils.EventletEvent.wait', autospec=True) - def test__soft_power_off_max_retries(self, sleep_mock, mock_exec, - mock_vendor): + def test__soft_power_off_max_retries(self, sleep_mock, mock_exec): def side_effect(driver_info, command, **kwargs): resp_dict = {"power status": ["Chassis Power is on\n", None], @@ -1586,8 +1581,6 @@ class IPMIToolPrivateMethodTestCase( ipmi._soft_power_off, task, self.info, timeout=2) self.assertEqual(expected, mock_exec.call_args_list) - # Should be removed when detect_vendor automatic invocation is moved. - self.assertFalse(mock_vendor.called) @mock.patch.object(ipmi, '_power_status', autospec=True) @mock.patch.object(ipmi, '_exec_ipmitool', autospec=True) @@ -1661,9 +1654,8 @@ class IPMIToolDriverTestCase(Base): self.assertEqual(sorted(expected), sorted(task.driver.get_properties())) - @mock.patch.object(ipmi.IPMIManagement, 'detect_vendor', autospec=True) @mock.patch.object(ipmi, '_exec_ipmitool', autospec=True) - def test_get_power_state(self, mock_exec, mock_detect): + def test_get_power_state(self, mock_exec): returns = iter([["Chassis Power is off\n", None], ["Chassis Power is on\n", None], ["\n", None]]) @@ -1671,7 +1663,6 @@ class IPMIToolDriverTestCase(Base): mock.call(self.info, "power status", kill_on_timeout=True), mock.call(self.info, "power status", kill_on_timeout=True)] mock_exec.side_effect = returns - mock_detect.return_value = None with task_manager.acquire(self.context, self.node.uuid) as task: pstate = self.power.get_power_state(task) @@ -1684,20 +1675,16 @@ class IPMIToolDriverTestCase(Base): self.assertEqual(states.ERROR, pstate) self.assertEqual(mock_exec.call_args_list, expected) - self.assertEqual(3, mock_detect.call_count) - @mock.patch.object(ipmi.IPMIManagement, 'detect_vendor', autospec=True) @mock.patch.object(ipmi, '_exec_ipmitool', autospec=True) - def test_get_power_state_exception(self, mock_exec, mock_vendor): + def test_get_power_state_exception(self, mock_exec): mock_exec.side_effect = processutils.ProcessExecutionError("error") - mock_vendor.return_value = None with task_manager.acquire(self.context, self.node.uuid) as task: self.assertRaises(exception.IPMIFailure, self.power.get_power_state, task) mock_exec.assert_called_once_with(self.info, "power status", kill_on_timeout=True) - self.assertEqual(1, mock_vendor.call_count) @mock.patch.object(ipmi, '_power_on', autospec=True) @mock.patch.object(ipmi, '_power_off', autospec=True) |