openstack/nova
Code Issues Proposed changes

fdatasync() downloaded images before use

Browse Source 
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
This commit is contained in:
Matthew Booth
2017-03-03 16:48:52 +00:00
parent dbe6d502b7
commit 1c657cda5a
2 changed files with 29 additions and 5 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
nova/image/glance.py
View File

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

28
nova/tests/unit/image/test_glance.py
View File

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