Merge "Fix iSCSI cleanup issue when using discovery"

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):
+    def _get_discoverydb_portals(self, connection_properties):
-        if all([key in connection_properties for key in ('target_portals',
+        """Retrieve iscsi portals information from the discoverydb.
-                                                         'target_iqns')]):
-            # Use targets specified by connection_properties
-            return list(zip(connection_properties['target_portals'],
-                        connection_properties['target_iqns'],
-                        self._get_luns(connection_properties)))
 
+        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:
+            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

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')