openstack/cinder
Code Issues Proposed changes

Clean up image tmp file if c-vol gets restarted

Browse Source 
When creating a volume from image, and then restarting
the cinder-volume process while the image is downloading,
cinder can't clean up the image tmp file under image
conversion dir.

This patch will clear all tmp files in init_host.

Change-Id: I58cf7c234e6d92c54ae3523a81bf4aefee039daf
Closes-Bug: #1497088
This commit is contained in:
wanghao 2015-09-18 17:49:14 +08:00 committed by wanghao
parent e90d1b01f6
commit def9485d8d
6 changed files with 101 additions and 12 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

27
cinder/image/image_utils.py
View File

@ -491,6 +491,29 @@ def create_temporary_file(*args, **kwargs):
return tmp
def cleanup_temporary_file(backend_name):
temp_dir = CONF.image_conversion_dir
if (not temp_dir or not os.path.exists(temp_dir)):
LOG.debug("Configuration image_conversion_dir is None or the path "
"doesn't exist.")
return
try:
# TODO(wanghao): Consider using os.scandir for better performance in
# future when cinder only supports Python version 3.5+.
files = os.listdir(CONF.image_conversion_dir)
# NOTE(wanghao): For multi-backend case, if one backend was slow
# starting but another backend is up and doing an image conversion,
# init_host should only clean the tmp files which belongs to its
# backend.
for tmp_file in files:
if tmp_file.endswith(backend_name):
path = os.path.join(temp_dir, tmp_file)
os.remove(path)
except OSError as e:
LOG.warning(_LW("Exception caught while clearing temporary image "
"files: %s"), e)
@contextlib.contextmanager
def temporary_file(*args, **kwargs):
tmp = None
@ -570,9 +593,9 @@ class TemporaryImages(object):
@classmethod
@contextlib.contextmanager
def fetch(cls, image_service, context, image_id):
def fetch(cls, image_service, context, image_id, suffix=''):
tmp_images = cls.for_image_service(image_service).temporary_images
with temporary_file() as tmp:
with temporary_file(suffix=suffix) as tmp:
fetch_verify_image(context, image_service, image_id, tmp)
user = context.user_id
if not tmp_images.get(user):

46
cinder/tests/unit/test_image_utils.py
View File

@ -1398,6 +1398,52 @@ class TestCreateTemporaryFile(test.TestCase):
mock_mkstemp.assert_called_once_with(dir=conv_dir)
mock_close.assert_called_once_with(fd)
@mock.patch('cinder.image.image_utils.os.remove')
@mock.patch('cinder.image.image_utils.os.path.join')
@mock.patch('cinder.image.image_utils.CONF')
@mock.patch('cinder.image.image_utils.os.listdir')
@mock.patch('cinder.image.image_utils.os.path.exists', return_value=True)
def test_cleanup_temporary_file(self, mock_path, mock_listdir, mock_conf,
mock_join, mock_remove):
mock_listdir.return_value = ['tmphost@backend1', 'tmphost@backend2']
conv_dir = mock.sentinel.image_conversion_dir
mock_conf.image_conversion_dir = conv_dir
mock_join.return_value = '/test/tmp/tmphost@backend1'
image_utils.cleanup_temporary_file('host@backend1')
mock_listdir.assert_called_once_with(conv_dir)
mock_remove.assert_called_once_with('/test/tmp/tmphost@backend1')
@mock.patch('cinder.image.image_utils.os.remove')
@mock.patch('cinder.image.image_utils.os.listdir')
@mock.patch('cinder.image.image_utils.CONF')
@mock.patch('cinder.image.image_utils.os.path.exists', return_value=False)
def test_cleanup_temporary_file_with_not_exist_path(self, mock_path,
mock_conf,
mock_listdir,
mock_remove):
conv_dir = mock.sentinel.image_conversion_dir
mock_conf.image_conversion_dir = conv_dir
image_utils.cleanup_temporary_file('host@backend1')
self.assertFalse(mock_listdir.called)
self.assertFalse(mock_remove.called)
@mock.patch('cinder.image.image_utils.os.remove')
@mock.patch('cinder.image.image_utils.os.path.join')
@mock.patch('cinder.image.image_utils.CONF')
@mock.patch('cinder.image.image_utils.os.listdir')
@mock.patch('cinder.image.image_utils.os.path.exists', return_value=True)
def test_cleanup_temporary_file_with_exception(self, mock_path,
mock_listdir, mock_conf,
mock_join, mock_remove):
mock_listdir.return_value = ['tmphost@backend1', 'tmphost@backend2']
conv_dir = mock.sentinel.image_conversion_dir
mock_conf.image_conversion_dir = conv_dir
mock_join.return_value = '/test/tmp/tmphost@backend1'
mock_remove.side_effect = OSError
image_utils.cleanup_temporary_file('host@backend1')
mock_listdir.assert_called_once_with(conv_dir)
mock_remove.assert_called_once_with('/test/tmp/tmphost@backend1')
class TestTemporaryFileContextManager(test.TestCase):
@mock.patch('cinder.image.image_utils.create_temporary_file',

13
cinder/tests/unit/test_volume_cleanup.py
View File

@ -77,7 +77,8 @@ class VolumeCleanupTestCase(base.BaseVolumeTestCase):
self.assertEqual("in-use", volume.status)
self._assert_workers_are_removed()
def test_init_host_clears_downloads(self):
@mock.patch('cinder.image.image_utils.cleanup_temporary_file')
def test_init_host_clears_downloads(self, mock_cleanup_tmp_file):
"""Test that init_host will unwedge a volume stuck in downloading."""
volume = tests_utils.create_volume(self.context, status='downloading',
size=0, host=CONF.host)
@ -91,11 +92,13 @@ class VolumeCleanupTestCase(base.BaseVolumeTestCase):
self.assertEqual(volume.id, mock_clear.call_args[0][1].id)
volume.refresh()
self.assertEqual("error", volume['status'])
mock_cleanup_tmp_file.assert_called_once_with(CONF.host)
self.volume.delete_volume(self.context, volume=volume)
self._assert_workers_are_removed()
def test_init_host_resumes_deletes(self):
@mock.patch('cinder.image.image_utils.cleanup_temporary_file')
def test_init_host_resumes_deletes(self, mock_cleanup_tmp_file):
"""init_host will resume deleting volume in deleting status."""
volume = tests_utils.create_volume(self.context, status='deleting',
size=0, host=CONF.host)
@ -108,9 +111,12 @@ class VolumeCleanupTestCase(base.BaseVolumeTestCase):
self.assertRaises(exception.VolumeNotFound, db.volume_get,
context.get_admin_context(), volume.id)
mock_cleanup_tmp_file.assert_called_once_with(CONF.host)
self._assert_workers_are_removed()
def test_create_volume_fails_with_creating_and_downloading_status(self):
@mock.patch('cinder.image.image_utils.cleanup_temporary_file')
def test_create_volume_fails_with_creating_and_downloading_status(
self, mock_cleanup_tmp_file):
"""Test init_host_with_service in case of volume.
While the status of volume is 'creating' or 'downloading',
@ -130,6 +136,7 @@ class VolumeCleanupTestCase(base.BaseVolumeTestCase):
self.assertEqual('error', volume['status'])
self.volume.delete_volume(self.context, volume)
self.assertTrue(mock_cleanup_tmp_file.called)
self._assert_workers_are_removed()
def test_create_snapshot_fails_with_creating_status(self):

18
cinder/tests/unit/volume/flows/test_create_volume_flow.py
View File

@ -767,7 +767,8 @@ class CreateVolumeFlowManagerTestCase(test.TestCase):
fake_volume_manager, fake_db, fake_driver)
volume = fake_volume.fake_volume_obj(
self.ctxt,
encryption_key_id=fakes.ENCRYPTION_KEY_ID)
encryption_key_id=fakes.ENCRYPTION_KEY_ID,
host='host@backend#pool')
fake_image_service = fake_image.FakeImageService()
image_meta = {}
@ -837,7 +838,8 @@ class CreateVolumeFlowManagerGlanceCinderBackendCase(test.TestCase):
fake_manager = create_volume_manager.CreateVolumeFromSpecTask(
mock.MagicMock(), fake_db, fake_driver)
fake_image_service = mock.MagicMock()
volume = fake_volume.fake_volume_obj(self.ctxt)
volume = fake_volume.fake_volume_obj(self.ctxt,
host='host@backend#pool')
image_volume = fake_volume.fake_volume_obj(self.ctxt,
volume_metadata={})
image_id = fakes.IMAGE_ID
@ -914,7 +916,8 @@ class CreateVolumeFlowManagerImageCacheTestCase(test.TestCase):
self, mock_get_internal_context, mock_create_from_img_dl,
mock_create_from_src, mock_handle_bootable, mock_fetch_img):
self.mock_driver.clone_image.return_value = (None, True)
volume = fake_volume.fake_volume_obj(self.ctxt)
volume = fake_volume.fake_volume_obj(self.ctxt,
host='host@backend#pool')
image_location = 'someImageLocationStr'
image_id = fakes.IMAGE_ID
@ -957,7 +960,8 @@ class CreateVolumeFlowManagerImageCacheTestCase(test.TestCase):
mock_handle_bootable, mock_fetch_img):
mock_get_internal_context.return_value = None
self.mock_driver.clone_image.return_value = (None, False)
volume = fake_volume.fake_volume_obj(self.ctxt)
volume = fake_volume.fake_volume_obj(self.ctxt,
host='host@backend#pool')
image_info = imageutils.QemuImgInfo()
image_info.virtual_size = '1073741824'
mock_qemu_info.return_value = image_info
@ -1076,7 +1080,8 @@ class CreateVolumeFlowManagerImageCacheTestCase(test.TestCase):
'volume_id': image_volume_id
}
volume = fake_volume.fake_volume_obj(self.ctxt)
volume = fake_volume.fake_volume_obj(self.ctxt,
host='host@backend#pool')
image_location = 'someImageLocationStr'
image_id = fakes.IMAGE_ID
@ -1254,7 +1259,8 @@ class CreateVolumeFlowManagerImageCacheTestCase(test.TestCase):
mock_handle_bootable, mock_fetch_img):
self.mock_driver.clone_image.return_value = (None, False)
mock_get_internal_context.return_value = None
volume = fake_volume.fake_volume_obj(self.ctxt)
volume = fake_volume.fake_volume_obj(self.ctxt,
host='host@backend#pool')
image_info = imageutils.QemuImgInfo()
image_info.virtual_size = '1073741824'
mock_qemu_info.return_value = image_info

4
cinder/volume/flows/manager/create_volume.py
View File

@ -766,10 +766,12 @@ class CreateVolumeFromSpecTask(flow_utils.CinderTask):
# Fall back to default behavior of creating volume,
# download the image data and copy it into the volume.
original_size = volume.size
backend_name = volume_utils.extract_host(volume.service_topic_queue)
try:
if not cloned:
with image_utils.TemporaryImages.fetch(
image_service, context, image_id) as tmp_image:
image_service, context, image_id,
backend_name) as tmp_image:
# Try to create the volume as the minimal size, then we can
# extend once the image has been downloaded.
data = image_utils.qemu_img_info(tmp_image)

5
cinder/volume/manager.py
View File

@ -63,6 +63,7 @@ from cinder import keymgr as key_manager
from cinder.i18n import _, _LE, _LI, _LW
from cinder.image import cache as image_cache
from cinder.image import glance
from cinder.image import image_utils
from cinder import manager
from cinder.message import api as message_api
from cinder.message import defined_messages
@ -465,6 +466,10 @@ class VolumeManager(manager.CleanableManager,
# that an entry exists in the service table
self.driver.set_initialized()
# Keep the image tmp file clean when init host.
backend_name = vol_utils.extract_host(self.service_topic_queue)
image_utils.cleanup_temporary_file(backend_name)
# collect and publish service capabilities
self.publish_service_capabilities(ctxt)
LOG.info(_LI("Driver initialization completed successfully."),