diff --git a/driver-requirements.txt b/driver-requirements.txt index 2d0af3fbf3..939cc13160 100644 --- a/driver-requirements.txt +++ b/driver-requirements.txt @@ -7,7 +7,7 @@ proliantutils>=2.1.10 pyghmi>=0.8.0 pysnmp -python-ironic-inspector-client +python-ironic-inspector-client>=1.5.0 python-oneviewclient<3.0.0,>=2.0.2 python-scciclient>=0.3.0 python-seamicroclient>=0.4.0 diff --git a/etc/ironic/ironic.conf.sample b/etc/ironic/ironic.conf.sample index 47296cc986..0b7de99713 100644 --- a/etc/ironic/ironic.conf.sample +++ b/etc/ironic/ironic.conf.sample @@ -1396,8 +1396,7 @@ #project_name = # ironic-inspector HTTP endpoint. If this is not set, the -# ironic-inspector client default (http://127.0.0.1:5050) will -# be used. (string value) +# service catalog will be used. (string value) #service_url = # period (in seconds) to check status of nodes on inspection diff --git a/ironic/conf/inspector.py b/ironic/conf/inspector.py index 50613e9f03..3627511eeb 100644 --- a/ironic/conf/inspector.py +++ b/ironic/conf/inspector.py @@ -22,8 +22,7 @@ opts = [ help=_('whether to enable inspection using ironic-inspector')), cfg.StrOpt('service_url', help=_('ironic-inspector HTTP endpoint. If this is not set, ' - 'the ironic-inspector client default ' - '(http://127.0.0.1:5050) will be used.')), + 'the service catalog will be used.')), cfg.IntOpt('status_check_period', default=60, help=_('period (in seconds) to check status of nodes ' 'on inspection')), diff --git a/ironic/drivers/modules/inspector.py b/ironic/drivers/modules/inspector.py index 59c5a31575..d194174b03 100644 --- a/ironic/drivers/modules/inspector.py +++ b/ironic/drivers/modules/inspector.py @@ -44,12 +44,22 @@ _INSPECTOR_SESSION = None def _get_inspector_session(): + if CONF.auth_strategy != 'keystone': + return + global _INSPECTOR_SESSION if not _INSPECTOR_SESSION: _INSPECTOR_SESSION = keystone.get_session('inspector') return _INSPECTOR_SESSION +def _get_client(): + """Helper to get inspector client instance.""" + return client.ClientV1(api_version=INSPECTOR_API_VERSION, + inspector_url=CONF.inspector.service_url, + session=_get_inspector_session()) + + class Inspector(base.InspectInterface): """In-band inspection via ironic-inspector project.""" @@ -131,20 +141,10 @@ class Inspector(base.InspectInterface): continue -def _call_inspector(func, uuid, context): - """Wrapper around calls to inspector.""" - # NOTE(dtantsur): due to bug #1428652 None is not accepted for base_url. - kwargs = {'api_version': INSPECTOR_API_VERSION} - if CONF.inspector.service_url: - kwargs['base_url'] = CONF.inspector.service_url - return func(uuid, auth_token=context.auth_token, **kwargs) - - def _start_inspection(node_uuid, context): """Call to inspector to start inspection.""" - context.ensure_thread_contain_context() try: - _call_inspector(client.introspect, node_uuid, context) + _get_client().introspect(node_uuid) except Exception as exc: LOG.exception(_LE('Exception during contacting ironic-inspector ' 'for inspection of node %(node)s: %(err)s'), @@ -172,13 +172,8 @@ def _check_status(task): LOG.debug('Calling to inspector to check status of node %s', task.node.uuid) - # NOTE(dtantsur): periodic tasks do not have proper tokens in context - if CONF.auth_strategy == 'keystone': - session = _get_inspector_session() - task.context.auth_token = keystone.get_admin_auth_token(session) - try: - status = _call_inspector(client.get_status, node.uuid, task.context) + status = _get_client().get_status(node.uuid) except Exception: # NOTE(dtantsur): get_status should not normally raise # let's assume it's a transient failure and retry later diff --git a/ironic/tests/unit/drivers/modules/test_inspector.py b/ironic/tests/unit/drivers/modules/test_inspector.py index 7124cf4bbc..2b489efe18 100644 --- a/ironic/tests/unit/drivers/modules/test_inspector.py +++ b/ironic/tests/unit/drivers/modules/test_inspector.py @@ -93,151 +93,129 @@ class CommonFunctionsTestCase(BaseTestCase): self.assertTrue(warn_mock.called) +@mock.patch('ironic.common.keystone.get_session', + lambda _n: mock.sentinel.session) @mock.patch.object(eventlet, 'spawn_n', lambda f, *a, **kw: f(*a, **kw)) -@mock.patch.object(client, 'introspect') +@mock.patch.object(client.ClientV1, 'introspect') +@mock.patch.object(client.ClientV1, '__init__', return_value=None) class InspectHardwareTestCase(BaseTestCase): - def test_ok(self, mock_introspect): + def test_ok(self, mock_init, mock_introspect): self.assertEqual(states.INSPECTING, self.driver.inspect.inspect_hardware(self.task)) - mock_introspect.assert_called_once_with( - self.node.uuid, + mock_init.assert_called_once_with( + session=mock.sentinel.session, api_version=self.api_version, - auth_token=self.task.context.auth_token) + inspector_url=None) + mock_introspect.assert_called_once_with(self.node.uuid) - def test_url(self, mock_introspect): + def test_url(self, mock_init, mock_introspect): self.config(service_url='meow', group='inspector') self.assertEqual(states.INSPECTING, self.driver.inspect.inspect_hardware(self.task)) - mock_introspect.assert_called_once_with( - self.node.uuid, + mock_init.assert_called_once_with( + session=mock.sentinel.session, api_version=self.api_version, - auth_token=self.task.context.auth_token, - base_url='meow') + inspector_url='meow') + mock_introspect.assert_called_once_with(self.node.uuid) @mock.patch.object(task_manager, 'acquire', autospec=True) - def test_error(self, mock_acquire, mock_introspect): + def test_error(self, mock_acquire, mock_init, mock_introspect): mock_introspect.side_effect = RuntimeError('boom') self.driver.inspect.inspect_hardware(self.task) - mock_introspect.assert_called_once_with( - self.node.uuid, - api_version=self.api_version, - auth_token=self.task.context.auth_token) + mock_introspect.assert_called_once_with(self.node.uuid) task = mock_acquire.return_value.__enter__.return_value self.assertIn('boom', task.node.last_error) task.process_event.assert_called_once_with('fail') + def test_is_standalone(self, mock_init, mock_introspect): + self.config(auth_strategy='noauth') + self.assertEqual(states.INSPECTING, + self.driver.inspect.inspect_hardware(self.task)) + mock_init.assert_called_once_with( + session=None, + api_version=self.api_version, + inspector_url=None) + mock_introspect.assert_called_once_with(self.node.uuid) -@mock.patch.object(client, 'get_status') + +@mock.patch('ironic.common.keystone.get_session', + lambda _n: mock.sentinel.session) +@mock.patch.object(client.ClientV1, 'get_status') +@mock.patch.object(client.ClientV1, '__init__', return_value=None) class CheckStatusTestCase(BaseTestCase): def setUp(self): super(CheckStatusTestCase, self).setUp() self.node.provision_state = states.INSPECTING - mock_session = mock.Mock() - mock_session.get_token.return_value = 'the token' - sess_patch = mock.patch.object(inspector, '_get_inspector_session', - return_value=mock_session) - sess_patch.start() - self.addCleanup(sess_patch.stop) - def test_not_inspecting(self, mock_get): + def test_not_inspecting(self, mock_init, mock_get): self.node.provision_state = states.MANAGEABLE inspector._check_status(self.task) self.assertFalse(mock_get.called) - def test_not_inspector(self, mock_get): + def test_not_inspector(self, mock_init, mock_get): self.task.driver.inspect = object() inspector._check_status(self.task) self.assertFalse(mock_get.called) - def test_not_finished(self, mock_get): + def test_not_finished(self, mock_init, mock_get): mock_get.return_value = {} inspector._check_status(self.task) - mock_get.assert_called_once_with(self.node.uuid, - api_version=self.api_version, - auth_token='the token') + mock_init.assert_called_once_with( + session=mock.sentinel.session, + api_version=self.api_version, + inspector_url=None) + mock_get.assert_called_once_with(self.node.uuid) self.assertFalse(self.task.process_event.called) - def test_exception_ignored(self, mock_get): + def test_exception_ignored(self, mock_init, mock_get): mock_get.side_effect = RuntimeError('boom') inspector._check_status(self.task) - mock_get.assert_called_once_with(self.node.uuid, - api_version=self.api_version, - auth_token='the token') + mock_init.assert_called_once_with( + session=mock.sentinel.session, + api_version=self.api_version, + inspector_url=None) + mock_get.assert_called_once_with(self.node.uuid) self.assertFalse(self.task.process_event.called) - def test_status_ok(self, mock_get): + def test_status_ok(self, mock_init, mock_get): mock_get.return_value = {'finished': True} inspector._check_status(self.task) - mock_get.assert_called_once_with(self.node.uuid, - api_version=self.api_version, - auth_token='the token') + mock_init.assert_called_once_with( + session=mock.sentinel.session, + api_version=self.api_version, + inspector_url=None) + mock_get.assert_called_once_with(self.node.uuid) self.task.process_event.assert_called_once_with('done') - def test_status_error(self, mock_get): + def test_status_error(self, mock_init, mock_get): mock_get.return_value = {'error': 'boom'} inspector._check_status(self.task) - mock_get.assert_called_once_with(self.node.uuid, - api_version=self.api_version, - auth_token='the token') + mock_init.assert_called_once_with( + session=mock.sentinel.session, + api_version=self.api_version, + inspector_url=None) + mock_get.assert_called_once_with(self.node.uuid) self.task.process_event.assert_called_once_with('fail') self.assertIn('boom', self.node.last_error) - def test_service_url(self, mock_get): + def test_service_url(self, mock_init, mock_get): self.config(service_url='meow', group='inspector') mock_get.return_value = {'finished': True} inspector._check_status(self.task) - mock_get.assert_called_once_with(self.node.uuid, - api_version=self.api_version, - auth_token='the token', - base_url='meow') + mock_init.assert_called_once_with( + session=mock.sentinel.session, + api_version=self.api_version, + inspector_url='meow') + mock_get.assert_called_once_with(self.node.uuid) self.task.process_event.assert_called_once_with('done') - def test_is_standalone(self, mock_get): + def test_is_standalone(self, mock_init, mock_get): self.config(auth_strategy='noauth') mock_get.return_value = {'finished': True} inspector._check_status(self.task) - mock_get.assert_called_once_with( - self.node.uuid, + mock_init.assert_called_once_with( + session=None, api_version=self.api_version, - auth_token=self.task.context.auth_token) + inspector_url=None) + mock_get.assert_called_once_with(self.node.uuid) self.task.process_event.assert_called_once_with('done') - - def test_not_standalone(self, mock_get): - self.config(auth_strategy='keystone') - mock_get.return_value = {'finished': True} - inspector._check_status(self.task) - mock_get.assert_called_once_with(self.node.uuid, - api_version=self.api_version, - auth_token='the token') - self.task.process_event.assert_called_once_with('done') - - -@mock.patch.object(eventlet.greenthread, 'spawn_n', - lambda f, *a, **kw: f(*a, **kw)) -@mock.patch.object(task_manager, 'acquire', autospec=True) -@mock.patch.object(inspector, '_check_status', autospec=True) -class PeriodicTaskTestCase(BaseTestCase): - def test_ok(self, mock_check, mock_acquire): - mgr = mock.MagicMock(spec=['iter_nodes']) - mgr.iter_nodes.return_value = [('1', 'd1'), ('2', 'd2')] - tasks = [mock.sentinel.task1, mock.sentinel.task2] - mock_acquire.side_effect = ( - mock.MagicMock(__enter__=mock.MagicMock(return_value=task)) - for task in tasks - ) - inspector.Inspector()._periodic_check_result( - mgr, mock.sentinel.context) - mock_check.assert_any_call(tasks[0]) - mock_check.assert_any_call(tasks[1]) - self.assertEqual(2, mock_acquire.call_count) - - def test_node_locked(self, mock_check, mock_acquire): - iter_nodes_ret = [('1', 'd1'), ('2', 'd2')] - mock_acquire.side_effect = ([exception.NodeLocked("boom")] * - len(iter_nodes_ret)) - mgr = mock.MagicMock(spec=['iter_nodes']) - mgr.iter_nodes.return_value = iter_nodes_ret - inspector.Inspector()._periodic_check_result( - mgr, mock.sentinel.context) - self.assertFalse(mock_check.called) - self.assertEqual(2, mock_acquire.call_count) diff --git a/ironic/tests/unit/drivers/third_party_driver_mock_specs.py b/ironic/tests/unit/drivers/third_party_driver_mock_specs.py index d84f9e895a..62500600fc 100644 --- a/ironic/tests/unit/drivers/third_party_driver_mock_specs.py +++ b/ironic/tests/unit/drivers/third_party_driver_mock_specs.py @@ -40,10 +40,21 @@ IBOOT_SPEC = ( # ironic_inspector IRONIC_INSPECTOR_CLIENT_SPEC = ( - 'introspect', - 'get_status', + 'ClientV1', ) + +class InspectorClientV1Specs(object): + def __init__(self, session, inspector_url, api_version): + pass + + def introspect(self, uuid): + pass + + def get_status(self, uuid): + pass + + # proliantutils PROLIANTUTILS_SPEC = ( 'exception', diff --git a/ironic/tests/unit/drivers/third_party_driver_mocks.py b/ironic/tests/unit/drivers/third_party_driver_mocks.py index 8fb57c971b..7490e39133 100644 --- a/ironic/tests/unit/drivers/third_party_driver_mocks.py +++ b/ironic/tests/unit/drivers/third_party_driver_mocks.py @@ -261,6 +261,7 @@ ironic_inspector_client = importutils.try_import('ironic_inspector_client') if not ironic_inspector_client: ironic_inspector_client = mock.MagicMock( spec_set=mock_specs.IRONIC_INSPECTOR_CLIENT_SPEC) + ironic_inspector_client.ClientV1 = mock_specs.InspectorClientV1Specs sys.modules['ironic_inspector_client'] = ironic_inspector_client if 'ironic.drivers.modules.inspector' in sys.modules: six.moves.reload_module( diff --git a/releasenotes/notes/inspector-session-179f83cbb0dc169b.yaml b/releasenotes/notes/inspector-session-179f83cbb0dc169b.yaml new file mode 100644 index 0000000000..546e1be3ec --- /dev/null +++ b/releasenotes/notes/inspector-session-179f83cbb0dc169b.yaml @@ -0,0 +1,8 @@ +--- +fixes: + - Ironic Inspector inspection interface will not fetch the service endpoint + for the service catalog, if "service_url" is not provided and keystone + support is enabled. +upgrade: + - Minimum required version of python-ironic-inspector-client was bumped + to 1.5.0 (released as part of the Mitaka cycle).