From 1b2742a3d397d67ea989f080d44cc537bdcf1f5a Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Tue, 26 May 2020 20:40:08 +0200 Subject: [PATCH] NetApp: Support custom igroups This patch adds compatibility for the NetApp driver to allow custom and OpenStack igroups to coexist with each other. Current NetApp dataontap code for iSCSI and FC fails when detaching a volume if we have a custom (as in not created by the Cinder driver) igroup on the array. This is because when we unmap a volume we look for an igroup created by the Cinder driver, which has the "openstack-" prefix, but we won't find it when there's a custom igroup because on attach we used the custom igroup instead of creating an "openstack-" one. Since the NetApp backend supports having multiple igroups with the same initiators we will only use the OpenStack one when mapping volumes and create one if it doesn't exist, even if there's already a custom one. This patch also adds code so we can properly unmap volumes that have been atached with old code and custom igroups. Closes-Bug: #1697490 Change-Id: I9490229fb4f2852cd1d5e5c838b6ca59fe946358 --- .../volume/drivers/netapp/dataontap/fakes.py | 6 +++++ .../netapp/dataontap/test_block_base.py | 9 ++++--- .../netapp/dataontap/test_block_cmode.py | 24 ++++++++++-------- .../drivers/netapp/dataontap/block_base.py | 17 +++++++------ .../drivers/netapp/dataontap/block_cmode.py | 25 +++++++++++-------- ...netapp-custom-igroup-e049b4f3b341dd54.yaml | 5 ++++ 6 files changed, 54 insertions(+), 32 deletions(-) create mode 100644 releasenotes/notes/fix-netapp-custom-igroup-e049b4f3b341dd54.yaml diff --git a/cinder/tests/unit/volume/drivers/netapp/dataontap/fakes.py b/cinder/tests/unit/volume/drivers/netapp/dataontap/fakes.py index 7658aa5cbeb..950e92283ee 100644 --- a/cinder/tests/unit/volume/drivers/netapp/dataontap/fakes.py +++ b/cinder/tests/unit/volume/drivers/netapp/dataontap/fakes.py @@ -213,6 +213,12 @@ IGROUP1 = { 'initiator-group-name': IGROUP1_NAME, } +CUSTOM_IGROUP = { + 'initiator-group-os-type': 'linux', + 'initiator-group-type': 'fcp', + 'initiator-group-name': 'node1', +} + ISCSI_VOLUME = { 'name': 'fake_volume', 'id': 'fake_id', diff --git a/cinder/tests/unit/volume/drivers/netapp/dataontap/test_block_base.py b/cinder/tests/unit/volume/drivers/netapp/dataontap/test_block_base.py index 06448e1a8af..a81778bc54f 100644 --- a/cinder/tests/unit/volume/drivers/netapp/dataontap/test_block_base.py +++ b/cinder/tests/unit/volume/drivers/netapp/dataontap/test_block_base.py @@ -400,10 +400,13 @@ class NetAppBlockStorageLibraryTestCase(test.TestCase): self.assertEqual( 0, self.library._create_igroup_add_initiators.call_count) + @ddt.data([], [fake.CUSTOM_IGROUP]) @mock.patch.object(uuid, 'uuid4', mock.Mock(return_value=fake.UUID1)) - def test_get_or_create_igroup_none_preexisting(self): - """This method also tests _create_igroup_add_initiators.""" - self.zapi_client.get_igroup_by_initiators.return_value = [] + def test_get_or_create_igroup_none_preexisting(self, igroups): + """Test _create_igroup_add_initiators with not OS igroups.""" + # We only care about the OpenStack igroups, so we must have the same + # result if there are no igroups and if the igroup is a custom one. + self.zapi_client.get_igroup_by_initiators.return_value = igroups igroup_name, os, ig_type = self.library._get_or_create_igroup( fake.FC_FORMATTED_INITIATORS, 'fcp', 'linux') diff --git a/cinder/tests/unit/volume/drivers/netapp/dataontap/test_block_cmode.py b/cinder/tests/unit/volume/drivers/netapp/dataontap/test_block_cmode.py index 75d03b4bff0..398d3d97e5c 100644 --- a/cinder/tests/unit/volume/drivers/netapp/dataontap/test_block_cmode.py +++ b/cinder/tests/unit/volume/drivers/netapp/dataontap/test_block_cmode.py @@ -207,12 +207,17 @@ class NetAppBlockStorageCmodeLibraryTestCase(test.TestCase): self.library.vserver, volume_list, []) def test_find_mapped_lun_igroup(self): - igroups = [fake.IGROUP1] + igroups = [fake.IGROUP1, fake.CUSTOM_IGROUP] self.zapi_client.get_igroup_by_initiators.return_value = igroups - lun_maps = [{'initiator-group': fake.IGROUP1_NAME, - 'lun-id': '1', - 'vserver': fake.VSERVER_NAME}] + lun_maps = [ + {'initiator-group': fake.IGROUP1_NAME, + 'lun-id': '1', + 'vserver': fake.VSERVER_NAME}, + {'initiator-group': fake.CUSTOM_IGROUP['initiator-group-name'], + 'lun-id': '2', + 'vserver': fake.VSERVER_NAME} + ] self.zapi_client.get_lun_map.return_value = lun_maps (igroup, lun_id) = self.library._find_mapped_lun_igroup( @@ -253,12 +258,11 @@ class NetAppBlockStorageCmodeLibraryTestCase(test.TestCase): self.assertIsNone(lun_id) def test_find_mapped_lun_igroup_no_igroup_prefix(self): - igroups = [{'initiator-group-os-type': 'linux', - 'initiator-group-type': 'fcp', - 'initiator-group-name': 'igroup2'}] + igroups = [fake.CUSTOM_IGROUP] + expected_igroup = fake.CUSTOM_IGROUP['initiator-group-name'] self.zapi_client.get_igroup_by_initiators.return_value = igroups - lun_maps = [{'initiator-group': 'igroup2', + lun_maps = [{'initiator-group': expected_igroup, 'lun-id': '1', 'vserver': fake.VSERVER_NAME}] self.zapi_client.get_lun_map.return_value = lun_maps @@ -266,8 +270,8 @@ class NetAppBlockStorageCmodeLibraryTestCase(test.TestCase): (igroup, lun_id) = self.library._find_mapped_lun_igroup( fake.LUN_PATH, fake.FC_FORMATTED_INITIATORS) - self.assertIsNone(igroup) - self.assertIsNone(lun_id) + self.assertEqual(expected_igroup, igroup) + self.assertEqual('1', lun_id) def test_clone_lun_zero_block_count(self): """Test for when clone lun is not passed a block count.""" diff --git a/cinder/volume/drivers/netapp/dataontap/block_base.py b/cinder/volume/drivers/netapp/dataontap/block_base.py index 30e3b8d25cd..3ff5fdd61f3 100644 --- a/cinder/volume/drivers/netapp/dataontap/block_base.py +++ b/cinder/volume/drivers/netapp/dataontap/block_base.py @@ -479,16 +479,17 @@ class NetAppBlockStorageLibrary(object): Creates igroup if not already present with given host os type, igroup type and adds initiators. """ + # Backend supports different igroups with the same initiators, so + # instead of reusing non OpenStack igroups, we make sure we only use + # our own, thus being compatible with custom igroups. igroups = self.zapi_client.get_igroup_by_initiators(initiator_list) - igroup_name = None - - if igroups: - igroup = igroups[0] + for igroup in igroups: igroup_name = igroup['initiator-group-name'] - host_os_type = igroup['initiator-group-os-type'] - initiator_group_type = igroup['initiator-group-type'] - - if not igroup_name: + if igroup_name.startswith(na_utils.OPENSTACK_PREFIX): + host_os_type = igroup['initiator-group-os-type'] + initiator_group_type = igroup['initiator-group-type'] + break + else: igroup_name = self._create_igroup_add_initiators( initiator_group_type, host_os_type, initiator_list) return igroup_name, host_os_type, initiator_group_type diff --git a/cinder/volume/drivers/netapp/dataontap/block_cmode.py b/cinder/volume/drivers/netapp/dataontap/block_cmode.py index f77f7d19688..ca8ba7225df 100644 --- a/cinder/volume/drivers/netapp/dataontap/block_cmode.py +++ b/cinder/volume/drivers/netapp/dataontap/block_cmode.py @@ -195,17 +195,20 @@ class NetAppBlockStorageCmodeLibrary(block_base.NetAppBlockStorageLibrary, def _find_mapped_lun_igroup(self, path, initiator_list): """Find an igroup for a LUN mapped to the given initiator(s).""" - initiator_igroups = self.zapi_client.get_igroup_by_initiators( - initiator_list) - lun_maps = self.zapi_client.get_lun_map(path) - if initiator_igroups and lun_maps: - for igroup in initiator_igroups: - igroup_name = igroup['initiator-group-name'] - if igroup_name.startswith(na_utils.OPENSTACK_PREFIX): - for lun_map in lun_maps: - if lun_map['initiator-group'] == igroup_name: - return igroup_name, lun_map['lun-id'] - return None, None + igroups = [igroup['initiator-group-name'] for igroup in + self.zapi_client.get_igroup_by_initiators(initiator_list)] + # Map igroup-name to lun-id, but only for the requested initiators. + lun_map = {v['initiator-group']: v['lun-id'] + for v in self.zapi_client.get_lun_map(path) + if v['initiator-group'] in igroups} + + igroup_name = lun_id = None + # Give preference to OpenStack igroups, just use the last one if not + # present to allow unmapping old mappings that used a custom igroup. + for igroup_name, lun_id in lun_map.items(): + if igroup_name.startswith(na_utils.OPENSTACK_PREFIX): + break + return igroup_name, lun_id def _clone_lun(self, name, new_name, space_reserved=None, qos_policy_group_name=None, src_block=0, dest_block=0, diff --git a/releasenotes/notes/fix-netapp-custom-igroup-e049b4f3b341dd54.yaml b/releasenotes/notes/fix-netapp-custom-igroup-e049b4f3b341dd54.yaml new file mode 100644 index 00000000000..f8f328d7616 --- /dev/null +++ b/releasenotes/notes/fix-netapp-custom-igroup-e049b4f3b341dd54.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - | + Fix NetApp iSCSI and FC driver issues with custom initiator groups. + (bug 1697490).