From c849b65e354df0adf3bd1d1dac7e935a7196f020 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Thu, 15 Dec 2022 00:09:04 +0000 Subject: [PATCH 1/8] Remove use of removeprefix This is not supported on Python 3.8 [1]. I have no idea why this was not failing CI. [1] https://docs.python.org/3.9/library/stdtypes.html#str.removeprefix Change-Id: I225e9ced0f75c415b1d2fee05440291e3d8635c0 Signed-off-by: Stephen Finucane --- nova/tests/unit/console/test_websocketproxy.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/nova/tests/unit/console/test_websocketproxy.py b/nova/tests/unit/console/test_websocketproxy.py index 46a51dbf7c3..b7d2cd3e843 100644 --- a/nova/tests/unit/console/test_websocketproxy.py +++ b/nova/tests/unit/console/test_websocketproxy.py @@ -675,7 +675,9 @@ def test_reject_open_redirect(self, url='//example.com/%2F..'): # now the same url but with extra leading '/' characters removed. if expected_cpython in errmsg: location = result[3].decode() - location = location.removeprefix('Location: ').rstrip('\r\n') + if location.startswith('Location: '): + location = location[len('Location: '):] + location = location.rstrip('\r\n') self.assertTrue( location.startswith('/example.com/%2F..'), msg='Redirect location is not the expected sanitized URL', From 2651741131dc37ca42467b28ffb54596435ad50d Mon Sep 17 00:00:00 2001 From: Lee Yarwood Date: Wed, 5 Feb 2020 12:36:40 +0000 Subject: [PATCH 2/8] images: Move qemu-img info calls into privsep This is mostly code motion from the nova.virt.images module into privsep to allow for both privileged and unprivileged calls to be made. A privileged_qemu_img_info function is introduced allowing QEMU to access devices requiring root privileges, such as host block devices. Change-Id: I5ac03f923d9d181d22d44d8ec8fbc31eb0c3999e --- nova/privsep/qemu.py | 63 ++++++++++++++++++++- nova/tests/unit/virt/libvirt/test_utils.py | 66 +++++++++++----------- nova/virt/images.py | 54 +++--------------- 3 files changed, 102 insertions(+), 81 deletions(-) diff --git a/nova/privsep/qemu.py b/nova/privsep/qemu.py index 5880fb1775e..07db81a3810 100644 --- a/nova/privsep/qemu.py +++ b/nova/privsep/qemu.py @@ -16,14 +16,25 @@ Helpers for qemu tasks. """ +import operator +import os + from oslo_concurrency import processutils from oslo_log import log as logging +from oslo_utils import units +from nova import exception +from nova.i18n import _ import nova.privsep.utils - LOG = logging.getLogger(__name__) +QEMU_IMG_LIMITS = processutils.ProcessLimits( + cpu_time=30, + address_space=1 * units.Gi) + +QEMU_VERSION_REQ_SHARED = 2010000 + @nova.privsep.sys_admin_pctxt.entrypoint def convert_image(source, dest, in_format, out_format, instances_path, @@ -71,3 +82,53 @@ def unprivileged_convert_image(source, dest, in_format, out_format, cmd = cmd + (source, dest) processutils.execute(*cmd) + + +@nova.privsep.sys_admin_pctxt.entrypoint +def privileged_qemu_img_info(path, format=None, qemu_version=None): + """Return an oject containing the parsed output from qemu-img info + + This is a privileged call to qemu-img info using the sys_admin_pctxt + entrypoint allowing host block devices etc to be accessed. + """ + return unprivileged_qemu_img_info( + path, format=format, qemu_version=qemu_version) + + +def unprivileged_qemu_img_info(path, format=None, qemu_version=None): + """Return an object containing the parsed output from qemu-img info.""" + try: + # The following check is about ploop images that reside within + # directories and always have DiskDescriptor.xml file beside them + if (os.path.isdir(path) and + os.path.exists(os.path.join(path, "DiskDescriptor.xml"))): + path = os.path.join(path, "root.hds") + + cmd = ('env', 'LC_ALL=C', 'LANG=C', 'qemu-img', 'info', path) + if format is not None: + cmd = cmd + ('-f', format) + # Check to see if the qemu version is >= 2.10 because if so, we need + # to add the --force-share flag. + if qemu_version and operator.ge(qemu_version, QEMU_VERSION_REQ_SHARED): + cmd = cmd + ('--force-share',) + out, err = processutils.execute(*cmd, prlimit=QEMU_IMG_LIMITS) + except processutils.ProcessExecutionError as exp: + if exp.exit_code == -9: + # this means we hit prlimits, make the exception more specific + msg = (_("qemu-img aborted by prlimits when inspecting " + "%(path)s : %(exp)s") % {'path': path, 'exp': exp}) + elif exp.exit_code == 1 and 'No such file or directory' in exp.stderr: + # The os.path.exists check above can race so this is a simple + # best effort at catching that type of failure and raising a more + # specific error. + raise exception.DiskNotFound(location=path) + else: + msg = (_("qemu-img failed to execute on %(path)s : %(exp)s") % + {'path': path, 'exp': exp}) + raise exception.InvalidDiskInfo(reason=msg) + + if not out: + msg = (_("Failed to run qemu-img info on %(path)s : %(error)s") % + {'path': path, 'error': err}) + raise exception.InvalidDiskInfo(reason=msg) + return out diff --git a/nova/tests/unit/virt/libvirt/test_utils.py b/nova/tests/unit/virt/libvirt/test_utils.py index 2888078cdef..1c3b3df0acf 100644 --- a/nova/tests/unit/virt/libvirt/test_utils.py +++ b/nova/tests/unit/virt/libvirt/test_utils.py @@ -32,6 +32,7 @@ from nova import objects from nova.objects import fields as obj_fields import nova.privsep.fs +import nova.privsep.qemu from nova import test from nova.tests import fixtures as nova_fixtures from nova.tests.unit import fake_instance @@ -107,18 +108,18 @@ def test_disk_backing(self, mock_execute, mock_exists): }) mock_execute.return_value = (output, '') d_backing = libvirt_utils.get_disk_backing_file(path) - mock_execute.assert_called_once_with('env', 'LC_ALL=C', 'LANG=C', - 'qemu-img', 'info', path, - prlimit=images.QEMU_IMG_LIMITS) + mock_execute.assert_called_once_with( + 'env', 'LC_ALL=C', 'LANG=C', 'qemu-img', 'info', path, + prlimit=nova.privsep.qemu.QEMU_IMG_LIMITS) mock_exists.assert_called_once_with(path) self.assertIsNone(d_backing) def _test_disk_size(self, mock_execute, path, expected_size): d_size = libvirt_utils.get_disk_size(path) self.assertEqual(expected_size, d_size) - mock_execute.assert_called_once_with('env', 'LC_ALL=C', 'LANG=C', - 'qemu-img', 'info', path, - prlimit=images.QEMU_IMG_LIMITS) + mock_execute.assert_called_once_with( + 'env', 'LC_ALL=C', 'LANG=C', 'qemu-img', 'info', path, + prlimit=nova.privsep.qemu.QEMU_IMG_LIMITS) @mock.patch('os.path.exists', return_value=True) def test_disk_size(self, mock_exists): @@ -163,9 +164,9 @@ def test_qemu_info_canon(self, mock_execute, mock_exists): """ mock_execute.return_value = (example_output, '') image_info = images.qemu_img_info(path) - mock_execute.assert_called_once_with('env', 'LC_ALL=C', 'LANG=C', - 'qemu-img', 'info', path, - prlimit=images.QEMU_IMG_LIMITS) + mock_execute.assert_called_once_with( + 'env', 'LC_ALL=C', 'LANG=C', 'qemu-img', 'info', path, + prlimit=nova.privsep.qemu.QEMU_IMG_LIMITS) mock_exists.assert_called_once_with(path) self.assertEqual('disk.config', image_info.image) self.assertEqual('raw', image_info.file_format) @@ -176,7 +177,7 @@ def test_qemu_info_canon(self, mock_execute, mock_exists): @mock.patch('os.path.exists', return_value=True) @mock.patch('oslo_concurrency.processutils.execute') def test_qemu_info_canon_qemu_2_10(self, mock_execute, mock_exists): - images.QEMU_VERSION = images.QEMU_VERSION_REQ_SHARED + images.QEMU_VERSION = nova.privsep.qemu.QEMU_VERSION_REQ_SHARED path = "disk.config" example_output = """image: disk.config file format: raw @@ -187,10 +188,9 @@ def test_qemu_info_canon_qemu_2_10(self, mock_execute, mock_exists): """ mock_execute.return_value = (example_output, '') image_info = images.qemu_img_info(path) - mock_execute.assert_called_once_with('env', 'LC_ALL=C', 'LANG=C', - 'qemu-img', 'info', path, - '--force-share', - prlimit=images.QEMU_IMG_LIMITS) + mock_execute.assert_called_once_with( + 'env', 'LC_ALL=C', 'LANG=C', 'qemu-img', 'info', path, + '--force-share', prlimit=nova.privsep.qemu.QEMU_IMG_LIMITS) mock_exists.assert_called_once_with(path) self.assertEqual('disk.config', image_info.image) self.assertEqual('raw', image_info.file_format) @@ -211,9 +211,9 @@ def test_qemu_info_canon2(self, mock_execute, mock_exists): """ mock_execute.return_value = (example_output, '') image_info = images.qemu_img_info(path) - mock_execute.assert_called_once_with('env', 'LC_ALL=C', 'LANG=C', - 'qemu-img', 'info', path, - prlimit=images.QEMU_IMG_LIMITS) + mock_execute.assert_called_once_with( + 'env', 'LC_ALL=C', 'LANG=C', 'qemu-img', 'info', path, + prlimit=nova.privsep.qemu.QEMU_IMG_LIMITS) mock_exists.assert_called_once_with(path) self.assertEqual('disk.config', image_info.image) self.assertEqual('qcow2', image_info.file_format) @@ -235,10 +235,10 @@ def test_qemu_info_ploop(self, mock_execute, mock_isdir, mock_exists): """ mock_execute.return_value = (example_output, '') image_info = images.qemu_img_info(path) - mock_execute.assert_called_once_with('env', 'LC_ALL=C', 'LANG=C', - 'qemu-img', 'info', - os.path.join(path, 'root.hds'), - prlimit=images.QEMU_IMG_LIMITS) + mock_execute.assert_called_once_with( + 'env', 'LC_ALL=C', 'LANG=C', 'qemu-img', 'info', + os.path.join(path, 'root.hds'), + prlimit=nova.privsep.qemu.QEMU_IMG_LIMITS) mock_isdir.assert_called_once_with(path) self.assertEqual(2, mock_exists.call_count) self.assertEqual(path, mock_exists.call_args_list[0][0][0]) @@ -266,9 +266,9 @@ def test_qemu_backing_file_actual(self, """ mock_execute.return_value = (example_output, '') image_info = images.qemu_img_info(path) - mock_execute.assert_called_once_with('env', 'LC_ALL=C', 'LANG=C', - 'qemu-img', 'info', path, - prlimit=images.QEMU_IMG_LIMITS) + mock_execute.assert_called_once_with( + 'env', 'LC_ALL=C', 'LANG=C', 'qemu-img', 'info', path, + prlimit=nova.privsep.qemu.QEMU_IMG_LIMITS) mock_exists.assert_called_once_with(path) self.assertEqual('disk.config', image_info.image) self.assertEqual('raw', image_info.file_format) @@ -295,9 +295,9 @@ def test_qemu_info_convert(self, mock_execute, mock_exists): """ mock_execute.return_value = (example_output, '') image_info = images.qemu_img_info(path) - mock_execute.assert_called_once_with('env', 'LC_ALL=C', 'LANG=C', - 'qemu-img', 'info', path, - prlimit=images.QEMU_IMG_LIMITS) + mock_execute.assert_called_once_with( + 'env', 'LC_ALL=C', 'LANG=C', 'qemu-img', 'info', path, + prlimit=nova.privsep.qemu.QEMU_IMG_LIMITS) mock_exists.assert_called_once_with(path) self.assertEqual('disk.config', image_info.image) self.assertEqual('raw', image_info.file_format) @@ -320,9 +320,9 @@ def test_qemu_info_snaps(self, mock_execute, mock_exists): """ mock_execute.return_value = (example_output, '') image_info = images.qemu_img_info(path) - mock_execute.assert_called_once_with('env', 'LC_ALL=C', 'LANG=C', - 'qemu-img', 'info', path, - prlimit=images.QEMU_IMG_LIMITS) + mock_execute.assert_called_once_with( + 'env', 'LC_ALL=C', 'LANG=C', 'qemu-img', 'info', path, + prlimit=nova.privsep.qemu.QEMU_IMG_LIMITS) mock_exists.assert_called_once_with(path) self.assertEqual('disk.config', image_info.image) self.assertEqual('raw', image_info.file_format) @@ -477,9 +477,9 @@ def test_get_disk_size(self, mock_execute, mock_exists): """ mock_execute.return_value = (example_output, '') self.assertEqual(4592640, disk.get_disk_size('/some/path')) - mock_execute.assert_called_once_with('env', 'LC_ALL=C', 'LANG=C', - 'qemu-img', 'info', path, - prlimit=images.QEMU_IMG_LIMITS) + mock_execute.assert_called_once_with( + 'env', 'LC_ALL=C', 'LANG=C', 'qemu-img', 'info', path, + prlimit=nova.privsep.qemu.QEMU_IMG_LIMITS) mock_exists.assert_called_once_with(path) def test_copy_image(self): diff --git a/nova/virt/images.py b/nova/virt/images.py index d01214beee0..7afb6641247 100644 --- a/nova/virt/images.py +++ b/nova/virt/images.py @@ -19,14 +19,12 @@ Handling of VM disk images. """ -import operator import os from oslo_concurrency import processutils from oslo_log import log as logging from oslo_utils import fileutils from oslo_utils import imageutils -from oslo_utils import units from nova.compute import utils as compute_utils import nova.conf @@ -35,20 +33,15 @@ from nova import image import nova.privsep.qemu +# This is set by the libvirt driver on startup. The version is used to +# determine what flags need to be set on the command line. +QEMU_VERSION = None + LOG = logging.getLogger(__name__) CONF = nova.conf.CONF IMAGE_API = image.API() -QEMU_IMG_LIMITS = processutils.ProcessLimits( - cpu_time=30, - address_space=1 * units.Gi) - -# This is set by the libvirt driver on startup. The version is used to -# determine what flags need to be set on the command line. -QEMU_VERSION = None -QEMU_VERSION_REQ_SHARED = 2010000 - def qemu_img_info(path, format=None): """Return an object containing the parsed output from qemu-img info.""" @@ -57,42 +50,9 @@ def qemu_img_info(path, format=None): if not os.path.exists(path) and CONF.libvirt.images_type != 'rbd': raise exception.DiskNotFound(location=path) - try: - # The following check is about ploop images that reside within - # directories and always have DiskDescriptor.xml file beside them - if (os.path.isdir(path) and - os.path.exists(os.path.join(path, "DiskDescriptor.xml"))): - path = os.path.join(path, "root.hds") - - cmd = ('env', 'LC_ALL=C', 'LANG=C', 'qemu-img', 'info', path) - if format is not None: - cmd = cmd + ('-f', format) - # Check to see if the qemu version is >= 2.10 because if so, we need - # to add the --force-share flag. - if QEMU_VERSION and operator.ge(QEMU_VERSION, QEMU_VERSION_REQ_SHARED): - cmd = cmd + ('--force-share',) - out, err = processutils.execute(*cmd, prlimit=QEMU_IMG_LIMITS) - except processutils.ProcessExecutionError as exp: - if exp.exit_code == -9: - # this means we hit prlimits, make the exception more specific - msg = (_("qemu-img aborted by prlimits when inspecting " - "%(path)s : %(exp)s") % {'path': path, 'exp': exp}) - elif exp.exit_code == 1 and 'No such file or directory' in exp.stderr: - # The os.path.exists check above can race so this is a simple - # best effort at catching that type of failure and raising a more - # specific error. - raise exception.DiskNotFound(location=path) - else: - msg = (_("qemu-img failed to execute on %(path)s : %(exp)s") % - {'path': path, 'exp': exp}) - raise exception.InvalidDiskInfo(reason=msg) - - if not out: - msg = (_("Failed to run qemu-img info on %(path)s : %(error)s") % - {'path': path, 'error': err}) - raise exception.InvalidDiskInfo(reason=msg) - - return imageutils.QemuImgInfo(out) + info = nova.privsep.qemu.unprivileged_qemu_img_info( + path, format=format, qemu_version=QEMU_VERSION) + return imageutils.QemuImgInfo(info) def convert_image(source, dest, in_format, out_format, run_as_root=False, From a6865f2f3884c9230260ff20c78c0c9f2350d843 Mon Sep 17 00:00:00 2001 From: Lee Yarwood Date: Mon, 10 Feb 2020 14:09:10 +0000 Subject: [PATCH 3/8] images: Allow the output format of qemu-img info to be controlled This will allow for the use of the JSON output format that is easier to parse within QemuImgInfo and should allow additional information to be extracted from qemu-img calls in the future. Change-Id: I0b6d1a98726ffa1ebc78fb3c4563a2e4b40ddeff --- nova/privsep/qemu.py | 11 +++++++--- nova/tests/unit/virt/libvirt/test_utils.py | 24 ++++++++++++++++++++++ nova/virt/images.py | 10 ++++++--- 3 files changed, 39 insertions(+), 6 deletions(-) diff --git a/nova/privsep/qemu.py b/nova/privsep/qemu.py index 07db81a3810..55bdfe2a774 100644 --- a/nova/privsep/qemu.py +++ b/nova/privsep/qemu.py @@ -85,17 +85,20 @@ def unprivileged_convert_image(source, dest, in_format, out_format, @nova.privsep.sys_admin_pctxt.entrypoint -def privileged_qemu_img_info(path, format=None, qemu_version=None): +def privileged_qemu_img_info(path, format=None, qemu_version=None, + output_format=None): """Return an oject containing the parsed output from qemu-img info This is a privileged call to qemu-img info using the sys_admin_pctxt entrypoint allowing host block devices etc to be accessed. """ return unprivileged_qemu_img_info( - path, format=format, qemu_version=qemu_version) + path, format=format, qemu_version=qemu_version, + output_format=output_format) -def unprivileged_qemu_img_info(path, format=None, qemu_version=None): +def unprivileged_qemu_img_info(path, format=None, qemu_version=None, + output_format=None): """Return an object containing the parsed output from qemu-img info.""" try: # The following check is about ploop images that reside within @@ -107,6 +110,8 @@ def unprivileged_qemu_img_info(path, format=None, qemu_version=None): cmd = ('env', 'LC_ALL=C', 'LANG=C', 'qemu-img', 'info', path) if format is not None: cmd = cmd + ('-f', format) + if output_format is not None: + cmd = cmd + ("--output=%s" % (output_format),) # Check to see if the qemu version is >= 2.10 because if so, we need # to add the --force-share flag. if qemu_version and operator.ge(qemu_version, QEMU_VERSION_REQ_SHARED): diff --git a/nova/tests/unit/virt/libvirt/test_utils.py b/nova/tests/unit/virt/libvirt/test_utils.py index 1c3b3df0acf..24a7f375f69 100644 --- a/nova/tests/unit/virt/libvirt/test_utils.py +++ b/nova/tests/unit/virt/libvirt/test_utils.py @@ -151,6 +151,30 @@ def test_disk_size(self, mock_exists): return_value=(output, '')) as mock_execute: self._test_disk_size(mock_execute, path, i) + @mock.patch('os.path.exists', return_value=True) + @mock.patch('oslo_concurrency.processutils.execute') + def test_qemu_img_info_json(self, mock_execute, mock_exists): + path = "disk.config" + example_output = """{ + "virtual-size": 67108864, + "filename": "disk.config", + "cluster-size": 65536, + "format": "raw", + "actual-size": 98304 +} +""" + mock_execute.return_value = (example_output, '') + image_info = images.qemu_img_info(path, output_format='json') + mock_execute.assert_called_once_with( + 'env', 'LC_ALL=C', 'LANG=C', 'qemu-img', 'info', path, + '--output=json', prlimit=nova.privsep.qemu.QEMU_IMG_LIMITS) + mock_exists.assert_called_once_with(path) + self.assertEqual('disk.config', image_info.image) + self.assertEqual('raw', image_info.file_format) + self.assertEqual(67108864, image_info.virtual_size) + self.assertEqual(98304, image_info.disk_size) + self.assertEqual(65536, image_info.cluster_size) + @mock.patch('os.path.exists', return_value=True) @mock.patch('oslo_concurrency.processutils.execute') def test_qemu_info_canon(self, mock_execute, mock_exists): diff --git a/nova/virt/images.py b/nova/virt/images.py index 7afb6641247..7e193bbc539 100644 --- a/nova/virt/images.py +++ b/nova/virt/images.py @@ -43,7 +43,7 @@ IMAGE_API = image.API() -def qemu_img_info(path, format=None): +def qemu_img_info(path, format=None, output_format=None): """Return an object containing the parsed output from qemu-img info.""" # TODO(mikal): this code should not be referring to a libvirt specific # flag. @@ -51,8 +51,12 @@ def qemu_img_info(path, format=None): raise exception.DiskNotFound(location=path) info = nova.privsep.qemu.unprivileged_qemu_img_info( - path, format=format, qemu_version=QEMU_VERSION) - return imageutils.QemuImgInfo(info) + path, format=format, qemu_version=QEMU_VERSION, + output_format=output_format) + if output_format: + return imageutils.QemuImgInfo(info, format=output_format) + else: + return imageutils.QemuImgInfo(info) def convert_image(source, dest, in_format, out_format, run_as_root=False, From 9c1b3343413c6a775564c7bbdf2a15a1cca60c73 Mon Sep 17 00:00:00 2001 From: Lee Yarwood Date: Mon, 2 Mar 2020 14:13:20 +0000 Subject: [PATCH 4/8] libvirt: Use oslo.utils >= 4.1.0 to fetch format-specific image data This change is a follow up to I0c3f14100a18107f7e416293f3d4fcc641ce5e55 and removes the direct call to nova.privsep.qemu with one to the images API that now returns an oslo_utils.imageutils.QemuImgInfo object. Version 4.1.0 of oslo.utils introducing support for the format-specific data returned by qemu-img info for LUKSv1 based images. Change-Id: I573396116e10cf87f80f1ded55f2cd8f498859e4 Conflicts, mostly missing extend encrpyted volumes: lower-constraints.txt nova/tests/unit/virt/libvirt/test_driver.py nova/virt/libvirt/driver.py Change-Id: I8754875e54822a8d132e16c44ccf9e98514b4e3b --- nova/virt/images.py | 16 ++++++++++++++++ requirements.txt | 2 +- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/nova/virt/images.py b/nova/virt/images.py index 7e193bbc539..ded5f9de5b4 100644 --- a/nova/virt/images.py +++ b/nova/virt/images.py @@ -59,6 +59,22 @@ def qemu_img_info(path, format=None, output_format=None): return imageutils.QemuImgInfo(info) +def privileged_qemu_img_info(path, format=None, output_format=None): + """Return an object containing the parsed output from qemu-img info.""" + # TODO(mikal): this code should not be referring to a libvirt specific + # flag. + if not os.path.exists(path) and CONF.libvirt.images_type != 'rbd': + raise exception.DiskNotFound(location=path) + + info = nova.privsep.qemu.privileged_qemu_img_info( + path, format=format, qemu_version=QEMU_VERSION, + output_format=output_format) + if output_format: + return imageutils.QemuImgInfo(info, format=output_format) + else: + return imageutils.QemuImgInfo(info) + + def convert_image(source, dest, in_format, out_format, run_as_root=False, compress=False): """Convert image to other format.""" diff --git a/requirements.txt b/requirements.txt index 9524d9ceeb2..325b030788f 100644 --- a/requirements.txt +++ b/requirements.txt @@ -42,7 +42,7 @@ oslo.log>=3.36.0 # Apache-2.0 oslo.reports>=1.18.0 # Apache-2.0 oslo.serialization!=2.19.1,>=2.21.1 # Apache-2.0 oslo.upgradecheck>=0.1.1 -oslo.utils>=3.40.2 # Apache-2.0 +oslo.utils>=4.1.0 # Apache-2.0 oslo.db>=4.44.0 # Apache-2.0 oslo.rootwrap>=5.8.0 # Apache-2.0 oslo.messaging>=7.0.0 # Apache-2.0 From 751cd8e5deb7be9535e70ed45e56f5ccf6f26b99 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Thu, 27 Feb 2020 11:47:41 +0000 Subject: [PATCH 5/8] libvirt: Remove QEMU_VERSION_REQ_SHARED MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The updated minimum required libvirt (4.0.0) and QEMU (2.11) for "Ussuri" satisfy the version requirements; this was done in Change-Id: Ia18e9be4d (22c1916b49 — libvirt: Bump MIN_{LIBVIRT,QEMU}_VERSION for "Ussuri", 2019-11-19). Drop the version constant QEMU_VERSION_REQ_SHARED and now-needless compatibility code; adjust/remove tests. Change-Id: If878a023c69f25a9ea45b7de2ff9eb1976aaeb8c Signed-off-by: Stephen Finucane --- nova/privsep/qemu.py | 21 ++++------ nova/tests/unit/virt/libvirt/test_driver.py | 18 --------- nova/tests/unit/virt/libvirt/test_utils.py | 43 +++++---------------- nova/virt/images.py | 10 +---- nova/virt/libvirt/driver.py | 6 +-- 5 files changed, 20 insertions(+), 78 deletions(-) diff --git a/nova/privsep/qemu.py b/nova/privsep/qemu.py index 55bdfe2a774..2b2acfd3e8c 100644 --- a/nova/privsep/qemu.py +++ b/nova/privsep/qemu.py @@ -16,7 +16,6 @@ Helpers for qemu tasks. """ -import operator import os from oslo_concurrency import processutils @@ -33,8 +32,6 @@ cpu_time=30, address_space=1 * units.Gi) -QEMU_VERSION_REQ_SHARED = 2010000 - @nova.privsep.sys_admin_pctxt.entrypoint def convert_image(source, dest, in_format, out_format, instances_path, @@ -85,20 +82,17 @@ def unprivileged_convert_image(source, dest, in_format, out_format, @nova.privsep.sys_admin_pctxt.entrypoint -def privileged_qemu_img_info(path, format=None, qemu_version=None, - output_format=None): +def privileged_qemu_img_info(path, format=None, output_format=None): """Return an oject containing the parsed output from qemu-img info This is a privileged call to qemu-img info using the sys_admin_pctxt entrypoint allowing host block devices etc to be accessed. """ return unprivileged_qemu_img_info( - path, format=format, qemu_version=qemu_version, - output_format=output_format) + path, format=format, output_format=output_format) -def unprivileged_qemu_img_info(path, format=None, qemu_version=None, - output_format=None): +def unprivileged_qemu_img_info(path, format=None, output_format=None): """Return an object containing the parsed output from qemu-img info.""" try: # The following check is about ploop images that reside within @@ -107,15 +101,14 @@ def unprivileged_qemu_img_info(path, format=None, qemu_version=None, os.path.exists(os.path.join(path, "DiskDescriptor.xml"))): path = os.path.join(path, "root.hds") - cmd = ('env', 'LC_ALL=C', 'LANG=C', 'qemu-img', 'info', path) + cmd = ( + 'env', 'LC_ALL=C', 'LANG=C', 'qemu-img', 'info', path, + '--force-share', + ) if format is not None: cmd = cmd + ('-f', format) if output_format is not None: cmd = cmd + ("--output=%s" % (output_format),) - # Check to see if the qemu version is >= 2.10 because if so, we need - # to add the --force-share flag. - if qemu_version and operator.ge(qemu_version, QEMU_VERSION_REQ_SHARED): - cmd = cmd + ('--force-share',) out, err = processutils.execute(*cmd, prlimit=QEMU_IMG_LIMITS) except processutils.ProcessExecutionError as exp: if exp.exit_code == -9: diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index 93b41583042..0db4523eb53 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -105,7 +105,6 @@ from nova.virt import firewall as base_firewall from nova.virt import hardware from nova.virt.image import model as imgmodel -from nova.virt import images from nova.virt.libvirt import blockinfo from nova.virt.libvirt import config as vconfig from nova.virt.libvirt import designer @@ -1207,23 +1206,6 @@ def test_next_min_qemu_version_ok(self, mock_warning, mock_get_libversion): break self.assertFalse(version_arg_found) - # NOTE(sdague): python2.7 and python3.5 have different behaviors - # when it comes to comparing against the sentinel, so - # has_min_version is needed to pass python3.5. - @mock.patch.object(nova.virt.libvirt.host.Host, "has_min_version", - return_value=True) - @mock.patch.object(fakelibvirt.Connection, 'getVersion', - return_value=mock.sentinel.qemu_version) - def test_qemu_image_version(self, mock_get_libversion, min_ver): - """Test that init_host sets qemu image version - - A sentinel is used here so that we aren't chasing this value - against minimums that get raised over time. - """ - drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True) - drvr.init_host("dummyhost") - self.assertEqual(images.QEMU_VERSION, mock.sentinel.qemu_version) - @mock.patch.object(fields.Architecture, "from_host", return_value=fields.Architecture.PPC64) def test_min_version_ppc_ok(self, mock_arch): diff --git a/nova/tests/unit/virt/libvirt/test_utils.py b/nova/tests/unit/virt/libvirt/test_utils.py index 24a7f375f69..97b13861456 100644 --- a/nova/tests/unit/virt/libvirt/test_utils.py +++ b/nova/tests/unit/virt/libvirt/test_utils.py @@ -110,7 +110,7 @@ def test_disk_backing(self, mock_execute, mock_exists): d_backing = libvirt_utils.get_disk_backing_file(path) mock_execute.assert_called_once_with( 'env', 'LC_ALL=C', 'LANG=C', 'qemu-img', 'info', path, - prlimit=nova.privsep.qemu.QEMU_IMG_LIMITS) + '--force-share', prlimit=nova.privsep.qemu.QEMU_IMG_LIMITS) mock_exists.assert_called_once_with(path) self.assertIsNone(d_backing) @@ -119,7 +119,7 @@ def _test_disk_size(self, mock_execute, path, expected_size): self.assertEqual(expected_size, d_size) mock_execute.assert_called_once_with( 'env', 'LC_ALL=C', 'LANG=C', 'qemu-img', 'info', path, - prlimit=nova.privsep.qemu.QEMU_IMG_LIMITS) + '--force-share', prlimit=nova.privsep.qemu.QEMU_IMG_LIMITS) @mock.patch('os.path.exists', return_value=True) def test_disk_size(self, mock_exists): @@ -167,29 +167,7 @@ def test_qemu_img_info_json(self, mock_execute, mock_exists): image_info = images.qemu_img_info(path, output_format='json') mock_execute.assert_called_once_with( 'env', 'LC_ALL=C', 'LANG=C', 'qemu-img', 'info', path, - '--output=json', prlimit=nova.privsep.qemu.QEMU_IMG_LIMITS) - mock_exists.assert_called_once_with(path) - self.assertEqual('disk.config', image_info.image) - self.assertEqual('raw', image_info.file_format) - self.assertEqual(67108864, image_info.virtual_size) - self.assertEqual(98304, image_info.disk_size) - self.assertEqual(65536, image_info.cluster_size) - - @mock.patch('os.path.exists', return_value=True) - @mock.patch('oslo_concurrency.processutils.execute') - def test_qemu_info_canon(self, mock_execute, mock_exists): - path = "disk.config" - example_output = """image: disk.config -file format: raw -virtual size: 64M (67108864 bytes) -cluster_size: 65536 -disk size: 96K -blah BLAH: bb -""" - mock_execute.return_value = (example_output, '') - image_info = images.qemu_img_info(path) - mock_execute.assert_called_once_with( - 'env', 'LC_ALL=C', 'LANG=C', 'qemu-img', 'info', path, + '--force-share', '--output=json', prlimit=nova.privsep.qemu.QEMU_IMG_LIMITS) mock_exists.assert_called_once_with(path) self.assertEqual('disk.config', image_info.image) @@ -200,8 +178,7 @@ def test_qemu_info_canon(self, mock_execute, mock_exists): @mock.patch('os.path.exists', return_value=True) @mock.patch('oslo_concurrency.processutils.execute') - def test_qemu_info_canon_qemu_2_10(self, mock_execute, mock_exists): - images.QEMU_VERSION = nova.privsep.qemu.QEMU_VERSION_REQ_SHARED + def test_qemu_info_canon(self, mock_execute, mock_exists): path = "disk.config" example_output = """image: disk.config file format: raw @@ -237,7 +214,7 @@ def test_qemu_info_canon2(self, mock_execute, mock_exists): image_info = images.qemu_img_info(path) mock_execute.assert_called_once_with( 'env', 'LC_ALL=C', 'LANG=C', 'qemu-img', 'info', path, - prlimit=nova.privsep.qemu.QEMU_IMG_LIMITS) + '--force-share', prlimit=nova.privsep.qemu.QEMU_IMG_LIMITS) mock_exists.assert_called_once_with(path) self.assertEqual('disk.config', image_info.image) self.assertEqual('qcow2', image_info.file_format) @@ -261,7 +238,7 @@ def test_qemu_info_ploop(self, mock_execute, mock_isdir, mock_exists): image_info = images.qemu_img_info(path) mock_execute.assert_called_once_with( 'env', 'LC_ALL=C', 'LANG=C', 'qemu-img', 'info', - os.path.join(path, 'root.hds'), + os.path.join(path, 'root.hds'), '--force-share', prlimit=nova.privsep.qemu.QEMU_IMG_LIMITS) mock_isdir.assert_called_once_with(path) self.assertEqual(2, mock_exists.call_count) @@ -292,7 +269,7 @@ def test_qemu_backing_file_actual(self, image_info = images.qemu_img_info(path) mock_execute.assert_called_once_with( 'env', 'LC_ALL=C', 'LANG=C', 'qemu-img', 'info', path, - prlimit=nova.privsep.qemu.QEMU_IMG_LIMITS) + '--force-share', prlimit=nova.privsep.qemu.QEMU_IMG_LIMITS) mock_exists.assert_called_once_with(path) self.assertEqual('disk.config', image_info.image) self.assertEqual('raw', image_info.file_format) @@ -321,7 +298,7 @@ def test_qemu_info_convert(self, mock_execute, mock_exists): image_info = images.qemu_img_info(path) mock_execute.assert_called_once_with( 'env', 'LC_ALL=C', 'LANG=C', 'qemu-img', 'info', path, - prlimit=nova.privsep.qemu.QEMU_IMG_LIMITS) + '--force-share', prlimit=nova.privsep.qemu.QEMU_IMG_LIMITS) mock_exists.assert_called_once_with(path) self.assertEqual('disk.config', image_info.image) self.assertEqual('raw', image_info.file_format) @@ -346,7 +323,7 @@ def test_qemu_info_snaps(self, mock_execute, mock_exists): image_info = images.qemu_img_info(path) mock_execute.assert_called_once_with( 'env', 'LC_ALL=C', 'LANG=C', 'qemu-img', 'info', path, - prlimit=nova.privsep.qemu.QEMU_IMG_LIMITS) + '--force-share', prlimit=nova.privsep.qemu.QEMU_IMG_LIMITS) mock_exists.assert_called_once_with(path) self.assertEqual('disk.config', image_info.image) self.assertEqual('raw', image_info.file_format) @@ -503,7 +480,7 @@ def test_get_disk_size(self, mock_execute, mock_exists): self.assertEqual(4592640, disk.get_disk_size('/some/path')) mock_execute.assert_called_once_with( 'env', 'LC_ALL=C', 'LANG=C', 'qemu-img', 'info', path, - prlimit=nova.privsep.qemu.QEMU_IMG_LIMITS) + '--force-share', prlimit=nova.privsep.qemu.QEMU_IMG_LIMITS) mock_exists.assert_called_once_with(path) def test_copy_image(self): diff --git a/nova/virt/images.py b/nova/virt/images.py index ded5f9de5b4..9d96fd36eda 100644 --- a/nova/virt/images.py +++ b/nova/virt/images.py @@ -33,10 +33,6 @@ from nova import image import nova.privsep.qemu -# This is set by the libvirt driver on startup. The version is used to -# determine what flags need to be set on the command line. -QEMU_VERSION = None - LOG = logging.getLogger(__name__) CONF = nova.conf.CONF @@ -51,8 +47,7 @@ def qemu_img_info(path, format=None, output_format=None): raise exception.DiskNotFound(location=path) info = nova.privsep.qemu.unprivileged_qemu_img_info( - path, format=format, qemu_version=QEMU_VERSION, - output_format=output_format) + path, format=format, output_format=output_format) if output_format: return imageutils.QemuImgInfo(info, format=output_format) else: @@ -67,8 +62,7 @@ def privileged_qemu_img_info(path, format=None, output_format=None): raise exception.DiskNotFound(location=path) info = nova.privsep.qemu.privileged_qemu_img_info( - path, format=format, qemu_version=QEMU_VERSION, - output_format=output_format) + path, format=format, output_format=output_format) if output_format: return imageutils.QemuImgInfo(info, format=output_format) else: diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index da988294531..fc6833fc9f6 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -686,11 +686,7 @@ def init_host(self, host): libvirt_utils.version_to_string(MIN_LIBVIRT_VERSION)) if CONF.libvirt.virt_type in ("qemu", "kvm"): - if self._host.has_min_version(hv_ver=MIN_QEMU_VERSION): - # "qemu-img info" calls are version dependent, so we need to - # store the version in the images module. - images.QEMU_VERSION = self._host.get_connection().getVersion() - else: + if not self._host.has_min_version(hv_ver=MIN_QEMU_VERSION): raise exception.InternalError( _('Nova requires QEMU version %s or greater.') % libvirt_utils.version_to_string(MIN_QEMU_VERSION)) From 0ab8d549103e280e78862cc1e1cebb842b28bb30 Mon Sep 17 00:00:00 2001 From: Lee Yarwood Date: Fri, 6 Mar 2020 17:02:35 +0000 Subject: [PATCH 6/8] images: Make JSON the default output format of calls to qemu-img info oslo.utils is planning to make JSON the default output format parsed when creating QemuImgInfo objects. As such this change makes JSON the default output_format requested when calling qemu-img info. The majority of this change is actually test removal from nova.tests.unit.virt.libvirt.test_utils as these human readable qemu-img based tests now duplicate tests found in oslo.utils itself. Change-Id: I56676713571e79f05ee3f0bffc5da8386e02c5d4 Conflicts: nova/tests/unit/virt/libvirt/test_driver.py nova/tests/unit/virt/test_images.py nova/virt/libvirt/driver.py Change-Id: I29fa320fdef9553cab046a501778fabbe48c1478 --- nova/privsep/qemu.py | 11 +- nova/tests/unit/privsep/test_qemu.py | 24 ++ nova/tests/unit/virt/libvirt/test_driver.py | 12 +- nova/tests/unit/virt/libvirt/test_utils.py | 280 -------------------- nova/tests/unit/virt/test_images.py | 5 +- nova/virt/images.py | 20 +- 6 files changed, 41 insertions(+), 311 deletions(-) diff --git a/nova/privsep/qemu.py b/nova/privsep/qemu.py index 2b2acfd3e8c..529c24faf31 100644 --- a/nova/privsep/qemu.py +++ b/nova/privsep/qemu.py @@ -82,17 +82,16 @@ def unprivileged_convert_image(source, dest, in_format, out_format, @nova.privsep.sys_admin_pctxt.entrypoint -def privileged_qemu_img_info(path, format=None, output_format=None): +def privileged_qemu_img_info(path, format=None): """Return an oject containing the parsed output from qemu-img info This is a privileged call to qemu-img info using the sys_admin_pctxt entrypoint allowing host block devices etc to be accessed. """ - return unprivileged_qemu_img_info( - path, format=format, output_format=output_format) + return unprivileged_qemu_img_info(path, format=format) -def unprivileged_qemu_img_info(path, format=None, output_format=None): +def unprivileged_qemu_img_info(path, format=None): """Return an object containing the parsed output from qemu-img info.""" try: # The following check is about ploop images that reside within @@ -103,12 +102,10 @@ def unprivileged_qemu_img_info(path, format=None, output_format=None): cmd = ( 'env', 'LC_ALL=C', 'LANG=C', 'qemu-img', 'info', path, - '--force-share', + '--force-share', '--output=json', ) if format is not None: cmd = cmd + ('-f', format) - if output_format is not None: - cmd = cmd + ("--output=%s" % (output_format),) out, err = processutils.execute(*cmd, prlimit=QEMU_IMG_LIMITS) except processutils.ProcessExecutionError as exp: if exp.exit_code == -9: diff --git a/nova/tests/unit/privsep/test_qemu.py b/nova/tests/unit/privsep/test_qemu.py index 5fbc1789837..85c48aa4ae2 100644 --- a/nova/tests/unit/privsep/test_qemu.py +++ b/nova/tests/unit/privsep/test_qemu.py @@ -52,3 +52,27 @@ def test_convert_image(self): def test_convert_image_unprivileged(self): self._test_convert_image(nova.privsep.qemu.unprivileged_convert_image) + + @mock.patch('oslo_concurrency.processutils.execute') + @mock.patch('os.path.isdir') + def _test_qemu_img_info(self, method, mock_isdir, mock_execute): + mock_isdir.return_value = False + mock_execute.return_value = (mock.sentinel.out, None) + expected_cmd = ( + 'env', 'LC_ALL=C', 'LANG=C', 'qemu-img', 'info', + mock.sentinel.path, '--force-share', '--output=json', '-f', + mock.sentinel.format) + + # Assert that the output from processutils is returned + self.assertEqual( + mock.sentinel.out, + method(mock.sentinel.path, format=mock.sentinel.format)) + # Assert that the expected command is used + mock_execute.assert_called_once_with( + *expected_cmd, prlimit=nova.privsep.qemu.QEMU_IMG_LIMITS) + + def test_privileged_qemu_img_info(self): + self._test_qemu_img_info(nova.privsep.qemu.privileged_qemu_img_info) + + def test_unprivileged_qemu_img_info(self): + self._test_qemu_img_info(nova.privsep.qemu.unprivileged_qemu_img_info) diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index 0db4523eb53..b153dbc7703 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -19577,10 +19577,10 @@ def test_prepare_domain_for_snapshot_live_snapshots(self): @mock.patch('os.path.exists') @mock.patch('os.path.getsize') @mock.patch('os.path.isdir') - @mock.patch('oslo_concurrency.processutils.execute') + @mock.patch('nova.virt.images.qemu_img_info') @mock.patch.object(host.Host, '_get_domain') def test_get_instance_disk_info_parallels_ct(self, mock_get_domain, - mock_execute, + mock_qemu_img_info, mock_isdir, mock_getsize, mock_exists, @@ -19595,10 +19595,9 @@ def test_get_instance_disk_info_parallels_ct(self, mock_get_domain, "" "") - ret = ("image: /test/disk/root.hds\n" - "file format: parallels\n" - "virtual size: 20G (21474836480 bytes)\n" - "disk size: 789M\n") + mock_qemu_img_info.return_value = mock.Mock( + virtual_size=21474836480, image="/test/disk/root.hds", + file_format="ploop", size=827327254, backing_file=None) self.flags(virt_type='parallels', group='libvirt') instance = objects.Instance(**self.test_instance) @@ -19618,7 +19617,6 @@ def getsize_sideeffect(*args, **kwargs): mock_getsize.side_effect = getsize_sideeffect mock_exists.return_value = True mock_isdir.return_value = True - mock_execute.return_value = (ret, '') info = drvr.get_instance_disk_info(instance) info = jsonutils.loads(info) diff --git a/nova/tests/unit/virt/libvirt/test_utils.py b/nova/tests/unit/virt/libvirt/test_utils.py index 97b13861456..c53dfd23930 100644 --- a/nova/tests/unit/virt/libvirt/test_utils.py +++ b/nova/tests/unit/virt/libvirt/test_utils.py @@ -37,7 +37,6 @@ from nova.tests import fixtures as nova_fixtures from nova.tests.unit import fake_instance from nova.tests.unit.virt.libvirt import fakelibvirt -from nova.virt.disk import api as disk from nova.virt import images from nova.virt.libvirt import guest as libvirt_guest from nova.virt.libvirt import utils as libvirt_utils @@ -93,244 +92,6 @@ def test_disk_type_ploop(self, mock_isdir, mock_exists): mock_exists.assert_called_once_with("%s/DiskDescriptor.xml" % path) self.assertEqual('ploop', d_type) - @mock.patch('os.path.exists', return_value=True) - @mock.patch('oslo_concurrency.processutils.execute') - def test_disk_backing(self, mock_execute, mock_exists): - path = '/myhome/disk.config' - template_output = """image: %(path)s -file format: raw -virtual size: 2K (2048 bytes) -cluster_size: 65536 -disk size: 96K -""" - output = template_output % ({ - 'path': path, - }) - mock_execute.return_value = (output, '') - d_backing = libvirt_utils.get_disk_backing_file(path) - mock_execute.assert_called_once_with( - 'env', 'LC_ALL=C', 'LANG=C', 'qemu-img', 'info', path, - '--force-share', prlimit=nova.privsep.qemu.QEMU_IMG_LIMITS) - mock_exists.assert_called_once_with(path) - self.assertIsNone(d_backing) - - def _test_disk_size(self, mock_execute, path, expected_size): - d_size = libvirt_utils.get_disk_size(path) - self.assertEqual(expected_size, d_size) - mock_execute.assert_called_once_with( - 'env', 'LC_ALL=C', 'LANG=C', 'qemu-img', 'info', path, - '--force-share', prlimit=nova.privsep.qemu.QEMU_IMG_LIMITS) - - @mock.patch('os.path.exists', return_value=True) - def test_disk_size(self, mock_exists): - path = '/myhome/disk.config' - template_output = """image: %(path)s -file format: raw -virtual size: %(v_size)s (%(vsize_b)s bytes) -cluster_size: 65536 -disk size: 96K -""" - for i in range(0, 128): - bytes = i * 65336 - kbytes = bytes / 1024 - mbytes = kbytes / 1024 - output = template_output % ({ - 'v_size': "%sM" % (mbytes), - 'vsize_b': i, - 'path': path, - }) - with mock.patch('oslo_concurrency.processutils.execute', - return_value=(output, '')) as mock_execute: - self._test_disk_size(mock_execute, path, i) - output = template_output % ({ - 'v_size': "%sK" % (kbytes), - 'vsize_b': i, - 'path': path, - }) - with mock.patch('oslo_concurrency.processutils.execute', - return_value=(output, '')) as mock_execute: - self._test_disk_size(mock_execute, path, i) - - @mock.patch('os.path.exists', return_value=True) - @mock.patch('oslo_concurrency.processutils.execute') - def test_qemu_img_info_json(self, mock_execute, mock_exists): - path = "disk.config" - example_output = """{ - "virtual-size": 67108864, - "filename": "disk.config", - "cluster-size": 65536, - "format": "raw", - "actual-size": 98304 -} -""" - mock_execute.return_value = (example_output, '') - image_info = images.qemu_img_info(path, output_format='json') - mock_execute.assert_called_once_with( - 'env', 'LC_ALL=C', 'LANG=C', 'qemu-img', 'info', path, - '--force-share', '--output=json', - prlimit=nova.privsep.qemu.QEMU_IMG_LIMITS) - mock_exists.assert_called_once_with(path) - self.assertEqual('disk.config', image_info.image) - self.assertEqual('raw', image_info.file_format) - self.assertEqual(67108864, image_info.virtual_size) - self.assertEqual(98304, image_info.disk_size) - self.assertEqual(65536, image_info.cluster_size) - - @mock.patch('os.path.exists', return_value=True) - @mock.patch('oslo_concurrency.processutils.execute') - def test_qemu_info_canon(self, mock_execute, mock_exists): - path = "disk.config" - example_output = """image: disk.config -file format: raw -virtual size: 64M (67108864 bytes) -cluster_size: 65536 -disk size: 96K -blah BLAH: bb -""" - mock_execute.return_value = (example_output, '') - image_info = images.qemu_img_info(path) - mock_execute.assert_called_once_with( - 'env', 'LC_ALL=C', 'LANG=C', 'qemu-img', 'info', path, - '--force-share', prlimit=nova.privsep.qemu.QEMU_IMG_LIMITS) - mock_exists.assert_called_once_with(path) - self.assertEqual('disk.config', image_info.image) - self.assertEqual('raw', image_info.file_format) - self.assertEqual(67108864, image_info.virtual_size) - self.assertEqual(98304, image_info.disk_size) - self.assertEqual(65536, image_info.cluster_size) - - @mock.patch('os.path.exists', return_value=True) - @mock.patch('oslo_concurrency.processutils.execute') - def test_qemu_info_canon2(self, mock_execute, mock_exists): - path = "disk.config" - example_output = """image: disk.config -file format: QCOW2 -virtual size: 67108844 -cluster_size: 65536 -disk size: 963434 -backing file: /var/lib/nova/a328c7998805951a_2 -""" - mock_execute.return_value = (example_output, '') - image_info = images.qemu_img_info(path) - mock_execute.assert_called_once_with( - 'env', 'LC_ALL=C', 'LANG=C', 'qemu-img', 'info', path, - '--force-share', prlimit=nova.privsep.qemu.QEMU_IMG_LIMITS) - mock_exists.assert_called_once_with(path) - self.assertEqual('disk.config', image_info.image) - self.assertEqual('qcow2', image_info.file_format) - self.assertEqual(67108844, image_info.virtual_size) - self.assertEqual(963434, image_info.disk_size) - self.assertEqual(65536, image_info.cluster_size) - self.assertEqual('/var/lib/nova/a328c7998805951a_2', - image_info.backing_file) - - @mock.patch('os.path.exists', return_value=True) - @mock.patch('os.path.isdir', return_value=True) - @mock.patch('oslo_concurrency.processutils.execute') - def test_qemu_info_ploop(self, mock_execute, mock_isdir, mock_exists): - path = "/var/lib/nova" - example_output = """image: root.hds -file format: parallels -virtual size: 3.0G (3221225472 bytes) -disk size: 706M -""" - mock_execute.return_value = (example_output, '') - image_info = images.qemu_img_info(path) - mock_execute.assert_called_once_with( - 'env', 'LC_ALL=C', 'LANG=C', 'qemu-img', 'info', - os.path.join(path, 'root.hds'), '--force-share', - prlimit=nova.privsep.qemu.QEMU_IMG_LIMITS) - mock_isdir.assert_called_once_with(path) - self.assertEqual(2, mock_exists.call_count) - self.assertEqual(path, mock_exists.call_args_list[0][0][0]) - self.assertEqual(os.path.join(path, 'DiskDescriptor.xml'), - mock_exists.call_args_list[1][0][0]) - self.assertEqual('root.hds', image_info.image) - self.assertEqual('parallels', image_info.file_format) - self.assertEqual(3221225472, image_info.virtual_size) - self.assertEqual(740294656, image_info.disk_size) - - @mock.patch('os.path.exists', return_value=True) - @mock.patch('oslo_concurrency.processutils.execute') - def test_qemu_backing_file_actual(self, - mock_execute, mock_exists): - path = "disk.config" - example_output = """image: disk.config -file format: raw -virtual size: 64M (67108864 bytes) -cluster_size: 65536 -disk size: 96K -Snapshot list: -ID TAG VM SIZE DATE VM CLOCK -1 d9a9784a500742a7bb95627bb3aace38 0 2012-08-20 10:52:46 00:00:00.000 -backing file: /var/lib/nova/a328c7998805951a_2 (actual path: /b/3a988059e51a_2) -""" - mock_execute.return_value = (example_output, '') - image_info = images.qemu_img_info(path) - mock_execute.assert_called_once_with( - 'env', 'LC_ALL=C', 'LANG=C', 'qemu-img', 'info', path, - '--force-share', prlimit=nova.privsep.qemu.QEMU_IMG_LIMITS) - mock_exists.assert_called_once_with(path) - self.assertEqual('disk.config', image_info.image) - self.assertEqual('raw', image_info.file_format) - self.assertEqual(67108864, image_info.virtual_size) - self.assertEqual(98304, image_info.disk_size) - self.assertEqual(1, len(image_info.snapshots)) - self.assertEqual('/b/3a988059e51a_2', - image_info.backing_file) - - @mock.patch('os.path.exists', return_value=True) - @mock.patch('oslo_concurrency.processutils.execute') - def test_qemu_info_convert(self, mock_execute, mock_exists): - path = "disk.config" - example_output = """image: disk.config -file format: raw -virtual size: 64M -disk size: 96K -Snapshot list: -ID TAG VM SIZE DATE VM CLOCK -1 d9a9784a500742a7bb95627bb3aace38 0 2012-08-20 10:52:46 00:00:00.000 -3 d9a9784a500742a7bb95627bb3aace38 0 2012-08-20 10:52:46 00:00:00.000 -4 d9a9784a500742a7bb95627bb3aace38 0 2012-08-20 10:52:46 00:00:00.000 -junk stuff: bbb -""" - mock_execute.return_value = (example_output, '') - image_info = images.qemu_img_info(path) - mock_execute.assert_called_once_with( - 'env', 'LC_ALL=C', 'LANG=C', 'qemu-img', 'info', path, - '--force-share', prlimit=nova.privsep.qemu.QEMU_IMG_LIMITS) - mock_exists.assert_called_once_with(path) - self.assertEqual('disk.config', image_info.image) - self.assertEqual('raw', image_info.file_format) - self.assertEqual(67108864, image_info.virtual_size) - self.assertEqual(98304, image_info.disk_size) - - @mock.patch('os.path.exists', return_value=True) - @mock.patch('oslo_concurrency.processutils.execute') - def test_qemu_info_snaps(self, mock_execute, mock_exists): - path = "disk.config" - example_output = """image: disk.config -file format: raw -virtual size: 64M (67108864 bytes) -disk size: 96K -Snapshot list: -ID TAG VM SIZE DATE VM CLOCK -1 d9a9784a500742a7bb95627bb3aace38 0 2012-08-20 10:52:46 00:00:00.000 -3 d9a9784a500742a7bb95627bb3aace38 0 2012-08-20 10:52:46 00:00:00.000 -4 d9a9784a500742a7bb95627bb3aace38 0 2012-08-20 10:52:46 00:00:00.000 -""" - mock_execute.return_value = (example_output, '') - image_info = images.qemu_img_info(path) - mock_execute.assert_called_once_with( - 'env', 'LC_ALL=C', 'LANG=C', 'qemu-img', 'info', path, - '--force-share', prlimit=nova.privsep.qemu.QEMU_IMG_LIMITS) - mock_exists.assert_called_once_with(path) - self.assertEqual('disk.config', image_info.image) - self.assertEqual('raw', image_info.file_format) - self.assertEqual(67108864, image_info.virtual_size) - self.assertEqual(98304, image_info.disk_size) - self.assertEqual(3, len(image_info.snapshots)) - def test_valid_hostname_normal(self): self.assertTrue(libvirt_utils.is_valid_hostname("hello.world.com")) @@ -467,22 +228,6 @@ def xend_probe_side_effect(): libvirt_utils.pick_disk_driver_name(version)) mock_execute.reset_mock() - @mock.patch('os.path.exists', return_value=True) - @mock.patch('oslo_concurrency.processutils.execute') - def test_get_disk_size(self, mock_execute, mock_exists): - path = '/some/path' - example_output = """image: 00000001 -file format: raw -virtual size: 4.4M (4592640 bytes) -disk size: 4.4M -""" - mock_execute.return_value = (example_output, '') - self.assertEqual(4592640, disk.get_disk_size('/some/path')) - mock_execute.assert_called_once_with( - 'env', 'LC_ALL=C', 'LANG=C', 'qemu-img', 'info', path, - '--force-share', prlimit=nova.privsep.qemu.QEMU_IMG_LIMITS) - mock_exists.assert_called_once_with(path) - def test_copy_image(self): dst_fd, dst_path = tempfile.mkstemp() try: @@ -741,31 +486,6 @@ class FakeImgInfo(object): del self.executes - def test_get_disk_backing_file(self): - with_actual_path = False - - def fake_execute(*args, **kwargs): - if with_actual_path: - return ("some: output\n" - "backing file: /foo/bar/baz (actual path: /a/b/c)\n" - "...: ...\n"), '' - else: - return ("some: output\n" - "backing file: /foo/bar/baz\n" - "...: ...\n"), '' - - def return_true(*args, **kwargs): - return True - - self.stub_out('oslo_concurrency.processutils.execute', fake_execute) - self.stub_out('os.path.exists', return_true) - - out = libvirt_utils.get_disk_backing_file('') - self.assertEqual(out, 'baz') - with_actual_path = True - out = libvirt_utils.get_disk_backing_file('') - self.assertEqual(out, 'c') - def test_get_instance_path_at_destination(self): instance = fake_instance.fake_instance_obj(None, name='fake_inst', uuid=uuids.instance) diff --git a/nova/tests/unit/virt/test_images.py b/nova/tests/unit/virt/test_images.py index 9dacc828add..7e2901b2059 100644 --- a/nova/tests/unit/virt/test_images.py +++ b/nova/tests/unit/virt/test_images.py @@ -41,13 +41,12 @@ def test_qemu_info_with_errors(self, path_exists, mock_exec): '/fake/path') @mock.patch.object(os.path, 'exists', return_value=True) - @mock.patch('oslo_concurrency.processutils.execute', - return_value=('stdout', None)) + @mock.patch('nova.privsep.qemu.unprivileged_qemu_img_info', + return_value={}) def test_qemu_info_with_no_errors(self, path_exists, utils_execute): image_info = images.qemu_img_info('/fake/path') self.assertTrue(image_info) - self.assertTrue(str(image_info)) @mock.patch.object(compute_utils, 'disk_ops_semaphore') @mock.patch('nova.privsep.utils.supports_direct_io', return_value=True) diff --git a/nova/virt/images.py b/nova/virt/images.py index 9d96fd36eda..fecc4bd09c9 100644 --- a/nova/virt/images.py +++ b/nova/virt/images.py @@ -39,34 +39,26 @@ IMAGE_API = image.API() -def qemu_img_info(path, format=None, output_format=None): +def qemu_img_info(path, format=None): """Return an object containing the parsed output from qemu-img info.""" # TODO(mikal): this code should not be referring to a libvirt specific # flag. if not os.path.exists(path) and CONF.libvirt.images_type != 'rbd': raise exception.DiskNotFound(location=path) - info = nova.privsep.qemu.unprivileged_qemu_img_info( - path, format=format, output_format=output_format) - if output_format: - return imageutils.QemuImgInfo(info, format=output_format) - else: - return imageutils.QemuImgInfo(info) + info = nova.privsep.qemu.unprivileged_qemu_img_info(path, format=format) + return imageutils.QemuImgInfo(info, format='json') -def privileged_qemu_img_info(path, format=None, output_format=None): +def privileged_qemu_img_info(path, format=None, output_format='json'): """Return an object containing the parsed output from qemu-img info.""" # TODO(mikal): this code should not be referring to a libvirt specific # flag. if not os.path.exists(path) and CONF.libvirt.images_type != 'rbd': raise exception.DiskNotFound(location=path) - info = nova.privsep.qemu.privileged_qemu_img_info( - path, format=format, output_format=output_format) - if output_format: - return imageutils.QemuImgInfo(info, format=output_format) - else: - return imageutils.QemuImgInfo(info) + info = nova.privsep.qemu.privileged_qemu_img_info(path, format=format) + return imageutils.QemuImgInfo(info, format='json') def convert_image(source, dest, in_format, out_format, run_as_root=False, From eeb0ae88f5df1ec472bb7687cbe132cee3930257 Mon Sep 17 00:00:00 2001 From: Dan Smith Date: Thu, 10 Nov 2022 09:55:48 -0800 Subject: [PATCH 7/8] [stable-only][cve] Check VMDK create-type against an allowed list NOTE(sbauza): Stable policy allows us to proactively merge a backport without waiting for the parent patch to be merged (exception to rule #4 in [1]. Marking [stable-only] in order to silence nova-tox-validate-backport [1] https://docs.openstack.org/project-team-guide/stable-branches.html#appropriate-fixes Conflicts vs victoria in: nova/conf/compute.py Related-Bug: #1996188 Change-Id: I5a399f1d3d702bfb76c067893e9c924904c8c360 --- nova/conf/compute.py | 9 ++++++ nova/tests/unit/virt/test_images.py | 46 +++++++++++++++++++++++++++++ nova/virt/images.py | 31 +++++++++++++++++++ 3 files changed, 86 insertions(+) diff --git a/nova/conf/compute.py b/nova/conf/compute.py index 6713f61d47b..f3d138a7a13 100644 --- a/nova/conf/compute.py +++ b/nova/conf/compute.py @@ -956,6 +956,15 @@ * Any integer >= 1 represents the maximum allowed. A value of 0 will cause the ``nova-compute`` service to fail to start, as 0 disk devices is an invalid configuration that would prevent instances from being able to boot. +"""), + cfg.ListOpt('vmdk_allowed_types', + default=['streamOptimized', 'monolithicSparse'], + help=""" +A list of strings describing allowed VMDK "create-type" subformats +that will be allowed. This is recommended to only include +single-file-with-sparse-header variants to avoid potential host file +exposure due to processing named extents. If this list is empty, then no +form of VMDK image will be allowed. """), ] diff --git a/nova/tests/unit/virt/test_images.py b/nova/tests/unit/virt/test_images.py index 7e2901b2059..c1115272a74 100644 --- a/nova/tests/unit/virt/test_images.py +++ b/nova/tests/unit/virt/test_images.py @@ -16,6 +16,8 @@ import mock from oslo_concurrency import processutils +from oslo_serialization import jsonutils +from oslo_utils import imageutils import six from nova.compute import utils as compute_utils @@ -128,3 +130,47 @@ def test_convert_image_without_direct_io_support(self, mock_execute, '-O', 'out_format', '-f', 'in_format', 'source', 'dest') mock_disk_op_sema.__enter__.assert_called_once() self.assertTupleEqual(expected, mock_execute.call_args[0]) + + def test_convert_image_vmdk_allowed_list_checking(self): + info = {'format': 'vmdk', + 'format-specific': { + 'type': 'vmdk', + 'data': { + 'create-type': 'monolithicFlat', + }}} + + # If the format is not in the allowed list, we should get an error + self.assertRaises(exception.ImageUnacceptable, + images.check_vmdk_image, 'foo', + imageutils.QemuImgInfo(jsonutils.dumps(info), + format='json')) + + # With the format in the allowed list, no error + self.flags(vmdk_allowed_types=['streamOptimized', 'monolithicFlat', + 'monolithicSparse'], + group='compute') + images.check_vmdk_image('foo', + imageutils.QemuImgInfo(jsonutils.dumps(info), + format='json')) + + # With an empty list, allow nothing + self.flags(vmdk_allowed_types=[], group='compute') + self.assertRaises(exception.ImageUnacceptable, + images.check_vmdk_image, 'foo', + imageutils.QemuImgInfo(jsonutils.dumps(info), + format='json')) + + @mock.patch.object(images, 'fetch') + @mock.patch('nova.privsep.qemu.unprivileged_qemu_img_info') + def test_fetch_checks_vmdk_rules(self, mock_info, mock_fetch): + info = {'format': 'vmdk', + 'format-specific': { + 'type': 'vmdk', + 'data': { + 'create-type': 'monolithicFlat', + }}} + mock_info.return_value = jsonutils.dumps(info) + with mock.patch('os.path.exists', return_value=True): + e = self.assertRaises(exception.ImageUnacceptable, + images.fetch_to_raw, None, 'foo', 'anypath') + self.assertIn('Invalid VMDK create-type specified', str(e)) diff --git a/nova/virt/images.py b/nova/virt/images.py index fecc4bd09c9..df7ffbd05bf 100644 --- a/nova/virt/images.py +++ b/nova/virt/images.py @@ -114,6 +114,34 @@ def get_info(context, image_href): return IMAGE_API.get(context, image_href) +def check_vmdk_image(image_id, data): + # Check some rules about VMDK files. Specifically we want to make + # sure that the "create-type" of the image is one that we allow. + # Some types of VMDK files can reference files outside the disk + # image and we do not want to allow those for obvious reasons. + + types = CONF.compute.vmdk_allowed_types + + if not len(types): + LOG.warning('Refusing to allow VMDK image as vmdk_allowed_' + 'types is empty') + msg = _('Invalid VMDK create-type specified') + raise exception.ImageUnacceptable(image_id=image_id, reason=msg) + + try: + create_type = data.format_specific['data']['create-type'] + except KeyError: + msg = _('Unable to determine VMDK create-type') + raise exception.ImageUnacceptable(image_id=image_id, reason=msg) + + if create_type not in CONF.compute.vmdk_allowed_types: + LOG.warning('Refusing to process VMDK file with create-type of %r ' + 'which is not in allowed set of: %s', create_type, + ','.join(CONF.compute.vmdk_allowed_types)) + msg = _('Invalid VMDK create-type specified') + raise exception.ImageUnacceptable(image_id=image_id, reason=msg) + + def fetch_to_raw(context, image_href, path, trusted_certs=None): path_tmp = "%s.part" % path fetch(context, image_href, path_tmp, trusted_certs) @@ -133,6 +161,9 @@ def fetch_to_raw(context, image_href, path, trusted_certs=None): reason=(_("fmt=%(fmt)s backed by: %(backing_file)s") % {'fmt': fmt, 'backing_file': backing_file})) + if fmt == 'vmdk': + check_vmdk_image(image_href, data) + if fmt != "raw" and CONF.force_raw_images: staged = "%s.converted" % path LOG.debug("%s was %s, converting to raw", image_href, fmt) From bec330341665388ea7e5d4e2650fc92e359f9713 Mon Sep 17 00:00:00 2001 From: John Garbutt Date: Wed, 1 Feb 2023 15:18:00 +0000 Subject: [PATCH 8/8] Pull in oslo.utils backports needed for CVE fix Change-Id: I26772876a2e325dba98aef457294c631f972b3aa --- requirements.txt | 2 +- tox.ini | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 325b030788f..9524d9ceeb2 100644 --- a/requirements.txt +++ b/requirements.txt @@ -42,7 +42,7 @@ oslo.log>=3.36.0 # Apache-2.0 oslo.reports>=1.18.0 # Apache-2.0 oslo.serialization!=2.19.1,>=2.21.1 # Apache-2.0 oslo.upgradecheck>=0.1.1 -oslo.utils>=4.1.0 # Apache-2.0 +oslo.utils>=3.40.2 # Apache-2.0 oslo.db>=4.44.0 # Apache-2.0 oslo.rootwrap>=5.8.0 # Apache-2.0 oslo.messaging>=7.0.0 # Apache-2.0 diff --git a/tox.ini b/tox.ini index 150d98d0d1b..87d67577ce6 100644 --- a/tox.ini +++ b/tox.ini @@ -38,12 +38,16 @@ passenv = [testenv:py27] commands = +# hack to bring in required backports, higher than upper constraints + pip install 'git+https://github.com/stackhpc/oslo.utils.git@c0de0b493e1c56ec985dca65becdf7d721c0b571' stestr run {posargs} env TEST_OSPROFILER=1 stestr run --combine --no-discover 'nova.tests.unit.test_profiler' stestr slowest [testenv:py36] commands = +# hack to bring in required backports, instead of pip install 'oslo.utils===4.1.2' + pip install 'git+https://github.com/stackhpc/oslo.utils.git@c0de0b493e1c56ec985dca65becdf7d721c0b571' stestr run {posargs} env TEST_OSPROFILER=1 stestr run --combine --no-discover 'nova.tests.unit.test_profiler'