From 4ee404466d3f2ced8bfdfb18927ef19a27967952 Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Wed, 8 Nov 2017 21:03:08 +0100 Subject: [PATCH] Fixing FC scanning Current FC tries to limit the scanning range by detecting the target and channel, unfortunately this code has a good number of implementation issues: - Matching uses local WWNN instead of target's WWPN. - Not using a shell to run the command, so the * glob won't expand. - Not using -l on grep command to list file names instead of contents. - Not making the search case insensitive. This patch fixes all these issues by using the target's WWPNs instead -taking into account FC Zone/Access control information if present- and supporting both possible connection information formats for the WWPNs (single value or list of values). Rescan tests have been modified to adhere to unit tests best practices, where each test case only tests the specific code in the method under test and mocks everything else. Closes-Bug: #1664653 Closes-Bug: #1684996 Closes-Bug: #1687607 Change-Id: Ib539f6a3652bab4399c30cd90f326829e839ec02 --- .../initiator/connectors/fibre_channel.py | 3 +- os_brick/initiator/linuxfc.py | 33 +++- os_brick/tests/initiator/test_linuxfc.py | 176 ++++++++++++------ 3 files changed, 141 insertions(+), 71 deletions(-) diff --git a/os_brick/initiator/connectors/fibre_channel.py b/os_brick/initiator/connectors/fibre_channel.py index fd5afff12..4e2448f3e 100644 --- a/os_brick/initiator/connectors/fibre_channel.py +++ b/os_brick/initiator/connectors/fibre_channel.py @@ -157,8 +157,7 @@ class FibreChannelConnector(base.BaseLinuxConnector): "Will rescan & retry. Try number: %(tries)s.", {'tries': self.tries}) - self._linuxfc.rescan_hosts(hbas, - connection_properties['target_lun']) + self._linuxfc.rescan_hosts(hbas, connection_properties) self.tries = self.tries + 1 self.host_device = None diff --git a/os_brick/initiator/linuxfc.py b/os_brick/initiator/linuxfc.py index 8954bca75..0cb1c69dd 100644 --- a/os_brick/initiator/linuxfc.py +++ b/os_brick/initiator/linuxfc.py @@ -19,6 +19,7 @@ import os from oslo_concurrency import processutils as putils from oslo_log import log as logging +import six from os_brick.initiator import linuxscsi @@ -34,7 +35,7 @@ class LinuxFibreChannel(linuxscsi.LinuxSCSI): else: return False - def _get_hba_channel_scsi_target(self, hba): + def _get_hba_channel_scsi_target(self, hba, conn_props): """Try to get the HBA channel and SCSI target for an HBA. This method only works for Fibre Channel targets that implement a @@ -43,28 +44,44 @@ class LinuxFibreChannel(linuxscsi.LinuxSCSI): :returns: List or None """ + # We want the target's WWPNs, so we use the initiator_target_map if + # present for this hba or default to target_wwns if not present. + wwpns = conn_props['target_wwn'] + if 'initiator_target_map' in conn_props: + wwpns = conn_props['initiator_target_map'].get(hba['port_name'], + wwpns) + + # If it's not a string then it's an iterable (most likely a list), + # so we need to create a BRE for the grep query. + if not isinstance(wwpns, six.string_types): + wwpns = '\|'.join(wwpns) + # Leave only the number from the host_device field (ie: host6) host_device = hba['host_device'] if host_device and len(host_device) > 4: host_device = host_device[4:] path = '/sys/class/fc_transport/target%s:' % host_device - cmd = 'grep %(wwnn)s %(path)s*/node_name' % {'wwnn': hba['node_name'], - 'path': path} + # Since we'll run the command in a shell ensure BRE are being used + cmd = 'grep -Gil "%(wwpns)s" %(path)s*/port_name' % {'wwpns': wwpns, + 'path': path} try: - out, _err = self._execute(cmd) + # We need to run command in shell to expand the * glob + out, _err = self._execute(cmd, shell=True) return [line.split('/')[4].split(':')[1:] for line in out.split('\n') if line.startswith(path)] except Exception as exc: LOG.debug('Could not get HBA channel and SCSI target ID, path: ' - '%(path)s, reason: %(reason)s', {'path': path, - 'reason': exc}) + '%(path)s*, reason: %(reason)s', {'path': path, + 'reason': exc}) return None - def rescan_hosts(self, hbas, target_lun): + def rescan_hosts(self, hbas, connection_properties): + target_lun = connection_properties['target_lun'] + for hba in hbas: # Try to get HBA channel and SCSI target to use as filters - cts = self._get_hba_channel_scsi_target(hba) + cts = self._get_hba_channel_scsi_target(hba, connection_properties) # If we couldn't get the channel and target use wildcards if not cts: cts = [('-', '-')] diff --git a/os_brick/tests/initiator/test_linuxfc.py b/os_brick/tests/initiator/test_linuxfc.py index c455edf3b..a5ccc5203 100644 --- a/os_brick/tests/initiator/test_linuxfc.py +++ b/os_brick/tests/initiator/test_linuxfc.py @@ -44,87 +44,141 @@ class LinuxFCTestCase(base.TestCase): has_fc = self.lfc.has_fc_support() self.assertTrue(has_fc) - def test_rescan_hosts(self): - # We check that we try to get the HBA channel and SCSI target - execute_results = ( - ('/sys/class/fc_transport/target10:2:3/node_name:' - '0x5006016090203181\n/sys/class/fc_transport/target10:4:5/' - 'node_name:0x5006016090203181', ''), - None, - None, - ('/sys/class/fc_transport/target11:6:7/node_name:' - '0x5006016090203181\n/sys/class/fc_transport/target11:8:9/' - 'node_name:0x5006016090203181', ''), - None, - None) - hbas = [{'host_device': 'host10', 'node_name': '5006016090203181'}, - {'host_device': 'host11', 'node_name': '5006016090203181'}] + @staticmethod + def __get_rescan_info(zone_manager=False): + + connection_properties = { + 'initiator_target_map': {'50014380186af83c': ['514f0c50023f6c00'], + '50014380186af83e': ['514f0c50023f6c01']}, + 'target_discovered': False, + 'target_lun': 1, + 'target_wwn': ['514f0c50023f6c00', '514f0c50023f6c01'] + } + + hbas = [ + {'device_path': ('/sys/devices/pci0000:00/0000:00:02.0/' + '0000:04:00.0/host6/fc_host/host6'), + 'host_device': 'host6', + 'node_name': '50014380186af83d', + 'port_name': '50014380186af83c'}, + {'device_path': ('/sys/devices/pci0000:00/0000:00:02.0/' + '0000:04:00.1/host7/fc_host/host7'), + 'host_device': 'host7', + 'node_name': '50014380186af83f', + 'port_name': '50014380186af83e'}, + ] + if not zone_manager: + del connection_properties['initiator_target_map'] + return hbas, connection_properties + + def test__get_hba_channel_scsi_target_single_wwpn(self): + execute_results = ('/sys/class/fc_transport/target6:0:1/port_name\n', + '') + hbas, con_props = self.__get_rescan_info() + con_props['target_wwn'] = con_props['target_wwn'][0] with mock.patch.object(self.lfc, '_execute', - side_effect=execute_results) as execute_mock: - self.lfc.rescan_hosts(hbas, 1) + return_value=execute_results) as execute_mock: + res = self.lfc._get_hba_channel_scsi_target(hbas[0], con_props) + execute_mock.assert_called_once_with( + 'grep -Gil "514f0c50023f6c00" ' + '/sys/class/fc_transport/target6:*/port_name', + shell=True) + expected = [['0', '1']] + self.assertListEqual(expected, res) + + def test__get_hba_channel_scsi_target_multiple_wwpn(self): + execute_results = ('/sys/class/fc_transport/target6:0:1/port_name\n' + '/sys/class/fc_transport/target6:0:2/port_name\n', + '') + hbas, con_props = self.__get_rescan_info() + with mock.patch.object(self.lfc, '_execute', + return_value=execute_results) as execute_mock: + res = self.lfc._get_hba_channel_scsi_target(hbas[0], con_props) + execute_mock.assert_called_once_with( + 'grep -Gil "514f0c50023f6c00\|514f0c50023f6c01" ' + '/sys/class/fc_transport/target6:*/port_name', + shell=True) + expected = [['0', '1'], ['0', '2']] + self.assertListEqual(expected, res) + + def test__get_hba_channel_scsi_target_zone_manager(self): + execute_results = ('/sys/class/fc_transport/target6:0:1/port_name\n', + '') + hbas, con_props = self.__get_rescan_info(zone_manager=True) + with mock.patch.object(self.lfc, '_execute', + return_value=execute_results) as execute_mock: + res = self.lfc._get_hba_channel_scsi_target(hbas[0], con_props) + execute_mock.assert_called_once_with( + 'grep -Gil "514f0c50023f6c00" ' + '/sys/class/fc_transport/target6:*/port_name', + shell=True) + expected = [['0', '1']] + self.assertListEqual(expected, res) + + def test__get_hba_channel_scsi_target_not_found(self): + hbas, con_props = self.__get_rescan_info(zone_manager=True) + with mock.patch.object(self.lfc, '_execute', + return_value=('', '')) as execute_mock: + res = self.lfc._get_hba_channel_scsi_target(hbas[0], con_props) + execute_mock.assert_called_once_with( + 'grep -Gil "514f0c50023f6c00" ' + '/sys/class/fc_transport/target6:*/port_name', + shell=True) + self.assertListEqual([], res) + + def test__get_hba_channel_scsi_target_exception(self): + hbas, con_props = self.__get_rescan_info(zone_manager=True) + with mock.patch.object(self.lfc, '_execute', + side_effect=Exception) as execute_mock: + res = self.lfc._get_hba_channel_scsi_target(hbas[0], con_props) + execute_mock.assert_called_once_with( + 'grep -Gil "514f0c50023f6c00" ' + '/sys/class/fc_transport/target6:*/port_name', + shell=True) + self.assertIsNone(res) + + def test_rescan_hosts(self): + get_chan_results = [[['2', '3'], ['4', '5']], [['6', '7']]] + + hbas, con_props = self.__get_rescan_info(zone_manager=True) + with mock.patch.object(self.lfc, '_execute', + return_value=None) as execute_mock, \ + mock.patch.object(self.lfc, '_get_hba_channel_scsi_target', + side_effect=get_chan_results) as mock_get_chan: + + self.lfc.rescan_hosts(hbas, con_props) expected_commands = [ - mock.call('grep 5006016090203181 /sys/class/fc_transport/' - 'target10:*/node_name'), - mock.call('tee', '-a', '/sys/class/scsi_host/host10/scan', + mock.call('tee', '-a', '/sys/class/scsi_host/host6/scan', process_input='2 3 1', root_helper=None, run_as_root=True), - mock.call('tee', '-a', '/sys/class/scsi_host/host10/scan', + mock.call('tee', '-a', '/sys/class/scsi_host/host6/scan', process_input='4 5 1', root_helper=None, run_as_root=True), - mock.call('grep 5006016090203181 /sys/class/fc_transport/' - 'target11:*/node_name'), - mock.call('tee', '-a', '/sys/class/scsi_host/host11/scan', + mock.call('tee', '-a', '/sys/class/scsi_host/host7/scan', process_input='6 7 1', - root_helper=None, run_as_root=True), - mock.call('tee', '-a', '/sys/class/scsi_host/host11/scan', - process_input='8 9 1', root_helper=None, run_as_root=True)] execute_mock.assert_has_calls(expected_commands) self.assertEqual(len(expected_commands), execute_mock.call_count) + expected_calls = [mock.call(hbas[0], con_props), + mock.call(hbas[1], con_props)] + mock_get_chan.assert_has_calls(expected_calls) + def test_rescan_hosts_wildcard(self): - hbas = [{'host_device': 'host10', 'node_name': '5006016090203181'}, - {'host_device': 'host11', 'node_name': '5006016090203181'}] + hbas, con_props = self.__get_rescan_info(zone_manager=True) with mock.patch.object(self.lfc, '_get_hba_channel_scsi_target', - return_value=None), \ + side_effect=(None, [])), \ mock.patch.object(self.lfc, '_execute', - return_value=None) as execute_mock: + side_effect=None) as execute_mock: - self.lfc.rescan_hosts(hbas, 1) + self.lfc.rescan_hosts(hbas, con_props) expected_commands = [ - mock.call('tee', '-a', '/sys/class/scsi_host/host10/scan', + mock.call('tee', '-a', '/sys/class/scsi_host/host6/scan', process_input='- - 1', root_helper=None, run_as_root=True), - mock.call('tee', '-a', '/sys/class/scsi_host/host11/scan', - process_input='- - 1', - root_helper=None, run_as_root=True)] - - execute_mock.assert_has_calls(expected_commands) - self.assertEqual(len(expected_commands), execute_mock.call_count) - - def test_rescan_hosts_wildcard_exception(self): - def _execute(cmd, *args, **kwargs): - if cmd.startswith('grep'): - raise Exception - - hbas = [{'host_device': 'host10', 'node_name': '5006016090203181'}, - {'host_device': 'host11', 'node_name': '5006016090203181'}] - with mock.patch.object(self.lfc, '_execute', - side_effect=_execute) as execute_mock: - - self.lfc.rescan_hosts(hbas, 1) - - expected_commands = [ - mock.call('grep 5006016090203181 /sys/class/fc_transport/' - 'target10:*/node_name'), - mock.call('tee', '-a', '/sys/class/scsi_host/host10/scan', - process_input='- - 1', - root_helper=None, run_as_root=True), - mock.call('grep 5006016090203181 /sys/class/fc_transport/' - 'target11:*/node_name'), - mock.call('tee', '-a', '/sys/class/scsi_host/host11/scan', + mock.call('tee', '-a', '/sys/class/scsi_host/host7/scan', process_input='- - 1', root_helper=None, run_as_root=True)]