Merge "Use conditional update for group update and delete"

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.