Fix image volume creation error

When "image_upload_use_cinder_backend = True" is specified to
the volume backend and the Glance cinder store is enabled,
upload-to-image command fails with the following error:

  Create clone_image_volume: xxx for image xxx failed
  (Exception: An object of type VolumeAttachmentList is required
   in field volume_attachment, not a )

This is due to unnecessary attributes specified in the volume
params, such as 'volume_attachment' or 'consistencygroup'.
This patch removes the unnecessary attributes from the params.

Change-Id: Ia416cb6686312e1591de7fe40a7ce7d6aa2fce7d
Closes-Bug: #1529762
Related-Bug: #1528087

diff --git a/cinder/tests/unit/test_volume.py b/cinder/tests/unit/test_volume.py
index adb9c7004a735a4243bd3df75f3f9309cd847002..ccf9ba3bff279ae85422183286cd8048982fb20f 100644 (file)
--- a/cinder/tests/unit/test_volume.py
+++ b/cinder/tests/unit/test_volume.py
@@ -3489,6 +3489,40 @@ class VolumeTestCase(BaseVolumeTestCase):
                           self.context,
                           volume_id)
 
+    @mock.patch('cinder.volume.drivers.lvm.LVMVolumeDriver.'
+                'create_cloned_volume')
+    @mock.patch('cinder.quota.QUOTAS.rollback')
+    @mock.patch('cinder.quota.QUOTAS.commit')
+    @mock.patch('cinder.quota.QUOTAS.reserve', return_value=["RESERVATION"])
+    def test_clone_image_volume(self, mock_reserve, mock_commit,
+                                mock_rollback, mock_cloned_volume):
+        vol = tests_utils.create_volume(self.context,
+                                        **self.volume_params)
+        # unnecessary attributes should be removed from image volume
+        vol.consistencygroup = None
+        result = self.volume._clone_image_volume(self.context, vol,
+                                                 {'id': 'fake'})
+
+        self.assertNotEqual(False, result)
+        mock_reserve.assert_called_once_with(self.context, volumes=1,
+                                             gigabytes=vol.size)
+        mock_commit.assert_called_once_with(self.context, ["RESERVATION"],
+                                            project_id=vol.project_id)
+
+    @mock.patch('cinder.quota.QUOTAS.rollback')
+    @mock.patch('cinder.quota.QUOTAS.commit')
+    @mock.patch('cinder.quota.QUOTAS.reserve', return_value=["RESERVATION"])
+    def test_clone_image_volume_creation_failure(self, mock_reserve,
+                                                 mock_commit, mock_rollback):
+        vol = tests_utils.create_volume(self.context, **self.volume_params)
+        with mock.patch.object(objects, 'Volume', side_effect=ValueError):
+            self.assertFalse(self.volume._clone_image_volume(self.context, vol,
+                                                             {'id': 'fake'}))
+
+        mock_reserve.assert_called_once_with(self.context, volumes=1,
+                                             gigabytes=vol.size)
+        mock_rollback.assert_called_once_with(self.context, ["RESERVATION"])
+
     @mock.patch('cinder.image.image_utils.TemporaryImages.fetch')
     @mock.patch('cinder.volume.flows.manager.create_volume.'
                 'CreateVolumeFromSpecTask._clone_image_volume')

diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py
index 40b9c2bd3b7e05eb71bbffbfb1ad2447efe45f49..42fa984fc93644517fc1d1c6a06e20bbc316241b 100644 (file)
--- a/cinder/volume/manager.py
+++ b/cinder/volume/manager.py
@@ -207,6 +207,15 @@ class VolumeManager(manager.SchedulerDependentManager):
 
     target = messaging.Target(version=RPC_API_VERSION)
 
+    # On cloning a volume, we shouldn't copy volume_type, consistencygroup
+    # and volume_attachment, because the db sets that according to [field]_id,
+    # which we do copy. We also skip some other values that are set during
+    # creation of Volume object.
+    _VOLUME_CLONE_SKIP_PROPERTIES = {
+        'id', '_name_id', 'name_id', 'name', 'status',
+        'attach_status', 'migration_status', 'volume_type',
+        'consistencygroup', 'volume_attachment'}
+
     def __init__(self, volume_driver=None, service_name=None,
                  *args, **kwargs):
         """Load the driver from the one specified in args, or from flags."""
@@ -1080,13 +1089,8 @@ class VolumeManager(manager.SchedulerDependentManager):
         QUOTAS.add_volume_type_opts(ctx, reserve_opts, volume_type_id)
         reservations = QUOTAS.reserve(ctx, **reserve_opts)
         try:
-            new_vol_values = dict(volume.items())
-            new_vol_values.pop('id', None)
-            new_vol_values.pop('_name_id', None)
-            new_vol_values.pop('name_id', None)
-            new_vol_values.pop('volume_type', None)
-            new_vol_values.pop('name', None)
-
+            new_vol_values = {k: volume[k] for k in set(volume.keys()) -
+                              self._VOLUME_CLONE_SKIP_PROPERTIES}
             new_vol_values['volume_type_id'] = volume_type_id
             new_vol_values['attach_status'] = 'detached'
             new_vol_values['status'] = 'creating'
@@ -1620,14 +1624,7 @@ class VolumeManager(manager.SchedulerDependentManager):
         rpcapi = volume_rpcapi.VolumeAPI()
 
         # Create new volume on remote host
-
-        skip = {'id', '_name_id', 'name_id', 'name', 'host', 'status',
-                'attach_status', 'migration_status', 'volume_type',
-                'consistencygroup', 'volume_attachment'}
-        # We don't copy volume_type, consistencygroup and volume_attachment,
-        # because the db sets that according to [field]_id, which we do copy.
-        # We also skip some other values that are either set manually later or
-        # during creation of Volume object.
+        skip = self._VOLUME_CLONE_SKIP_PROPERTIES | {'host'}
         new_vol_values = {k: volume[k] for k in set(volume.keys()) - skip}
         if new_type_id:
             new_vol_values['volume_type_id'] = new_type_id