From: Tom Barron Date: Wed, 25 Mar 2015 15:28:08 +0000 (-0400) Subject: Only use operational LIFs for iscsi target details X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=9d77c0e1435d377790eaf812691b29e8b7a6b2c7;p=openstack-build%2Fcinder-build.git Only use operational LIFs for iscsi target details When Nova invokes our cDOT driver's initialize_connection() method in order to attach an iSCSI volume to an instance, we have been returning the first target from the list of targets returned from the filer API call to get iscsi target details. In failover mode, this may be a target with a LIF that is down, causing the attach attempt to fail in Nova. This commit fixes that issue so that attaches still work in failover by returning the first target with an *operational* LIF. Closes-bug: 1437419 Change-Id: I3f53c3644f7d94f5188151c16d70e565b745ad4a --- diff --git a/cinder/tests/test_netapp.py b/cinder/tests/test_netapp.py index ef78eb8ff..d21f96ec4 100644 --- a/cinder/tests/test_netapp.py +++ b/cinder/tests/test_netapp.py @@ -1,5 +1,5 @@ # Copyright (c) 2012 NetApp, Inc. -# All Rights Reserved. +# All rights reserved. # # Licensed under the Apache License, Version 2.0 (the "License"); you may # not use this file except in compliance with the License. You may obtain @@ -30,6 +30,7 @@ from cinder import test from cinder.volume import configuration as conf from cinder.volume.drivers.netapp import common from cinder.volume.drivers.netapp.dataontap.client import client_base +from cinder.volume.drivers.netapp.dataontap.client import client_cmode from cinder.volume.drivers.netapp.dataontap import ssc_cmode from cinder.volume.drivers.netapp import options from cinder.volume.drivers.netapp import utils @@ -646,6 +647,9 @@ class NetAppDirectCmodeISCSIDriverTestCase(test.TestCase): self.driver.delete_volume(self.volume) def test_map_unmap(self): + self.mock_object(client_cmode.Client, + 'get_operational_network_interface_addresses', + mock.Mock(return_value=[])) self.driver.create_volume(self.volume) updates = self.driver.create_export(None, self.volume) self.assertTrue(updates['provider_location']) @@ -667,6 +671,9 @@ class NetAppDirectCmodeISCSIDriverTestCase(test.TestCase): self.driver.delete_volume(self.volume) def test_map_by_creating_igroup(self): + self.mock_object(client_cmode.Client, + 'get_operational_network_interface_addresses', + mock.Mock(return_value=[])) self.driver.create_volume(self.volume) updates = self.driver.create_export(None, self.volume) self.assertTrue(updates['provider_location']) diff --git a/cinder/tests/volume/drivers/netapp/dataontap/client/fakes.py b/cinder/tests/volume/drivers/netapp/dataontap/client/fakes.py new file mode 100644 index 000000000..82e94ab56 --- /dev/null +++ b/cinder/tests/volume/drivers/netapp/dataontap/client/fakes.py @@ -0,0 +1,31 @@ +# Copyright (c) - 2015, Tom Barron. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + + +from lxml import etree + + +GET_OPERATIONAL_NETWORK_INTERFACE_ADDRESSES_RESPONSE = etree.XML(""" + + 2 + + +
%(address1)s
+
+ +
%(address2)s
+
+
+
+""" % {"address1": "1.2.3.4", "address2": "99.98.97.96"}) diff --git a/cinder/tests/volume/drivers/netapp/dataontap/client/test_client_cmode.py b/cinder/tests/volume/drivers/netapp/dataontap/client/test_client_cmode.py index 382dea266..25a23ae67 100644 --- a/cinder/tests/volume/drivers/netapp/dataontap/client/test_client_cmode.py +++ b/cinder/tests/volume/drivers/netapp/dataontap/client/test_client_cmode.py @@ -1,5 +1,5 @@ -# Copyright (c) 2014 Alex Meade. All rights reserved. -# All Rights Reserved. +# Copyright (c) 2014 Alex Meade. +# All rights reserved. # # Licensed under the Apache License, Version 2.0 (the "License"); you may # not use this file except in compliance with the License. You may obtain @@ -21,6 +21,7 @@ import six from cinder import exception from cinder import test +from cinder.tests.volume.drivers.netapp.dataontap.client import fakes as fake from cinder.volume.drivers.netapp.dataontap.client import api as netapp_api from cinder.volume.drivers.netapp.dataontap.client import client_cmode from cinder.volume.drivers.netapp import utils as netapp_utils @@ -658,3 +659,14 @@ class NetAppCmodeClientTestCase(test.TestCase): actual_bytes = self.client.get_file_usage(fake_vserver, fake_path) self.assertEqual(expected_bytes, actual_bytes) + + def test_get_operational_network_interface_addresses(self): + expected_result = ['1.2.3.4', '99.98.97.96'] + api_response = netapp_api.NaElement( + fake.GET_OPERATIONAL_NETWORK_INTERFACE_ADDRESSES_RESPONSE) + self.connection.invoke_successfully.return_value = api_response + + address_list = ( + self.client.get_operational_network_interface_addresses()) + + self.assertEqual(expected_result, address_list) diff --git a/cinder/tests/volume/drivers/netapp/dataontap/fakes.py b/cinder/tests/volume/drivers/netapp/dataontap/fakes.py index b55d1a90f..39075ba2c 100644 --- a/cinder/tests/volume/drivers/netapp/dataontap/fakes.py +++ b/cinder/tests/volume/drivers/netapp/dataontap/fakes.py @@ -20,7 +20,6 @@ METADATA = {'OsType': 'linux', 'SpaceReserved': 'true'} UUID1 = '12345678-1234-5678-1234-567812345678' LUN1 = '/vol/vol0/lun1' -IGROUP1_NAME = 'openstack-igroup1' VSERVER1_NAME = 'openstack-vserver' FC_VOLUME = {'name': 'fake_volume'} @@ -70,6 +69,45 @@ FC_TARGET_INFO_UNMAP = {'driver_volume_type': 'fibre_channel', 'data': {'target_wwn': FC_TARGET_WWPNS, 'initiator_target_map': FC_I_T_MAP}} -IGROUP1 = {'initiator-group-os-type': 'linux', - 'initiator-group-type': 'fcp', - 'initiator-group-name': IGROUP1_NAME} +IGROUP1_NAME = 'openstack-igroup1' + +IGROUP1 = { + 'initiator-group-os-type': 'linux', + 'initiator-group-type': 'fcp', + 'initiator-group-name': IGROUP1_NAME, +} + +ISCSI_VOLUME = { + 'name': 'fake_volume', 'id': 'fake_id', + 'provider_auth': 'fake provider auth', +} + +ISCSI_LUN = {'name': ISCSI_VOLUME, 'lun_id': 42} + +ISCSI_SERVICE_IQN = 'fake_iscsi_service_iqn' + +ISCSI_CONNECTION_PROPERTIES = { + 'data': { + 'auth_method': 'fake', + 'auth_password': 'auth', + 'auth_username': 'provider', + 'target_discovered': False, + 'target_iqn': ISCSI_SERVICE_IQN, + 'target_lun': 42, + 'target_portal': '1.2.3.4:3260', + 'volume_id': 'fake_id', + }, + 'driver_volume_type': 'iscsi', +} + +ISCSI_CONNECTOR = { + 'ip': '1.1.1.1', + 'host': 'fake_host', + 'initiator': 'fake_initiator_iqn', +} + +ISCSI_TARGET_DETAILS_LIST = [ + {'address': '5.6.7.8', 'port': '3260'}, + {'address': '1.2.3.4', 'port': '3260'}, + {'address': '99.98.97.96', 'port': '3260'}, +] diff --git a/cinder/tests/volume/drivers/netapp/dataontap/test_block_7mode.py b/cinder/tests/volume/drivers/netapp/dataontap/test_block_7mode.py index 9e095786d..874875e9c 100644 --- a/cinder/tests/volume/drivers/netapp/dataontap/test_block_7mode.py +++ b/cinder/tests/volume/drivers/netapp/dataontap/test_block_7mode.py @@ -364,3 +364,10 @@ class NetAppBlockStorage7modeLibraryTestCase(test.TestCase): self.library._check_volume_type_for_lun, {'vol': 'vol'}, mock_lun, {'ref': 'ref'}) get_specs.assert_called_once_with({'vol': 'vol'}) + + def test_get_preferred_target_from_list(self): + + result = self.library._get_preferred_target_from_list( + fake.ISCSI_TARGET_DETAILS_LIST) + + self.assertEqual(fake.ISCSI_TARGET_DETAILS_LIST[0], result) diff --git a/cinder/tests/volume/drivers/netapp/dataontap/test_block_base.py b/cinder/tests/volume/drivers/netapp/dataontap/test_block_base.py index ce4378766..6637307ce 100644 --- a/cinder/tests/volume/drivers/netapp/dataontap/test_block_base.py +++ b/cinder/tests/volume/drivers/netapp/dataontap/test_block_base.py @@ -19,6 +19,7 @@ Mock unit tests for the NetApp block storage library """ +import copy import uuid import mock @@ -441,3 +442,148 @@ class NetAppBlockStorageLibraryTestCase(test.TestCase): def test_is_lun_valid_on_storage(self): self.assertTrue(self.library._is_lun_valid_on_storage('lun')) + + def test_initialize_connection_iscsi(self): + target_details_list = fake.ISCSI_TARGET_DETAILS_LIST + volume = fake.ISCSI_VOLUME + connector = fake.ISCSI_CONNECTOR + self.mock_object(block_base.NetAppBlockStorageLibrary, '_map_lun', + mock.Mock(return_value=fake.ISCSI_LUN['lun_id'])) + self.zapi_client.get_iscsi_target_details.return_value = ( + target_details_list) + self.mock_object(block_base.NetAppBlockStorageLibrary, + '_get_preferred_target_from_list', + mock.Mock(return_value=target_details_list[1])) + self.zapi_client.get_iscsi_service_details.return_value = ( + fake.ISCSI_SERVICE_IQN) + self.mock_object( + na_utils, 'get_iscsi_connection_properties', + mock.Mock(return_value=fake.ISCSI_CONNECTION_PROPERTIES)) + + target_info = self.library.initialize_connection_iscsi(volume, + connector) + + self.assertEqual(fake.ISCSI_CONNECTION_PROPERTIES, target_info) + block_base.NetAppBlockStorageLibrary._map_lun.assert_called_once_with( + 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\ + .assert_called_once_with( + target_details_list) + self.zapi_client.get_iscsi_service_details.assert_called_once_with() + + def test_initialize_connection_iscsi_no_target_list(self): + volume = fake.ISCSI_VOLUME + connector = fake.ISCSI_CONNECTOR + self.mock_object(block_base.NetAppBlockStorageLibrary, '_map_lun', + mock.Mock(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') + self.mock_object( + na_utils, 'get_iscsi_connection_properties', + mock.Mock(return_value=fake.ISCSI_CONNECTION_PROPERTIES)) + + self.assertRaises(exception.VolumeBackendAPIException, + self.library.initialize_connection_iscsi, + volume, connector) + + self.assertEqual( + 0, block_base.NetAppBlockStorageLibrary + ._get_preferred_target_from_list.call_count) + self.assertEqual( + 0, self.zapi_client.get_iscsi_service_details.call_count) + self.assertEqual( + 0, na_utils.get_iscsi_connection_properties.call_count) + + def test_initialize_connection_iscsi_no_preferred_target(self): + volume = fake.ISCSI_VOLUME + connector = fake.ISCSI_CONNECTOR + self.mock_object(block_base.NetAppBlockStorageLibrary, '_map_lun', + mock.Mock(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', + mock.Mock(return_value=None)) + self.mock_object(na_utils, 'get_iscsi_connection_properties') + + self.assertRaises(exception.VolumeBackendAPIException, + self.library.initialize_connection_iscsi, + volume, connector) + + self.assertEqual(0, self.zapi_client + .get_iscsi_service_details.call_count) + self.assertEqual(0, na_utils.get_iscsi_connection_properties + .call_count) + + def test_initialize_connection_iscsi_no_iscsi_service_details(self): + target_details_list = fake.ISCSI_TARGET_DETAILS_LIST + volume = fake.ISCSI_VOLUME + connector = fake.ISCSI_CONNECTOR + self.mock_object(block_base.NetAppBlockStorageLibrary, '_map_lun', + mock.Mock(return_value=fake.ISCSI_LUN['lun_id'])) + self.zapi_client.get_iscsi_target_details.return_value = ( + target_details_list) + self.mock_object(block_base.NetAppBlockStorageLibrary, + '_get_preferred_target_from_list', + mock.Mock(return_value=target_details_list[1])) + self.zapi_client.get_iscsi_service_details.return_value = None + self.mock_object(na_utils, 'get_iscsi_connection_properties') + + self.assertRaises(exception.VolumeBackendAPIException, + self.library.initialize_connection_iscsi, + volume, + connector) + + block_base.NetAppBlockStorageLibrary._map_lun.assert_called_once_with( + 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\ + .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) + + self.assertEqual(target_details_list[0], result) + + def test_get_preferred_target_from_empty_list(self): + target_details_list = [] + + result = self.library._get_preferred_target_from_list( + target_details_list) + + self.assertEqual(None, result) + + def test_get_preferred_target_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) + + self.assertEqual(target_details_list[1], result) + + def test_get_preferred_target_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) + + self.assertEqual(target_details_list[0], result) + + def test_get_preferred_target_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) + + self.assertEqual(target_details_list[1], result) diff --git a/cinder/tests/volume/drivers/netapp/dataontap/test_block_cmode.py b/cinder/tests/volume/drivers/netapp/dataontap/test_block_cmode.py index 5cefcf092..b47c46975 100644 --- a/cinder/tests/volume/drivers/netapp/dataontap/test_block_cmode.py +++ b/cinder/tests/volume/drivers/netapp/dataontap/test_block_cmode.py @@ -261,3 +261,16 @@ class NetAppBlockStorageCmodeLibraryTestCase(test.TestCase): self.zapi_client.set_lun_qos_policy_group.assert_called_once_with( '/vol/lun', 'qos') self.assertEqual(1, driver_log.call_count) + + 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_network_interface_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) diff --git a/cinder/volume/drivers/netapp/dataontap/block_7mode.py b/cinder/volume/drivers/netapp/dataontap/block_7mode.py index 8f11aa028..01f7c8e20 100644 --- a/cinder/volume/drivers/netapp/dataontap/block_7mode.py +++ b/cinder/volume/drivers/netapp/dataontap/block_7mode.py @@ -338,3 +338,12 @@ class NetAppBlockStorage7modeLibrary(block_base. raise exception.ManageExistingVolumeTypeMismatch( reason=_("Setting LUN QoS policy group is not supported" " on this storage family and ONTAP version.")) + + def _get_preferred_target_from_list(self, target_details_list): + # 7-mode iSCSI LIFs migrate from controller to controller + # in failover and flap operational state in transit, so + # we don't filter these on operational state. + + return (super(NetAppBlockStorage7modeLibrary, self) + ._get_preferred_target_from_list(target_details_list, + filter=None)) diff --git a/cinder/volume/drivers/netapp/dataontap/block_base.py b/cinder/volume/drivers/netapp/dataontap/block_base.py index 8eb754487..27633d627 100644 --- a/cinder/volume/drivers/netapp/dataontap/block_base.py +++ b/cinder/volume/drivers/netapp/dataontap/block_base.py @@ -608,31 +608,28 @@ class NetAppBlockStorageLibrary(object): initiator_name = connector['initiator'] name = volume['name'] lun_id = self._map_lun(name, [initiator_name], 'iscsi', None) + msg = _("Mapped LUN %(name)s to the initiator %(initiator_name)s") msg_fmt = {'name': name, 'initiator_name': initiator_name} LOG.debug(msg % msg_fmt) - target_details_list = self.zapi_client.get_iscsi_target_details() - msg = _("Successfully fetched target details for LUN %(name)s and " - "initiator %(initiator_name)s") - msg_fmt = {'name': name, 'initiator_name': initiator_name} - LOG.debug(msg % msg_fmt) - if not target_details_list: - msg = _('Failed to get LUN target details for the LUN %s') + target_list = self.zapi_client.get_iscsi_target_details() + if not target_list: + msg = _('Failed to get LUN target list for the LUN %s') raise exception.VolumeBackendAPIException(data=msg % name) - target_details = None - for tgt_detail in target_details_list: - if tgt_detail.get('interface-enabled', 'true') == 'true': - target_details = tgt_detail - break - if not target_details: - target_details = target_details_list[0] - (address, port) = (target_details['address'], target_details['port']) + msg = _("Successfully fetched target list for LUN %(name)s and " + "initiator %(initiator_name)s") + msg_fmt = {'name': name, 'initiator_name': initiator_name} + LOG.debug(msg % msg_fmt) - if not target_details['address'] and target_details['port']: + preferred_target = self._get_preferred_target_from_list( + target_list) + if preferred_target is None: msg = _('Failed to get target portal for the LUN %s') raise exception.VolumeBackendAPIException(data=msg % name) + (address, port) = (preferred_target['address'], + preferred_target['port']) iqn = self.zapi_client.get_iscsi_service_details() if not iqn: @@ -642,9 +639,21 @@ class NetAppBlockStorageLibrary(object): properties = na_utils.get_iscsi_connection_properties(lun_id, volume, iqn, address, port) - return properties + 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 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 af9abca2c..3487f5413 100644 --- a/cinder/volume/drivers/netapp/dataontap/block_cmode.py +++ b/cinder/volume/drivers/netapp/dataontap/block_cmode.py @@ -283,3 +283,25 @@ class NetAppBlockStorageCmodeLibrary(block_base. " type. Ensure LUN volume with ssc features is" " present on vserver %(vs)s.") % {'ref': existing_ref, 'vs': self.vserver})) + + def _get_preferred_target_from_list(self, target_details_list): + # 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_network_interface_addresses()) + + return (super(NetAppBlockStorageCmodeLibrary, self) + ._get_preferred_target_from_list(target_details_list, + filter=operational_addresses)) diff --git a/cinder/volume/drivers/netapp/dataontap/client/client_base.py b/cinder/volume/drivers/netapp/dataontap/client/client_base.py index c8ac0f047..130d903fd 100644 --- a/cinder/volume/drivers/netapp/dataontap/client/client_base.py +++ b/cinder/volume/drivers/netapp/dataontap/client/client_base.py @@ -59,6 +59,13 @@ class Client(object): if not isinstance(elem, netapp_api.NaElement): raise ValueError('Expects NaElement') + def send_request(self, api_name, api_args=None, enable_tunneling=True): + """Sends request to Ontapi.""" + request = netapp_api.NaElement(api_name) + if api_args: + request.translate_struct(api_args) + return self.connection.invoke_successfully(request, enable_tunneling) + def create_lun(self, volume_name, lun_name, size, metadata, qos_policy_group=None): """Issues API request for creating LUN on volume.""" diff --git a/cinder/volume/drivers/netapp/dataontap/client/client_cmode.py b/cinder/volume/drivers/netapp/dataontap/client/client_cmode.py index 00b32fd5a..68450a590 100644 --- a/cinder/volume/drivers/netapp/dataontap/client/client_cmode.py +++ b/cinder/volume/drivers/netapp/dataontap/client/client_cmode.py @@ -460,3 +460,26 @@ class Client(client_base.Client): msg = _("Data ONTAP API version could not be determined.") raise exception.VolumeBackendAPIException(data=msg) return failed_apis + + def get_operational_network_interface_addresses(self): + """Gets the IP addresses of operational LIFs on the vserver.""" + + api_args = { + 'query': { + 'net-interface-info': { + 'operational-status': 'up' + } + }, + 'desired-attributes': { + 'net-interface-info': { + 'address': None, + } + } + } + result = self.send_request('net-interface-get-iter', api_args) + + lif_info_list = result.get_child_by_name( + 'attributes-list') or netapp_api.NaElement('none') + + return [lif_info.get_child_content('address') for lif_info in + lif_info_list.get_children()]