From b35728019e0eb89c213eed7bc35a1f062c99dcca Mon Sep 17 00:00:00 2001 From: Flavio Percoco Date: Wed, 18 Dec 2013 15:18:17 +0100 Subject: [PATCH] Filter out deleted images from storage usage All database API's currently include deleted images in the calc of storage usage. This is not an issue when deleted images don't have locations. However, there are cases where a deleted image has deleted locations as well and that causes the current algorithm to count those locations as if they were allocating space. Besides this bug, it makes sense to not load deleted / killed / pending_delete images from the database if we're actually not considering them as valid images. The patch also filters out deleted locations. NOTE: In the case of locations, it was not possible to add a test for the deleted locations because it requires some changes that are not worth in this patch. In order to mark a location as deleted, it's necessary to go through the API and use a PATCH operation. Since this is a database test, it doesn't make much sense to add API calls to it. Calling the image_destroy function with an empty location list will remove all the locations which won't help testing that specific case. I'll work on a better solution for that in a follow-up patch. DocImpact Change-Id: I82f08a8f522c81541e4f77597c2ba0aeb68556ce Closes-Bug: #1261738 --- glance/db/simple/api.py | 13 +++++++++- glance/db/sqlalchemy/api.py | 9 ++++++- glance/tests/functional/db/base.py | 26 ++++++++++++++++++- glance/tests/functional/db/test_sqlalchemy.py | 8 ++++++ 4 files changed, 53 insertions(+), 3 deletions(-) diff --git a/glance/db/simple/api.py b/glance/db/simple/api.py index d126366138..9fa3832292 100644 --- a/glance/db/simple/api.py +++ b/glance/db/simple/api.py @@ -580,6 +580,12 @@ def image_destroy(context, image_id): DATA['images'][image_id]['deleted'] = True DATA['images'][image_id]['deleted_at'] = timeutils.utcnow() + # NOTE(flaper87): Move the image to one of the deleted statuses + # if it hasn't been done yet. + if (DATA['images'][image_id]['status'] not in + ['deleted', 'pending_delete']): + DATA['images'][image_id]['status'] = 'deleted' + _image_locations_set(image_id, []) for prop in DATA['images'][image_id]['properties']: @@ -687,8 +693,13 @@ def user_get_storage_usage(context, owner_id, image_id=None, session=None): images = image_get_all(context, filters={'owner': owner_id}) total = 0 for image in images: + if image['status'] in ['killed', 'pending_delete', 'deleted']: + continue + if image['id'] != image_id: - total = total + (image['size'] * len(image['locations'])) + locations = [l for l in image['locations'] + if not l.get('deleted', False)] + total += (image['size'] * len(locations)) return total diff --git a/glance/db/sqlalchemy/api.py b/glance/db/sqlalchemy/api.py index 5b77d09521..b20d100945 100644 --- a/glance/db/sqlalchemy/api.py +++ b/glance/db/sqlalchemy/api.py @@ -712,8 +712,15 @@ def _image_get_disk_usage_by_owner(owner, session, image_id=None): if image_id is not None: query = query.filter(models.Image.id != image_id) query = query.filter(models.Image.size > 0) + query = query.filter(~models.Image.status.in_(['killed', + 'pending_delete', + 'deleted'])) images = query.all() - total = sum([i.size * len(i.locations) for i in images]) + + total = 0 + for i in images: + locations = [l for l in i.locations if not l['deleted']] + total += (i.size * len(locations)) return total diff --git a/glance/tests/functional/db/base.py b/glance/tests/functional/db/base.py index bf3c3d4fef..81191f00b3 100644 --- a/glance/tests/functional/db/base.py +++ b/glance/tests/functional/db/base.py @@ -1166,7 +1166,8 @@ class DriverQuotaTests(test_utils.BaseTestCase): super(DriverQuotaTests, self).setUp() self.owner_id1 = uuidutils.generate_uuid() self.context1 = context.RequestContext( - is_admin=False, auth_tok='user:user:user', user=self.owner_id1) + is_admin=False, user=self.owner_id1, tenant=self.owner_id1, + auth_tok='%s:%s:user' % (self.owner_id1, self.owner_id1)) self.db_api = db_tests.get_db(self.config) db_tests.reset_db(self.db_api) self.addCleanup(timeutils.clear_time_override) @@ -1232,6 +1233,29 @@ class DriverQuotaTests(test_utils.BaseTestCase): x = self.db_api.user_get_storage_usage(self.context1, self.owner_id1) self.assertEqual(total, x) + def test_storage_quota_deleted_image(self): + # NOTE(flaper87): This needs to be tested for + # soft deleted images as well. Currently there's no + # good way to delete locations. + dt1 = timeutils.utcnow() + sz = 53 + new_fixture_dict = {'id': 'SOMEID', 'created_at': dt1, + 'updated_at': dt1, 'size': sz, + 'owner': self.owner_id1} + new_fixture = build_image_fixture(**new_fixture_dict) + new_fixture['locations'].append({'url': 'file:///some/path/file', + 'metadata': {}}) + self.db_api.image_create(self.context1, new_fixture) + + total = reduce(lambda x, y: x + y, + [f['size'] for f in self.owner1_fixtures]) + x = self.db_api.user_get_storage_usage(self.context1, self.owner_id1) + self.assertEqual(total + (sz * 2), x) + + self.db_api.image_destroy(self.context1, 'SOMEID') + x = self.db_api.user_get_storage_usage(self.context1, self.owner_id1) + self.assertEqual(total, x) + class TaskTests(test_utils.BaseTestCase): diff --git a/glance/tests/functional/db/test_sqlalchemy.py b/glance/tests/functional/db/test_sqlalchemy.py index 2be31ad6a0..42798a233b 100644 --- a/glance/tests/functional/db/test_sqlalchemy.py +++ b/glance/tests/functional/db/test_sqlalchemy.py @@ -106,3 +106,11 @@ class TestSqlAlchemyTask(base.TaskTests): db_tests.load(get_db, reset_db) super(TestSqlAlchemyTask, self).setUp() self.addCleanup(db_tests.reset) + + +class TestSqlAlchemyQuota(base.DriverQuotaTests): + + def setUp(self): + db_tests.load(get_db, reset_db) + super(TestSqlAlchemyQuota, self).setUp() + self.addCleanup(db_tests.reset)