HyperV: use os-brick for volume related operations

This patch refactors volumeops.py to use os-brick for volume
related operations.

The immediate benefits are:
* FC support
* improved iSCSI MPIO support
* simplified volume connect/disconnect workflow

Change-Id: Ib21947141aadca1fa6cb99afc07a175ce14d192e
Depends-On: I19dfc8dd2e9e8a1b17675b55c63de903804480e4
Implements: blueprint hyperv-use-os-brick

nova/conf/hyperv.py

@@ -84,11 +84,9 @@ in order to limit the CPU features used by the instance.
         help="""
 Mounted disk query retry count
 
-The number of times to retry checking for a disk mounted via iSCSI.
+The number of times to retry checking for a mounted disk.
-During long stress runs the WMI query that is looking for the iSCSI
+The query runs until the device can be found or the retry
-device number can incorrectly return no data. If the query is
+count is reached.
-retried the appropriate data can then be obtained. The query runs
-until the device can be found or the retry count is reached.
 
 Possible values:
 
@@ -106,7 +104,7 @@ Related options:
         help="""
 Mounted disk query retry interval
 
-Interval between checks for a mounted iSCSI disk, in seconds.
+Interval between checks for a mounted disk, in seconds.
 
 Possible values:
 
@@ -259,12 +257,8 @@ Related options:
         help="""
 Volume attach retry count
 
-The number of times to retry to attach a volume. This option is used
+The number of times to retry attaching a volume. Volume attachment
-to avoid incorrectly returned no data when the system is under load.
+is retried until success or the given retry count is reached.
-Volume attachment is retried until success or the given retry count
-is reached. To prepare the Hyper-V node to be able to attach to
-volumes provided by cinder you must first make sure the Windows iSCSI
-initiator service is running and started automatically.
 
 Possible values:
 
@@ -321,6 +315,22 @@ extra specs:
 Windows / Hyper-V Server 2016. Acceptable values::
 
     64, 128, 256, 512, 1024
+"""),
+    cfg.BoolOpt('use_multipath_io',
+                default=False,
+                help="""
+Use multipath connections when attaching iSCSI or FC disks.
+
+This requires the Multipath IO Windows feature to be enabled. MPIO must be
+configured to claim such devices.
+"""),
+    cfg.ListOpt('iscsi_initiator_list',
+                default=[],
+                help="""
+List of iSCSI initiators that will be used for estabilishing iSCSI sessions.
+
+If none are specified, the Microsoft iSCSI initiator service will choose the
+initiator.
 """)
 ]
 

nova/tests/unit/virt/hyperv/test_driver.py

@@ -196,8 +196,7 @@ class HyperVDriverTestCase(test_base.HyperVBaseTestCase):
 
     def test_get_volume_connector(self):
         self.driver.get_volume_connector(mock.sentinel.instance)
-        self.driver._volumeops.get_volume_connector.assert_called_once_with(
+        self.driver._volumeops.get_volume_connector.assert_called_once_with()
-            mock.sentinel.instance)
 
     def test_get_available_resource(self):
         self.driver.get_available_resource(mock.sentinel.nodename)

nova/tests/unit/virt/hyperv/test_livemigrationops.py

@@ -135,8 +135,7 @@ class LiveMigrationOpsTestCase(test_base.HyperVBaseTestCase):
 
     @mock.patch('nova.virt.hyperv.volumeops.VolumeOps.get_disk_path_mapping')
     @mock.patch('nova.virt.hyperv.imagecache.ImageCache.get_cached_image')
-    @mock.patch('nova.virt.hyperv.volumeops.VolumeOps'
+    @mock.patch('nova.virt.hyperv.volumeops.VolumeOps.connect_volumes')
-                '.initialize_volumes_connection')
     def _test_pre_live_migration(self, mock_initialize_connection,
                                  mock_get_cached_image,
                                  mock_get_disk_path_mapping,

nova/tests/unit/virt/hyperv/test_volumeops.py

@@ -14,10 +14,8 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
-import os
-
 import mock
-from os_win import exceptions as os_win_exc
+from os_brick.initiator import connector
 from oslo_config import cfg
 from oslo_utils import units
 
@@ -138,58 +136,114 @@ class VolumeOpsTestCase(test_base.HyperVBaseTestCase):
     def test_disconnect_volumes(self, mock_get_volume_driver):
         block_device_info = get_fake_block_dev_info()
         block_device_mapping = block_device_info['block_device_mapping']
-        block_device_mapping[0]['connection_info'] = {
-            'driver_volume_type': mock.sentinel.fake_vol_type}
         fake_volume_driver = mock_get_volume_driver.return_value
 
         self._volumeops.disconnect_volumes(block_device_info)
-        fake_volume_driver.disconnect_volumes.assert_called_once_with(
+        fake_volume_driver.disconnect_volume.assert_called_once_with(
-            block_device_mapping)
+            block_device_mapping[0]['connection_info'])
 
+    @mock.patch('time.sleep')
     @mock.patch.object(volumeops.VolumeOps, '_get_volume_driver')
-    def test_attach_volume(self, mock_get_volume_driver):
+    def _test_attach_volume(self, mock_get_volume_driver, mock_sleep,
-        fake_conn_info = {
+                            attach_failed):
-            'data': {'qos_specs': mock.sentinel.qos_specs}
+        fake_conn_info = get_fake_connection_info(
-        }
+            qos_specs=mock.sentinel.qos_specs)
+        fake_volume_driver = mock_get_volume_driver.return_value
 
-        mock_volume_driver = mock_get_volume_driver.return_value
+        expected_try_count = 1
+        if attach_failed:
+            expected_try_count += CONF.hyperv.volume_attach_retry_count
 
-        self._volumeops.attach_volume(fake_conn_info,
+            fake_volume_driver.set_disk_qos_specs.side_effect = (
-                                      mock.sentinel.instance_name,
+                test.TestingException)
-                                      disk_bus=mock.sentinel.disk_bus)
 
-        mock_volume_driver.attach_volume.assert_called_once_with(
+            self.assertRaises(exception.VolumeAttachFailed,
+                              self._volumeops.attach_volume,
                               fake_conn_info,
-            mock.sentinel.instance_name,
+                              mock.sentinel.inst_name,
-            disk_bus=mock.sentinel.disk_bus)
+                              mock.sentinel.disk_bus)
-        mock_volume_driver.set_disk_qos_specs.assert_called_once_with(
+        else:
-            fake_conn_info, mock.sentinel.qos_specs)
+            self._volumeops.attach_volume(
+                fake_conn_info,
+                mock.sentinel.inst_name,
+                mock.sentinel.disk_bus)
 
-    def test_get_volume_connector(self):
+        mock_get_volume_driver.assert_any_call(
-        mock_instance = mock.DEFAULT
+            fake_conn_info)
-        initiator = self._volumeops._volutils.get_iscsi_initiator.return_value
+        fake_volume_driver.attach_volume.assert_has_calls(
-        expected = {'ip': CONF.my_ip,
+            [mock.call(fake_conn_info,
-                    'host': CONF.host,
+                       mock.sentinel.inst_name,
-                    'initiator': initiator}
+                       mock.sentinel.disk_bus)] * expected_try_count)
+        fake_volume_driver.set_disk_qos_specs.assert_has_calls(
+            [mock.call(fake_conn_info,
+                       mock.sentinel.qos_specs)] * expected_try_count)
 
-        response = self._volumeops.get_volume_connector(instance=mock_instance)
+        if attach_failed:
+            fake_volume_driver.disconnect_volume.assert_called_once_with(
+                fake_conn_info)
+            mock_sleep.assert_has_calls(
+                [mock.call(CONF.hyperv.volume_attach_retry_interval)] *
+                    CONF.hyperv.volume_attach_retry_count)
+        else:
+            mock_sleep.assert_not_called()
 
-        self._volumeops._volutils.get_iscsi_initiator.assert_called_once_with()
+    def test_attach_volume(self):
-        self.assertEqual(expected, response)
+        self._test_attach_volume(attach_failed=False)
+
+    def test_attach_volume_exc(self):
+        self._test_attach_volume(attach_failed=True)
 
     @mock.patch.object(volumeops.VolumeOps, '_get_volume_driver')
-    def test_initialize_volumes_connection(self, mock_get_volume_driver):
+    def test_disconnect_volume(self, mock_get_volume_driver):
+        fake_volume_driver = mock_get_volume_driver.return_value
+
+        self._volumeops.disconnect_volume(mock.sentinel.conn_info)
+
+        mock_get_volume_driver.assert_called_once_with(
+            mock.sentinel.conn_info)
+        fake_volume_driver.disconnect_volume.assert_called_once_with(
+            mock.sentinel.conn_info)
+
+    @mock.patch.object(volumeops.VolumeOps, '_get_volume_driver')
+    def test_detach_volume(self, mock_get_volume_driver):
+        fake_volume_driver = mock_get_volume_driver.return_value
+        fake_conn_info = {'data': 'fake_conn_info_data'}
+
+        self._volumeops.detach_volume(fake_conn_info,
+                                      mock.sentinel.inst_name)
+
+        mock_get_volume_driver.assert_called_once_with(
+            fake_conn_info)
+        fake_volume_driver.detach_volume.assert_called_once_with(
+            fake_conn_info, mock.sentinel.inst_name)
+        fake_volume_driver.disconnect_volume.assert_called_once_with(
+            fake_conn_info)
+
+    @mock.patch.object(connector, 'get_connector_properties')
+    def test_get_volume_connector(self, mock_get_connector):
+        conn = self._volumeops.get_volume_connector()
+
+        mock_get_connector.assert_called_once_with(
+            root_helper=None,
+            my_ip=CONF.my_block_storage_ip,
+            multipath=CONF.hyperv.use_multipath_io,
+            enforce_multipath=True,
+            host=CONF.host)
+        self.assertEqual(mock_get_connector.return_value, conn)
+
+    @mock.patch.object(volumeops.VolumeOps, '_get_volume_driver')
+    def test_connect_volumes(self, mock_get_volume_driver):
         block_device_info = get_fake_block_dev_info()
 
-        self._volumeops.initialize_volumes_connection(block_device_info)
+        self._volumeops.connect_volumes(block_device_info)
 
         init_vol_conn = (
-            mock_get_volume_driver.return_value.initialize_volume_connection)
+            mock_get_volume_driver.return_value.connect_volume)
         init_vol_conn.assert_called_once_with(
             block_device_info['block_device_mapping'][0]['connection_info'])
 
     @mock.patch.object(volumeops.VolumeOps,
-                       'get_mounted_disk_path_from_volume')
+                       'get_disk_resource_path')
     def test_get_disk_path_mapping(self, mock_get_disk_path):
         block_device_info = get_fake_block_dev_info()
         block_device_mapping = block_device_info['block_device_mapping']
@@ -208,27 +262,16 @@ class VolumeOpsTestCase(test_base.HyperVBaseTestCase):
         self.assertEqual(expected_disk_path_mapping,
                          resulted_disk_path_mapping)
 
-    def test_group_block_devices_by_type(self):
-        block_device_map = get_fake_block_dev_info()['block_device_mapping']
-        block_device_map[0]['connection_info'] = {
-            'driver_volume_type': 'iscsi'}
-        result = self._volumeops._group_block_devices_by_type(
-            block_device_map)
-
-        expected = {'iscsi': [block_device_map[0]]}
-        self.assertEqual(expected, result)
-
     @mock.patch.object(volumeops.VolumeOps, '_get_volume_driver')
-    def test_get_mounted_disk_path_from_volume(self, mock_get_volume_driver):
+    def test_get_disk_resource_path(self, mock_get_volume_driver):
         fake_conn_info = get_fake_connection_info()
         fake_volume_driver = mock_get_volume_driver.return_value
 
-        resulted_disk_path = self._volumeops.get_mounted_disk_path_from_volume(
+        resulted_disk_path = self._volumeops.get_disk_resource_path(
             fake_conn_info)
 
-        mock_get_volume_driver.assert_called_once_with(
+        mock_get_volume_driver.assert_called_once_with(fake_conn_info)
-            connection_info=fake_conn_info)
+        get_mounted_disk = fake_volume_driver.get_disk_resource_path
-        get_mounted_disk = fake_volume_driver.get_mounted_disk_path_from_volume
         get_mounted_disk.assert_called_once_with(fake_conn_info)
         self.assertEqual(get_mounted_disk.return_value,
                          resulted_disk_path)
@@ -251,397 +294,280 @@ class VolumeOpsTestCase(test_base.HyperVBaseTestCase):
         self.assertTrue(mock_warning.called)
 
 
-class ISCSIVolumeDriverTestCase(test_base.HyperVBaseTestCase):
+class BaseVolumeDriverTestCase(test_base.HyperVBaseTestCase):
-    """Unit tests for Hyper-V ISCSIVolumeDriver class."""
+    """Unit tests for Hyper-V BaseVolumeDriver class."""
 
     def setUp(self):
-        super(ISCSIVolumeDriverTestCase, self).setUp()
+        super(BaseVolumeDriverTestCase, self).setUp()
-        self._volume_driver = volumeops.ISCSIVolumeDriver()
-        self._volume_driver._vmutils = mock.MagicMock()
-        self._volume_driver._volutils = mock.MagicMock()
 
-    def test_login_storage_target_auth_exception(self):
+        self._base_vol_driver = volumeops.BaseVolumeDriver()
-        connection_info = get_fake_connection_info(
-            auth_method='fake_auth_method')
 
-        self.assertRaises(exception.UnsupportedBDMVolumeAuthMethod,
+        self._base_vol_driver._diskutils = mock.Mock()
-                          self._volume_driver.login_storage_target,
+        self._base_vol_driver._vmutils = mock.Mock()
-                          connection_info)
+        self._base_vol_driver._conn = mock.Mock()
+        self._vmutils = self._base_vol_driver._vmutils
+        self._diskutils = self._base_vol_driver._diskutils
+        self._conn = self._base_vol_driver._conn
 
-    @mock.patch.object(volumeops.ISCSIVolumeDriver,
+    @mock.patch.object(connector.InitiatorConnector, 'factory')
-                       '_get_mounted_disk_from_lun')
+    def test_connector(self, mock_conn_factory):
-    def _check_login_storage_target(self, mock_get_mounted_disk_from_lun,
+        self._base_vol_driver._conn = None
-                                    dev_number):
+        self._base_vol_driver._protocol = mock.sentinel.protocol
-        connection_info = get_fake_connection_info()
+        self._base_vol_driver._extra_connector_args = dict(
-        login_target = self._volume_driver._volutils.login_storage_target
+            fake_conn_arg=mock.sentinel.conn_val)
-        get_number = self._volume_driver._volutils.get_device_number_for_target
-        get_number.return_value = dev_number
 
-        self._volume_driver.login_storage_target(connection_info)
+        conn = self._base_vol_driver._connector
 
-        get_number.assert_called_once_with(mock.sentinel.fake_iqn,
+        self.assertEqual(mock_conn_factory.return_value, conn)
-                                           mock.sentinel.fake_lun)
+        mock_conn_factory.assert_called_once_with(
-        if not dev_number:
+            protocol=mock.sentinel.protocol,
-            login_target.assert_called_once_with(
+            root_helper=None,
-                mock.sentinel.fake_lun, mock.sentinel.fake_iqn,
+            use_multipath=CONF.hyperv.use_multipath_io,
-                mock.sentinel.fake_portal, mock.sentinel.fake_user,
+            device_scan_attempts=CONF.hyperv.mounted_disk_query_retry_count,
-                mock.sentinel.fake_pass)
+            device_scan_interval=(
-            mock_get_mounted_disk_from_lun.assert_called_once_with(
+                CONF.hyperv.mounted_disk_query_retry_interval),
-                mock.sentinel.fake_iqn, mock.sentinel.fake_lun, True)
+            **self._base_vol_driver._extra_connector_args)
+
+    def test_connect_volume(self):
+        conn_info = get_fake_connection_info()
+
+        dev_info = self._base_vol_driver.connect_volume(conn_info)
+        expected_dev_info = self._conn.connect_volume.return_value
+
+        self.assertEqual(expected_dev_info, dev_info)
+        self._conn.connect_volume.assert_called_once_with(
+            conn_info['data'])
+
+    def test_disconnect_volume(self):
+        conn_info = get_fake_connection_info()
+
+        self._base_vol_driver.disconnect_volume(conn_info)
+
+        self._conn.disconnect_volume.assert_called_once_with(
+            conn_info['data'])
+
+    @mock.patch.object(volumeops.BaseVolumeDriver, '_get_disk_res_path')
+    def _test_get_disk_resource_path_by_conn_info(self,
+                                                  mock_get_disk_res_path,
+                                                  disk_found=True):
+        conn_info = get_fake_connection_info()
+        mock_vol_paths = [mock.sentinel.disk_path] if disk_found else []
+        self._conn.get_volume_paths.return_value = mock_vol_paths
+
+        if disk_found:
+            disk_res_path = self._base_vol_driver.get_disk_resource_path(
+                conn_info)
+
+            self._conn.get_volume_paths.assert_called_once_with(
+                conn_info['data'])
+            self.assertEqual(mock_get_disk_res_path.return_value,
+                             disk_res_path)
+            mock_get_disk_res_path.assert_called_once_with(
+                mock.sentinel.disk_path)
         else:
-            self.assertFalse(login_target.called)
+            self.assertRaises(exception.DiskNotFound,
+                              self._base_vol_driver.get_disk_resource_path,
+                              conn_info)
 
-    def test_login_storage_target_already_logged(self):
+    def test_get_existing_disk_res_path(self):
-        self._check_login_storage_target(dev_number=1)
+        self._test_get_disk_resource_path_by_conn_info()
 
-    def test_login_storage_target(self):
+    def test_get_unfound_disk_res_path(self):
-        self._check_login_storage_target(dev_number=0)
+        self._test_get_disk_resource_path_by_conn_info(disk_found=False)
 
-    def _check_logout_storage_target(self, disconnected_luns_count=0):
+    def test_get_block_dev_res_path(self):
-        self._volume_driver._volutils.get_target_lun_count.return_value = 1
+        self._base_vol_driver._is_block_dev = True
 
-        self._volume_driver.logout_storage_target(
+        mock_get_dev_number = (
-            target_iqn=mock.sentinel.fake_iqn,
+            self._diskutils.get_device_number_from_device_name)
-            disconnected_luns_count=disconnected_luns_count)
+        mock_get_dev_number.return_value = mock.sentinel.dev_number
-
+        self._vmutils.get_mounted_disk_by_drive_number.return_value = (
-        logout_storage = self._volume_driver._volutils.logout_storage_target
-
-        if disconnected_luns_count:
-            logout_storage.assert_called_once_with(mock.sentinel.fake_iqn)
-        else:
-            self.assertFalse(logout_storage.called)
-
-    def test_logout_storage_target_skip(self):
-        self._check_logout_storage_target()
-
-    def test_logout_storage_target(self):
-        self._check_logout_storage_target(disconnected_luns_count=1)
-
-    @mock.patch.object(volumeops.ISCSIVolumeDriver,
-                       '_get_mounted_disk_from_lun')
-    def test_get_mounted_disk_path_from_volume(self,
-                                               mock_get_mounted_disk_from_lun):
-        connection_info = get_fake_connection_info()
-        resulted_disk_path = (
-            self._volume_driver.get_mounted_disk_path_from_volume(
-                connection_info))
-
-        mock_get_mounted_disk_from_lun.assert_called_once_with(
-            connection_info['data']['target_iqn'],
-            connection_info['data']['target_lun'],
-            wait_for_device=True)
-        self.assertEqual(mock_get_mounted_disk_from_lun.return_value,
-                         resulted_disk_path)
-
-    @mock.patch.object(volumeops.ISCSIVolumeDriver,
-                       '_get_mounted_disk_from_lun')
-    @mock.patch.object(volumeops.ISCSIVolumeDriver, 'logout_storage_target')
-    @mock.patch.object(volumeops.ISCSIVolumeDriver, 'login_storage_target')
-    def test_attach_volume_exception(self, mock_login_storage_target,
-                                     mock_logout_storage_target,
-                                     mock_get_mounted_disk):
-        connection_info = get_fake_connection_info()
-        mock_get_mounted_disk.side_effect = os_win_exc.HyperVException
-
-        self.assertRaises(os_win_exc.HyperVException,
-                          self._volume_driver.attach_volume, connection_info,
-                          mock.sentinel.instance_name)
-        mock_logout_storage_target.assert_called_with(mock.sentinel.fake_iqn)
-
-    @mock.patch.object(volumeops.ISCSIVolumeDriver,
-                       '_get_mounted_disk_from_lun')
-    @mock.patch.object(volumeops.ISCSIVolumeDriver, 'login_storage_target')
-    def _check_attach_volume(self, mock_login_storage_target,
-                             mock_get_mounted_disk_from_lun, disk_bus):
-        connection_info = get_fake_connection_info()
-
-        get_ide_path = self._volume_driver._vmutils.get_vm_ide_controller
-        get_scsi_path = self._volume_driver._vmutils.get_vm_scsi_controller
-        fake_ide_path = get_ide_path.return_value
-        fake_scsi_path = get_scsi_path.return_value
-        fake_mounted_disk_path = mock_get_mounted_disk_from_lun.return_value
-        attach_vol = self._volume_driver._vmutils.attach_volume_to_controller
-
-        get_free_slot = self._volume_driver._vmutils.get_free_controller_slot
-        get_free_slot.return_value = 1
-
-        self._volume_driver.attach_volume(
-            connection_info=connection_info,
-            instance_name=mock.sentinel.instance_name,
-            disk_bus=disk_bus)
-
-        mock_login_storage_target.assert_called_once_with(connection_info)
-        mock_get_mounted_disk_from_lun.assert_called_once_with(
-            mock.sentinel.fake_iqn,
-            mock.sentinel.fake_lun,
-            wait_for_device=True)
-        if disk_bus == constants.CTRL_TYPE_IDE:
-            get_ide_path.assert_called_once_with(
-                mock.sentinel.instance_name, 0)
-            attach_vol.assert_called_once_with(mock.sentinel.instance_name,
-                                               fake_ide_path, 0,
-                                               fake_mounted_disk_path,
-                                               serial=mock.sentinel.serial)
-        else:
-            get_scsi_path.assert_called_once_with(mock.sentinel.instance_name)
-            get_free_slot.assert_called_once_with(fake_scsi_path)
-            attach_vol.assert_called_once_with(mock.sentinel.instance_name,
-                                               fake_scsi_path, 1,
-                                               fake_mounted_disk_path,
-                                               serial=mock.sentinel.serial)
-
-    def test_attach_volume_ide(self):
-        self._check_attach_volume(disk_bus=constants.CTRL_TYPE_IDE)
-
-    def test_attach_volume_scsi(self):
-        self._check_attach_volume(disk_bus=constants.CTRL_TYPE_SCSI)
-
-    @mock.patch.object(volumeops.ISCSIVolumeDriver,
-                       '_get_mounted_disk_from_lun')
-    @mock.patch.object(volumeops.ISCSIVolumeDriver, 'logout_storage_target')
-    def test_detach_volume(self, mock_logout_storage_target,
-                           mock_get_mounted_disk_from_lun):
-        connection_info = get_fake_connection_info()
-
-        self._volume_driver.detach_volume(connection_info,
-                                          mock.sentinel.instance_name)
-
-        mock_get_mounted_disk_from_lun.assert_called_once_with(
-            mock.sentinel.fake_iqn,
-            mock.sentinel.fake_lun,
-            wait_for_device=True)
-        self._volume_driver._vmutils.detach_vm_disk.assert_called_once_with(
-            mock.sentinel.instance_name,
-            mock_get_mounted_disk_from_lun.return_value)
-        mock_logout_storage_target.assert_called_once_with(
-            mock.sentinel.fake_iqn)
-
-    def test_get_mounted_disk_from_lun(self):
-        with test.nested(
-            mock.patch.object(self._volume_driver._volutils,
-                              'get_device_number_for_target'),
-            mock.patch.object(self._volume_driver._vmutils,
-                              'get_mounted_disk_by_drive_number')
-            ) as (mock_get_device_number_for_target,
-                  mock_get_mounted_disk_by_drive_number):
-
-            mock_get_device_number_for_target.return_value = 0
-            mock_get_mounted_disk_by_drive_number.return_value = (
             mock.sentinel.disk_path)
 
-            disk = self._volume_driver._get_mounted_disk_from_lun(
+        disk_path = self._base_vol_driver._get_disk_res_path(
-                mock.sentinel.target_iqn,
+            mock.sentinel.dev_name)
-                mock.sentinel.target_lun)
-            self.assertEqual(mock.sentinel.disk_path, disk)
 
-    def test_get_target_from_disk_path(self):
+        mock_get_dev_number.assert_called_once_with(mock.sentinel.dev_name)
-        result = self._volume_driver.get_target_from_disk_path(
+        self._vmutils.get_mounted_disk_by_drive_number.assert_called_once_with(
-            mock.sentinel.physical_drive_path)
+            mock.sentinel.dev_number)
 
-        mock_get_target = (
+        self.assertEqual(mock.sentinel.disk_path, disk_path)
-            self._volume_driver._volutils.get_target_from_disk_path)
-        mock_get_target.assert_called_once_with(
-            mock.sentinel.physical_drive_path)
-        self.assertEqual(mock_get_target.return_value, result)
 
-    @mock.patch('time.sleep')
+    def test_get_virt_disk_res_path(self):
-    def test_get_mounted_disk_from_lun_failure(self, fake_sleep):
+        # For virtual disk images, we expect the resource path to be the
-        self.flags(mounted_disk_query_retry_count=1, group='hyperv')
+        # actual image path, as opposed to passthrough disks, in which case we
+        # need the Msvm_DiskDrive resource path when attaching it to a VM.
+        self._base_vol_driver._is_block_dev = False
 
-        with mock.patch.object(self._volume_driver._volutils,
+        path = self._base_vol_driver._get_disk_res_path(
-                               'get_device_number_for_target') as m_device_num:
+            mock.sentinel.disk_path)
-            m_device_num.side_effect = [None, -1]
+        self.assertEqual(mock.sentinel.disk_path, path)
 
-            self.assertRaises(exception.NotFound,
+    @mock.patch.object(volumeops.BaseVolumeDriver,
-                              self._volume_driver._get_mounted_disk_from_lun,
+                       '_get_disk_res_path')
-                              mock.sentinel.target_iqn,
+    @mock.patch.object(volumeops.BaseVolumeDriver, '_get_disk_ctrl_and_slot')
-                              mock.sentinel.target_lun)
+    @mock.patch.object(volumeops.BaseVolumeDriver,
-
+                       'connect_volume')
-    @mock.patch.object(volumeops.ISCSIVolumeDriver, 'logout_storage_target')
+    def _test_attach_volume(self, mock_connect_volume,
-    def test_disconnect_volumes(self, mock_logout_storage_target):
+                            mock_get_disk_ctrl_and_slot,
-        block_device_info = get_fake_block_dev_info()
+                            mock_get_disk_res_path,
+                            is_block_dev=True):
         connection_info = get_fake_connection_info()
-        block_device_mapping = block_device_info['block_device_mapping']
+        self._base_vol_driver._is_block_dev = is_block_dev
-        block_device_mapping[0]['connection_info'] = connection_info
+        mock_connect_volume.return_value = dict(path=mock.sentinel.raw_path)
 
-        self._volume_driver.disconnect_volumes(block_device_mapping)
+        mock_get_disk_res_path.return_value = (
+            mock.sentinel.disk_path)
+        mock_get_disk_ctrl_and_slot.return_value = (
+            mock.sentinel.ctrller_path,
+            mock.sentinel.slot)
 
-        mock_logout_storage_target.assert_called_once_with(
+        self._base_vol_driver.attach_volume(
-            mock.sentinel.fake_iqn, 1)
+            connection_info=connection_info,
+            instance_name=mock.sentinel.instance_name,
+            disk_bus=mock.sentinel.disk_bus)
 
-    def test_get_target_lun_count(self):
+        if is_block_dev:
-        result = self._volume_driver.get_target_lun_count(
+            self._vmutils.attach_volume_to_controller.assert_called_once_with(
-            mock.sentinel.target_iqn)
+                mock.sentinel.instance_name,
+                mock.sentinel.ctrller_path,
+                mock.sentinel.slot,
+                mock.sentinel.disk_path,
+                serial=connection_info['serial'])
+        else:
+            self._vmutils.attach_drive.assert_called_once_with(
+                mock.sentinel.instance_name,
+                mock.sentinel.disk_path,
+                mock.sentinel.ctrller_path,
+                mock.sentinel.slot)
 
-        mock_get_lun_count = self._volume_driver._volutils.get_target_lun_count
+        mock_get_disk_res_path.assert_called_once_with(
-        mock_get_lun_count.assert_called_once_with(mock.sentinel.target_iqn)
+            mock.sentinel.raw_path)
-        self.assertEqual(mock_get_lun_count.return_value, result)
+        mock_get_disk_ctrl_and_slot.assert_called_once_with(
+            mock.sentinel.instance_name, mock.sentinel.disk_bus)
 
-    @mock.patch.object(volumeops.ISCSIVolumeDriver, 'login_storage_target')
+    def test_attach_volume_image_file(self):
-    def test_initialize_volume_connection(self, mock_login_storage_target):
+        self._test_attach_volume(is_block_dev=False)
-        self._volume_driver.initialize_volume_connection(
+
-            mock.sentinel.connection_info)
+    def test_attach_volume_block_dev(self):
-        mock_login_storage_target.assert_called_once_with(
+        self._test_attach_volume(is_block_dev=True)
-            mock.sentinel.connection_info)
+
+    @mock.patch.object(volumeops.BaseVolumeDriver,
+                       'get_disk_resource_path')
+    def test_detach_volume(self, mock_get_disk_resource_path):
+        connection_info = get_fake_connection_info()
+
+        self._base_vol_driver.detach_volume(connection_info,
+                                            mock.sentinel.instance_name)
+
+        mock_get_disk_resource_path.assert_called_once_with(
+            connection_info)
+        self._vmutils.detach_vm_disk.assert_called_once_with(
+            mock.sentinel.instance_name,
+            mock_get_disk_resource_path.return_value,
+            is_physical=self._base_vol_driver._is_block_dev)
+
+    def test_get_disk_ctrl_and_slot_ide(self):
+        ctrl, slot = self._base_vol_driver._get_disk_ctrl_and_slot(
+            mock.sentinel.instance_name,
+            disk_bus=constants.CTRL_TYPE_IDE)
+
+        expected_ctrl = self._vmutils.get_vm_ide_controller.return_value
+        expected_slot = 0
+
+        self._vmutils.get_vm_ide_controller.assert_called_once_with(
+            mock.sentinel.instance_name, 0)
+
+        self.assertEqual(expected_ctrl, ctrl)
+        self.assertEqual(expected_slot, slot)
+
+    def test_get_disk_ctrl_and_slot_scsi(self):
+        ctrl, slot = self._base_vol_driver._get_disk_ctrl_and_slot(
+            mock.sentinel.instance_name,
+            disk_bus=constants.CTRL_TYPE_SCSI)
+
+        expected_ctrl = self._vmutils.get_vm_scsi_controller.return_value
+        expected_slot = (
+            self._vmutils.get_free_controller_slot.return_value)
+
+        self._vmutils.get_vm_scsi_controller.assert_called_once_with(
+            mock.sentinel.instance_name)
+        self._vmutils.get_free_controller_slot(
+           self._vmutils.get_vm_scsi_controller.return_value)
+
+        self.assertEqual(expected_ctrl, ctrl)
+        self.assertEqual(expected_slot, slot)
+
+    def test_set_disk_qos_specs(self):
+        # This base method is a noop, we'll just make sure
+        # it doesn't error out.
+        self._base_vol_driver.set_disk_qos_specs(
+            mock.sentinel.conn_info, mock.sentinel.disk_qos_spes)
+
+
+class ISCSIVolumeDriverTestCase(test_base.HyperVBaseTestCase):
+    """Unit tests for Hyper-V BaseVolumeDriver class."""
+
+    def test_extra_conn_args(self):
+        fake_iscsi_initiator = (
+            'PCI\\VEN_1077&DEV_2031&SUBSYS_17E8103C&REV_02\\'
+            '4&257301f0&0&0010_0')
+        self.flags(iscsi_initiator_list=[fake_iscsi_initiator],
+                   group='hyperv')
+        expected_extra_conn_args = dict(
+            initiator_list=[fake_iscsi_initiator])
+
+        vol_driver = volumeops.ISCSIVolumeDriver()
+
+        self.assertEqual(expected_extra_conn_args,
+                         vol_driver._extra_connector_args)
 
 
 class SMBFSVolumeDriverTestCase(test_base.HyperVBaseTestCase):
     """Unit tests for the Hyper-V SMBFSVolumeDriver class."""
 
-    _FAKE_SHARE = '//1.2.3.4/fake_share'
+    _FAKE_EXPORT_PATH = '//ip/share/'
-    _FAKE_SHARE_NORMALIZED = _FAKE_SHARE.replace('/', '\\')
+    _FAKE_CONN_INFO = get_fake_connection_info(export=_FAKE_EXPORT_PATH)
-    _FAKE_DISK_NAME = 'fake_volume_name.vhdx'
-    _FAKE_USERNAME = 'fake_username'
-    _FAKE_PASSWORD = 'fake_password'
-    _FAKE_SMB_OPTIONS = '-o username=%s,password=%s' % (_FAKE_USERNAME,
-                                                        _FAKE_PASSWORD)
-    _FAKE_CONNECTION_INFO = {'data': {'export': _FAKE_SHARE,
-                                      'name': _FAKE_DISK_NAME,
-                                      'options': _FAKE_SMB_OPTIONS,
-                                      'volume_id': 'fake_vol_id'}}
 
     def setUp(self):
         super(SMBFSVolumeDriverTestCase, self).setUp()
         self._volume_driver = volumeops.SMBFSVolumeDriver()
-        self._volume_driver._vmutils = mock.MagicMock()
+        self._volume_driver._conn = mock.Mock()
-        self._volume_driver._smbutils = mock.MagicMock()
+        self._conn = self._volume_driver._conn
-        self._volume_driver._volutils = mock.MagicMock()
-
-    @mock.patch.object(volumeops.SMBFSVolumeDriver,
-                       '_get_disk_path')
-    def test_get_mounted_disk_path_from_volume(self, mock_get_disk_path):
-        disk_path = self._volume_driver.get_mounted_disk_path_from_volume(
-            mock.sentinel.conn_info)
-
-        self.assertEqual(mock_get_disk_path.return_value, disk_path)
-        mock_get_disk_path.assert_called_once_with(mock.sentinel.conn_info)
-
-    @mock.patch.object(volumeops.SMBFSVolumeDriver, 'ensure_share_mounted')
-    @mock.patch.object(volumeops.SMBFSVolumeDriver, '_get_disk_path')
-    def _check_attach_volume(self, mock_get_disk_path,
-                             mock_ensure_share_mounted, disk_bus):
-        mock_get_disk_path.return_value = mock.sentinel.disk_path
-
-        self._volume_driver.attach_volume(
-            self._FAKE_CONNECTION_INFO,
-            mock.sentinel.instance_name,
-            disk_bus)
-
-        if disk_bus == constants.CTRL_TYPE_IDE:
-            get_vm_ide_controller = (
-                self._volume_driver._vmutils.get_vm_ide_controller)
-            get_vm_ide_controller.assert_called_once_with(
-                mock.sentinel.instance_name, 0)
-            ctrller_path = get_vm_ide_controller.return_value
-            slot = 0
-        else:
-            get_vm_scsi_controller = (
-                self._volume_driver._vmutils.get_vm_scsi_controller)
-            get_vm_scsi_controller.assert_called_once_with(
-                mock.sentinel.instance_name)
-            get_free_controller_slot = (
-                self._volume_driver._vmutils.get_free_controller_slot)
-            get_free_controller_slot.assert_called_once_with(
-                get_vm_scsi_controller.return_value)
-
-            ctrller_path = get_vm_scsi_controller.return_value
-            slot = get_free_controller_slot.return_value
-
-        mock_ensure_share_mounted.assert_called_once_with(
-            self._FAKE_CONNECTION_INFO)
-        mock_get_disk_path.assert_called_once_with(self._FAKE_CONNECTION_INFO)
-        self._volume_driver._vmutils.attach_drive.assert_called_once_with(
-            mock.sentinel.instance_name, mock.sentinel.disk_path,
-            ctrller_path, slot)
-
-    def test_attach_volume_ide(self):
-        self._check_attach_volume(disk_bus=constants.CTRL_TYPE_IDE)
-
-    def test_attach_volume_scsi(self):
-        self._check_attach_volume(disk_bus=constants.CTRL_TYPE_SCSI)
-
-    @mock.patch.object(volumeops.SMBFSVolumeDriver, 'ensure_share_mounted')
-    @mock.patch.object(volumeops.SMBFSVolumeDriver, '_get_disk_path')
-    def test_attach_non_existing_image(self, mock_get_disk_path,
-                                       mock_ensure_share_mounted):
-        self._volume_driver._vmutils.attach_drive.side_effect = (
-            os_win_exc.HyperVException)
-        self.assertRaises(exception.VolumeAttachFailed,
-                          self._volume_driver.attach_volume,
-                          self._FAKE_CONNECTION_INFO,
-                          mock.sentinel.instance_name)
-
-    @mock.patch.object(volumeops.SMBFSVolumeDriver, '_get_disk_path')
-    def test_detach_volume(self, mock_get_disk_path):
-        mock_get_disk_path.return_value = (
-            mock.sentinel.disk_path)
-
-        self._volume_driver.detach_volume(self._FAKE_CONNECTION_INFO,
-                                          mock.sentinel.instance_name)
-
-        self._volume_driver._vmutils.detach_vm_disk.assert_called_once_with(
-            mock.sentinel.instance_name, mock.sentinel.disk_path,
-            is_physical=False)
-
-    def test_parse_credentials(self):
-        username, password = self._volume_driver._parse_credentials(
-            self._FAKE_SMB_OPTIONS)
-        self.assertEqual(self._FAKE_USERNAME, username)
-        self.assertEqual(self._FAKE_PASSWORD, password)
 
     def test_get_export_path(self):
-        result = self._volume_driver._get_export_path(
+        export_path = self._volume_driver._get_export_path(
-            self._FAKE_CONNECTION_INFO)
+            self._FAKE_CONN_INFO)
+        expected_path = self._FAKE_EXPORT_PATH.replace('/', '\\')
+        self.assertEqual(expected_path, export_path)
 
-        expected = self._FAKE_SHARE.replace('/', '\\')
+    @mock.patch.object(volumeops.BaseVolumeDriver, 'attach_volume')
-        self.assertEqual(expected, result)
+    def test_attach_volume(self, mock_attach):
+        # The tested method will just apply a lock before calling
+        # the superclass method.
+        self._volume_driver.attach_volume(
+            self._FAKE_CONN_INFO,
+            mock.sentinel.instance_name,
+            disk_bus=mock.sentinel.disk_bus)
 
-    def test_get_disk_path(self):
+        mock_attach.assert_called_once_with(
-        expected = os.path.join(self._FAKE_SHARE_NORMALIZED,
+            self._FAKE_CONN_INFO,
-                                self._FAKE_DISK_NAME)
+            mock.sentinel.instance_name,
+            disk_bus=mock.sentinel.disk_bus)
 
-        disk_path = self._volume_driver._get_disk_path(
+    @mock.patch.object(volumeops.BaseVolumeDriver, 'detach_volume')
-            self._FAKE_CONNECTION_INFO)
+    def test_detach_volume(self, mock_detach):
+        self._volume_driver.detach_volume(
+            self._FAKE_CONN_INFO,
+            instance_name=mock.sentinel.instance_name)
 
-        self.assertEqual(expected, disk_path)
+        mock_detach.assert_called_once_with(
-
+            self._FAKE_CONN_INFO,
-    @mock.patch.object(volumeops.SMBFSVolumeDriver, '_parse_credentials')
+            instance_name=mock.sentinel.instance_name)
-    def _test_ensure_mounted(self, mock_parse_credentials, is_mounted=False):
-        mock_mount_smb_share = self._volume_driver._smbutils.mount_smb_share
-        self._volume_driver._smbutils.check_smb_mapping.return_value = (
-            is_mounted)
-        mock_parse_credentials.return_value = (
-            self._FAKE_USERNAME, self._FAKE_PASSWORD)
-
-        self._volume_driver.ensure_share_mounted(
-            self._FAKE_CONNECTION_INFO)
-
-        if is_mounted:
-            self.assertFalse(
-                mock_mount_smb_share.called)
-        else:
-            mock_mount_smb_share.assert_called_once_with(
-                self._FAKE_SHARE_NORMALIZED,
-                username=self._FAKE_USERNAME,
-                password=self._FAKE_PASSWORD)
-
-    def test_ensure_mounted_new_share(self):
-        self._test_ensure_mounted()
-
-    def test_ensure_already_mounted(self):
-        self._test_ensure_mounted(is_mounted=True)
-
-    def test_disconnect_volumes(self):
-        block_device_mapping = [
-            {'connection_info': self._FAKE_CONNECTION_INFO}]
-        self._volume_driver.disconnect_volumes(block_device_mapping)
-        mock_unmount_share = self._volume_driver._smbutils.unmount_smb_share
-        mock_unmount_share.assert_called_once_with(
-            self._FAKE_SHARE_NORMALIZED)
 
     @mock.patch.object(volumeops.VolumeOps, 'bytes_per_sec_to_iops')
     @mock.patch.object(volumeops.VolumeOps, 'validate_qos_specs')
-    @mock.patch.object(volumeops.SMBFSVolumeDriver, '_get_disk_path')
+    @mock.patch.object(volumeops.BaseVolumeDriver, 'get_disk_resource_path')
     def test_set_disk_qos_specs(self, mock_get_disk_path,
                                 mock_validate_qos_specs,
                                 mock_bytes_per_sec_to_iops):
@@ -652,17 +578,16 @@ class SMBFSVolumeDriverTestCase(test_base.HyperVBaseTestCase):
         expected_supported_specs = ['total_iops_sec', 'total_bytes_sec']
         mock_set_qos_specs = self._volume_driver._vmutils.set_disk_qos_specs
         mock_bytes_per_sec_to_iops.return_value = fake_total_iops_sec
+        mock_get_disk_path.return_value = mock.sentinel.disk_path
 
-        self._volume_driver.set_disk_qos_specs(mock.sentinel.connection_info,
+        self._volume_driver.set_disk_qos_specs(self._FAKE_CONN_INFO,
                                                storage_qos_specs)
 
         mock_validate_qos_specs.assert_called_once_with(
             storage_qos_specs, expected_supported_specs)
         mock_bytes_per_sec_to_iops.assert_called_once_with(
             fake_total_bytes_sec)
-        mock_disk_path = mock_get_disk_path.return_value
+        mock_get_disk_path.assert_called_once_with(self._FAKE_CONN_INFO)
-        mock_get_disk_path.assert_called_once_with(
-            mock.sentinel.connection_info)
         mock_set_qos_specs.assert_called_once_with(
-            mock_disk_path,
+            mock.sentinel.disk_path,
             fake_total_iops_sec)

nova/virt/hyperv/constants.py

@@ -85,3 +85,7 @@ FLAVOR_ESPEC_REMOTEFX_MONITORS = 'os:monitors'
 FLAVOR_ESPEC_REMOTEFX_VRAM = 'os:vram'
 
 IOPS_BASE_SIZE = 8 * units.Ki
+
+STORAGE_PROTOCOL_ISCSI = 'iscsi'
+STORAGE_PROTOCOL_FC = 'fibre_channel'
+STORAGE_PROTOCOL_SMBFS = 'smbfs'

nova/virt/hyperv/driver.py

@@ -181,7 +181,7 @@ class HyperVDriver(driver.ComputeDriver):
                                              instance.name)
 
     def get_volume_connector(self, instance):
-        return self._volumeops.get_volume_connector(instance)
+        return self._volumeops.get_volume_connector()
 
     def get_available_resource(self, nodename):
         return self._hostops.get_available_resource()

nova/virt/hyperv/livemigrationops.py

@@ -96,7 +96,7 @@ class LiveMigrationOps(object):
             if not boot_from_volume and instance.image_ref:
                 self._imagecache.get_cached_image(context, instance)
 
-        self._volumeops.initialize_volumes_connection(block_device_info)
+        self._volumeops.connect_volumes(block_device_info)
 
         disk_path_mapping = self._volumeops.get_disk_path_mapping(
             block_device_info)

nova/virt/hyperv/volumeops.py

@@ -17,16 +17,12 @@
 """
 Management class for Storage-related functions (attach, detach, etc).
 """
-import collections
-import os
-import re
 import time
 
-from os_win import exceptions as os_win_exc
+from os_brick.initiator import connector
 from os_win import utilsfactory
 from oslo_log import log as logging
-from oslo_utils import excutils
+from oslo_utils import strutils
-from six.moves import range
 
 import nova.conf
 from nova import exception
@@ -46,14 +42,13 @@ class VolumeOps(object):
 
     def __init__(self):
         self._vmutils = utilsfactory.get_vmutils()
-        self._volutils = utilsfactory.get_iscsi_initiator_utils()
-        self._initiator = None
         self._default_root_device = 'vda'
-        self.volume_drivers = {'smbfs': SMBFSVolumeDriver(),
+        self.volume_drivers = {
-                               'iscsi': ISCSIVolumeDriver()}
+            constants.STORAGE_PROTOCOL_SMBFS: SMBFSVolumeDriver(),
+            constants.STORAGE_PROTOCOL_ISCSI: ISCSIVolumeDriver(),
+            constants.STORAGE_PROTOCOL_FC: FCVolumeDriver()}
 
-    def _get_volume_driver(self, driver_type=None, connection_info=None):
+    def _get_volume_driver(self, connection_info):
-        if connection_info:
         driver_type = connection_info.get('driver_volume_type')
         if driver_type not in self.volume_drivers:
             raise exception.VolumeDriverNotFound(driver_type=driver_type)
@@ -65,28 +60,74 @@ class VolumeOps(object):
 
     def disconnect_volumes(self, block_device_info):
         mapping = driver.block_device_info_get_mapping(block_device_info)
-        block_devices = self._group_block_devices_by_type(
+        for vol in mapping:
-            mapping)
+            self.disconnect_volume(vol['connection_info'])
-        for driver_type, block_device_mapping in block_devices.items():
-            volume_driver = self._get_volume_driver(driver_type)
-            volume_driver.disconnect_volumes(block_device_mapping)
 
     def attach_volume(self, connection_info, instance_name,
                       disk_bus=constants.CTRL_TYPE_SCSI):
-        volume_driver = self._get_volume_driver(
+        tries_left = CONF.hyperv.volume_attach_retry_count + 1
-            connection_info=connection_info)
+
-        volume_driver.attach_volume(connection_info, instance_name,
+        while tries_left:
-                                    disk_bus=disk_bus)
+            try:
+                self._attach_volume(connection_info,
+                                    instance_name,
+                                    disk_bus)
+                break
+            except Exception as ex:
+                tries_left -= 1
+                if not tries_left:
+                    LOG.exception(
+                        _LE("Failed to attach volume %(connection_info)s "
+                            "to instance %(instance_name)s. "),
+                        {'connection_info': strutils.mask_dict_password(
+                             connection_info),
+                         'instance_name': instance_name})
+
+                    self.disconnect_volume(connection_info)
+                    raise exception.VolumeAttachFailed(
+                        volume_id=connection_info['serial'],
+                        reason=ex)
+                else:
+                    LOG.warning(
+                        _LW("Failed to attach volume %(connection_info)s "
+                            "to instance %(instance_name)s. "
+                            "Tries left: %(tries_left)s."),
+                        {'connection_info': strutils.mask_dict_password(
+                             connection_info),
+                         'instance_name': instance_name,
+                         'tries_left': tries_left})
+
+                    time.sleep(CONF.hyperv.volume_attach_retry_interval)
+
+    def _attach_volume(self, connection_info, instance_name,
+                       disk_bus=constants.CTRL_TYPE_SCSI):
+        LOG.debug(
+            "Attaching volume: %(connection_info)s to %(instance_name)s",
+            {'connection_info': strutils.mask_dict_password(connection_info),
+             'instance_name': instance_name})
+        volume_driver = self._get_volume_driver(connection_info)
+        volume_driver.attach_volume(connection_info,
+                                    instance_name,
+                                    disk_bus)
 
         qos_specs = connection_info['data'].get('qos_specs') or {}
         if qos_specs:
             volume_driver.set_disk_qos_specs(connection_info,
                                              qos_specs)
 
+    def disconnect_volume(self, connection_info):
+        volume_driver = self._get_volume_driver(connection_info)
+        volume_driver.disconnect_volume(connection_info)
+
     def detach_volume(self, connection_info, instance_name):
-        volume_driver = self._get_volume_driver(
+        LOG.debug("Detaching volume: %(connection_info)s "
-            connection_info=connection_info)
+                  "from %(instance_name)s",
+                  {'connection_info': strutils.mask_dict_password(
+                      connection_info),
+                   'instance_name': instance_name})
+        volume_driver = self._get_volume_driver(connection_info)
         volume_driver.detach_volume(connection_info, instance_name)
+        volume_driver.disconnect_volume(connection_info)
 
     def fix_instance_volume_disk_paths(self, instance_name, block_device_info):
         # Mapping containing the current disk paths for each volume.
@@ -107,25 +148,23 @@ class VolumeOps(object):
                 self._vmutils.set_disk_host_res(vm_disk['resource_path'],
                                                 actual_disk_path)
 
-    def get_volume_connector(self, instance):
+    def get_volume_connector(self):
-        if not self._initiator:
+        # NOTE(lpetrut): the Windows os-brick connectors
-            self._initiator = self._volutils.get_iscsi_initiator()
+        # do not use a root helper.
-            if not self._initiator:
+        conn = connector.get_connector_properties(
-                LOG.warning(_LW('Could not determine iscsi initiator name'),
+            root_helper=None,
-                            instance=instance)
+            my_ip=CONF.my_block_storage_ip,
-        return {
+            multipath=CONF.hyperv.use_multipath_io,
-            'ip': CONF.my_block_storage_ip,
+            enforce_multipath=True,
-            'host': CONF.host,
+            host=CONF.host)
-            'initiator': self._initiator,
+        return conn
-        }
 
-    def initialize_volumes_connection(self, block_device_info):
+    def connect_volumes(self, block_device_info):
         mapping = driver.block_device_info_get_mapping(block_device_info)
         for vol in mapping:
             connection_info = vol['connection_info']
-            volume_driver = self._get_volume_driver(
+            volume_driver = self._get_volume_driver(connection_info)
-                connection_info=connection_info)
+            volume_driver.connect_volume(connection_info)
-            volume_driver.initialize_volume_connection(connection_info)
 
     def get_disk_path_mapping(self, block_device_info):
         block_mapping = driver.block_device_info_get_mapping(block_device_info)
@@ -134,23 +173,13 @@ class VolumeOps(object):
             connection_info = vol['connection_info']
             disk_serial = connection_info['serial']
 
-            disk_path = self.get_mounted_disk_path_from_volume(connection_info)
+            disk_path = self.get_disk_resource_path(connection_info)
             disk_path_mapping[disk_serial] = disk_path
         return disk_path_mapping
 
-    def _group_block_devices_by_type(self, block_device_mapping):
+    def get_disk_resource_path(self, connection_info):
-        block_devices = collections.defaultdict(list)
+        volume_driver = self._get_volume_driver(connection_info)
-        for volume in block_device_mapping:
+        return volume_driver.get_disk_resource_path(connection_info)
-            connection_info = volume['connection_info']
-            volume_type = connection_info.get('driver_volume_type')
-            block_devices[volume_type].append(volume)
-        return block_devices
-
-    def get_mounted_disk_path_from_volume(self, connection_info):
-        volume_driver = self._get_volume_driver(
-            connection_info=connection_info)
-        return volume_driver.get_mounted_disk_path_from_volume(
-            connection_info)
 
     @staticmethod
     def bytes_per_sec_to_iops(no_bytes):
@@ -173,93 +202,92 @@ class VolumeOps(object):
             LOG.warning(msg)
 
 
-class ISCSIVolumeDriver(object):
+class BaseVolumeDriver(object):
+    _is_block_dev = True
+    _protocol = None
+    _extra_connector_args = {}
+
     def __init__(self):
+        self._conn = None
+        self._diskutils = utilsfactory.get_diskutils()
         self._vmutils = utilsfactory.get_vmutils()
-        self._volutils = utilsfactory.get_iscsi_initiator_utils()
 
-    def login_storage_target(self, connection_info):
+    @property
-        data = connection_info['data']
+    def _connector(self):
-        target_lun = data['target_lun']
+        if not self._conn:
-        target_iqn = data['target_iqn']
+            scan_attempts = CONF.hyperv.mounted_disk_query_retry_count
-        target_portal = data['target_portal']
+            scan_interval = CONF.hyperv.mounted_disk_query_retry_interval
-        auth_method = data.get('auth_method')
-        auth_username = data.get('auth_username')
-        auth_password = data.get('auth_password')
 
-        if auth_method and auth_method.upper() != 'CHAP':
+            self._conn = connector.InitiatorConnector.factory(
-            LOG.error(_LE("Cannot log in target %(target_iqn)s. Unsupported "
+                protocol=self._protocol,
-                          "iSCSI authentication method: %(auth_method)s."),
+                root_helper=None,
-                      {'target_iqn': target_iqn,
+                use_multipath=CONF.hyperv.use_multipath_io,
-                       'auth_method': auth_method})
+                device_scan_attempts=scan_attempts,
-            raise exception.UnsupportedBDMVolumeAuthMethod(
+                device_scan_interval=scan_interval,
-                auth_method=auth_method)
+                **self._extra_connector_args)
+        return self._conn
 
-        # Check if we already logged in
+    def connect_volume(self, connection_info):
-        if self._volutils.get_device_number_for_target(target_iqn, target_lun):
+        return self._connector.connect_volume(connection_info['data'])
-            LOG.debug("Already logged in on storage target. No need to "
+
-                      "login. Portal: %(target_portal)s, "
+    def disconnect_volume(self, connection_info):
-                      "IQN: %(target_iqn)s, LUN: %(target_lun)s",
+        self._connector.disconnect_volume(connection_info['data'])
-                      {'target_portal': target_portal,
+
-                       'target_iqn': target_iqn, 'target_lun': target_lun})
+    def get_disk_resource_path(self, connection_info):
+        disk_paths = self._connector.get_volume_paths(connection_info['data'])
+        if not disk_paths:
+            vol_id = connection_info['serial']
+            err_msg = _("Could not find disk path. Volume id: %s")
+            raise exception.DiskNotFound(err_msg % vol_id)
+
+        return self._get_disk_res_path(disk_paths[0])
+
+    def _get_disk_res_path(self, disk_path):
+        if self._is_block_dev:
+            # We need the Msvm_DiskDrive resource path as this
+            # will be used when the disk is attached to an instance.
+            disk_number = self._diskutils.get_device_number_from_device_name(
+                disk_path)
+            disk_res_path = self._vmutils.get_mounted_disk_by_drive_number(
+                disk_number)
         else:
-            LOG.debug("Logging in on storage target. Portal: "
+            disk_res_path = disk_path
-                      "%(target_portal)s, IQN: %(target_iqn)s, "
+        return disk_res_path
-                      "LUN: %(target_lun)s",
-                      {'target_portal': target_portal,
-                       'target_iqn': target_iqn, 'target_lun': target_lun})
-            self._volutils.login_storage_target(target_lun, target_iqn,
-                                                target_portal, auth_username,
-                                                auth_password)
-            # Wait for the target to be mounted
-            self._get_mounted_disk_from_lun(target_iqn, target_lun, True)
-
-    def disconnect_volumes(self, block_device_mapping):
-        iscsi_targets = collections.defaultdict(int)
-        for vol in block_device_mapping:
-            target_iqn = vol['connection_info']['data']['target_iqn']
-            iscsi_targets[target_iqn] += 1
-
-        for target_iqn, disconnected_luns in iscsi_targets.items():
-            self.logout_storage_target(target_iqn, disconnected_luns)
-
-    def logout_storage_target(self, target_iqn, disconnected_luns_count=1):
-        total_available_luns = self._volutils.get_target_lun_count(
-            target_iqn)
-
-        if total_available_luns == disconnected_luns_count:
-            LOG.debug("Logging off storage target %s", target_iqn)
-            self._volutils.logout_storage_target(target_iqn)
-        else:
-            LOG.debug("Skipping disconnecting target %s as there "
-                      "are LUNs still being used.", target_iqn)
-
-    def get_mounted_disk_path_from_volume(self, connection_info):
-        data = connection_info['data']
-        target_lun = data['target_lun']
-        target_iqn = data['target_iqn']
-
-        # Getting the mounted disk
-        return self._get_mounted_disk_from_lun(target_iqn, target_lun,
-                                               wait_for_device=True)
 
     def attach_volume(self, connection_info, instance_name,
                       disk_bus=constants.CTRL_TYPE_SCSI):
-        """Attach a volume to the SCSI controller or to the IDE controller if
+        dev_info = self.connect_volume(connection_info)
-        ebs_root is True
-        """
-        target_iqn = None
-        LOG.debug("Attach_volume: %(connection_info)s to %(instance_name)s",
-                  {'connection_info': connection_info,
-                   'instance_name': instance_name})
-        try:
-            self.login_storage_target(connection_info)
 
         serial = connection_info['serial']
-            # Getting the mounted disk
+        disk_path = self._get_disk_res_path(dev_info['path'])
-            mounted_disk_path = self.get_mounted_disk_path_from_volume(
+        ctrller_path, slot = self._get_disk_ctrl_and_slot(instance_name,
-                connection_info)
+                                                          disk_bus)
+        if self._is_block_dev:
+            # We need to tag physical disk resources with the volume
+            # serial number, in order to be able to retrieve them
+            # during live migration.
+            self._vmutils.attach_volume_to_controller(instance_name,
+                                                      ctrller_path,
+                                                      slot,
+                                                      disk_path,
+                                                      serial=serial)
+        else:
+            self._vmutils.attach_drive(instance_name,
+                                       disk_path,
+                                       ctrller_path,
+                                       slot)
 
+    def detach_volume(self, connection_info, instance_name):
+        disk_path = self.get_disk_resource_path(connection_info)
+
+        LOG.debug("Detaching disk %(disk_path)s "
+                  "from instance: %(instance_name)s",
+                  dict(disk_path=disk_path,
+                       instance_name=instance_name))
+        self._vmutils.detach_vm_disk(instance_name, disk_path,
+                                     is_physical=self._is_block_dev)
+
+    def _get_disk_ctrl_and_slot(self, instance_name, disk_bus):
         if disk_bus == constants.CTRL_TYPE_IDE:
             # Find the IDE controller for the vm.
             ctrller_path = self._vmutils.get_vm_ide_controller(
@@ -271,91 +299,31 @@ class ISCSIVolumeDriver(object):
             ctrller_path = self._vmutils.get_vm_scsi_controller(
                 instance_name)
             slot = self._vmutils.get_free_controller_slot(ctrller_path)
-
+        return ctrller_path, slot
-            self._vmutils.attach_volume_to_controller(instance_name,
-                                                      ctrller_path,
-                                                      slot,
-                                                      mounted_disk_path,
-                                                      serial=serial)
-        except Exception:
-            with excutils.save_and_reraise_exception():
-                LOG.error(_LE('Unable to attach volume to instance %s'),
-                          instance_name)
-                target_iqn = connection_info['data']['target_iqn']
-                if target_iqn:
-                    self.logout_storage_target(target_iqn)
-
-    def detach_volume(self, connection_info, instance_name):
-        """Detach a volume to the SCSI controller."""
-        LOG.debug("Detach_volume: %(connection_info)s "
-                  "from %(instance_name)s",
-                  {'connection_info': connection_info,
-                   'instance_name': instance_name})
-
-        target_iqn = connection_info['data']['target_iqn']
-        mounted_disk_path = self.get_mounted_disk_path_from_volume(
-                connection_info)
-
-        LOG.debug("Detaching physical disk from instance: %s",
-                  mounted_disk_path)
-        self._vmutils.detach_vm_disk(instance_name, mounted_disk_path)
-
-        self.logout_storage_target(target_iqn)
-
-    def _get_mounted_disk_from_lun(self, target_iqn, target_lun,
-                                   wait_for_device=False):
-        # The WMI query in get_device_number_for_target can incorrectly
-        # return no data when the system is under load.  This issue can
-        # be avoided by adding a retry.
-        for i in range(CONF.hyperv.mounted_disk_query_retry_count):
-            device_number = self._volutils.get_device_number_for_target(
-                target_iqn, target_lun)
-            if device_number in (None, -1):
-                attempt = i + 1
-                LOG.debug('Attempt %d to get device_number '
-                          'from get_device_number_for_target failed. '
-                          'Retrying...', attempt)
-                time.sleep(CONF.hyperv.mounted_disk_query_retry_interval)
-            else:
-                break
-
-        if device_number in (None, -1):
-            raise exception.NotFound(_('Unable to find a mounted disk for '
-                                       'target_iqn: %s') % target_iqn)
-        LOG.debug('Device number: %(device_number)s, '
-                  'target lun: %(target_lun)s',
-                  {'device_number': device_number, 'target_lun': target_lun})
-        # Finding Mounted disk drive
-        for i in range(0, CONF.hyperv.volume_attach_retry_count):
-            mounted_disk_path = self._vmutils.get_mounted_disk_by_drive_number(
-                device_number)
-            if mounted_disk_path or not wait_for_device:
-                break
-            time.sleep(CONF.hyperv.volume_attach_retry_interval)
-
-        if not mounted_disk_path:
-            raise exception.NotFound(_('Unable to find a mounted disk for '
-                                       'target_iqn: %s. Please ensure that '
-                                       'the host\'s SAN policy is set to '
-                                       '"OfflineAll" or "OfflineShared"') %
-                                     target_iqn)
-        return mounted_disk_path
-
-    def get_target_from_disk_path(self, physical_drive_path):
-        return self._volutils.get_target_from_disk_path(physical_drive_path)
-
-    def get_target_lun_count(self, target_iqn):
-        return self._volutils.get_target_lun_count(target_iqn)
-
-    def initialize_volume_connection(self, connection_info):
-        self.login_storage_target(connection_info)
 
     def set_disk_qos_specs(self, connection_info, disk_qos_specs):
-        LOG.info(_LI("The iSCSI Hyper-V volume driver does not support QoS. "
+        LOG.info(_LI("The %(protocol)s Hyper-V volume driver "
-                     "Ignoring QoS specs."))
+                     "does not support QoS. Ignoring QoS specs."),
+                 dict(protocol=self._protocol))
 
 
-def export_path_synchronized(f):
+class ISCSIVolumeDriver(BaseVolumeDriver):
+    _is_block_dev = True
+    _protocol = constants.STORAGE_PROTOCOL_ISCSI
+
+    def __init__(self, *args, **kwargs):
+        self._extra_connector_args = dict(
+            initiator_list=CONF.hyperv.iscsi_initiator_list)
+
+        super(ISCSIVolumeDriver, self).__init__(*args, **kwargs)
+
+
+class SMBFSVolumeDriver(BaseVolumeDriver):
+    _is_block_dev = False
+    _protocol = constants.STORAGE_PROTOCOL_SMBFS
+    _extra_connector_args = dict(local_path_for_loopback=True)
+
+    def export_path_synchronized(f):
         def wrapper(inst, connection_info, *args, **kwargs):
             export_path = inst._get_export_path(connection_info)
 
@@ -365,109 +333,19 @@ def export_path_synchronized(f):
             return inner()
         return wrapper
 
-
-class SMBFSVolumeDriver(object):
-    def __init__(self):
-        self._smbutils = utilsfactory.get_smbutils()
-        self._vmutils = utilsfactory.get_vmutils()
-        self._username_regex = re.compile(r'user(?:name)?=([^, ]+)')
-        self._password_regex = re.compile(r'pass(?:word)?=([^, ]+)')
-
-    def get_mounted_disk_path_from_volume(self, connection_info):
-        return self._get_disk_path(connection_info)
-
-    @export_path_synchronized
-    def attach_volume(self, connection_info, instance_name,
-                      disk_bus=constants.CTRL_TYPE_SCSI):
-        self.ensure_share_mounted(connection_info)
-
-        disk_path = self._get_disk_path(connection_info)
-
-        try:
-            if disk_bus == constants.CTRL_TYPE_IDE:
-                ctrller_path = self._vmutils.get_vm_ide_controller(
-                    instance_name, 0)
-                slot = 0
-            else:
-                ctrller_path = self._vmutils.get_vm_scsi_controller(
-                 instance_name)
-                slot = self._vmutils.get_free_controller_slot(ctrller_path)
-
-            self._vmutils.attach_drive(instance_name,
-                                       disk_path,
-                                       ctrller_path,
-                                       slot)
-        except os_win_exc.HyperVException as exn:
-            LOG.exception(_LE('Attach volume failed to %(instance_name)s: '
-                              '%(exn)s'), {'instance_name': instance_name,
-                                           'exn': exn})
-            raise exception.VolumeAttachFailed(
-                volume_id=connection_info['data']['volume_id'],
-                reason=exn.message)
-
-    def detach_volume(self, connection_info, instance_name):
-        LOG.debug("Detaching volume: %(connection_info)s "
-                  "from %(instance_name)s",
-                  {'connection_info': connection_info,
-                   'instance_name': instance_name})
-
-        disk_path = self._get_disk_path(connection_info)
-        export_path = self._get_export_path(connection_info)
-
-        self._vmutils.detach_vm_disk(instance_name, disk_path,
-                                     is_physical=False)
-        self._unmount_smb_share(export_path)
-
-    def disconnect_volumes(self, block_device_mapping):
-        export_paths = set()
-        for vol in block_device_mapping:
-            connection_info = vol['connection_info']
-            export_path = self._get_export_path(connection_info)
-            export_paths.add(export_path)
-
-        for export_path in export_paths:
-            self._unmount_smb_share(export_path)
-
     def _get_export_path(self, connection_info):
         return connection_info['data']['export'].replace('/', '\\')
 
-    def _get_disk_path(self, connection_info):
+    @export_path_synchronized
-        export = self._get_export_path(connection_info)
+    def attach_volume(self, *args, **kwargs):
-        disk_name = connection_info['data']['name']
+        super(SMBFSVolumeDriver, self).attach_volume(*args, **kwargs)
-        disk_path = os.path.join(export, disk_name)
-        return disk_path
 
-    def ensure_share_mounted(self, connection_info):
+    @export_path_synchronized
-        export_path = self._get_export_path(connection_info)
+    def disconnect_volume(self, *args, **kwargs):
-
+        # We synchronize those operations based on the share path in order to
-        if not self._smbutils.check_smb_mapping(export_path):
+        # avoid the situation when a SMB share is unmounted while a volume
-            opts_str = connection_info['data'].get('options', '')
+        # exported by it is about to be attached to an instance.
-            username, password = self._parse_credentials(opts_str)
+        super(SMBFSVolumeDriver, self).disconnect_volume(*args, **kwargs)
-            self._smbutils.mount_smb_share(export_path,
-                                           username=username,
-                                           password=password)
-
-    def _parse_credentials(self, opts_str):
-        match = self._username_regex.findall(opts_str)
-        username = match[0] if match and match[0] != 'guest' else None
-
-        match = self._password_regex.findall(opts_str)
-        password = match[0] if match else None
-
-        return username, password
-
-    def initialize_volume_connection(self, connection_info):
-        self.ensure_share_mounted(connection_info)
-
-    def _unmount_smb_share(self, export_path):
-        # We synchronize share unmount and volume attach operations based on
-        # the share path in order to avoid the situation when a SMB share is
-        # unmounted while a volume exported by it is about to be attached to
-        # an instance.
-        @utils.synchronized(export_path)
-        def unmount_synchronized():
-            self._smbutils.unmount_smb_share(export_path)
-        unmount_synchronized()
 
     def set_disk_qos_specs(self, connection_info, qos_specs):
         supported_qos_specs = ['total_iops_sec', 'total_bytes_sec']
@@ -479,5 +357,10 @@ class SMBFSVolumeDriver(object):
                                 total_bytes_sec))
 
         if total_iops_sec:
-            disk_path = self._get_disk_path(connection_info)
+            disk_path = self.get_disk_resource_path(connection_info)
             self._vmutils.set_disk_qos_specs(disk_path, total_iops_sec)
+
+
+class FCVolumeDriver(BaseVolumeDriver):
+    _is_block_dev = True
+    _protocol = constants.STORAGE_PROTOCOL_FC

releasenotes/notes/bp-hyperv-use-os-brick-bf576a5bc97f0ea2.yaml

@@ -0,0 +1,11 @@
+---
+features:
+  - |
+    The Hyper-V driver now uses os-brick for volume related
+    operations, introducing the following new features:
+
+    - Attaching volumes over fibre channel on a passthrough
+      basis.
+    - Improved iSCSI MPIO support, by connecting to multiple
+      iSCSI targets/portals when available and allowing using
+      a predefined list of initiator HBAs.