From 8de15e9a276dc4261dd0656e26ca5a917825f441 Mon Sep 17 00:00:00 2001
From: Sean Mooney <work@seanmooney.info>
Date: Tue, 10 Sep 2024 14:41:15 +0100
Subject: [PATCH] only safety check bootable files created from glance
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
---
nova/tests/fixtures/libvirt_imagebackend.py | 4 +-
.../regressions/test_bug_2079850.py | 29 ++++++++-------
nova/tests/unit/virt/libvirt/test_driver.py | 8 +++-
.../unit/virt/libvirt/test_imagebackend.py | 15 ++++----
nova/virt/libvirt/driver.py | 34 ++++++++---------
nova/virt/libvirt/imagebackend.py | 37 ++++++++++++-------
nova/virt/libvirt/utils.py | 9 ++++-
7 files changed, 81 insertions(+), 55 deletions(-)
diff --git a/nova/tests/fixtures/libvirt_imagebackend.py b/nova/tests/fixtures/libvirt_imagebackend.py
index a414fe62aa58..a17d83c12c51 100644
--- a/nova/tests/fixtures/libvirt_imagebackend.py
+++ b/nova/tests/fixtures/libvirt_imagebackend.py
@@ -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)
diff --git a/nova/tests/functional/regressions/test_bug_2079850.py b/nova/tests/functional/regressions/test_bug_2079850.py
index 4e9f1fcf65f7..515a51fbe3e0 100644
--- a/nova/tests/functional/regressions/test_bug_2079850.py
+++ b/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()
diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py
index edea0177ac17..f83416e24011 100644
--- a/nova/tests/unit/virt/libvirt/test_driver.py
+++ b/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)
diff --git a/nova/tests/unit/virt/libvirt/test_imagebackend.py b/nova/tests/unit/virt/libvirt/test_imagebackend.py
index 329b4701adaf..9d35f66fa47d 100644
--- a/nova/tests/unit/virt/libvirt/test_imagebackend.py
+++ b/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)
diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py
index 16834846e2d4..3dcb9222b27f 100644
--- a/nova/virt/libvirt/driver.py
+++ b/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,
diff --git a/nova/virt/libvirt/imagebackend.py b/nova/virt/libvirt/imagebackend.py
index 65077e63a198..a86322cf97d4 100644
--- a/nova/virt/libvirt/imagebackend.py
+++ b/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
diff --git a/nova/virt/libvirt/utils.py b/nova/virt/libvirt/utils.py
index bc3ea9e76f5a..202d7187a4a0 100644
--- a/nova/virt/libvirt/utils.py
+++ b/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()