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
This commit is contained in:
parent
7c91a177ed
commit
b093ae3514
|
@ -210,6 +210,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):
|
||||
|
||||
|
|
|
@ -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]
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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,
|
||||
|
|
|
@ -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')
|
||||
|
|
|
@ -430,6 +430,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):
|
||||
|
|
|
@ -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):
|
||||
|
|
Loading…
Reference in New Issue