From baf6b5ebf05ce7a2fb012f4a90cf34c61059d97a Mon Sep 17 00:00:00 2001 From: Abhishek Kekane Date: Mon, 6 Jul 2020 07:49:31 +0000 Subject: [PATCH] Improve lazy loading mechanism for multiple stores Glance has a facility lazy loading for legacy images which will be called on get/list api calls to add store information in image's location metadata based on location URL of image. Even if admin decides to change the store names in glance-api.conf same will also be updated in location metadata for all images related to that particular store. Current implementation of legacy image performs this operation on each get/list call as location metadata is not getting updated in database or it doesn't handle to perform store name check in glance-api.conf. Improvements done: 1. Save updated location metadata information in database permenantly 2. Add logic to perform lazy loading only if store information is not present in location metadata or store present in location metadata is not defined in glance's enbaled_backends configuration option. Change-Id: I789fa7adfb459e7861c90a51f418a635c0c22244 Closes-Bug: #1886374 (cherry picked from commit ab0e5268a9c2614572659d763b3c0b6fc36dd0cf) (cherry picked from commit 9a104acbae328788451442f71aefbc5c20a98f3c) --- glance/api/authorization.py | 12 +++--- glance/api/v2/images.py | 12 ++++-- glance/common/store_utils.py | 35 ++++++++++----- glance/tests/unit/common/test_utils.py | 45 +++++++++++++++++--- glance/tests/unit/v2/test_images_resource.py | 2 +- 5 files changed, 78 insertions(+), 28 deletions(-) diff --git a/glance/api/authorization.py b/glance/api/authorization.py index b498c8bb77..791c537187 100644 --- a/glance/api/authorization.py +++ b/glance/api/authorization.py @@ -32,12 +32,12 @@ LOG = logging.getLogger(__name__) def lazy_update_store_info(func): """Update store information in location metadata""" @functools.wraps(func) - def wrapped(context, image, **kwargs): + def wrapped(context, image, image_repo, **kwargs): if CONF.enabled_backends: store_utils.update_store_in_locations( - image.locations, image.image_id) + image, image_repo) - return func(context, image, **kwargs) + return func(context, image, image_repo, **kwargs) return wrapped @@ -54,7 +54,7 @@ def is_image_mutable(context, image): @lazy_update_store_info -def proxy_image(context, image): +def proxy_image(context, image, image_repo): if is_image_mutable(context, image): return ImageProxy(image, context) else: @@ -127,11 +127,11 @@ class ImageRepoProxy(glance.domain.proxy.Repo): def get(self, image_id): image = self.image_repo.get(image_id) - return proxy_image(self.context, image) + return proxy_image(self.context, image, self.image_repo) def list(self, *args, **kwargs): images = self.image_repo.list(*args, **kwargs) - return [proxy_image(self.context, i) for i in images] + return [proxy_image(self.context, i, self.image_repo) for i in images] def _validate_image_accepts_members(visibility): diff --git a/glance/api/v2/images.py b/glance/api/v2/images.py index 2259802767..23f7c24867 100644 --- a/glance/api/v2/images.py +++ b/glance/api/v2/images.py @@ -523,13 +523,15 @@ class ImagesController(object): val_data = self._validate_validation_data(image, value) # NOTE(abhishekk): get glance store based on location uri + updated_location = value if CONF.enabled_backends: - store_utils.update_store_in_locations(value, image.image_id) + updated_location = store_utils.get_updated_store_location( + value) try: # NOTE(flwang): _locations_proxy's setattr method will check if # the update is acceptable. - image.locations = value + image.locations = updated_location if image.status == 'queued': for k, v in val_data.items(): setattr(image, k, v) @@ -553,8 +555,10 @@ class ImagesController(object): val_data = self._validate_validation_data(image, [value]) # NOTE(abhishekk): get glance store based on location uri + updated_location = value if CONF.enabled_backends: - store_utils.update_store_in_locations([value], image.image_id) + updated_location = store_utils.get_updated_store_location( + [value])[0] pos = self._get_locations_op_pos(path_pos, len(image.locations), True) @@ -562,7 +566,7 @@ class ImagesController(object): msg = _("Invalid position for adding a location.") raise webob.exc.HTTPBadRequest(explanation=msg) try: - image.locations.insert(pos, value) + image.locations.insert(pos, updated_location) if image.status == 'queued': for k, v in val_data.items(): setattr(image, k, v) diff --git a/glance/common/store_utils.py b/glance/common/store_utils.py index ab8fbc2ee1..b0f2b06564 100644 --- a/glance/common/store_utils.py +++ b/glance/common/store_utils.py @@ -178,22 +178,35 @@ def _get_store_id_from_uri(uri): return -def update_store_in_locations(locations, image_id): +def update_store_in_locations(image, image_repo): + for loc in image.locations: + if (not loc['metadata'].get( + 'store') or loc['metadata'].get( + 'store') not in CONF.enabled_backends): + store_id = _get_store_id_from_uri(loc['url']) + if store_id: + if 'store' in loc['metadata']: + old_store = loc['metadata']['store'] + if old_store != store_id: + LOG.debug("Store '%(old)s' has changed to " + "'%(new)s' by operator, updating " + "the same in the location of image " + "'%(id)s'", {'old': old_store, + 'new': store_id, + 'id': image.image_id}) + + loc['metadata']['store'] = store_id + image_repo.save(image) + + +def get_updated_store_location(locations): for loc in locations: store_id = _get_store_id_from_uri(loc['url']) if store_id: - if 'store' in loc['metadata']: - old_store = loc['metadata']['store'] - if old_store != store_id: - LOG.debug("Store '%(old)s' has changed to " - "'%(new)s' by operator, updating " - "the same in the location of image " - "'%(id)s'", {'old': old_store, - 'new': store_id, - 'id': image_id}) - loc['metadata']['store'] = store_id + return locations + def get_dir_separator(): separator = '' diff --git a/glance/tests/unit/common/test_utils.py b/glance/tests/unit/common/test_utils.py index 530f4d9c94..324fe239b6 100644 --- a/glance/tests/unit/common/test_utils.py +++ b/glance/tests/unit/common/test_utils.py @@ -31,28 +31,61 @@ from glance.tests import utils as test_utils class TestStoreUtils(test_utils.BaseTestCase): """Test glance.common.store_utils module""" - def _test_update_store_in_location(self, metadata, store_id, expected): + def _test_update_store_in_location(self, metadata, store_id, expected, + store_id_call_count=1, + save_call_count=1): + image = mock.Mock() + image_repo = mock.Mock() + image_repo.save = mock.Mock() locations = [{ 'url': 'rbd://aaaaaaaa/images/id', 'metadata': metadata }] + image.locations = locations with mock.patch.object( store_utils, '_get_store_id_from_uri') as mock_get_store_id: mock_get_store_id.return_value = store_id - store_utils.update_store_in_locations(locations, mock.Mock()) - self.assertEqual(locations[0]['metadata'].get('store'), expected) + store_utils.update_store_in_locations(image, image_repo) + self.assertEqual(image.locations[0]['metadata'].get( + 'store'), expected) + self.assertEqual(store_id_call_count, mock_get_store_id.call_count) + self.assertEqual(save_call_count, image_repo.save.call_count) def test_update_store_location_with_no_store(self): + enabled_backends = { + "rbd1": "rbd", + "rbd2": "rbd" + } + self.config(enabled_backends=enabled_backends) self._test_update_store_in_location({}, 'rbd1', 'rbd1') def test_update_store_location_with_different_store(self): - self._test_update_store_in_location({'store': 'rbd2'}, 'rbd1', 'rbd1') + enabled_backends = { + "ceph1": "rbd", + "ceph2": "rbd" + } + self.config(enabled_backends=enabled_backends) + self._test_update_store_in_location( + {'store': 'rbd2'}, 'ceph1', 'ceph1') def test_update_store_location_with_same_store(self): - self._test_update_store_in_location({'store': 'rbd1'}, 'rbd1', 'rbd1') + enabled_backends = { + "rbd1": "rbd", + "rbd2": "rbd" + } + self.config(enabled_backends=enabled_backends) + self._test_update_store_in_location({'store': 'rbd1'}, 'rbd1', 'rbd1', + store_id_call_count=0, + save_call_count=0) def test_update_store_location_with_store_none(self): - self._test_update_store_in_location({}, None, None) + enabled_backends = { + "rbd1": "rbd", + "rbd2": "rbd" + } + self.config(enabled_backends=enabled_backends) + self._test_update_store_in_location({}, None, None, + save_call_count=0) class TestUtils(test_utils.BaseTestCase): diff --git a/glance/tests/unit/v2/test_images_resource.py b/glance/tests/unit/v2/test_images_resource.py index a23a197353..89a4c6593f 100644 --- a/glance/tests/unit/v2/test_images_resource.py +++ b/glance/tests/unit/v2/test_images_resource.py @@ -5172,7 +5172,7 @@ class TestMultiImagesController(base.MultiIsolatedUnitTest): request = unit_test_utils.get_fake_request() self.assertRaises(webob.exc.HTTPConflict, self.controller.import_image, - request, UUID1, + request, UUID2, {'method': {'name': 'glance-direct'}}) def test_image_lazy_loading_store(self):