From 3e84bdb6db67856558aa8ed167721e5e6e2599e6 Mon Sep 17 00:00:00 2001 From: Pavlo Shchelokovskyy Date: Wed, 21 Jun 2017 07:48:04 +0000 Subject: [PATCH] Use adapters for cinderclient deprecates the `[cinder]url` option in favor of [cinder]endpoint_override. Change-Id: Idd02e8cf0892965a3138479e49ec40cfeda7c96d Partial-Bug: #1699547 --- devstack/lib/ironic | 2 +- etc/ironic/ironic.conf.sample | 45 +++++++++- ironic/common/cinder.py | 49 +++++++---- ironic/conf/cinder.py | 10 ++- ironic/tests/unit/common/test_cinder.py | 86 +++++++++++-------- ...precated-cinder-opts-e10c153768285cab.yaml | 8 ++ .../keystoneauth-config-1baa45a0a2dd93b4.yaml | 4 + 7 files changed, 144 insertions(+), 60 deletions(-) create mode 100644 releasenotes/notes/deprecated-cinder-opts-e10c153768285cab.yaml diff --git a/devstack/lib/ironic b/devstack/lib/ironic index b5a00b99d1..e044891304 100644 --- a/devstack/lib/ironic +++ b/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 diff --git a/etc/ironic/ironic.conf.sample b/etc/ironic/ironic.conf.sample index b32d0a6e75..99f1fed87f 100644 --- a/etc/ironic/ironic.conf.sample +++ b/etc/ironic/ironic.conf.sample @@ -948,12 +948,28 @@ # Domain name to scope to (string value) #domain_name = +# Always use this endpoint URL for requests for this client. +# (string value) +#endpoint_override = + # Verify HTTPS connections. (boolean value) #insecure = false # PEM encoded client certificate key file (string value) #keyfile = +# 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 = + +# 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 = + # User's password (string value) #password = @@ -971,10 +987,22 @@ # Deprecated group/name - [cinder]/tenant_name #project_name = +# The default region_name for endpoint URL discovery. (string +# value) +#region_name = + # 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 = + +# The default service_type for endpoint URL discovery. (string +# value) +#service_type = volumev3 + # Tenant ID (string value) #tenant_id = @@ -987,8 +1015,12 @@ # Trust ID (string value) #trust_id = -# URL for connecting to cinder. If set, the value must start -# with either http:// or https://. (uri value) +# DEPRECATED: URL for connecting to cinder. If set, the 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 = # User's domain id (string value) @@ -1004,6 +1036,15 @@ # Deprecated group/name - [cinder]/user_name #username = +# 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 = + [cisco_ucs] diff --git a/ironic/common/cinder.py b/ironic/common/cinder.py index d174d7e396..3f356fcd07 100644 --- a/ironic/common/cinder.py +++ b/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', auth=auth) + _CINDER_SESSION = keystone.get_session('cinder') 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 = { - 'connect_retries': CONF.cinder.retries - } - # 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 + service_auth = keystone.get_auth('cinder') + session = _get_cinder_session() - if CONF.keystone.region_name: - params['region_name'] = CONF.keystone.region_name + # TODO(pas-ha) remove in Rocky + 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() - - return client.Client(**params) + adapter = keystone.get_adapter('cinder', session=session, + auth=service_auth, **adapter_opts) + # 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}) diff --git a/ironic/conf/cinder.py b/ironic/conf/cinder.py index 4cb79bc59b..a67cbd72a6 100644 --- a/ironic/conf/cinder.py +++ b/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') diff --git a/ironic/tests/unit/common/test_cinder.py b/ironic/tests/unit/common/test_cinder.py index ecc1b4f5f5..ce581f5547 100644 --- a/ironic/tests/unit/common/test_cinder.py +++ b/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( - 'cinder', auth=mock_auth.return_value) - mock_auth.assert_called_once_with('cinder') + mock_keystone_session.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.object(cinderclient.Client, '__init__', autospec=True) +@mock.patch('ironic.common.keystone.get_adapter', 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): - mock_session_obj = mock.Mock() - expected = {'connect_retries': 2, - 'session': mock_session_obj} - mock_session.return_value = mock_session_obj - mock_client_init.return_value = None - cinder.get_client() - mock_session.assert_called_once_with() - mock_client_init.assert_called_once_with(mock.ANY, **expected) + def _assert_client_call(self, init_mock, url, auth=mock.sentinel.auth): + cinder.get_client(self.context) + init_mock.assert_called_once_with( + mock.ANY, + session=mock.sentinel.session, + auth=auth, + endpoint_override=url, + connect_retries=2, + global_request_id='global') - def test_get_client_with_endpoint_override( - self, mock_client_init, mock_session): - self.config(url='http://test-url', group='cinder') - mock_session_obj = mock.Mock() - expected = {'connect_retries': 2, - 'endpoint_override': 'http://test-url', - 'session': mock_session_obj} - mock_session.return_value = mock_session_obj - mock_client_init.return_value = None - cinder.get_client() - mock_client_init.assert_called_once_with(mock.ANY, **expected) - mock_session.assert_called_once_with() + def test_get_client(self, mock_client_init, mock_session, mock_auth, + mock_sauth, mock_adapter): + + mock_adapter.return_value = mock_adapter_obj = mock.Mock() + mock_adapter_obj.get_endpoint.return_value = 'cinder_url' + self._assert_client_call(mock_client_init, 'cinder_url') + mock_session.assert_called_once_with('cinder') + mock_auth.assert_called_once_with('cinder') + mock_adapter.assert_called_once_with('cinder', + session=mock.sentinel.session, + auth=mock.sentinel.auth) + 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 - cinder.get_client() - mock_client_init.assert_called_once_with(mock.ANY, **expected) - mock_session.assert_called_once_with() + self.config(url='http://test-url', group='cinder') + mock_adapter.return_value = mock_adapter_obj = mock.Mock() + mock_adapter_obj.get_endpoint.return_value = 'http://test-url' + + 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): diff --git a/releasenotes/notes/deprecated-cinder-opts-e10c153768285cab.yaml b/releasenotes/notes/deprecated-cinder-opts-e10c153768285cab.yaml new file mode 100644 index 0000000000..a3663f1098 --- /dev/null +++ b/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. diff --git a/releasenotes/notes/keystoneauth-config-1baa45a0a2dd93b4.yaml b/releasenotes/notes/keystoneauth-config-1baa45a0a2dd93b4.yaml index 164fee987b..d9c4a7ee06 100644 --- a/releasenotes/notes/keystoneauth-config-1baa45a0a2dd93b4.yaml +++ b/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.