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

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

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]

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

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,

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,
+class TestImageAtomicOps(base.TestDriver,
-                            base.FunctionalInitWrapper):
+                         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')

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

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