From e38caa2de0c4ac1762822286a4075c593d3eefb2 Mon Sep 17 00:00:00 2001 From: Michael Still Date: Tue, 20 Mar 2018 17:24:52 +1100 Subject: Move image conversion to privsep. Following a similar pattern to previous changes, move calls to qemu-img to convert between image formats to use privsep. Change-Id: I2c3df909a783e1480d3ab4ca10b34b84ac9e4b5f blueprint: hurrah-for-privsep --- nova/privsep/qemu.py | 59 ++++++++++++ nova/privsep/utils.py | 78 +++++++++++++++ nova/tests/unit/privsep/test_utils.py | 111 ++++++++++++++++++++++ nova/tests/unit/test_utils.py | 91 ------------------ nova/tests/unit/virt/libvirt/test_driver.py | 6 +- nova/tests/unit/virt/libvirt/test_imagebackend.py | 58 +++++------ nova/tests/unit/virt/libvirt/test_utils.py | 21 ++-- nova/tests/unit/virt/test_images.py | 10 +- nova/utils.py | 49 ---------- nova/virt/images.py | 31 ++---- nova/virt/libvirt/driver.py | 3 +- 11 files changed, 301 insertions(+), 216 deletions(-) create mode 100644 nova/privsep/qemu.py create mode 100644 nova/privsep/utils.py create mode 100644 nova/tests/unit/privsep/test_utils.py (limited to 'nova') diff --git a/nova/privsep/qemu.py b/nova/privsep/qemu.py new file mode 100644 index 0000000000..30bdd18ddf --- /dev/null +++ b/nova/privsep/qemu.py @@ -0,0 +1,59 @@ +# Copyright 2018 Michael Still and Aptira +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +""" +Helpers for qemu tasks. +""" + +from oslo_concurrency import processutils +from oslo_log import log as logging + +import nova.privsep.utils + + +LOG = logging.getLogger(__name__) + + +@nova.privsep.sys_admin_pctxt.entrypoint +def convert_image(source, dest, in_format, out_format, instances_path): + unprivileged_convert_image(source, dest, in_format, out_format, + instances_path) + + +# NOTE(mikal): this method is deliberately not wrapped in a privsep entrypoint +def unprivileged_convert_image(source, dest, in_format, out_format, + instances_path): + # NOTE(mdbooth): qemu-img convert defaults to cache=unsafe, which means + # that data is not synced to disk at completion. We explicitly use + # cache=none here to (1) ensure that we don't interfere with other + # applications using the host's io cache, and (2) ensure that the data is + # on persistent storage when the command exits. Without (2), a host crash + # may leave a corrupt image in the image cache, which Nova cannot recover + # automatically. + # NOTE(zigo): we cannot use -t none if the instances dir is mounted on a + # filesystem that doesn't have support for O_DIRECT, which is the case + # for example with tmpfs. This simply crashes "openstack server create" + # in environments like live distributions. In such case, the best choice + # is writethrough, which is power-failure safe, but still faster than + # writeback. + if nova.privsep.utils.supports_direct_io(instances_path): + cache_mode = 'none' + else: + cache_mode = 'writethrough' + cmd = ('qemu-img', 'convert', '-t', cache_mode, '-O', out_format) + + if in_format is not None: + cmd = cmd + ('-f', in_format) + cmd = cmd + (source, dest) + processutils.execute(*cmd) diff --git a/nova/privsep/utils.py b/nova/privsep/utils.py new file mode 100644 index 0000000000..150f27a12b --- /dev/null +++ b/nova/privsep/utils.py @@ -0,0 +1,78 @@ +# Copyright 2010 United States Government as represented by the +# Administrator of the National Aeronautics and Space Administration. +# Copyright 2011 Justin Santa Barbara +# Copyright 2018 Michael Still and Aptira +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +# This module is utility methods that privsep depends on. Privsep isn't allowed +# to depend on anything outside the privsep directory, so these need to be +# here. That said, other parts of nova can call into these utilities if +# needed. + +import errno +import mmap +import os + +from oslo_log import log as logging +from oslo_utils import excutils + + +LOG = logging.getLogger(__name__) + + +def supports_direct_io(dirpath): + + if not hasattr(os, 'O_DIRECT'): + LOG.debug("This python runtime does not support direct I/O") + return False + + testfile = os.path.join(dirpath, ".directio.test") + + hasDirectIO = True + fd = None + try: + fd = os.open(testfile, os.O_CREAT | os.O_WRONLY | os.O_DIRECT) + # Check is the write allowed with 512 byte alignment + align_size = 512 + m = mmap.mmap(-1, align_size) + m.write(b"x" * align_size) + os.write(fd, m) + LOG.debug("Path '%(path)s' supports direct I/O", + {'path': dirpath}) + except OSError as e: + if e.errno == errno.EINVAL: + LOG.debug("Path '%(path)s' does not support direct I/O: " + "'%(ex)s'", {'path': dirpath, 'ex': e}) + hasDirectIO = False + else: + with excutils.save_and_reraise_exception(): + LOG.error("Error on '%(path)s' while checking " + "direct I/O: '%(ex)s'", + {'path': dirpath, 'ex': e}) + except Exception as e: + with excutils.save_and_reraise_exception(): + LOG.error("Error on '%(path)s' while checking direct I/O: " + "'%(ex)s'", {'path': dirpath, 'ex': e}) + finally: + # ensure unlink(filepath) will actually remove the file by deleting + # the remaining link to it in close(fd) + if fd is not None: + os.close(fd) + + try: + os.unlink(testfile) + except Exception: + pass + + return hasDirectIO diff --git a/nova/tests/unit/privsep/test_utils.py b/nova/tests/unit/privsep/test_utils.py new file mode 100644 index 0000000000..92e173b958 --- /dev/null +++ b/nova/tests/unit/privsep/test_utils.py @@ -0,0 +1,111 @@ +# Copyright 2011 Justin Santa Barbara +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +import errno +import mock +import os + +import nova.privsep.utils +from nova import test + + +class SupportDirectIOTestCase(test.NoDBTestCase): + def setUp(self): + super(SupportDirectIOTestCase, self).setUp() + # O_DIRECT is not supported on all Python runtimes, so on platforms + # where it's not supported (e.g. Mac), we can still test the code-path + # by stubbing out the value. + if not hasattr(os, 'O_DIRECT'): + # `mock` seems to have trouble stubbing an attr that doesn't + # originally exist, so falling back to stubbing out the attribute + # directly. + os.O_DIRECT = 16384 + self.addCleanup(delattr, os, 'O_DIRECT') + self.einval = OSError() + self.einval.errno = errno.EINVAL + self.test_path = os.path.join('.', '.directio.test') + self.io_flags = os.O_CREAT | os.O_WRONLY | os.O_DIRECT + + open_patcher = mock.patch('os.open') + write_patcher = mock.patch('os.write') + close_patcher = mock.patch('os.close') + unlink_patcher = mock.patch('os.unlink') + self.addCleanup(open_patcher.stop) + self.addCleanup(write_patcher.stop) + self.addCleanup(close_patcher.stop) + self.addCleanup(unlink_patcher.stop) + self.mock_open = open_patcher.start() + self.mock_write = write_patcher.start() + self.mock_close = close_patcher.start() + self.mock_unlink = unlink_patcher.start() + + def test_supports_direct_io(self): + self.mock_open.return_value = 3 + + self.assertTrue(nova.privsep.utils.supports_direct_io('.')) + + self.mock_open.assert_called_once_with(self.test_path, self.io_flags) + self.mock_write.assert_called_once_with(3, mock.ANY) + # ensure unlink(filepath) will actually remove the file by deleting + # the remaining link to it in close(fd) + self.mock_close.assert_called_once_with(3) + self.mock_unlink.assert_called_once_with(self.test_path) + + def test_supports_direct_io_with_exception_in_write(self): + self.mock_open.return_value = 3 + self.mock_write.side_effect = ValueError() + + self.assertRaises(ValueError, nova.privsep.utils.supports_direct_io, + '.') + + self.mock_open.assert_called_once_with(self.test_path, self.io_flags) + self.mock_write.assert_called_once_with(3, mock.ANY) + # ensure unlink(filepath) will actually remove the file by deleting + # the remaining link to it in close(fd) + self.mock_close.assert_called_once_with(3) + self.mock_unlink.assert_called_once_with(self.test_path) + + def test_supports_direct_io_with_exception_in_open(self): + self.mock_open.side_effect = ValueError() + + self.assertRaises(ValueError, nova.privsep.utils.supports_direct_io, + '.') + + self.mock_open.assert_called_once_with(self.test_path, self.io_flags) + self.mock_write.assert_not_called() + self.mock_close.assert_not_called() + self.mock_unlink.assert_called_once_with(self.test_path) + + def test_supports_direct_io_with_oserror_in_write(self): + self.mock_open.return_value = 3 + self.mock_write.side_effect = self.einval + + self.assertFalse(nova.privsep.utils.supports_direct_io('.')) + + self.mock_open.assert_called_once_with(self.test_path, self.io_flags) + self.mock_write.assert_called_once_with(3, mock.ANY) + # ensure unlink(filepath) will actually remove the file by deleting + # the remaining link to it in close(fd) + self.mock_close.assert_called_once_with(3) + self.mock_unlink.assert_called_once_with(self.test_path) + + def test_supports_direct_io_with_oserror_in_open(self): + self.mock_open.side_effect = self.einval + + self.assertFalse(nova.privsep.utils.supports_direct_io('.')) + + self.mock_open.assert_called_once_with(self.test_path, self.io_flags) + self.mock_write.assert_not_called() + self.mock_close.assert_not_called() + self.mock_unlink.assert_called_once_with(self.test_path) diff --git a/nova/tests/unit/test_utils.py b/nova/tests/unit/test_utils.py index 861619cbfa..82b59ceef1 100644 --- a/nova/tests/unit/test_utils.py +++ b/nova/tests/unit/test_utils.py @@ -13,7 +13,6 @@ # under the License. import datetime -import errno import hashlib import importlib import os @@ -1392,93 +1391,3 @@ class GetEndpointTestCase(test.NoDBTestCase): self.adap.get_endpoint_data.assert_not_called() self.assertEqual(3, self.adap.get_endpoint.call_count) self.assertEqual('public', self.adap.interface) - - -class SupportDirectIOTestCase(test.NoDBTestCase): - - def setUp(self): - super(SupportDirectIOTestCase, self).setUp() - # O_DIRECT is not supported on all Python runtimes, so on platforms - # where it's not supported (e.g. Mac), we can still test the code-path - # by stubbing out the value. - if not hasattr(os, 'O_DIRECT'): - # `mock` seems to have trouble stubbing an attr that doesn't - # originally exist, so falling back to stubbing out the attribute - # directly. - os.O_DIRECT = 16384 - self.addCleanup(delattr, os, 'O_DIRECT') - self.einval = OSError() - self.einval.errno = errno.EINVAL - self.test_path = os.path.join('.', '.directio.test') - self.io_flags = os.O_CREAT | os.O_WRONLY | os.O_DIRECT - - open_patcher = mock.patch('os.open') - write_patcher = mock.patch('os.write') - close_patcher = mock.patch('os.close') - unlink_patcher = mock.patch('os.unlink') - self.addCleanup(open_patcher.stop) - self.addCleanup(write_patcher.stop) - self.addCleanup(close_patcher.stop) - self.addCleanup(unlink_patcher.stop) - self.mock_open = open_patcher.start() - self.mock_write = write_patcher.start() - self.mock_close = close_patcher.start() - self.mock_unlink = unlink_patcher.start() - - def test_supports_direct_io(self): - self.mock_open.return_value = 3 - - self.assertTrue(utils.supports_direct_io('.')) - - self.mock_open.assert_called_once_with(self.test_path, self.io_flags) - self.mock_write.assert_called_once_with(3, mock.ANY) - # ensure unlink(filepath) will actually remove the file by deleting - # the remaining link to it in close(fd) - self.mock_close.assert_called_once_with(3) - self.mock_unlink.assert_called_once_with(self.test_path) - - def test_supports_direct_io_with_exception_in_write(self): - self.mock_open.return_value = 3 - self.mock_write.side_effect = ValueError() - - self.assertRaises(ValueError, utils.supports_direct_io, '.') - - self.mock_open.assert_called_once_with(self.test_path, self.io_flags) - self.mock_write.assert_called_once_with(3, mock.ANY) - # ensure unlink(filepath) will actually remove the file by deleting - # the remaining link to it in close(fd) - self.mock_close.assert_called_once_with(3) - self.mock_unlink.assert_called_once_with(self.test_path) - - def test_supports_direct_io_with_exception_in_open(self): - self.mock_open.side_effect = ValueError() - - self.assertRaises(ValueError, utils.supports_direct_io, '.') - - self.mock_open.assert_called_once_with(self.test_path, self.io_flags) - self.mock_write.assert_not_called() - self.mock_close.assert_not_called() - self.mock_unlink.assert_called_once_with(self.test_path) - - def test_supports_direct_io_with_oserror_in_write(self): - self.mock_open.return_value = 3 - self.mock_write.side_effect = self.einval - - self.assertFalse(utils.supports_direct_io('.')) - - self.mock_open.assert_called_once_with(self.test_path, self.io_flags) - self.mock_write.assert_called_once_with(3, mock.ANY) - # ensure unlink(filepath) will actually remove the file by deleting - # the remaining link to it in close(fd) - self.mock_close.assert_called_once_with(3) - self.mock_unlink.assert_called_once_with(self.test_path) - - def test_supports_direct_io_with_oserror_in_open(self): - self.mock_open.side_effect = self.einval - - self.assertFalse(utils.supports_direct_io('.')) - - self.mock_open.assert_called_once_with(self.test_path, self.io_flags) - self.mock_write.assert_not_called() - self.mock_close.assert_not_called() - self.mock_unlink.assert_called_once_with(self.test_path) diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index e2a8239f97..69e5c00ce8 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -7988,7 +7988,7 @@ class LibvirtConnTestCase(test.NoDBTestCase, def connection_supports_direct_io_stub(dirpath): return directio_supported - self.stub_out('nova.utils.supports_direct_io', + self.stub_out('nova.privsep.utils.supports_direct_io', connection_supports_direct_io_stub) instance_ref = objects.Instance(**self.test_instance) @@ -17862,7 +17862,7 @@ class LibvirtDriverTestCase(test.NoDBTestCase): # get_guest_xml(). @mock.patch.object(libvirt_driver.LibvirtDriver, '_set_host_enabled') @mock.patch.object(libvirt_driver.LibvirtDriver, '_build_device_metadata') - @mock.patch('nova.utils.supports_direct_io') + @mock.patch('nova.privsep.utils.supports_direct_io') @mock.patch('nova.api.metadata.base.InstanceMetadata') def _test_finish_migration(self, mock_instance_metadata, mock_supports_direct_io, @@ -18805,7 +18805,7 @@ class LibvirtDriverTestCase(test.NoDBTestCase): # get_guest_xml(). @mock.patch.object(libvirt_driver.LibvirtDriver, '_set_host_enabled') @mock.patch.object(libvirt_driver.LibvirtDriver, '_build_device_metadata') - @mock.patch('nova.utils.supports_direct_io') + @mock.patch('nova.privsep.utils.supports_direct_io') @mock.patch('nova.api.metadata.base.InstanceMetadata') def _test_rescue(self, instance, mock_instance_metadata, mock_supports_direct_io, diff --git a/nova/tests/unit/virt/libvirt/test_imagebackend.py b/nova/tests/unit/virt/libvirt/test_imagebackend.py index c95c72f18d..7e21b06d28 100644 --- a/nova/tests/unit/virt/libvirt/test_imagebackend.py +++ b/nova/tests/unit/virt/libvirt/test_imagebackend.py @@ -670,16 +670,14 @@ class LvmTestCase(_ImageTestCase, test.NoDBTestCase): self.LV = '%s_%s' % (self.INSTANCE['uuid'], self.NAME) self.PATH = os.path.join('/dev', self.VG, self.LV) - @mock.patch('nova.utils.supports_direct_io', return_value=True) + @mock.patch('nova.privsep.utils.supports_direct_io', return_value=True) @mock.patch.object(imagebackend.lvm, 'create_volume') @mock.patch.object(imagebackend.disk, 'get_disk_size', return_value=TEMPLATE_SIZE) - @mock.patch.object(imagebackend.utils, 'execute') - def _create_image(self, sparse, mock_execute, mock_get, mock_create, + @mock.patch('nova.privsep.qemu.convert_image') + def _create_image(self, sparse, mock_convert_image, mock_get, mock_create, mock_ignored): fn = mock.MagicMock() - cmd = ('qemu-img', 'convert', '-t', 'none', '-O', 'raw', - self.TEMPLATE_PATH, self.PATH) image = self.image_class(self.INSTANCE, self.NAME) @@ -691,7 +689,9 @@ class LvmTestCase(_ImageTestCase, test.NoDBTestCase): fn.assert_called_once_with(target=self.TEMPLATE_PATH) mock_get.assert_called_once_with(self.TEMPLATE_PATH) - mock_execute.assert_called_once_with(*cmd, run_as_root=True) + path = '/dev/%s/%s_%s' % (self.VG, self.INSTANCE.uuid, self.NAME) + mock_convert_image.assert_called_once_with( + self.TEMPLATE_PATH, path, None, 'raw', CONF.instances_path) @mock.patch.object(imagebackend.lvm, 'create_volume') def _create_image_generated(self, sparse, mock_create): @@ -705,25 +705,25 @@ class LvmTestCase(_ImageTestCase, test.NoDBTestCase): self.SIZE, sparse=sparse) fn.assert_called_once_with(target=self.PATH, ephemeral_size=None) - @mock.patch('nova.utils.supports_direct_io', return_value=True) + @mock.patch('nova.privsep.utils.supports_direct_io', return_value=True) @mock.patch.object(imagebackend.disk, 'resize2fs') @mock.patch.object(imagebackend.lvm, 'create_volume') @mock.patch.object(imagebackend.disk, 'get_disk_size', return_value=TEMPLATE_SIZE) - @mock.patch.object(imagebackend.utils, 'execute') - def _create_image_resize(self, sparse, mock_execute, mock_get, + @mock.patch('nova.privsep.qemu.convert_image') + def _create_image_resize(self, sparse, mock_convert_image, mock_get, mock_create, mock_resize, mock_ignored): fn = mock.MagicMock() fn(target=self.TEMPLATE_PATH) - cmd = ('qemu-img', 'convert', '-t', 'none', '-O', 'raw', - self.TEMPLATE_PATH, self.PATH) image = self.image_class(self.INSTANCE, self.NAME) image.create_image(fn, self.TEMPLATE_PATH, self.SIZE) mock_create.assert_called_once_with(self.VG, self.LV, self.SIZE, sparse=sparse) mock_get.assert_called_once_with(self.TEMPLATE_PATH) - mock_execute.assert_called_once_with(*cmd, run_as_root=True) + mock_convert_image.assert_called_once_with( + self.TEMPLATE_PATH, self.PATH, None, 'raw', + CONF.instances_path) mock_resize.assert_called_once_with(self.PATH, run_as_root=True) @mock.patch.object(imagebackend.fileutils, 'ensure_tree') @@ -928,7 +928,8 @@ class EncryptedLvmTestCase(_ImageTestCase, test.NoDBTestCase): def _create_image(self, sparse): with test.nested( - mock.patch('nova.utils.supports_direct_io', return_value=True), + mock.patch('nova.privsep.utils.supports_direct_io', + return_value=True), mock.patch.object(self.lvm, 'create_volume', mock.Mock()), mock.patch.object(self.lvm, 'remove_volumes', mock.Mock()), mock.patch.object(self.disk, 'resize2fs', mock.Mock()), @@ -941,7 +942,7 @@ class EncryptedLvmTestCase(_ImageTestCase, test.NoDBTestCase): mock.Mock()), mock.patch.object(self.libvirt_utils, 'remove_logical_volumes', mock.Mock()), - mock.patch.object(self.utils, 'execute', mock.Mock())): + mock.patch('nova.privsep.qemu.convert_image')): fn = mock.Mock() image = self.image_class(self.INSTANCE, self.NAME) @@ -960,14 +961,9 @@ class EncryptedLvmTestCase(_ImageTestCase, test.NoDBTestCase): CONF.ephemeral_storage_encryption.cipher, CONF.ephemeral_storage_encryption.key_size, self.KEY) - cmd = ('qemu-img', - 'convert', - '-t', 'none', - '-O', - 'raw', - self.TEMPLATE_PATH, - self.PATH) - self.utils.execute.assert_called_with(*cmd, run_as_root=True) + nova.privsep.qemu.convert_image.assert_called_with( + self.TEMPLATE_PATH, self.PATH, None, 'raw', + CONF.instances_path) def _create_image_generated(self, sparse): with test.nested( @@ -983,7 +979,7 @@ class EncryptedLvmTestCase(_ImageTestCase, test.NoDBTestCase): mock.Mock()), mock.patch.object(self.libvirt_utils, 'remove_logical_volumes', mock.Mock()), - mock.patch.object(self.utils, 'execute', mock.Mock())): + mock.patch('nova.privsep.qemu.convert_image')): fn = mock.Mock() image = self.image_class(self.INSTANCE, self.NAME) @@ -1008,7 +1004,8 @@ class EncryptedLvmTestCase(_ImageTestCase, test.NoDBTestCase): def _create_image_resize(self, sparse): with test.nested( - mock.patch('nova.utils.supports_direct_io', return_value=True), + mock.patch('nova.privsep.utils.supports_direct_io', + return_value=True), mock.patch.object(self.lvm, 'create_volume', mock.Mock()), mock.patch.object(self.lvm, 'remove_volumes', mock.Mock()), mock.patch.object(self.disk, 'resize2fs', mock.Mock()), @@ -1021,7 +1018,7 @@ class EncryptedLvmTestCase(_ImageTestCase, test.NoDBTestCase): mock.Mock()), mock.patch.object(self.libvirt_utils, 'remove_logical_volumes', mock.Mock()), - mock.patch.object(self.utils, 'execute', mock.Mock())): + mock.patch('nova.privsep.qemu.convert_image')): fn = mock.Mock() image = self.image_class(self.INSTANCE, self.NAME) @@ -1042,14 +1039,9 @@ class EncryptedLvmTestCase(_ImageTestCase, test.NoDBTestCase): CONF.ephemeral_storage_encryption.cipher, CONF.ephemeral_storage_encryption.key_size, self.KEY) - cmd = ('qemu-img', - 'convert', - '-t', 'none', - '-O', - 'raw', - self.TEMPLATE_PATH, - self.PATH) - self.utils.execute.assert_called_with(*cmd, run_as_root=True) + nova.privsep.qemu.convert_image.assert_called_with( + self.TEMPLATE_PATH, self.PATH, None, 'raw', + CONF.instances_path) self.disk.resize2fs.assert_called_with(self.PATH, run_as_root=True) def test_create_image(self): diff --git a/nova/tests/unit/virt/libvirt/test_utils.py b/nova/tests/unit/virt/libvirt/test_utils.py index e5a710b0e4..1ab4a50cdc 100644 --- a/nova/tests/unit/virt/libvirt/test_utils.py +++ b/nova/tests/unit/virt/libvirt/test_utils.py @@ -629,12 +629,9 @@ disk size: 4.4M mock_images.assert_called_once_with( _context, image_id, target) - @mock.patch('nova.utils.supports_direct_io', return_value=True) - def test_fetch_raw_image(self, mock_direct_io): - - def fake_execute(*cmd, **kwargs): - self.executes.append(cmd) - return None, None + @mock.patch('nova.privsep.utils.supports_direct_io', return_value=True) + @mock.patch('nova.privsep.qemu.unprivileged_convert_image') + def test_fetch_raw_image(self, mock_convert_image, mock_direct_io): def fake_rename(old, new): self.executes.append(('mv', old, new)) @@ -666,7 +663,6 @@ disk size: 4.4M return FakeImgInfo() - self.stub_out('nova.utils.execute', fake_execute) self.stub_out('os.rename', fake_rename) self.stub_out('os.unlink', fake_unlink) self.stub_out('nova.virt.images.fetch', lambda *_, **__: None) @@ -686,19 +682,21 @@ disk size: 4.4M target = 't.qcow2' self.executes = [] - expected_commands = [('qemu-img', 'convert', '-t', 'none', - '-O', 'raw', '-f', 'qcow2', - 't.qcow2.part', 't.qcow2.converted'), - ('rm', 't.qcow2.part'), + expected_commands = [('rm', 't.qcow2.part'), ('mv', 't.qcow2.converted', 't.qcow2')] images.fetch_to_raw(context, image_id, target) self.assertEqual(self.executes, expected_commands) + mock_convert_image.assert_called_with( + 't.qcow2.part', 't.qcow2.converted', 'qcow2', 'raw', + CONF.instances_path) + mock_convert_image.reset_mock() target = 't.raw' self.executes = [] expected_commands = [('mv', 't.raw.part', 't.raw')] images.fetch_to_raw(context, image_id, target) self.assertEqual(self.executes, expected_commands) + mock_convert_image.assert_not_called() target = 'backing.qcow2' self.executes = [] @@ -706,6 +704,7 @@ disk size: 4.4M self.assertRaises(exception.ImageUnacceptable, images.fetch_to_raw, context, image_id, target) self.assertEqual(self.executes, expected_commands) + mock_convert_image.assert_not_called() del self.executes diff --git a/nova/tests/unit/virt/test_images.py b/nova/tests/unit/virt/test_images.py index c7ed95d859..1386f7a6c0 100644 --- a/nova/tests/unit/virt/test_images.py +++ b/nova/tests/unit/virt/test_images.py @@ -49,7 +49,7 @@ class QemuTestCase(test.NoDBTestCase): self.assertTrue(image_info) self.assertTrue(str(image_info)) - @mock.patch('nova.utils.supports_direct_io', return_value=True) + @mock.patch('nova.privsep.utils.supports_direct_io', return_value=True) @mock.patch.object(utils, 'execute', side_effect=processutils.ProcessExecutionError) def test_convert_image_with_errors(self, mocked_execute, mock_direct_io): @@ -101,8 +101,8 @@ class QemuTestCase(test.NoDBTestCase): images.fetch_to_raw, None, 'href123', '/no/path') - @mock.patch('nova.utils.supports_direct_io', return_value=True) - @mock.patch('nova.utils.execute') + @mock.patch('nova.privsep.utils.supports_direct_io', return_value=True) + @mock.patch('oslo_concurrency.processutils.execute') def test_convert_image_with_direct_io_support(self, mock_execute, mock_direct_io): images._convert_image('source', 'dest', 'in_format', 'out_format', @@ -111,8 +111,8 @@ class QemuTestCase(test.NoDBTestCase): '-f', 'in_format', 'source', 'dest') self.assertTupleEqual(expected, mock_execute.call_args[0]) - @mock.patch('nova.utils.supports_direct_io', return_value=False) - @mock.patch('nova.utils.execute') + @mock.patch('nova.privsep.utils.supports_direct_io', return_value=False) + @mock.patch('oslo_concurrency.processutils.execute') def test_convert_image_without_direct_io_support(self, mock_execute, mock_direct_io): images._convert_image('source', 'dest', 'in_format', 'out_format', diff --git a/nova/utils.py b/nova/utils.py index bd93aea3aa..c2e304fc99 100644 --- a/nova/utils.py +++ b/nova/utils.py @@ -20,11 +20,9 @@ import contextlib import copy import datetime -import errno import functools import hashlib import inspect -import mmap import os import pyclbr import random @@ -1341,53 +1339,6 @@ def get_endpoint(ksa_adapter): "interfaces: %s" % interfaces) -def supports_direct_io(dirpath): - - if not hasattr(os, 'O_DIRECT'): - LOG.debug("This python runtime does not support direct I/O") - return False - - testfile = os.path.join(dirpath, ".directio.test") - - hasDirectIO = True - fd = None - try: - fd = os.open(testfile, os.O_CREAT | os.O_WRONLY | os.O_DIRECT) - # Check is the write allowed with 512 byte alignment - align_size = 512 - m = mmap.mmap(-1, align_size) - m.write(b"x" * align_size) - os.write(fd, m) - LOG.debug("Path '%(path)s' supports direct I/O", - {'path': dirpath}) - except OSError as e: - if e.errno == errno.EINVAL: - LOG.debug("Path '%(path)s' does not support direct I/O: " - "'%(ex)s'", {'path': dirpath, 'ex': e}) - hasDirectIO = False - else: - with excutils.save_and_reraise_exception(): - LOG.error("Error on '%(path)s' while checking " - "direct I/O: '%(ex)s'", - {'path': dirpath, 'ex': e}) - except Exception as e: - with excutils.save_and_reraise_exception(): - LOG.error("Error on '%(path)s' while checking direct I/O: " - "'%(ex)s'", {'path': dirpath, 'ex': e}) - finally: - # ensure unlink(filepath) will actually remove the file by deleting - # the remaining link to it in close(fd) - if fd is not None: - os.close(fd) - - try: - os.unlink(testfile) - except Exception: - pass - - return hasDirectIO - - def generate_hostid(host, project_id): """Generate an obfuscated host id representing the host. diff --git a/nova/virt/images.py b/nova/virt/images.py index dfd415052d..01bcb4d64c 100644 --- a/nova/virt/images.py +++ b/nova/virt/images.py @@ -32,6 +32,7 @@ import nova.conf from nova import exception from nova.i18n import _ from nova import image +import nova.privsep.qemu from nova import utils LOG = logging.getLogger(__name__) @@ -116,30 +117,14 @@ def convert_image_unsafe(source, dest, out_format, run_as_root=False): def _convert_image(source, dest, in_format, out_format, run_as_root): - # NOTE(mdbooth): qemu-img convert defaults to cache=unsafe, which means - # that data is not synced to disk at completion. We explicitly use - # cache=none here to (1) ensure that we don't interfere with other - # applications using the host's io cache, and (2) ensure that the data is - # on persistent storage when the command exits. Without (2), a host crash - # may leave a corrupt image in the image cache, which Nova cannot recover - # automatically. - # NOTE(zigo): we cannot use -t none if the instances dir is mounted on a - # filesystem that doesn't have support for O_DIRECT, which is the case - # for example with tmpfs. This simply crashes "openstack server create" - # in environments like live distributions. In such case, the best choice - # is writethrough, which is power-failure safe, but still faster than - # writeback. - if utils.supports_direct_io(CONF.instances_path): - cache_mode = 'none' - else: - cache_mode = 'writethrough' - cmd = ('qemu-img', 'convert', '-t', cache_mode, '-O', out_format) - - if in_format is not None: - cmd = cmd + ('-f', in_format) - cmd = cmd + (source, dest) try: - utils.execute(*cmd, run_as_root=run_as_root) + if not run_as_root: + nova.privsep.qemu.unprivileged_convert_image( + source, dest, in_format, out_format, CONF.instances_path) + else: + nova.privsep.qemu.convert_image( + source, dest, in_format, out_format, CONF.instances_path) + except processutils.ProcessExecutionError as exp: msg = (_("Unable to convert image to %(format)s: %(exp)s") % {'format': out_format, 'exp': exp}) diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index bc59a329fe..472749350f 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -90,6 +90,7 @@ from nova.pci import manager as pci_manager from nova.pci import utils as pci_utils import nova.privsep.libvirt import nova.privsep.path +import nova.privsep.utils from nova import rc_fields from nova import utils from nova import version @@ -405,7 +406,7 @@ class LibvirtDriver(driver.ComputeDriver): # is safe for migration provided the filesystem is cache coherent # (cluster filesystems typically are, but things like NFS are not). self._disk_cachemode = "none" - if not utils.supports_direct_io(CONF.instances_path): + if not nova.privsep.utils.supports_direct_io(CONF.instances_path): self._disk_cachemode = "writethrough" return self._disk_cachemode -- cgit v1.2.1