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

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

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):
+    def test_get_or_create_igroup_none_preexisting(self, igroups):
-        """This method also tests _create_igroup_add_initiators."""
+        """Test _create_igroup_add_initiators with not OS igroups."""
-        self.zapi_client.get_igroup_by_initiators.return_value = []
+        # 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')

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_maps = [
-                     'lun-id': '1',
+            {'initiator-group': fake.IGROUP1_NAME,
-                     'vserver': fake.VSERVER_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',
+        igroups = [fake.CUSTOM_IGROUP]
-                    'initiator-group-type': 'fcp',
+        expected_igroup = fake.CUSTOM_IGROUP['initiator-group-name']
-                    'initiator-group-name': 'igroup2'}]
         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.assertEqual(expected_igroup, igroup)
-        self.assertIsNone(lun_id)
+        self.assertEqual('1', lun_id)
 
     def test_clone_lun_zero_block_count(self):
         """Test for when clone lun is not passed a block count."""

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
+        for igroup in igroups:
-
-        if igroups:
-            igroup = igroups[0]
             igroup_name = igroup['initiator-group-name']
-            host_os_type = igroup['initiator-group-os-type']
+            if igroup_name.startswith(na_utils.OPENSTACK_PREFIX):
-            initiator_group_type = igroup['initiator-group-type']
+                host_os_type = igroup['initiator-group-os-type']
-
+                initiator_group_type = igroup['initiator-group-type']
-        if not igroup_name:
+                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

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(
+        igroups = [igroup['initiator-group-name'] for igroup in
-            initiator_list)
+                   self.zapi_client.get_igroup_by_initiators(initiator_list)]
-        lun_maps = self.zapi_client.get_lun_map(path)
+        # Map igroup-name to lun-id, but only for the requested initiators.
-        if initiator_igroups and lun_maps:
+        lun_map = {v['initiator-group']: v['lun-id']
-            for igroup in initiator_igroups:
+                   for v in self.zapi_client.get_lun_map(path)
-                igroup_name = igroup['initiator-group-name']
+                   if v['initiator-group'] in igroups}
-                if igroup_name.startswith(na_utils.OPENSTACK_PREFIX):
+
-                    for lun_map in lun_maps:
+        igroup_name = lun_id = None
-                        if lun_map['initiator-group'] == igroup_name:
+        # Give preference to OpenStack igroups, just use the last one if not
-                            return igroup_name, lun_map['lun-id']
+        # present to allow unmapping old mappings that used a custom igroup.
-        return None, None
+        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,

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