From 641798f75f50e7b4db3c7a8ccefcb8b590228893 Mon Sep 17 00:00:00 2001 From: Eric Fried Date: Tue, 28 Mar 2017 17:13:09 -0500 Subject: Glance download: only fsync files Recent changes [1][2] added fsync to the data file in GlanceImageServiceV2.download. This raises EINVAL if the file is a pipe/FIFO or socket [3]. This change set adds a static _safe_fsync method to GlanceImageServiceV2 which conditions the fsync call not to run if the file handle represents a pipe/FIFO or socket, and uses that call from the download method. [1] https://review.openstack.org/#/c/441246/ [2] https://review.openstack.org/#/c/443583/ [3] http://man7.org/linux/man-pages/man2/fsync.2.html#ERRORS Change-Id: Ied5788deadcf3d1336a48288cf49d8571db23659 Closes-Bug: #1677047 --- nova/image/glance.py | 20 ++++++++++- nova/tests/unit/image/test_glance.py | 65 +++++++++++++++++++++++++++--------- 2 files changed, 69 insertions(+), 16 deletions(-) diff --git a/nova/image/glance.py b/nova/image/glance.py index 00eb04121f..7d819f915a 100644 --- a/nova/image/glance.py +++ b/nova/image/glance.py @@ -22,6 +22,7 @@ import inspect import itertools import os import random +import stat import sys import time @@ -47,6 +48,7 @@ from nova import objects from nova.objects import fields from nova import service_auth + LOG = logging.getLogger(__name__) CONF = nova.conf.CONF @@ -272,6 +274,22 @@ class GlanceImageServiceV2(object): return _images + @staticmethod + def _safe_fsync(fh): + """Performs os.fsync on a filehandle only if it is supported. + + fsync on a pipe, FIFO, or socket raises OSError with EINVAL. This + method discovers whether the target filehandle is one of these types + and only performs fsync if it isn't. + + :param fh: Open filehandle (not a path or fileno) to maybe fsync. + """ + fileno = fh.fileno() + mode = os.fstat(fileno).st_mode + # A pipe answers True to S_ISFIFO + if not any(check(mode) for check in (stat.S_ISFIFO, stat.S_ISSOCK)): + os.fsync(fileno) + def download(self, context, image_id, data=None, dst_path=None): """Calls out to Glance for data and writes data.""" if CONF.glance.allowed_direct_url_schemes and dst_path is not None: @@ -371,7 +389,7 @@ class GlanceImageServiceV2(object): # subsequent host crash we don't have running instances # using a corrupt backing file. data.flush() - os.fsync(data.fileno()) + self._safe_fsync(data) data.close() def create(self, context, image_meta, data=None): diff --git a/nova/tests/unit/image/test_glance.py b/nova/tests/unit/image/test_glance.py index bf4251ae7f..879ce27ee0 100644 --- a/nova/tests/unit/image/test_glance.py +++ b/nova/tests/unit/image/test_glance.py @@ -553,7 +553,7 @@ class TestDownloadNoDirectUri(test.NoDBTestCase): @mock.patch.object(six.moves.builtins, 'open') @mock.patch('nova.image.glance.GlanceImageServiceV2.show') - @mock.patch('os.fsync') + @mock.patch('nova.image.glance.GlanceImageServiceV2._safe_fsync') def test_download_no_data_dest_path_v2(self, fsync_mock, show_mock, open_mock): client = mock.MagicMock() @@ -569,8 +569,7 @@ class TestDownloadNoDirectUri(test.NoDBTestCase): client.call.assert_called_once_with(ctx, 2, 'data', mock.sentinel.image_id) open_mock.assert_called_once_with(mock.sentinel.dst_path, 'wb') - fsync_mock.assert_called_once_with( - writer.fileno.return_value) + fsync_mock.assert_called_once_with(writer) self.assertIsNone(res) writer.write.assert_has_calls( [ @@ -664,7 +663,7 @@ class TestDownloadNoDirectUri(test.NoDBTestCase): @mock.patch.object(six.moves.builtins, 'open') @mock.patch('nova.image.glance.GlanceImageServiceV2._get_transfer_module') @mock.patch('nova.image.glance.GlanceImageServiceV2.show') - @mock.patch('os.fsync') + @mock.patch('nova.image.glance.GlanceImageServiceV2._safe_fsync') def test_download_direct_exception_fallback_v2( self, fsync_mock, show_mock, get_tran_mock, open_mock): # Test that we fall back to downloading to the dst_path @@ -701,8 +700,7 @@ class TestDownloadNoDirectUri(test.NoDBTestCase): mock.sentinel.loc_meta) client.call.assert_called_once_with(ctx, 2, 'data', mock.sentinel.image_id) - fsync_mock.assert_called_once_with( - open_mock.return_value.fileno.return_value) + fsync_mock.assert_called_once_with(writer) # NOTE(jaypipes): log messages call open() in part of the # download path, so here, we just check that the last open() # call was done for the dst_path file descriptor. @@ -719,7 +717,7 @@ class TestDownloadNoDirectUri(test.NoDBTestCase): @mock.patch.object(six.moves.builtins, 'open') @mock.patch('nova.image.glance.GlanceImageServiceV2._get_transfer_module') @mock.patch('nova.image.glance.GlanceImageServiceV2.show') - @mock.patch('os.fsync') + @mock.patch('nova.image.glance.GlanceImageServiceV2._safe_fsync') def test_download_direct_no_mod_fallback( self, fsync_mock, show_mock, get_tran_mock, open_mock): # Test that we fall back to downloading to the dst_path @@ -751,8 +749,7 @@ class TestDownloadNoDirectUri(test.NoDBTestCase): get_tran_mock.assert_called_once_with('file') client.call.assert_called_once_with(ctx, 2, 'data', mock.sentinel.image_id) - fsync_mock.assert_called_once_with( - open_mock.return_value.fileno.return_value) + fsync_mock.assert_called_once_with(writer) # NOTE(jaypipes): log messages call open() in part of the # download path, so here, we just check that the last open() # call was done for the dst_path file descriptor. @@ -827,7 +824,7 @@ class TestDownloadSignatureVerification(test.NoDBTestCase): @mock.patch('nova.image.glance.LOG') @mock.patch('nova.image.glance.GlanceImageServiceV2.show') @mock.patch('cursive.signature_utils.get_verifier') - @mock.patch('os.fsync') + @mock.patch('nova.image.glance.GlanceImageServiceV2._safe_fsync') def test_download_dst_path_signature_verification_v2(self, mock_fsync, mock_get_verifier, @@ -852,8 +849,7 @@ class TestDownloadSignatureVerification(test.NoDBTestCase): mock_log.info.assert_called_once_with(mock.ANY, mock.ANY) self.assertEqual(len(self.fake_img_data), mock_dest.write.call_count) self.assertTrue(mock_dest.close.called) - mock_fsync.assert_called_once_with( - mock_dest.fileno.return_value) + mock_fsync.assert_called_once_with(mock_dest) @mock.patch('nova.image.glance.LOG') @mock.patch('nova.image.glance.GlanceImageServiceV2.show') @@ -908,7 +904,7 @@ class TestDownloadSignatureVerification(test.NoDBTestCase): @mock.patch('cursive.signature_utils.get_verifier') @mock.patch('nova.image.glance.LOG') @mock.patch('nova.image.glance.GlanceImageServiceV2.show') - @mock.patch('os.fsync') + @mock.patch('nova.image.glance.GlanceImageServiceV2._safe_fsync') def test_download_dst_path_signature_fail_v2(self, mock_fsync, mock_show, mock_log, mock_get_verifier, @@ -925,8 +921,7 @@ class TestDownloadSignatureVerification(test.NoDBTestCase): data=None, dst_path=fake_path) mock_log.error.assert_called_once_with(mock.ANY, mock.ANY) mock_open.assert_called_once_with(fake_path, 'wb') - mock_fsync.assert_called_once_with( - mock_open.return_value.fileno.return_value) + mock_fsync.assert_called_once_with(mock_dest) mock_dest.truncate.assert_called_once_with(0) self.assertTrue(mock_dest.close.called) @@ -1709,3 +1704,43 @@ class TestTranslateToGlance(test.NoDBTestCase): nova_image_dict = self.fixture image_v2_dict = glance._translate_to_glance(nova_image_dict) self.assertEqual(expected_v2_image, image_v2_dict) + + +@mock.patch('stat.S_ISSOCK') +@mock.patch('stat.S_ISFIFO') +@mock.patch('os.fsync') +@mock.patch('os.fstat') +class TestSafeFSync(test.NoDBTestCase): + """Validate _safe_fsync.""" + @staticmethod + def common(mock_isfifo, isfifo, mock_issock, issock, mock_fstat): + """Execution & assertions common to all test cases.""" + fh = mock.Mock() + mock_isfifo.return_value = isfifo + mock_issock.return_value = issock + glance.GlanceImageServiceV2._safe_fsync(fh) + fh.fileno.assert_called_once_with() + mock_fstat.assert_called_once_with(fh.fileno.return_value) + mock_isfifo.assert_called_once_with(mock_fstat.return_value.st_mode) + # Condition short-circuits, so S_ISSOCK is only called if !S_ISFIFO + if isfifo: + mock_issock.assert_not_called() + else: + mock_issock.assert_called_once_with( + mock_fstat.return_value.st_mode) + return fh + + def test_fsync(self, mock_fstat, mock_fsync, mock_isfifo, mock_issock): + """Validate path where fsync is called.""" + fh = self.common(mock_isfifo, False, mock_issock, False, mock_fstat) + mock_fsync.assert_called_once_with(fh.fileno.return_value) + + def test_fifo(self, mock_fstat, mock_fsync, mock_isfifo, mock_issock): + """Validate fsync not called for pipe/fifo.""" + self.common(mock_isfifo, True, mock_issock, False, mock_fstat) + mock_fsync.assert_not_called() + + def test_sock(self, mock_fstat, mock_fsync, mock_isfifo, mock_issock): + """Validate fsync not called for socket.""" + self.common(mock_isfifo, False, mock_issock, True, mock_fstat) + mock_fsync.assert_not_called() -- cgit v1.2.1