Use is_instance_storage_share instead of ssh

The libvirt-driver implements is_instance_storage_share twice:
Once on driver-internally as `_is_path_shared_with` needing an ssh
trust, and via the driver interface `is_instance_storage_share`,
relying on RPC.

By moving exposing `is_instance_storage_share` in ComputeVirtAPI,
the driver can use this instead of the internal method removing
the need for an SSH key in case of having a shared storage.

Change-Id: Icddad1ab000ea07dea7af3aa5f7c18974f4eb8fb

nova/compute/manager.py

@@ -522,6 +522,9 @@ def update_compute_provider_status(self, context, rp_uuid, enabled):
             self.reportclient.set_traits_for_provider(
                 context, rp_uuid, new_traits, generation=trait_info.generation)
 
+    def is_instance_storage_shared(self, context, instance, host=None):
+        self._compute._is_instance_storage_shared(context, instance, host)
+
 
 class ComputeManager(manager.Manager):
     """Manages the running instances from creation to destruction."""

nova/tests/unit/compute/test_virtapi.py

@@ -56,6 +56,12 @@ def test_update_compute_provider_status(self):
                             nova_context.get_admin_context(), uuids.rp_uuid,
                             enabled=False)
 
+    def test_is_instance_storage_shared(self):
+        self.assertExpected('is_instance_storage_shared',
+                            nova_context.get_admin_context(),
+                            mock.sentinel.instance,
+                            mock.sentinel.host)
+
 
 class FakeVirtAPITest(VirtAPIBaseTest):
 
@@ -74,6 +80,8 @@ def assertExpected(self, method, *args, **kwargs):
             self.virtapi.exit_wait_early(*args, **kwargs)
         elif method == 'update_compute_provider_status':
             self.virtapi.update_compute_provider_status(*args, **kwargs)
+        elif method == 'is_instance_storage_shared':
+            self.virtapi.is_instance_storage_shared(*args, **kwargs)
         else:
             self.fail("Unhandled FakeVirtAPI method: %s" % method)
 
@@ -116,6 +124,9 @@ def _prepare_for_instance_event(self, instance, name, tag):
         self._events.append(m)
         return m
 
+    def _is_instance_storage_shared(self, context, instance, host):
+        return
+
 
 class ComputeVirtAPITest(VirtAPIBaseTest):
 
@@ -230,3 +241,16 @@ def test_update_compute_provider_status(self):
         self.virtapi.update_compute_provider_status(
             ctxt, uuids.rp_uuid, enabled=True)
         self.assertEqual(set(), self.compute.provider_traits[uuids.rp_uuid])
+
+    def test_is_instance_storage_shared(self):
+        ctxt = nova_context.get_admin_context()
+
+        with mock.patch.object(self.compute,
+                                '_is_instance_storage_shared') as mock_shared:
+            self.virtapi.is_instance_storage_shared(ctxt,
+                                                    mock.sentinel.instance,
+                                                    mock.sentinel.host)
+            mock_shared.assert_called_once_with(self.virtapi,
+                                                ctxt,
+                                                mock.sentinel.instance,
+                                                mock.sentinel.host)

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

@@ -18512,48 +18512,32 @@ def test_set_cache_mode_invalid_object(self):
         drvr._set_cache_mode(fake_conf)
         self.assertEqual(fake_conf.driver_cache, 'fake')
 
-    @mock.patch('os.unlink')
-    @mock.patch.object(os.path, 'exists')
+    @mock.patch.object(fake.FakeVirtAPI, 'is_instance_storage_shared')
     def _test_shared_storage_detection(self, is_same,
-                                       mock_exists, mock_unlink):
+                                       mock_is_instance_storage_shared):
         drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True)
         drvr.get_host_ip_addr = mock.MagicMock(return_value='bar')
-        mock_exists.return_value = is_same
-        with test.nested(
-            mock.patch.object(drvr._remotefs, 'create_file'),
-            mock.patch.object(drvr._remotefs, 'remove_file')
-        ) as (mock_rem_fs_create, mock_rem_fs_remove):
-            result = drvr._is_path_shared_with('host', '/path')
-        mock_rem_fs_create.assert_any_call('host', mock.ANY)
-        create_args, create_kwargs = mock_rem_fs_create.call_args
-        self.assertTrue(create_args[1].startswith('/path'))
-        if is_same:
-            mock_unlink.assert_called_once_with(mock.ANY)
-        else:
-            mock_rem_fs_remove.assert_called_with('host', mock.ANY)
-            remove_args, remove_kwargs = mock_rem_fs_remove.call_args
-            self.assertTrue(remove_args[1].startswith('/path'))
-        return result
+        mock_is_instance_storage_shared.return_value = is_same
+        return drvr._is_instance_storage_shared(None,
+                                                mock.sentinel.instance,
+                                                'host')
 
     def test_shared_storage_detection_same_host(self):
         self.assertTrue(self._test_shared_storage_detection(True))
 
     def test_shared_storage_detection_different_host(self):
         self.assertFalse(self._test_shared_storage_detection(False))
 
-    @mock.patch.object(os, 'unlink')
-    @mock.patch.object(os.path, 'exists')
-    @mock.patch('oslo_concurrency.processutils.execute')
+    @mock.patch.object(fake.FakeVirtAPI, 'is_instance_storage_shared')
     @mock.patch.object(libvirt_driver.LibvirtDriver, 'get_host_ip_addr',
                        return_value='foo')
-    def test_shared_storage_detection_easy(self, mock_get, mock_exec,
-                                           mock_exists, mock_unlink):
+    def test_shared_storage_detection_easy(self, mock_get,
+                                           mock_is_instance_storage_shared):
         drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True)
-        self.assertTrue(drvr._is_path_shared_with('foo', '/path'))
+        self.assertTrue(drvr._is_instance_storage_shared(
+            None, mock.sentinel.instance, 'foo'))
         mock_get.assert_called_once_with()
-        mock_exec.assert_not_called()
-        mock_exists.assert_not_called()
-        mock_unlink.assert_not_called()
+        mock_is_instance_storage_shared.assert_not_called()
 
     def test_store_pid_remove_pid(self):
         instance = objects.Instance(**self.test_instance)
@@ -21585,7 +21569,7 @@ def _create_instance(self, params=None):
     @mock.patch('nova.virt.libvirt.driver.LibvirtDriver.get_host_ip_addr',
                 return_value='10.0.0.1')
     @mock.patch(('nova.virt.libvirt.driver.LibvirtDriver.'
-                 '_is_path_shared_with'), return_value=False)
+                 '_is_instance_storage_shared'), return_value=False)
     @mock.patch('os.rename')
     @mock.patch('os.path.exists', return_value=True)
     @mock.patch('oslo_concurrency.processutils.execute',
@@ -21616,7 +21600,7 @@ def test_migrate_disk_and_power_off_exception(
     @mock.patch('nova.virt.libvirt.driver.LibvirtDriver.get_host_ip_addr',
                 return_value='10.0.0.1')
     @mock.patch(('nova.virt.libvirt.driver.LibvirtDriver.'
-                 '_is_path_shared_with'), return_value=False)
+                 '_is_instance_storage_shared'), return_value=False)
     @mock.patch('os.rename')
     @mock.patch('os.path.exists', return_value=True)
     @mock.patch('oslo_concurrency.processutils.execute')
@@ -21656,6 +21640,7 @@ def _test_migrate_disk_and_power_off(
         mock_vtpm.assert_called_with(
             instance.uuid, mock.ANY, mock.ANY, '10.0.0.1', mock.ANY, mock.ANY)
         mock_unplug_vifs.assert_called_once()
+        return instance
 
     def test_migrate_disk_and_power_off(self):
         flavor = {'root_gb': 10, 'ephemeral_gb': 20}
@@ -21768,14 +21753,14 @@ def _test_migrate_disk_and_power_off_resize_check(self, expected_exc):
         """Test for nova.virt.libvirt.libvirt_driver.LibvirtConnection
         .migrate_disk_and_power_off.
         """
-        instance = self._create_instance()
+        self.instance = self._create_instance()
         flavor = {'root_gb': 10, 'ephemeral_gb': 20}
         flavor_obj = objects.Flavor(**flavor)
 
         # Migration is not implemented for LVM backed instances
         self.assertRaises(expected_exc,
               self.drvr.migrate_disk_and_power_off,
-              None, instance, '10.0.0.1', flavor_obj, None)
+              None, self.instance, '10.0.0.1', flavor_obj, None)
 
     @mock.patch('nova.virt.libvirt.driver.LibvirtDriver.unplug_vifs',
                 new=mock.Mock())
@@ -21786,7 +21771,7 @@ def _test_migrate_disk_and_power_off_resize_check(self, expected_exc):
     @mock.patch('nova.virt.libvirt.driver.LibvirtDriver'
                 '._get_instance_disk_info')
     @mock.patch('nova.virt.libvirt.driver.LibvirtDriver'
-                '._is_path_shared_with')
+                '._is_instance_storage_shared')
     def _test_migrate_disk_and_power_off_backing_file(self,
                                                       shared_storage,
                                                       mock_is_shared_storage,
@@ -21839,7 +21824,7 @@ def test_migrate_disk_and_power_off_lvm(self):
         self._test_migrate_disk_and_power_off_resize_check(expected_exc)
 
     @mock.patch.object(libvirt_driver.LibvirtDriver,
-                       '_is_path_shared_with', return_value=False)
+                       '_is_instance_storage_shared', return_value=False)
     def test_migrate_disk_and_power_off_resize_cannot_ssh(self,
                                                           mock_is_shared):
         def fake_execute(*args, **kwargs):
@@ -21848,7 +21833,7 @@ def fake_execute(*args, **kwargs):
 
         expected_exc = exception.InstanceFaultRollback
         self._test_migrate_disk_and_power_off_resize_check(expected_exc)
-        mock_is_shared.assert_called_once_with('10.0.0.1', test.MatchType(str))
+        mock_is_shared.assert_called_once_with(None, self.instance, '10.0.0.1')
 
     @mock.patch('nova.virt.libvirt.driver.LibvirtDriver'
                 '._get_instance_disk_info')
@@ -21968,7 +21953,7 @@ def test_migrate_disk_and_power_off_resize_error_eph(self, mock_get,
     @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._destroy')
     @mock.patch('nova.virt.libvirt.utils.get_instance_path')
     @mock.patch('nova.virt.libvirt.driver.LibvirtDriver'
-                '._is_path_shared_with')
+                '._is_instance_storage_shared')
     @mock.patch('nova.virt.libvirt.driver.LibvirtDriver'
                 '._get_instance_disk_info')
     def test_migrate_disk_and_power_off_resize_copy_disk_info(

nova/virt/fake.py

@@ -670,6 +670,9 @@ def exit_wait_early(self, events):
     def update_compute_provider_status(self, context, rp_uuid, enabled):
         pass
 
+    def is_instance_storage_shared(self, context, instance, host=None):
+        pass
+
 
 class FakeComputeVirtAPI(FakeVirtAPI):
     def __init__(self, compute):

nova/virt/libvirt/driver.py

@@ -11036,26 +11036,16 @@ def _get_disk_size_reserved_for_image_cache(self):
         return compute_utils.convert_mb_to_ceil_gb(
             self.image_cache_manager.get_disk_usage() / 1024.0 / 1024.0)
 
-    def _is_path_shared_with(self, dest, path):
+    def _is_instance_storage_shared(self, context, instance, dest):
         # NOTE (rmk): There are two methods of determining whether we are
         #             on the same filesystem: the source and dest IP are the
-        #             same, or we create a file on the dest system via SSH
-        #             and check whether the source system can also see it.
-        shared_path = (dest == self.get_host_ip_addr())
-        if not shared_path:
-            tmp_file = uuidutils.generate_uuid(dashed=False) + '.tmp'
-            tmp_path = os.path.join(path, tmp_file)
+        #             same...
+        if dest == self.get_host_ip_addr():
+            return True
 
-            try:
-                self._remotefs.create_file(dest, tmp_path)
-                if os.path.exists(tmp_path):
-                    shared_path = True
-                    os.unlink(tmp_path)
-                else:
-                    self._remotefs.remove_file(dest, tmp_path)
-            except Exception:
-                pass
-        return shared_path
+        # NOTE (fwiesel): Or we rely on the drivers api pair
+        # check_instance_shared_storage_local / check_instance_shared_storage
+        return self.virtapi.is_instance_storage_shared(context, instance, dest)
 
     def migrate_disk_and_power_off(self, context, instance, dest,
                                    flavor, network_info,
@@ -11095,7 +11085,9 @@ def migrate_disk_and_power_off(self, context, instance, dest,
         # shared storage for instance dir (eg. NFS).
         inst_base = libvirt_utils.get_instance_path(instance)
         inst_base_resize = inst_base + "_resize"
-        shared_instance_path = self._is_path_shared_with(dest, inst_base)
+        shared_instance_path = self._is_instance_storage_shared(context,
+                                                                instance,
+                                                                dest)
 
         # try to create the directory on the remote compute node
         # if this fails we pass the exception up the stack so we can catch

nova/virt/virtapi.py

@@ -34,3 +34,6 @@ def update_compute_provider_status(self, context, rp_uuid, enabled):
             the trait would be added.
         """
         raise NotImplementedError()
+
+    def is_instance_storage_shared(self, context, instance, host=None):
+        raise NotImplementedError()