From 6d3733beb5b4a6ca1f75a9e70d92514db983689e Mon Sep 17 00:00:00 2001 From: Tomoki Sekiyama Date: Tue, 9 Dec 2014 17:09:43 -0500 Subject: [PATCH] Failover to alternative iSCSI portals on login failure When the main iSCSI portal is unreachable by network failure etc., volume attach will fail even if the array has the other portal addresses alive. To enable nova-compute and brick (with corresponding patches) to fall-back to alternative portals, this patch makes Cinder to contains the multiple portal addresses and corresponding IQNs and LUNs in connection_info returned by initialize_connection API. The main portal information is also returned in target_portal, target_iqn, target_lun for backward compatibility. For example: {"connection_info": { "driver_volume_type": "iscsi", ... "data": {"target_portal": "10.0.1.2:3260", "target_portals": ["10.0.1.2:3260", "10.0.2.2:3260"], "target_iqn": "iqn.2014-2.org.example:vol1-1", "target_iqns":["iqn.2014-2.org.example:vol1-1", "iqn.2014-2.org.example:vol1-2"], "target_lun": 1, "target_luns": [1, 2], ...}}} This patch makes LVM iSCSI backend return multiple portals, iqns, and luns when iscsi_secondary_ip_addresses is specified in cinder.conf. It also make brick/initiator/connector.py to failover to alternative paths, if specified, when it fails to establish the main path. In addition, to unify the data structure, target_portal, target_iqn, target_lun will also be included even if 'multipath=True' is specified in connector information. Change-Id: I55600496487dfaa72301621d1269e50044d9b5dc Partially Implements: blueprint iscsi-alternative-portal --- cinder/brick/initiator/connector.py | 72 +++++++++------- cinder/tests/brick/test_brick_connector.py | 85 ++++++++++++++++++- .../tests/targets/test_base_iscsi_driver.py | 15 ++++ cinder/tests/test_volume.py | 5 +- cinder/volume/driver.py | 45 +++++++--- cinder/volume/targets/iscsi.py | 22 +++-- 6 files changed, 185 insertions(+), 59 deletions(-) diff --git a/cinder/brick/initiator/connector.py b/cinder/brick/initiator/connector.py index 5c0d53ef1..84f24e853 100644 --- a/cinder/brick/initiator/connector.py +++ b/cinder/brick/initiator/connector.py @@ -217,18 +217,27 @@ class ISCSIConnector(InitiatorConnector): super(ISCSIConnector, self).set_execute(execute) self._linuxscsi.set_execute(execute) - def _iterate_multiple_targets(self, connection_properties, ips_iqns_luns): - for ip, iqn, lun in ips_iqns_luns: + def _iterate_all_targets(self, connection_properties): + for ip, iqn, lun in self._get_all_targets(connection_properties): props = copy.deepcopy(connection_properties) props['target_portal'] = ip props['target_iqn'] = iqn props['target_lun'] = lun + for key in ('target_portals', 'target_iqns', 'target_luns'): + props.pop(key, None) yield props - def _multipath_targets(self, connection_properties): - return zip(connection_properties.get('target_portals', []), - connection_properties.get('target_iqns', []), - connection_properties.get('target_luns', [])) + def _get_all_targets(self, connection_properties): + if all([key in connection_properties for key in ('target_portals', + 'target_iqns', + 'target_luns')]): + return zip(connection_properties['target_portals'], + connection_properties['target_iqns'], + connection_properties['target_luns']) + + return [(connection_properties['target_portal'], + connection_properties['target_iqn'], + connection_properties.get('target_lun', 0))] def _discover_iscsi_portals(self, connection_properties): if all([key in connection_properties for key in ('target_portals', @@ -273,8 +282,16 @@ class ISCSIConnector(InitiatorConnector): self._rescan_iscsi() host_devices = self._get_device_path(connection_properties) else: - self._connect_to_iscsi_portal(connection_properties) - host_devices = self._get_device_path(connection_properties) + target_props = connection_properties + for props in self._iterate_all_targets(connection_properties): + if self._connect_to_iscsi_portal(props): + target_props = props + break + else: + LOG.warn(_LW( + 'Failed to login to any of the iSCSI targets.')) + + host_devices = self._get_device_path(target_props) # The /dev/disk/by-path/... node is not always present immediately # TODO(justinsb): This retry-with-delay is a pattern, move to utils? @@ -293,7 +310,7 @@ class ISCSIConnector(InitiatorConnector): if self.use_multipath: self._rescan_iscsi() else: - self._run_iscsiadm(connection_properties, ("--rescan",)) + self._run_iscsiadm(target_props, ("--rescan",)) tries = tries + 1 if all(map(lambda x: not os.path.exists(x), host_devices)): @@ -358,13 +375,7 @@ class ISCSIConnector(InitiatorConnector): # When multiple portals/iqns/luns are specified, we need to remove # unused devices created by logging into other LUNs' session. - ips_iqns_luns = self._multipath_targets(connection_properties) - if not ips_iqns_luns: - ips_iqns_luns = [[connection_properties['target_portal'], - connection_properties['target_iqn'], - connection_properties.get('target_lun', 0)]] - for props in self._iterate_multiple_targets(connection_properties, - ips_iqns_luns): + for props in self._iterate_all_targets(connection_properties): self._disconnect_volume_iscsi(props) def _disconnect_volume_iscsi(self, connection_properties): @@ -388,16 +399,8 @@ class ISCSIConnector(InitiatorConnector): self._disconnect_from_iscsi_portal(connection_properties) def _get_device_path(self, connection_properties): - multipath_targets = self._multipath_targets(connection_properties) - if multipath_targets: - return ["/dev/disk/by-path/ip-%s-iscsi-%s-lun-%s" % x for x in - multipath_targets] - - path = ("/dev/disk/by-path/ip-%(portal)s-iscsi-%(iqn)s-lun-%(lun)s" % - {'portal': connection_properties['target_portal'], - 'iqn': connection_properties['target_iqn'], - 'lun': connection_properties.get('target_lun', 0)}) - return [path] + return ["/dev/disk/by-path/ip-%s-iscsi-%s-lun-%s" % x for x in + self._get_all_targets(connection_properties)] def get_initiator(self): """Secure helper to read file as root.""" @@ -536,17 +539,20 @@ class ISCSIConnector(InitiatorConnector): ("--login",), check_exit_code=[0, 255]) except putils.ProcessExecutionError as err: - # as this might be one of many paths, - # only set successful logins to startup automatically - if err.exit_code in [15]: - self._iscsiadm_update(connection_properties, - "node.startup", - "automatic") - return + # exit_code=15 means the session already exists, so it should + # be regarded as successful login. + if err.exit_code not in [15]: + LOG.warn(_LW('Failed to login iSCSI target %(iqn)s ' + 'on portal %(portal)s (exit code %(err)s).'), + {'iqn': connection_properties['target_iqn'], + 'portal': connection_properties['target_portal'], + 'err': err.exit_code}) + return False self._iscsiadm_update(connection_properties, "node.startup", "automatic") + return True def _disconnect_from_iscsi_portal(self, connection_properties): self._iscsiadm_update(connection_properties, "node.startup", "manual", diff --git a/cinder/tests/brick/test_brick_connector.py b/cinder/tests/brick/test_brick_connector.py index 931a9ec95..314513c46 100644 --- a/cinder/tests/brick/test_brick_connector.py +++ b/cinder/tests/brick/test_brick_connector.py @@ -223,15 +223,15 @@ class ISCSIConnectorTestCase(ConnectorTestCase): initiator = self.connector.get_initiator() self.assertEqual(initiator, 'iqn.1234-56.foo.bar:01:23456789abc') - @test.testtools.skipUnless(os.path.exists('/dev/disk/by-path'), - 'Test requires /dev/disk/by-path') - def test_connect_volume(self): + def _test_connect_volume(self, extra_props, additional_commands): self.stubs.Set(os.path, 'exists', lambda x: True) location = '10.0.2.15:3260' name = 'volume-00000001' iqn = 'iqn.2010-10.org.openstack:%s' % name vol = {'id': 1, 'name': name} connection_info = self.iscsi_connection(vol, location, iqn) + for key, value in extra_props.iteritems(): + connection_info['data'][key] = value device = self.connector.connect_volume(connection_info['data']) dev_str = '/dev/disk/by-path/ip-%s-iscsi-%s-lun-1' % (location, iqn) self.assertEqual(device['type'], 'block') @@ -255,12 +255,89 @@ class ISCSIConnectorTestCase(ConnectorTestCase): ('iscsiadm -m node -T %s -p %s --logout' % (iqn, location)), ('iscsiadm -m node -T %s -p %s --op delete' % - (iqn, location)), ] + (iqn, location)), ] + additional_commands LOG.debug("self.cmds = %s" % self.cmds) LOG.debug("expected = %s" % expected_commands) self.assertEqual(expected_commands, self.cmds) + @test.testtools.skipUnless(os.path.exists('/dev/disk/by-path'), + 'Test requires /dev/disk/by-path') + def test_connect_volume(self): + self._test_connect_volume({}, []) + + @test.testtools.skipUnless(os.path.exists('/dev/disk/by-path'), + 'Test requires /dev/disk/by-path') + def test_connect_volume_with_alternative_targets(self): + location = '10.0.2.15:3260' + location2 = '10.0.3.15:3260' + iqn = 'iqn.2010-10.org.openstack:volume-00000001' + iqn2 = 'iqn.2010-10.org.openstack:volume-00000001-2' + extra_props = {'target_portals': [location, location2], + 'target_iqns': [iqn, iqn2], + 'target_luns': [1, 2]} + additional_commands = [('blockdev --flushbufs /dev/sdb'), + ('tee -a /sys/block/sdb/device/delete'), + ('iscsiadm -m node -T %s -p %s --op update' + ' -n node.startup -v manual' % + (iqn2, location2)), + ('iscsiadm -m node -T %s -p %s --logout' % + (iqn2, location2)), + ('iscsiadm -m node -T %s -p %s --op delete' % + (iqn2, location2))] + self._test_connect_volume(extra_props, additional_commands) + + @test.testtools.skipUnless(os.path.exists('/dev/disk/by-path'), + 'Test requires /dev/disk/by-path') + @mock.patch.object(os.path, 'exists') + @mock.patch.object(connector.ISCSIConnector, '_run_iscsiadm') + def test_connect_volume_with_alternative_targets_primary_error( + self, mock_iscsiadm, mock_exists): + location = '10.0.2.15:3260' + location2 = '10.0.3.15:3260' + name = 'volume-00000001' + iqn = 'iqn.2010-10.org.openstack:%s' % name + iqn2 = 'iqn.2010-10.org.openstack:%s-2' % name + vol = {'id': 1, 'name': name} + connection_info = self.iscsi_connection(vol, location, iqn) + connection_info['data']['target_portals'] = [location, location2] + connection_info['data']['target_iqns'] = [iqn, iqn2] + connection_info['data']['target_luns'] = [1, 2] + dev_str2 = '/dev/disk/by-path/ip-%s-iscsi-%s-lun-2' % (location2, iqn2) + + def fake_run_iscsiadm(iscsi_properties, iscsi_command, **kwargs): + if iscsi_properties['target_portal'] == location: + if iscsi_command == ('--login',): + raise putils.ProcessExecutionError(None, None, 21) + return mock.DEFAULT + + mock_iscsiadm.side_effect = fake_run_iscsiadm + mock_exists.side_effect = lambda x: x == dev_str2 + device = self.connector.connect_volume(connection_info['data']) + self.assertEqual('block', device['type']) + self.assertEqual(dev_str2, device['path']) + props = connection_info['data'].copy() + for key in ('target_portals', 'target_iqns', 'target_luns'): + props.pop(key, None) + props['target_portal'] = location2 + props['target_iqn'] = iqn2 + props['target_lun'] = 2 + mock_iscsiadm.assert_any_call(props, ('--login',), + check_exit_code=[0, 255]) + + mock_iscsiadm.reset_mock() + self.connector.disconnect_volume(connection_info['data'], device) + props = connection_info['data'].copy() + for key in ('target_portals', 'target_iqns', 'target_luns'): + props.pop(key, None) + mock_iscsiadm.assert_any_call(props, ('--logout',), + check_exit_code=[0, 21, 255]) + props['target_portal'] = location2 + props['target_iqn'] = iqn2 + props['target_lun'] = 2 + mock_iscsiadm.assert_any_call(props, ('--logout',), + check_exit_code=[0, 21, 255]) + def test_connect_volume_with_multipath(self): location = '10.0.2.15:3260' name = 'volume-00000001' diff --git a/cinder/tests/targets/test_base_iscsi_driver.py b/cinder/tests/targets/test_base_iscsi_driver.py index a6ee62598..9bd5d752c 100644 --- a/cinder/tests/targets/test_base_iscsi_driver.py +++ b/cinder/tests/targets/test_base_iscsi_driver.py @@ -78,6 +78,21 @@ class TestBaseISCSITargetDriver(test.TestCase): self.assertEqual(self.expected_iscsi_properties, self.target._get_iscsi_properties(self.testvol)) + def test_get_iscsi_properties_multiple_targets(self): + testvol = self.testvol.copy() + expected_iscsi_properties = self.expected_iscsi_properties.copy() + iqn = expected_iscsi_properties['target_iqn'] + testvol.update( + {'provider_location': '10.10.7.1:3260;10.10.8.1:3260 ' + 'iqn.2010-10.org.openstack:' + 'volume-%s 0' % self.fake_volume_id}) + expected_iscsi_properties.update( + {'target_portals': ['10.10.7.1:3260', '10.10.8.1:3260'], + 'target_iqns': [iqn, iqn], + 'target_luns': [0, 0]}) + self.assertEqual(expected_iscsi_properties, + self.target._get_iscsi_properties(testvol)) + def test_build_iscsi_auth_string(self): auth_string = 'chap chap-user chap-password' self.assertEqual(auth_string, diff --git a/cinder/tests/test_volume.py b/cinder/tests/test_volume.py index bc3c8e100..e17d401b1 100644 --- a/cinder/tests/test_volume.py +++ b/cinder/tests/test_volume.py @@ -4534,7 +4534,10 @@ class ISCSITestCase(DriverTestCase): "attached_mode": "rw"} iscsi_driver = \ cinder.volume.targets.tgt.TgtAdm(configuration=self.configuration) - result = iscsi_driver._get_iscsi_properties(volume, multipath=True) + result = iscsi_driver._get_iscsi_properties(volume) + self.assertEqual(result["target_portal"], "1.1.1.1:3260") + self.assertEqual(result["target_iqn"], "iqn:iqn") + self.assertEqual(result["target_lun"], 0) self.assertEqual(["1.1.1.1:3260", "2.2.2.2:3261"], result["target_portals"]) self.assertEqual(["iqn:iqn", "iqn:iqn"], result["target_iqns"]) diff --git a/cinder/volume/driver.py b/cinder/volume/driver.py index 17cee2407..4bdca465c 100644 --- a/cinder/volume/driver.py +++ b/cinder/volume/driver.py @@ -1235,10 +1235,15 @@ class ISCSIDriver(VolumeDriver): :access_mode: the volume access mode allow client used ('rw' or 'ro' currently supported) - In some of drivers, When multipath=True is specified, :target_iqn, - :target_portal, :target_lun may be replaced with :target_iqns, - :target_portals, :target_luns, which contain lists of multiple values. - In this case, the initiator should establish sessions to all the path. + In some of drivers that support multiple connections (for multipath + and for single path with failover on connection failure), it returns + :target_iqns, :target_portals, :target_luns, which contain lists of + multiple values. The main portal information is also returned in + :target_iqn, :target_portal, :target_lun for backward compatibility. + + Note that some of drivers don't return :target_portals even if they + support multipath. Then the connector should use sendtargets discovery + to find the other portals if it supports multipath. """ properties = {} @@ -1276,14 +1281,13 @@ class ISCSIDriver(VolumeDriver): else: lun = 0 - if multipath: + if nr_portals > 1: properties['target_portals'] = portals properties['target_iqns'] = [iqn] * nr_portals properties['target_luns'] = [lun] * nr_portals - else: - properties['target_portal'] = portals[0] - properties['target_iqn'] = iqn - properties['target_lun'] = lun + properties['target_portal'] = portals[0] + properties['target_iqn'] = iqn + properties['target_lun'] = lun properties['volume_id'] = volume['id'] @@ -1351,14 +1355,31 @@ class ISCSIDriver(VolumeDriver): } } + If the backend driver supports multiple connections for multipath and + for single path with failover, "target_portals", "target_iqns", + "target_luns" are also populated:: + + { + 'driver_volume_type': 'iscsi' + 'data': { + 'target_discovered': False, + 'target_iqn': 'iqn.2010-10.org.openstack:volume1', + 'target_iqns': ['iqn.2010-10.org.openstack:volume1', + 'iqn.2010-10.org.openstack:volume1-2'], + 'target_portal': '10.0.0.1:3260', + 'target_portals': ['10.0.0.1:3260', '10.0.1.1:3260'] + 'target_lun': 1, + 'target_luns': [1, 1], + 'volume_id': 1, + 'access_mode': 'rw' + } + } """ # NOTE(jdg): Yes, this is duplicated in the volume/target # drivers, for now leaving it as there are 3'rd party # drivers that don't use target drivers, but inherit from # this base class and use this init data - iscsi_properties = self._get_iscsi_properties(volume, - connector.get( - 'multipath')) + iscsi_properties = self._get_iscsi_properties(volume) return { 'driver_volume_type': 'iscsi', 'data': iscsi_properties diff --git a/cinder/volume/targets/iscsi.py b/cinder/volume/targets/iscsi.py index a71dda35e..bdc099830 100644 --- a/cinder/volume/targets/iscsi.py +++ b/cinder/volume/targets/iscsi.py @@ -70,10 +70,15 @@ class ISCSITarget(driver.Target): :access_mode: the volume access mode allow client used ('rw' or 'ro' currently supported) - When multipath=True is specified, :target_iqn, :target_portal, - :target_lun may be replaced with :target_iqns, :target_portals, - :target_luns, which contain lists of multiple values. - In this case, the initiator should establish sessions to all the path. + In some of drivers that support multiple connections (for multipath + and for single path with failover on connection failure), it returns + :target_iqns, :target_portals, :target_luns, which contain lists of + multiple values. The main portal information is also returned in + :target_iqn, :target_portal, :target_lun for backward compatibility. + + Note that some of drivers don't return :target_portals even if they + support multipath. Then the connector should use sendtargets discovery + to find the other portals if it supports multipath. """ properties = {} @@ -113,14 +118,13 @@ class ISCSITarget(driver.Target): else: lun = 0 - if multipath: + if nr_portals > 1 or multipath: properties['target_portals'] = portals properties['target_iqns'] = [iqn] * nr_portals properties['target_luns'] = [lun] * nr_portals - else: - properties['target_portal'] = portals[0] - properties['target_iqn'] = iqn - properties['target_lun'] = lun + properties['target_portal'] = portals[0] + properties['target_iqn'] = iqn + properties['target_lun'] = lun properties['volume_id'] = volume['id'] -- 2.45.2