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.