diff --git a/cinder/image/image_utils.py b/cinder/image/image_utils.py index 19a967877..cb6bcb8ea 100644 --- a/cinder/image/image_utils.py +++ b/cinder/image/image_utils.py @@ -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): diff --git a/cinder/tests/unit/test_image_utils.py b/cinder/tests/unit/test_image_utils.py index 8164d5e2d..8bdf4b2e7 100644 --- a/cinder/tests/unit/test_image_utils.py +++ b/cinder/tests/unit/test_image_utils.py @@ -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', diff --git a/cinder/tests/unit/test_volume_cleanup.py b/cinder/tests/unit/test_volume_cleanup.py index 6e67e7b74..190e9ea15 100644 --- a/cinder/tests/unit/test_volume_cleanup.py +++ b/cinder/tests/unit/test_volume_cleanup.py @@ -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): diff --git a/cinder/tests/unit/volume/flows/test_create_volume_flow.py b/cinder/tests/unit/volume/flows/test_create_volume_flow.py index 83b0037d6..b70ae1c8a 100644 --- a/cinder/tests/unit/volume/flows/test_create_volume_flow.py +++ b/cinder/tests/unit/volume/flows/test_create_volume_flow.py @@ -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 diff --git a/cinder/volume/flows/manager/create_volume.py b/cinder/volume/flows/manager/create_volume.py index add697b93..5b3afe559 100644 --- a/cinder/volume/flows/manager/create_volume.py +++ b/cinder/volume/flows/manager/create_volume.py @@ -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) diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py index 4b082b1c0..e7adc95c4 100644 --- a/cinder/volume/manager.py +++ b/cinder/volume/manager.py @@ -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."),