Fix invalid cache image-volume creation

During the volume ref creation in clone_image_volume
we were including a _name_id attribute which is invalid
in the base olso object.  The result is silent failure
for a key error. Note that the comments for the column
in the db specifcally state "do not call directly".
The variable is also denoted as private by the _ prefix.

Fix this by just popping the "_name_id' attribute.

In addition there were a number of things that needed
some cleanup:
1. Don't iterate over list of tuples to get K/V for dict
   Just use Dict(xxxx)
2. Don't use delete dict.key
   This isn't safe, because if the key DNE you'll get a key exception
   Instead use pop which is safe if key DNE
3. Add an error message when this fails, because currently the logs
   don't give an operation a chance of knowing it failed, let alone
   an opportunity to debug it

Finally the cache volume creation is still calling the db
directly and doing a resource create as opposed to
using the new volume object create handler.  I went ahead
and converted this to use objects, however that resulted in
breaking two of the volume-->image unit tests that use cinder
as a glance backend.

I've opened bug 1525773 for those issues and added a skip
to the tests in question.

Change-Id: Ieda40c1111d4d4d69488944eb332962eb9658266
Closes-Bug: #1525452

diff --git a/cinder/tests/unit/test_volume.py b/cinder/tests/unit/test_volume.py
index 82f806d5e773126eddb348da19f2b8eb971761f0..92b593f2c4745c87e5ff8ebc43c3b8e7035041b3 100644 (file)
--- a/cinder/tests/unit/test_volume.py
+++ b/cinder/tests/unit/test_volume.py
@@ -5956,6 +5956,7 @@ class CopyVolumeToImageTestCase(BaseVolumeTestCase):
                               self.context,
                               saving_image_id)
 
+    @test.testtools.skip('SKIP BUG #1173266')
     @mock.patch.object(QUOTAS, 'reserve')
     @mock.patch.object(QUOTAS, 'commit')
     @mock.patch.object(vol_manager.VolumeManager, 'create_volume')
@@ -6002,6 +6003,7 @@ class CopyVolumeToImageTestCase(BaseVolumeTestCase):
         image = self._test_copy_volume_to_image_with_image_volume()
         self.assertIsNone(image.get('locations'))
 
+    @test.testtools.skip('SKIP BUG #1173266')
     @mock.patch.object(vol_manager.VolumeManager, 'delete_volume')
     @mock.patch.object(fake_image._FakeImageService, 'add_location',
                        side_effect=exception.Invalid)

diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py
index d63e2360c89d0a5a061b9abca4516aa29997ec83..3f867e53dfa6da7ecb28839964f18b8f5bd95948 100644 (file)
--- a/cinder/volume/manager.py
+++ b/cinder/volume/manager.py
@@ -1076,22 +1076,29 @@ class VolumeManager(manager.SchedulerDependentManager):
         reservations = QUOTAS.reserve(ctx, **reserve_opts)
 
         try:
-            new_vol_values = {}
-            for k, v in volume.items():
-                new_vol_values[k] = v
-            del new_vol_values['id']
-            del new_vol_values['_name_id']
-            del new_vol_values['volume_type']
+            new_vol_values = dict(volume.items())
+            new_vol_values.pop('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['volume_type_id'] = volume_type_id
             new_vol_values['attach_status'] = 'detached'
-            new_vol_values['volume_attachment'] = []
             new_vol_values['status'] = 'creating'
             new_vol_values['project_id'] = ctx.project_id
             new_vol_values['display_name'] = 'image-%s' % image_meta['id']
             new_vol_values['source_volid'] = volume.id
+
             LOG.debug('Creating image volume entry: %s.', new_vol_values)
-            image_volume = self.db.volume_create(ctx, new_vol_values)
-        except Exception:
+            image_volume = objects.Volume(context=ctx, **new_vol_values)
+            image_volume.create()
+        except Exception as ex:
+            LOG.exception(_LE('Create clone_image_volume: %(volume_id)s'
+                              'for image %(image_id)s, '
+                              'failed (Exception: %(except)s)'),
+                          {'volume_id': volume.id,
+                           'image_id': image_meta['id'],
+                           'except': ex})
             QUOTAS.rollback(ctx, reservations)
             return False