From ffa12c98971e35056a7867cd61ba5a39630cc0a7 Mon Sep 17 00:00:00 2001 From: Mark Sturdevant Date: Tue, 27 May 2014 14:49:34 -0700 Subject: [PATCH] 3PAR volume detach with host in a host set When deleting the last VLUN for a host, try to delete the host. If the host is in a host set this will fail. When that happens, log a nice message including the reason, but don't fail or hang the delete VLUN. Leave hosts in host sets. Closes-Bug: #1317134 Change-Id: Ie3f8f94c05a6a318de866598f9e9c4c5d84926f4 --- cinder/tests/fake_hp_client_exceptions.py | 17 +++-- cinder/tests/test_hp3par.py | 48 ++++++++++-- .../volume/drivers/san/hp/hp_3par_common.py | 74 ++++++++++++++----- 3 files changed, 108 insertions(+), 31 deletions(-) diff --git a/cinder/tests/fake_hp_client_exceptions.py b/cinder/tests/fake_hp_client_exceptions.py index 295b710ca..07b5baf8c 100644 --- a/cinder/tests/fake_hp_client_exceptions.py +++ b/cinder/tests/fake_hp_client_exceptions.py @@ -20,6 +20,13 @@ class HTTPConflict(Exception): http_status = 409 message = "Conflict" + def __init__(self, error=None): + if error and 'message' in error: + self._error_desc = error['message'] + + def get_description(self): + return self._error_desc + class HTTPNotFound(Exception): http_status = 404 @@ -31,9 +38,8 @@ class HTTPForbidden(Exception): message = "Forbidden" def __init__(self, error=None): - if error: - if 'code' in error: - self._error_code = error['code'] + if error and 'code' in error: + self._error_code = error['code'] def get_code(self): return self._error_code @@ -49,9 +55,8 @@ class HTTPServerError(Exception): message = "Error" def __init__(self, error=None): - if error: - if 'message' in error: - self._error_desc = error['message'] + if error and 'message' in error: + self._error_desc = error['message'] def get_description(self): return self._error_desc diff --git a/cinder/tests/test_hp3par.py b/cinder/tests/test_hp3par.py index ce1018145..561945d55 100644 --- a/cinder/tests/test_hp3par.py +++ b/cinder/tests/test_hp3par.py @@ -1047,11 +1047,6 @@ class TestHP3PARFCDriver(HP3PARBaseDriver, test.TestCase): 'volumeName': self.VOLUME_3PAR_NAME, 'lun': None, 'type': 0}] - self.driver.terminate_connection( - self.volume, - self.connector, - force=True) - expected = [ mock.call.login(HP3PAR_USER_NAME, HP3PAR_USER_PASS), mock.call.getHostVLUNs(self.FAKE_HOST), @@ -1063,8 +1058,51 @@ class TestHP3PARFCDriver(HP3PARBaseDriver, test.TestCase): mock.call.getPorts(), mock.call.logout()] + self.driver.terminate_connection(self.volume, self.connector) + mock_client.assert_has_calls(expected) + mock_client.reset_mock() + + # mock some deleteHost exceptions that are handled + delete_with_vlun = hpexceptions.HTTPConflict( + error={'message': "has exported VLUN"}) + delete_with_hostset = hpexceptions.HTTPConflict( + error={'message': "host is a member of a set"}) + mock_client.deleteHost = mock.Mock( + side_effect=[delete_with_vlun, delete_with_hostset]) + + self.driver.terminate_connection(self.volume, self.connector) + mock_client.assert_has_calls(expected) + mock_client.reset_mock() + + self.driver.terminate_connection(self.volume, self.connector) mock_client.assert_has_calls(expected) + def test_terminate_connection_more_vols(self): + mock_client = self.setup_driver() + # mock more than one vlun on the host (don't even try to remove host) + mock_client.getHostVLUNs.return_value = \ + [ + {'active': True, + 'volumeName': self.VOLUME_3PAR_NAME, + 'lun': None, 'type': 0}, + {'active': True, + 'volumeName': 'there-is-another-volume', + 'lun': None, 'type': 0}, + ] + + expect_less = [ + mock.call.login(HP3PAR_USER_NAME, HP3PAR_USER_PASS), + mock.call.getHostVLUNs(self.FAKE_HOST), + mock.call.deleteVLUN( + self.VOLUME_3PAR_NAME, + None, + self.FAKE_HOST), + mock.call.getPorts(), + mock.call.logout()] + + self.driver.terminate_connection(self.volume, self.connector) + mock_client.assert_has_calls(expect_less) + def test_get_volume_stats(self): # setup_mock_client drive with default configuration # and return the mock HTTP 3PAR client diff --git a/cinder/volume/drivers/san/hp/hp_3par_common.py b/cinder/volume/drivers/san/hp/hp_3par_common.py index c9229fb57..50510e38b 100644 --- a/cinder/volume/drivers/san/hp/hp_3par_common.py +++ b/cinder/volume/drivers/san/hp/hp_3par_common.py @@ -128,10 +128,11 @@ class HP3PARCommon(object): 2.0.9 - Remove unused 3PAR driver method bug #1310807 2.0.10 - Fixed an issue with 3PAR vlun location bug #1315542 2.0.11 - Remove hp3parclient requirement from unit tests #1315195 + 2.0.12 - Volume detach hangs when host is in a host set bug #1317134 """ - VERSION = "2.0.11" + VERSION = "2.0.12" stats = {} @@ -492,7 +493,7 @@ class HP3PARCommon(object): if found_vlun is None: msg = (_("3PAR vlun %(name)s not found on host %(host)s") % {'name': volume_name, 'host': hostname}) - LOG.warn(msg) + LOG.info(msg) return found_vlun def create_vlun(self, volume, host, nsp=None): @@ -506,26 +507,59 @@ class HP3PARCommon(object): def delete_vlun(self, volume, hostname): volume_name = self._get_3par_vol_name(volume['id']) - vlun = self._get_vlun(volume_name, hostname) + vluns = self.client.getHostVLUNs(hostname) - if vlun is not None: - # VLUN Type of MATCHED_SET 4 requires the port to be provided - if self.VLUN_TYPE_MATCHED_SET == vlun['type']: - self.client.deleteVLUN(volume_name, vlun['lun'], hostname, - vlun['portPos']) - else: - self.client.deleteVLUN(volume_name, vlun['lun'], hostname) + for vlun in vluns: + if volume_name in vlun['volumeName']: + break + else: + msg = ( + _("3PAR vlun for volume %(name)s not found on host %(host)s") % + {'name': volume_name, 'host': hostname}) + LOG.info(msg) + return - try: - self._delete_3par_host(hostname) - self._remove_hosts_naming_dict_host(hostname) - except hpexceptions.HTTPConflict as ex: - # host will only be removed after all vluns - # have been removed - if 'has exported VLUN' in ex.get_description(): - pass - else: - raise + # VLUN Type of MATCHED_SET 4 requires the port to be provided + if self.VLUN_TYPE_MATCHED_SET == vlun['type']: + self.client.deleteVLUN(volume_name, vlun['lun'], hostname, + vlun['portPos']) + else: + self.client.deleteVLUN(volume_name, vlun['lun'], hostname) + + # Determine if there are other volumes attached to the host. + # This will determine whether we should try removing host from host set + # and deleting the host. + for vlun in vluns: + if volume_name not in vlun['volumeName']: + # Found another volume + break + else: + # We deleted the last vlun, so try to delete the host too. + # This check avoids the old unnecessary try/fail when vluns exist + # but adds a minor race condition if a vlun is manually deleted + # externally at precisely the wrong time. Worst case is leftover + # host, so it is worth the unlikely risk. + + try: + self._delete_3par_host(hostname) + self._remove_hosts_naming_dict_host(hostname) + except Exception as ex: + # Any exception down here is only logged. The vlun is deleted. + + # If the host is in a host set, the delete host will fail and + # the host will remain in the host set. This is desired + # because cinder was not responsible for the host set + # assignment. The host set could be used outside of cinder + # for future needs (e.g. export volume to host set). + + # The log info explains why the host was left alone. + msg = (_("3PAR vlun for volume '%(name)s' was deleted, " + "but the host '%(host)s' was not deleted because: " + "%(reason)s") % + {'name': volume_name, + 'host': hostname, + 'reason': ex.get_description()}) + LOG.info(msg) def _remove_hosts_naming_dict_host(self, hostname): items = self.hosts_naming_dict.items() -- 2.45.2