From 825a0ec754f0e5c94819c9e22482841bf3b7fd16 Mon Sep 17 00:00:00 2001 From: Dan Smith Date: Tue, 28 Jul 2020 06:49:50 -0700 Subject: [PATCH] Add image_delete_property_atomic() helper This adds a new DB API method to atomically delete a property on an image in a way that we can be sure that it is deleted one and only once, and without affecting the rest of the image. This can be used in conjunction with image_set_property_atomic() to create and delete properties for locking without the risk of clobbering other image data in the process. Change-Id: I0b71a7df04cd330749f35b07f96a120b49b412c7 (cherry picked from commit b093ae3514dea32a8829239e0297e3e170efff58) --- glance/db/__init__.py | 4 ++ glance/db/simple/api.py | 15 ++++++++ glance/db/sqlalchemy/api.py | 28 ++++++++++++++ glance/domain/proxy.py | 5 +++ glance/tests/functional/db/test_sqlalchemy.py | 37 +++++++++++++++++-- glance/tests/unit/test_db.py | 21 +++++++++++ glance/tests/unit/test_domain_proxy.py | 12 ++++++ 7 files changed, 119 insertions(+), 3 deletions(-) diff --git a/glance/db/__init__.py b/glance/db/__init__.py index 7d8e4e8279..04bdddd145 100644 --- a/glance/db/__init__.py +++ b/glance/db/__init__.py @@ -224,6 +224,10 @@ class ImageRepo(object): self.db_api.image_set_property_atomic( image.image_id, name, value) + def delete_property_atomic(self, image, name, value): + self.db_api.image_delete_property_atomic( + image.image_id, name, value) + class ImageProxy(glance.domain.proxy.Image): diff --git a/glance/db/simple/api.py b/glance/db/simple/api.py index dfdc51cad1..3f4acf7385 100644 --- a/glance/db/simple/api.py +++ b/glance/db/simple/api.py @@ -439,6 +439,21 @@ def image_set_property_atomic(image_id, name, value): image['properties'].append(prop) +def image_delete_property_atomic(image_id, name, value): + try: + image = DATA['images'][image_id] + except KeyError: + LOG.warn(_LW('Could not find image %s'), image_id) + raise exception.ImageNotFound() + + for i, prop in enumerate(image['properties']): + if prop['name'] == name and prop['value'] == value: + del image['properties'][i] + return + + raise exception.NotFound() + + def _image_get(context, image_id, force_show_deleted=False, status=None): try: image = DATA['images'][image_id] diff --git a/glance/db/sqlalchemy/api.py b/glance/db/sqlalchemy/api.py index c359d6c2b4..13dd21914a 100644 --- a/glance/db/sqlalchemy/api.py +++ b/glance/db/sqlalchemy/api.py @@ -834,6 +834,34 @@ def image_set_property_atomic(image_id, name, value): # caused us to fail if we lost the race +def image_delete_property_atomic(image_id, name, value): + """ + Atomically delete an image property. + + This will only succeed if the referenced image has a property set + to exactly the value provided. + + :param image_id: The ID of the image on which to delete the property + :param name: The property name + :param value: The value the property is expected to be set to + :raises NotFound: If the property does not exist + """ + session = get_session() + with session.begin(): + connection = session.connection() + table = models.ImageProperty.__table__ + + result = connection.execute(table.delete().where( + sa_sql.and_(table.c.name == name, + table.c.value == value, + table.c.image_id == image_id, + table.c.deleted == 0))) + if result.rowcount == 1: + return + + raise exception.NotFound() + + @retry(retry_on_exception=_retry_on_deadlock, wait_fixed=500, stop_max_attempt_number=50) @utils.no_4byte_params diff --git a/glance/domain/proxy.py b/glance/domain/proxy.py index 01ec4a0961..2572aba128 100644 --- a/glance/domain/proxy.py +++ b/glance/domain/proxy.py @@ -109,6 +109,11 @@ class Repo(object): assert hasattr(item, 'image_id'), msg self.base.set_property_atomic(item, name, value) + def delete_property_atomic(self, item, name, value): + msg = '%s is only valid for images' % __name__ + assert hasattr(item, 'image_id'), msg + self.base.delete_property_atomic(item, name, value) + class MemberRepo(object): def __init__(self, image, base, diff --git a/glance/tests/functional/db/test_sqlalchemy.py b/glance/tests/functional/db/test_sqlalchemy.py index 9dce9f42f0..0093a44f49 100644 --- a/glance/tests/functional/db/test_sqlalchemy.py +++ b/glance/tests/functional/db/test_sqlalchemy.py @@ -172,12 +172,12 @@ class TestMetadefSqlAlchemyDriver(base_metadef.TestMetadefDriver, self.addCleanup(db_tests.reset) -class TestImageAtomicUpdate(base.TestDriver, - base.FunctionalInitWrapper): +class TestImageAtomicOps(base.TestDriver, + base.FunctionalInitWrapper): def setUp(self): db_tests.load(get_db, reset_db) - super(TestImageAtomicUpdate, self).setUp() + super(TestImageAtomicOps, self).setUp() self.addCleanup(db_tests.reset) self.image = self.db_api.image_create( @@ -306,3 +306,34 @@ class TestImageAtomicUpdate(base.TestDriver, # Make sure the update affected only the intended image self.assertOnlyImageHasProp(image_id2, 'test_property', 'baz') + + def test_delete(self): + """Try to double-delete a property atomically. + + This should ensure that a second attempt fails. + """ + + self.db_api.image_delete_property_atomic(self.image['id'], + 'speed', '88mph') + + self.assertRaises(exception.NotFound, + self.db_api.image_delete_property_atomic, + self.image['id'], 'speed', '88mph') + + def test_delete_create_delete(self): + """Try to delete, re-create, and then re-delete property.""" + self.db_api.image_delete_property_atomic(self.image['id'], + 'speed', '88mph') + self.db_api.image_update(self.adm_context, self.image['id'], + {'properties': {'speed': '89mph'}}, + purge_props=True) + + # We should no longer be able to delete the property by the *old* + # value + self.assertRaises(exception.NotFound, + self.db_api.image_delete_property_atomic, + self.image['id'], 'speed', '88mph') + + # Only the new value should result in proper deletion + self.db_api.image_delete_property_atomic(self.image['id'], + 'speed', '89mph') diff --git a/glance/tests/unit/test_db.py b/glance/tests/unit/test_db.py index 2c5a24867c..1cf25fd211 100644 --- a/glance/tests/unit/test_db.py +++ b/glance/tests/unit/test_db.py @@ -457,6 +457,27 @@ class TestImageRepo(test_utils.BaseTestCase): image = self.image_repo.get(image.image_id) self.assertEqual({'foo': 'bar'}, image.extra_properties) + def test_image_delete_property_atomic(self): + image_id = uuid.uuid4() + image = _db_fixture(image_id, name='test') + + self.assertRaises(exception.NotFound, + self.db.image_delete_property_atomic, + image_id, 'foo', 'bar') + self.db.image_create(self.context, image) + self.db.image_set_property_atomic(image_id, 'foo', 'bar') + self.db.image_delete_property_atomic(image_id, 'foo', 'bar') + image = self.image_repo.get(image_id) + self.assertEqual({}, image.extra_properties) + + def test_delete_property_atomic(self): + image = self.image_repo.get(UUID1) + self.image_repo.set_property_atomic(image, 'foo', 'bar') + image = self.image_repo.get(image.image_id) + self.image_repo.delete_property_atomic(image, 'foo', 'bar') + image = self.image_repo.get(image.image_id) + self.assertEqual({}, image.extra_properties) + class TestEncryptedLocations(test_utils.BaseTestCase): def setUp(self): diff --git a/glance/tests/unit/test_domain_proxy.py b/glance/tests/unit/test_domain_proxy.py index 30042da916..d4309e93df 100644 --- a/glance/tests/unit/test_domain_proxy.py +++ b/glance/tests/unit/test_domain_proxy.py @@ -51,6 +51,7 @@ class FakeRepo(object): save = fake_method remove = fake_method set_property_atomic = fake_method + delete_property_atomic = fake_method class TestProxyRepoPlain(test_utils.BaseTestCase): @@ -93,6 +94,17 @@ class TestProxyRepoPlain(test_utils.BaseTestCase): self._test_method, 'set_property_atomic', None, 'notimage', 'foo', 'bar') + def test_delete_property_atomic(self): + image = mock.MagicMock() + image.image_id = 'foo' + self._test_method('delete_property_atomic', None, image, 'foo', 'bar') + + def test_delete_property_nonimage(self): + self.assertRaises( + AssertionError, + self._test_method, + 'delete_property_atomic', None, 'notimage', 'foo', 'bar') + class TestProxyRepoWrapping(test_utils.BaseTestCase): def setUp(self):