From 29fb0aca1623eb0c4a565d728501667a4c5e8136 Mon Sep 17 00:00:00 2001 From: Matthew Booth Date: Fri, 18 Mar 2016 12:12:51 +0000 Subject: [PATCH] Fix conversion of config disks to qcow2 during resize/migration finish_migration contains some code which converts raw disks to qcow2 when moving from a host which uses raw disks to a host which uses qcow2 disks. This was erroneously also converted config disks, which are hard-coded to be raw even when other disks are qcow2. This has always been broken, but it wasn't previously exposed because a subsequent bug unconditionally overwrote the qcow2 config disk with a new, raw one. Change I03e08fae97416ebe5cdedcf238a06d1b90203c5d changed the order of these 2 events, so that the erroneously-converted config drive was no longer overwritten, resulting in a qcow2 file being presented to the instance as raw. This change explicitly filters out config disks from conversion. Change-Id: I6bf3cd4f9e0e152bf69732d9a17f93c86dedbd40 Closes-Bug: #1558343 --- nova/tests/unit/virt/libvirt/test_driver.py | 45 +++++++++++++++------ nova/virt/libvirt/driver.py | 41 ++++++++++++------- 2 files changed, 58 insertions(+), 28 deletions(-) diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index 9160d76dc6e6..d34d678527d9 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -13846,14 +13846,20 @@ class LibvirtDriverTestCase(test.NoDBTestCase): **inst) @staticmethod - def _disk_info(): + def _disk_info(type='qcow2', config_disk=False): # 10G root and 512M swap disk - disk_info = [{'disk_size': 1, 'type': 'qcow2', + disk_info = [{'disk_size': 1, 'type': type, 'virt_disk_size': 10737418240, 'path': '/test/disk', 'backing_file': '/base/disk'}, - {'disk_size': 1, 'type': 'qcow2', + {'disk_size': 1, 'type': type, 'virt_disk_size': 536870912, 'path': '/test/disk.swap', 'backing_file': '/base/swap_512'}] + + if config_disk: + disk_info.append({'disk_size': 1, 'type': 'raw', + 'virt_disk_size': 1024, + 'path': '/test/disk.config'}) + return jsonutils.dumps(disk_info) def test_migrate_disk_and_power_off_exception(self): @@ -14300,17 +14306,15 @@ class LibvirtDriverTestCase(test.NoDBTestCase): def test_disk_size_from_instance_disk_info(self): instance_data = {'root_gb': 10, 'ephemeral_gb': 20, 'swap_gb': 30} inst = objects.Instance(**instance_data) - info = {'path': '/path/disk'} self.assertEqual(10 * units.Gi, - self.drvr._disk_size_from_instance(inst, info)) + self.drvr._disk_size_from_instance(inst, 'disk')) - info = {'path': '/path/disk.local'} self.assertEqual(20 * units.Gi, - self.drvr._disk_size_from_instance(inst, info)) + self.drvr._disk_size_from_instance(inst, + 'disk.local')) - info = {'path': '/path/disk.swap'} self.assertEqual(0, - self.drvr._disk_size_from_instance(inst, info)) + self.drvr._disk_size_from_instance(inst, 'disk.swap')) @mock.patch('nova.utils.execute') def test_disk_raw_to_qcow2(self, mock_execute): @@ -14446,10 +14450,25 @@ class LibvirtDriverTestCase(test.NoDBTestCase): migration.source_node = 'fake-source-node' migration.dest_node = 'fake-dest-node' image_meta = objects.ImageMeta.from_dict(self.test_image_meta) - self.drvr.finish_migration( - context.get_admin_context(), migration, ins_ref, - self._disk_info(), [], image_meta, - resize_instance, None, power_on) + + # Source disks are raw to test conversion + disk_info = self._disk_info(type='raw', config_disk=True) + + with mock.patch.object(self.drvr, '_disk_raw_to_qcow2', + autospec=True) as mock_raw_to_qcow2: + self.drvr.finish_migration( + context.get_admin_context(), migration, ins_ref, + disk_info, [], image_meta, + resize_instance, None, power_on) + + # Assert that we converted the root and swap disks + convert_calls = [mock.call('/test/disk'), + mock.call('/test/disk.swap')] + mock_raw_to_qcow2.assert_has_calls(convert_calls, any_order=True) + + # Implicitly assert that we did not convert the config disk + self.assertEqual(len(convert_calls), mock_raw_to_qcow2.call_count) + self.assertTrue(self.fake_create_domain_called) self.assertEqual( resize_instance, self.fake_disk_resize_called) diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index ee9ac0764b59..a4b301288dfb 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -7256,7 +7256,7 @@ class LibvirtDriver(driver.ComputeDriver): raise loopingcall.LoopingCallDone() @staticmethod - def _disk_size_from_instance(instance, info): + def _disk_size_from_instance(instance, disk_name): """Determines the disk size from instance properties Returns the disk size by using the disk name to determine whether it @@ -7265,11 +7265,13 @@ class LibvirtDriver(driver.ComputeDriver): Returns 0 if the disk name not match (disk, disk.local). """ - fname = os.path.basename(info['path']) - if fname == 'disk': + if disk_name == 'disk': size = instance.root_gb - elif fname == 'disk.local': + elif disk_name == 'disk.local': size = instance.ephemeral_gb + # N.B. We don't handle ephemeral disks named disk.ephN here, + # which is almost certainly a bug. It's not clear what this function + # should return if an instance has multiple ephemeral disks. else: size = 0 return size * units.Gi @@ -7342,16 +7344,22 @@ class LibvirtDriver(driver.ComputeDriver): block_device_info=None, inject_files=False, fallback_from_host=migration.source_compute) - # resize disks. only "disk" and "disk.local" are necessary. + # Resize root disk and a single ephemeral disk called disk.local + # Also convert raw disks to qcow2 if migrating to host which uses + # qcow2 from host which uses raw. + # TODO(mbooth): Handle resize of multiple ephemeral disks, and + # ephemeral disks not called disk.local. disk_info = jsonutils.loads(disk_info) for info in disk_info: - size = self._disk_size_from_instance(instance, info) + path = info['path'] + disk_name = os.path.basename(path) + + size = self._disk_size_from_instance(instance, disk_name) if resize_instance: - image = imgmodel.LocalFileImage(info['path'], - info['type']) + image = imgmodel.LocalFileImage(path, info['type']) self._disk_resize(image, size) - # NOTE(mdbooth): The 2 lines below look wrong, but are actually + # NOTE(mdbooth): The code below looks wrong, but is actually # required to prevent a security hole when migrating from a host # with use_cow_images=False to one with use_cow_images=True. # Imagebackend uses use_cow_images to select between the @@ -7377,14 +7385,17 @@ class LibvirtDriver(driver.ComputeDriver): # users. It is tightly-coupled to implementation quirks of 2 # out of 5 backends in imagebackend and defends against a severe # security flaw which is not at all obvious without deep analysis, - # and is therefore undesirable to developers. It also introduces a - # bug, as it converts all disks to qcow2 regardless of their - # intended format (config disks are always supposed to be raw). We - # should aim to remove it. This will not be possible, though, until - # we can represent the storage layout of a specific instance + # and is therefore undesirable to developers. We should aim to + # remove it. This will not be possible, though, until we can + # represent the storage layout of a specific instance # independent of the default configuration of the local compute # host. - if info['type'] == 'raw' and CONF.use_cow_images: + + # Config disks are hard-coded to be raw even when + # use_cow_images=True (see _get_disk_config_image_type),so don't + # need to be converted. + if (disk_name != 'disk.config' and + info['type'] == 'raw' and CONF.use_cow_images): self._disk_raw_to_qcow2(info['path']) xml = self._get_guest_xml(context, instance, network_info,