Hyper-V: properly handle loopback shares

At the moment, the Hyper-V driver uses the UNC path of images
stored on SMB shares, regardless if the share is remote or not.
According to the MS docs, this is not supported for SOFS SMB shares.

This is troublesome for the Hyper-C scenario, as Hyper-V will attempt
to modify the image ACLs, making them unusable.

The fix consists in checking if the share is local, and use the
local path in that case.

Depends-On: I8e4426b3b1044d24563adf7826ab9f141c2495b8
Closes-Bug: #1580122

Change-Id: Ib736b39f3bfa58b2737647de23f741ffcf85180b

hyperv/nova/pathutils.py

@@ -20,10 +20,14 @@ import time
 import nova.conf
 from nova import exception
 from os_win.utils import pathutils
+from os_win import utilsfactory
+from oslo_log import log as logging
 
-from hyperv.i18n import _
+from hyperv.i18n import _, _LE
 from hyperv.nova import constants
 
+LOG = logging.getLogger(__name__)
+
 CONF = nova.conf.CONF
 CONF.import_opt('instances_path', 'nova.compute.manager')
 
@@ -36,6 +40,10 @@ ERROR_INVALID_NAME = 123
 # the os_win.pathutils.PathUtils class into this PathUtils.
 class PathUtils(pathutils.PathUtils):
 
+    def __init__(self):
+        super(PathUtils, self).__init__()
+        self._smbutils = utilsfactory.get_smbutils()
+
     def get_instances_dir(self, remote_server=None):
         local_instance_path = os.path.normpath(CONF.instances_path)
 
@@ -179,3 +187,18 @@ class PathUtils(pathutils.PathUtils):
 
             shared_storage = os.path.exists(src_path)
         return shared_storage
+
+    def get_loopback_share_path(self, share_address):
+        # In case of loopback shares, we use the local share path.
+        share_name = share_address.strip('\\').split('\\')[1]
+        local_share_path = self._smbutils.get_smb_share_path(
+            share_name)
+        try:
+            if local_share_path and self.check_dirs_shared_storage(
+                    local_share_path, share_address):
+                return local_share_path
+        except Exception:
+            err_msg = _LE("Got exception while verifying if share %s "
+                          "is local, thus it will not be handled as a "
+                          "loopback share.")
+            LOG.exception(err_msg)

hyperv/nova/volumeops.py

@@ -40,6 +40,7 @@ import six
 
 from hyperv.i18n import _, _LI, _LE, _LW
 from hyperv.nova import constants
+from hyperv.nova import pathutils
 
 LOG = logging.getLogger(__name__)
 
@@ -483,6 +484,7 @@ class SMBFSVolumeDriver(BaseVolumeDriver):
     _is_block_dev = False
 
     def __init__(self):
+        self._pathutils = pathutils.PathUtils()
         self._smbutils = utilsfactory.get_smbutils()
         self._username_regex = re.compile(r'user(?:name)?=([^, ]+)')
         self._password_regex = re.compile(r'pass(?:word)?=([^, ]+)')
@@ -517,10 +519,17 @@ class SMBFSVolumeDriver(BaseVolumeDriver):
         self._smbutils.unmount_smb_share(export_path)
 
     def _get_export_path(self, connection_info):
-        return connection_info['data']['export'].replace('/', '\\')
+        return connection_info[
+            'data']['export'].replace('/', '\\').rstrip('\\')
 
     def _get_disk_path(self, connection_info):
         export = self._get_export_path(connection_info)
+
+        # In case of loopback shares, we use the local share path.
+        local_share_path = self._pathutils.get_loopback_share_path(export)
+        if local_share_path:
+            export = local_share_path
+
         disk_name = connection_info['data']['name']
         disk_path = os.path.join(export, disk_name)
         return disk_path

hyperv/tests/unit/test_pathutils.py

@@ -15,6 +15,7 @@
 import os
 import time
 
+import ddt
 import mock
 from nova import exception
 from six.moves import builtins
@@ -24,6 +25,7 @@ from hyperv.nova import pathutils
 from hyperv.tests.unit import test_base
 
 
+@ddt.ddt
 class PathUtilsTestCase(test_base.HyperVBaseTestCase):
     """Unit tests for the Hyper-V PathUtils class."""
 
@@ -34,6 +36,8 @@ class PathUtilsTestCase(test_base.HyperVBaseTestCase):
 
         self._pathutils = pathutils.PathUtils()
         self._pathutils._smb_conn_attr = mock.MagicMock()
+        self._pathutils._smbutils = mock.MagicMock()
+        self._smbutils = self._pathutils._smbutils
 
     def _mock_lookup_configdrive_path(self, ext, rescue=False):
         self._pathutils.get_instance_dir = mock.MagicMock(
@@ -173,3 +177,34 @@ class PathUtilsTestCase(test_base.HyperVBaseTestCase):
 
         mock_named_tempfile.assert_called_once_with(dir=fake_dest_dir)
         mock_exists.assert_called_once_with(expected_src_tmp_path)
+
+    @ddt.data({},
+              {'local_share_path': None},
+              {'is_same_dir': True},
+              {'raised_exc': Exception})
+    @ddt.unpack
+    @mock.patch.object(pathutils.PathUtils, 'check_dirs_shared_storage')
+    def test_get_loopback_share_path(
+            self, mock_check_dirs_shared_storage,
+            local_share_path=mock.sentinel.local_share_path,
+            is_same_dir=False, raised_exc=None):
+        self._smbutils.get_smb_share_path.return_value = local_share_path
+        mock_check_dirs_shared_storage.side_effect = (
+            raised_exc or [is_same_dir])
+
+        share_address = r'\\1.2.3.4\fake_share'
+        expected_path = (
+            local_share_path
+            if local_share_path and is_same_dir and not raised_exc
+            else None)
+        share_path = self._pathutils.get_loopback_share_path(share_address)
+
+        self.assertEqual(expected_path, share_path)
+        self._smbutils.get_smb_share_path.assert_called_once_with(
+            'fake_share')
+
+        if local_share_path:
+            mock_check_dirs_shared_storage.assert_called_once_with(
+                local_share_path, share_address)
+        else:
+            self.assertFalse(mock_check_dirs_shared_storage.called)

hyperv/tests/unit/test_volumeops.py

@@ -651,6 +651,7 @@ class ISCSIVolumeDriverTestCase(test_base.HyperVBaseTestCase):
         mock_get_mounted_disk.assert_called_once_with(mock.sentinel.dev_path)
 
 
+@ddt.ddt
 class SMBFSVolumeDriverTestCase(test_base.HyperVBaseTestCase):
     """Unit tests for the Hyper-V SMBFSVolumeDriver class."""
 
@@ -671,6 +672,9 @@ class SMBFSVolumeDriverTestCase(test_base.HyperVBaseTestCase):
         self._volume_driver = volumeops.SMBFSVolumeDriver()
         self._volume_driver._vmutils = mock.MagicMock()
         self._volume_driver._smbutils = mock.MagicMock()
+        self._volume_driver._pathutils = mock.MagicMock()
+        self._smbutils = self._volume_driver._smbutils
+        self._pathutils = self._volume_driver._pathutils
 
     @mock.patch.object(volumeops.SMBFSVolumeDriver, '_get_disk_path')
     def test_get_disk_resource_path(self, mock_get_disk_path):
@@ -693,13 +697,19 @@ class SMBFSVolumeDriverTestCase(test_base.HyperVBaseTestCase):
         expected = self._FAKE_SHARE.replace('/', '\\')
         self.assertEqual(expected, result)
 
-    def test_get_disk_path(self):
-        expected = os.path.join(self._FAKE_SHARE_NORMALIZED,
-                                self._FAKE_DISK_NAME)
-
+    @ddt.data(True, False)
+    def test_get_disk_path(self, is_local):
+        fake_local_share_path = 'fake_local_share_path' if is_local else None
+        mock_get_loopback_share_path = self._pathutils.get_loopback_share_path
+        mock_get_loopback_share_path.return_value = fake_local_share_path
+        expected = os.path.join(
+            fake_local_share_path or self._FAKE_SHARE_NORMALIZED,
+            self._FAKE_DISK_NAME)
         disk_path = self._volume_driver._get_disk_path(
             self._FAKE_CONNECTION_INFO)
 
+        mock_get_loopback_share_path.assert_called_once_with(
+            self._FAKE_SHARE_NORMALIZED)
         self.assertEqual(expected, disk_path)
 
     @mock.patch.object(volumeops.SMBFSVolumeDriver, '_parse_credentials')