From 7d211d6221ec17544984f0c33e11c3428e50aa3b Mon Sep 17 00:00:00 2001 From: Ivan Kolodyazhny Date: Thu, 30 Apr 2020 18:46:07 +0300 Subject: [PATCH] Use resource_backend for volumes and groups resource_backend should be used during groups update validation procedure instead of 'host' attribute because it works well both in clustered and single node deployments. Change-Id: I4f80bb3e39e04ac5c524c7c1ad8859eef76a910a Closes-Bug: #1876133 --- cinder/group/api.py | 36 ++++++++++--------- cinder/tests/unit/group/test_groups_api.py | 28 +++++++++------ ...-actions-in-a-a-mode-5d554b30a26da22c.yaml | 10 ++++++ 3 files changed, 47 insertions(+), 27 deletions(-) create mode 100644 releasenotes/notes/fix-groups-actions-in-a-a-mode-5d554b30a26da22c.yaml diff --git a/cinder/group/api.py b/cinder/group/api.py index 04f20fdd46a..29c138e2f34 100644 --- a/cinder/group/api.py +++ b/cinder/group/api.py @@ -723,7 +723,7 @@ class API(base.Base): for add_vol in add_volumes_list: try: - add_vol_ref = self.db.volume_get(context, add_vol) + add_vol_ref = objects.Volume.get_by_id(context, add_vol) except exception.VolumeNotFound: msg = (_("Cannot add volume %(volume_id)s to " "group %(group_id)s because volume cannot be " @@ -731,7 +731,7 @@ class API(base.Base): {'volume_id': add_vol, 'group_id': group.id}) raise exception.InvalidVolume(reason=msg) - orig_group = add_vol_ref.get('group_id', None) + orig_group = add_vol_ref.group_id if orig_group: # If volume to be added is already in the group to be updated, # it should have been removed from the add_volumes_list in the @@ -740,7 +740,7 @@ class API(base.Base): msg = (_("Cannot add volume %(volume_id)s to group " "%(group_id)s because it is already in " "group %(orig_group)s.") % - {'volume_id': add_vol_ref['id'], + {'volume_id': add_vol_ref.id, 'group_id': group.id, 'orig_group': orig_group}) raise exception.InvalidVolume(reason=msg) @@ -749,15 +749,15 @@ class API(base.Base): msg = (_("Cannot add volume %(volume_id)s to group " "%(group_id)s as they belong to different " "projects.") % - {'volume_id': add_vol_ref['id'], + {'volume_id': add_vol_ref.id, 'group_id': group.id}) raise exception.InvalidVolume(reason=msg) - add_vol_type_id = add_vol_ref.get('volume_type_id', None) + add_vol_type_id = add_vol_ref.volume_type_id if not add_vol_type_id: msg = (_("Cannot add volume %(volume_id)s to group " "%(group_id)s because it has no volume " "type.") % - {'volume_id': add_vol_ref['id'], + {'volume_id': add_vol_ref.id, 'group_id': group.id}) raise exception.InvalidVolume(reason=msg) vol_type_ids = [v_type.id for v_type in group.volume_types] @@ -766,27 +766,29 @@ class API(base.Base): "%(group_id)s because volume type " "%(volume_type)s is not supported by the " "group.") % - {'volume_id': add_vol_ref['id'], + {'volume_id': add_vol_ref.id, 'group_id': group.id, 'volume_type': add_vol_type_id}) raise exception.InvalidVolume(reason=msg) - if (add_vol_ref['status'] not in + if (add_vol_ref.status not in VALID_ADD_VOL_TO_GROUP_STATUS): msg = (_("Cannot add volume %(volume_id)s to group " "%(group_id)s because volume is in an " "invalid state: %(status)s. Valid states are: " "%(valid)s.") % - {'volume_id': add_vol_ref['id'], + {'volume_id': add_vol_ref.id, 'group_id': group.id, - 'status': add_vol_ref['status'], + 'status': add_vol_ref.status, 'valid': VALID_ADD_VOL_TO_GROUP_STATUS}) raise exception.InvalidVolume(reason=msg) - # group.host and add_vol_ref['host'] are in this format: - # 'host@backend#pool'. Extract host (host@backend) before - # doing comparison. - vol_host = volume_utils.extract_host(add_vol_ref['host']) - group_host = volume_utils.extract_host(group.host) + # group.resource_backend and add_vol_ref.resource_backend are + # in this format like 'host@backend#pool' in a non-HA + # deployment and will contain cluster_name in + # A/A HA deployment. + vol_host = volume_utils.extract_host( + add_vol_ref.resource_backend) + group_host = volume_utils.extract_host(group.resource_backend) if group_host != vol_host: raise exception.InvalidVolume( reason=_("Volume is not local to this node.")) @@ -794,12 +796,12 @@ class API(base.Base): # Volume exists. It will be added to CG. if add_volumes_new: add_volumes_new += "," - add_volumes_new += add_vol_ref['id'] + add_volumes_new += add_vol_ref.id else: msg = (_("Cannot add volume %(volume_id)s to group " "%(group_id)s because volume does not exist.") % - {'volume_id': add_vol_ref['id'], + {'volume_id': add_vol_ref.id, 'group_id': group.id}) raise exception.InvalidVolume(reason=msg) diff --git a/cinder/tests/unit/group/test_groups_api.py b/cinder/tests/unit/group/test_groups_api.py index 227cf4f1b86..10dbd4f3414 100644 --- a/cinder/tests/unit/group/test_groups_api.py +++ b/cinder/tests/unit/group/test_groups_api.py @@ -274,6 +274,8 @@ class GroupAPITestCase(test.TestCase): self.group_api._validate_add_volumes, self.ctxt, [], ['123456789'], grp) + @ddt.data(['test_host@fakedrv#fakepool', 'test_host@fakedrv#fakepool'], + ['test_host@fakedrv#fakepool', 'test_host2@fakedrv#fakepool']) @mock.patch('cinder.volume.rpcapi.VolumeAPI.update_group') @mock.patch('cinder.db.volume_get_all_by_generic_group') @mock.patch('cinder.group.api.API._cast_create_group') @@ -281,7 +283,7 @@ class GroupAPITestCase(test.TestCase): @mock.patch('cinder.objects.Group') @mock.patch('cinder.db.group_type_get') @mock.patch('cinder.db.volume_types_get_by_name_or_id') - def test_update(self, mock_volume_types_get, + def test_update(self, hosts, mock_volume_types_get, mock_group_type_get, mock_group, mock_update_quota, mock_cast_create_group, mock_volume_get_all, mock_rpc_update_group): @@ -307,26 +309,32 @@ class GroupAPITestCase(test.TestCase): self.assertEqual(grp.obj_to_primitive(), ret_group.obj_to_primitive()) ret_group.volume_types = [vol_type] - ret_group.host = "test_host@fakedrv#fakepool" + ret_group.host = hosts[0] + # set resource_backend directly because ret_group + # is instance of MagicMock + ret_group.resource_backend = 'fake-cluster' ret_group.status = fields.GroupStatus.AVAILABLE + ret_group.id = fake.GROUP_ID vol1 = utils.create_volume( - self.ctxt, host=ret_group.host, - availability_zone=ret_group.availability_zone, - volume_type_id=fake.VOLUME_TYPE_ID) - - vol2 = utils.create_volume( - self.ctxt, host=ret_group.host, + self.ctxt, host=hosts[1], availability_zone=ret_group.availability_zone, volume_type_id=fake.VOLUME_TYPE_ID, - group_id=fake.GROUP_ID) + cluster_name='fake-cluster') + + vol2 = utils.create_volume( + self.ctxt, host=hosts[1], + availability_zone=ret_group.availability_zone, + volume_type_id=fake.VOLUME_TYPE_ID, + group_id=fake.GROUP_ID, + cluster_name='fake-cluster') vol2_dict = { 'id': vol2.id, 'group_id': fake.GROUP_ID, 'volume_type_id': fake.VOLUME_TYPE_ID, 'availability_zone': ret_group.availability_zone, - 'host': ret_group.host, + 'host': hosts[1], 'status': 'available', } mock_volume_get_all.return_value = [vol2_dict] diff --git a/releasenotes/notes/fix-groups-actions-in-a-a-mode-5d554b30a26da22c.yaml b/releasenotes/notes/fix-groups-actions-in-a-a-mode-5d554b30a26da22c.yaml new file mode 100644 index 00000000000..1bd9fb836fe --- /dev/null +++ b/releasenotes/notes/fix-groups-actions-in-a-a-mode-5d554b30a26da22c.yaml @@ -0,0 +1,10 @@ +--- +fixes: + - | + Fixed volume group action in Active/Active HA deployment: + + * Update group + (`#1876133 `_) + + * Create group from group snapshot + (`#1867906 `_)