From e2d304810fb4eac15f73959abc3a434975904ff5 Mon Sep 17 00:00:00 2001 From: Lucian Petrut Date: Thu, 22 Sep 2016 15:46:45 +0300 Subject: [PATCH] Properly handle shared storage during live migration After a live migration is performed, we always try to delete the instance files. If the migration fails, the destination node is not cleaned up as the HyperVLiveMigrateData object is not used at the moment and the nova compute manager won't know whether a cleanup is needed or not. Also, in order to support using CSVs for storing instance files, we need to explicitly specify whether image files will be migrated or not when requesting os-win to migrate the VM. This change addresses those issues. Depends-On: I48dc6da29534bf0c477c742311ac15438d8b63ad Change-Id: Icf2f258ef8900b5970ff153a7b981ee53e58dd2d Partial-Bug: #1565895 --- hyperv/nova/cluster/livemigrationops.py | 11 +- hyperv/nova/driver.py | 7 +- hyperv/nova/livemigrationops.py | 46 ++++++-- .../unit/cluster/test_livemigrationops.py | 25 ++-- hyperv/tests/unit/test_driver.py | 9 +- hyperv/tests/unit/test_livemigrationops.py | 110 ++++++++++++++---- 6 files changed, 156 insertions(+), 52 deletions(-) diff --git a/hyperv/nova/cluster/livemigrationops.py b/hyperv/nova/cluster/livemigrationops.py index 65ea37e3..aba5515a 100644 --- a/hyperv/nova/cluster/livemigrationops.py +++ b/hyperv/nova/cluster/livemigrationops.py @@ -67,11 +67,13 @@ class ClusterLiveMigrationOps(livemigrationops.LiveMigrationOps): with excutils.save_and_reraise_exception(): LOG.debug("Calling live migration recover_method " "for instance.", instance=instance_ref) - recover_method(context, instance_ref, dest, block_migration) + recover_method(context, instance_ref, dest, block_migration, + migrate_data) LOG.debug("Calling live migration post_method for instance.", instance=instance_ref) - post_method(context, instance_ref, dest, block_migration) + post_method(context, instance_ref, dest, + block_migration, migrate_data) def pre_live_migration(self, context, instance, block_device_info, network_info): @@ -81,7 +83,8 @@ class ClusterLiveMigrationOps(livemigrationops.LiveMigrationOps): super(ClusterLiveMigrationOps, self).pre_live_migration( context, instance, block_device_info, network_info) - def post_live_migration(self, context, instance, block_device_info): + def post_live_migration(self, context, instance, block_device_info, + migrate_data): if not self._is_instance_clustered(instance.name): super(ClusterLiveMigrationOps, self).post_live_migration( - context, instance, block_device_info) + context, instance, block_device_info, migrate_data) diff --git a/hyperv/nova/driver.py b/hyperv/nova/driver.py index 2031fffa..28ac48a3 100644 --- a/hyperv/nova/driver.py +++ b/hyperv/nova/driver.py @@ -240,18 +240,21 @@ class HyperVDriver(driver.ComputeDriver): block_device_info, destroy_disks=True, migrate_data=None): - self.destroy(context, instance, network_info, block_device_info) + self.destroy(context, instance, network_info, block_device_info, + destroy_disks=destroy_disks) def pre_live_migration(self, context, instance, block_device_info, network_info, disk_info, migrate_data): self._livemigrationops.pre_live_migration(context, instance, block_device_info, network_info) + return migrate_data def post_live_migration(self, context, instance, block_device_info, migrate_data=None): self._livemigrationops.post_live_migration(context, instance, - block_device_info) + block_device_info, + migrate_data) def post_live_migration_at_source(self, context, instance, network_info): """Unplug VIFs from networks at source.""" diff --git a/hyperv/nova/livemigrationops.py b/hyperv/nova/livemigrationops.py index 9985019e..eaafa5fc 100644 --- a/hyperv/nova/livemigrationops.py +++ b/hyperv/nova/livemigrationops.py @@ -50,28 +50,41 @@ class LiveMigrationOps(object): LOG.debug("live_migration called", instance=instance_ref) instance_name = instance_ref["name"] + if migrate_data and 'is_shared_instance_path' in migrate_data: + shared_storage = migrate_data.is_shared_instance_path + else: + shared_storage = ( + self._pathutils.check_remote_instances_dir_shared(dest)) + if migrate_data: + migrate_data.is_shared_instance_path = shared_storage + else: + migrate_data = migrate_data_obj.HyperVLiveMigrateData( + is_shared_instance_path=shared_storage) + try: # We must make sure that the console log workers are stopped, # otherwise we won't be able to delete / move VM log files. self._serial_console_ops.stop_console_handler(instance_name) - shared_storage = ( - self._pathutils.check_remote_instances_dir_shared(dest)) if not shared_storage: self._vmops.copy_vm_dvd_disks(instance_name, dest) self._pathutils.copy_vm_console_logs(instance_name, dest) - self._livemigrutils.live_migrate_vm(instance_name, - dest) + self._livemigrutils.live_migrate_vm( + instance_name, + dest, + migrate_disks=not shared_storage) except Exception: with excutils.save_and_reraise_exception(): LOG.debug("Calling live migration recover_method " "for instance: %s", instance_name) - recover_method(context, instance_ref, dest, block_migration) + recover_method(context, instance_ref, dest, block_migration, + migrate_data) LOG.debug("Calling live migration post_method for instance: %s", instance_name) - post_method(context, instance_ref, dest, block_migration) + post_method(context, instance_ref, dest, + block_migration, migrate_data) def pre_live_migration(self, context, instance, block_device_info, network_info): @@ -98,11 +111,14 @@ class LiveMigrationOps(object): instance.host, disk_path_mapping) - def post_live_migration(self, context, instance, block_device_info): + def post_live_migration(self, context, instance, block_device_info, + migrate_data): self._volumeops.disconnect_volumes(block_device_info) - self._pathutils.get_instance_dir(instance.name, - create_dir=False, - remove_dir=True) + + if not migrate_data.is_shared_instance_path: + self._pathutils.get_instance_dir(instance.name, + create_dir=False, + remove_dir=True) def post_live_migration_at_destination(self, ctxt, instance_ref, network_info, block_migration): @@ -114,8 +130,14 @@ class LiveMigrationOps(object): src_compute_info, dst_compute_info, block_migration=False, disk_over_commit=False): - LOG.debug("check_can_live_migrate_destination called", instance_ref) - return migrate_data_obj.HyperVLiveMigrateData() + LOG.debug("check_can_live_migrate_destination called", + instance=instance_ref) + + migrate_data = migrate_data_obj.HyperVLiveMigrateData() + migrate_data.is_shared_instance_path = ( + self._pathutils.check_remote_instances_dir_shared( + instance_ref.host)) + return migrate_data def cleanup_live_migration_destination_check(self, ctxt, dest_check_data): LOG.debug("cleanup_live_migration_destination_check called") diff --git a/hyperv/tests/unit/cluster/test_livemigrationops.py b/hyperv/tests/unit/cluster/test_livemigrationops.py index b8fc7d59..a78d302b 100644 --- a/hyperv/tests/unit/cluster/test_livemigrationops.py +++ b/hyperv/tests/unit/cluster/test_livemigrationops.py @@ -50,14 +50,16 @@ class ClusterLiveMigrationOpsTestCase(test_base.HyperVBaseTestCase): self.livemigrops.live_migration( self._fake_context, mock_instance, dest, post_method, - mock.sentinel.recover_method, block_migration=False, - migrate_data=None) + mock.sentinel.recover_method, + block_migration=mock.sentinel.block_migration, + migrate_data=mock.sentinel.migrate_data) clustutils = self.livemigrops._clustutils clustutils.live_migrate_vm.assert_called_once_with( mock_instance.name, dest) post_method.assert_called_once_with( - self._fake_context, mock_instance, dest, False) + self._fake_context, mock_instance, dest, + mock.sentinel.block_migration, mock.sentinel.migrate_data) def test_live_migration_in_cluster_exception(self): mock_instance = fake_instance.fake_instance_obj(self._fake_context) @@ -75,10 +77,14 @@ class ClusterLiveMigrationOpsTestCase(test_base.HyperVBaseTestCase): os_win_exc.HyperVVMNotFoundException, self.livemigrops.live_migration, self._fake_context, mock_instance, dest, mock.sentinel.post_method, - recover_method, block_migration=False, migrate_data=None) + recover_method, + block_migration=mock.sentinel.block_migration, + migrate_data=mock.sentinel.migrate_data) recover_method.assert_called_once_with( - self._fake_context, mock_instance, dest, False) + self._fake_context, mock_instance, dest, + mock.sentinel.block_migration, + mock.sentinel.migrate_data) @mock.patch.object(base_livemigrationops.LiveMigrationOps, 'live_migration') @@ -126,7 +132,8 @@ class ClusterLiveMigrationOpsTestCase(test_base.HyperVBaseTestCase): def test_post_live_migration_clustered(self, mock_post_live_migration): self.livemigrops.post_live_migration(self._fake_context, mock.sentinel.fake_instance, - mock.sentinel.bdi) + mock.sentinel.bdi, + mock.sentinel.migrate_data) self.assertFalse(mock_post_live_migration.called) @@ -136,8 +143,10 @@ class ClusterLiveMigrationOpsTestCase(test_base.HyperVBaseTestCase): self.livemigrops._clustutils.vm_exists.return_value = False self.livemigrops.post_live_migration(self._fake_context, mock.sentinel.fake_instance, - mock.sentinel.bdi) + mock.sentinel.bdi, + mock.sentinel.migrate_data) mock_post_live_migration.assert_called_once_with( self._fake_context, mock.sentinel.fake_instance, - mock.sentinel.bdi) + mock.sentinel.bdi, + mock.sentinel.migrate_data) diff --git a/hyperv/tests/unit/test_driver.py b/hyperv/tests/unit/test_driver.py index 551040e5..fe7242c2 100644 --- a/hyperv/tests/unit/test_driver.py +++ b/hyperv/tests/unit/test_driver.py @@ -307,14 +307,16 @@ class HyperVDriverTestCase(test_base.HyperVBaseTestCase): mock_destroy.assert_called_once_with( mock.sentinel.context, mock.sentinel.instance, - mock.sentinel.network_info, mock.sentinel.block_device_info) + mock.sentinel.network_info, mock.sentinel.block_device_info, + destroy_disks=mock.sentinel.destroy_disks) def test_pre_live_migration(self): - self.driver.pre_live_migration( + migrate_data = self.driver.pre_live_migration( mock.sentinel.context, mock.sentinel.instance, mock.sentinel.block_device_info, mock.sentinel.network_info, mock.sentinel.disk_info, mock.sentinel.migrate_data) + self.assertEqual(mock.sentinel.migrate_data, migrate_data) pre_live_migration = self.driver._livemigrationops.pre_live_migration pre_live_migration.assert_called_once_with( mock.sentinel.context, mock.sentinel.instance, @@ -328,7 +330,8 @@ class HyperVDriverTestCase(test_base.HyperVBaseTestCase): post_live_migration = self.driver._livemigrationops.post_live_migration post_live_migration.assert_called_once_with( mock.sentinel.context, mock.sentinel.instance, - mock.sentinel.block_device_info) + mock.sentinel.block_device_info, + mock.sentinel.migrate_data) def test_post_live_migration_at_source(self): self.driver.post_live_migration_at_source( diff --git a/hyperv/tests/unit/test_livemigrationops.py b/hyperv/tests/unit/test_livemigrationops.py index 2938bdce..535a52ce 100644 --- a/hyperv/tests/unit/test_livemigrationops.py +++ b/hyperv/tests/unit/test_livemigrationops.py @@ -13,7 +13,9 @@ # License for the specific language governing permissions and limitations # under the License. +import ddt import mock +from nova.objects import migrate_data as migrate_data_obj from os_win import exceptions as os_win_exc from oslo_config import cfg @@ -25,6 +27,7 @@ from hyperv.tests.unit import test_base CONF = cfg.CONF +@ddt.ddt class LiveMigrationOpsTestCase(test_base.HyperVBaseTestCase): """Unit tests for the Hyper-V LiveMigrationOps class.""" @@ -34,13 +37,21 @@ class LiveMigrationOpsTestCase(test_base.HyperVBaseTestCase): self._livemigrops = livemigrationops.LiveMigrationOps() self._livemigrops._block_dev_man = mock.MagicMock() self._livemigrops._pathutils = mock.MagicMock() + self._pathutils = self._livemigrops._pathutils + @ddt.data({'migrate_data_received': False}, + {'migrate_data_version': '1.0'}, + {'shared_storage': True}, + {'side_effect': os_win_exc.HyperVException}) + @ddt.unpack @mock.patch.object(serialconsoleops.SerialConsoleOps, 'stop_console_handler') @mock.patch('hyperv.nova.vmops.VMOps.copy_vm_dvd_disks') - def _test_live_migration(self, mock_copy_dvd_disks, - mock_stop_console_handler, side_effect=None, - shared_storage=False): + def test_live_migration(self, mock_copy_dvd_disks, + mock_stop_console_handler, side_effect=None, + shared_storage=False, + migrate_data_received=True, + migrate_data_version='1.1'): mock_instance = fake_instance.fake_instance_obj(self.context) mock_post = mock.MagicMock() mock_recover = mock.MagicMock() @@ -52,23 +63,56 @@ class LiveMigrationOpsTestCase(test_base.HyperVBaseTestCase): self._livemigrops._pathutils.check_remote_instances_dir_shared) mock_check_shared_storage.return_value = shared_storage + if migrate_data_received: + migrate_data = migrate_data_obj.HyperVLiveMigrateData() + if migrate_data_version != '1.0': + migrate_data.is_shared_instance_path = shared_storage + else: + migrate_data = None + if side_effect is os_win_exc.HyperVException: self.assertRaises(os_win_exc.HyperVException, self._livemigrops.live_migration, self.context, mock_instance, fake_dest, - mock_post, mock_recover, False, None) + mock_post, mock_recover, + mock.sentinel.block_migr, + migrate_data) mock_recover.assert_called_once_with(self.context, mock_instance, - fake_dest, False) + fake_dest, + mock.sentinel.block_migr, + migrate_data) else: self._livemigrops.live_migration(context=self.context, instance_ref=mock_instance, dest=fake_dest, post_method=mock_post, - recover_method=mock_recover) + recover_method=mock_recover, + block_migration=( + mock.sentinel.block_migr), + migrate_data=migrate_data) + + mock_post.assert_called_once_with(self.context, + mock_instance, + fake_dest, + mock.sentinel.block_migr, + mock.ANY) + # The last argument, the migrate_data object, should be created + # by the callee if not received. + post_call_args_list = mock_post.call_args_list[0][0] + migrate_data_arg = post_call_args_list[-1] + self.assertIsInstance( + migrate_data_arg, + migrate_data_obj.HyperVLiveMigrateData) + self.assertEqual(shared_storage, + migrate_data_arg.is_shared_instance_path) + + if not migrate_data_received or migrate_data_version == '1.0': + mock_check_shared_storage.assert_called_once_with(fake_dest) + else: + self.assertFalse(mock_check_shared_storage.called) mock_stop_console_handler.assert_called_once_with( mock_instance.name) - mock_check_shared_storage.assert_called_once_with(fake_dest) if not shared_storage: mock_copy_logs.assert_called_once_with(mock_instance.name, @@ -81,18 +125,9 @@ class LiveMigrationOpsTestCase(test_base.HyperVBaseTestCase): mock_live_migr = self._livemigrops._livemigrutils.live_migrate_vm mock_live_migr.assert_called_once_with(mock_instance.name, - fake_dest) - mock_post.assert_called_once_with(self.context, mock_instance, - fake_dest, False) - - def test_live_migration(self): - self._test_live_migration() - - def test_live_migration_shared_storage(self): - self._test_live_migration(shared_storage=True) - - def test_live_migration_exception(self): - self._test_live_migration(side_effect=os_win_exc.HyperVException) + fake_dest, + migrate_disks=( + not shared_storage)) @mock.patch('hyperv.nova.volumeops.VolumeOps.get_disk_path_mapping') @mock.patch('hyperv.nova.imagecache.ImageCache.get_cached_image') @@ -140,15 +175,29 @@ class LiveMigrationOpsTestCase(test_base.HyperVBaseTestCase): def test_pre_live_migration_without_phys_disks_attached(self): self._test_pre_live_migration(phys_disks_attached=False) + @ddt.data({}, + {'shared_storage': True}) + @ddt.unpack @mock.patch('hyperv.nova.volumeops.VolumeOps.disconnect_volumes') - def test_post_live_migration(self, mock_disconnect_volumes): + def test_post_live_migration(self, mock_disconnect_volumes, + shared_storage=False): + migrate_data = migrate_data_obj.HyperVLiveMigrateData( + is_shared_instance_path=shared_storage) + self._livemigrops.post_live_migration( self.context, mock.sentinel.instance, - mock.sentinel.block_device_info) + mock.sentinel.block_device_info, + migrate_data) mock_disconnect_volumes.assert_called_once_with( mock.sentinel.block_device_info) - self._livemigrops._pathutils.get_instance_dir.assert_called_once_with( - mock.sentinel.instance.name, create_dir=False, remove_dir=True) + mock_get_inst_dir = self._pathutils.get_instance_dir + + if not shared_storage: + mock_get_inst_dir.assert_called_once_with( + mock.sentinel.instance.name, + create_dir=False, remove_dir=True) + else: + self.assertFalse(mock_get_inst_dir.called) @mock.patch.object(livemigrationops.vmops.VMOps, 'post_start_vifs') def test_post_live_migration_at_destination(self, mock_post_start_vifs): @@ -158,3 +207,18 @@ class LiveMigrationOpsTestCase(test_base.HyperVBaseTestCase): mock_post_start_vifs.assert_called_once_with( mock.sentinel.instance, mock.sentinel.network_info) + + @mock.patch.object(migrate_data_obj, 'HyperVLiveMigrateData') + def test_check_can_live_migrate_destination(self, mock_migr_data_cls): + mock_instance = fake_instance.fake_instance_obj(self.context) + migr_data = self._livemigrops.check_can_live_migrate_destination( + mock.sentinel.context, mock_instance, mock.sentinel.src_comp_info, + mock.sentinel.dest_comp_info) + + mock_check_shared_inst_dir = ( + self._pathutils.check_remote_instances_dir_shared) + mock_check_shared_inst_dir.assert_called_once_with(mock_instance.host) + + self.assertEqual(mock_migr_data_cls.return_value, migr_data) + self.assertEqual(mock_check_shared_inst_dir.return_value, + migr_data.is_shared_instance_path)