From c65869626594522231709d9c90a30524a1f0d18c Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Mon, 3 Dec 2018 12:16:10 +0100 Subject: [PATCH] NetApp: Return all iSCSI targets-portals NetApp iSCSI drivers use discovery mode for multipathing, which means that we'll fail to attach a volume if the IP provided for the discovery is not accessible from the host. Something similar would happen when using singlepath, as we are only trying to connect to one target/portal. This patch changes NetApp drivers so the return target_iqns, target_portals, and target_luns parameters whenever there are more than one option. Closes-Bug: #1806398 Change-Id: If6b5ad23c899032dbf7535ed91251cbfe54a223a --- .../netapp/dataontap/test_block_base.py | 50 +++++++++---------- .../netapp/dataontap/test_block_cmode.py | 13 ----- .../tests/unit/volume/drivers/netapp/fakes.py | 14 ++++++ .../unit/volume/drivers/netapp/test_utils.py | 32 +++++++++--- .../drivers/netapp/dataontap/block_base.py | 32 +++++------- .../drivers/netapp/dataontap/block_cmode.py | 23 --------- cinder/volume/drivers/netapp/utils.py | 25 +++++++--- ...netapp-non-discovery-19af4e10f7b190ea.yaml | 8 +++ 8 files changed, 100 insertions(+), 97 deletions(-) create mode 100644 releasenotes/notes/netapp-non-discovery-19af4e10f7b190ea.yaml diff --git a/cinder/tests/unit/volume/drivers/netapp/dataontap/test_block_base.py b/cinder/tests/unit/volume/drivers/netapp/dataontap/test_block_base.py index 074cff8c661..6581e484de5 100644 --- a/cinder/tests/unit/volume/drivers/netapp/dataontap/test_block_base.py +++ b/cinder/tests/unit/volume/drivers/netapp/dataontap/test_block_base.py @@ -685,8 +685,8 @@ class NetAppBlockStorageLibraryTestCase(test.TestCase): self.zapi_client.get_iscsi_target_details.return_value = ( target_details_list) self.mock_object(block_base.NetAppBlockStorageLibrary, - '_get_preferred_target_from_list', - return_value=target_details_list[1]) + '_get_targets_from_list', + return_value=target_details_list) self.zapi_client.get_iscsi_service_details.return_value = ( fake.ISCSI_SERVICE_IQN) self.mock_object(na_utils, @@ -722,7 +722,7 @@ class NetAppBlockStorageLibraryTestCase(test.TestCase): fake.ISCSI_VOLUME['name'], [fake.ISCSI_CONNECTOR['initiator']], 'iscsi', None) self.zapi_client.get_iscsi_target_details.assert_called_once_with() - block_base.NetAppBlockStorageLibrary._get_preferred_target_from_list\ + block_base.NetAppBlockStorageLibrary._get_targets_from_list\ .assert_called_once_with( target_details_list) self.zapi_client.get_iscsi_service_details.assert_called_once_with() @@ -734,7 +734,7 @@ class NetAppBlockStorageLibraryTestCase(test.TestCase): return_value=fake.ISCSI_LUN['lun_id']) self.zapi_client.get_iscsi_target_details.return_value = None self.mock_object(block_base.NetAppBlockStorageLibrary, - '_get_preferred_target_from_list') + '_get_targets_from_list') self.mock_object(na_utils, 'get_iscsi_connection_properties', return_value=fake.ISCSI_CONNECTION_PROPERTIES) @@ -745,7 +745,7 @@ class NetAppBlockStorageLibraryTestCase(test.TestCase): self.assertEqual( 0, block_base.NetAppBlockStorageLibrary - ._get_preferred_target_from_list.call_count) + ._get_targets_from_list.call_count) self.assertEqual( 0, self.zapi_client.get_iscsi_service_details.call_count) self.assertEqual( @@ -758,7 +758,7 @@ class NetAppBlockStorageLibraryTestCase(test.TestCase): return_value=fake.ISCSI_LUN['lun_id']) self.zapi_client.get_iscsi_target_details.return_value = None self.mock_object(block_base.NetAppBlockStorageLibrary, - '_get_preferred_target_from_list', + '_get_targets_from_list', return_value=None) self.mock_object(na_utils, 'get_iscsi_connection_properties') @@ -780,8 +780,8 @@ class NetAppBlockStorageLibraryTestCase(test.TestCase): self.zapi_client.get_iscsi_target_details.return_value = ( target_details_list) self.mock_object(block_base.NetAppBlockStorageLibrary, - '_get_preferred_target_from_list', - return_value=target_details_list[1]) + '_get_targets_from_list', + return_value=target_details_list) self.zapi_client.get_iscsi_service_details.return_value = None self.mock_object(na_utils, 'get_iscsi_connection_properties') @@ -794,53 +794,49 @@ class NetAppBlockStorageLibraryTestCase(test.TestCase): fake.ISCSI_VOLUME['name'], [fake.ISCSI_CONNECTOR['initiator']], 'iscsi', None) self.zapi_client.get_iscsi_target_details.assert_called_once_with() - block_base.NetAppBlockStorageLibrary._get_preferred_target_from_list\ + block_base.NetAppBlockStorageLibrary._get_targets_from_list\ .assert_called_once_with(target_details_list) def test_get_target_details_list(self): target_details_list = fake.ISCSI_TARGET_DETAILS_LIST - result = self.library._get_preferred_target_from_list( - target_details_list) + result = self.library._get_targets_from_list(target_details_list) - self.assertEqual(target_details_list[0], result) + self.assertEqual(target_details_list, result) def test_get_preferred_target_from_empty_list(self): target_details_list = [] - result = self.library._get_preferred_target_from_list( - target_details_list) + result = self.library._get_targets_from_list(target_details_list) - self.assertIsNone(result) + self.assertFalse(bool(result)) - def test_get_preferred_target_from_list_with_one_interface_disabled(self): + def test_get_targets_from_list_with_one_interface_disabled(self): target_details_list = copy.deepcopy(fake.ISCSI_TARGET_DETAILS_LIST) target_details_list[0]['interface-enabled'] = 'false' - result = self.library._get_preferred_target_from_list( - target_details_list) + result = self.library._get_targets_from_list(target_details_list) - self.assertEqual(target_details_list[1], result) + self.assertEqual(target_details_list[1:], result) - def test_get_preferred_target_from_list_with_all_interfaces_disabled(self): + def test_get_targets_from_list_with_all_interfaces_disabled(self): target_details_list = copy.deepcopy(fake.ISCSI_TARGET_DETAILS_LIST) for target in target_details_list: target['interface-enabled'] = 'false' - result = self.library._get_preferred_target_from_list( - target_details_list) + result = self.library._get_targets_from_list(target_details_list) - self.assertEqual(target_details_list[0], result) + self.assertEqual(target_details_list, result) - def test_get_preferred_target_from_list_with_filter(self): + def test_get_targets_from_list_with_filter(self): target_details_list = fake.ISCSI_TARGET_DETAILS_LIST filter = [target_detail['address'] for target_detail in target_details_list[1:]] - result = self.library._get_preferred_target_from_list( - target_details_list, filter) + result = self.library._get_targets_from_list(target_details_list, + filter) - self.assertEqual(target_details_list[1], result) + self.assertEqual(target_details_list[1:], result) @mock.patch.object(na_utils, 'check_flags', mock.Mock()) @mock.patch.object(block_base, 'LOG', mock.Mock()) diff --git a/cinder/tests/unit/volume/drivers/netapp/dataontap/test_block_cmode.py b/cinder/tests/unit/volume/drivers/netapp/dataontap/test_block_cmode.py index 1935c3bdec1..0786bf2df9d 100644 --- a/cinder/tests/unit/volume/drivers/netapp/dataontap/test_block_cmode.py +++ b/cinder/tests/unit/volume/drivers/netapp/dataontap/test_block_cmode.py @@ -336,19 +336,6 @@ class NetAppBlockStorageCmodeLibraryTestCase(test.TestCase): fake.VOLUME_ID, fake.LUN_ID, fake.LUN_SIZE, fake.LUN_METADATA, None) - def test_get_preferred_target_from_list(self): - target_details_list = fake.ISCSI_TARGET_DETAILS_LIST - operational_addresses = [ - target['address'] - for target in target_details_list[2:]] - self.zapi_client.get_operational_lif_addresses = ( - mock.Mock(return_value=operational_addresses)) - - result = self.library._get_preferred_target_from_list( - target_details_list) - - self.assertEqual(target_details_list[2], result) - @ddt.data({'replication_backends': [], 'cluster_credentials': False}, {'replication_backends': ['target_1', 'target_2'], 'cluster_credentials': True}) diff --git a/cinder/tests/unit/volume/drivers/netapp/fakes.py b/cinder/tests/unit/volume/drivers/netapp/fakes.py index 6f74f55ae21..28536f12703 100644 --- a/cinder/tests/unit/volume/drivers/netapp/fakes.py +++ b/cinder/tests/unit/volume/drivers/netapp/fakes.py @@ -23,8 +23,10 @@ import cinder.volume.drivers.netapp.options as na_opts ISCSI_FAKE_LUN_ID = 1 ISCSI_FAKE_IQN = 'iqn.1993-08.org.debian:01:10' +ISCSI_FAKE_IQN2 = 'iqn.1993-08.org.debian:01:11' ISCSI_FAKE_ADDRESS_IPV4 = '10.63.165.216' +ISCSI_FAKE_ADDRESS2_IPV4 = '10.63.165.217' ISCSI_FAKE_ADDRESS_IPV6 = 'fe80::72a4:a152:aad9:30d9' ISCSI_FAKE_PORT = '2232' @@ -38,6 +40,18 @@ ISCSI_FAKE_TARGET['port'] = ISCSI_FAKE_PORT ISCSI_FAKE_VOLUME = {'id': 'fake_id', 'provider_auth': 'None stack password'} ISCSI_FAKE_VOLUME_NO_AUTH = {'id': 'fake_id', 'provider_auth': ''} +ISCSI_MP_TARGET_INFO_DICT = {'target_discovered': False, + 'target_portal': '10.63.165.216:2232', + 'target_portals': ['10.63.165.216:2232', + '10.63.165.217:2232'], + 'target_iqn': ISCSI_FAKE_IQN, + 'target_iqns': [ISCSI_FAKE_IQN, ISCSI_FAKE_IQN2], + 'target_lun': ISCSI_FAKE_LUN_ID, + 'target_luns': [ISCSI_FAKE_LUN_ID] * 2, + 'volume_id': ISCSI_FAKE_VOLUME['id'], + 'auth_method': 'None', 'auth_username': 'stack', + 'auth_password': 'password'} + FC_ISCSI_TARGET_INFO_DICT = {'target_discovered': False, 'target_portal': '10.63.165.216:2232', 'target_iqn': ISCSI_FAKE_IQN, diff --git a/cinder/tests/unit/volume/drivers/netapp/test_utils.py b/cinder/tests/unit/volume/drivers/netapp/test_utils.py index 85478004c2c..be8051de321 100644 --- a/cinder/tests/unit/volume/drivers/netapp/test_utils.py +++ b/cinder/tests/unit/volume/drivers/netapp/test_utils.py @@ -119,40 +119,56 @@ class NetAppDriverUtilsTestCase(test.TestCase): self.assertAlmostEqual(na_utils.round_down(-5.567, '0'), -5) def test_iscsi_connection_properties(self): - actual_properties = na_utils.get_iscsi_connection_properties( fake.ISCSI_FAKE_LUN_ID, fake.ISCSI_FAKE_VOLUME, - fake.ISCSI_FAKE_IQN, fake.ISCSI_FAKE_ADDRESS_IPV4, - fake.ISCSI_FAKE_PORT) + [fake.ISCSI_FAKE_IQN, fake.ISCSI_FAKE_IQN2], + [fake.ISCSI_FAKE_ADDRESS_IPV4, fake.ISCSI_FAKE_ADDRESS2_IPV4], + [fake.ISCSI_FAKE_PORT, fake.ISCSI_FAKE_PORT]) actual_properties_mapped = actual_properties['data'] self.assertDictEqual(actual_properties_mapped, - fake.FC_ISCSI_TARGET_INFO_DICT) + fake.ISCSI_MP_TARGET_INFO_DICT) + + def test_iscsi_connection_properties_single_iqn(self): + actual_properties = na_utils.get_iscsi_connection_properties( + fake.ISCSI_FAKE_LUN_ID, fake.ISCSI_FAKE_VOLUME, + fake.ISCSI_FAKE_IQN, + [fake.ISCSI_FAKE_ADDRESS_IPV4, fake.ISCSI_FAKE_ADDRESS2_IPV4], + [fake.ISCSI_FAKE_PORT, fake.ISCSI_FAKE_PORT]) + + actual_properties_mapped = actual_properties['data'] + expected = fake.ISCSI_MP_TARGET_INFO_DICT.copy() + expected['target_iqns'][1] = expected['target_iqns'][0] + + self.assertDictEqual(actual_properties_mapped, + fake.ISCSI_MP_TARGET_INFO_DICT) def test_iscsi_connection_lun_id_type_str(self): FAKE_LUN_ID = '1' actual_properties = na_utils.get_iscsi_connection_properties( FAKE_LUN_ID, fake.ISCSI_FAKE_VOLUME, fake.ISCSI_FAKE_IQN, - fake.ISCSI_FAKE_ADDRESS_IPV4, fake.ISCSI_FAKE_PORT) + [fake.ISCSI_FAKE_ADDRESS_IPV4], [fake.ISCSI_FAKE_PORT]) actual_properties_mapped = actual_properties['data'] self.assertIs(int, type(actual_properties_mapped['target_lun'])) + self.assertDictEqual(actual_properties_mapped, + fake.FC_ISCSI_TARGET_INFO_DICT) def test_iscsi_connection_lun_id_type_dict(self): FAKE_LUN_ID = {'id': 'fake_id'} self.assertRaises(TypeError, na_utils.get_iscsi_connection_properties, FAKE_LUN_ID, fake.ISCSI_FAKE_VOLUME, - fake.ISCSI_FAKE_IQN, fake.ISCSI_FAKE_ADDRESS_IPV4, - fake.ISCSI_FAKE_PORT) + fake.ISCSI_FAKE_IQN, [fake.ISCSI_FAKE_ADDRESS_IPV4], + [fake.ISCSI_FAKE_PORT]) def test_iscsi_connection_properties_ipv6(self): actual_properties = na_utils.get_iscsi_connection_properties( '1', fake.ISCSI_FAKE_VOLUME_NO_AUTH, fake.ISCSI_FAKE_IQN, - fake.ISCSI_FAKE_ADDRESS_IPV6, fake.ISCSI_FAKE_PORT) + [fake.ISCSI_FAKE_ADDRESS_IPV6], [fake.ISCSI_FAKE_PORT]) self.assertDictEqual(actual_properties['data'], fake.FC_ISCSI_TARGET_INFO_DICT_IPV6) diff --git a/cinder/volume/drivers/netapp/dataontap/block_base.py b/cinder/volume/drivers/netapp/dataontap/block_base.py index c6147969931..8ca86bf5c58 100644 --- a/cinder/volume/drivers/netapp/dataontap/block_base.py +++ b/cinder/volume/drivers/netapp/dataontap/block_base.py @@ -839,13 +839,12 @@ class NetAppBlockStorageLibrary(object): "initiator %(initiator_name)s", {'name': name, 'initiator_name': initiator_name}) - preferred_target = self._get_preferred_target_from_list( - target_list) - if preferred_target is None: + targets = self._get_targets_from_list(target_list) + if not targets: msg = _('Failed to get target portal for the LUN %s') raise exception.VolumeBackendAPIException(data=msg % name) - (address, port) = (preferred_target['address'], - preferred_target['port']) + addresses = [target['address'] for target in targets] + ports = [target['port'] for target in targets] iqn = self.zapi_client.get_iscsi_service_details() if not iqn: @@ -853,8 +852,8 @@ class NetAppBlockStorageLibrary(object): raise exception.VolumeBackendAPIException(data=msg % name) properties = na_utils.get_iscsi_connection_properties(lun_id, volume, - iqn, address, - port) + iqn, addresses, + ports) if self.configuration.use_chap_auth: chap_username, chap_password = self._configure_chap(initiator_name) @@ -881,18 +880,13 @@ class NetAppBlockStorageLibrary(object): properties['data']['discovery_auth_username'] = username properties['data']['discovery_auth_password'] = password - def _get_preferred_target_from_list(self, target_details_list, - filter=None): - preferred_target = None - for target in target_details_list: - if filter and target['address'] not in filter: - continue - if target.get('interface-enabled', 'true') == 'true': - preferred_target = target - break - if preferred_target is None and len(target_details_list) > 0: - preferred_target = target_details_list[0] - return preferred_target + def _get_targets_from_list(self, target_details_list, filter=None): + targets = [target for target in target_details_list + if (not filter or target['address'] in filter) and + target.get('interface-enabled', 'true') == 'true'] + if not targets and len(target_details_list) > 0: + targets = target_details_list + return targets def terminate_connection_iscsi(self, volume, connector, **kwargs): """Driver entry point to unattach a volume from an instance. diff --git a/cinder/volume/drivers/netapp/dataontap/block_cmode.py b/cinder/volume/drivers/netapp/dataontap/block_cmode.py index fec8c297358..e1058eb0866 100644 --- a/cinder/volume/drivers/netapp/dataontap/block_cmode.py +++ b/cinder/volume/drivers/netapp/dataontap/block_cmode.py @@ -390,29 +390,6 @@ class NetAppBlockStorageCmodeLibrary(block_base.NetAppBlockStorageLibrary, msg = 'Deleted LUN with name %(name)s and QoS info %(qos)s' LOG.debug(msg, {'name': volume['name'], 'qos': qos_policy_group_info}) - def _get_preferred_target_from_list(self, target_details_list, - filter=None): - # cDOT iSCSI LIFs do not migrate from controller to controller - # in failover. Rather, an iSCSI LIF must be configured on each - # controller and the initiator has to take responsibility for - # using a LIF that is UP. In failover, the iSCSI LIF on the - # downed controller goes DOWN until the controller comes back up. - # - # Currently Nova only accepts a single target when obtaining - # target details from Cinder, so we pass back the first portal - # with an UP iSCSI LIF. There are plans to have Nova accept - # and try multiple targets. When that happens, we can and should - # remove this filter and return all targets since their operational - # state could change between the time we test here and the time - # Nova uses the target. - - operational_addresses = ( - self.zapi_client.get_operational_lif_addresses()) - - return (super(NetAppBlockStorageCmodeLibrary, self) - ._get_preferred_target_from_list(target_details_list, - filter=operational_addresses)) - def _setup_qos_for_volume(self, volume, extra_specs): try: qos_policy_group_info = na_utils.get_valid_qos_policy_group_info( diff --git a/cinder/volume/drivers/netapp/utils.py b/cinder/volume/drivers/netapp/utils.py index c647e53a09d..94f756addf0 100644 --- a/cinder/volume/drivers/netapp/utils.py +++ b/cinder/volume/drivers/netapp/utils.py @@ -165,18 +165,29 @@ def log_extra_spec_warnings(extra_specs): 'new': DEPRECATED_SSC_SPECS[spec]}) -def get_iscsi_connection_properties(lun_id, volume, iqn, - address, port): +def get_iscsi_connection_properties(lun_id, volume, iqns, + addresses, ports): # literal ipv6 address - if netutils.is_valid_ipv6(address): - address = netutils.escape_ipv6(address) + addresses = [netutils.escape_ipv6(a) if netutils.is_valid_ipv6(a) else a + for a in addresses] + + lun_id = int(lun_id) + if isinstance(iqns, six.string_types): + iqns = [iqns] * len(addresses) + + target_portals = ['%s:%s' % (a, p) for a, p in zip(addresses, ports)] properties = {} properties['target_discovered'] = False - properties['target_portal'] = '%s:%s' % (address, port) - properties['target_iqn'] = iqn - properties['target_lun'] = int(lun_id) + properties['target_portal'] = target_portals[0] + properties['target_iqn'] = iqns[0] + properties['target_lun'] = lun_id properties['volume_id'] = volume['id'] + if len(addresses) > 1: + properties['target_portals'] = target_portals + properties['target_iqns'] = iqns + properties['target_luns'] = [lun_id] * len(addresses) + auth = volume['provider_auth'] if auth: (auth_method, auth_username, auth_secret) = auth.split() diff --git a/releasenotes/notes/netapp-non-discovery-19af4e10f7b190ea.yaml b/releasenotes/notes/netapp-non-discovery-19af4e10f7b190ea.yaml new file mode 100644 index 00000000000..6bd34e0b19d --- /dev/null +++ b/releasenotes/notes/netapp-non-discovery-19af4e10f7b190ea.yaml @@ -0,0 +1,8 @@ +--- +fixes: + - | + NetApp iSCSI drivers no longer use the discovery mechanism for multipathing + and they always return all target/portals when attaching a volume. Thanks + to this, volumes will be successfully attached even if the target/portal + selected as primary is down, this will be the case for both, multipath and + single path connections.