openstack/nova
Code Issues Proposed changes

only safety check bootable files created from glance

Browse Source 
For blank files that are created by nova such as swap
disks and ephemeral disks we do not need need to safety
check them as they always are just bare filesystems.

In the future we should refactor the qcow imagebackend to
not require backing files for swap and ephemeral disks
but for now we simply disable the check to workaround
the addition of the gpt image inspector and the incompatiblity
with vfat. future versions of oslo will account for vfat boot
recored. this is a minimal patch to avoid needing a new oslo
release for 2024.2

Closes-Bug: #2079850
Change-Id: I7df3d9859aa4be3a012ff919f375a7a3d9992af4
This commit is contained in:
Sean Mooney 2024-09-10 14:41:15 +01:00
parent e310049eab
commit 8de15e9a27
7 changed files with 81 additions and 55 deletions

4
nova/tests/fixtures/libvirt_imagebackend.py vendored

@ -215,7 +215,9 @@ class LibvirtImageBackendFixture(fixtures.Fixture):
return image_init
def _fake_cache(self, fetch_func, filename, size=None, *args, **kwargs):
def _fake_cache(
self, fetch_func, filename, size=None, safe=False, *args,
**kwargs):
# Execute the template function so we can test the arguments it was
# called with.
fetch_func(target=filename, *args, **kwargs)

29
nova/tests/functional/regressions/test_bug_2079850.py

@ -3,6 +3,8 @@ import functools
import os
import shutil
from unittest import mock
import fixtures
from oslo_utils.fixture import uuidsentinel as uuids
@ -11,7 +13,6 @@ from oslo_utils import units
import nova.conf
from nova import exception
from nova import objects
from nova import test
from nova.virt.libvirt import driver
@ -53,17 +54,16 @@ class TestBugBackingFilePartitionTables(test.NoDBTestCase):
file_path = os.path.join(self.base_dir.path, 'test_file')
libvirt_utils.create_image(file_path, 'raw', '64M')
self.assertTrue(os.path.exists(file_path))
# nova should ensure that any file we create has a partition table
# inspector = format_inspector.GPTInspector.from_file(file_path)
# self.assertIsNotNone(inspector)
# inspector.safety_check()
# however the libvirt_utils.create_image method does not create a
# partition table so we should expect this to fail
# FIXME(sean-k-mooney): oslo currently detect vfat as an mbr partition
self.assertRaises(
format_inspector.ImageFormatError,
format_inspector.GPTInspector.from_file, file_path)
# nova files should pass the RawFileInspector safety check
inspector = format_inspector.RawFileInspector.from_file(file_path)
self.assertIsNotNone(inspector)
inspector.safety_check()
def test_cache_file(self):
"""Test the qcow2 cache interaction for ephemeral disks
@ -85,8 +85,11 @@ class TestBugBackingFilePartitionTables(test.NoDBTestCase):
os_type=None, is_block_dev=False)
# this need to be multiples of 1G
size = 1 * units.Gi
fname = "ephemeral_%s_%s" % (size, ".qcow")
e = self.assertRaises(exception.InvalidDiskInfo,
image.cache, fetch_func=fn, context=None, filename=fname,
size=size, ephemeral_size=1)
self.assertIn("Base image failed safety check", str(e))
fname = "ephemeral_%s_%s" % (size, ".img")
with mock.patch.object(
imagebackend, '_update_utime_ignore_eacces') as m:
image.cache(
fetch_func=fn, context=None, filename=fname,
size=size, ephemeral_size=1, safe=True)
m.assert_called_once()

8
nova/tests/unit/virt/libvirt/test_driver.py

@ -14903,6 +14903,7 @@ class LibvirtConnTestCase(test.NoDBTestCase,
'qcow2',
virt_disk_size,
backing_file=backfile_path,
safe=False
)
@mock.patch('nova.virt.libvirt.imagebackend.Image.exists',
@ -15054,12 +15055,14 @@ class LibvirtConnTestCase(test.NoDBTestCase,
'qcow2',
disk_info_byname['disk']['virt_disk_size'],
backing_file=root_backing,
safe=False
),
mock.call(
CONF.instances_path + '/disk.local',
'qcow2',
disk_info_byname['disk.local']['virt_disk_size'],
backing_file=ephemeral_backing,
safe=True
),
])
@ -16626,7 +16629,8 @@ class LibvirtConnTestCase(test.NoDBTestCase,
context=self.context)
backend.disks['disk.swap'].cache.assert_called_once_with(
fetch_func=mock.ANY, filename='swap_%i' % expected,
size=expected * units.Mi, context=self.context, swap_mb=expected)
size=expected * units.Mi, context=self.context, swap_mb=expected,
safe=True)
@mock.patch.object(nova.virt.libvirt.imagebackend.Image, 'cache')
def test_create_vz_container_with_swap(self, mock_cache):
@ -16711,7 +16715,7 @@ class LibvirtConnTestCase(test.NoDBTestCase,
backend.disks['disk.eph0'].cache.assert_called_once_with(
fetch_func=mock.ANY, context=self.context,
filename=filename, size=100 * units.Gi, ephemeral_size=mock.ANY,
specified_fs=None)
specified_fs=None, safe=True)
def test_create_image_resize_snap_backend(self):
drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)

15
nova/tests/unit/virt/libvirt/test_imagebackend.py

@ -514,7 +514,7 @@ class Qcow2TestCase(_ImageTestCase, test.NoDBTestCase):
image.cache(fn, self.TEMPLATE)
fn.assert_called_once_with(target=self.TEMPLATE_PATH)
fn.assert_called_once_with(target=self.TEMPLATE_PATH, safe=False)
mock_exists.assert_has_calls(exist_calls)
@mock.patch.object(os.path, 'exists')
@ -545,7 +545,7 @@ class Qcow2TestCase(_ImageTestCase, test.NoDBTestCase):
image.cache(fn, self.TEMPLATE)
fn.assert_called_once_with(target=self.TEMPLATE_PATH)
fn.assert_called_once_with(target=self.TEMPLATE_PATH, safe=False)
mock_exists.assert_has_calls(exist_calls)
@mock.patch.object(os.path, 'exists')
@ -587,7 +587,8 @@ class Qcow2TestCase(_ImageTestCase, test.NoDBTestCase):
mock_verify.assert_called_once_with(self.TEMPLATE_PATH, self.SIZE)
mock_create.assert_called_once_with(
self.PATH, 'qcow2', self.SIZE, backing_file=self.TEMPLATE_PATH)
self.PATH, 'qcow2', self.SIZE, backing_file=self.TEMPLATE_PATH,
safe=False)
fn.assert_called_once_with(target=self.TEMPLATE_PATH)
mock_exist.assert_has_calls(exist_calls)
self.assertTrue(mock_sync.called)
@ -828,7 +829,7 @@ class LvmTestCase(_ImageTestCase, test.NoDBTestCase):
image.cache(fn, self.TEMPLATE)
fn.assert_called_once_with(target=self.TEMPLATE_PATH)
fn.assert_called_once_with(target=self.TEMPLATE_PATH, safe=False)
mock_ensure.assert_called_once_with(self.TEMPLATE_DIR)
mock_exists.assert_has_calls(exist_calls)
@ -1369,7 +1370,7 @@ class RbdTestCase(_ImageTestCase, test.NoDBTestCase):
image.cache(fn, self.TEMPLATE)
mock_ensure.assert_called_once_with(self.TEMPLATE_DIR)
fn.assert_called_once_with(target=self.TEMPLATE_PATH)
fn.assert_called_once_with(target=self.TEMPLATE_PATH, safe=False)
mock_img_exist.assert_called_with()
mock_os_exist.assert_has_calls([
mock.call(self.TEMPLATE_DIR), mock.call(self.TEMPLATE_PATH)
@ -1392,7 +1393,7 @@ class RbdTestCase(_ImageTestCase, test.NoDBTestCase):
mock_os_exist.assert_has_calls([
mock.call(self.TEMPLATE_DIR), mock.call(self.TEMPLATE_PATH)
])
fn.assert_called_once_with(target=self.TEMPLATE_PATH)
fn.assert_called_once_with(target=self.TEMPLATE_PATH, safe=False)
@mock.patch.object(os.path, 'exists', return_value=True)
@mock.patch.object(imagebackend.Rbd, 'exists', return_value=True)
@ -2078,7 +2079,7 @@ class PloopTestCase(_ImageTestCase, test.NoDBTestCase):
mock_ensure.assert_called_once_with(self.TEMPLATE_DIR)
mock_exists.assert_has_calls(exist_calls)
fn.assert_called_once_with(target=self.TEMPLATE_PATH)
fn.assert_called_once_with(target=self.TEMPLATE_PATH, safe=False)
@mock.patch.object(imagebackend.Ploop, 'get_disk_size',
return_value=2048)

34
nova/virt/libvirt/driver.py

@ -4926,7 +4926,8 @@ class LibvirtDriver(driver.ComputeDriver):
'%dG' % ephemeral_size,
specified_fs)
return
libvirt_utils.create_image(target, 'raw', f'{ephemeral_size}G')
libvirt_utils.create_image(
target, 'raw', f'{ephemeral_size}G', safe=True)
# Run as root only for block devices.
disk_api.mkfs(os_type, fs_label, target, run_as_root=is_block_dev,
@ -5169,11 +5170,9 @@ class LibvirtDriver(driver.ComputeDriver):
vm_mode=vm_mode)
fname = "ephemeral_%s_%s" % (ephemeral_gb, file_extension)
size = ephemeral_gb * units.Gi
disk_image.cache(fetch_func=fn,
context=context,
filename=fname,
size=size,
ephemeral_size=ephemeral_gb)
disk_image.cache(
fetch_func=fn, context=context, filename=fname, size=size,
ephemeral_size=ephemeral_gb, safe=True)
for idx, eph in enumerate(driver.block_device_info_get_ephemerals(
block_device_info)):
@ -5195,12 +5194,10 @@ class LibvirtDriver(driver.ComputeDriver):
vm_mode=vm_mode)
size = eph['size'] * units.Gi
fname = "ephemeral_%s_%s" % (eph['size'], file_extension)
disk_image.cache(fetch_func=fn,
context=context,
filename=fname,
size=size,
ephemeral_size=eph['size'],
specified_fs=specified_fs)
disk_image.cache(
fetch_func=fn, context=context, filename=fname, size=size,
ephemeral_size=eph['size'], specified_fs=specified_fs,
safe=True)
if swap_mb > 0:
size = swap_mb * units.Mi
@ -5208,9 +5205,10 @@ class LibvirtDriver(driver.ComputeDriver):
swap = image('disk.swap', disk_info_mapping=disk_info_mapping)
# Short circuit the exists() tests if we already created a disk
created_disks = created_disks or not swap.exists()
swap.cache(fetch_func=self._create_swap, context=context,
filename="swap_%s" % swap_mb,
size=size, swap_mb=swap_mb)
swap.cache(
fetch_func=self._create_swap, context=context,
filename="swap_%s" % swap_mb, size=size, swap_mb=swap_mb,
safe=True)
if created_disks:
LOG.debug('Created local disks', instance=instance)
@ -11650,14 +11648,16 @@ class LibvirtDriver(driver.ComputeDriver):
os_type=instance.os_type,
filename=cache_name,
size=info['virt_disk_size'],
ephemeral_size=info['virt_disk_size'] / units.Gi)
ephemeral_size=info['virt_disk_size'] / units.Gi,
safe=True)
elif cache_name.startswith('swap'):
flavor = instance.get_flavor()
swap_mb = flavor.swap
disk.cache(fetch_func=self._create_swap,
filename="swap_%s" % swap_mb,
size=swap_mb * units.Mi,
swap_mb=swap_mb)
swap_mb=swap_mb,
safe=True)
else:
self._try_fetch_image_cache(disk,
libvirt_utils.fetch_image,

37
nova/virt/libvirt/imagebackend.py

@ -135,7 +135,8 @@ class Image(metaclass=abc.ABCMeta):
return False
@abc.abstractmethod
def create_image(self, prepare_template, base, size, *args, **kwargs):
def create_image(
self, prepare_template, base, size, safe=False, *args, **kwargs):
"""Create image from template.
Contains specific behavior for each image type.
@ -144,6 +145,7 @@ class Image(metaclass=abc.ABCMeta):
Should accept `target` argument.
:base: Template name
:size: Size of created image in bytes
:safe: True if image contains a safe filesystem
"""
pass
@ -263,7 +265,8 @@ class Image(metaclass=abc.ABCMeta):
def exists(self):
return os.path.exists(self.path)
def cache(self, fetch_func, filename, size=None, *args, **kwargs):
def cache(self, fetch_func, filename, size=None, safe=False, *args,
**kwargs):
"""Creates image from template.
Ensures that template and image not already exists.
@ -298,8 +301,9 @@ class Image(metaclass=abc.ABCMeta):
fetch_func(target=target, *args, **kwargs)
if not self.exists() or not os.path.exists(base):
self.create_image(fetch_func_sync, base, size,
*args, **kwargs)
self.create_image(
fetch_func_sync, base, size, safe=safe, *args,
**kwargs)
if size:
# create_image() only creates the base image if needed, so
@ -593,7 +597,8 @@ class Flat(Image):
if os.path.exists(self.path):
self.driver_format = self.resolve_driver_format()
def create_image(self, prepare_template, base, size, *args, **kwargs):
def create_image(
self, prepare_template, base, size, safe=False, *args, **kwargs):
filename = self._get_lock_name(base)
@utils.synchronized(filename, external=True, lock_path=self.lock_path)
@ -663,13 +668,14 @@ class Qcow2(Image):
self.disk_info_path = os.path.join(os.path.dirname(path), 'disk.info')
self.resolve_driver_format()
def create_image(self, prepare_template, base, size, *args, **kwargs):
def create_image(
self, prepare_template, base, size, safe=False, *args, **kwargs):
filename = self._get_lock_name(base)
@utils.synchronized(filename, external=True, lock_path=self.lock_path)
def create_qcow2_image(base, target, size):
def create_qcow2_image(base, target, size, safe=False):
libvirt_utils.create_image(
target, 'qcow2', size, backing_file=base)
target, 'qcow2', size, backing_file=base, safe=safe)
# Download the unmodified base image unless we already have a copy.
if not os.path.exists(base):
@ -679,7 +685,9 @@ class Qcow2(Image):
# before we inspect it for other attributes. We do this each time
# because additional safety checks could have been added since we
# downloaded the image.
if not CONF.workarounds.disable_deep_image_inspection:
# NOTE(sean-k-mooney) If the image was created by nova as a swap
# or ephemeral disk it is safe to skip the deep inspection.
if not CONF.workarounds.disable_deep_image_inspection and not safe:
inspector = format_inspector.detect_file_format(base)
try:
inspector.safety_check()
@ -724,7 +732,7 @@ class Qcow2(Image):
if not os.path.exists(self.path):
with fileutils.remove_path_on_error(self.path):
create_qcow2_image(base, self.path, size)
create_qcow2_image(base, self.path, size, safe=safe)
def resize_image(self, size):
image = imgmodel.LocalFileImage(self.path, imgmodel.FORMAT_QCOW2)
@ -800,7 +808,8 @@ class Lvm(Image):
def _can_fallocate(self):
return False
def create_image(self, prepare_template, base, size, *args, **kwargs):
def create_image(
self, prepare_template, base, size, safe=False, *args, **kwargs):
def encrypt_lvm_image():
dmcrypt.create_volume(self.path.rpartition('/')[2],
self.lv_path,
@ -997,7 +1006,8 @@ class Rbd(Image):
LOG.warning("Ignoring failure to remove %(path)s: "
"%(error)s", {'path': base, 'error': e})
def create_image(self, prepare_template, base, size, *args, **kwargs):
def create_image(
self, prepare_template, base, size, safe=False, *args, **kwargs):
if not self.exists():
self._remove_non_raw_cache_image(base)
@ -1272,7 +1282,8 @@ class Ploop(Image):
# Create new ploop disk (in case of epehemeral) or
# copy ploop disk from glance image
def create_image(self, prepare_template, base, size, *args, **kwargs):
def create_image(
self, prepare_template, base, size, safe=False, *args, **kwargs):
filename = os.path.basename(base)
# Copy main file of ploop disk, restore DiskDescriptor.xml for it

9
nova/virt/libvirt/utils.py

@ -133,7 +133,8 @@ def create_image(
disk_format: str,
disk_size: ty.Optional[ty.Union[str, int]],
backing_file: ty.Optional[str] = None,
encryption: ty.Optional[EncryptionOptions] = None
encryption: ty.Optional[EncryptionOptions] = None,
safe: bool = False,
) -> None:
"""Disk image creation with qemu-img
:param path: Desired location of the disk image
@ -147,6 +148,7 @@ def create_image(
:param backing_file: (Optional) Backing file to use.
:param encryption: (Optional) Dict detailing various encryption attributes
such as the format and passphrase.
:param safe: If True, the image is know to be safe.
"""
cmd = [
'env', 'LC_ALL=C', 'LANG=C', 'qemu-img', 'create', '-f', disk_format
@ -157,7 +159,10 @@ def create_image(
# before we inspect it for other attributes. We do this each time
# because additional safety checks could have been added since we
# downloaded the image.
if not CONF.workarounds.disable_deep_image_inspection:
# Note(sean-k-mooney): We only need to perform the safety check for
# the backing file if the image is not created by nova for swap or
# ephemeral disks.
if not CONF.workarounds.disable_deep_image_inspection and not safe:
inspector = format_inspector.detect_file_format(backing_file)
try:
inspector.safety_check()