From af9392168b2d36d20c39f7e749f3a511802dc7e9 Mon Sep 17 00:00:00 2001 From: Fabian Wiesel Date: Mon, 23 Sep 2024 15:27:09 +0200 Subject: [PATCH] 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 | 4 ++ nova/tests/unit/virt/libvirt/test_driver.py | 56 ++++++++------------- nova/virt/fake.py | 3 ++ nova/virt/libvirt/driver.py | 27 ++++------ 4 files changed, 36 insertions(+), 54 deletions(-) diff --git a/nova/compute/manager.py b/nova/compute/manager.py index e89199cd4bf..e29634c1a8b 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -522,6 +522,10 @@ 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( + self, context, instance, host) + class ComputeManager(manager.Manager): """Manages the running instances from creation to destruction.""" diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index 23b7b060907..96832d3115e 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -18512,28 +18512,13 @@ 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)) @@ -18541,19 +18526,16 @@ def test_shared_storage_detection_same_host(self): 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 +21567,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 +21598,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 +21638,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 +21751,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 +21769,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,16 +21822,17 @@ 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): + # TODO how to do this def fake_execute(*args, **kwargs): raise processutils.ProcessExecutionError() self.stub_out('oslo_concurrency.processutils.execute', fake_execute) 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 +21952,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( diff --git a/nova/virt/fake.py b/nova/virt/fake.py index 11be13ef9ef..8b1659c6940 100644 --- a/nova/virt/fake.py +++ b/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): diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index 85a4b463ed2..44e76ebc97a 100644 --- a/nova/virt/libvirt/driver.py +++ b/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, host=dest) def migrate_disk_and_power_off(self, context, instance, dest, flavor, network_info, @@ -11095,7 +11085,8 @@ 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