From 7ba64cfc62cae8e312ae731379e601a6d148a846 Mon Sep 17 00:00:00 2001 From: Yuriy Zveryanskyy Date: Fri, 6 Jun 2014 11:55:02 +0300 Subject: [PATCH] Use temporary dir for image conversion Image cache uses master_dir as temporary directory for images conversion (pass tmp_path to fetch_to_raw()). This can cause a problem because links created after conversion and working files can be candidates for deletion. This patch uses own temporary directory for image conversion. Closes-bug: #1326556 Change-Id: I0ad1aeba95a177a5ea6a1d0a09ea37c5ff2b0837 --- ironic/drivers/modules/image_cache.py | 20 +++++++++------- ironic/tests/drivers/test_image_cache.py | 30 +++++++++++++++++++++++- 2 files changed, 40 insertions(+), 10 deletions(-) diff --git a/ironic/drivers/modules/image_cache.py b/ironic/drivers/modules/image_cache.py index 0d525ebcf6..b4fd0b8c4f 100644 --- a/ironic/drivers/modules/image_cache.py +++ b/ironic/drivers/modules/image_cache.py @@ -133,15 +133,17 @@ class ImageCache(object): """ #TODO(ghe): timeout and retry for downloads #TODO(ghe): logging when image cannot be created - fd, tmp_path = tempfile.mkstemp(dir=self.master_dir) - os.close(fd) - images.fetch_to_raw(ctx, uuid, tmp_path, - self._image_service) - # NOTE(dtantsur): no need for global lock here - master_path - # will have link count >1 at any moment, so won't be cleaned up - os.link(tmp_path, master_path) - os.link(master_path, dest_path) - os.unlink(tmp_path) + tmp_dir = tempfile.mkdtemp(dir=self.master_dir) + tmp_path = os.path.join(tmp_dir, uuid) + try: + images.fetch_to_raw(ctx, uuid, tmp_path, + self._image_service) + # NOTE(dtantsur): no need for global lock here - master_path + # will have link count >1 at any moment, so won't be cleaned up + os.link(tmp_path, master_path) + os.link(master_path, dest_path) + finally: + utils.rmtree_without_raise(tmp_dir) @lockutils.synchronized('master_image', 'ironic-') def clean_up(self): diff --git a/ironic/tests/drivers/test_image_cache.py b/ironic/tests/drivers/test_image_cache.py index 581606c553..783361818b 100644 --- a/ironic/tests/drivers/test_image_cache.py +++ b/ironic/tests/drivers/test_image_cache.py @@ -21,7 +21,9 @@ import os import tempfile import time +from ironic.common import exception from ironic.common import images +from ironic.common import utils from ironic.drivers.modules import image_cache from ironic.tests import base @@ -89,8 +91,8 @@ class TestImageCacheFetch(base.TestCase): def test__download_image(self, mock_fetch_to_raw): def _fake_fetch_to_raw(ctx, uuid, tmp_path, *args): self.assertEqual(self.uuid, uuid) - self.assertTrue(os.path.isfile(tmp_path)) self.assertNotEqual(self.dest_path, tmp_path) + self.assertNotEqual(os.path.dirname(tmp_path), self.master_dir) with open(tmp_path, 'w') as fp: fp.write("TEST") @@ -191,3 +193,29 @@ class TestImageCacheCleanUp(base.TestCase): for filename in files: self.assertTrue(os.path.exists(filename)) self.assertTrue(mock_log.called) + + @mock.patch.object(utils, 'rmtree_without_raise') + @mock.patch.object(images, 'fetch_to_raw') + def test_temp_images_not_cleaned(self, mock_fetch_to_raw, mock_rmtree): + def _fake_fetch_to_raw(ctx, uuid, tmp_path, *args): + with open(tmp_path, 'w') as fp: + fp.write("TEST" * 10) + + # assume cleanup from another thread at this moment + self.cache.clean_up() + self.assertTrue(os.path.exists(tmp_path)) + + mock_fetch_to_raw.side_effect = _fake_fetch_to_raw + master_path = os.path.join(self.master_dir, 'uuid') + dest_path = os.path.join(tempfile.mkdtemp(), 'dest') + self.cache._download_image('uuid', master_path, dest_path) + self.assertTrue(mock_rmtree.called) + + @mock.patch.object(utils, 'rmtree_without_raise') + @mock.patch.object(images, 'fetch_to_raw') + def test_temp_dir_exception(self, mock_fetch_to_raw, mock_rmtree): + mock_fetch_to_raw.side_effect = exception.IronicException + self.assertRaises(exception.IronicException, + self.cache._download_image, + 'uuid', 'fake', 'fake') + self.assertTrue(mock_rmtree.called)