From 174764340d3c965d31143b39af4ab2e8ecefe594 Mon Sep 17 00:00:00 2001 From: Matt Riedemann Date: Fri, 4 May 2018 12:58:07 -0400 Subject: [PATCH] libvirt: check image type before removing snapshots in _cleanup_resize Change Ic683f83e428106df64be42287e2c5f3b40e73da4 added some disk cleanup logic to _cleanup_resize because some image backends (Qcow2, Flat and Ploop) will re-create the instance directory and disk.info file when initializing the image backend object. However, that change did not take into account volume-backed instances being resized will not have a root disk *and* if the local disk is shared storage, removing the instance directory effectively deletes the instance files, like the console.log, on the destination host as well. Change I29fac80d08baf64bf69e54cf673e55123174de2a was made to resolve that issue. However (see the pattern?), if you're doing a resize of a volume-backed instance that is not on shared storage, we won't remove the instance directory from the source host in _cleanup_resize. If the admin then later tries to live migrate the instance back to that host, it will fail with DestinationDiskExists in the pre_live_migration() method. This change is essentially a revert of I29fac80d08baf64bf69e54cf673e55123174de2a and alternate fix for Ic683f83e428106df64be42287e2c5f3b40e73da4. Since the root problem is that creating certain imagebackend objects will recreate the instance directory and disk.info on the source host, we simply need to avoid creating the imagebackend object. The only reason we are getting an imagebackend object in _cleanup_resize is to remove image snapshot clones, which is only implemented by the Rbd image backend. Therefore, we can check to see if the image type supports clones and if not, don't go through the imagebackend init routine that, for some, will recreate the disk. Conflicts: nova/tests/unit/virt/libvirt/test_driver.py NOTE(mriedem): The conflict is due to not having change Icdd039bb4374269d9da38e7f8d2e15e05ca8aadb in Queens. Change-Id: Ib10081150e125961cba19cfa821bddfac4614408 Closes-Bug: #1769131 Related-Bug: #1666831 Related-Bug: #1728603 (cherry picked from commit 8e3385707cb1ced55cd12b1314d8c0b68d354c38) --- nova/tests/unit/virt/libvirt/test_driver.py | 11 +++-- nova/virt/libvirt/driver.py | 46 +++++++++------------ 2 files changed, 25 insertions(+), 32 deletions(-) diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index fc20daf5f885..18ec7a1d6393 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -18129,9 +18129,6 @@ class LibvirtDriverTestCase(test.NoDBTestCase): fake_net = _fake_network_info(self, 1) drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False) - drvr.image_backend = mock.Mock() - drvr.image_backend.by_name.return_value = drvr.image_backend - drvr.image_backend.exists.return_value = False with test.nested( mock.patch('nova.compute.utils.is_volume_backed_instance', @@ -18139,12 +18136,14 @@ class LibvirtDriverTestCase(test.NoDBTestCase): mock.patch.object(os.path, 'exists'), mock.patch.object(libvirt_utils, 'get_instance_path'), mock.patch.object(utils, 'execute'), - mock.patch.object(shutil, 'rmtree'), + mock.patch.object(drvr.image_backend, 'by_name', + new_callable=mock.NonCallableMock), mock.patch.object(drvr, '_undefine_domain'), mock.patch.object(drvr, 'unplug_vifs'), mock.patch.object(drvr, 'unfilter_instance') ) as (mock_volume_backed, mock_exists, mock_get_path, - mock_exec, mock_rmtree, mock_undef, mock_unplug, mock_unfilter): + mock_exec, mock_image_by_name, mock_undef, mock_unplug, + mock_unfilter): mock_exists.return_value = True mock_get_path.return_value = '/fake/inst' @@ -18152,7 +18151,6 @@ class LibvirtDriverTestCase(test.NoDBTestCase): mock_get_path.assert_called_once_with(ins_ref) mock_exec.assert_called_once_with('rm', '-rf', '/fake/inst_resize', delay_on_retry=True, attempts=5) - mock_rmtree.assert_called_once_with('/fake/inst') mock_undef.assert_called_once_with(ins_ref) mock_unplug.assert_called_once_with(ins_ref, fake_net) mock_unfilter.assert_called_once_with(ins_ref, fake_net) @@ -18198,6 +18196,7 @@ class LibvirtDriverTestCase(test.NoDBTestCase): def test_cleanup_resize_snap_backend(self): CONF.set_override('policy_dirs', [], group='oslo_policy') + self.flags(images_type='rbd', group='libvirt') ins_ref = self._create_instance({'host': CONF.host}) drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False) drvr.image_backend = mock.Mock() diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index 3440fb4cb4e2..2a42b6076699 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -1203,32 +1203,26 @@ class LibvirtDriver(driver.ComputeDriver): utils.execute('rm', '-rf', target, delay_on_retry=True, attempts=5) - root_disk = self.image_backend.by_name(instance, 'disk') - # TODO(nic): Set ignore_errors=False in a future release. - # It is set to True here to avoid any upgrade issues surrounding - # instances being in pending resize state when the software is updated; - # in that case there will be no snapshot to remove. Once it can be - # reasonably assumed that no such instances exist in the wild - # anymore, it should be set back to False (the default) so it will - # throw errors, like it should. - if root_disk.exists(): - root_disk.remove_snap(libvirt_utils.RESIZE_SNAPSHOT_NAME, - ignore_errors=True) - - # NOTE(mjozefcz): - # self.image_backend.image for some backends recreates instance - # directory and image disk.info - remove it here if exists - # Do not remove inst_base for volume-backed instances since that - # could potentially remove the files on the destination host - # if using shared storage. - if (os.path.exists(inst_base) and not root_disk.exists() and - not compute_utils.is_volume_backed_instance( - context, instance)): - try: - shutil.rmtree(inst_base) - except OSError as e: - if e.errno != errno.ENOENT: - raise + # NOTE(mriedem): Some image backends will recreate the instance path + # and disk.info during init, and all we need the root disk for + # here is removing cloned snapshots which is backend-specific, so + # check that first before initializing the image backend object. If + # there is ever an image type that supports clone *and* re-creates + # the instance directory and disk.info on init, this condition will + # need to be re-visited to make sure that backend doesn't re-create + # the disk. Refer to bugs: 1666831 1728603 1769131 + if self.image_backend.backend(CONF.libvirt.images_type).SUPPORTS_CLONE: + root_disk = self.image_backend.by_name(instance, 'disk') + # TODO(nic): Set ignore_errors=False in a future release. + # It is set to True here to avoid any upgrade issues surrounding + # instances being in pending resize state when the software is + # updated; in that case there will be no snapshot to remove. + # Once it can be reasonably assumed that no such instances exist + # in the wild anymore, it should be set back to False + # (the default) so it will throw errors, like it should. + if root_disk.exists(): + root_disk.remove_snap(libvirt_utils.RESIZE_SNAPSHOT_NAME, + ignore_errors=True) if instance.host != CONF.host: self._undefine_domain(instance)