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

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

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

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)

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)

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,

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

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
 

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

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.
 

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

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