Add image_set_property_atomic() helper

This adds a new DB API method to atomically create a property on an image
in a way that we can be sure it is created once and only once for the
purposes of exclusion of multiple threads.

Change-Id: Ifdb711cb241ef13eccaa5ae29a234f2fe4a52eb8
Related-Bug: #1884596
(cherry picked from commit 2a51843138)

glance/db/__init__.py

@@ -220,6 +220,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):
 

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]

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

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,

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')

glance/tests/unit/test_db.py

@@ -437,6 +437,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):

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):