From bb8ad77e60098c49f2937b13463e695103587e1c Mon Sep 17 00:00:00 2001 From: Rajat Dhasmana Date: Thu, 26 Dec 2024 20:20:19 +0530 Subject: [PATCH] RBD: Fix upload volume with different format With change[1], we introduced an optimized path to upload an RBD volume to image. However, this does not work well when the image format is not 'raw' and conversion is required. Currently there are 2 places where we need handling of the RBDVolumeIOWrapper object: 1. format inspector 2. qemu-img commands While testing, I found out the following issues that needs to be addressed to fix the upload path with conversion: 1. Passing RBDVolumeIOWrapper to format inspector We fail when calling privsep since RPC cannot serialize the RBDVolumeIOWrapper object and fails with the following error ERROR oslo_messaging.rpc.server TypeError: can not serialize 'RBDVolumeIOWrapper' object 2. Handling in format inspector Either we need to pass the volume_fd var to several methods in format inspector code or introduce a condition to find out if we get volume path or RBDVolumeIOWrapper and open it if it's a path 3. Handling in qemu-img commands We need to use rbd:{pool-name}/{image-name} format instead of file path when executing qemu-img commands like convert[2] otherwise we will fail with the below error. Stderr: "qemu-img: Could not open '': Could not open '': No such file or directory\n" Not to mention passing the volume_fd object to all the qemu related methods and handling cases when volume_path is none and volume_fd contains the RBD IO object. Even after fixing all the issues, it's still not sure that the optimized path offers better performance compared to the traditional workflow as qemu-img will use librbd to read the volume file and convert it to a local path. Due to the above reasons, we only use the optimized path when the image format is same as volume format (i.e. raw) and container format is not 'compressed'. [1] https://review.opendev.org/c/openstack/cinder/+/928024 [2] https://docs.ceph.com/en/latest/rbd/qemu-rbd/#running-qemu-with-rbd Closes-Bug: #2092534 Change-Id: I32b505aa69c71b62e7e3a52d65d38165d34e97d8 (cherry picked from commit 7d320764980a0e33a9abad14db41a4cdee71d57a) --- cinder/tests/unit/volume/drivers/test_rbd.py | 57 +++++++++++++++---- cinder/volume/drivers/rbd.py | 37 +++++++++++- ...d-upload-diff-format-38fc4ef24d7145ba.yaml | 7 +++ 3 files changed, 86 insertions(+), 15 deletions(-) create mode 100644 releasenotes/notes/fix-rbd-upload-diff-format-38fc4ef24d7145ba.yaml diff --git a/cinder/tests/unit/volume/drivers/test_rbd.py b/cinder/tests/unit/volume/drivers/test_rbd.py index 2a98cbdd5c3..4d92afdcc03 100644 --- a/cinder/tests/unit/volume/drivers/test_rbd.py +++ b/cinder/tests/unit/volume/drivers/test_rbd.py @@ -26,6 +26,7 @@ import uuid import castellan import ddt +from oslo_utils import fileutils from oslo_utils import imageutils from oslo_utils import units @@ -3501,24 +3502,56 @@ class RBDTestCase(test.TestCase): self.assertEqual({'provider_location': None}, ret) + @ddt.data(('bare', 'raw'), + ('bare', 'qcow2'), + ('compressed', 'raw'), + ('compressed', 'qcow2')) + @ddt.unpack @common_mocks - def test_copy_volume_to_image(self): + def test_copy_volume_to_image(self, container_format, disk_format): + fake_image_meta = { + 'id': 'e105244f-4cb8-447b-8452-6f1da459e3ab', + 'container_format': container_format, + 'disk_format': disk_format, + } 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) + if container_format != 'compressed' and disk_format == 'raw': + self.driver.copy_volume_to_image(mock.sentinel.context, + mock.sentinel.volume, + mock.sentinel.image_service, + fake_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, + fake_image_meta, + None, + mock.sentinel.volume, + volume_fd= + mock.sentinel.rbd_handle) + else: + with mock.patch.object(self.driver, '_execute'), \ + mock.patch.object(fileutils, 'remove_path_on_error'), \ + mock.patch.object(os, 'unlink'), \ + mock.patch.object( + volume_utils, 'image_conversion_dir') as fake_dir: + fake_path = 'fake_path' + fake_vol = 'volume-' + fake_image_meta['id'] + fake_dir.return_value = fake_path + fake_vol_path = os.path.join(fake_path, fake_vol) + self.driver.copy_volume_to_image(mock.sentinel.context, + mock.sentinel.volume, + mock.sentinel.image_service, + fake_image_meta) + mock_get_rbd_handle.assert_not_called() + mock_uv.assert_called_once_with(mock.sentinel.context, + mock.sentinel.image_service, + fake_image_meta, + fake_vol_path, + mock.sentinel.volume) class ManagedRBDTestCase(test_driver.BaseDriverTestCase): diff --git a/cinder/volume/drivers/rbd.py b/cinder/volume/drivers/rbd.py index 03069cdf58e..7c44203ac21 100644 --- a/cinder/volume/drivers/rbd.py +++ b/cinder/volume/drivers/rbd.py @@ -2089,10 +2089,41 @@ class RBDDriver(driver.CloneableImageVD, driver.MigrateVD, return connector._get_rbd_handle(conn['data']) def copy_volume_to_image(self, context, volume, image_service, image_meta): - source_handle = self._get_rbd_handle(volume) + if image_meta.get('container_format') != 'compressed' and ( + image_meta['disk_format'] == 'raw'): + source_handle = self._get_rbd_handle(volume) - volume_utils.upload_volume(context, image_service, image_meta, None, - volume, volume_fd=source_handle) + volume_utils.upload_volume(context, image_service, image_meta, + None, volume, volume_fd=source_handle) + else: + # When the image format is different from volume format, we will + # fallback to the old workflow because of the following issues: + # 1. Passing RBDVolumeIOWrapper to format inspector + # We fail when calling privsep since RPC cannot serialize the + # RBDVolumeIOWrapper object + # 2. Handling in format inspector + # Determine if it's RBD file descriptor and only open the volume + # file if it's not + # 3. Handling in qemu-img commands + # Use rbd:{pool-name}/{image-name} format instead of file path + # https://docs.ceph.com/en/latest/rbd/qemu-rbd/#running-qemu-with-rbd # noqa + # + # Even if above issues are addressed, qemu-img convert will create + # a local copy of converted volume file so will need to determine + # the performance vs this workflow. + 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) def extend_volume(self, volume: Volume, new_size: str) -> None: """Extend an existing volume.""" diff --git a/releasenotes/notes/fix-rbd-upload-diff-format-38fc4ef24d7145ba.yaml b/releasenotes/notes/fix-rbd-upload-diff-format-38fc4ef24d7145ba.yaml new file mode 100644 index 00000000000..511478d9ce7 --- /dev/null +++ b/releasenotes/notes/fix-rbd-upload-diff-format-38fc4ef24d7145ba.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + RBD driver `bug #2092534 + `_: Fixed + uploading a volume to image when image has different format + than volume.