diff --git a/nova/compute/manager.py b/nova/compute/manager.py index baf44f93c0fa..462829632685 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -9196,7 +9196,9 @@ class ComputeManager(manager.Manager): # vpmem must be cleaned do_cleanup = (not migrate_data.is_shared_instance_path or has_vpmem or has_mdevs or power_management_possible) - destroy_disks = not migrate_data.is_shared_block_storage + destroy_disks = not ( + migrate_data.is_shared_block_storage or + migrate_data.is_shared_instance_path) elif isinstance(migrate_data, migrate_data_obj.HyperVLiveMigrateData): # NOTE(claudiub): We need to cleanup any zombie Planned VM. do_cleanup = True diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py index 249d1a5302d5..9f4b19c52ef3 100644 --- a/nova/tests/unit/compute/test_compute_mgr.py +++ b/nova/tests/unit/compute/test_compute_mgr.py @@ -11363,7 +11363,7 @@ class ComputeManagerMigrationTestCase(test.NoDBTestCase, do_cleanup, destroy_disks = self.compute._live_migration_cleanup_flags( migrate_data, migr_ctxt) self.assertTrue(do_cleanup) - self.assertTrue(destroy_disks) + self.assertFalse(destroy_disks) def test_live_migration_cleanup_flags_block_migrate_libvirt(self): migrate_data = objects.LibvirtLiveMigrateData( @@ -11390,7 +11390,7 @@ class ComputeManagerMigrationTestCase(test.NoDBTestCase, do_cleanup, destroy_disks = self.compute._live_migration_cleanup_flags( migrate_data) self.assertFalse(do_cleanup) - self.assertTrue(destroy_disks) + self.assertFalse(destroy_disks) def test_live_migration_cleanup_flags_shared_libvirt(self): migrate_data = objects.LibvirtLiveMigrateData( @@ -11401,6 +11401,16 @@ class ComputeManagerMigrationTestCase(test.NoDBTestCase, self.assertFalse(do_cleanup) self.assertFalse(destroy_disks) + def test_live_migration_cleanup_flags_shared_path_libvirt_mdev(self): + migrate_data = objects.LibvirtLiveMigrateData( + is_shared_block_storage=False, + is_shared_instance_path=True, + target_mdevs={}) + do_cleanup, destroy_disks = self.compute._live_migration_cleanup_flags( + migrate_data) + self.assertTrue(do_cleanup) + self.assertFalse(destroy_disks) + def test_live_migration_cleanup_flags_live_migrate(self): do_cleanup, destroy_disks = self.compute._live_migration_cleanup_flags( {}) diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index 6c3ccb8ae2d3..b9ce37c3d0b6 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -20859,7 +20859,8 @@ class LibvirtConnTestCase(test.NoDBTestCase, # is_shared_block_storage=True and destroy_disks=False. instance = objects.Instance(self.context, **self.test_instance) migrate_data = objects.LibvirtLiveMigrateData( - is_shared_block_storage=True) + is_shared_block_storage=True, + is_shared_instance_path=False) drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI()) drvr.cleanup( self.context, instance, network_info={}, destroy_disks=False, @@ -20869,6 +20870,25 @@ class LibvirtConnTestCase(test.NoDBTestCase, self.assertTrue(instance.cleaned) save.assert_called_once_with() + @mock.patch.object(libvirt_driver.LibvirtDriver, 'delete_instance_files', + return_value=True) + @mock.patch.object(objects.Instance, 'save') + @mock.patch.object(libvirt_driver.LibvirtDriver, '_undefine_domain') + def test_cleanup_migrate_data_block_storage_and_share_instance_dir( + self, _undefine_domain, save, delete_instance_files + ): + # Test the case when the instance directory is on shared storage + # (e.g. NFS) and the instance is booted form volume. + instance = objects.Instance(self.context, **self.test_instance) + migrate_data = objects.LibvirtLiveMigrateData( + is_shared_block_storage=True, + is_shared_instance_path=True) + drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI()) + drvr.cleanup( + self.context, instance, network_info={}, destroy_disks=False, + migrate_data=migrate_data, destroy_vifs=False) + delete_instance_files.assert_not_called() + @mock.patch.object(libvirt_driver.LibvirtDriver, 'delete_instance_files', return_value=True) @mock.patch.object(objects.Instance, 'save') diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index 1d84e3173c61..9c9780477048 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -1648,12 +1648,12 @@ class LibvirtDriver(driver.ComputeDriver): cleanup_instance_dir = True cleanup_instance_disks = True else: - # NOTE(mdbooth): I think the theory here was that if this is a - # migration with shared block storage then we need to delete the - # instance directory because that's not shared. I'm pretty sure - # this is wrong. + # NOTE(mheler): For shared block storage we only need to clean up + # the instance directory when it's not on a shared path. if migrate_data and 'is_shared_block_storage' in migrate_data: - cleanup_instance_dir = migrate_data.is_shared_block_storage + cleanup_instance_dir = ( + migrate_data.is_shared_block_storage and + not migrate_data.is_shared_instance_path) # NOTE(lyarwood): The following workaround allows operators to # ensure that non-shared instance directories are removed after an diff --git a/releasenotes/notes/bug-2080436-568b03b5b5ba5760.yaml b/releasenotes/notes/bug-2080436-568b03b5b5ba5760.yaml new file mode 100644 index 000000000000..fad6e75b0024 --- /dev/null +++ b/releasenotes/notes/bug-2080436-568b03b5b5ba5760.yaml @@ -0,0 +1,8 @@ +--- +fixes: + - | + Fixes a regression for live migration on shared storage that + was removing the backing disk and instance folder during the + cleanup of a virtual machine post live migration. + `bug 2080436 + `__ for details.