From 8185d1b5db421441ebba128f23f840a42f9bf050 Mon Sep 17 00:00:00 2001 From: Avishay Traeger Date: Tue, 26 Nov 2013 21:10:22 +0200 Subject: [PATCH] Fix Storwize terminate_connection with no host Nova may pass a connector to Cinder with no 'host' field, which was causing a KeyError in the Storwize driver. This patch resolves this case by doing the following: 1. If the volume is mapped to only 1 host, unmap it 2. If the volume was not mapped or mapped to multiple hosts, print a warning but don't raise an exception Change-Id: I0ec1c24adbdfbcf1c4868b4981a2e2618d4b411c Closes-Bug: #1244257 --- cinder/tests/test_storwize_svc.py | 48 ++++++++++++++-- cinder/volume/drivers/storwize_svc.py | 81 +++++++++++++++++++-------- 2 files changed, 101 insertions(+), 28 deletions(-) diff --git a/cinder/tests/test_storwize_svc.py b/cinder/tests/test_storwize_svc.py index 24a219d21..bf48525fe 100644 --- a/cinder/tests/test_storwize_svc.py +++ b/cinder/tests/test_storwize_svc.py @@ -975,7 +975,7 @@ port_speed!N/A del self._mappings_list[this_mapping] return ('', '') - # List information about vdisk-host mappings + # List information about host->vdisk mappings def _cmd_lshostvdiskmap(self, **kwargs): index = 1 no_hdr = 0 @@ -998,6 +998,32 @@ port_speed!N/A return self._print_info_cmd(rows=rows, **kwargs) + # List information about vdisk->host mappings + def _cmd_lsvdiskhostmap(self, **kwargs): + mappings_found = 0 + vdisk_name = kwargs['obj'] + + if vdisk_name not in self._volumes_list: + return self._errors['CMMVC5753E'] + + rows = [] + rows.append(['id name', 'SCSI_id', 'host_id', 'host_name', 'vdisk_UID', + 'IO_group_id', 'IO_group_name']) + + for k, mapping in self._mappings_list.iteritems(): + if (mapping['vol'] == vdisk_name): + mappings_found += 1 + volume = self._volumes_list[mapping['vol']] + host = self._hosts_list[mapping['host']] + rows.append([volume['id'], volume['name'], host['id'], + host['host_name'], volume['uid'], + volume['IO_group_id'], volume['IO_group_name']]) + + if mappings_found: + return self._print_info_cmd(rows=rows, **kwargs) + else: + return ('', '') + # Create a FlashCopy mapping def _cmd_mkfcmap(self, **kwargs): source = '' @@ -1369,6 +1395,8 @@ port_speed!N/A out, err = self._cmd_rmvdiskhostmap(**kwargs) elif command == 'lshostvdiskmap': out, err = self._cmd_lshostvdiskmap(**kwargs) + elif command == 'lsvdiskhostmap': + out, err = self._cmd_lsvdiskhostmap(**kwargs) elif command == 'mkfcmap': out, err = self._cmd_mkfcmap(**kwargs) elif command == 'prestartfcmap': @@ -1998,8 +2026,10 @@ class StorwizeSVCDriverTestCase(test.TestCase): # Try to remove connection from volume that isn't mapped (should print # message but NOT fail) - vol_no_exist = {'name': 'i_dont_exist'} - self.driver.terminate_connection(vol_no_exist, self._connector) + unmapped_vol = self._generate_vol_info(None, None) + self.driver.create_volume(unmapped_vol) + self.driver.terminate_connection(unmapped_vol, self._connector) + self.driver.delete_volume(unmapped_vol) # Remove the mapping from the 1st volume and delete it self.driver.terminate_connection(volume1, self._connector) @@ -2010,9 +2040,19 @@ class StorwizeSVCDriverTestCase(test.TestCase): host_name = self.driver._get_host_from_connector(self._connector) self.assertIsNotNone(host_name) - # Remove the mapping from the 2nd volume and delete it. The host should + # Remove the mapping from the 2nd volume. The host should # be automatically removed because there are no more mappings. self.driver.terminate_connection(volume2, self._connector) + + # Check if we successfully terminate connections when the host is not + # specified (see bug #1244257) + fake_conn = {'ip': '127.0.0.1', 'initiator': 'iqn.fake'} + self.driver.initialize_connection(volume2, self._connector) + host_name = self.driver._get_host_from_connector(self._connector) + self.assertIsNotNone(host_name) + self.driver.terminate_connection(volume2, fake_conn) + host_name = self.driver._get_host_from_connector(self._connector) + self.assertIsNone(host_name) self.driver.delete_volume(volume2) self._assert_vol_exists(volume2['name'], False) diff --git a/cinder/volume/drivers/storwize_svc.py b/cinder/volume/drivers/storwize_svc.py index d03da19c5..cca77b9d5 100644 --- a/cinder/volume/drivers/storwize_svc.py +++ b/cinder/volume/drivers/storwize_svc.py @@ -551,8 +551,7 @@ class StorwizeSVCDriver(san.SanDriver): """ - prefix = self._connector_to_hostname_prefix(connector) - LOG.debug(_('enter: _get_host_from_connector: prefix %s') % prefix) + LOG.debug(_('enter: _get_host_from_connector: %s') % str(connector)) # Get list of host in the storage ssh_cmd = ['svcinfo', 'lshost', '-delim', '!'] @@ -627,6 +626,22 @@ class StorwizeSVCDriver(san.SanDriver): {'host': connector['host'], 'host_name': host_name}) return host_name + def _get_vdiskhost_mappings(self, vdisk_name): + """Return the defined storage mappings for a vdisk.""" + + return_data = {} + ssh_cmd = ['svcinfo', 'lsvdiskhostmap', '-delim', '!', vdisk_name] + out, err = self._run_ssh(ssh_cmd) + + mappings = out.strip().split('\n') + if len(mappings): + header = mappings.pop(0) + for mapping_line in mappings: + mapping_data = self._get_hdr_dic(header, mapping_line, '!') + return_data[mapping_data['host_name']] = mapping_data + + return return_data + def _get_hostvdisk_mappings(self, host_name): """Return the defined storage mappings for a host.""" @@ -890,31 +905,49 @@ class StorwizeSVCDriver(san.SanDriver): 'conn': str(connector)}) vol_name = volume['name'] - host_name = self._get_host_from_connector(connector) - # Verify that _get_host_from_connector returned the host. - # This should always succeed as we terminate an existing connection. - self._driver_assert( - host_name is not None, - _('_get_host_from_connector failed to return the host name ' - 'for connector')) - - # Check if vdisk-host mapping exists, remove if it does - mapping_data = self._get_hostvdisk_mappings(host_name) - if vol_name in mapping_data: - ssh_cmd = ['svctask', 'rmvdiskhostmap', '-host', host_name, - vol_name] - out, err = self._run_ssh(ssh_cmd) - # Verify CLI behaviour - no output is returned from - # rmvdiskhostmap - self._assert_ssh_return(len(out.strip()) == 0, - 'terminate_connection', ssh_cmd, out, err) - del mapping_data[vol_name] + if 'host' in connector: + host_name = self._get_host_from_connector(connector) + self._driver_assert( + host_name is not None, + _('_get_host_from_connector failed to return the host name ' + 'for connector')) else: - LOG.error(_('terminate_connection: No mapping of volume ' - '%(vol_name)s to host %(host_name)s found') % - {'vol_name': vol_name, 'host_name': host_name}) + # See bug #1244257 + host_name = None + + # Check if vdisk-host mapping exists, remove if it does. If no host + # name was given, but only one mapping exists, we can use that. + mapping_data = self._get_vdiskhost_mappings(vol_name) + if len(mapping_data) == 0: + LOG.warning(_('terminate_connection: No mapping of volume ' + '%(vol_name)s to any host found.') % + {'vol_name': vol_name}) + return + if host_name is None: + if len(mapping_data) > 1: + LOG.warning(_('terminate_connection: Multiple mappings of ' + 'volume %(vol_name)s found, no host ' + 'specified.') % {'vol_name': vol_name}) + return + else: + host_name = mapping_data.keys()[0] + else: + if host_name not in mapping_data: + LOG.error(_('terminate_connection: No mapping of volume ' + '%(vol_name)s to host %(host_name)s found') % + {'vol_name': vol_name, 'host_name': host_name}) + return + + # We have a valid host_name now + ssh_cmd = ['svctask', 'rmvdiskhostmap', '-host', host_name, + vol_name] + out, err = self._run_ssh(ssh_cmd) + # Verify CLI behaviour - no output is returned from rmvdiskhostmap + self._assert_ssh_return(len(out.strip()) == 0, + 'terminate_connection', ssh_cmd, out, err) # If this host has no more mappings, delete it + mapping_data = self._get_hostvdisk_mappings(host_name) if not mapping_data: self._delete_host(host_name) -- 2.45.2