Switch Inspector interface to pass keystoneauth sessions

This enables fetching the service URL from keystone catalog.

Change-Id: If862481533cf121bc6e829f0f39893134578ec30

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

etc/ironic/ironic.conf.sample

@@ -1396,8 +1396,7 @@
 #project_name = <None>
 
 # ironic-inspector HTTP endpoint. If this is not set, the
-# ironic-inspector client default (http://127.0.0.1:5050) will
+# service catalog will be used. (string value)
-# be used. (string value)
 #service_url = <None>
 
 # period (in seconds) to check status of nodes on inspection

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 '
+                      'the service catalog will be used.')),
-                      '(http://127.0.0.1:5050) will be used.')),
     cfg.IntOpt('status_check_period', default=60,
                help=_('period (in seconds) to check status of nodes '
                       'on inspection')),

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

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(
+        mock_init.assert_called_once_with(
-            self.node.uuid,
+            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(
+        mock_init.assert_called_once_with(
-            self.node.uuid,
+            session=mock.sentinel.session,
             api_version=self.api_version,
-            auth_token=self.task.context.auth_token,
+            inspector_url='meow')
-            base_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(
+        mock_introspect.assert_called_once_with(self.node.uuid)
-            self.node.uuid,
-            api_version=self.api_version,
-            auth_token=self.task.context.auth_token)
         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,
+        mock_init.assert_called_once_with(
-                                         api_version=self.api_version,
+            session=mock.sentinel.session,
-                                         auth_token='the token')
+            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,
+        mock_init.assert_called_once_with(
-                                         api_version=self.api_version,
+            session=mock.sentinel.session,
-                                         auth_token='the token')
+            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,
+        mock_init.assert_called_once_with(
-                                         api_version=self.api_version,
+            session=mock.sentinel.session,
-                                         auth_token='the token')
+            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,
+        mock_init.assert_called_once_with(
-                                         api_version=self.api_version,
+            session=mock.sentinel.session,
-                                         auth_token='the token')
+            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,
+        mock_init.assert_called_once_with(
-                                         api_version=self.api_version,
+            session=mock.sentinel.session,
-                                         auth_token='the token',
+            api_version=self.api_version,
-                                         base_url='meow')
+            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(
+        mock_init.assert_called_once_with(
-            self.node.uuid,
+            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)

ironic/tests/unit/drivers/third_party_driver_mock_specs.py

@@ -40,10 +40,21 @@ IBOOT_SPEC = (
 
 # ironic_inspector
 IRONIC_INSPECTOR_CLIENT_SPEC = (
-    'introspect',
+    'ClientV1',
-    'get_status',
 )
 
+
+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',

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(

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).