From 7c95f3969f4c0df5f818f96cb662dddc17a8dd6f Mon Sep 17 00:00:00 2001 From: Rajat Dhasmana Date: Mon, 20 Apr 2020 09:00:39 +0000 Subject: [PATCH] RBD: Cleanup temporary file during exception During copy image to encrypted volume operation, we create a temporary image file to convert source image into an encrypted image and rename it to source image name. If any error occurs during this process, there is no code to cleanup the temporary '.luks' file. This patch adds the following cleanup. Change-Id: I05dfc996a635299990fdafba841ab6c4fd7d32c7 Closes-Bug: 1873738 --- cinder/image/image_utils.py | 4 ++ cinder/tests/unit/volume/drivers/test_rbd.py | 38 +++++++++++++++++++ cinder/volume/drivers/rbd.py | 15 +++++--- ...-during-convert-fail-3848e9dbe7e15fc6.yaml | 6 +++ 4 files changed, 57 insertions(+), 6 deletions(-) create mode 100644 releasenotes/notes/cleanup-rbd-temp-file-during-convert-fail-3848e9dbe7e15fc6.yaml diff --git a/cinder/image/image_utils.py b/cinder/image/image_utils.py index 8828169a1b7..c472c1476e5 100644 --- a/cinder/image/image_utils.py +++ b/cinder/image/image_utils.py @@ -243,6 +243,10 @@ def _convert_image(prefix, source, dest, out_format, passphrase_file=None, compress=False): """Convert image to other format. + NOTE: If the qemu-img convert command fails and this function raises an + exception, a non-empty dest file may be left in the filesystem. + It is the responsibility of the caller to decide what to do with this file. + :param prefix: command prefix, i.e. cgexec for throttling :param source: source filename :param dest: destination filename diff --git a/cinder/tests/unit/volume/drivers/test_rbd.py b/cinder/tests/unit/volume/drivers/test_rbd.py index 15b36c4ed9b..49109beda4e 100644 --- a/cinder/tests/unit/volume/drivers/test_rbd.py +++ b/cinder/tests/unit/volume/drivers/test_rbd.py @@ -1309,6 +1309,39 @@ class RBDTestCase(test.TestCase): mock_image_service, None] self.driver.copy_image_to_volume(*args) + @mock.patch('cinder.volume.drivers.rbd.fileutils.delete_if_exists') + @mock.patch('cinder.volume.volume_utils.check_encryption_provider', + return_value={'encryption_key_id': fake.ENCRYPTION_KEY_ID}) + @mock.patch('cinder.image.image_utils.convert_image') + def _copy_image_encrypted(self, mock_convert, mock_encrypt_key, + mock_temp_delete): + key_mgr = fake_keymgr.fake_api() + self.mock_object(castellan.key_manager, 'API', return_value=key_mgr) + key_id = key_mgr.store(self.context, KeyObject()) + self.volume_a.encryption_key_id = key_id + + enc_info = {'encryption_key_id': key_id, + 'cipher': 'aes-xts-essiv', + 'key_size': 256} + with mock.patch('cinder.volume.volume_utils.check_encryption_provider', + return_value=enc_info), \ + mock.patch('cinder.volume.drivers.rbd.open'), \ + mock.patch('os.rename'): + with mock.patch.object(tempfile, 'NamedTemporaryFile'): + with mock.patch.object(os.path, 'exists') as mock_exists: + mock_exists.return_value = True + with mock.patch.object(image_utils, 'fetch_to_raw'): + with mock.patch.object(self.driver, 'delete_volume'): + with mock.patch.object(self.driver, '_resize'): + mock_image_service = mock.MagicMock() + args = [self.context, self.volume_a, + mock_image_service, None] + self.driver.copy_image_to_encrypted_volume( + *args) + mock_temp_delete.assert_called() + self.assertEqual(1, + mock_temp_delete.call_count) + @common_mocks def test_copy_image_no_volume_tmp(self): self.cfg.image_conversion_dir = None @@ -1319,6 +1352,11 @@ class RBDTestCase(test.TestCase): self.cfg.image_conversion_dir = '/var/run/cinder/tmp' self._copy_image() + @common_mocks + def test_copy_image_volume_tmp_encrypted(self): + self.cfg.image_conversion_dir = '/var/run/cinder/tmp' + self._copy_image_encrypted() + @ddt.data(True, False) @common_mocks @mock.patch('cinder.volume.drivers.rbd.RBDDriver._get_usage_info') diff --git a/cinder/volume/drivers/rbd.py b/cinder/volume/drivers/rbd.py index dc47034541d..02b2d3b3c84 100644 --- a/cinder/volume/drivers/rbd.py +++ b/cinder/volume/drivers/rbd.py @@ -1580,13 +1580,16 @@ class RBDDriver(driver.CloneableImageVD, driver.MigrateVD, # Convert the raw image to luks dest_image_path = src_image_path + '.luks' - image_utils.convert_image(src_image_path, dest_image_path, - 'luks', src_format='raw', - cipher_spec=cipher_spec, - passphrase_file=pass_file.name) + try: + image_utils.convert_image(src_image_path, dest_image_path, + 'luks', src_format='raw', + cipher_spec=cipher_spec, + passphrase_file=pass_file.name) - # Replace the original image with the now encrypted image - os.rename(dest_image_path, src_image_path) + # Replace the original image with the now encrypted image + os.rename(dest_image_path, src_image_path) + finally: + fileutils.delete_if_exists(dest_image_path) def _copy_image_to_volume(self, context, volume, image_service, image_id, encrypted=False): diff --git a/releasenotes/notes/cleanup-rbd-temp-file-during-convert-fail-3848e9dbe7e15fc6.yaml b/releasenotes/notes/cleanup-rbd-temp-file-during-convert-fail-3848e9dbe7e15fc6.yaml new file mode 100644 index 00000000000..de9a4868ca1 --- /dev/null +++ b/releasenotes/notes/cleanup-rbd-temp-file-during-convert-fail-3848e9dbe7e15fc6.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + `Bug #1873738 `_: RBD Driver: + Added cleanup for residue destination file if the copy image to encrypted volume + operation fails. \ No newline at end of file