openstack
/
nova
Code Issues Proposed changes

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
This commit is contained in:
Matthew Booth 2016-06-08 10:13:00 +01:00
parent eec3a2b9e8
commit cc2ae7526b
7 changed files with 39 additions and 75 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
nova/tests/unit/virt/libvirt/test_driver.py
View File

@ -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'

31
nova/tests/unit/virt/libvirt/test_imagebackend.py
View File

@ -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",

21
nova/tests/unit/virt/libvirt/test_utils.py
View File

@ -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):

26
nova/virt/images.py
View File

@ -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)

6
nova/virt/libvirt/driver.py
View File

@ -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,

10
nova/virt/libvirt/imagebackend.py
View File

@ -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):

8
nova/virt/libvirt/utils.py
View File

@ -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):