From e6ce9557f84cdcdf4ffdd12ce73a008c96c7b94a Mon Sep 17 00:00:00 2001 From: Thomas Goirand Date: Tue, 28 Nov 2017 23:22:56 +0100 Subject: [PATCH] qemu-img do not use cache=none if no O_DIRECT support If /var/lib/nova/instances is mounted on a filesystem like tmpfs that doesn't have support for O_DIRECT, "qemu-img convert" currently crashes because it's unconditionally using the "-t none" flag. This patch therefore: - moves the _supports_direct_io() function out of the libvirt driver, from nova/virt/libvirt/driver.py to nova/utils.py and makes it public. - uses that function to decide to use -t none or -t writethrough when converting images with qemu-img. Closes-Bug: #1734784 Co-Authored-By: melanie witt Change-Id: Ifb47de00abf3f83442ca5264fbc24885df924a19 --- nova/tests/unit/test_utils.py | 53 ++++++++++++++++++ nova/tests/unit/virt/libvirt/test_driver.py | 60 ++------------------- nova/tests/unit/virt/libvirt/test_utils.py | 3 +- nova/tests/unit/virt/test_images.py | 23 +++++++- nova/utils.py | 49 +++++++++++++++++ nova/virt/images.py | 13 ++++- nova/virt/libvirt/driver.py | 50 +---------------- 7 files changed, 143 insertions(+), 108 deletions(-) diff --git a/nova/tests/unit/test_utils.py b/nova/tests/unit/test_utils.py index 629d7a942a28..aef876dffd14 100644 --- a/nova/tests/unit/test_utils.py +++ b/nova/tests/unit/test_utils.py @@ -13,6 +13,7 @@ # under the License. import datetime +import errno import hashlib import importlib import os @@ -25,6 +26,7 @@ from keystoneauth1 import exceptions as ks_exc from keystoneauth1.identity import base as ks_identity from keystoneauth1 import session as ks_session import mock +from mox3 import mox import netaddr from oslo_concurrency import processutils from oslo_config import cfg @@ -1438,3 +1440,54 @@ 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) + + def _behave_supports_direct_io(self, raise_open=False, raise_write=False, + exc=ValueError()): + open_behavior = os.open(os.path.join('.', '.directio.test'), + os.O_CREAT | os.O_WRONLY | os.O_DIRECT) + if raise_open: + open_behavior.AndRaise(exc) + else: + open_behavior.AndReturn(3) + write_bahavior = os.write(3, mox.IgnoreArg()) + if raise_write: + write_bahavior.AndRaise(exc) + + # ensure unlink(filepath) will actually remove the file by deleting + # the remaining link to it in close(fd) + os.close(3) + + os.unlink(3) + + def test_supports_direct_io(self): + # 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') + + einval = OSError() + einval.errno = errno.EINVAL + self.mox.StubOutWithMock(os, 'open') + self.mox.StubOutWithMock(os, 'write') + self.mox.StubOutWithMock(os, 'close') + self.mox.StubOutWithMock(os, 'unlink') + _supports_direct_io = utils.supports_direct_io + + self._behave_supports_direct_io() + self._behave_supports_direct_io(raise_write=True) + self._behave_supports_direct_io(raise_open=True) + self._behave_supports_direct_io(raise_write=True, exc=einval) + self._behave_supports_direct_io(raise_open=True, exc=einval) + + self.mox.ReplayAll() + self.assertTrue(_supports_direct_io('.')) + self.assertRaises(ValueError, _supports_direct_io, '.') + self.assertRaises(ValueError, _supports_direct_io, '.') + self.assertFalse(_supports_direct_io('.')) + self.assertFalse(_supports_direct_io('.')) + self.mox.VerifyAll() diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index d90bc65ff6ec..fb4764703e05 100755 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -6835,57 +6835,6 @@ class LibvirtConnTestCase(test.NoDBTestCase, self.assertEqual(len(interfaces), 2) self.assertEqual(interfaces[0].get('type'), 'bridge') - def _behave_supports_direct_io(self, raise_open=False, raise_write=False, - exc=ValueError()): - open_behavior = os.open(os.path.join('.', '.directio.test'), - os.O_CREAT | os.O_WRONLY | os.O_DIRECT) - if raise_open: - open_behavior.AndRaise(exc) - else: - open_behavior.AndReturn(3) - write_bahavior = os.write(3, mox.IgnoreArg()) - if raise_write: - write_bahavior.AndRaise(exc) - - # ensure unlink(filepath) will actually remove the file by deleting - # the remaining link to it in close(fd) - os.close(3) - - os.unlink(3) - - def test_supports_direct_io(self): - # 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') - - einval = OSError() - einval.errno = errno.EINVAL - self.mox.StubOutWithMock(os, 'open') - self.mox.StubOutWithMock(os, 'write') - self.mox.StubOutWithMock(os, 'close') - self.mox.StubOutWithMock(os, 'unlink') - _supports_direct_io = libvirt_driver.LibvirtDriver._supports_direct_io - - self._behave_supports_direct_io() - self._behave_supports_direct_io(raise_write=True) - self._behave_supports_direct_io(raise_open=True) - self._behave_supports_direct_io(raise_write=True, exc=einval) - self._behave_supports_direct_io(raise_open=True, exc=einval) - - self.mox.ReplayAll() - self.assertTrue(_supports_direct_io('.')) - self.assertRaises(ValueError, _supports_direct_io, '.') - self.assertRaises(ValueError, _supports_direct_io, '.') - self.assertFalse(_supports_direct_io('.')) - self.assertFalse(_supports_direct_io('.')) - self.mox.VerifyAll() - def _check_xml_and_container(self, instance): instance_ref = objects.Instance(**instance) image_meta = objects.ImageMeta.from_dict(self.test_image_meta) @@ -6982,12 +6931,11 @@ class LibvirtConnTestCase(test.NoDBTestCase, self.stub_out('os.open', os_open_stub) - @staticmethod def connection_supports_direct_io_stub(dirpath): return directio_supported - self.stubs.Set(libvirt_driver.LibvirtDriver, - '_supports_direct_io', connection_supports_direct_io_stub) + self.stub_out('nova.utils.supports_direct_io', + connection_supports_direct_io_stub) instance_ref = objects.Instance(**self.test_instance) image_meta = objects.ImageMeta.from_dict(self.test_image_meta) @@ -16526,7 +16474,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.object(libvirt_driver.LibvirtDriver, '_supports_direct_io') + @mock.patch('nova.utils.supports_direct_io') @mock.patch('nova.api.metadata.base.InstanceMetadata') def _test_finish_migration(self, mock_instance_metadata, mock_supports_direct_io, @@ -17485,7 +17433,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.object(libvirt_driver.LibvirtDriver, '_supports_direct_io') + @mock.patch('nova.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_utils.py b/nova/tests/unit/virt/libvirt/test_utils.py index 631e83b657bd..965a4ee4dfb6 100644 --- a/nova/tests/unit/virt/libvirt/test_utils.py +++ b/nova/tests/unit/virt/libvirt/test_utils.py @@ -628,7 +628,8 @@ disk size: 4.4M mock_images.assert_called_once_with( _context, image_id, target) - def test_fetch_raw_image(self): + @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) diff --git a/nova/tests/unit/virt/test_images.py b/nova/tests/unit/virt/test_images.py index c6864f295df9..0eeee35ff2f6 100644 --- a/nova/tests/unit/virt/test_images.py +++ b/nova/tests/unit/virt/test_images.py @@ -45,9 +45,10 @@ 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.object(utils, 'execute', side_effect=processutils.ProcessExecutionError) - def test_convert_image_with_errors(self, mocked_execute): + def test_convert_image_with_errors(self, mocked_execute, mock_direct_io): self.assertRaises(exception.ImageUnacceptable, images.convert_image, '/path/that/does/not/exist', @@ -77,3 +78,23 @@ class QemuTestCase(test.NoDBTestCase): 'Image href123 is unacceptable.*', images.fetch_to_raw, None, 'href123', '/no/path') + + @mock.patch('nova.utils.supports_direct_io', return_value=True) + @mock.patch('nova.utils.execute') + def test_convert_image_with_direct_io_support(self, mock_execute, + mock_direct_io): + images._convert_image('source', 'dest', 'in_format', 'out_format', + run_as_root=False) + expected = ('qemu-img', 'convert', '-t', 'none', '-O', 'out_format', + '-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') + def test_convert_image_without_direct_io_support(self, mock_execute, + mock_direct_io): + images._convert_image('source', 'dest', 'in_format', 'out_format', + run_as_root=False) + expected = ('qemu-img', 'convert', '-t', 'writethrough', + '-O', 'out_format', '-f', 'in_format', 'source', 'dest') + self.assertTupleEqual(expected, mock_execute.call_args[0]) diff --git a/nova/utils.py b/nova/utils.py index f83f5e5c771b..10bae57a4db9 100644 --- a/nova/utils.py +++ b/nova/utils.py @@ -20,9 +20,11 @@ import contextlib import copy import datetime +import errno import functools import hashlib import inspect +import mmap import os import pyclbr import random @@ -1379,3 +1381,50 @@ def get_endpoint(ksa_adapter): raise ks_exc.EndpointNotFound( "Could not find requested endpoint for any of the following " "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 diff --git a/nova/virt/images.py b/nova/virt/images.py index be2a9d9e0621..b78389000c20 100644 --- a/nova/virt/images.py +++ b/nova/virt/images.py @@ -118,7 +118,18 @@ def _convert_image(source, dest, in_format, out_format, run_as_root): # 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. - cmd = ('qemu-img', 'convert', '-t', 'none', '-O', out_format) + # 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) diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index 047b6ebb47b3..5fcf0d4408e6 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -32,7 +32,6 @@ import errno import functools import glob import itertools -import mmap import operator import os import pwd @@ -410,7 +409,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 self._supports_direct_io(CONF.instances_path): + if not utils.supports_direct_io(CONF.instances_path): self._disk_cachemode = "writethrough" return self._disk_cachemode @@ -2961,53 +2960,6 @@ class LibvirtDriver(driver.ComputeDriver): return ctype.ConsoleSerial(host=hostname, port=port) raise exception.ConsoleTypeUnavailable(console_type='serial') - @staticmethod - 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 - @staticmethod def _create_ephemeral(target, ephemeral_size, fs_label, os_type, is_block_dev=False,