From 1c657cda5a9abd3d3d0edd8c3144ebb6aed11c0f Mon Sep 17 00:00:00 2001 From: Matthew Booth Date: Fri, 3 Mar 2017 16:48:52 +0000 Subject: [PATCH] fdatasync() downloaded images before use Image download closes the filehandle of the downloaded image before use, but doesn't fdatasync it. This means that in the event of a host crash we may be left with only the file metadata when the host comes back up: i.e. an empty file of the correct size. Nova cannot detect this data corruption, so operator intervention is required. By adding an fdatasync() before closing the file handle we ensure that the downloaded file is either entirely present, or entirely not present. See also change I33bd99b0, which fixes this issue for downloads requiring a subsequent conversion step. Partial-Bug: #1669844 Change-Id: Id9905a87f16f66530623800e33e2581c555ae81d --- nova/image/glance.py | 6 ++++++ nova/tests/unit/image/test_glance.py | 28 +++++++++++++++++++++++----- 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/nova/image/glance.py b/nova/image/glance.py index b727c3fa18e2..080efd4f2a86 100644 --- a/nova/image/glance.py +++ b/nova/image/glance.py @@ -20,6 +20,7 @@ from __future__ import absolute_import import copy import inspect import itertools +import os import random import sys import time @@ -366,6 +367,11 @@ class GlanceImageServiceV2(object): {'path': dst_path, 'exception': ex}) finally: if close_file: + # Ensure that the data is pushed all the way down to + # persistent storage. This ensures that in the event of a + # subsequent host crash we don't have running instances + # using a corrupt backing file. + os.fdatasync(data.fileno()) 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 c2417a0fac62..be7b95923758 100644 --- a/nova/tests/unit/image/test_glance.py +++ b/nova/tests/unit/image/test_glance.py @@ -565,7 +565,9 @@ class TestDownloadNoDirectUri(test.NoDBTestCase): @mock.patch.object(six.moves.builtins, 'open') @mock.patch('nova.image.glance.GlanceImageServiceV2.show') - def test_download_no_data_dest_path_v2(self, show_mock, open_mock): + @mock.patch('os.fdatasync') + def test_download_no_data_dest_path_v2(self, fdatasync_mock, show_mock, + open_mock): client = mock.MagicMock() client.call.return_value = [1, 2, 3] ctx = mock.sentinel.ctx @@ -579,6 +581,8 @@ 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') + fdatasync_mock.assert_called_once_with( + writer.fileno.return_value) self.assertIsNone(res) writer.write.assert_has_calls( [ @@ -672,8 +676,9 @@ 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.fdatasync') def test_download_direct_exception_fallback_v2( - self, show_mock, get_tran_mock, open_mock): + self, fdatasync_mock, show_mock, get_tran_mock, open_mock): # Test that we fall back to downloading to the dst_path # if the download method of the transfer module raised # an exception. @@ -708,6 +713,8 @@ class TestDownloadNoDirectUri(test.NoDBTestCase): mock.sentinel.loc_meta) client.call.assert_called_once_with(ctx, 2, 'data', mock.sentinel.image_id) + fdatasync_mock.assert_called_once_with( + open_mock.return_value.fileno.return_value) # 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. @@ -724,8 +731,9 @@ 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.fdatasync') def test_download_direct_no_mod_fallback( - self, show_mock, get_tran_mock, open_mock): + self, fdatasync_mock, show_mock, get_tran_mock, open_mock): # Test that we fall back to downloading to the dst_path # if no appropriate transfer module is found... # an exception. @@ -755,6 +763,8 @@ 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) + fdatasync_mock.assert_called_once_with( + open_mock.return_value.fileno.return_value) # 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. @@ -826,7 +836,9 @@ class TestDownloadSignatureVerification(test.NoDBTestCase): @mock.patch('nova.image.glance.LOG') @mock.patch('nova.image.glance.GlanceImageServiceV2.show') @mock.patch('nova.signature_utils.get_verifier') + @mock.patch('os.fdatasync') def test_download_dst_path_signature_verification_v2(self, + mock_fdatasync, mock_get_verifier, mock_show, mock_log, @@ -846,6 +858,8 @@ 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_fdatasync.assert_called_once_with( + mock_dest.fileno.return_value) @mock.patch('nova.image.glance.LOG') @mock.patch('nova.image.glance.GlanceImageServiceV2.show') @@ -901,8 +915,10 @@ class TestDownloadSignatureVerification(test.NoDBTestCase): @mock.patch('nova.signature_utils.get_verifier') @mock.patch('nova.image.glance.LOG') @mock.patch('nova.image.glance.GlanceImageServiceV2.show') - def test_download_dst_path_signature_fail_v2(self, mock_show, - mock_log, mock_get_verifier, + @mock.patch('os.fdatasync') + def test_download_dst_path_signature_fail_v2(self, mock_fdatasync, + mock_show, mock_log, + mock_get_verifier, mock_open): service = glance.GlanceImageServiceV2(self.client) mock_get_verifier.return_value = self.BadVerifier() @@ -916,6 +932,8 @@ 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_fdatasync.assert_called_once_with( + mock_open.return_value.fileno.return_value) mock_dest.truncate.assert_called_once_with(0) self.assertTrue(mock_dest.close.called)