From 22d8f1fcbf4325d79ac18adfe87198c785f1df52 Mon Sep 17 00:00:00 2001 From: Abhishek Kekane Date: Wed, 24 Jun 2020 19:44:54 +0000 Subject: [PATCH] Fix: Interrupted copy-image leaking data on subsequent operation If copying existing image in other stores fails while staging the data to staging directory due to power, network or any other reason. Then subsequent try may lead to data leaks in stores. To fix this, added check of the actual image size with the size of image file present in the staging area. If it does not match then delete the image file from staging area so that the entire image will be staged again. Change-Id: I44bfefb6eee421e18e5e95a0dafaef0ea4e170da Closes-Bug: #1885003 --- .../flows/_internal_plugins/copy_image.py | 21 ++++++- .../unit/async_/flows/test_copy_image.py | 62 +++++++++++++++++++ 2 files changed, 82 insertions(+), 1 deletion(-) diff --git a/glance/async_/flows/_internal_plugins/copy_image.py b/glance/async_/flows/_internal_plugins/copy_image.py index 2a8e7f2ec2..9ec0260de3 100644 --- a/glance/async_/flows/_internal_plugins/copy_image.py +++ b/glance/async_/flows/_internal_plugins/copy_image.py @@ -59,7 +59,26 @@ class _CopyImage(task.Task): self.image_id) if os.path.exists(file_path): - return file_path, 0 + # NOTE (abhishekk): If previous copy-image operation is failed + # due to power failure, network failure or any other reason and + # the image data here is partial then clear the staging area and + # re-stage the fresh image data. + # Ref: https://bugs.launchpad.net/glance/+bug/1885003 + size_in_staging = os.path.getsize(file_path) + if image.size == size_in_staging: + return file_path, 0 + else: + LOG.debug(("Found partial image data in staging " + "%(fn)s, deleting it to re-stage " + "again"), {'fn': file_path}) + try: + os.unlink(file_path) + except OSError as e: + LOG.error(_LE("Deletion of staged " + "image data from %(fn)s has failed because " + "[Errno %(en)d]"), {'fn': file_path, + 'en': e.errno}) + raise # At first search image in default_backend default_store = CONF.glance_store.default_backend diff --git a/glance/tests/unit/async_/flows/test_copy_image.py b/glance/tests/unit/async_/flows/test_copy_image.py index 251531a420..fa79053a9c 100644 --- a/glance/tests/unit/async_/flows/test_copy_image.py +++ b/glance/tests/unit/async_/flows/test_copy_image.py @@ -14,6 +14,7 @@ # under the License. import datetime +import os from unittest import mock import glance_store as store_api @@ -139,6 +140,67 @@ class TestCopyImageTask(test_utils.BaseTestCase): mock_store_api.assert_called_once_with( "os_glance_staging_store") + @mock.patch.object(os, 'unlink') + @mock.patch.object(os.path, 'getsize') + @mock.patch.object(os.path, 'exists') + @mock.patch.object(store_api, 'get_store_from_store_identifier') + def test_copy_image_to_staging_store_partial_data_exists( + self, mock_store_api, mock_exists, mock_getsize, mock_unlink): + mock_store_api.return_value = self.staging_store + mock_exists.return_value = True + mock_getsize.return_value = 3 + + copy_image_task = copy_image._CopyImage( + self.task.task_id, self.task_type, self.image_repo, + self.image_id) + with mock.patch.object(self.image_repo, 'get') as get_mock: + get_mock.return_value = mock.MagicMock( + image_id=self.images[0]['id'], + locations=self.images[0]['locations'], + status=self.images[0]['status'], + size=4 + ) + with mock.patch.object(store_api, 'get') as get_data: + get_data.return_value = (b"dddd", 4) + copy_image_task.execute() + mock_exists.assert_called_once() + mock_getsize.assert_called_once() + mock_unlink.assert_called_once() + self.staging_store.add.assert_called_once() + mock_store_api.assert_called_once_with( + "os_glance_staging_store") + + @mock.patch.object(os, 'unlink') + @mock.patch.object(os.path, 'getsize') + @mock.patch.object(os.path, 'exists') + @mock.patch.object(store_api, 'get_store_from_store_identifier') + def test_copy_image_to_staging_store_data_exists( + self, mock_store_api, mock_exists, mock_getsize, mock_unlink): + mock_store_api.return_value = self.staging_store + mock_exists.return_value = True + mock_getsize.return_value = 4 + + copy_image_task = copy_image._CopyImage( + self.task.task_id, self.task_type, self.image_repo, + self.image_id) + with mock.patch.object(self.image_repo, 'get') as get_mock: + get_mock.return_value = mock.MagicMock( + image_id=self.images[0]['id'], + locations=self.images[0]['locations'], + status=self.images[0]['status'], + size=4 + ) + copy_image_task.execute() + mock_exists.assert_called_once() + mock_store_api.assert_called_once_with( + "os_glance_staging_store") + mock_getsize.assert_called_once() + # As valid image data already exists in staging area + # it does not remove it and also does not download + # it again to staging area + mock_unlink.assert_not_called() + self.staging_store.add.assert_not_called() + @mock.patch.object(store_api, 'get_store_from_store_identifier') def test_copy_non_existing_image_to_staging_store_(self, mock_store_api): mock_store_api.return_value = self.staging_store