From 1905398f617ad02fb8546ac621eb1b292003a59a Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Tue, 20 Jun 2017 14:15:26 +0200 Subject: [PATCH] Fix iSCSI cleanup issue when using discovery With the latest iSCSI disconnect refactoring [1] we have introduced a regression where not all devices are properly cleaned up on a multipath connection if we are using target discovery. On connection we do the discovery so we get the targets, but on disconnection the code is expecting to have all the ips, iqns, and luns information on the connection properties, so it will only flush the multipath and disconnect volumes that were connected using the information from the connection property. This patch fixes this retrieving the targets information from the discoverydb that was gathered during the connection phase. Output from discoverydb query looks like this: SENDTARGETS: DiscoveryAddress: 192.168.1.33,3260 DiscoveryAddress: 192.168.1.2,3260 Target: iqn.2004-04.com.qnap:ts-831x:iscsi.cinder-20170531114245.9eff88 Portal: 192.168.1.3:3260,1 Iface Name: default Portal: 192.168.1.2:3260,1 Iface Name: default Target: iqn.2004-04.com.qnap:ts-831x:iscsi.cinder-20170531114447.9eff88 Portal: 192.168.1.3:3260,1 Iface Name: default Portal: 192.168.1.2:3260,1 Iface Name: default DiscoveryAddress: 192.168.1.38,3260 iSNS: No targets found. STATIC: No targets found. FIRMWARE: No targets found. [1] https://review.openstack.org/455392 Change-Id: Iada5d4fbeb07aeaf3afb953a289b6b89778c382c Closes-Bug: #1699061 --- os_brick/initiator/connectors/iscsi.py | 115 ++++++++++++-- .../tests/initiator/connectors/test_iscsi.py | 142 ++++++++++++++++-- 2 files changed, 235 insertions(+), 22 deletions(-) diff --git a/os_brick/initiator/connectors/iscsi.py b/os_brick/initiator/connectors/iscsi.py index 04ecef05a..df407038b 100644 --- a/os_brick/initiator/connectors/iscsi.py +++ b/os_brick/initiator/connectors/iscsi.py @@ -16,6 +16,7 @@ import collections import glob import os +import re import threading import time @@ -26,6 +27,7 @@ from oslo_utils import excutils from oslo_utils import strutils from os_brick import exception +from os_brick.i18n import _ from os_brick import initiator from os_brick.initiator.connectors import base from os_brick.initiator.connectors import base_iscsi @@ -159,16 +161,36 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector): # entry: [tcp, [1], 192.168.121.250:3260,1 ...] return [entry[2] for entry in self._get_iscsi_sessions_full()] - def _get_ips_iqns_luns(self, connection_properties): + def _get_ips_iqns_luns(self, connection_properties, discover=True): """Build a list of ips, iqns, and luns. + Used only when doing multipath, we have 3 cases: + + - All information is in the connection properties + - We have to do an iSCSI discovery to get the information + - We don't want to do another discovery and we query the discoverydb + :param connection_properties: The dictionary that describes all of the target volume attributes. :type connection_properties: dict + :param discover: Wheter doing an iSCSI discovery is acceptable. + :type discover: bool :returns: list of tuples of (ip, iqn, lun) """ try: - ips_iqns_luns = self._discover_iscsi_portals(connection_properties) + if ('target_portals' in connection_properties and + 'target_iqns' in connection_properties): + # Use targets specified by connection_properties + ips_iqns_luns = list( + zip(connection_properties['target_portals'], + connection_properties['target_iqns'], + self._get_luns(connection_properties))) + else: + method = (self._discover_iscsi_portals if discover + else self._get_discoverydb_portals) + ips_iqns_luns = method(connection_properties) + except exception.TargetPortalsNotFound: + raise except Exception: if 'target_portals' in connection_properties: raise exception.TargetPortalsNotFound( @@ -289,14 +311,74 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector): num_luns = len(con_props['target_iqns']) if iqns is None else len(iqns) return luns or [con_props.get('target_lun')] * num_luns - def _discover_iscsi_portals(self, connection_properties): - if all([key in connection_properties for key in ('target_portals', - 'target_iqns')]): - # Use targets specified by connection_properties - return list(zip(connection_properties['target_portals'], - connection_properties['target_iqns'], - self._get_luns(connection_properties))) + def _get_discoverydb_portals(self, connection_properties): + """Retrieve iscsi portals information from the discoverydb. + Example of discoverydb command output: + + SENDTARGETS: + DiscoveryAddress: 192.168.1.33,3260 + DiscoveryAddress: 192.168.1.2,3260 + Target: iqn.2004-04.com.qnap:ts-831x:iscsi.cinder-20170531114245.9eff88 + Portal: 192.168.1.3:3260,1 + Iface Name: default + Portal: 192.168.1.2:3260,1 + Iface Name: default + Target: iqn.2004-04.com.qnap:ts-831x:iscsi.cinder-20170531114447.9eff88 + Portal: 192.168.1.3:3260,1 + Iface Name: default + Portal: 192.168.1.2:3260,1 + Iface Name: default + DiscoveryAddress: 192.168.1.38,3260 + iSNS: + No targets found. + STATIC: + No targets found. + FIRMWARE: + No targets found. + + :param connection_properties: The dictionary that describes all + of the target volume attributes. + :type connection_properties: dict + :returns: list of tuples of (ip, iqn, lun) + """ + out = self._run_iscsiadm_bare(['-m', 'discoverydb', + '-o', 'show', + '-P', 1])[0] or "" + regex = ('^SENDTARGETS:\n.*?^DiscoveryAddress: ' + + connection_properties['target_portal'] + + '.*?\n(.*?)^(?:DiscoveryAddress|iSNS):.*') + info = re.search(regex, out, re.DOTALL | re.MULTILINE) + + ips = [] + iqns = [] + + if info: + iscsi_transport = ('iser' if self._get_transport() == 'iser' + else 'default') + iface = 'Iface Name: ' + iscsi_transport + current_iqn = '' + current_ip = '' + for line in info.group(1).splitlines(): + line = line.strip() + if line.startswith('Target:'): + current_iqn = line[8:] + elif line.startswith('Portal:'): + current_ip = line[8:].split(',')[0] + elif line.startswith(iface): + if current_iqn and current_ip: + iqns.append(current_iqn) + ips.append(current_ip) + current_ip = '' + + if not iqns: + raise exception.TargetPortalsNotFound( + _('Unable to find target portals information on discoverydb.')) + + luns = self._get_luns(connection_properties, iqns) + return list(zip(ips, iqns, luns)) + + def _discover_iscsi_portals(self, connection_properties): out = None iscsi_transport = ('iser' if self._get_transport() == 'iser' else 'default') @@ -640,9 +722,21 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector): If ips_iqns_luns parameter is provided connection_properties won't be used to get them. + + When doing multipath we may not have all the information on the + connection properties (sendtargets was used on connect) so we may have + to retrieve the info from the discoverydb. Call _get_ips_iqns_luns to + do the right things. """ if not ips_iqns_luns: - ips_iqns_luns = self._get_all_targets(connection_properties) + if self.use_multipath: + # We are only called from disconnect, so don't discover + ips_iqns_luns = self._get_ips_iqns_luns(connection_properties, + discover=False) + else: + ips_iqns_luns = self._get_all_targets(connection_properties) + LOG.debug('Getting connected devices for (ips,iqns,luns)=%s', + ips_iqns_luns) nodes = self._get_iscsi_nodes() sessions = self._get_iscsi_sessions_full() # Use (portal, iqn) to map the session value @@ -673,6 +767,7 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector): else: others.add(device) + LOG.debug('Resulting device map %s', device_map) return device_map @utils.trace diff --git a/os_brick/tests/initiator/connectors/test_iscsi.py b/os_brick/tests/initiator/connectors/test_iscsi.py index 61f1caa50..6dd15de0c 100644 --- a/os_brick/tests/initiator/connectors/test_iscsi.py +++ b/os_brick/tests/initiator/connectors/test_iscsi.py @@ -27,6 +27,10 @@ from os_brick.tests.initiator import test_connector @ddt.ddt class ISCSIConnectorTestCase(test_connector.ConnectorTestCase): + SINGLE_CON_PROPS = {'volume_id': 'vol_id', + 'target_portal': 'ip1:port1', + 'target_iqn': 'tgt1', + 'target_lun': '1'} CON_PROPS = { 'volume_id': 'vol_id', 'target_portal': 'ip1:port1', @@ -108,11 +112,15 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase): res = self.connector._get_iscsi_nodes() self.assertEqual([], res) + @mock.patch.object(iscsi.ISCSIConnector, '_get_ips_iqns_luns') @mock.patch('glob.glob') @mock.patch.object(iscsi.ISCSIConnector, '_get_iscsi_sessions_full') @mock.patch.object(iscsi.ISCSIConnector, '_get_iscsi_nodes') def test_get_connection_devices(self, nodes_mock, sessions_mock, - glob_mock): + glob_mock, iql_mock): + self.connector.use_multipath = True + iql_mock.return_value = self.connector._get_all_targets(self.CON_PROPS) + # List sessions from other targets and non tcp sessions sessions_mock.return_value = [ ('non-tcp:', '0', 'ip1:port1', '1', 'tgt1'), @@ -136,6 +144,7 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase): ('ip2:port2', 'tgt2'): ({'sdb'}, {'sdc'}), ('ip3:port3', 'tgt3'): (set(), set())} self.assertDictEqual(expected, res) + iql_mock.assert_called_once_with(self.CON_PROPS, discover=False) @mock.patch('glob.glob') @mock.patch.object(iscsi.ISCSIConnector, '_get_iscsi_sessions_full') @@ -525,6 +534,109 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase): force=mock.sentinel.Force, ignore_errors=mock.sentinel.ignore_errors) + @ddt.data(True, False) + @mock.patch.object(iscsi.ISCSIConnector, '_get_transport') + @mock.patch.object(iscsi.ISCSIConnector, '_run_iscsiadm_bare') + def test_get_discoverydb_portals(self, is_iser, iscsiadm_mock, + transport_mock): + params = {'iqn1': self.SINGLE_CON_PROPS['target_iqn'], + 'iqn2': 'iqn.2004-04.com.qnap:ts-831x:iscsi.cinder-2017.9ef', + 'ip1': self.SINGLE_CON_PROPS['target_portal'], + 'ip2': '192.168.1.3:3260', + 'transport': 'iser' if is_iser else 'default', + 'other_transport': 'default' if is_iser else 'iser'} + + iscsiadm_mock.return_value = ( + 'SENDTARGETS:\n' + 'DiscoveryAddress: 192.168.1.33,3260\n' + 'DiscoveryAddress: %(ip1)s\n' + 'Target: %(iqn1)s\n' + ' Portal: %(ip2)s,1\n' + ' Iface Name: %(transport)s\n' + ' Portal: %(ip1)s,1\n' + ' Iface Name: %(transport)s\n' + ' Portal: %(ip1)s,1\n' + ' Iface Name: %(other_transport)s\n' + 'Target: %(iqn2)s\n' + ' Portal: %(ip2)s,1\n' + ' Iface Name: %(transport)s\n' + ' Portal: %(ip1)s,1\n' + ' Iface Name: %(transport)s\n' + 'DiscoveryAddress: 192.168.1.38,3260\n' + 'iSNS:\n' + 'No targets found.\n' + 'STATIC:\n' + 'No targets found.\n' + 'FIRMWARE:\n' + 'No targets found.\n' % params, None) + transport_mock.return_value = 'iser' if is_iser else 'non-iser' + + res = self.connector._get_discoverydb_portals(self.SINGLE_CON_PROPS) + expected = [(params['ip2'], params['iqn1'], + self.SINGLE_CON_PROPS['target_lun']), + (params['ip1'], params['iqn1'], + self.SINGLE_CON_PROPS['target_lun']), + (params['ip2'], params['iqn2'], + self.SINGLE_CON_PROPS['target_lun']), + (params['ip1'], params['iqn2'], + self.SINGLE_CON_PROPS['target_lun'])] + self.assertListEqual(expected, res) + iscsiadm_mock.assert_called_once_with( + ['-m', 'discoverydb', '-o', 'show', '-P', 1]) + transport_mock.assert_called_once_with() + + @mock.patch.object(iscsi.ISCSIConnector, '_get_transport', return_value='') + @mock.patch.object(iscsi.ISCSIConnector, '_run_iscsiadm_bare') + def test_get_discoverydb_portals_error(self, iscsiadm_mock, + transport_mock): + """DiscoveryAddress is not present.""" + iscsiadm_mock.return_value = ( + 'SENDTARGETS:\n' + 'DiscoveryAddress: 192.168.1.33,3260\n' + 'DiscoveryAddress: 192.168.1.38,3260\n' + 'iSNS:\n' + 'No targets found.\n' + 'STATIC:\n' + 'No targets found.\n' + 'FIRMWARE:\n' + 'No targets found.\n', None) + + self.assertRaises(exception.TargetPortalsNotFound, + self.connector._get_discoverydb_portals, + self.SINGLE_CON_PROPS) + iscsiadm_mock.assert_called_once_with( + ['-m', 'discoverydb', '-o', 'show', '-P', 1]) + transport_mock.assert_not_called() + + @mock.patch.object(iscsi.ISCSIConnector, '_get_transport', return_value='') + @mock.patch.object(iscsi.ISCSIConnector, '_run_iscsiadm_bare') + def test_get_discoverydb_portals_error_is_present(self, iscsiadm_mock, + transport_mock): + """DiscoveryAddress is present but wrong iterface.""" + params = {'iqn': self.SINGLE_CON_PROPS['target_iqn'], + 'ip': self.SINGLE_CON_PROPS['target_portal']} + iscsiadm_mock.return_value = ( + 'SENDTARGETS:\n' + 'DiscoveryAddress: 192.168.1.33,3260\n' + 'DiscoveryAddress: %(ip)s\n' + 'Target: %(iqn)s\n' + ' Portal: %(ip)s,1\n' + ' Iface Name: iser\n' + 'DiscoveryAddress: 192.168.1.38,3260\n' + 'iSNS:\n' + 'No targets found.\n' + 'STATIC:\n' + 'No targets found.\n' + 'FIRMWARE:\n' + 'No targets found.\n' % params, None) + + self.assertRaises(exception.TargetPortalsNotFound, + self.connector._get_discoverydb_portals, + self.SINGLE_CON_PROPS) + iscsiadm_mock.assert_called_once_with( + ['-m', 'discoverydb', '-o', 'show', '-P', 1]) + transport_mock.assert_called_once_with() + @mock.patch.object(iscsi.ISCSIConnector, '_disconnect_connection') @mock.patch.object(iscsi.ISCSIConnector, '_get_connection_devices') @mock.patch.object(linuxscsi.LinuxSCSI, 'flush_multipath_device') @@ -793,32 +905,38 @@ Setting up iSCSI targets: unused @mock.patch.object(iscsi.ISCSIConnector, '_discover_iscsi_portals') def test_get_ips_iqns_luns_with_target_iqns(self, discover_mock): res = self.connector._get_ips_iqns_luns(self.CON_PROPS) - self.assertEqual(discover_mock.return_value, res) + expected = list(self.connector._get_all_targets(self.CON_PROPS)) + self.assertListEqual(expected, res) + discover_mock.assert_not_called() + + @mock.patch.object(iscsi.ISCSIConnector, '_get_discoverydb_portals') + @mock.patch.object(iscsi.ISCSIConnector, '_discover_iscsi_portals') + def test_get_ips_iqns_luns_discoverydb(self, discover_mock, + db_portals_mock): + db_portals_mock.return_value = [('ip1:port1', 'tgt1', '1'), + ('ip2:port2', 'tgt2', '2')] + res = self.connector._get_ips_iqns_luns(self.SINGLE_CON_PROPS, + discover=False) + self.assertListEqual(db_portals_mock.return_value, res) + db_portals_mock.assert_called_once_with(self.SINGLE_CON_PROPS) + discover_mock.assert_not_called() @mock.patch.object(iscsi.ISCSIConnector, '_discover_iscsi_portals') def test_get_ips_iqns_luns_no_target_iqns_share_iqn(self, discover_mock): - con_props = {'volume_id': 'vol_id', - 'target_portal': 'ip1:port1', - 'target_iqn': 'tgt1', - 'target_lun': '1'} discover_mock.return_value = [('ip1:port1', 'tgt1', '1'), ('ip1:port1', 'tgt2', '1'), ('ip2:port2', 'tgt1', '2'), ('ip2:port2', 'tgt2', '2')] - res = self.connector._get_ips_iqns_luns(con_props) + res = self.connector._get_ips_iqns_luns(self.SINGLE_CON_PROPS) expected = {('ip1:port1', 'tgt1', '1'), ('ip2:port2', 'tgt1', '2')} self.assertEqual(expected, set(res)) @mock.patch.object(iscsi.ISCSIConnector, '_discover_iscsi_portals') def test_get_ips_iqns_luns_no_target_iqns_diff_iqn(self, discover_mock): - con_props = {'volume_id': 'vol_id', - 'target_portal': 'ip1:port1', - 'target_iqn': 'tgt1', - 'target_lun': '1'} discover_mock.return_value = [('ip1:port1', 'tgt1', '1'), ('ip2:port2', 'tgt2', '2')] - res = self.connector._get_ips_iqns_luns(con_props) + res = self.connector._get_ips_iqns_luns(self.SINGLE_CON_PROPS) self.assertEqual(discover_mock.return_value, res) @mock.patch.object(iscsi.ISCSIConnector, '_get_iscsi_sessions_full')