Use adapters for cinderclient

deprecates the `[cinder]url` option in favor of
[cinder]endpoint_override.

Change-Id: Idd02e8cf0892965a3138479e49ec40cfeda7c96d
Partial-Bug: #1699547

devstack/lib/ironic

@@ -1113,7 +1113,7 @@ function configure_ironic_conductor {
     # TODO(pas-ha) this block is for transition period only,
     # after all clients are moved to use keystoneauth adapters,
     # it will be deleted
-    local sections_with_adapter="service_catalog glance"
+    local sections_with_adapter="service_catalog glance cinder"
     for conf_section in $sections_with_adapter; do
         configure_adapter_for $conf_section
     done

etc/ironic/ironic.conf.sample

@@ -948,12 +948,28 @@
 # Domain name to scope to (string value)
 #domain_name = <None>
 
+# Always use this endpoint URL for requests for this client.
+# (string value)
+#endpoint_override = <None>
+
 # Verify HTTPS connections. (boolean value)
 #insecure = false
 
 # PEM encoded client certificate key file (string value)
 #keyfile = <None>
 
+# The maximum major version of a given API, intended to be
+# used as the upper bound of a range with min_version.
+# Mutually exclusive with version. (string value)
+#max_version = <None>
+
+# The minimum major version of a given API, intended to be
+# used as the lower bound of a range with max_version.
+# Mutually exclusive with version. If min_version is given
+# with no max_version it is as if max version is "latest".
+# (string value)
+#min_version = <None>
+
 # User's password (string value)
 #password = <None>
 
@@ -971,10 +987,22 @@
 # Deprecated group/name - [cinder]/tenant_name
 #project_name = <None>
 
+# The default region_name for endpoint URL discovery. (string
+# value)
+#region_name = <None>
+
 # Client retries in the case of a failed request connection.
 # (integer value)
 #retries = 3
 
+# The default service_name for endpoint URL discovery. (string
+# value)
+#service_name = <None>
+
+# The default service_type for endpoint URL discovery. (string
+# value)
+#service_type = volumev3
+
 # Tenant ID (string value)
 #tenant_id = <None>
 
@@ -987,8 +1015,12 @@
 # Trust ID (string value)
 #trust_id = <None>
 
-# URL for connecting to cinder. If set, the value must start
+# DEPRECATED: URL for connecting to cinder. If set, the value
-# with either http:// or https://. (uri value)
+# must start with either http:// or https://. (uri value)
+# This option is deprecated for removal.
+# Its value may be silently ignored in the future.
+# Reason: Use [cinder]/endpoint_override option to set a
+# specific cinder API url to connect to.
 #url = <None>
 
 # User's domain id (string value)
@@ -1004,6 +1036,15 @@
 # Deprecated group/name - [cinder]/user_name
 #username = <None>
 
+# List of interfaces, in order of preference, for endpoint
+# URL. (list value)
+#valid_interfaces = internal,public
+
+# Minimum Major API version within a given Major API version
+# for endpoint URL discovery. Mutually exclusive with
+# min_version and max_version (string value)
+#version = <None>
+
 
 [cisco_ucs]
 

ironic/common/cinder.py

@@ -35,32 +35,45 @@ _CINDER_SESSION = None
 def _get_cinder_session():
     global _CINDER_SESSION
     if not _CINDER_SESSION:
-        auth = keystone.get_auth('cinder')
+        _CINDER_SESSION = keystone.get_session('cinder')
-        _CINDER_SESSION = keystone.get_session('cinder', auth=auth)
     return _CINDER_SESSION
 
 
-def get_client():
+def get_client(context):
     """Get a cinder client connection.
 
+    :param context: request context,
+                    instance of ironic.common.context.RequestContext
     :returns: A cinder client.
     """
-    params = {
+    service_auth = keystone.get_auth('cinder')
-        'connect_retries': CONF.cinder.retries
+    session = _get_cinder_session()
-    }
-    # TODO(jtaryma): Add support for noauth
-    # NOTE(TheJulia): If a URL is provided for cinder, we will pass
-    # along the URL to python-cinderclient. Otherwise the library
-    # handles keystone url autodetection.
-    if CONF.cinder.url:
-        params['endpoint_override'] = CONF.cinder.url
 
-    if CONF.keystone.region_name:
+    # TODO(pas-ha) remove in Rocky
-        params['region_name'] = CONF.keystone.region_name
+    adapter_opts = {}
+    # NOTE(pas-ha) new option must always win if set
+    if CONF.cinder.url and not CONF.cinder.endpoint_override:
+        adapter_opts['endpoint_override'] = CONF.cinder.url
+    if CONF.keystone.region_name and not CONF.cinder.region_name:
+        adapter_opts['region_name'] = CONF.keystone.region_name
 
-    params['session'] = _get_cinder_session()
+    adapter = keystone.get_adapter('cinder', session=session,
-
+                                   auth=service_auth, **adapter_opts)
-    return client.Client(**params)
+    # TODO(pas-ha) use versioned endpoint data to select required
+    # cinder api version
+    cinder_url = adapter.get_endpoint()
+    # TODO(pas-ha) investigate possibility of passing a user context here,
+    # similar to what neutron/glance-related code does
+    # NOTE(pas-ha) cinderclient has both 'connect_retries' (passed to
+    # ksa.Adapter) and 'retries' (used in its subclass of ksa.Adapter) options.
+    # The first governs retries on establishing the HTTP connection,
+    # the second governs retries on OverLimit exceptions from API.
+    # The description of [cinder]/retries fits the first,
+    # so this is what we pass.
+    return client.Client(session=session, auth=service_auth,
+                         endpoint_override=cinder_url,
+                         connect_retries=CONF.cinder.retries,
+                         global_request_id=context.global_id)
 
 
 def is_volume_available(volume):
@@ -140,7 +153,7 @@ def _init_client(task):
     """
     node = task.node
     try:
-        return get_client()
+        return get_client(task.context)
     except Exception as e:
         msg = (_('Failed to initialize cinder client for operations on node '
                  '%(uuid)s: %(err)s') % {'uuid': node.uuid, 'err': e})

ironic/conf/cinder.py

@@ -19,6 +19,10 @@ from ironic.conf import auth
 opts = [
     cfg.URIOpt('url',
                schemes=('http', 'https'),
+               deprecated_for_removal=True,
+               deprecated_reason=_('Use [cinder]/endpoint_override option '
+                                   'to set a specific cinder API URL to '
+                                   'connect to.'),
                help=_('URL for connecting to cinder. If set, the value must '
                       'start with either http:// or https://.')),
     cfg.IntOpt('retries',
@@ -37,10 +41,12 @@ opts = [
 ]
 
 
+# NOTE(pas-ha) cinder V3 which ironic requires is registered as volumev3
+# service type ATM
 def register_opts(conf):
     conf.register_opts(opts, group='cinder')
-    auth.register_auth_opts(conf, 'cinder')
+    auth.register_auth_opts(conf, 'cinder', service_type='volumev3')
 
 
 def list_opts():
-    return auth.add_auth_opts(opts)
+    return auth.add_auth_opts(opts, service_type='volumev3')

ironic/tests/unit/common/test_cinder.py

@@ -21,6 +21,7 @@ from oslo_utils import uuidutils
 from six.moves import http_client
 
 from ironic.common import cinder
+from ironic.common import context
 from ironic.common import exception
 from ironic.common import keystone
 from ironic.conductor import task_manager
@@ -39,26 +40,31 @@ class TestCinderSession(base.TestCase):
         self.config(timeout=1,
                     retries=2,
                     group='cinder')
+        cinder._CINDER_SESSION = None
 
     def test__get_cinder_session(self, mock_keystone_session, mock_auth):
         """Check establishing new session when no session exists."""
         mock_keystone_session.return_value = 'session1'
         self.assertEqual('session1', cinder._get_cinder_session())
-        mock_keystone_session.assert_called_once_with(
+        mock_keystone_session.assert_called_once_with('cinder')
-            'cinder', auth=mock_auth.return_value)
-        mock_auth.assert_called_once_with('cinder')
 
         """Check if existing session is used."""
         mock_keystone_session.reset_mock()
-        mock_auth.reset_mock()
         mock_keystone_session.return_value = 'session2'
         self.assertEqual('session1', cinder._get_cinder_session())
         self.assertFalse(mock_keystone_session.called)
         self.assertFalse(mock_auth.called)
 
 
-@mock.patch.object(cinder, '_get_cinder_session', autospec=True)
+@mock.patch('ironic.common.keystone.get_adapter', autospec=True)
-@mock.patch.object(cinderclient.Client, '__init__', autospec=True)
+@mock.patch('ironic.common.keystone.get_service_auth', autospec=True,
+            return_value=mock.sentinel.sauth)
+@mock.patch('ironic.common.keystone.get_auth', autospec=True,
+            return_value=mock.sentinel.auth)
+@mock.patch('ironic.common.keystone.get_session', autospec=True,
+            return_value=mock.sentinel.session)
+@mock.patch.object(cinderclient.Client, '__init__', autospec=True,
+                   return_value=None)
 class TestCinderClient(base.TestCase):
 
     def setUp(self):
@@ -66,42 +72,48 @@ class TestCinderClient(base.TestCase):
         self.config(timeout=1,
                     retries=2,
                     group='cinder')
+        cinder._CINDER_SESSION = None
+        self.context = context.RequestContext(global_request_id='global')
 
-    def test_get_client(self, mock_client_init, mock_session):
+    def _assert_client_call(self, init_mock, url, auth=mock.sentinel.auth):
-        mock_session_obj = mock.Mock()
+        cinder.get_client(self.context)
-        expected = {'connect_retries': 2,
+        init_mock.assert_called_once_with(
-                    'session': mock_session_obj}
+            mock.ANY,
-        mock_session.return_value = mock_session_obj
+            session=mock.sentinel.session,
-        mock_client_init.return_value = None
+            auth=auth,
-        cinder.get_client()
+            endpoint_override=url,
-        mock_session.assert_called_once_with()
+            connect_retries=2,
-        mock_client_init.assert_called_once_with(mock.ANY, **expected)
+            global_request_id='global')
 
-    def test_get_client_with_endpoint_override(
+    def test_get_client(self, mock_client_init, mock_session, mock_auth,
-            self, mock_client_init, mock_session):
+                        mock_sauth, mock_adapter):
-        self.config(url='http://test-url', group='cinder')
+
-        mock_session_obj = mock.Mock()
+        mock_adapter.return_value = mock_adapter_obj = mock.Mock()
-        expected = {'connect_retries': 2,
+        mock_adapter_obj.get_endpoint.return_value = 'cinder_url'
-                    'endpoint_override': 'http://test-url',
+        self._assert_client_call(mock_client_init, 'cinder_url')
-                    'session': mock_session_obj}
+        mock_session.assert_called_once_with('cinder')
-        mock_session.return_value = mock_session_obj
+        mock_auth.assert_called_once_with('cinder')
-        mock_client_init.return_value = None
+        mock_adapter.assert_called_once_with('cinder',
-        cinder.get_client()
+                                             session=mock.sentinel.session,
-        mock_client_init.assert_called_once_with(mock.ANY, **expected)
+                                             auth=mock.sentinel.auth)
-        mock_session.assert_called_once_with()
+        self.assertFalse(mock_sauth.called)
+
+    def test_get_client_deprecated_opts(self, mock_client_init, mock_session,
+                                        mock_auth, mock_sauth, mock_adapter):
 
-    def test_get_client_with_region(self, mock_client_init, mock_session):
-        mock_session_obj = mock.Mock()
-        expected = {'connect_retries': 2,
-                    'region_name': 'test-region',
-                    'session': mock_session_obj}
-        mock_session.return_value = mock_session_obj
         self.config(region_name='test-region',
                     group='keystone')
-        mock_client_init.return_value = None
+        self.config(url='http://test-url', group='cinder')
-        cinder.get_client()
+        mock_adapter.return_value = mock_adapter_obj = mock.Mock()
-        mock_client_init.assert_called_once_with(mock.ANY, **expected)
+        mock_adapter_obj.get_endpoint.return_value = 'http://test-url'
-        mock_session.assert_called_once_with()
+
+        self._assert_client_call(mock_client_init, 'http://test-url')
+        mock_auth.assert_called_once_with('cinder')
+        mock_session.assert_called_once_with('cinder')
+        mock_adapter.assert_called_once_with(
+            'cinder', session=mock.sentinel.session, auth=mock.sentinel.auth,
+            endpoint_override='http://test-url', region_name='test-region')
+        self.assertFalse(mock_sauth.called)
 
 
 class TestCinderUtils(db_base.DbTestCase):

releasenotes/notes/deprecated-cinder-opts-e10c153768285cab.yaml

@@ -0,0 +1,8 @@
+---
+deprecations:
+  - |
+    Configuration option ``[cinder]/url`` is deprecated
+    and will be ignored in the Rocky release.
+    Instead, use ``[cinder]/endpoint_override`` configuration option to set
+    a specific cinder API address when automatic discovery of the cinder API
+    endpoint from keystone catalog is not desired.

releasenotes/notes/keystoneauth-config-1baa45a0a2dd93b4.yaml

@@ -4,3 +4,7 @@ features:
     Adds the ability to set keystoneauth settings in the
     ``[glance]`` configuration section for service automatic
     discovery.
+  - |
+    Adds the ability to set keystoneauth settings in the
+    ``[cinder]`` configuration section for service automatic
+    discovery.