From 614c82fe160d8e776cd6acc6f6f1b12c63ddfb0f Mon Sep 17 00:00:00 2001 From: Navneet Singh Date: Thu, 2 Oct 2014 00:01:41 +0530 Subject: [PATCH] Fix LUN misalignment issue with NetApp iSCSI drivers This patch fixes LUN misalignment issues that can be experienced when provisioning Cinder volumes with the iSCSI protocol. In the fix, two new SAN options for configuring LUN OS and initiator OS used while attaching volume can be specified. This gives the admin flexibility to configure backends correctly for multiple hypervisor and guest OS platforms. Closes-Bug: 1410107 Change-Id: If09e1ad41880e8070afddde5606c6536b03396fe --- cinder/tests/test_netapp_eseries_iscsi.py | 9 +- .../netapp/dataontap/test_block_base.py | 107 ++++++++++++++++-- .../drivers/netapp/dataontap/block_base.py | 98 ++++++++++------ cinder/volume/drivers/netapp/eseries/iscsi.py | 10 +- cinder/volume/drivers/netapp/options.py | 22 ++-- 5 files changed, 190 insertions(+), 56 deletions(-) diff --git a/cinder/tests/test_netapp_eseries_iscsi.py b/cinder/tests/test_netapp_eseries_iscsi.py index e972adc91..35b91efdb 100644 --- a/cinder/tests/test_netapp_eseries_iscsi.py +++ b/cinder/tests/test_netapp_eseries_iscsi.py @@ -45,6 +45,7 @@ def create_configuration(): configuration = conf.Configuration(None) configuration.append_config_values(options.netapp_basicauth_opts) configuration.append_config_values(options.netapp_eseries_opts) + configuration.append_config_values(options.netapp_san_opts) return configuration @@ -948,11 +949,17 @@ class NetAppEseriesISCSIDriverTestCase(test.TestCase): def test_setup_error_unsupported_host_type(self): configuration = self._set_config(create_configuration()) - configuration.netapp_eseries_host_type = 'garbage' + configuration.netapp_host_type = 'garbage' driver = common.NetAppDriver(configuration=configuration) self.assertRaises(exception.NetAppDriverException, driver.check_for_setup_error) + def test_check_host_type_default(self): + configuration = self._set_config(create_configuration()) + driver = common.NetAppDriver(configuration=configuration) + driver._check_host_type() + self.assertEqual('LnxALUA', driver.host_type) + def test_do_setup_all_default(self): configuration = self._set_config(create_configuration()) driver = common.NetAppDriver(configuration=configuration) 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 6637307ce..91779f9c0 100644 --- a/cinder/tests/volume/drivers/netapp/dataontap/test_block_base.py +++ b/cinder/tests/volume/drivers/netapp/dataontap/test_block_base.py @@ -25,6 +25,7 @@ import uuid import mock from cinder import exception +from cinder.i18n import _ from cinder import test from cinder.tests.volume.drivers.netapp.dataontap import fakes as fake from cinder.volume.drivers.netapp.dataontap import block_base @@ -99,8 +100,10 @@ class NetAppBlockStorageLibraryTestCase(test.TestCase): def test_map_lun(self, mock_get_or_create_igroup, mock_get_lun_attr): os = 'linux' protocol = 'fcp' + self.library.host_type = 'linux' mock_get_lun_attr.return_value = {'Path': fake.LUN1, 'OsType': os} - mock_get_or_create_igroup.return_value = fake.IGROUP1_NAME + mock_get_or_create_igroup.return_value = (fake.IGROUP1_NAME, os, + 'iscsi') self.zapi_client.map_lun.return_value = '1' lun_id = self.library._map_lun('fake_volume', @@ -113,6 +116,28 @@ class NetAppBlockStorageLibraryTestCase(test.TestCase): self.zapi_client.map_lun.assert_called_once_with( fake.LUN1, fake.IGROUP1_NAME, lun_id=None) + @mock.patch.object(block_base.NetAppBlockStorageLibrary, '_get_lun_attr') + @mock.patch.object(block_base.NetAppBlockStorageLibrary, + '_get_or_create_igroup') + @mock.patch.object(block_base, 'LOG', mock.Mock()) + def test_map_lun_mismatch_host_os( + self, mock_get_or_create_igroup, mock_get_lun_attr): + os = 'windows' + protocol = 'fcp' + self.library.host_type = 'linux' + mock_get_lun_attr.return_value = {'Path': fake.LUN1, 'OsType': os} + mock_get_or_create_igroup.return_value = (fake.IGROUP1_NAME, os, + 'iscsi') + self.library._map_lun('fake_volume', + fake.FC_FORMATTED_INITIATORS, + protocol, None) + mock_get_or_create_igroup.assert_called_once_with( + fake.FC_FORMATTED_INITIATORS, protocol, + self.library.host_type) + self.zapi_client.map_lun.assert_called_once_with( + fake.LUN1, fake.IGROUP1_NAME, lun_id=None) + self.assertEqual(1, block_base.LOG.warning.call_count) + @mock.patch.object(block_base.NetAppBlockStorageLibrary, '_get_lun_attr') @mock.patch.object(block_base.NetAppBlockStorageLibrary, @@ -124,7 +149,8 @@ class NetAppBlockStorageLibraryTestCase(test.TestCase): os = 'linux' protocol = 'fcp' mock_get_lun_attr.return_value = {'Path': fake.LUN1, 'OsType': os} - mock_get_or_create_igroup.return_value = fake.IGROUP1_NAME + mock_get_or_create_igroup.return_value = (fake.IGROUP1_NAME, os, + 'iscsi') mock_find_mapped_lun_igroup.return_value = (fake.IGROUP1_NAME, '2') self.zapi_client.map_lun.side_effect = netapp_api.NaApiError @@ -146,7 +172,8 @@ class NetAppBlockStorageLibraryTestCase(test.TestCase): os = 'linux' protocol = 'fcp' mock_get_lun_attr.return_value = {'Path': fake.LUN1, 'OsType': os} - mock_get_or_create_igroup.return_value = fake.IGROUP1_NAME + mock_get_or_create_igroup.return_value = (fake.IGROUP1_NAME, os, + 'iscsi') mock_find_mapped_lun_igroup.return_value = (None, None) self.zapi_client.map_lun.side_effect = netapp_api.NaApiError @@ -179,19 +206,24 @@ class NetAppBlockStorageLibraryTestCase(test.TestCase): def test_get_or_create_igroup_preexisting(self): self.zapi_client.get_igroup_by_initiators.return_value = [fake.IGROUP1] - - igroup_name = self.library._get_or_create_igroup( + self.library._create_igroup_add_initiators = mock.Mock() + igroup_name, host_os, ig_type = self.library._get_or_create_igroup( fake.FC_FORMATTED_INITIATORS, 'fcp', 'linux') - self.assertEqual(igroup_name, fake.IGROUP1_NAME) + self.assertEqual(fake.IGROUP1_NAME, igroup_name) + self.assertEqual('linux', host_os) + self.assertEqual('fcp', ig_type) self.zapi_client.get_igroup_by_initiators.assert_called_once_with( fake.FC_FORMATTED_INITIATORS) + self.assertEqual( + 0, self.library._create_igroup_add_initiators.call_count) @mock.patch.object(uuid, 'uuid4', mock.Mock(return_value=fake.UUID1)) def test_get_or_create_igroup_none_preexisting(self): + """This method also tests _create_igroup_add_initiators.""" self.zapi_client.get_igroup_by_initiators.return_value = [] - igroup_name = self.library._get_or_create_igroup( + igroup_name, os, ig_type = self.library._get_or_create_igroup( fake.FC_FORMATTED_INITIATORS, 'fcp', 'linux') self.assertEqual(igroup_name, 'openstack-' + fake.UUID1) @@ -199,6 +231,8 @@ class NetAppBlockStorageLibraryTestCase(test.TestCase): igroup_name, 'fcp', 'linux') self.assertEqual(len(fake.FC_FORMATTED_INITIATORS), self.zapi_client.add_igroup_initiator.call_count) + self.assertEqual('linux', os) + self.assertEqual('fcp', ig_type) def test_get_fc_target_wwpns(self): self.assertRaises(NotImplementedError, @@ -354,10 +388,22 @@ class NetAppBlockStorageLibraryTestCase(test.TestCase): na_utils.LOG.warning.assert_called_once_with(warn_msg) @mock.patch.object(na_utils, 'check_flags') - def test_do_setup(self, mock_check_flags): + def test_do_setup_san_configured(self, mock_check_flags): + self.library.configuration.netapp_lun_ostype = 'windows' + self.library.configuration.netapp_host_type = 'solaris' self.library.do_setup(mock.Mock()) + self.assertTrue(mock_check_flags.called) + self.assertEqual('windows', self.library.lun_ostype) + self.assertEqual('solaris', self.library.host_type) + @mock.patch.object(na_utils, 'check_flags') + def test_do_setup_san_unconfigured(self, mock_check_flags): + self.library.configuration.netapp_lun_ostype = None + self.library.configuration.netapp_host_type = None + self.library.do_setup(mock.Mock()) self.assertTrue(mock_check_flags.called) + self.assertEqual('linux', self.library.lun_ostype) + self.assertEqual('linux', self.library.host_type) def test_get_existing_vol_manage_missing_id_path(self): self.assertRaises(exception.ManageExistingInvalidReference, @@ -587,3 +633,48 @@ class NetAppBlockStorageLibraryTestCase(test.TestCase): target_details_list, filter) self.assertEqual(target_details_list[1], result) + + @mock.patch.object(na_utils, 'check_flags', mock.Mock()) + @mock.patch.object(block_base, 'LOG', mock.Mock()) + def test_setup_error_invalid_lun_os(self): + self.library.configuration.netapp_lun_ostype = 'unknown' + self.library.do_setup(mock.Mock()) + self.assertRaises(exception.NetAppDriverException, + self.library.check_for_setup_error) + msg = _("Invalid value for NetApp configuration" + " option netapp_lun_ostype.") + block_base.LOG.error.assert_called_once_with(msg) + + @mock.patch.object(na_utils, 'check_flags', mock.Mock()) + @mock.patch.object(block_base, 'LOG', mock.Mock()) + def test_setup_error_invalid_host_type(self): + self.library.configuration.netapp_lun_ostype = 'linux' + self.library.configuration.netapp_host_type = 'future_os' + self.library.do_setup(mock.Mock()) + self.assertRaises(exception.NetAppDriverException, + self.library.check_for_setup_error) + msg = _("Invalid value for NetApp configuration" + " option netapp_host_type.") + block_base.LOG.error.assert_called_once_with(msg) + + @mock.patch.object(na_utils, 'check_flags', mock.Mock()) + def test_check_for_setup_error_both_config(self): + self.library.configuration.netapp_lun_ostype = 'linux' + self.library.configuration.netapp_host_type = 'linux' + self.library.do_setup(mock.Mock()) + self.zapi_client.get_lun_list.return_value = ['lun1'] + self.library._extract_and_populate_luns = mock.Mock() + self.library.check_for_setup_error() + self.library._extract_and_populate_luns.assert_called_once_with( + ['lun1']) + + @mock.patch.object(na_utils, 'check_flags', mock.Mock()) + def test_check_for_setup_error_no_os_host(self): + self.library.configuration.netapp_lun_ostype = None + self.library.configuration.netapp_host_type = None + self.library.do_setup(mock.Mock()) + self.zapi_client.get_lun_list.return_value = ['lun1'] + self.library._extract_and_populate_luns = mock.Mock() + self.library.check_for_setup_error() + self.library._extract_and_populate_luns.assert_called_once_with( + ['lun1']) diff --git a/cinder/volume/drivers/netapp/dataontap/block_base.py b/cinder/volume/drivers/netapp/dataontap/block_base.py index 27633d627..d6c2057ad 100644 --- a/cinder/volume/drivers/netapp/dataontap/block_base.py +++ b/cinder/volume/drivers/netapp/dataontap/block_base.py @@ -73,6 +73,14 @@ class NetAppBlockStorageLibrary(object): IGROUP_PREFIX = 'openstack-' REQUIRED_FLAGS = ['netapp_login', 'netapp_password', 'netapp_server_hostname'] + ALLOWED_LUN_OS_TYPES = ['linux', 'aix', 'hpux', 'image', 'windows', + 'windows_2008', 'windows_gpt', 'solaris', + 'solaris_efi', 'netware', 'openvms', 'hyper_v'] + ALLOWED_IGROUP_HOST_TYPES = ['linux', 'aix', 'hpux', 'windows', 'solaris', + 'netware', 'default', 'vmware', 'openvms', + 'xen', 'hyper_v'] + DEFAULT_LUN_OS = 'linux' + DEFAULT_HOST_TYPE = 'linux' def __init__(self, driver_name, driver_protocol, **kwargs): @@ -83,6 +91,8 @@ class NetAppBlockStorageLibrary(object): self.zapi_client = None self._stats = {} self.lun_table = {} + self.lun_ostype = None + self.host_type = None self.lookup_service = fczm_utils.create_lookup_service() self.app_version = kwargs.get("app_version", "unknown") self.db = kwargs.get('db') @@ -93,16 +103,30 @@ class NetAppBlockStorageLibrary(object): self.configuration.append_config_values(na_opts.netapp_transport_opts) self.configuration.append_config_values( na_opts.netapp_provisioning_opts) + self.configuration.append_config_values(na_opts.netapp_san_opts) def do_setup(self, context): na_utils.check_flags(self.REQUIRED_FLAGS, self.configuration) + self.lun_ostype = (self.configuration.netapp_lun_ostype + or self.DEFAULT_LUN_OS) + self.host_type = (self.configuration.netapp_host_type + or self.DEFAULT_HOST_TYPE) def check_for_setup_error(self): """Check that the driver is working and can communicate. Discovers the LUNs on the NetApp server. """ - + if self.lun_ostype not in self.ALLOWED_LUN_OS_TYPES: + msg = _("Invalid value for NetApp configuration" + " option netapp_lun_ostype.") + LOG.error(msg) + raise exception.NetAppDriverException(msg) + if self.host_type not in self.ALLOWED_IGROUP_HOST_TYPES: + msg = _("Invalid value for NetApp configuration" + " option netapp_host_type.") + LOG.error(msg) + raise exception.NetAppDriverException(msg) lun_list = self.zapi_client.get_lun_list() self._extract_and_populate_luns(lun_list) LOG.debug("Success getting list of LUNs from server.") @@ -137,7 +161,7 @@ class NetAppBlockStorageLibrary(object): size = default_size if not int(volume['size'])\ else int(volume['size']) * units.Gi - metadata = {'OsType': 'linux', + metadata = {'OsType': self.lun_ostype, 'SpaceReserved': 'true', 'Path': '/vol/%s/%s' % (ontap_volume_name, lun_name)} @@ -257,14 +281,16 @@ class NetAppBlockStorageLibrary(object): def _map_lun(self, name, initiator_list, initiator_type, lun_id=None): """Maps LUN to the initiator(s) and returns LUN ID assigned.""" metadata = self._get_lun_attr(name, 'metadata') - os = metadata['OsType'] path = metadata['Path'] - if self._check_allowed_os(os): - os = os - else: - os = 'default' - igroup_name = self._get_or_create_igroup(initiator_list, - initiator_type, os) + igroup_name, ig_host_os, ig_type = self._get_or_create_igroup( + initiator_list, initiator_type, self.host_type) + if ig_host_os != self.host_type: + LOG.warning(_LW("LUN misalignment may occur for current" + " initiator group %(ig_nm)s) with host OS type" + " %(ig_os)s. Please configure initiator group" + " manually according to the type of the" + " host OS."), + {'ig_nm': igroup_name, 'ig_os': ig_host_os}) try: return self.zapi_client.map_lun(path, igroup_name, lun_id=lun_id) except na_api.NaApiError: @@ -290,39 +316,37 @@ class NetAppBlockStorageLibrary(object): """Checks whether any LUNs are mapped to the given initiator(s).""" return self.zapi_client.has_luns_mapped_to_initiators(initiator_list) - def _get_or_create_igroup(self, initiator_list, initiator_type, - os='default'): + def _get_or_create_igroup(self, initiator_list, initiator_group_type, + host_os_type): """Checks for an igroup for a set of one or more initiators. - Creates igroup if not found. + Creates igroup if not already present with given host os type, + igroup type and adds initiators. """ - igroups = self.zapi_client.get_igroup_by_initiators(initiator_list) - igroup_name = None - for igroup in igroups: - if igroup['initiator-group-os-type'] == os: - if igroup['initiator-group-type'] == initiator_type or \ - igroup['initiator-group-type'] == 'mixed': - if igroup['initiator-group-name'].startswith( - self.IGROUP_PREFIX): - igroup_name = igroup['initiator-group-name'] - break + + if igroups: + igroup = igroups[0] + igroup_name = igroup['initiator-group-name'] + host_os_type = igroup['initiator-group-os-type'] + initiator_group_type = igroup['initiator-group-type'] + if not igroup_name: - igroup_name = self.IGROUP_PREFIX + six.text_type(uuid.uuid4()) - self.zapi_client.create_igroup(igroup_name, initiator_type, os) - for initiator in initiator_list: - self.zapi_client.add_igroup_initiator(igroup_name, initiator) + igroup_name = self._create_igroup_add_initiators( + initiator_group_type, host_os_type, initiator_list) + return igroup_name, host_os_type, initiator_group_type + + def _create_igroup_add_initiators(self, initiator_group_type, + host_os_type, initiator_list): + """Creates igroup and adds initiators.""" + igroup_name = self.IGROUP_PREFIX + six.text_type(uuid.uuid4()) + self.zapi_client.create_igroup(igroup_name, initiator_group_type, + host_os_type) + for initiator in initiator_list: + self.zapi_client.add_igroup_initiator(igroup_name, initiator) return igroup_name - def _check_allowed_os(self, os): - """Checks if the os type supplied is NetApp supported.""" - if os in ['linux', 'aix', 'hpux', 'windows', 'solaris', - 'netware', 'vmware', 'openvms', 'xen', 'hyper_v']: - return True - else: - return False - def _add_lun_to_table(self, lun): """Adds LUN to cache table.""" if not isinstance(lun, NetAppLun): @@ -609,7 +633,7 @@ class NetAppBlockStorageLibrary(object): 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 = "Mapped LUN %(name)s to the initiator %(initiator_name)s" msg_fmt = {'name': name, 'initiator_name': initiator_name} LOG.debug(msg % msg_fmt) @@ -618,8 +642,8 @@ class NetAppBlockStorageLibrary(object): msg = _('Failed to get LUN target list for the LUN %s') raise exception.VolumeBackendAPIException(data=msg % name) - msg = _("Successfully fetched target list for LUN %(name)s and " - "initiator %(initiator_name)s") + 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) diff --git a/cinder/volume/drivers/netapp/eseries/iscsi.py b/cinder/volume/drivers/netapp/eseries/iscsi.py index 0cb3e1959..eeb1a2663 100644 --- a/cinder/volume/drivers/netapp/eseries/iscsi.py +++ b/cinder/volume/drivers/netapp/eseries/iscsi.py @@ -50,6 +50,7 @@ CONF.register_opts(na_opts.netapp_basicauth_opts) CONF.register_opts(na_opts.netapp_connection_opts) CONF.register_opts(na_opts.netapp_eseries_opts) CONF.register_opts(na_opts.netapp_transport_opts) +CONF.register_opts(na_opts.netapp_san_opts) class NetAppEseriesISCSIDriver(driver.ISCSIDriver): @@ -93,6 +94,8 @@ class NetAppEseriesISCSIDriver(driver.ISCSIDriver): SSC_UPDATE_INTERVAL = 60 # seconds WORLDWIDENAME = 'worldWideName' + DEFAULT_HOST_TYPE = 'linux_dm_mp' + def __init__(self, *args, **kwargs): super(NetAppEseriesISCSIDriver, self).__init__(*args, **kwargs) na_utils.validate_instantiation(**kwargs) @@ -101,6 +104,7 @@ class NetAppEseriesISCSIDriver(driver.ISCSIDriver): na_opts.netapp_connection_opts) self.configuration.append_config_values(na_opts.netapp_transport_opts) self.configuration.append_config_values(na_opts.netapp_eseries_opts) + self.configuration.append_config_values(na_opts.netapp_san_opts) self._backend_name = self.configuration.safe_get( "volume_backend_name") or "NetApp_ESeries" self._objects = {'disk_pool_refs': [], 'pools': [], @@ -143,9 +147,9 @@ class NetAppEseriesISCSIDriver(driver.ISCSIDriver): self._start_periodic_tasks() def _check_host_type(self): - self.host_type =\ - self.HOST_TYPES.get(self.configuration.netapp_eseries_host_type, - None) + host_type = (self.configuration.netapp_host_type + or self.DEFAULT_HOST_TYPE) + self.host_type = self.HOST_TYPES.get(host_type) if not self.host_type: raise exception.NetAppDriverException( _('Configured host type is not supported.')) diff --git a/cinder/volume/drivers/netapp/options.py b/cinder/volume/drivers/netapp/options.py index bc8459cfc..e85979bd0 100644 --- a/cinder/volume/drivers/netapp/options.py +++ b/cinder/volume/drivers/netapp/options.py @@ -159,13 +159,7 @@ netapp_eseries_opts = [ 'specified storage pools. Only dynamic disk pools are ' 'currently supported. Specify the value of this option to' ' be a comma separated list of disk pool names to be used' - ' for provisioning.')), - cfg.StrOpt('netapp_eseries_host_type', - default='linux_dm_mp', - help=('This option is used to define how the controllers in ' - 'the E-Series storage array will work with the ' - 'particular operating system on the hosts that are ' - 'connected to it.')), ] + ' for provisioning.')), ] netapp_nfs_extra_opts = [ cfg.StrOpt('netapp_copyoffload_tool_path', default=None, @@ -173,6 +167,19 @@ netapp_nfs_extra_opts = [ 'offload tool binary. Ensure that the binary has execute ' 'permissions set which allow the effective user of the ' 'cinder-volume process to execute the file.')), ] +netapp_san_opts = [ + cfg.StrOpt('netapp_lun_ostype', + default=None, + help=('This option defines the type of operating system that' + ' will access a LUN exported from Data ONTAP; it is' + ' assigned to the LUN at the time it is created.')), + cfg.StrOpt('netapp_host_type', + deprecated_name='netapp_eseries_host_type', + default=None, + help=('This option defines the type of operating system for' + ' all initiators that can access a LUN. This information' + ' is used when mapping LUNs to individual hosts or' + ' groups of hosts.'))] CONF = cfg.CONF CONF.register_opts(netapp_proxy_opts) @@ -185,3 +192,4 @@ CONF.register_opts(netapp_provisioning_opts) CONF.register_opts(netapp_img_cache_opts) CONF.register_opts(netapp_eseries_opts) CONF.register_opts(netapp_nfs_extra_opts) +CONF.register_opts(netapp_san_opts) -- 2.45.2