From 9a104acbae328788451442f71aefbc5c20a98f3c 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) --- 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 8acb8e81f0..a3ae18f51b 100644 --- a/glance/api/v2/images.py +++ b/glance/api/v2/images.py @@ -622,13 +622,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) @@ -652,8 +654,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) @@ -661,7 +665,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 8e47371525..ade5898b17 100644 --- a/glance/common/store_utils.py +++ b/glance/common/store_utils.py @@ -180,22 +180,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 ebbba9e3bd..9e2cae56f3 100644 --- a/glance/tests/unit/common/test_utils.py +++ b/glance/tests/unit/common/test_utils.py @@ -35,28 +35,61 @@ CONF = cfg.CONF 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 75cf61a694..a6c8d7b074 100644 --- a/glance/tests/unit/v2/test_images_resource.py +++ b/glance/tests/unit/v2/test_images_resource.py @@ -5248,7 +5248,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_delete_from_store_as_non_owner(self):