From cc2ae7526b35155687a5a1abbe4f2f80618c8f87 Mon Sep 17 00:00:00 2001 From: Matthew Booth Date: Wed, 8 Jun 2016 10:13:00 +0100 Subject: [PATCH] Remove max_size argument to images.fetch and fetch_to_raw images.fetch was passed a max_size argument, but did not use it. images.fetch_to_raw used the max_size argument to check that the image being downloaded is not larger than target instance's root disk. However, this check does not make sense in this context. fetch_to_raw is used to download directly to the image cache, which means when booting multiple instances on the same compute it only executes once. However, the check obviously needs to happen against every instance, not just the first to use a particular image. Consequently every image backend duplicates this check, making the check in fetch_to_raw both confusing and redundant. There are a couple of callers outside the libvirt driver. These do not pass the max_size argument, and are therefore unaffected. Implements: blueprint libvirt-instance-storage Change-Id: I70a559f3dc9b59097ff6923920f4377cca00d1b2 --- nova/tests/unit/virt/libvirt/test_driver.py | 12 +++---- .../unit/virt/libvirt/test_imagebackend.py | 31 +++++++++---------- nova/tests/unit/virt/libvirt/test_utils.py | 21 +++---------- nova/virt/images.py | 26 +++------------- nova/virt/libvirt/driver.py | 6 ++-- nova/virt/libvirt/imagebackend.py | 10 +++--- nova/virt/libvirt/utils.py | 8 ++--- 7 files changed, 39 insertions(+), 75 deletions(-) diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index 0bd0d37bf038..59d750120ec9 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -9247,8 +9247,7 @@ class LibvirtConnTestCase(test.NoDBTestCase): fetch_image_mock.assert_has_calls([ mock.call(context=self.context, target=backfile_path, - image_id=self.test_instance['image_ref'], - max_size=25165824), + image_id=self.test_instance['image_ref']), mock.call(self.context, kernel_path, self.test_instance['kernel_id']), mock.call(self.context, ramdisk_path, @@ -10609,7 +10608,7 @@ class LibvirtConnTestCase(test.NoDBTestCase): def side_effect(fetch_func, filename, size=None, *args, **kwargs): def second_call(fetch_func, filename, size=None, *args, **kwargs): # call copy_from_host ourselves because we mocked image.cache() - fetch_func('fake-target', 'fake-max-size') + fetch_func('fake-target') # further calls have no side effect mock_cache.side_effect = None mock_cache.side_effect = second_call @@ -10654,8 +10653,7 @@ class LibvirtConnTestCase(test.NoDBTestCase): self.flags(default_ephemeral_format='ext3') drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False) drvr._create_ephemeral('/dev/something', 20, 'myVol', 'linux', - is_block_dev=True, max_size=20, - specified_fs='ext4') + is_block_dev=True, specified_fs='ext4') mock_exec.assert_called_once_with('mkfs', '-t', 'ext4', '-F', '-L', 'myVol', '/dev/something', run_as_root=True) @@ -10696,7 +10694,7 @@ class LibvirtConnTestCase(test.NoDBTestCase): '/dev/something', run_as_root=True) self.mox.ReplayAll() drvr._create_ephemeral('/dev/something', 20, 'myVol', 'linux', - is_block_dev=True, max_size=20) + is_block_dev=True) def test_create_ephemeral_with_conf(self): CONF.set_override('default_ephemeral_format', 'ext4') @@ -10736,7 +10734,7 @@ class LibvirtConnTestCase(test.NoDBTestCase): utils.execute('mkswap', '/dev/something', run_as_root=False) self.mox.ReplayAll() - drvr._create_swap('/dev/something', 1, max_size=20) + drvr._create_swap('/dev/something', 1) def test_get_console_output_file(self): fake_libvirt_utils.files['console.log'] = '01234567890' diff --git a/nova/tests/unit/virt/libvirt/test_imagebackend.py b/nova/tests/unit/virt/libvirt/test_imagebackend.py index 9aa2e2997f23..4d8527ef7671 100644 --- a/nova/tests/unit/virt/libvirt/test_imagebackend.py +++ b/nova/tests/unit/virt/libvirt/test_imagebackend.py @@ -281,7 +281,7 @@ class FlatTestCase(_ImageTestCase, test.NoDBTestCase): def test_create_image(self): fn = self.prepare_mocks() - fn(target=self.TEMPLATE_PATH, max_size=None, image_id=None) + fn(target=self.TEMPLATE_PATH, image_id=None) imagebackend.libvirt_utils.copy_image(self.TEMPLATE_PATH, self.PATH) self.mox.ReplayAll() @@ -304,7 +304,7 @@ class FlatTestCase(_ImageTestCase, test.NoDBTestCase): return_value=imageutils.QemuImgInfo()) def test_create_image_extend(self, fake_qemu_img_info): fn = self.prepare_mocks() - fn(max_size=self.SIZE, target=self.TEMPLATE_PATH, image_id=None) + fn(target=self.TEMPLATE_PATH, image_id=None) imagebackend.libvirt_utils.copy_image(self.TEMPLATE_PATH, self.PATH) image = imgmodel.LocalFileImage(self.PATH, imgmodel.FORMAT_RAW) imagebackend.disk.extend(image, self.SIZE) @@ -443,7 +443,7 @@ class Qcow2TestCase(_ImageTestCase, test.NoDBTestCase): def test_create_image(self): fn = self.prepare_mocks() - fn(max_size=None, target=self.TEMPLATE_PATH) + fn(target=self.TEMPLATE_PATH) imagebackend.libvirt_utils.create_cow_image(self.TEMPLATE_PATH, self.PATH) self.mox.ReplayAll() @@ -455,7 +455,7 @@ class Qcow2TestCase(_ImageTestCase, test.NoDBTestCase): def test_create_image_with_size(self): fn = self.prepare_mocks() - fn(max_size=self.SIZE, target=self.TEMPLATE_PATH) + fn(target=self.TEMPLATE_PATH) self.mox.StubOutWithMock(os.path, 'exists') self.mox.StubOutWithMock(imagebackend.Image, 'verify_base_size') @@ -498,7 +498,7 @@ class Qcow2TestCase(_ImageTestCase, test.NoDBTestCase): def test_generate_resized_backing_files(self): fn = self.prepare_mocks() - fn(max_size=self.SIZE, target=self.TEMPLATE_PATH) + fn(target=self.TEMPLATE_PATH) self.mox.StubOutWithMock(os.path, 'exists') self.mox.StubOutWithMock(imagebackend.libvirt_utils, 'get_disk_backing_file') @@ -531,7 +531,7 @@ class Qcow2TestCase(_ImageTestCase, test.NoDBTestCase): def test_qcow2_exists_and_has_no_backing_file(self): fn = self.prepare_mocks() - fn(max_size=self.SIZE, target=self.TEMPLATE_PATH) + fn(target=self.TEMPLATE_PATH) self.mox.StubOutWithMock(os.path, 'exists') self.mox.StubOutWithMock(imagebackend.libvirt_utils, 'get_disk_backing_file') @@ -597,7 +597,7 @@ class LvmTestCase(_ImageTestCase, test.NoDBTestCase): def _create_image(self, sparse): fn = self.prepare_mocks() - fn(max_size=None, target=self.TEMPLATE_PATH) + fn(target=self.TEMPLATE_PATH) self.lvm.create_volume(self.VG, self.LV, self.TEMPLATE_SIZE, @@ -629,7 +629,7 @@ class LvmTestCase(_ImageTestCase, test.NoDBTestCase): def _create_image_resize(self, sparse): fn = self.prepare_mocks() - fn(max_size=self.SIZE, target=self.TEMPLATE_PATH) + fn(target=self.TEMPLATE_PATH) self.lvm.create_volume(self.VG, self.LV, self.SIZE, sparse=sparse) self.disk.get_disk_size(self.TEMPLATE_PATH @@ -720,7 +720,7 @@ class LvmTestCase(_ImageTestCase, test.NoDBTestCase): def test_create_image_negative(self): fn = self.prepare_mocks() - fn(max_size=self.SIZE, target=self.TEMPLATE_PATH) + fn(target=self.TEMPLATE_PATH) self.lvm.create_volume(self.VG, self.LV, self.SIZE, @@ -831,7 +831,6 @@ class EncryptedLvmTestCase(_ImageTestCase, test.NoDBTestCase): context=self.CONTEXT) fn.assert_called_with(context=self.CONTEXT, - max_size=self.TEMPLATE_SIZE, target=self.TEMPLATE_PATH) self.lvm.create_volume.assert_called_with(self.VG, self.LV, @@ -909,8 +908,8 @@ class EncryptedLvmTestCase(_ImageTestCase, test.NoDBTestCase): image.create_image(fn, self.TEMPLATE_PATH, self.SIZE, context=self.CONTEXT) - fn.assert_called_with(context=self.CONTEXT, max_size=self.SIZE, - target=self.TEMPLATE_PATH) + fn.assert_called_with(context=self.CONTEXT, + target=self.TEMPLATE_PATH) self.disk.get_disk_size.assert_called_with(self.TEMPLATE_PATH) self.lvm.create_volume.assert_called_with( self.VG, @@ -982,7 +981,6 @@ class EncryptedLvmTestCase(_ImageTestCase, test.NoDBTestCase): fn.assert_called_with( context=self.CONTEXT, - max_size=self.SIZE, target=self.TEMPLATE_PATH) self.disk.get_disk_size.assert_called_with( self.TEMPLATE_PATH) @@ -1024,7 +1022,6 @@ class EncryptedLvmTestCase(_ImageTestCase, test.NoDBTestCase): fn.assert_called_with( context=self.CONTEXT, - max_size=self.SIZE, target=self.TEMPLATE_PATH) self.disk.get_disk_size.assert_called_with(self.TEMPLATE_PATH) self.lvm.create_volume.assert_called_with( @@ -1243,7 +1240,7 @@ class RbdTestCase(_ImageTestCase, test.NoDBTestCase): def test_create_image(self): fn = self.mox.CreateMockAnything() - fn(max_size=None, target=self.TEMPLATE_PATH) + fn(target=self.TEMPLATE_PATH) rbd_utils.rbd.RBD_FEATURE_LAYERING = 1 @@ -1269,7 +1266,7 @@ class RbdTestCase(_ImageTestCase, test.NoDBTestCase): def test_create_image_resize(self): fn = self.mox.CreateMockAnything() full_size = self.SIZE * 2 - fn(max_size=full_size, target=self.TEMPLATE_PATH) + fn(target=self.TEMPLATE_PATH) rbd_utils.rbd.RBD_FEATURE_LAYERING = 1 @@ -1615,7 +1612,7 @@ class PloopTestCase(_ImageTestCase, test.NoDBTestCase): def test_create_image(self): self.stubs.Set(imagebackend.Ploop, 'get_disk_size', lambda a, b: 2048) fn = self.prepare_mocks() - fn(target=self.TEMPLATE_PATH, max_size=2048, image_id=None) + fn(target=self.TEMPLATE_PATH, image_id=None) img_path = os.path.join(self.PATH, "root.hds") imagebackend.libvirt_utils.copy_image(self.TEMPLATE_PATH, img_path) self.utils.execute("ploop", "restore-descriptor", "-f", "raw", diff --git a/nova/tests/unit/virt/libvirt/test_utils.py b/nova/tests/unit/virt/libvirt/test_utils.py index a4bc4726f524..67f5e80196dd 100644 --- a/nova/tests/unit/virt/libvirt/test_utils.py +++ b/nova/tests/unit/virt/libvirt/test_utils.py @@ -578,7 +578,7 @@ disk size: 4.4M image_id = '4' libvirt_utils.fetch_image(context, target, image_id) mock_images.assert_called_once_with( - context, image_id, target, max_size=0) + context, image_id, target) @mock.patch('nova.virt.images.fetch') def test_fetch_initrd_image(self, mock_images): @@ -590,7 +590,7 @@ disk size: 4.4M image_id = '4' libvirt_utils.fetch_raw_image(_context, target, image_id) mock_images.assert_called_once_with( - _context, image_id, target, max_size=0) + _context, image_id, target) def test_fetch_raw_image(self): @@ -622,14 +622,9 @@ disk size: 4.4M else: backing_file = None - if 'big' in path: - virtual_size = 2 - else: - virtual_size = 1 - FakeImgInfo.file_format = file_format FakeImgInfo.backing_file = backing_file - FakeImgInfo.virtual_size = virtual_size + FakeImgInfo.virtual_size = 1 return FakeImgInfo() @@ -657,7 +652,7 @@ disk size: 4.4M '-f', 'qcow2'), ('rm', 't.qcow2.part'), ('mv', 't.qcow2.converted', 't.qcow2')] - images.fetch_to_raw(context, image_id, target, max_size=1) + images.fetch_to_raw(context, image_id, target) self.assertEqual(self.executes, expected_commands) target = 't.raw' @@ -673,14 +668,6 @@ disk size: 4.4M images.fetch_to_raw, context, image_id, target) self.assertEqual(self.executes, expected_commands) - target = 'big.qcow2' - self.executes = [] - expected_commands = [('rm', '-f', 'big.qcow2.part')] - self.assertRaises(exception.FlavorDiskSmallerThanImage, - images.fetch_to_raw, - context, image_id, target, max_size=1) - self.assertEqual(self.executes, expected_commands) - del self.executes def test_get_disk_backing_file(self): diff --git a/nova/virt/images.py b/nova/virt/images.py index 0eb644f17d93..41614eb09053 100644 --- a/nova/virt/images.py +++ b/nova/virt/images.py @@ -29,7 +29,7 @@ from oslo_utils import units import nova.conf from nova import exception -from nova.i18n import _, _LE +from nova.i18n import _ from nova import image from nova import utils @@ -107,7 +107,7 @@ def _convert_image(source, dest, in_format, out_format, run_as_root): raise exception.ImageUnacceptable(image_id=source, reason=msg) -def fetch(context, image_href, path, max_size=0): +def fetch(context, image_href, path): with fileutils.remove_path_on_error(path): IMAGE_API.download(context, image_href, dest_path=path) @@ -116,9 +116,9 @@ def get_info(context, image_href): return IMAGE_API.get(context, image_href) -def fetch_to_raw(context, image_href, path, max_size=0): +def fetch_to_raw(context, image_href, path): path_tmp = "%s.part" % path - fetch(context, image_href, path_tmp, max_size=max_size) + fetch(context, image_href, path_tmp) with fileutils.remove_path_on_error(path_tmp): data = qemu_img_info(path_tmp) @@ -135,24 +135,6 @@ def fetch_to_raw(context, image_href, path, max_size=0): reason=(_("fmt=%(fmt)s backed by: %(backing_file)s") % {'fmt': fmt, 'backing_file': backing_file})) - # We can't generally shrink incoming images, so disallow - # images > size of the flavor we're booting. Checking here avoids - # an immediate DoS where we convert large qcow images to raw - # (which may compress well but not be sparse). - # TODO(p-draigbrady): loop through all flavor sizes, so that - # we might continue here and not discard the download. - # If we did that we'd have to do the higher level size checks - # irrespective of whether the base image was prepared or not. - disk_size = data.virtual_size - if max_size and max_size < disk_size: - LOG.error(_LE('%(base)s virtual size %(disk_size)s ' - 'larger than flavor root disk size %(size)s'), - {'base': path, - 'disk_size': disk_size, - 'size': max_size}) - raise exception.FlavorDiskSmallerThanImage( - flavor_size=max_size, image_size=disk_size) - if fmt != "raw" and CONF.force_raw_images: staged = "%s.converted" % path LOG.debug("%s was %s, converting to raw", image_href, fmt) diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index 9b7b99ffeb39..38fc2d8264ea 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -2870,7 +2870,7 @@ class LibvirtDriver(driver.ComputeDriver): @staticmethod def _create_ephemeral(target, ephemeral_size, fs_label, os_type, is_block_dev=False, - max_size=None, context=None, specified_fs=None): + context=None, specified_fs=None): if not is_block_dev: libvirt_utils.create_image('raw', target, '%dG' % ephemeral_size) @@ -2879,7 +2879,7 @@ class LibvirtDriver(driver.ComputeDriver): specified_fs=specified_fs) @staticmethod - def _create_swap(target, swap_mb, max_size=None, context=None): + def _create_swap(target, swap_mb, context=None): """Create a swap file of specified size.""" libvirt_utils.create_image('raw', target, '%dM' % swap_mb) utils.mkfs('swap', target) @@ -6593,7 +6593,7 @@ class LibvirtDriver(driver.ComputeDriver): {'image_id': image_id, 'host': fallback_from_host}, instance=instance) - def copy_from_host(target, max_size): + def copy_from_host(target): libvirt_utils.copy_image(src=target, dest=target, host=fallback_from_host, diff --git a/nova/virt/libvirt/imagebackend.py b/nova/virt/libvirt/imagebackend.py index de06e4da9835..a1077456e1ca 100644 --- a/nova/virt/libvirt/imagebackend.py +++ b/nova/virt/libvirt/imagebackend.py @@ -498,7 +498,7 @@ class Flat(Image): prepare_template(target=self.path, *args, **kwargs) else: if not os.path.exists(base): - prepare_template(target=base, max_size=size, *args, **kwargs) + prepare_template(target=base, *args, **kwargs) # NOTE(mikal): Update the mtime of the base file so the image # cache manager knows it is in use. @@ -556,7 +556,7 @@ class Qcow2(Image): # Download the unmodified base image unless we already have a copy. if not os.path.exists(base): - prepare_template(target=base, max_size=size, *args, **kwargs) + prepare_template(target=base, *args, **kwargs) # NOTE(ankit): Update the mtime of the base file so the image # cache manager knows it is in use. @@ -721,7 +721,7 @@ class Lvm(Image): prepare_template(target=self.path, *args, **kwargs) else: if not os.path.exists(base): - prepare_template(target=base, max_size=size, *args, **kwargs) + prepare_template(target=base, *args, **kwargs) with self.remove_volume_on_error(self.path): create_lvm_image(base, size) @@ -842,7 +842,7 @@ class Rbd(Image): def create_image(self, prepare_template, base, size, *args, **kwargs): if not self.exists(): - prepare_template(target=base, max_size=size, *args, **kwargs) + prepare_template(target=base, *args, **kwargs) # prepare_template() may have cloned the image into a new rbd # image already instead of downloading it locally @@ -1047,7 +1047,7 @@ class Ploop(Image): reason=reason) if not os.path.exists(base): - prepare_template(target=base, max_size=size, *args, **kwargs) + prepare_template(target=base, *args, **kwargs) self.verify_base_size(base, size) if os.path.exists(self.path): diff --git a/nova/virt/libvirt/utils.py b/nova/virt/libvirt/utils.py index ff811c221fc4..bb891e9bab21 100644 --- a/nova/virt/libvirt/utils.py +++ b/nova/virt/libvirt/utils.py @@ -415,18 +415,18 @@ def get_fs_info(path): 'used': used} -def fetch_image(context, target, image_id, max_size=0): +def fetch_image(context, target, image_id): """Grab image.""" - images.fetch_to_raw(context, image_id, target, max_size=max_size) + images.fetch_to_raw(context, image_id, target) -def fetch_raw_image(context, target, image_id, max_size=0): +def fetch_raw_image(context, target, image_id): """Grab initrd or kernel image. This function does not attempt raw conversion, as these images will already be in raw format. """ - images.fetch(context, image_id, target, max_size=max_size) + images.fetch(context, image_id, target) def get_instance_path(instance, forceold=False, relative=False):