openstack/ironic
Code Issues Proposed changes

Use temporary dir for image conversion

Browse Source 
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
This commit is contained in:
Yuriy Zveryanskyy 2014-06-06 11:55:02 +03:00
parent 76b2244c5e
commit 7ba64cfc62
2 changed files with 40 additions and 10 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

20
ironic/drivers/modules/image_cache.py
View File

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

30
ironic/tests/drivers/test_image_cache.py
View File

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