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

@@ -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
+        if delete_volumes:
-        group.terminated_at = timeutils.utcnow()
+            # We're overloading the term "delete_volumes" somewhat to also
-        group.save()
+            # 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:
+        # Validate name.
-            msg = _("Group status must be available, "
+        if name == group.name:
-                    "but current status is: %s.") % group.status
+            name = None
-            raise exception.InvalidGroup(reason=msg)
+
+        # 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)
+        if not group.conditional_update(fields, expected):
-        group.save()
+            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

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(

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

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.