From 4da861ae07106504f89a084ad190e8276847eb3a Mon Sep 17 00:00:00 2001 From: whoami-rajat Date: Mon, 23 Mar 2020 14:46:06 +0000 Subject: [PATCH] Refactor methods in cinder store Some of the methods in cinder store are only used by the Store class methods and not used outside. It makes sense to bound them to the class. This refactoring will also help in handling the None imports inside the class configure methods which will be done in a followup. This patch also updates cinderclient version from v2(deprecated) to v3(current). Change-Id: I576b787012e73ebaf3274cfd8c31f79894284891 --- glance_store/_drivers/cinder.py | 209 +++++++++--------- glance_store/tests/unit/test_cinder_store.py | 24 +- .../tests/unit/test_multistore_cinder.py | 24 +- 3 files changed, 123 insertions(+), 134 deletions(-) diff --git a/glance_store/_drivers/cinder.py b/glance_store/_drivers/cinder.py index e8572b6b..bef673f2 100644 --- a/glance_store/_drivers/cinder.py +++ b/glance_store/_drivers/cinder.py @@ -37,7 +37,7 @@ import glance_store.location try: from cinderclient import exceptions as cinder_exception - from cinderclient.v2 import client as cinderclient + from cinderclient.v3 import client as cinderclient from os_brick.initiator import connector from oslo_privsep import priv_context except ImportError: @@ -336,80 +336,6 @@ Related options: ] -def get_root_helper(backend=None): - if backend: - rootwrap = getattr(CONF, backend).rootwrap_config - else: - rootwrap = CONF.glance_store.rootwrap_config - - return 'sudo glance-rootwrap %s' % rootwrap - - -def is_user_overriden(conf, backend=None): - if backend: - store_conf = getattr(conf, backend) - else: - store_conf = conf.glance_store - - return all([store_conf.get('cinder_store_' + key) - for key in ['user_name', 'password', - 'project_name', 'auth_address']]) - - -def get_cinderclient(conf, context=None, backend=None): - if backend: - glance_store = getattr(conf, backend) - else: - glance_store = conf.glance_store - - user_overriden = is_user_overriden(conf, backend=backend) - if user_overriden: - username = glance_store.cinder_store_user_name - password = glance_store.cinder_store_password - project = glance_store.cinder_store_project_name - url = glance_store.cinder_store_auth_address - else: - username = context.user - password = context.auth_token - project = context.tenant - - if glance_store.cinder_endpoint_template: - url = glance_store.cinder_endpoint_template % context.to_dict() - else: - info = glance_store.cinder_catalog_info - service_type, service_name, interface = info.split(':') - try: - catalog = keystone_sc.ServiceCatalogV2(context.service_catalog) - url = catalog.url_for( - region_name=glance_store.cinder_os_region_name, - service_type=service_type, - service_name=service_name, - interface=interface) - except keystone_exc.EndpointNotFound: - reason = _("Failed to find Cinder from a service catalog.") - raise exceptions.BadStoreConfiguration(store_name="cinder", - reason=reason) - - c = cinderclient.Client(username, - password, - project, - auth_url=url, - region_name=glance_store.cinder_os_region_name, - insecure=glance_store.cinder_api_insecure, - retries=glance_store.cinder_http_retries, - cacert=glance_store.cinder_ca_certificates_file) - - LOG.debug('Cinderclient connection created for user %(user)s using URL: ' - '%(url)s.', {'user': username, 'url': url}) - - # noauth extracts user_id:project_id from auth_token - if not user_overriden: - c.client.auth_token = context.auth_token or '%s:%s' % (username, - project) - c.client.management_url = url - return c - - class StoreLocation(glance_store.location.StoreLocation): """Class describing a Cinder URI.""" @@ -433,24 +359,6 @@ class StoreLocation(glance_store.location.StoreLocation): raise exceptions.BadStoreUri(message=reason) -@contextlib.contextmanager -def temporary_chown(path, backend=None): - owner_uid = os.getuid() - orig_uid = os.stat(path).st_uid - - if orig_uid != owner_uid: - processutils.execute('chown', owner_uid, path, - run_as_root=True, - root_helper=get_root_helper(backend=backend)) - try: - yield - finally: - if orig_uid != owner_uid: - processutils.execute('chown', orig_uid, path, - run_as_root=True, - root_helper=get_root_helper(backend=backend)) - - class Store(glance_store.driver.Store): """Cinder backend store adapter.""" @@ -466,6 +374,96 @@ class Store(glance_store.driver.Store): if self.backend_group: self._set_url_prefix() + def get_root_helper(self): + if self.backend_group: + rootwrap = getattr(CONF, self.backend_group).rootwrap_config + else: + rootwrap = CONF.glance_store.rootwrap_config + + return 'sudo glance-rootwrap %s' % rootwrap + + def is_user_overriden(self): + if self.backend_group: + store_conf = getattr(self.conf, self.backend_group) + else: + store_conf = self.conf.glance_store + + return all([store_conf.get('cinder_store_' + key) + for key in ['user_name', 'password', + 'project_name', 'auth_address']]) + + def get_cinderclient(self, context=None): + if self.backend_group: + glance_store = getattr(self.conf, self.backend_group) + else: + glance_store = self.conf.glance_store + + user_overriden = self.is_user_overriden() + if user_overriden: + username = glance_store.cinder_store_user_name + password = glance_store.cinder_store_password + project = glance_store.cinder_store_project_name + url = glance_store.cinder_store_auth_address + else: + username = context.user + password = context.auth_token + project = context.tenant + + if glance_store.cinder_endpoint_template: + url = glance_store.cinder_endpoint_template % context.to_dict() + else: + info = glance_store.cinder_catalog_info + service_type, service_name, interface = info.split(':') + try: + catalog = keystone_sc.ServiceCatalogV2( + context.service_catalog) + url = catalog.url_for( + region_name=glance_store.cinder_os_region_name, + service_type=service_type, + service_name=service_name, + interface=interface) + except keystone_exc.EndpointNotFound: + reason = _("Failed to find Cinder from a service catalog.") + raise exceptions.BadStoreConfiguration(store_name="cinder", + reason=reason) + + c = cinderclient.Client( + username, password, project, auth_url=url, + region_name=glance_store.cinder_os_region_name, + insecure=glance_store.cinder_api_insecure, + retries=glance_store.cinder_http_retries, + cacert=glance_store.cinder_ca_certificates_file) + + LOG.debug( + 'Cinderclient connection created for user %(user)s using URL: ' + '%(url)s.', {'user': username, 'url': url}) + + # noauth extracts user_id:project_id from auth_token + if not user_overriden: + c.client.auth_token = context.auth_token or '%s:%s' % (username, + project) + c.client.management_url = url + return c + + @contextlib.contextmanager + def temporary_chown(self, path): + owner_uid = os.getuid() + orig_uid = os.stat(path).st_uid + + if orig_uid != owner_uid: + processutils.execute( + 'chown', owner_uid, path, + run_as_root=True, + root_helper=self.get_root_helper()) + try: + yield + finally: + if orig_uid != owner_uid: + processutils.execute( + 'chown', orig_uid, path, + run_as_root=True, + root_helper=self.get_root_helper()) + def get_schemes(self): return ('cinder',) @@ -473,8 +471,7 @@ class Store(glance_store.driver.Store): self._url_prefix = "cinder://" def _check_context(self, context, require_tenant=False): - user_overriden = is_user_overriden(self.conf, - backend=self.backend_group) + user_overriden = self.is_user_overriden() if user_overriden and not require_tenant: return if context is None: @@ -523,7 +520,7 @@ class Store(glance_store.driver.Store): def _open_cinder_volume(self, client, volume, mode): attach_mode = 'rw' if mode == 'wb' else 'ro' device = None - root_helper = get_root_helper(backend=self.backend_group) + root_helper = self.get_root_helper() priv_context.init(root_helper=shlex.split(root_helper)) host = socket.gethostname() if self.backend_group: @@ -558,9 +555,9 @@ class Store(glance_store.driver.Store): not conn.do_local_attach): yield device['path'] else: - with temporary_chown(device['path'], - backend=self.backend_group), \ - open(device['path'], mode) as f: + with self.temporary_chown( + device['path'], backend=self.backend_group + ), open(device['path'], mode) as f: yield f except Exception: LOG.exception(_LE('Exception while accessing to cinder volume ' @@ -637,8 +634,7 @@ class Store(glance_store.driver.Store): loc = location.store_location self._check_context(context) try: - client = get_cinderclient(self.conf, context, - backend=self.backend_group) + client = self.get_cinderclient(context) volume = client.volumes.get(loc.volume_id) size = int(volume.metadata.get('image_size', volume.size * units.Gi)) @@ -672,9 +668,7 @@ class Store(glance_store.driver.Store): try: self._check_context(context) - volume = get_cinderclient( - self.conf, context, - backend=self.backend_group).volumes.get(loc.volume_id) + volume = self.get_cinderclient(context).volumes.get(loc.volume_id) return int(volume.metadata.get('image_size', volume.size * units.Gi)) except cinder_exception.NotFound: @@ -708,8 +702,7 @@ class Store(glance_store.driver.Store): """ self._check_context(context, require_tenant=True) - client = get_cinderclient(self.conf, context, - backend=self.backend_group) + client = self.get_cinderclient(context) os_hash_value = hashlib.new(str(hashing_algo)) checksum = hashlib.md5() bytes_written = 0 @@ -834,9 +827,7 @@ class Store(glance_store.driver.Store): loc = location.store_location self._check_context(context) try: - volume = get_cinderclient( - self.conf, context, - backend=self.backend_group).volumes.get(loc.volume_id) + volume = self.get_cinderclient(context).volumes.get(loc.volume_id) volume.delete() except cinder_exception.NotFound: raise exceptions.NotFound(image=loc.volume_id) diff --git a/glance_store/tests/unit/test_cinder_store.py b/glance_store/tests/unit/test_cinder_store.py index 8850cef7..0e9db378 100644 --- a/glance_store/tests/unit/test_cinder_store.py +++ b/glance_store/tests/unit/test_cinder_store.py @@ -24,7 +24,7 @@ import tempfile import time import uuid -from cinderclient.v2 import client as cinderclient +from cinderclient.v3 import client as cinderclient from os_brick.initiator import connector from oslo_concurrency import processutils from oslo_utils import units @@ -64,7 +64,7 @@ class TestCinderStore(base.StoreBaseTest, self.hash_algo = 'sha256' def test_get_cinderclient(self): - cc = cinder.get_cinderclient(self.conf, self.context) + cc = self.store.get_cinderclient(self.context) self.assertEqual('fake_token', cc.client.auth_token) self.assertEqual('http://foo/public_url', cc.client.management_url) @@ -73,7 +73,7 @@ class TestCinderStore(base.StoreBaseTest, self.config(cinder_store_password='test_password') self.config(cinder_store_project_name='test_project') self.config(cinder_store_auth_address='test_address') - cc = cinder.get_cinderclient(self.conf, self.context) + cc = self.store.get_cinderclient(self.context) self.assertIsNone(cc.client.auth_token) self.assertEqual('test_address', cc.client.management_url) @@ -95,9 +95,9 @@ class TestCinderStore(base.StoreBaseTest, with mock.patch.object(os, 'stat', return_value=fake_stat()), \ mock.patch.object(os, 'getuid', return_value=2), \ mock.patch.object(processutils, 'execute') as mock_execute, \ - mock.patch.object(cinder, 'get_root_helper', + mock.patch.object(cinder.Store, 'get_root_helper', return_value='sudo'): - with cinder.temporary_chown('test'): + with self.store.temporary_chown('test'): pass expected_calls = [mock.call('chown', 2, 'test', run_as_root=True, root_helper='sudo'), @@ -177,9 +177,9 @@ class TestCinderStore(base.StoreBaseTest, with mock.patch.object(cinder.Store, '_wait_volume_status', return_value=fake_volume), \ - mock.patch.object(cinder, 'temporary_chown', + mock.patch.object(cinder.Store, 'temporary_chown', side_effect=fake_chown), \ - mock.patch.object(cinder, 'get_root_helper', + mock.patch.object(cinder.Store, 'get_root_helper', return_value=root_helper), \ mock.patch.object(connector.InitiatorConnector, 'factory', side_effect=fake_factory): @@ -250,7 +250,7 @@ class TestCinderStore(base.StoreBaseTest, self.assertEqual('rb', mode) yield volume_file - with mock.patch.object(cinder, 'get_cinderclient') as mock_cc, \ + with mock.patch.object(cinder.Store, 'get_cinderclient') as mock_cc, \ mock.patch.object(self.store, '_open_cinder_volume', side_effect=fake_open): mock_cc.return_value = FakeObject(client=fake_client, @@ -276,7 +276,7 @@ class TestCinderStore(base.StoreBaseTest, fake_volume = FakeObject(size=5, metadata={}) fake_volumes = {fake_volume_uuid: fake_volume} - with mock.patch.object(cinder, 'get_cinderclient') as mocked_cc: + with mock.patch.object(cinder.Store, 'get_cinderclient') as mocked_cc: mocked_cc.return_value = FakeObject(client=fake_client, volumes=fake_volumes) @@ -293,7 +293,7 @@ class TestCinderStore(base.StoreBaseTest, metadata={'image_size': expected_image_size}) fake_volumes = {fake_volume_uuid: fake_volume} - with mock.patch.object(cinder, 'get_cinderclient') as mocked_cc: + with mock.patch.object(cinder.Store, 'get_cinderclient') as mocked_cc: mocked_cc.return_value = FakeObject(client=fake_client, volumes=fake_volumes) @@ -321,7 +321,7 @@ class TestCinderStore(base.StoreBaseTest, self.assertEqual('wb', mode) yield volume_file - with mock.patch.object(cinder, 'get_cinderclient') as mock_cc, \ + with mock.patch.object(cinder.Store, 'get_cinderclient') as mock_cc, \ mock.patch.object(self.store, '_open_cinder_volume', side_effect=fake_open): mock_cc.return_value = FakeObject(client=fake_client, @@ -375,7 +375,7 @@ class TestCinderStore(base.StoreBaseTest, fake_volume = FakeObject(delete=mock.Mock()) fake_volumes = {fake_volume_uuid: fake_volume} - with mock.patch.object(cinder, 'get_cinderclient') as mocked_cc: + with mock.patch.object(cinder.Store, 'get_cinderclient') as mocked_cc: mocked_cc.return_value = FakeObject(client=fake_client, volumes=fake_volumes) diff --git a/glance_store/tests/unit/test_multistore_cinder.py b/glance_store/tests/unit/test_multistore_cinder.py index 12ae7133..0622cb60 100644 --- a/glance_store/tests/unit/test_multistore_cinder.py +++ b/glance_store/tests/unit/test_multistore_cinder.py @@ -93,8 +93,7 @@ class TestMultiCinderStore(base.MultiStoreBaseTest, self.assertEqual("cinder://", self.store.url_prefix) def test_get_cinderclient(self): - cc = cinder.get_cinderclient(self.conf, self.context, - backend="cinder1") + cc = self.store.get_cinderclient(self.context) self.assertEqual('fake_token', cc.client.auth_token) self.assertEqual('http://foo/public_url', cc.client.management_url) @@ -103,8 +102,7 @@ class TestMultiCinderStore(base.MultiStoreBaseTest, self.config(cinder_store_password='test_password', group="cinder1") self.config(cinder_store_project_name='test_project', group="cinder1") self.config(cinder_store_auth_address='test_address', group="cinder1") - cc = cinder.get_cinderclient(self.conf, self.context, - backend="cinder1") + cc = self.store.get_cinderclient(self.context) self.assertIsNone(cc.client.auth_token) self.assertEqual('test_address', cc.client.management_url) @@ -115,9 +113,9 @@ class TestMultiCinderStore(base.MultiStoreBaseTest, with mock.patch.object(os, 'stat', return_value=fake_stat()), \ mock.patch.object(os, 'getuid', return_value=2), \ mock.patch.object(processutils, 'execute') as mock_execute, \ - mock.patch.object(cinder, 'get_root_helper', + mock.patch.object(cinder.Store, 'get_root_helper', return_value='sudo'): - with cinder.temporary_chown('test'): + with self.store.temporary_chown('test'): pass expected_calls = [mock.call('chown', 2, 'test', run_as_root=True, root_helper='sudo'), @@ -197,9 +195,9 @@ class TestMultiCinderStore(base.MultiStoreBaseTest, with mock.patch.object(cinder.Store, '_wait_volume_status', return_value=fake_volume), \ - mock.patch.object(cinder, 'temporary_chown', + mock.patch.object(cinder.Store, 'temporary_chown', side_effect=fake_chown), \ - mock.patch.object(cinder, 'get_root_helper', + mock.patch.object(cinder.Store, 'get_root_helper', return_value=root_helper), \ mock.patch.object(connector.InitiatorConnector, 'factory', side_effect=fake_factory): @@ -271,7 +269,7 @@ class TestMultiCinderStore(base.MultiStoreBaseTest, self.assertEqual('rb', mode) yield volume_file - with mock.patch.object(cinder, 'get_cinderclient') as mock_cc, \ + with mock.patch.object(cinder.Store, 'get_cinderclient') as mock_cc, \ mock.patch.object(self.store, '_open_cinder_volume', side_effect=fake_open): mock_cc.return_value = FakeObject(client=fake_client, @@ -299,7 +297,7 @@ class TestMultiCinderStore(base.MultiStoreBaseTest, fake_volume = FakeObject(size=5, metadata={}) fake_volumes = {fake_volume_uuid: fake_volume} - with mock.patch.object(cinder, 'get_cinderclient') as mocked_cc: + with mock.patch.object(cinder.Store, 'get_cinderclient') as mocked_cc: mocked_cc.return_value = FakeObject(client=fake_client, volumes=fake_volumes) @@ -318,7 +316,7 @@ class TestMultiCinderStore(base.MultiStoreBaseTest, metadata={'image_size': expected_image_size}) fake_volumes = {fake_volume_uuid: fake_volume} - with mock.patch.object(cinder, 'get_cinderclient') as mocked_cc: + with mock.patch.object(cinder.Store, 'get_cinderclient') as mocked_cc: mocked_cc.return_value = FakeObject(client=fake_client, volumes=fake_volumes) @@ -347,7 +345,7 @@ class TestMultiCinderStore(base.MultiStoreBaseTest, self.assertEqual('wb', mode) yield volume_file - with mock.patch.object(cinder, 'get_cinderclient') as mock_cc, \ + with mock.patch.object(cinder.Store, 'get_cinderclient') as mock_cc, \ mock.patch.object(self.store, '_open_cinder_volume', side_effect=fake_open): mock_cc.return_value = FakeObject(client=fake_client, @@ -403,7 +401,7 @@ class TestMultiCinderStore(base.MultiStoreBaseTest, fake_volume = FakeObject(delete=mock.Mock()) fake_volumes = {fake_volume_uuid: fake_volume} - with mock.patch.object(cinder, 'get_cinderclient') as mocked_cc: + with mock.patch.object(cinder.Store, 'get_cinderclient') as mocked_cc: mocked_cc.return_value = FakeObject(client=fake_client, volumes=fake_volumes)