From 68170e04110bfd3cb0aad086ccc5bd037e03608a Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Fri, 2 Dec 2022 13:17:45 +0000 Subject: [PATCH] Use SDK for remaining ironic driver calls We would like nova not to use ironicclient, but instead to invoke the ironic API directly through an openstacksdk connection. This concludes our migration of the driver and switches the remaining ironic calls to use SDK. Change-Id: Ibb5b168ee0944463b996e96f033bd3dfb498e304 Signed-off-by: Stephen Finucane --- nova/tests/unit/virt/ironic/test_driver.py | 270 +++++++++++---------- nova/tests/unit/virt/ironic/utils.py | 17 +- nova/virt/ironic/driver.py | 103 ++++---- 3 files changed, 194 insertions(+), 196 deletions(-) diff --git a/nova/tests/unit/virt/ironic/test_driver.py b/nova/tests/unit/virt/ironic/test_driver.py index a8da85a0b2e9..9b32fd288b5d 100644 --- a/nova/tests/unit/virt/ironic/test_driver.py +++ b/nova/tests/unit/virt/ironic/test_driver.py @@ -19,7 +19,7 @@ import base64 from unittest import mock import fixtures -from ironicclient import exc as ironic_exception +from openstack.baremetal.v1 import node as _node from openstack import exceptions as sdk_exc from oslo_config import cfg from oslo_service import loopingcall @@ -1271,13 +1271,12 @@ class IronicDriverTestCase(test.NoDBTestCase): @mock.patch.object(objects.Instance, 'save') @mock.patch.object(loopingcall, 'FixedIntervalLoopingCall') - @mock.patch.object(FAKE_CLIENT, 'node') @mock.patch.object(ironic_driver.IronicDriver, '_add_volume_target_info') @mock.patch.object(ironic_driver.IronicDriver, '_wait_for_active') @mock.patch.object(ironic_driver.IronicDriver, '_add_instance_info_to_node') def _test_spawn(self, mock_aiitn, mock_wait_active, - mock_avti, mock_node, mock_looping, mock_save, + mock_avti, mock_looping, mock_save, config_drive_value=None): node_id = 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee' node = _get_cached_node(driver='fake', id=node_id) @@ -1286,7 +1285,8 @@ class IronicDriverTestCase(test.NoDBTestCase): instance.flavor = fake_flavor self.mock_conn.get_node.return_value = node - mock_node.validate.return_value = ironic_utils.get_test_validation() + self.mock_conn.validate_node.return_value = \ + ironic_utils.get_test_validation() fake_looping_call = FakeLoopingCall() mock_looping.return_value = fake_looping_call @@ -1296,8 +1296,11 @@ class IronicDriverTestCase(test.NoDBTestCase): self.driver.spawn(self.ctx, instance, image_meta, [], None, {}) self.mock_conn.get_node.assert_called_once_with( - node_id, fields=ironic_driver._NODE_FIELDS) - mock_node.validate.assert_called_once_with(node_id) + node_id, fields=ironic_driver._NODE_FIELDS, + ) + self.mock_conn.validate_node.assert_called_once_with( + node_id, required=None, + ) mock_aiitn.assert_called_once_with(node, instance, test.MatchType(objects.ImageMeta), fake_flavor, block_device_info=None) @@ -1336,7 +1339,6 @@ class IronicDriverTestCase(test.NoDBTestCase): @mock.patch.object(configdrive, 'required_by') @mock.patch.object(loopingcall, 'FixedIntervalLoopingCall') - @mock.patch.object(FAKE_CLIENT, 'node') @mock.patch.object(ironic_driver.IronicDriver, 'destroy') @mock.patch.object(ironic_driver.IronicDriver, '_add_volume_target_info') @mock.patch.object(ironic_driver.IronicDriver, '_wait_for_active') @@ -1344,7 +1346,7 @@ class IronicDriverTestCase(test.NoDBTestCase): '_add_instance_info_to_node') def test_spawn_destroyed_after_failure(self, mock_aiitn, mock_wait_active, mock_avti, - mock_destroy, mock_node, + mock_destroy, mock_looping, mock_required_by): mock_required_by.return_value = False node_uuid = 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee' @@ -1353,8 +1355,9 @@ class IronicDriverTestCase(test.NoDBTestCase): instance = fake_instance.fake_instance_obj(self.ctx, node=node_uuid) instance.flavor = fake_flavor - mock_node.get.return_value = node - mock_node.validate.return_value = ironic_utils.get_test_validation() + self.mock_conn.get_node.return_value = node + self.mock_conn.validate_node.return_value = \ + ironic_utils.get_test_validation() fake_looping_call = FakeLoopingCall() mock_looping.return_value = fake_looping_call @@ -1538,9 +1541,8 @@ class IronicDriverTestCase(test.NoDBTestCase): ) @mock.patch.object(configdrive, 'required_by') - @mock.patch.object(FAKE_CLIENT, 'node') @mock.patch.object(ironic_driver.IronicDriver, '_add_volume_target_info') - def test_spawn_node_driver_validation_fail(self, mock_avti, mock_node, + def test_spawn_node_driver_validation_fail(self, mock_avti, mock_required_by): mock_required_by.return_value = False node_id = 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee' @@ -1549,9 +1551,12 @@ class IronicDriverTestCase(test.NoDBTestCase): instance = fake_instance.fake_instance_obj(self.ctx, node=node_id) instance.flavor = flavor - mock_node.validate.return_value = ironic_utils.get_test_validation( - power={'result': False}, deploy={'result': False}, - storage={'result': False}) + self.mock_conn.validate_node.return_value = \ + ironic_utils.get_test_validation( + power=_node.ValidationResult(result=False, reason=None), + deploy=_node.ValidationResult(result=False, reason=None), + storage=_node.ValidationResult(result=False, reason=None), + ) self.mock_conn.get_node.return_value = node image_meta = ironic_utils.get_test_image_meta() @@ -1560,16 +1565,17 @@ class IronicDriverTestCase(test.NoDBTestCase): self.mock_conn.get_node.assert_called_once_with( node_id, fields=ironic_driver._NODE_FIELDS) mock_avti.assert_called_once_with(self.ctx, instance, None) - mock_node.validate.assert_called_once_with(node_id) + self.mock_conn.validate_node.assert_called_once_with( + node_id, required=None, + ) @mock.patch.object(configdrive, 'required_by') @mock.patch.object(objects.Instance, 'save') - @mock.patch.object(FAKE_CLIENT, 'node') @mock.patch.object(ironic_driver.IronicDriver, '_add_volume_target_info') @mock.patch.object(ironic_driver.IronicDriver, '_generate_configdrive') def test_spawn_node_configdrive_fail(self, mock_configdrive, - mock_avti, mock_node, mock_save, + mock_avti, mock_save, mock_required_by): mock_required_by.return_value = True node_id = 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee' @@ -1578,7 +1584,8 @@ class IronicDriverTestCase(test.NoDBTestCase): instance = fake_instance.fake_instance_obj(self.ctx, node=node_id) instance.flavor = flavor self.mock_conn.get_node.return_value = node - mock_node.validate.return_value = ironic_utils.get_test_validation() + self.mock_conn.validate_node.return_value = \ + ironic_utils.get_test_validation() image_meta = ironic_utils.get_test_image_meta() mock_configdrive.side_effect = test.TestingException() @@ -1589,16 +1596,17 @@ class IronicDriverTestCase(test.NoDBTestCase): self.mock_conn.get_node.assert_called_once_with( node_id, fields=ironic_driver._NODE_FIELDS) - mock_node.validate.assert_called_once_with(node_id) + self.mock_conn.validate_node.assert_called_once_with( + node_id, required=None, + ) mock_cleanup_deploy.assert_called_with(node, instance, None) @mock.patch.object(configdrive, 'required_by') - @mock.patch.object(FAKE_CLIENT, 'node') @mock.patch.object(ironic_driver.IronicDriver, '_add_volume_target_info') @mock.patch.object(ironic_driver.IronicDriver, '_cleanup_deploy') def test_spawn_node_trigger_deploy_fail(self, mock_cleanup_deploy, mock_avti, - mock_node, mock_required_by): + mock_required_by): mock_required_by.return_value = False node_id = 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee' node = _get_cached_node(driver='fake', id=node_id) @@ -1608,7 +1616,8 @@ class IronicDriverTestCase(test.NoDBTestCase): image_meta = ironic_utils.get_test_image_meta() self.mock_conn.get_node.return_value = node - mock_node.validate.return_value = ironic_utils.get_test_validation() + self.mock_conn.validate_node.return_value = \ + ironic_utils.get_test_validation() self.mock_conn.set_node_provision_state.side_effect = \ sdk_exc.SDKException('foo') @@ -1620,18 +1629,19 @@ class IronicDriverTestCase(test.NoDBTestCase): self.mock_conn.get_node.assert_called_once_with( node_id, fields=ironic_driver._NODE_FIELDS) - mock_node.validate.assert_called_once_with(node_id) + self.mock_conn.validate_node.assert_called_once_with( + node_id, required=None, + ) mock_cleanup_deploy.assert_called_once_with(node, instance, None) @mock.patch.object(configdrive, 'required_by') @mock.patch.object(loopingcall, 'FixedIntervalLoopingCall') @mock.patch.object(objects.Instance, 'save') - @mock.patch.object(FAKE_CLIENT, 'node') @mock.patch.object(ironic_driver.IronicDriver, '_add_volume_target_info') @mock.patch.object(ironic_driver.IronicDriver, '_wait_for_active') def test_spawn_sets_default_ephemeral_device(self, mock_wait, mock_avti, - mock_node, mock_save, + mock_save, mock_looping, mock_required_by): mock_required_by.return_value = False @@ -1645,12 +1655,11 @@ class IronicDriverTestCase(test.NoDBTestCase): self.assertTrue(mock_save.called) self.assertEqual('/dev/sda1', instance.default_ephemeral_device) - @mock.patch.object(FAKE_CLIENT, 'node') @mock.patch.object(ironic_driver.IronicDriver, '_remove_instance_info_from_node') @mock.patch.object(ironic_driver.IronicDriver, '_cleanup_deploy') def _test_destroy(self, state, mock_cleanup_deploy, - mock_remove_instance_info, mock_node): + mock_remove_instance_info): node_id = 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee' network_info = 'foo' @@ -1786,8 +1795,7 @@ class IronicDriverTestCase(test.NoDBTestCase): node.uuid, "deleted", ) - @mock.patch.object(FAKE_CLIENT, 'node') - def test_destroy_unassociate_fail(self, mock_node): + def test_destroy_unassociate_fail(self): node_id = 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee' node = _get_cached_node( driver='fake', id=node_id, @@ -1828,25 +1836,24 @@ class IronicDriverTestCase(test.NoDBTestCase): @mock.patch.object(ironic_driver.IronicDriver, '_validate_instance_and_node') - @mock.patch.object(FAKE_CLIENT.node, 'inject_nmi') - def test_trigger_crash_dump(self, mock_nmi, fake_validate): + def test_trigger_crash_dump(self, fake_validate): node = _get_cached_node() fake_validate.return_value = node instance = fake_instance.fake_instance_obj(self.ctx, node=node.uuid) self.driver.trigger_crash_dump(instance) - mock_nmi.assert_called_once_with(node.uuid) + self.mock_conn.inject_nmi_to_node.assert_called_once_with(node.uuid) @mock.patch.object(ironic_driver.IronicDriver, '_validate_instance_and_node') - @mock.patch.object(FAKE_CLIENT.node, 'inject_nmi') - def test_trigger_crash_dump_error(self, mock_nmi, fake_validate): + def test_trigger_crash_dump_error(self, fake_validate): node = _get_cached_node() fake_validate.return_value = node - mock_nmi.side_effect = ironic_exception.BadRequest() + self.mock_conn.inject_nmi_to_node.side_effect = \ + sdk_exc.BadRequestException() instance = fake_instance.fake_instance_obj(self.ctx, node=node.uuid) - self.assertRaises(ironic_exception.BadRequest, + self.assertRaises(sdk_exc.BadRequestException, self.driver.trigger_crash_dump, instance) @mock.patch.object(loopingcall, 'FixedIntervalLoopingCall') @@ -2440,9 +2447,9 @@ class IronicDriverTestCase(test.NoDBTestCase): self.mock_conn.get_node.return_value = node instance = fake_instance.fake_instance_obj(self.ctx) network_info = utils.get_test_network_info() - mock_pvifs.side_effect = ironic_exception.BadRequest('fake error') + mock_pvifs.side_effect = sdk_exc.BadRequestException('fake error') self.assertRaises( - ironic_exception.BadRequest, + sdk_exc.BadRequestException, self.driver.prepare_networks_before_block_device_mapping, instance, network_info) mock_pvifs.assert_called_once_with(node, instance, network_info) @@ -2458,7 +2465,7 @@ class IronicDriverTestCase(test.NoDBTestCase): def test_clean_networks_preparation_error(self, mock_upvifs): instance = fake_instance.fake_instance_obj(self.ctx) network_info = utils.get_test_network_info() - mock_upvifs.side_effect = ironic_exception.BadRequest('fake error') + mock_upvifs.side_effect = sdk_exc.BadRequestException('fake error') self.driver.clean_networks_preparation(instance, network_info) mock_upvifs.assert_called_once_with(instance, network_info) @@ -2509,7 +2516,7 @@ class IronicDriverTestCase(test.NoDBTestCase): def test_prepare_for_spawn_invalid_instance(self): instance = fake_instance.fake_instance_obj(self.ctx, node=None) - self.assertRaises(ironic_exception.BadRequest, + self.assertRaises(exception.NovaException, self.driver.prepare_for_spawn, instance) @@ -2714,33 +2721,34 @@ class IronicDriverSyncTestCase(IronicDriverTestCase): self.driver.unrescue, self.ctx, instance, ) - def test__can_send_version(self): + @mock.patch('openstack.utils.supports_microversion') + def test__can_send_version(self, mock_supports_microversion): + mock_supports_microversion.return_value = True + + version = '%d.%d' % cw.IRONIC_API_VERSION self.assertIsNone( - self.driver._can_send_version( - min_version='%d.%d' % cw.IRONIC_API_VERSION)) + self.driver._can_send_version(version) + ) - def test__can_send_version_too_new(self): - self.assertRaises(exception.IronicAPIVersionNotAvailable, - self.driver._can_send_version, - min_version='%d.%d' % (cw.IRONIC_API_VERSION[0], - cw.IRONIC_API_VERSION[1] + 1)) + @mock.patch('openstack.utils.supports_microversion') + def test__can_send_version_too_new(self, mock_supports_microversion): + mock_supports_microversion.return_value = False - def test__can_send_version_too_old(self): + version = '%d.%d' % ( + cw.IRONIC_API_VERSION[0], cw.IRONIC_API_VERSION[1] + 1, + ) self.assertRaises( exception.IronicAPIVersionNotAvailable, self.driver._can_send_version, - max_version='%d.%d' % (cw.PRIOR_IRONIC_API_VERSION[0], - cw.PRIOR_IRONIC_API_VERSION[1] - 1)) + version, + ) - @mock.patch.object(cw.IronicClientWrapper, 'current_api_version', - autospec=True) - @mock.patch.object(cw.IronicClientWrapper, 'is_api_version_negotiated', - autospec=True) - def test__can_send_version_not_negotiated(self, mock_is_negotiated, - mock_api_version): - mock_is_negotiated.return_value = False - self.assertIsNone(self.driver._can_send_version()) - self.assertFalse(mock_api_version.called) + # DELETEME + def test__can_send_version_too_old(self): + pass + + def test__can_send_version_not_negotiated(self): + pass @mock.patch.object(instance_metadata, 'InstanceMetadata') @@ -2963,7 +2971,7 @@ class HashRingTestCase(test.NoDBTestCase): self.flags(peer_list=['host1', 'host2'], group='ironic') self._test__refresh_hash_ring(services, expected_hosts, uncalled=['host3']) - mock_can_send.assert_called_once_with(min_version='1.46') + mock_can_send.assert_called_once_with('1.46') @mock.patch.object(ironic_driver.IronicDriver, '_can_send_version') def test__refresh_hash_ring_peer_list_old_api(self, mock_can_send): @@ -2976,7 +2984,7 @@ class HashRingTestCase(test.NoDBTestCase): self.flags(peer_list=['host1', 'host2'], group='ironic') self._test__refresh_hash_ring(services, expected_hosts, uncalled=['host3']) - mock_can_send.assert_called_once_with(min_version='1.46') + mock_can_send.assert_called_once_with('1.46') @mock.patch.object(ironic_driver, 'LOG', autospec=True) def test__check_peer_list(self, mock_log): @@ -3214,13 +3222,12 @@ class NodeCacheTestCase(test.NoDBTestCase): self.assertEqual(expected_cache, self.driver.node_cache) -@mock.patch.object(FAKE_CLIENT, 'node') class IronicDriverConsoleTestCase(test.NoDBTestCase): @mock.patch.object(cw, 'IronicClientWrapper', lambda *_: FAKE_CLIENT_WRAPPER) @mock.patch.object(objects.ServiceList, 'get_all_computes_by_hv_type') def setUp(self, mock_services): - super(IronicDriverConsoleTestCase, self).setUp() + super().setUp() self.driver = ironic_driver.IronicDriver(fake.FakeVirtAPI()) @@ -3252,7 +3259,7 @@ class IronicDriverConsoleTestCase(test.NoDBTestCase): } } - def test__get_node_console_with_reset_success(self, mock_node): + def test__get_node_console_with_reset_success(self): temp_data = {'target_mode': True} def _fake_get_console(node_uuid): @@ -3262,27 +3269,27 @@ class IronicDriverConsoleTestCase(test.NoDBTestCase): # Set it up so that _fake_get_console() returns 'mode' temp_data['target_mode'] = mode - mock_node.get_console.side_effect = _fake_get_console - mock_node.set_console_mode.side_effect = _fake_set_console_mode + self.mock_conn.get_node_console.side_effect = _fake_get_console + self.mock_conn.set_node_console_mode.side_effect = \ + _fake_set_console_mode expected = self._create_console_data()['console_info'] result = self.driver._get_node_console_with_reset(self.instance) - self.assertGreater(mock_node.get_console.call_count, 1) - self.assertEqual(2, mock_node.set_console_mode.call_count) + self.assertGreater(self.mock_conn.get_node_console.call_count, 1) + self.assertEqual(2, self.mock_conn.set_node_console_mode.call_count) self.assertEqual(self.node.uuid, result['node'].uuid) self.assertThat(result['console_info'], nova_matchers.DictMatches(expected)) @mock.patch.object(ironic_driver, 'LOG', autospec=True) - def test__get_node_console_with_reset_console_disabled(self, mock_log, - mock_node): + def test__get_node_console_with_reset_console_disabled(self, mock_log): def _fake_log_debug(msg, *args, **kwargs): regex = r'Console is disabled for instance .*' self.assertThat(msg, matchers.MatchesRegex(regex)) - mock_node.get_console.return_value = \ + self.mock_conn.get_node_console.return_value = \ self._create_console_data(enabled=False) mock_log.debug.side_effect = _fake_log_debug @@ -3290,37 +3297,37 @@ class IronicDriverConsoleTestCase(test.NoDBTestCase): self.driver._get_node_console_with_reset, self.instance) - mock_node.get_console.assert_called_once_with(self.node.uuid) - mock_node.set_console_mode.assert_not_called() + self.mock_conn.get_node_console.assert_called_once_with(self.node.uuid) + self.mock_conn.set_node_console_mode.assert_not_called() self.assertTrue(mock_log.debug.called) @mock.patch.object(ironic_driver, 'LOG', autospec=True) - def test__get_node_console_with_reset_set_mode_failed(self, mock_log, - mock_node): + def test__get_node_console_with_reset_set_mode_failed(self, mock_log): def _fake_log_error(msg, *args, **kwargs): regex = r'Failed to set console mode .*' self.assertThat(msg, matchers.MatchesRegex(regex)) - mock_node.get_console.return_value = self._create_console_data() - mock_node.set_console_mode.side_effect = exception.NovaException() + self.mock_conn.get_node_console.return_value = \ + self._create_console_data() + self.mock_conn.set_node_console_mode.side_effect = \ + sdk_exc.SDKException() mock_log.error.side_effect = _fake_log_error self.assertRaises(exception.ConsoleNotAvailable, self.driver._get_node_console_with_reset, self.instance) - mock_node.get_console.assert_called_once_with(self.node.uuid) - self.assertEqual(2, mock_node.set_console_mode.call_count) + self.mock_conn.get_node_console.assert_called_once_with(self.node.uuid) + self.assertEqual(2, self.mock_conn.set_node_console_mode.call_count) self.assertTrue(mock_log.error.called) @mock.patch.object(ironic_driver, 'LOG', autospec=True) - def test__get_node_console_with_reset_wait_failed(self, mock_log, - mock_node): + def test__get_node_console_with_reset_wait_failed(self, mock_log): def _fake_get_console(node_uuid): - if mock_node.set_console_mode.called: + if self.mock_conn.set_node_console_mode.called: # After the call to set_console_mode(), then _wait_state() # will call _get_console() to check the result. - raise exception.NovaException() + raise sdk_exc.SDKException() else: return self._create_console_data() @@ -3328,23 +3335,22 @@ class IronicDriverConsoleTestCase(test.NoDBTestCase): regex = r'Failed to acquire console information for instance .*' self.assertThat(msg, matchers.MatchesRegex(regex)) - mock_node.get_console.side_effect = _fake_get_console + self.mock_conn.get_node_console.side_effect = _fake_get_console mock_log.error.side_effect = _fake_log_error self.assertRaises(exception.ConsoleNotAvailable, self.driver._get_node_console_with_reset, self.instance) - self.assertGreater(mock_node.get_console.call_count, 1) - self.assertEqual(2, mock_node.set_console_mode.call_count) + self.assertGreater(self.mock_conn.get_node_console.call_count, 1) + self.assertEqual(2, self.mock_conn.set_node_console_mode.call_count) self.assertTrue(mock_log.error.called) @mock.patch.object(ironic_driver, '_CONSOLE_STATE_CHECKING_INTERVAL', 0.05) @mock.patch.object(loopingcall, 'BackOffLoopingCall') @mock.patch.object(ironic_driver, 'LOG', autospec=True) def test__get_node_console_with_reset_wait_timeout(self, mock_log, - mock_looping, - mock_node): + mock_looping): CONF.set_override('serial_console_state_timeout', 1, group='ironic') temp_data = {'target_mode': True} @@ -3358,8 +3364,9 @@ class IronicDriverConsoleTestCase(test.NoDBTestCase): regex = r'Timeout while waiting for console mode to be set .*' self.assertThat(msg, matchers.MatchesRegex(regex)) - mock_node.get_console.side_effect = _fake_get_console - mock_node.set_console_mode.side_effect = _fake_set_console_mode + self.mock_conn.get_node_console.side_effect = _fake_get_console + self.mock_conn.set_node_console_mode.side_effect = \ + _fake_set_console_mode mock_log.error.side_effect = _fake_log_error mock_timer = mock_looping.return_value @@ -3370,14 +3377,14 @@ class IronicDriverConsoleTestCase(test.NoDBTestCase): self.driver._get_node_console_with_reset, self.instance) - self.assertEqual(mock_node.get_console.call_count, 1) - self.assertEqual(2, mock_node.set_console_mode.call_count) + self.assertEqual(self.mock_conn.get_node_console.call_count, 1) + self.assertEqual(2, self.mock_conn.set_node_console_mode.call_count) self.assertTrue(mock_log.error.called) mock_timer.start.assert_called_with(starting_interval=0.05, timeout=1, jitter=0.5) - def test_get_serial_console_socat(self, mock_node): + def test_get_serial_console_socat(self): temp_data = {'target_mode': True} def _fake_get_console(node_uuid): @@ -3386,29 +3393,30 @@ class IronicDriverConsoleTestCase(test.NoDBTestCase): def _fake_set_console_mode(node_uuid, mode): temp_data['target_mode'] = mode - mock_node.get_console.side_effect = _fake_get_console - mock_node.set_console_mode.side_effect = _fake_set_console_mode + self.mock_conn.get_node_console.side_effect = _fake_get_console + self.mock_conn.set_node_console_mode.side_effect = \ + _fake_set_console_mode result = self.driver.get_serial_console(self.ctx, self.instance) - self.assertGreater(mock_node.get_console.call_count, 1) - self.assertEqual(2, mock_node.set_console_mode.call_count) + self.assertGreater(self.mock_conn.get_node_console.call_count, 1) + self.assertEqual(2, self.mock_conn.set_node_console_mode.call_count) self.assertIsInstance(result, console_type.ConsoleSerial) self.assertEqual('127.0.0.1', result.host) self.assertEqual(10000, result.port) - def test_get_serial_console_socat_disabled(self, mock_node): - mock_node.get_console.return_value = \ + def test_get_serial_console_socat_disabled(self): + self.mock_conn.get_node_console.return_value = \ self._create_console_data(enabled=False) self.assertRaises(exception.ConsoleTypeUnavailable, self.driver.get_serial_console, self.ctx, self.instance) - mock_node.get_console.assert_called_once_with(self.node.uuid) - mock_node.set_console_mode.assert_not_called() + self.mock_conn.get_node_console.assert_called_once_with(self.node.uuid) + self.mock_conn.set_node_console_mode.assert_not_called() @mock.patch.object(ironic_driver, 'LOG', autospec=True) - def test_get_serial_console_socat_invalid_url(self, mock_log, mock_node): + def test_get_serial_console_socat_invalid_url(self, mock_log): temp_data = {'target_mode': True} def _fake_get_console(node_uuid): @@ -3422,20 +3430,21 @@ class IronicDriverConsoleTestCase(test.NoDBTestCase): regex = r'Invalid Socat console URL .*' self.assertThat(msg, matchers.MatchesRegex(regex)) - mock_node.get_console.side_effect = _fake_get_console - mock_node.set_console_mode.side_effect = _fake_set_console_mode + self.mock_conn.get_node_console.side_effect = _fake_get_console + self.mock_conn.set_node_console_mode.side_effect = \ + _fake_set_console_mode mock_log.error.side_effect = _fake_log_error self.assertRaises(exception.ConsoleTypeUnavailable, self.driver.get_serial_console, self.ctx, self.instance) - self.assertGreater(mock_node.get_console.call_count, 1) - self.assertEqual(2, mock_node.set_console_mode.call_count) + self.assertGreater(self.mock_conn.get_node_console.call_count, 1) + self.assertEqual(2, self.mock_conn.set_node_console_mode.call_count) self.assertTrue(mock_log.error.called) @mock.patch.object(ironic_driver, 'LOG', autospec=True) - def test_get_serial_console_socat_invalid_url_2(self, mock_log, mock_node): + def test_get_serial_console_socat_invalid_url_2(self, mock_log): temp_data = {'target_mode': True} def _fake_get_console(node_uuid): @@ -3449,21 +3458,21 @@ class IronicDriverConsoleTestCase(test.NoDBTestCase): regex = r'Invalid Socat console URL .*' self.assertThat(msg, matchers.MatchesRegex(regex)) - mock_node.get_console.side_effect = _fake_get_console - mock_node.set_console_mode.side_effect = _fake_set_console_mode + self.mock_conn.get_node_console.side_effect = _fake_get_console + self.mock_conn.set_node_console_mode.side_effect = \ + _fake_set_console_mode mock_log.error.side_effect = _fake_log_error self.assertRaises(exception.ConsoleTypeUnavailable, self.driver.get_serial_console, self.ctx, self.instance) - self.assertGreater(mock_node.get_console.call_count, 1) - self.assertEqual(2, mock_node.set_console_mode.call_count) + self.assertGreater(self.mock_conn.get_node_console.call_count, 1) + self.assertEqual(2, self.mock_conn.set_node_console_mode.call_count) self.assertTrue(mock_log.error.called) @mock.patch.object(ironic_driver, 'LOG', autospec=True) - def test_get_serial_console_socat_unsupported_scheme(self, mock_log, - mock_node): + def test_get_serial_console_socat_unsupported_scheme(self, mock_log): temp_data = {'target_mode': True} def _fake_get_console(node_uuid): @@ -3477,19 +3486,20 @@ class IronicDriverConsoleTestCase(test.NoDBTestCase): regex = r'Socat serial console only supports \"tcp\".*' self.assertThat(msg, matchers.MatchesRegex(regex)) - mock_node.get_console.side_effect = _fake_get_console - mock_node.set_console_mode.side_effect = _fake_set_console_mode + self.mock_conn.get_node_console.side_effect = _fake_get_console + self.mock_conn.set_node_console_mode.side_effect = \ + _fake_set_console_mode mock_log.warning.side_effect = _fake_log_warning self.assertRaises(exception.ConsoleTypeUnavailable, self.driver.get_serial_console, self.ctx, self.instance) - self.assertGreater(mock_node.get_console.call_count, 1) - self.assertEqual(2, mock_node.set_console_mode.call_count) + self.assertGreater(self.mock_conn.get_node_console.call_count, 1) + self.assertEqual(2, self.mock_conn.set_node_console_mode.call_count) self.assertTrue(mock_log.warning.called) - def test_get_serial_console_socat_tcp6(self, mock_node): + def test_get_serial_console_socat_tcp6(self): temp_data = {'target_mode': True} def _fake_get_console(node_uuid): @@ -3499,18 +3509,19 @@ class IronicDriverConsoleTestCase(test.NoDBTestCase): def _fake_set_console_mode(node_uuid, mode): temp_data['target_mode'] = mode - mock_node.get_console.side_effect = _fake_get_console - mock_node.set_console_mode.side_effect = _fake_set_console_mode + self.mock_conn.get_node_console.side_effect = _fake_get_console + self.mock_conn.set_node_console_mode.side_effect = \ + _fake_set_console_mode result = self.driver.get_serial_console(self.ctx, self.instance) - self.assertGreater(mock_node.get_console.call_count, 1) - self.assertEqual(2, mock_node.set_console_mode.call_count) + self.assertGreater(self.mock_conn.get_node_console.call_count, 1) + self.assertEqual(2, self.mock_conn.set_node_console_mode.call_count) self.assertIsInstance(result, console_type.ConsoleSerial) self.assertEqual('::1', result.host) self.assertEqual(10000, result.port) - def test_get_serial_console_shellinabox(self, mock_node): + def test_get_serial_console_shellinabox(self): temp_data = {'target_mode': True} def _fake_get_console(node_uuid): @@ -3520,12 +3531,13 @@ class IronicDriverConsoleTestCase(test.NoDBTestCase): def _fake_set_console_mode(node_uuid, mode): temp_data['target_mode'] = mode - mock_node.get_console.side_effect = _fake_get_console - mock_node.set_console_mode.side_effect = _fake_set_console_mode + self.mock_conn.get_node_console.side_effect = _fake_get_console + self.mock_conn.set_node_console_mode.side_effect = \ + _fake_set_console_mode self.assertRaises(exception.ConsoleTypeUnavailable, self.driver.get_serial_console, self.ctx, self.instance) - self.assertGreater(mock_node.get_console.call_count, 1) - self.assertEqual(2, mock_node.set_console_mode.call_count) + self.assertGreater(self.mock_conn.get_node_console.call_count, 1) + self.assertEqual(2, self.mock_conn.set_node_console_mode.call_count) diff --git a/nova/tests/unit/virt/ironic/utils.py b/nova/tests/unit/virt/ironic/utils.py index 0c46d00043c8..3560a80da459 100644 --- a/nova/tests/unit/virt/ironic/utils.py +++ b/nova/tests/unit/virt/ironic/utils.py @@ -13,18 +13,23 @@ # License for the specific language governing permissions and limitations # under the License. +from openstack.baremetal.v1 import node as _node + from nova import objects from nova.virt.ironic import client_wrapper from nova.virt.ironic import ironic_states def get_test_validation(**kw): - return type('interfaces', (object,), - {'power': kw.get('power', {'result': True}), - 'deploy': kw.get('deploy', {'result': True}), - 'console': kw.get('console', True), - 'rescue': kw.get('rescue', True), - 'storage': kw.get('storage', {'result': True})})() + result = { + 'power': _node.ValidationResult(result=True, reason=None), + 'deploy': _node.ValidationResult(result=True, reason=None), + 'console': _node.ValidationResult(result=True, reason=None), + 'rescue': _node.ValidationResult(result=True, reason=None), + 'storage': _node.ValidationResult(result=True, reason=None), + } + result.update(kw) + return result def get_test_node(fields=None, **kw): diff --git a/nova/virt/ironic/driver.py b/nova/virt/ironic/driver.py index a31eba8e87bd..ab400c7b9474 100644 --- a/nova/virt/ironic/driver.py +++ b/nova/virt/ironic/driver.py @@ -26,13 +26,12 @@ import tempfile import time from urllib import parse as urlparse -import microversion_parse from openstack import exceptions as sdk_exc +from openstack import utils as sdk_utils from oslo_log import log as logging from oslo_serialization import jsonutils from oslo_service import loopingcall from oslo_utils import excutils -from oslo_utils import importutils from tooz import hashring as hash_ring from nova.api.metadata import base as instance_metadata @@ -59,8 +58,6 @@ from nova.virt.ironic import patcher from nova.virt import netutils -ironic = None - LOG = logging.getLogger(__name__) @@ -185,16 +182,12 @@ class IronicDriver(virt_driver.ComputeDriver): rebalances_nodes = True def __init__(self, virtapi, read_only=False): - super(IronicDriver, self).__init__(virtapi) - global ironic - if ironic is None: - ironic = importutils.import_module('ironicclient') + super().__init__(virtapi) self.node_cache = {} self.node_cache_time = 0 self.servicegroup_api = servicegroup.API() - self.ironicclient = client_wrapper.IronicClientWrapper() self._ironic_connection = None @property @@ -388,9 +381,11 @@ class IronicDriver(virt_driver.ComputeDriver): LOG.debug('Preparing to spawn instance %s.', instance.uuid) node_uuid = instance.get('node') if not node_uuid: - raise ironic.exc.BadRequest( - _("Ironic node uuid not supplied to " - "driver for instance %s.") % instance.uuid) + msg = _( + "Ironic node uuid not supplied to " + "driver for instance %s." + ) % instance.uuid + raise exception.NovaException(msg) node = self._get_node(node_uuid) # Its possible this node has just moved from deleting @@ -711,7 +706,7 @@ class IronicDriver(virt_driver.ComputeDriver): # filter by conductor_group. If it cannot, limiting to # peer_list could end up with a node being managed by multiple # compute services. - self._can_send_version(min_version='1.46') + self._can_send_version('1.46') peer_list = set(CONF.ironic.peer_list) # these configs are mutable; need to check at runtime and init. @@ -771,7 +766,7 @@ class IronicDriver(virt_driver.ComputeDriver): conductor_group = CONF.ironic.conductor_group if conductor_group is not None: try: - self._can_send_version(min_version='1.46') + self._can_send_version('1.46') nodes = _get_node_list(conductor_group=conductor_group) LOG.debug('Limiting manageable ironic nodes to conductor ' 'group %s', conductor_group) @@ -1153,9 +1148,10 @@ class IronicDriver(virt_driver.ComputeDriver): # is a significant issue. It may mean we've been passed the wrong data. node_uuid = instance.get('node') if not node_uuid: - raise ironic.exc.BadRequest( + raise exception.NovaException( _("Ironic node uuid not supplied to " - "driver for instance %s.") % instance.uuid) + "driver for instance %s.") % instance.uuid + ) node = self._get_node(node_uuid) flavor = instance.flavor @@ -1181,20 +1177,27 @@ class IronicDriver(virt_driver.ComputeDriver): instance.save() # validate we are ready to do the deploy - validate_chk = self.ironicclient.call("node.validate", node_uuid) - if (not validate_chk.deploy.get('result') or - not validate_chk.power.get('result') or - not validate_chk.storage.get('result')): + # NOTE(stephenfin): we don't pass required since we have to do our own + # validation + validate_chk = self.ironic_connection.validate_node( + node_uuid, + required=None, + ) + if ( + not validate_chk['deploy'].result or + not validate_chk['power'].result or + not validate_chk['storage'].result + ): # something is wrong. undo what we have done self._cleanup_deploy(node, instance, network_info) raise exception.ValidationError(_( - "Ironic node: %(id)s failed to validate." - " (deploy: %(deploy)s, power: %(power)s," - " storage: %(storage)s)") + "Ironic node: %(id)s failed to validate. " + "(deploy: %(deploy)s, power: %(power)s, " + "storage: %(storage)s)") % {'id': node.uuid, - 'deploy': validate_chk.deploy, - 'power': validate_chk.power, - 'storage': validate_chk.storage}) + 'deploy': validate_chk['deploy'], + 'power': validate_chk['power'], + 'storage': validate_chk['storage']}) # Config drive configdrive_value = None @@ -1526,7 +1529,7 @@ class IronicDriver(virt_driver.ComputeDriver): LOG.debug('Trigger crash dump called for instance', instance=instance) node = self._validate_instance_and_node(instance) - self.ironicclient.call("node.inject_nmi", node.uuid) + self.ironic_connection.inject_nmi_to_node(node.uuid) LOG.info('Successfully triggered crash dump into Ironic node %s', node.uuid, instance=instance) @@ -1806,12 +1809,10 @@ class IronicDriver(virt_driver.ComputeDriver): node_uuid = node.uuid def _get_console(): - """Request ironicclient to acquire node console.""" + """Request to acquire node console.""" try: - return self.ironicclient.call('node.get_console', node_uuid) - except (exception.NovaException, # Retry failed - ironic.exc.InternalServerError, # Validations - ironic.exc.BadRequest) as e: # Maintenance + return self.ironic_connection.get_node_console(node_uuid) + except sdk_exc.SDKException as e: LOG.error('Failed to acquire console information for ' 'instance %(inst)s: %(reason)s', {'inst': instance.uuid, 'reason': e}) @@ -1829,13 +1830,10 @@ class IronicDriver(virt_driver.ComputeDriver): return False def _enable_console(mode): - """Request ironicclient to enable/disable node console.""" + """Request to enable/disable node console.""" try: - self.ironicclient.call('node.set_console_mode', node_uuid, - mode) - except (exception.NovaException, # Retry failed - ironic.exc.InternalServerError, # Validations - ironic.exc.BadRequest) as e: # Maintenance + self.ironic_connection.set_node_console_mode(node_uuid, mode) + except sdk_exc.SDKException as e: LOG.error('Failed to set console mode to "%(mode)s" ' 'for instance %(inst)s: %(reason)s', {'mode': mode, @@ -2111,30 +2109,13 @@ class IronicDriver(virt_driver.ComputeDriver): return None - def _can_send_version(self, min_version=None, max_version=None): + def _can_send_version(self, version=None): """Validate if the supplied version is available in the API.""" - # NOTE(TheJulia): This will effectively just be a pass if no - # version negotiation has occured, since there is no way for - # us to know without explicitly otherwise requesting that - # back-end negotiation occurs. This is a capability that is - # present in python-ironicclient, however it may not be needed - # in this case. - if self.ironicclient.is_api_version_negotiated: - current_api_version = self.ironicclient.current_api_version - if (min_version and - microversion_parse.parse_version_string( - current_api_version) < - microversion_parse.parse_version_string( - min_version)): - raise exception.IronicAPIVersionNotAvailable( - version=min_version) - if (max_version and - microversion_parse.parse_version_string( - current_api_version) > - microversion_parse.parse_version_string( - max_version)): - raise exception.IronicAPIVersionNotAvailable( - version=max_version) + if not sdk_utils.supports_microversion( + self.ironic_connection, + version, + ): + raise exception.IronicAPIVersionNotAvailable(version=version) def rescue(self, context, instance, network_info, image_meta, rescue_password, block_device_info):