diff --git a/glance/db/__init__.py b/glance/db/__init__.py index 8e4169e766..818c6442d2 100644 --- a/glance/db/__init__.py +++ b/glance/db/__init__.py @@ -206,6 +206,10 @@ class ImageRepo(object): new_values = self.db_api.image_destroy(self.context, image.image_id) image.updated_at = new_values['updated_at'] + def set_property_atomic(self, image, name, value): + self.db_api.image_set_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 94e07c5a22..dfdc51cad1 100644 --- a/glance/db/simple/api.py +++ b/glance/db/simple/api.py @@ -426,6 +426,19 @@ def _sort_images(images, sort_key, sort_dir): return images +def image_set_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() + + prop = _image_property_format(image_id, + name, + value) + image['properties'].append(prop) + + 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 76363545fc..c359d6c2b4 100644 --- a/glance/db/sqlalchemy/api.py +++ b/glance/db/sqlalchemy/api.py @@ -778,6 +778,62 @@ def _update_values(image_ref, values): setattr(image_ref, k, values[k]) +def image_set_property_atomic(image_id, name, value): + """ + Atomically set an image property to a value. + + This will only succeed if the property does not currently exist + and it was created successfully. This can be used by multiple + competing threads to ensure that only one of those threads + succeeded in creating the property. + + Note that ImageProperty objects are marked as deleted=$id and so we must + first try to atomically update-and-undelete such a property, if it + exists. If that does not work, we should try to create the property. The + latter should fail with DBDuplicateEntry because of the UniqueConstraint + across ImageProperty(image_id, name). + + :param image_id: The ID of the image on which to create the property + :param name: The property name + :param value: The value to set for the property + :raises Duplicate: If the property already exists + """ + session = get_session() + with session.begin(): + connection = session.connection() + table = models.ImageProperty.__table__ + + # This should be: + # UPDATE image_properties SET value=$value, deleted=0 + # WHERE name=$name AND deleted!=0 + result = connection.execute(table.update().where( + sa_sql.and_(table.c.name == name, + table.c.image_id == image_id, + table.c.deleted != 0)).values( + value=value, deleted=0)) + if result.rowcount == 1: + # Found and updated a deleted property, so we win + return + + # There might have been no deleted property, or the property + # exists and is undeleted, so try to create it and use that + # to determine if we've lost the race or not. + + try: + connection.execute(table.insert(), + dict(deleted=False, + created_at=timeutils.utcnow(), + image_id=image_id, + name=name, + value=value)) + except db_exception.DBDuplicateEntry: + # Lost the race to create the new property + raise exception.Duplicate() + + # If we got here, we created a new row, UniqueConstraint would have + # caused us to fail if we lost the race + + @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 391df84de5..01ec4a0961 100644 --- a/glance/domain/proxy.py +++ b/glance/domain/proxy.py @@ -104,6 +104,11 @@ class Repo(object): result = self.base.remove(base_item) return self.helper.proxy(result) + def set_property_atomic(self, item, name, value): + msg = '%s is only valid for images' % __name__ + assert hasattr(item, 'image_id'), msg + self.base.set_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 102bd2e785..9dce9f42f0 100644 --- a/glance/tests/functional/db/test_sqlalchemy.py +++ b/glance/tests/functional/db/test_sqlalchemy.py @@ -170,3 +170,139 @@ class TestMetadefSqlAlchemyDriver(base_metadef.TestMetadefDriver, db_tests.load(get_db, reset_db_metadef) super(TestMetadefSqlAlchemyDriver, self).setUp() self.addCleanup(db_tests.reset) + + +class TestImageAtomicUpdate(base.TestDriver, + base.FunctionalInitWrapper): + + def setUp(self): + db_tests.load(get_db, reset_db) + super(TestImageAtomicUpdate, self).setUp() + + self.addCleanup(db_tests.reset) + self.image = self.db_api.image_create( + self.adm_context, + {'status': 'active', + 'owner': self.adm_context.owner, + 'properties': {'speed': '88mph'}}) + + @staticmethod + def _propdict(list_of_props): + """ + Convert a list of ImageProperty objects to dict, ignoring + deleted values. + """ + return {x.name: x.value + for x in list_of_props + if x.deleted == 0} + + def assertOnlyImageHasProp(self, image_id, name, value): + images_with_prop = self.db_api.image_get_all( + self.adm_context, + {'properties': {name: value}}) + self.assertEqual(1, len(images_with_prop)) + self.assertEqual(image_id, images_with_prop[0]['id']) + + def test_update(self): + """Try to double-create a property atomically. + + This should ensure that a second attempt to create the property + atomically fails with Duplicate. + """ + + # Atomically create the property + self.db_api.image_set_property_atomic(self.image['id'], + 'test_property', 'foo') + + # Make sure only the matched image got it + self.assertOnlyImageHasProp(self.image['id'], 'test_property', 'foo') + + # Trying again should fail + self.assertRaises(exception.Duplicate, + self.db_api.image_set_property_atomic, + self.image['id'], 'test_property', 'bar') + + # Ensure that only the first one stuck + image = self.db_api.image_get(self.adm_context, self.image['id']) + self.assertEqual({'speed': '88mph', 'test_property': 'foo'}, + self._propdict(image['properties'])) + self.assertOnlyImageHasProp(self.image['id'], 'test_property', 'foo') + + def test_update_drop_update(self): + """Try to create, delete, re-create property atomically. + + If we fail to undelete and claim the property, this will + fail as duplicate. + """ + + # Atomically create the property + self.db_api.image_set_property_atomic(self.image['id'], + 'test_property', 'foo') + + # Ensure that it stuck + image = self.db_api.image_get(self.adm_context, self.image['id']) + self.assertEqual({'speed': '88mph', 'test_property': 'foo'}, + self._propdict(image['properties'])) + self.assertOnlyImageHasProp(self.image['id'], 'test_property', 'foo') + + # Update the image with the property removed, like image_repo.save() + new_props = self._propdict(image['properties']) + del new_props['test_property'] + self.db_api.image_update(self.adm_context, self.image['id'], + values={'properties': new_props}, + purge_props=True) + + # Make sure that a fetch shows the property deleted + image = self.db_api.image_get(self.adm_context, self.image['id']) + self.assertEqual({'speed': '88mph'}, + self._propdict(image['properties'])) + + # Atomically update the property, which still exists, but is + # deleted + self.db_api.image_set_property_atomic(self.image['id'], + 'test_property', 'bar') + + # Makes sure we updated the property and undeleted it + image = self.db_api.image_get(self.adm_context, self.image['id']) + self.assertEqual({'speed': '88mph', 'test_property': 'bar'}, + self._propdict(image['properties'])) + self.assertOnlyImageHasProp(self.image['id'], 'test_property', 'bar') + + def test_update_prop_multiple_images(self): + """Create and delete properties on two images, then set on one. + + This tests that the resurrect-from-deleted mode of the method only + matchs deleted properties from our image. + """ + + images = self.db_api.image_get_all(self.adm_context) + + image_id1 = images[0]['id'] + image_id2 = images[-1]['id'] + + # Atomically create the property on each image + self.db_api.image_set_property_atomic(image_id1, + 'test_property', 'foo') + self.db_api.image_set_property_atomic(image_id2, + 'test_property', 'bar') + + # Make sure they got the right property value each + self.assertOnlyImageHasProp(image_id1, 'test_property', 'foo') + self.assertOnlyImageHasProp(image_id2, 'test_property', 'bar') + + # Delete the property on both images + self.db_api.image_update(self.adm_context, image_id1, + {'properties': {}}, + purge_props=True) + self.db_api.image_update(self.adm_context, image_id2, + {'properties': {}}, + purge_props=True) + + # Set the property value on one of the images. Both will have a + # deleted previous value for the property, but only one should + # be updated + self.db_api.image_set_property_atomic(image_id2, + 'test_property', 'baz') + + # Make sure the update affected only the intended image + self.assertOnlyImageHasProp(image_id2, 'test_property', 'baz') diff --git a/glance/tests/unit/test_db.py b/glance/tests/unit/test_db.py index 1b51d7c192..585dc5f92e 100644 --- a/glance/tests/unit/test_db.py +++ b/glance/tests/unit/test_db.py @@ -410,6 +410,26 @@ class TestImageRepo(test_utils.BaseTestCase): self.context, image_id) + def test_image_set_property_atomic(self): + image_id = uuid.uuid4() + image = _db_fixture(image_id, name='test') + + self.assertRaises(exception.ImageNotFound, + self.db.image_set_property_atomic, + image_id, 'foo', 'bar') + + self.db.image_create(self.context, image) + self.db.image_set_property_atomic(image_id, 'foo', 'bar') + image = self.db.image_get(self.context, image_id) + self.assertEqual('foo', image['properties'][0]['name']) + self.assertEqual('bar', image['properties'][0]['value']) + + def test_set_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.assertEqual({'foo': 'bar'}, 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 dfe117fbc3..30042da916 100644 --- a/glance/tests/unit/test_domain_proxy.py +++ b/glance/tests/unit/test_domain_proxy.py @@ -50,6 +50,7 @@ class FakeRepo(object): add = fake_method save = fake_method remove = fake_method + set_property_atomic = fake_method class TestProxyRepoPlain(test_utils.BaseTestCase): @@ -81,6 +82,17 @@ class TestProxyRepoPlain(test_utils.BaseTestCase): def test_remove(self): self._test_method('add', None, 'flying') + def test_set_property_atomic(self): + image = mock.MagicMock() + image.image_id = 'foo' + self._test_method('set_property_atomic', None, image, 'foo', 'bar') + + def test_set_property_nonimage(self): + self.assertRaises( + AssertionError, + self._test_method, + 'set_property_atomic', None, 'notimage', 'foo', 'bar') + class TestProxyRepoWrapping(test_utils.BaseTestCase): def setUp(self):