From 1e1a935185029657be26d6d9be59e38f396a2f96 Mon Sep 17 00:00:00 2001 From: Peter Wang Date: Thu, 12 Oct 2017 17:59:16 +0800 Subject: [PATCH] Unity: Fix duplicate hosts created with same name 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 d8dd30f16b8712385b2f0d43c4d0a45f5fe4be41) (cherry picked from commit 85bc7559fb3b8c8ff3d508b3ef54bc4ba4a0591f) --- .../drivers/dell_emc/unity/test_adapter.py | 10 ++-- .../drivers/dell_emc/unity/test_client.py | 36 +++++++++---- .../volume/drivers/dell_emc/unity/adapter.py | 13 ++--- .../volume/drivers/dell_emc/unity/client.py | 52 +++++++++++-------- 4 files changed, 70 insertions(+), 41 deletions(-) diff --git a/cinder/tests/unit/volume/drivers/dell_emc/unity/test_adapter.py b/cinder/tests/unit/volume/drivers/dell_emc/unity/test_adapter.py index dacca2eb2c..7bcac64c17 100644 --- a/cinder/tests/unit/volume/drivers/dell_emc/unity/test_adapter.py +++ b/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 diff --git a/cinder/tests/unit/volume/drivers/dell_emc/unity/test_client.py b/cinder/tests/unit/volume/drivers/dell_emc/unity/test_client.py index fc7548285c..d12bcc5d39 100644 --- a/cinder/tests/unit/volume/drivers/dell_emc/unity/test_client.py +++ b/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() diff --git a/cinder/volume/drivers/dell_emc/unity/adapter.py b/cinder/volume/drivers/dell_emc/unity/adapter.py index 3ea612410b..596a4d7472 100644 --- a/cinder/volume/drivers/dell_emc/unity/adapter.py +++ b/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) diff --git a/cinder/volume/drivers/dell_emc/unity/client.py b/cinder/volume/drivers/dell_emc/unity/client.py index b9c0197a60..b080152a6c 100644 --- a/cinder/volume/drivers/dell_emc/unity/client.py +++ b/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()