From 0fdf2e141ccf04bdf23a2ec099b7826a14b7bf9c Mon Sep 17 00:00:00 2001 From: Eric Harney Date: Wed, 4 Sep 2024 14:53:10 +0000 Subject: [PATCH] Optimize rbd upload volume to image Open the source volume directly rather than using a temporary file. Closes-Bug: #2080060 Change-Id: I5415be9ad70a0d622dcb6fcb292e7897a3195a6c --- cinder/image/image_utils.py | 19 ++++++++---- cinder/tests/unit/test_image_utils.py | 31 +++++++++++++++++++ .../tests/unit/volume/drivers/test_quobyte.py | 7 +++-- cinder/tests/unit/volume/drivers/test_rbd.py | 19 ++++++++++++ cinder/tests/unit/windows/test_iscsi.py | 3 +- cinder/tests/unit/windows/test_smbfs.py | 3 +- cinder/volume/drivers/rbd.py | 24 +++++++------- cinder/volume/volume_utils.py | 6 ++-- ...ptimize-image-upload-836c9df06674a665.yaml | 5 +++ 9 files changed, 91 insertions(+), 26 deletions(-) create mode 100644 releasenotes/notes/rbd-optimize-image-upload-836c9df06674a665.yaml diff --git a/cinder/image/image_utils.py b/cinder/image/image_utils.py index 65141ee1dbe..3d851f56d26 100644 --- a/cinder/image/image_utils.py +++ b/cinder/image/image_utils.py @@ -1086,6 +1086,7 @@ def upload_volume(context: context.RequestContext, image_service: glance.GlanceImageService, image_meta: dict, volume_path: str, + volume_fd = None, volume_format: str = 'raw', run_as_root: bool = True, compress: bool = True, @@ -1102,12 +1103,18 @@ def upload_volume(context: context.RequestContext, if (image_meta['disk_format'] == volume_format): LOG.debug("%s was %s, no need to convert to %s", image_id, volume_format, image_meta['disk_format']) - with chown_if_needed(volume_path): - with open(volume_path, 'rb') as image_file: - image_service.update(context, image_id, {}, - tpool.Proxy(image_file), - store_id=store_id, - base_image_ref=base_image_ref) + if volume_fd is not None: + image_service.update(context, image_id, {}, + volume_fd, + store_id=store_id, + base_image_ref=base_image_ref) + else: + with chown_if_needed(volume_path): + with open(volume_path, 'rb') as image_file: + image_service.update(context, image_id, {}, + tpool.Proxy(image_file), + store_id=store_id, + base_image_ref=base_image_ref) return with temporary_file(prefix='vol_upload_') as tmp: diff --git a/cinder/tests/unit/test_image_utils.py b/cinder/tests/unit/test_image_utils.py index 2918cf852a9..ce9b84b7c49 100644 --- a/cinder/tests/unit/test_image_utils.py +++ b/cinder/tests/unit/test_image_utils.py @@ -937,6 +937,37 @@ class TestUploadVolume(test.TestCase): ctxt, image_meta['id'], {}, mock_proxy.return_value, store_id=None, base_image_ref=None) + @mock.patch('eventlet.tpool.Proxy') + @mock.patch('cinder.image.image_utils.utils.temporary_chown') + @mock.patch('cinder.image.image_utils.open', new_callable=mock.mock_open) + @mock.patch('cinder.image.image_utils.qemu_img_info') + @mock.patch('cinder.image.image_utils.convert_image') + @mock.patch('cinder.image.image_utils.temporary_file') + @mock.patch('cinder.image.image_utils.os') + def test_same_format_fd(self, mock_os, mock_temp, mock_convert, mock_info, + mock_open, mock_chown, mock_proxy): + ctxt = mock.sentinel.context + image_service = mock.Mock() + image_meta = {'id': 'test_id', + 'disk_format': 'raw', + 'container_format': mock.sentinel.container_format} + mock_os.name = 'posix' + mock_os.access.return_value = False + + output = image_utils.upload_volume(ctxt, image_service, image_meta, + None, + volume_fd=mock.sentinel.volume_fd) + + self.assertIsNone(output) + self.assertFalse(mock_convert.called) + self.assertFalse(mock_info.called) + mock_chown.assert_not_called() + mock_open.assert_not_called() + mock_proxy.assert_not_called() + image_service.update.assert_called_once_with( + ctxt, image_meta['id'], {}, mock.sentinel.volume_fd, + store_id=None, base_image_ref=None) + @mock.patch('cinder.image.accelerator.ImageAccel._get_engine') @mock.patch('cinder.image.accelerator.ImageAccel.is_engine_ready', return_value = True) diff --git a/cinder/tests/unit/volume/drivers/test_quobyte.py b/cinder/tests/unit/volume/drivers/test_quobyte.py index fa6a91e5b0e..fe422fc23be 100644 --- a/cinder/tests/unit/volume/drivers/test_quobyte.py +++ b/cinder/tests/unit/volume/drivers/test_quobyte.py @@ -1335,7 +1335,7 @@ class QuobyteDriverTestCase(test.TestCase): mock_upload_volume.assert_called_once_with( mock.ANY, mock.ANY, mock.ANY, upload_path, run_as_root=False, store_id=None, base_image_ref=None, compress=True, - volume_format='raw') + volume_format='raw', volume_fd=None) self.assertTrue(mock_create_temporary_file.called) @mock.patch('cinder.db.volume_glance_metadata_get', return_value={}) @@ -1392,7 +1392,7 @@ class QuobyteDriverTestCase(test.TestCase): mock_upload_volume.assert_called_once_with( mock.ANY, mock.ANY, mock.ANY, upload_path, run_as_root=False, store_id=None, base_image_ref=None, compress=True, - volume_format='raw') + volume_format='raw', volume_fd=None) self.assertTrue(mock_create_temporary_file.called) @mock.patch('cinder.db.volume_glance_metadata_get', return_value={}) @@ -1449,7 +1449,8 @@ class QuobyteDriverTestCase(test.TestCase): mock_convert_image.assert_called_once_with( volume_path, upload_path, 'raw', run_as_root=False) mock_upload_volume.assert_called_once_with( - mock.ANY, mock.ANY, mock.ANY, upload_path, run_as_root=False, + mock.ANY, mock.ANY, mock.ANY, upload_path, volume_fd=None, + run_as_root=False, store_id=None, base_image_ref=None, compress=True, volume_format='raw') self.assertTrue(mock_create_temporary_file.called) diff --git a/cinder/tests/unit/volume/drivers/test_rbd.py b/cinder/tests/unit/volume/drivers/test_rbd.py index 9952f8a722d..337267b524d 100644 --- a/cinder/tests/unit/volume/drivers/test_rbd.py +++ b/cinder/tests/unit/volume/drivers/test_rbd.py @@ -3475,6 +3475,25 @@ class RBDTestCase(test.TestCase): self.assertEqual({'provider_location': None}, ret) + @common_mocks + def test_copy_volume_to_image(self): + mock_uv = self.mock_object(cinder.volume.volume_utils, 'upload_volume') + mock_get_rbd_handle = self.mock_object( + self.driver, '_get_rbd_handle', + return_value=mock.sentinel.rbd_handle) + + self.driver.copy_volume_to_image(mock.sentinel.context, + mock.sentinel.volume, + mock.sentinel.image_service, + mock.sentinel.image_meta) + mock_get_rbd_handle.assert_called_once_with(mock.sentinel.volume) + mock_uv.assert_called_once_with(mock.sentinel.context, + mock.sentinel.image_service, + mock.sentinel.image_meta, + None, + mock.sentinel.volume, + volume_fd=mock.sentinel.rbd_handle) + class ManagedRBDTestCase(test_driver.BaseDriverTestCase): driver_name = "cinder.volume.drivers.rbd.RBDDriver" diff --git a/cinder/tests/unit/windows/test_iscsi.py b/cinder/tests/unit/windows/test_iscsi.py index 8f4e764902e..0b3c4bc9bc7 100644 --- a/cinder/tests/unit/windows/test_iscsi.py +++ b/cinder/tests/unit/windows/test_iscsi.py @@ -416,7 +416,8 @@ class TestWindowsISCSIDriver(test.TestCase): expected_tmp_vhd_path) mock_upload_volume.assert_called_once_with( mock.sentinel.context, mock.sentinel.image_service, - fake_image_meta, expected_tmp_vhd_path, volume_format='vhd', + fake_image_meta, expected_tmp_vhd_path, volume_fd=None, + volume_format='vhd', store_id='fake-store', base_image_ref=None, compress=True, run_as_root=True) mock_delete_if_exists.assert_called_once_with( diff --git a/cinder/tests/unit/windows/test_smbfs.py b/cinder/tests/unit/windows/test_smbfs.py index ead798c9a4e..70b0dcb0571 100644 --- a/cinder/tests/unit/windows/test_smbfs.py +++ b/cinder/tests/unit/windows/test_smbfs.py @@ -789,7 +789,8 @@ class WindowsSmbFsTestCase(test.TestCase): fake_upload_volume.assert_called_once_with( mock.sentinel.context, mock.sentinel.image_service, - fake_image_meta, upload_path, volume_format=fake_img_format, + fake_image_meta, upload_path, volume_fd=None, + volume_format=fake_img_format, store_id='fake-store', base_image_ref=None, compress=True, run_as_root=True) diff --git a/cinder/volume/drivers/rbd.py b/cinder/volume/drivers/rbd.py index 39db852d507..612913907cf 100644 --- a/cinder/volume/drivers/rbd.py +++ b/cinder/volume/drivers/rbd.py @@ -2081,20 +2081,18 @@ class RBDDriver(driver.CloneableImageVD, driver.MigrateVD, raise exception.ReplicationError(reason=err_msg, volume_id=volume.id) + def _get_rbd_handle(self, volume: Volume): + conn = self.initialize_connection(volume, {}) + + connector = volume_utils.brick_get_connector('rbd') + + return connector._get_rbd_handle(conn['data']) + def copy_volume_to_image(self, context, volume, image_service, image_meta): - tmp_dir = volume_utils.image_conversion_dir() - tmp_file = os.path.join(tmp_dir, - volume.name + '-' + image_meta['id']) - with fileutils.remove_path_on_error(tmp_file): - args = ['rbd', 'export', - '--pool', self.configuration.rbd_pool, - volume.name, tmp_file] - args.extend(self._ceph_args()) - self._try_execute(*args) - volume_utils.upload_volume(context, image_service, - image_meta, tmp_file, - volume) - os.unlink(tmp_file) + source_handle = self._get_rbd_handle(volume) + + volume_utils.upload_volume(context, image_service, image_meta, None, + volume, volume_fd=source_handle) def extend_volume(self, volume: Volume, new_size: str) -> None: """Extend an existing volume.""" diff --git a/cinder/volume/volume_utils.py b/cinder/volume/volume_utils.py index 8d6a015328d..671476008bc 100644 --- a/cinder/volume/volume_utils.py +++ b/cinder/volume/volume_utils.py @@ -1338,7 +1338,8 @@ def upload_volume(context: context.RequestContext, volume: 'objects.Volume', volume_format: str = 'raw', run_as_root: bool = True, - compress: bool = True) -> None: + compress: bool = True, + volume_fd = None) -> None: # retrieve store information from extra-specs store_id = volume.volume_type.extra_specs.get('image_service:store_id') @@ -1351,7 +1352,8 @@ def upload_volume(context: context.RequestContext, volume_format=volume_format, run_as_root=run_as_root, compress=compress, store_id=store_id, - base_image_ref=base_image_ref) + base_image_ref=base_image_ref, + volume_fd=volume_fd) def get_backend_configuration(backend_name, backend_opts=None): diff --git a/releasenotes/notes/rbd-optimize-image-upload-836c9df06674a665.yaml b/releasenotes/notes/rbd-optimize-image-upload-836c9df06674a665.yaml new file mode 100644 index 00000000000..c390d2b95fe --- /dev/null +++ b/releasenotes/notes/rbd-optimize-image-upload-836c9df06674a665.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - | + RBD driver: No longer copy the RBD source volume image to a temporary + file when uploading a volume to an image.