From d6800e143ddff8898d693bbef65d2143544d7ad0 Mon Sep 17 00:00:00 2001 From: Long Quan Sha Date: Thu, 11 Jun 2015 18:46:56 +0800 Subject: [PATCH] reuse the deleted image-member before create a new image-member If glance backend database is not MySQL or postgreSQL,the unique constraint of image-member only includes image-id and member. If then an image-member is deleted, then create it again with the same parameters, glance initiates a query to check if there is already an existing one, but the result doesn't include the record which was marked as deleted, glance will try to create a new one with the same parameters, it will fail with SQL0803N error. To fix this,we should check all existing image-member records including the deleted image-member before create image-member, then update it if it exists, otherwise create a new one. APIImpact Closes-Bug: #1462315 Implements: bp reuse-the-deleted-image-member Change-Id: I84f88d133bf4ac6daa0ff5d148aed86c0ff2cb2d --- glance/db/__init__.py | 18 +++++++- glance/db/registry/api.py | 15 +++++-- glance/db/simple/api.py | 3 +- glance/db/sqlalchemy/api.py | 20 ++++++--- glance/registry/api/v1/members.py | 6 ++- glance/tests/unit/v1/test_registry_api.py | 43 ++++++++++++++++++++ glance/tests/unit/v2/test_registry_client.py | 25 ++++++++++++ 7 files changed, 117 insertions(+), 13 deletions(-) diff --git a/glance/db/__init__.py b/glance/db/__init__.py index 50e3e2ff3f..8b8665b4a1 100644 --- a/glance/db/__init__.py +++ b/glance/db/__init__.py @@ -353,8 +353,22 @@ class ImageMemberRepo(object): raise exception.Duplicate(msg) image_member_values = self._format_image_member_to_db(image_member) - new_values = self.db_api.image_member_create(self.context, - image_member_values) + # Note(shalq): find the image member including the member marked with + # deleted. We will use only one record to represent membership between + # the same image and member. The record of the deleted image member + # will be reused, if it exists, update its properties instead of + # creating a new one. + members = self.db_api.image_member_find(self.context, + image_id=self.image.image_id, + member=image_member.member_id, + include_deleted=True) + if members: + new_values = self.db_api.image_member_update(self.context, + members[0]['id'], + image_member_values) + else: + new_values = self.db_api.image_member_create(self.context, + image_member_values) image_member.created_at = new_values['created_at'] image_member.updated_at = new_values['updated_at'] image_member.id = new_values['id'] diff --git a/glance/db/registry/api.py b/glance/db/registry/api.py index ff1820d835..c236492870 100644 --- a/glance/db/registry/api.py +++ b/glance/db/registry/api.py @@ -188,15 +188,24 @@ def image_member_delete(client, memb_id, session=None): @_get_client -def image_member_find(client, image_id=None, member=None, status=None): - """Find all members that meet the given criteria +def image_member_find(client, image_id=None, member=None, status=None, + include_deleted=False): + """Find all members that meet the given criteria. + + Note, currently include_deleted should be true only when create a new + image membership, as there may be a deleted image membership between + the same image and tenant, the membership will be reused in this case. + It should be false in other cases. :param image_id: identifier of image entity :param member: tenant to which membership has been granted + :include_deleted: A boolean indicating whether the result should include + the deleted record of image member """ return client.image_member_find(image_id=image_id, member=member, - status=status) + status=status, + include_deleted=include_deleted) @_get_client diff --git a/glance/db/simple/api.py b/glance/db/simple/api.py index bfb5066955..1e738f481a 100644 --- a/glance/db/simple/api.py +++ b/glance/db/simple/api.py @@ -454,7 +454,8 @@ def image_property_delete(context, prop_ref, image_ref): @log_call -def image_member_find(context, image_id=None, member=None, status=None): +def image_member_find(context, image_id=None, member=None, + status=None, include_deleted=False): filters = [] images = DATA['images'] members = DATA['members'] diff --git a/glance/db/sqlalchemy/api.py b/glance/db/sqlalchemy/api.py index f82fb8e0d8..1e046d42c6 100644 --- a/glance/db/sqlalchemy/api.py +++ b/glance/db/sqlalchemy/api.py @@ -1074,21 +1074,31 @@ def _image_member_get(context, memb_id, session): return query.one() -def image_member_find(context, image_id=None, member=None, status=None): - """Find all members that meet the given criteria +def image_member_find(context, image_id=None, member=None, + status=None, include_deleted=False): + """Find all members that meet the given criteria. + + Note, currently include_deleted should be true only when create a new + image membership, as there may be a deleted image membership between + the same image and tenant, the membership will be reused in this case. + It should be false in other cases. :param image_id: identifier of image entity :param member: tenant to which membership has been granted + :include_deleted: A boolean indicating whether the result should include + the deleted record of image member """ session = get_session() - members = _image_member_find(context, session, image_id, member, status) + members = _image_member_find(context, session, image_id, + member, status, include_deleted) return [_image_member_format(m) for m in members] def _image_member_find(context, session, image_id=None, - member=None, status=None): + member=None, status=None, include_deleted=False): query = session.query(models.ImageMember) - query = query.filter_by(deleted=False) + if not include_deleted: + query = query.filter_by(deleted=False) if not context.is_admin: query = query.join(models.Image) diff --git a/glance/registry/api/v1/members.py b/glance/registry/api/v1/members.py index 3db07d42ea..61128cfac6 100644 --- a/glance/registry/api/v1/members.py +++ b/glance/registry/api/v1/members.py @@ -157,7 +157,8 @@ class Controller(object): # Try to find the corresponding membership members = self.db_api.image_member_find(req.context, image_id=datum['image_id'], - member=datum['member']) + member=datum['member'], + include_deleted=True) try: member = members[0] except IndexError: @@ -256,7 +257,8 @@ class Controller(object): # Look up an existing membership... members = self.db_api.image_member_find(req.context, image_id=image_id, - member=id) + member=id, + include_deleted=True) if members: if can_share is not None: values = dict(can_share=can_share) diff --git a/glance/tests/unit/v1/test_registry_api.py b/glance/tests/unit/v1/test_registry_api.py index 0936818991..faf70ceee4 100644 --- a/glance/tests/unit/v1/test_registry_api.py +++ b/glance/tests/unit/v1/test_registry_api.py @@ -1766,6 +1766,49 @@ class TestRegistryAPI(base.IsolatedUnitTest, test_utils.RegistryAPIMixIn): res = req.get_response(api) self.assertEqual(res.status_int, 403) + def test_add_member_delete_create(self): + """ + Test check that the same member can be successfully added after delete + it, and the same record will be reused for the same membership. + """ + # add a member + UUID8 = _gen_uuid() + extra_fixture = self.get_fixture(id=UUID8, size=19, protected=False, + owner='test user') + + db_api.image_create(self.context, extra_fixture) + fixture = dict(can_share=True) + test_uri = '/images/%s/members/test_add_member_delete_create' + body = jsonutils.dumps(dict(member=fixture)) + self.get_api_response_ext(204, url=test_uri % UUID8, + method='PUT', body=body, + content_type='json') + memb_list = db_api.image_member_find(self.context, image_id=UUID8) + self.assertEqual(1, len(memb_list)) + memb_list2 = db_api.image_member_find(self.context, + image_id=UUID8, + include_deleted=True) + self.assertEqual(1, len(memb_list2)) + # delete the member + self.get_api_response_ext(204, method='DELETE', + url=test_uri % UUID8) + memb_list = db_api.image_member_find(self.context, image_id=UUID8) + self.assertEqual(0, len(memb_list)) + memb_list2 = db_api.image_member_find(self.context, + image_id=UUID8, + include_deleted=True) + self.assertEqual(1, len(memb_list2)) + # create it again + self.get_api_response_ext(204, url=test_uri % UUID8, + method='PUT', body=body, + content_type='json') + memb_list = db_api.image_member_find(self.context, image_id=UUID8) + self.assertEqual(1, len(memb_list)) + memb_list2 = db_api.image_member_find(self.context, + image_id=UUID8, + include_deleted=True) + self.assertEqual(1, len(memb_list2)) + def test_get_on_image_member(self): """ Test GET on image members raises 405 and produces correct Allow headers diff --git a/glance/tests/unit/v2/test_registry_client.py b/glance/tests/unit/v2/test_registry_client.py index f307bce982..4ad52df871 100644 --- a/glance/tests/unit/v2/test_registry_client.py +++ b/glance/tests/unit/v2/test_registry_client.py @@ -639,6 +639,31 @@ class TestRegistryV2Client(base.IsolatedUnitTest, num_members = len(memb_list) self.assertEqual(0, num_members) + def test_image_member_find_include_deleted(self): + """Tests getting member images include the delted member""" + values = dict(image_id=UUID2, member='pattieblack') + # create a member + member = self.client.image_member_create(values=values) + memb_list = self.client.image_member_find(member='pattieblack') + memb_list2 = self.client.image_member_find(member='pattieblack', + include_deleted=True) + self.assertEqual(1, len(memb_list)) + self.assertEqual(1, len(memb_list2)) + # delete the member + self.client.image_member_delete(memb_id=member['id']) + memb_list = self.client.image_member_find(member='pattieblack') + memb_list2 = self.client.image_member_find(member='pattieblack', + include_deleted=True) + self.assertEqual(0, len(memb_list)) + self.assertEqual(1, len(memb_list2)) + # create it again + member = self.client.image_member_create(values=values) + memb_list = self.client.image_member_find(member='pattieblack') + memb_list2 = self.client.image_member_find(member='pattieblack', + include_deleted=True) + self.assertEqual(1, len(memb_list)) + self.assertEqual(2, len(memb_list2)) + def test_add_update_members(self): """Tests updating image members""" values = dict(image_id=UUID2, member='pattieblack')