From fdfb2d51a4f362091ab5e94981d18d7741f11cf6 Mon Sep 17 00:00:00 2001 From: Sean McGinnis Date: Fri, 1 Sep 2017 16:12:56 -0500 Subject: [PATCH] Use conditional update for group update and delete Consistency groups had conditional updating to handle API race conditions, but the switch to groups did not include that. Adding conditional update handling for update and delete so we have the same protection. Also relaxed the restriction on update to allow updating name or description when in states other than Available. Change-Id: I9ddd7e881be23be8b7d37063d87417c47deda9e8 Closes-bug: #1673319 --- cinder/group/api.py | 58 +++++++++++++------ .../api/contrib/test_consistencygroups.py | 9 ++- cinder/tests/unit/api/v3/test_groups.py | 30 +++++++++- .../notes/group-update-d423eaa18dbcecc1.yaml | 6 ++ 4 files changed, 84 insertions(+), 19 deletions(-) create mode 100644 releasenotes/notes/group-update-d423eaa18dbcecc1.yaml diff --git a/cinder/group/api.py b/cinder/group/api.py index b012927b3f8..a7da0ce7869 100644 --- a/cinder/group/api.py +++ b/cinder/group/api.py @@ -516,6 +516,7 @@ class API(base.Base): @wrap_check_policy def delete(self, context, group, delete_volumes=False): + if not group.host: self.update_quota(context, group, -1, group.project_id) @@ -540,6 +541,9 @@ class API(base.Base): raise exception.InvalidGroup( reason=_("Group has existing snapshots.")) + # TODO(smcginnis): Add conditional update handling for volumes + # Should probably utilize the volume_api.delete code to handle + # cascade snapshot deletion and force delete. volumes = self.db.volume_get_all_by_generic_group(context.elevated(), group.id) if volumes and not delete_volumes: @@ -570,9 +574,31 @@ class API(base.Base): self.db.volumes_update(context, volumes_model_update) - group.status = c_fields.GroupStatus.DELETING - group.terminated_at = timeutils.utcnow() - group.save() + if delete_volumes: + # We're overloading the term "delete_volumes" somewhat to also + # mean to delete the group regardless of the state. + expected = {} + else: + expected = {'status': (c_fields.GroupStatus.AVAILABLE, + c_fields.GroupStatus.ERROR)} + filters = [~db.group_has_group_snapshot_filter(), + ~db.group_has_volumes_filter( + attached_or_with_snapshots=delete_volumes), + ~db.group_creating_from_src(group_id=group.id)] + values = {'status': c_fields.GroupStatus.DELETING} + + if not group.conditional_update(values, expected, filters): + if delete_volumes: + reason = _('Group status must be available or error and must ' + 'not have dependent group snapshots') + else: + reason = _('Group must not have attached volumes, volumes ' + 'with snapshots, or dependent group snapshots') + msg = _('Cannot delete group %(id)s. %(reason)s, and ' + 'it cannot be the source for an ongoing group or group ' + 'snapshot creation.') % { + 'id': group.id, 'reason': reason} + raise exception.InvalidGroup(reason=msg) self.volume_rpcapi.delete_group(context, group) @@ -580,10 +606,13 @@ class API(base.Base): def update(self, context, group, name, description, add_volumes, remove_volumes): """Update group.""" - if group.status != c_fields.GroupStatus.AVAILABLE: - msg = _("Group status must be available, " - "but current status is: %s.") % group.status - raise exception.InvalidGroup(reason=msg) + # Validate name. + if name == group.name: + name = None + + # Validate description. + if description == group.description: + description = None add_volumes_list = [] remove_volumes_list = [] @@ -605,14 +634,6 @@ class API(base.Base): volumes = self.db.volume_get_all_by_generic_group(context, group.id) - # Validate name. - if name == group.name: - name = None - - # Validate description. - if description == group.description: - description = None - # Validate volumes in add_volumes and remove_volumes. add_volumes_new = "" remove_volumes_new = "" @@ -631,6 +652,7 @@ class API(base.Base): {'group_id': group.id}) raise exception.InvalidGroup(reason=msg) + expected = {} fields = {'updated_at': timeutils.utcnow()} # Update name and description in db now. No need to @@ -643,10 +665,12 @@ class API(base.Base): # Only update name or description. Set status to available. fields['status'] = c_fields.GroupStatus.AVAILABLE else: + expected['status'] = c_fields.GroupStatus.AVAILABLE fields['status'] = c_fields.GroupStatus.UPDATING - group.update(fields) - group.save() + if not group.conditional_update(fields, expected): + msg = _("Group status must be available.") + raise exception.InvalidGroup(reason=msg) # Do an RPC call only if the update request includes # adding/removing volumes. add_volumes_new and remove_volumes_new diff --git a/cinder/tests/unit/api/contrib/test_consistencygroups.py b/cinder/tests/unit/api/contrib/test_consistencygroups.py index abba2abc268..9688e562e22 100644 --- a/cinder/tests/unit/api/contrib/test_consistencygroups.py +++ b/cinder/tests/unit/api/contrib/test_consistencygroups.py @@ -1111,16 +1111,23 @@ class ConsistencyGroupsAPITestCase(test.TestCase): consistencygroup.destroy() def test_update_consistencygroup_invalid_state(self): + volume_type_id = utils.create_volume_type( + context.get_admin_context(), self, name='my_vol_type')['id'] consistencygroup = self._create_consistencygroup( status=fields.ConsistencyGroupStatus.CREATING, + volume_type_ids=[volume_type_id], ctxt=self.ctxt) + add_volume_id = utils.create_volume( + self.ctxt, + testcase_instance=self, + volume_type_id=volume_type_id)['id'] req = webob.Request.blank('/v2/%s/consistencygroups/%s/update' % (fake.PROJECT_ID, consistencygroup.id)) req.method = 'PUT' req.headers['Content-Type'] = 'application/json' body = {"consistencygroup": {"name": "new name", "description": None, - "add_volumes": None, + "add_volumes": add_volume_id, "remove_volumes": None, }} req.body = jsonutils.dump_as_bytes(body) res = req.get_response(fakes.wsgi_app( diff --git a/cinder/tests/unit/api/v3/test_groups.py b/cinder/tests/unit/api/v3/test_groups.py index 2789985c383..ad5c57c6403 100644 --- a/cinder/tests/unit/api/v3/test_groups.py +++ b/cinder/tests/unit/api/v3/test_groups.py @@ -846,6 +846,25 @@ class GroupsAPITestCase(test.TestCase): add_volume.destroy() add_volume2.destroy() + @ddt.data(fields.GroupStatus.CREATING, fields.GroupStatus.UPDATING) + def test_update_group_any_state(self, status): + self.group1.status = status + req = fakes.HTTPRequest.blank('/v3/%s/groups/%s/update' % + (fake.PROJECT_ID, self.group1.id), + version=GROUP_MICRO_VERSION) + body = {"group": {"name": "new name", + "description": "new description", + "add_volumes": None, + "remove_volumes": None, }} + + res_dict = self.controller.update( + req, self.group1.id, body) + self.assertEqual(http_client.ACCEPTED, res_dict.status_int) + + group = objects.Group.get_by_id(self.ctxt, self.group1.id) + self.assertEqual("new name", group.name) + self.assertEqual("new description", group.description) + def test_update_group_add_volume_not_found(self): self.group1.status = fields.GroupStatus.AVAILABLE self.group1.save() @@ -959,18 +978,27 @@ class GroupsAPITestCase(test.TestCase): @ddt.data(fields.GroupStatus.CREATING, fields.GroupStatus.UPDATING) def test_update_group_invalid_state(self, status): self.group1.status = status + add_volume = utils.create_volume( + self.ctxt, + volume_type_id=self.volume_type1.id, + host=self.group1.host) req = fakes.HTTPRequest.blank('/v3/%s/groups/%s/update' % (fake.PROJECT_ID, self.group1.id), version=GROUP_MICRO_VERSION) + body = {"group": {"name": "new name", "description": None, - "add_volumes": None, + "add_volumes": add_volume.id, "remove_volumes": None, }} self.assertRaises(webob.exc.HTTPBadRequest, self.controller.update, req, self.group1.id, body) + vol = objects.Volume.get_by_id(self.ctxt, add_volume.id) + self.assertEqual(add_volume.status, vol.status) + add_volume.destroy() + @ddt.data(('3.11', 'fake_group_001', fields.GroupStatus.AVAILABLE, exception.VersionNotFoundForAPIMethod), diff --git a/releasenotes/notes/group-update-d423eaa18dbcecc1.yaml b/releasenotes/notes/group-update-d423eaa18dbcecc1.yaml new file mode 100644 index 00000000000..8d99e20a492 --- /dev/null +++ b/releasenotes/notes/group-update-d423eaa18dbcecc1.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + Volume group updates of any kind had previously required the group to be + in ``Available`` status. Updates to the group name or + description will now work regardless of the volume group status.