Unity: Fix duplicate hosts created with same name (#10)

In some circumstance, there is a race condition in querying/creating
Unity host thus multiple hosts with same name were created.

This issue stops any further attach/detach operation.

This patch leverage the DSL in cinder to avoid above race condition.

Change-Id: I3b775da8c5e862def54e9d44d7b157cf17a07a24
Closes-bug: #1726284
(cherry picked from commit d8dd30f)
(cherry picked from commit 85bc7559fb3b8c8ff3d508b3ef54bc4ba4a0591f)

Author: peter-wangxu

cinder/tests/unit/volume/drivers/dell_emc/unity/test_adapter.py

@@ -127,11 +127,7 @@ def delete_snap(snap):
             raise ex.SnapDeleteIsCalled()
 
     @staticmethod
-    def create_host(name, uids):
-        return test_client.MockResource(name=name)
-
-    @staticmethod
-    def get_host(name):
+    def create_host(name):
         return test_client.MockResource(name=name)
 
     @staticmethod
@@ -188,6 +184,10 @@ def thin_clone(obj, name, io_limit_policy, description, new_size_gb):
         else:
             raise ex.UnityThinCloneLimitExceededError
 
+    @staticmethod
+    def update_host_initiators(host, wwns):
+        return None
+
     @property
     def system(self):
         return self._system

cinder/tests/unit/volume/drivers/dell_emc/unity/test_client.py

@@ -12,17 +12,16 @@
 # WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
 # License for the specific language governing permissions and limitations
 # under the License.
-
 import unittest
 
 from mock import mock
 from oslo_utils import units
 
+from cinder import coordination
 from cinder.tests.unit.volume.drivers.dell_emc.unity \
     import fake_exception as ex
 from cinder.volume.drivers.dell_emc.unity import client
 
-
 ########################
 #
 #   Start of Mocks
@@ -48,6 +47,7 @@ def __init__(self, name=None, _id=None):
         self.max_kbps = None
         self.pool_name = 'Pool0'
         self._storage_resource = None
+        self.host_cache = []
 
     @property
     def id(self):
@@ -250,6 +250,10 @@ def create_host(name):
     def get_host(name):
         if name == 'not_found':
             raise ex.UnityResourceNotFoundError()
+        if name == 'host1':
+            ret = MockResource(name)
+            ret.initiator_id = ['old-iqn']
+            return ret
         return MockResource(name)
 
     @staticmethod
@@ -410,16 +414,18 @@ def test_get_snap_not_found(self):
         ret = self.client.get_snap('not_found')
         self.assertIsNone(ret)
 
-    def test_create_host_found(self):
-        iqns = ['iqn.1-1.com.e:c.a.a0']
-        host = self.client.create_host('host1', iqns)
+    @mock.patch.object(coordination.Coordinator, 'get_lock')
+    def test_create_host_found(self, fake_coordination):
+        host = self.client.create_host('host1')
 
         self.assertEqual('host1', host.name)
         self.assertLessEqual(['iqn.1-1.com.e:c.a.a0'], host.initiator_id)
 
-    def test_create_host_not_found(self):
-        host = self.client.create_host('not_found', [])
+    @mock.patch.object(coordination.Coordinator, 'get_lock')
+    def test_create_host_not_found(self, fake):
+        host = self.client.create_host('not_found')
         self.assertEqual('not_found', host.name)
+        self.assertIn('not_found', self.client.host_cache)
 
     def test_attach_lun(self):
         lun = MockResource(_id='lun1', name='l1')
@@ -440,8 +446,20 @@ def f():
 
         self.assertRaises(ex.DetachIsCalled, f)
 
-    def test_get_host(self):
-        self.assertEqual('host2', self.client.get_host('host2').name)
+    @mock.patch.object(coordination.Coordinator, 'get_lock')
+    def test_create_host(self, fake):
+        self.assertEqual('host2', self.client.create_host('host2').name)
+
+    @mock.patch.object(coordination.Coordinator, 'get_lock')
+    def test_create_host_in_cache(self, fake):
+        self.client.host_cache['already_in'] = MockResource(name='already_in')
+        host = self.client.create_host('already_in')
+        self.assertIn('already_in', self.client.host_cache)
+        self.assertEqual('already_in', host.name)
+
+    def test_update_host_initiators(self):
+        host = MockResource(name='host_init')
+        host = self.client.update_host_initiators(host, 'fake-iqn-1')
 
     def test_get_iscsi_target_info(self):
         ret = self.client.get_iscsi_target_info()

cinder/volume/drivers/dell_emc/unity/adapter.py

@@ -287,8 +287,9 @@ def delete_volume(self, volume):
 
     @cinder_utils.trace
     def _initialize_connection(self, lun_or_snap, connector, vol_id):
-        host = self.client.create_host(connector['host'],
-                                       self.get_connector_uids(connector))
+        host = self.client.create_host(connector['host'])
+        self.client.update_host_initiators(
+            host, self.get_connector_uids(connector))
         hlu = self.client.attach(host, lun_or_snap)
         data = self.get_connection_info(hlu, host, connector)
         data['target_discovered'] = True
@@ -308,7 +309,7 @@ def initialize_connection(self, volume, connector):
 
     @cinder_utils.trace
     def _terminate_connection(self, lun_or_snap, connector):
-        host = self.client.get_host(connector['host'])
+        host = self.client.create_host(connector['host'])
         self.client.detach(host, lun_or_snap)
 
     @cinder_utils.trace
@@ -565,8 +566,8 @@ def _thin_clone(self, vol_params, src_snap, src_lun=None):
                 io_limit_policy=vol_params.io_limit_policy,
                 new_size_gb=vol_params.size)
         except storops_ex.UnityThinCloneLimitExceededError:
-            LOG.info('Number of thin clones of base LUN exceeds system '
-                     'limit, dd-copy a new one and thin clone from it.')
+            LOG.info(_LI('Number of thin clones of base LUN exceeds system '
+                         'limit, dd-copy a new one and thin clone from it.'))
             # Copy via dd if thin clone meets the system limit
             hidden = copy.copy(vol_params)
             hidden.name = 'hidden-%s' % vol_params.name
@@ -738,7 +739,7 @@ def _terminate_connection(self, lun_or_snap, connector):
                 'driver_volume_type': self.driver_volume_type,
                 'data': {}
             }
-            host = self.client.get_host(connector['host'])
+            host = self.client.create_host(connector['host'])
             if len(host.host_luns) == 0:
                 targets = self.client.get_fc_target_info(
                     logged_in_only=True, allowed_ports=self.allowed_ports)

cinder/volume/drivers/dell_emc/unity/client.py

@@ -24,11 +24,11 @@
     # Set storops_ex to be None for unit test
     storops_ex = None
 
+from cinder import coordination
 from cinder import exception
 from cinder.i18n import _, _LW
 from cinder.volume.drivers.dell_emc.unity import utils
 
-
 LOG = log.getLogger(__name__)
 
 
@@ -43,6 +43,7 @@ def __init__(self, host, username, password, verify_cert=True):
         self.username = username
         self.password = password
         self.verify_cert = verify_cert
+        self.host_cache = {}
 
     @property
     def system(self):
@@ -186,28 +187,40 @@ def get_snap(self, name=None):
                 {'name': name, 'err': err})
         return None
 
-    def create_host(self, name, uids):
-        """Creates a host on Unity.
-
-        Creates a host on Unity which has the uids associated.
-
-        :param name: name of the host
-        :param uids: iqns or wwns list
-        :return: UnitHost object
-        """
+    @coordination.synchronized('{self.host}-{name}')
+    def create_host(self, name):
+        """Provides existing host if exists else create one."""
+        if name not in self.host_cache:
+            try:
+                host = self.system.get_host(name=name)
+            except storops_ex.UnityResourceNotFoundError:
+                LOG.debug('Host %s not found.  Create a new one.',
+                          name)
+                host = self.system.create_host(name=name)
 
-        try:
-            host = self.system.get_host(name=name)
-        except storops_ex.UnityResourceNotFoundError:
-            LOG.debug('Existing host %s not found.  Create a new one.', name)
-            host = self.system.create_host(name=name)
+            self.host_cache[name] = host
+        else:
+            host = self.host_cache[name]
+        return host
 
+    def update_host_initiators(self, host, uids):
+        """Updates host with the supplied uids."""
         host_initiators_ids = self.get_host_initiator_ids(host)
         un_registered = [h for h in uids if h not in host_initiators_ids]
-        for uid in un_registered:
-            host.add_initiator(uid, force_create=True)
+        if un_registered:
+            for uid in un_registered:
+                try:
+                    host.add_initiator(uid, force_create=True)
+                except storops_ex.UnityHostInitiatorExistedError:
+                    # This make concurrent modification of
+                    # host initiators safe
+                    LOG.debug(
+                        'The uid(%s) was already in '
+                        '%s.', uid, host.name)
+            host.update()
+            # Update host cached with new initiators.
+            self.host_cache[host.name] = host
 
-        host.update()
         return host
 
     @staticmethod
@@ -241,9 +254,6 @@ def detach(host, lun_or_snap):
         lun_or_snap.update()
         host.detach(lun_or_snap)
 
-    def get_host(self, name):
-        return self.system.get_host(name=name)
-
     def get_ethernet_ports(self):
         return self.system.get_ethernet_port()