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

@@ -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,6 +1103,12 @@ 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'])
+            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, {},

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)

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)

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"

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(

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)
 

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()
+        source_handle = self._get_rbd_handle(volume)
-        tmp_file = os.path.join(tmp_dir,
+
-                                volume.name + '-' + image_meta['id'])
+        volume_utils.upload_volume(context, image_service, image_meta, None,
-        with fileutils.remove_path_on_error(tmp_file):
+                                   volume, volume_fd=source_handle)
-            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."""

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):

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.