Do proper cleanup if connect volume fails

If during the create image from volume the connect method fails with an
unexpected exception we will not be terminating the connection or
removing the export.  We will also not remove the export if the DB fails
when initializing the connection.

This patch fixes this making sure we are properly cleaning things up.

Change-Id: Ifa20366e6197bdce014d7f922d4700b684be1c23
Closes-Bug: #1685843

cinder/tests/unit/volume/test_connection.py

@@ -322,6 +322,15 @@ class VolumeAttachDetachTestCase(base.BaseVolumeTestCase):
                           self.context,
                           volume_id)
 
+    @mock.patch('cinder.volume.manager.LOG', mock.Mock())
+    def test_initialize_connection(self):
+        volume = mock.Mock(save=mock.Mock(side_effect=Exception))
+        with mock.patch.object(self.volume, 'driver') as driver_mock:
+            self.assertRaises(exception.ExportFailure,
+                              self.volume.initialize_connection, self.context,
+                              volume, mock.Mock())
+        driver_mock.remove_export.assert_called_once_with(mock.ANY, volume)
+
     def test_run_attach_detach_2volumes_for_instance(self):
         """Make sure volume can be attached and detached from instance."""
         # attach first volume to the instance

cinder/tests/unit/volume/test_image.py

@@ -563,6 +563,33 @@ class ImageVolumeTestCases(base.BaseVolumeTestCase):
         # We must have called detach method.
         self.assertEqual(1, mock_detach.call_count)
 
+    @mock.patch('cinder.utils.brick_get_connector_properties')
+    @mock.patch('cinder.utils.brick_get_connector')
+    @mock.patch('cinder.volume.driver.BaseVD._connect_device')
+    @mock.patch('cinder.volume.driver.BaseVD._detach_volume')
+    @mock.patch('cinder.image.image_utils.qemu_img_info')
+    def test_create_volume_from_image_unavailable_no_attach_info(
+            self, mock_qemu_info, mock_detach, mock_connect, *args):
+        """Test create volume with ImageCopyFailure
+
+        We'll raise an exception on _connect_device call to confirm that it
+        detaches the volume even if the exception doesn't have attach_info.
+        """
+        mock_connect.side_effect = NameError
+        image_info = imageutils.QemuImgInfo()
+        image_info.virtual_size = '1073741824'
+        mock_qemu_info.return_value = image_info
+
+        unbound_copy_method = cinder.volume.driver.BaseVD.copy_image_to_volume
+        bound_copy_method = unbound_copy_method.__get__(self.volume.driver)
+        with mock.patch.object(self.volume.driver, 'copy_image_to_volume',
+                               side_effect=bound_copy_method):
+            self.assertRaises(exception.ImageCopyFailure,
+                              self._create_volume_from_image,
+                              fakeout_copy_image_to_volume=False)
+        # We must have called detach method.
+        self.assertEqual(1, mock_detach.call_count)
+
     @mock.patch('cinder.image.image_utils.qemu_img_info')
     def test_create_volume_from_image_clone_image_volume(self, mock_qemu_info):
         """Test create volume from image via image volume.

cinder/volume/driver.py

@@ -434,10 +434,10 @@ class BaseVD(object):
                        force=False, remote=False):
         """Disconnect the volume from the host."""
         # Use Brick's code to do attach/detach
-        connector = attach_info['connector']
-        connector.disconnect_volume(attach_info['conn']['data'],
-                                    attach_info['device'])
-
+        if attach_info:
+            connector = attach_info['connector']
+            connector.disconnect_volume(attach_info['conn']['data'],
+                                        attach_info['device'])
         if remote:
             # Call remote manager's terminate_connection which includes
             # driver's terminate_connection and remove export
@@ -976,20 +976,20 @@ class BaseVD(object):
 
         try:
             attach_info = self._connect_device(conn)
-        except exception.DeviceUnavailable as exc:
+        except Exception as exc:
             # We may have reached a point where we have attached the volume,
             # so we have to detach it (do the cleanup).
-            attach_info = exc.kwargs.get('attach_info', None)
-            if attach_info:
-                try:
-                    LOG.debug('Device for volume %s is unavailable but did '
-                              'attach, detaching it.', volume['id'])
-                    self._detach_volume(context, attach_info, volume,
-                                        properties, force=True,
-                                        remote=remote)
-                except Exception:
-                    LOG.exception('Error detaching volume %s',
-                                  volume['id'])
+            attach_info = getattr(exc, 'kwargs', {}).get('attach_info', None)
+
+            try:
+                LOG.debug('Device for volume %s is unavailable but did '
+                          'attach, detaching it.', volume['id'])
+                self._detach_volume(context, attach_info, volume,
+                                    properties, force=True,
+                                    remote=remote)
+            except Exception:
+                LOG.exception('Error detaching volume %s',
+                              volume['id'])
             raise
 
         return (attach_info, volume)

cinder/volume/manager.py

@@ -1523,8 +1523,12 @@ class VolumeManager(manager.CleanableManager,
             if model_update:
                 volume.update(model_update)
                 volume.save()
-        except exception.CinderException as ex:
+        except Exception as ex:
             LOG.exception("Model update failed.", resource=volume)
+            try:
+                self.driver.remove_export(context.elevated(), volume)
+            except Exception:
+                LOG.exception('Could not remove export after DB model failed.')
             raise exception.ExportFailure(reason=six.text_type(ex))
 
         try: