From 1eaad7fbd47445ace2c930deaeb2c727cd57c647 Mon Sep 17 00:00:00 2001 From: Taylor Jakobson Date: Wed, 24 May 2017 07:33:00 -0400 Subject: [PATCH] ISCSI volume attachment fixes This change: - adds support for multiple volume attachments based on LUN ID - handles the new exceptions raised by pypowervm during iSCSI discovery (and logout) - improves iSCSI attach unit tests - add additional parameters to discovery and remove calls - add support for multipath - add support for RR Change-Id: If9f3b581bcde57f9ce84b9a2cbe0bb2f9ecaa68c --- nova_powervm/conf/powervm.py | 11 +- nova_powervm/tests/virt/powervm/test_slot.py | 9 +- .../tests/virt/powervm/volume/test_iscsi.py | 131 ++++++++++++++---- nova_powervm/virt/powervm/driver.py | 1 + nova_powervm/virt/powervm/slot.py | 3 +- nova_powervm/virt/powervm/volume/__init__.py | 2 + nova_powervm/virt/powervm/volume/iscsi.py | 120 ++++++++++++++-- 7 files changed, 235 insertions(+), 42 deletions(-) diff --git a/nova_powervm/conf/powervm.py b/nova_powervm/conf/powervm.py index 4d425a59..68f70381 100644 --- a/nova_powervm/conf/powervm.py +++ b/nova_powervm/conf/powervm.py @@ -144,7 +144,16 @@ vol_adapter_opts = [ default=1, min=1, help='Indicates a minimum number of Virtual I/O Servers that ' 'are required to support a Cinder volume attach with the ' - 'vSCSI volume connector.') + 'vSCSI volume connector.'), + cfg.BoolOpt('volume_use_multipath', + default=False, + help="Use multipath connections when attaching iSCSI or FC"), + cfg.StrOpt('iscsi_iface', + default='default', + help="The iSCSI transport iface to use to connect to target in " + "case offload support is desired. Do not confuse the " + "iscsi_iface parameter to be provided here with the " + "actual transport name.") ] # NPIV Options. Only applicable if the 'fc_attach_strategy' is set to 'npiv'. diff --git a/nova_powervm/tests/virt/powervm/test_slot.py b/nova_powervm/tests/virt/powervm/test_slot.py index 0d7e8378..e1b3ff60 100644 --- a/nova_powervm/tests/virt/powervm/test_slot.py +++ b/nova_powervm/tests/virt/powervm/test_slot.py @@ -108,7 +108,8 @@ class TestSwiftSlotManager(test.TestCase): self.slot_mgr.init_recreate_map(mock.Mock(), self._vol_drv_iter()) self.assertEqual(1, mock_ftsk.call_count) mock_rebuild_slot.assert_called_once_with( - self.slot_mgr, mock.ANY, {'udid': ['uuid2']}, ['a', 'b']) + self.slot_mgr, mock.ANY, {'udid': ['uuid2'], 'iscsi': ['uuid1']}, + ['a', 'b']) @mock.patch('pypowervm.tasks.slot_map.RebuildSlotMap') @mock.patch('pypowervm.tasks.storage.ComprehensiveScrub') @@ -152,6 +153,10 @@ class TestSwiftSlotManager(test.TestCase): mock_scsi.vol_type.return_value = 'vscsi' mock_scsi.is_volume_on_vios.side_effect = ((False, None), (True, 'udid')) + mock_iscsi = mock.Mock() + mock_iscsi.vol_type.return_value = 'iscsi' + mock_iscsi.is_volume_on_vios.side_effect = ((True, 'iscsi'), + (False, None)) mock_npiv1 = mock.Mock() mock_npiv1.vol_type.return_value = 'npiv' @@ -161,6 +166,6 @@ class TestSwiftSlotManager(test.TestCase): mock_npiv2.vol_type.return_value = 'npiv' mock_npiv2._fabric_names.return_value = ['a', 'b', 'c'] - vol_drv = [mock_scsi, mock_npiv1, mock_npiv2] + vol_drv = [mock_scsi, mock_npiv1, mock_npiv2, mock_iscsi] for type in vol_drv: yield mock.Mock(), type diff --git a/nova_powervm/tests/virt/powervm/volume/test_iscsi.py b/nova_powervm/tests/virt/powervm/volume/test_iscsi.py index 969ef526..7d5d0a00 100644 --- a/nova_powervm/tests/virt/powervm/volume/test_iscsi.py +++ b/nova_powervm/tests/virt/powervm/volume/test_iscsi.py @@ -21,6 +21,7 @@ from nova_powervm.tests.virt.powervm.volume import test_driver as test_vol from nova_powervm.virt.powervm.volume import iscsi from pypowervm import const as pvm_const +from pypowervm import exceptions as pvm_exc from pypowervm.tasks import hdisk from pypowervm.tests.tasks.util import load_file from pypowervm.tests import test_fixtures as pvm_fx @@ -29,7 +30,7 @@ from pypowervm.wrappers import virtual_io_server as pvm_vios CONF = cfg.CONF -VIOS_FEED = 'fake_vios_feed2.txt' +VIOS_FEED = 'fake_vios_feed.txt' class TestISCSIAdapter(test_vol.TestVolumeAdapter): @@ -55,16 +56,16 @@ class TestISCSIAdapter(test_vol.TestVolumeAdapter): autospec=True) def init_vol_adpt(mock_initiator, mock_mgmt_part, mock_pvm_uuid, mock_vios): - self.trans_type = 'iscsi' - self.iqn = 'iqn.2016-08.bar.foo:target' - self.lun = '1' + self.iqn = 'iqn.2016-08.com.foo:bar' + self.lun = 1 self.host_ip = '10.0.0.1' self.user = 'user' self.password = 'password' self.serial = 'f042c68a-c5a5-476a-ba34-2f6d43f4226c' con_info = { 'serial': self.serial, - 'driver_volume_type': self.trans_type, + 'driver_volume_type': 'iscsi', + 'connector': {}, 'data': { 'target_iqn': self.iqn, 'target_lun': self.lun, @@ -73,15 +74,38 @@ class TestISCSIAdapter(test_vol.TestVolumeAdapter): 'auth_password': self.password }, } + self.auth_method = 'CHAP' + multi_con_info = { + 'serial': self.serial, + 'driver_volume_type': 'iser', + 'connector': {'multipath': True}, + 'data': { + 'target_iqn': self.iqn, + 'target_lun': self.lun, + 'target_portal': self.host_ip, + 'auth_method': self.auth_method, + 'auth_username': self.user, + 'auth_password': self.password, + 'discovery_auth_method': self.auth_method, + 'discovery_auth_username': self.user, + 'discovery_auth_password': self.password, + 'target_iqns': [self.iqn], + 'target_luns': [self.lun], + 'target_portals': [self.host_ip] + }, + } mock_inst = mock.MagicMock() mock_pvm_uuid.return_value = '1234' - mock_initiator.return_value = 'initiatior iqn' + mock_initiator.return_value = 'initiator iqn' # The getter can just return the VIOS values (to remove a read # that would otherwise need to be mocked). mock_vios.getter.return_value = self.feed - return iscsi.IscsiVolumeAdapter(self.adpt, 'host_uuid', mock_inst, - con_info) - self.vol_drv = init_vol_adpt() + single_path = iscsi.IscsiVolumeAdapter(self.adpt, 'host_uuid', + mock_inst, con_info) + multi_path = iscsi.IscsiVolumeAdapter(self.adpt, 'host_uuid', + mock_inst, multi_con_info) + return single_path, multi_path + self.vol_drv, self.multi_vol_drv = init_vol_adpt() # setup system_metadata tests self.devname = "/dev/fake" @@ -110,21 +134,66 @@ class TestISCSIAdapter(test_vol.TestVolumeAdapter): self.assertIsInstance(pv, pvm_stor.PV) self.assertEqual(62, lpar_slot_num) self.assertEqual('the_lua', lua) - self.assertEqual('ISCSI-target', target_name) + self.assertEqual('ISCSI-bar_%s' % self.lun, target_name) return 'fake_map' mock_build_map.side_effect = build_map_func - # Run the method self.vol_drv.connect_volume(self.slot_mgr) # As initialized above, remove_maps returns True to trigger update. - self.assertEqual(1, mock_add_map.call_count) - self.assertEqual(1, self.ft_fx.patchers['update'].mock.call_count) - self.assertEqual(1, mock_build_map.call_count) - mock_discover.assert_called_with( - self.adpt, self.host_ip, self.user, self.password, self.iqn, - self.feed[0].uuid, transport_type=self.trans_type) + self.assertEqual(2, mock_add_map.call_count) + self.assertEqual(2, self.ft_fx.patchers['update'].mock.call_count) + self.assertEqual(2, mock_build_map.call_count) + + calls = [mock.call(self.adpt, self.host_ip, self.user, self.password, + self.iqn, self.feed[0].uuid, lunid=self.lun, + multipath=False, iface_name='default', + discovery_auth=None, discovery_username=None, + auth=None, discovery_password=None), + mock.call(self.adpt, self.host_ip, self.user, self.password, + self.iqn, self.feed[1].uuid, lunid=self.lun, + multipath=False, iface_name='default', + discovery_auth=None, discovery_username=None, + auth=None, discovery_password=None)] + multi_calls = [ + mock.call(self.adpt, [self.host_ip], self.user, self.password, + [self.iqn], self.feed[0].uuid, lunid=[self.lun], + iface_name='iser', multipath=True, + auth=self.auth_method, discovery_auth=self.auth_method, + discovery_username=self.user, + discovery_password=self.password), + mock.call(self.adpt, [self.host_ip], self.user, self.password, + [self.iqn], self.feed[1].uuid, lunid=[self.lun], + iface_name='iser', multipath=True, + auth=self.auth_method, discovery_auth=self.auth_method, + discovery_username=self.user, + discovery_password=self.password)] + mock_discover.assert_has_calls(calls, any_order=True) + self.multi_vol_drv.connect_volume(self.slot_mgr) + mock_discover.assert_has_calls(multi_calls, any_order=True) + + @mock.patch('pypowervm.tasks.hdisk.discover_iscsi', autospec=True) + @mock.patch('nova_powervm.virt.powervm.vm.get_vm_id') + def test_connect_volume_discover_fail(self, mock_get_vm_id, mock_discover): + mock_get_vm_id.return_value = '2' + mock_discover.side_effect = pvm_exc.ISCSIDiscoveryFailed( + vios_uuid='fake_vios', status='fake_status') + + # Run the method + self.assertRaises(pvm_exc.MultipleExceptionsInFeedTask, + self.vol_drv.connect_volume, self.slot_mgr) + + @mock.patch('pypowervm.tasks.hdisk.discover_iscsi', autospec=True) + @mock.patch('nova_powervm.virt.powervm.vm.get_vm_id') + def test_connect_volume_job_fail(self, mock_get_vm_id, mock_discover): + mock_get_vm_id.return_value = '2' + mock_discover.side_effect = pvm_exc.JobRequestFailed( + operation_name='ISCSIDiscovery', error='fake_err') + + # Run the method + self.assertRaises(pvm_exc.MultipleExceptionsInFeedTask, + self.vol_drv.connect_volume, self.slot_mgr) @mock.patch('pypowervm.tasks.hdisk.remove_iscsi', autospec=True) @mock.patch('pypowervm.wrappers.virtual_io_server.VIOS.hdisk_from_uuid', @@ -135,12 +204,13 @@ class TestISCSIAdapter(test_vol.TestVolumeAdapter): mock_hdisk_from_uuid, mock_remove_iscsi): # The mock return values mock_hdisk_from_uuid.return_value = 'device_name' - mock_get_vm_id.return_value = 'partition_id' + mock_get_vm_id.return_value = '2' self.vol_drv._set_devname('/dev/fake') + self.multi_vol_drv._set_devname('/dev/fake') def validate_remove_maps(vios_w, vm_uuid, match_func): self.assertIsInstance(vios_w, pvm_vios.VIOS) - self.assertEqual('partition_id', vm_uuid) + self.assertEqual('2', vm_uuid) return 'removed' mock_remove_maps.side_effect = validate_remove_maps @@ -148,11 +218,24 @@ class TestISCSIAdapter(test_vol.TestVolumeAdapter): self.vol_drv.disconnect_volume(self.slot_mgr) # As initialized above, remove_maps returns True to trigger update. - self.assertEqual(1, mock_remove_maps.call_count) - fake_iqn = self.vol_drv.connection_info['data']['target_iqn'] - mock_remove_iscsi.assert_called_once_with( - self.adpt, fake_iqn, '3443DB77-AED1-47ED-9AA5-3DB9C6CF7089') - self.assertEqual(1, self.ft_fx.patchers['update'].mock.call_count) + self.assertEqual(2, mock_remove_maps.call_count) + self.assertEqual(2, self.ft_fx.patchers['update'].mock.call_count) + calls = [mock.call(self.adpt, self.iqn, self.feed[0].uuid, + lun=self.lun, iface_name='default', + portal=self.host_ip, multipath=False), + mock.call(self.adpt, self.iqn, self.feed[1].uuid, + lun=self.lun, iface_name='default', + portal=self.host_ip, multipath=False)] + multi_calls = [mock.call(self.adpt, [self.iqn], self.feed[0].uuid, + lun=[self.lun], iface_name='iser', + portal=[self.host_ip], multipath=True), + mock.call(self.adpt, [self.iqn], self.feed[1].uuid, + lun=[self.lun], iface_name='iser', + portal=[self.host_ip], multipath=True)] + mock_remove_iscsi.assert_has_calls(calls, any_order=True) + mock_remove_iscsi.reset_mock() + self.multi_vol_drv.disconnect_volume(self.slot_mgr) + mock_remove_iscsi.assert_has_calls(multi_calls, any_order=True) def test_min_xags(self): xags = self.vol_drv.min_xags() diff --git a/nova_powervm/virt/powervm/driver.py b/nova_powervm/virt/powervm/driver.py index 83220a2d..1e10cd6a 100644 --- a/nova_powervm/virt/powervm/driver.py +++ b/nova_powervm/virt/powervm/driver.py @@ -1085,6 +1085,7 @@ class PowerVMDriver(driver.ComputeDriver): self.adapter, self.host_uuid, instance) if wwpn_list is not None: connector["wwpns"] = wwpn_list + connector["multipath"] = CONF.powervm.volume_use_multipath connector['host'] = vol_attach.get_hostname_for_volume(instance) connector['initiator'] = vol_attach.get_iscsi_initiator(self.adapter) return connector diff --git a/nova_powervm/virt/powervm/slot.py b/nova_powervm/virt/powervm/slot.py index 71073a1d..aed83224 100644 --- a/nova_powervm/virt/powervm/slot.py +++ b/nova_powervm/virt/powervm/slot.py @@ -28,6 +28,7 @@ from nova_powervm.virt.powervm import exception as p_exc LOG = logging.getLogger(__name__) _SLOT_KEY = "CLIENT_SLOT_DATA" +_SLOT_VOLUME_TYPES = ['vscsi', 'fileio', 'rbd', 'iscsi'] def build_slot_mgr(instance, store_api, adapter=None, vol_drv_iter=None): @@ -124,7 +125,7 @@ class NovaSlotManager(slot_map.SlotMapStore): pv_vscsi_vol_to_vio = {} fabric_names = [] for bdm, vol_drv in vol_drv_iter: - if vol_drv.vol_type() in ['vscsi', 'fileio', 'rbd']: + if vol_drv.vol_type() in _SLOT_VOLUME_TYPES: self._pv_vscsi_vol_to_vio(vol_drv, pv_vscsi_vol_to_vio) elif len(fabric_names) == 0 and vol_drv.vol_type() == 'npiv': fabric_names = vol_drv._fabric_names() diff --git a/nova_powervm/virt/powervm/volume/__init__.py b/nova_powervm/virt/powervm/volume/__init__.py index 2063ef10..5a8a370f 100644 --- a/nova_powervm/virt/powervm/volume/__init__.py +++ b/nova_powervm/virt/powervm/volume/__init__.py @@ -37,6 +37,8 @@ FC_STRATEGY_MAPPING = { _STATIC_VOLUME_MAPPINGS = { 'iscsi': 'nova_powervm.virt.powervm.volume.iscsi.' 'IscsiVolumeAdapter', + 'iser': 'nova_powervm.virt.powervm.volume.iscsi.' + 'IscsiVolumeAdapter', 'local': 'nova_powervm.virt.powervm.volume.local.' 'LocalVolumeAdapter', 'nfs': 'nova_powervm.virt.powervm.volume.nfs.NFSVolumeAdapter', diff --git a/nova_powervm/virt/powervm/volume/iscsi.py b/nova_powervm/virt/powervm/volume/iscsi.py index e0e3bb33..59eb5e67 100644 --- a/nova_powervm/virt/powervm/volume/iscsi.py +++ b/nova_powervm/virt/powervm/volume/iscsi.py @@ -13,6 +13,7 @@ # WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the # License for the specific language governing permissions and limitations # under the License. +import copy from oslo_concurrency import lockutils from oslo_log import log as logging @@ -22,6 +23,7 @@ from nova_powervm.virt.powervm import vm from nova_powervm.virt.powervm.volume import driver as v_driver from nova_powervm.virt.powervm.volume import volume as volume from pypowervm import const as pvm_const +from pypowervm import exceptions as pvm_exc from pypowervm.tasks import hdisk from pypowervm.utils import transaction as tx from pypowervm.wrappers import virtual_io_server as pvm_vios @@ -48,6 +50,10 @@ class IscsiVolumeAdapter(volume.VscsiVolumeAdapter, stg_ftsk=None): super(IscsiVolumeAdapter, self).__init__( adapter, host_uuid, instance, connection_info, stg_ftsk=stg_ftsk) + if connection_info['driver_volume_type'] == 'iser': + self.iface_name = 'iser' + else: + self.iface_name = CONF.powervm.iscsi_iface @classmethod def vol_type(cls): @@ -79,6 +85,58 @@ class IscsiVolumeAdapter(volume.VscsiVolumeAdapter, """ raise NotImplementedError() + def is_volume_on_vios(self, vios_w): + """Returns whether or not the volume is on a VIOS. + + :param vios_w: The Virtual I/O Server wrapper. + :return: True if the volume driver's volume is on the VIOS. False + otherwise. + :return: The udid of the device. + """ + device_name, udid = self._discover_volume_on_vios(vios_w) + return (device_name and udid) is not None, udid + + def _is_multipath(self): + return self.connection_info["connector"].get("multipath", False) + + def _discover_vol(self, vios_w, props): + portal = props.get("target_portals", props.get("target_portal")) + iqn = props.get("target_iqns", props.get("target_iqn")) + lun = props.get("target_luns", props.get("target_lun")) + auth = props.get("auth_method") + user = props.get("auth_username") + password = props.get("auth_password") + discovery_auth = props.get("discovery_auth_method") + discovery_username = props.get("discovery_auth_username") + discovery_password = props.get("discovery_auth_password") + try: + return hdisk.discover_iscsi( + self.adapter, portal, user, password, iqn, vios_w.uuid, + lunid=lun, iface_name=self.iface_name, auth=auth, + discovery_auth=discovery_auth, + discovery_username=discovery_username, + discovery_password=discovery_password, + multipath=self._is_multipath()) + except (pvm_exc.ISCSIDiscoveryFailed, pvm_exc.JobRequestFailed) as e: + LOG.warning(e) + + def _discover_volume_on_vios(self, vios_w): + """Discovers an hdisk on a single vios for the volume. + + :param vios_w: VIOS wrapper to process + :returns: Device name or None + :returns: LUN or None + """ + device_name = udid = None + if self._is_multipath(): + device_name, udid = self._discover_vol( + vios_w, self.connection_info["data"]) + else: + for props in self._iterate_all_targets( + self.connection_info["data"]): + device_name, udid = self._discover_vol(vios_w, props) + return device_name, udid + def _connect_volume_to_vio(self, vios_w, slot_mgr): """Attempts to connect a volume to a given VIO. @@ -90,18 +148,13 @@ class IscsiVolumeAdapter(volume.VscsiVolumeAdapter, not (could be the Virtual I/O Server does not have connectivity to the hdisk). """ - transport_type = self.connection_info["driver_volume_type"] - host_ip = self.connection_info["data"]["target_portal"] - iqn = self.connection_info["data"]["target_iqn"] - password = self.connection_info["data"]["auth_password"] - user = self.connection_info["data"]["auth_username"] - target_name = "ISCSI-" + iqn.split(":")[1] - device_name, udid = hdisk.discover_iscsi( - self.adapter, host_ip, user, password, iqn, vios_w.uuid, - transport_type=transport_type) - slot, lua = slot_mgr.build_map.get_vscsi_slot(vios_w, device_name) + device_name, udid = self._discover_volume_on_vios(vios_w) if device_name is not None and udid is not None: + slot, lua = slot_mgr.build_map.get_vscsi_slot(vios_w, device_name) device_name = '/dev/' + device_name + iqn = self.connection_info["data"]["target_iqn"] + lun = self.connection_info["data"]["target_lun"] + target_name = "ISCSI-%s_%s" % (iqn.split(":")[1], str(lun)) # Found a hdisk on this Virtual I/O Server. Add the action to # map it to the VM when the stg_ftsk is executed. with lockutils.lock(hash(self)): @@ -183,14 +236,30 @@ class IscsiVolumeAdapter(volume.VscsiVolumeAdapter, with lockutils.lock(hash(self)): self._add_remove_mapping(partition_id, vios_w.uuid, device_name, slot_mgr) - target_iqn = self.connection_info["data"]["target_iqn"] + conn_data = self.connection_info["data"] + iqn = conn_data.get("target_iqns", conn_data.get("target_iqn")) + portal = conn_data.get("target_portals", + conn_data.get("target_portal")) + lun = conn_data.get("target_luns", + conn_data.get("target_lun")) + + def remove(): + try: + hdisk.remove_iscsi( + self.adapter, iqn, vios_w.uuid, lun=lun, + iface_name=self.iface_name, portal=portal, + multipath=self._is_multipath()) + except (pvm_exc.ISCSIRemoveFailed, + pvm_exc.JobRequestFailed) as e: + LOG.warning(e) - def logout(): - hdisk.remove_iscsi(self.adapter, target_iqn, vios_w.uuid) self.stg_ftsk.add_post_execute(task.FunctorTask( - logout, name='remove_iSCSI_%s' % target_iqn)) + remove, name='remove_%s_from_vios_%s' % (device_name, + vios_w.uuid))) + # Found a valid element to remove return True + try: # See logic in _connect_volume for why this new FeedTask is here. discon_ftsk = tx.FeedTask( @@ -215,3 +284,26 @@ class IscsiVolumeAdapter(volume.VscsiVolumeAdapter, ex_args = {'volume_id': self.volume_id, 'reason': six.text_type(e), 'instance_name': self.instance.name} raise p_exc.VolumeDetachFailed(**ex_args) + + # Taken from os_brick.initiator.connectors.base_iscsi.py + def _iterate_all_targets(self, connection_properties): + for portal, iqn, lun in self._get_all_targets(connection_properties): + props = copy.deepcopy(connection_properties) + props['target_portal'] = portal + props['target_iqn'] = iqn + props['target_lun'] = lun + for key in ('target_portals', 'target_iqns', 'target_luns'): + props.pop(key, None) + yield props + + 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))]