Detach volume on device validation failure

If device validation fails when attaching a volume for some driver
operation (copy_volume_data, copy_image_to_volume, copy_volume_to_image)
we may end up with an attached volume that we don't cleanup.

This patch tries to detach the volume if we fail when validating the
device after we have attached the volume.

This happens for example on multipath when we have properly detected the
paths but they are all in a failed state when we try to read from the
device on validation.

Closes-Bug: #1502138
Change-Id: I73be4206930eba7da064e22d86ff2136191acb0b

diff --git a/cinder/tests/unit/test_volume.py b/cinder/tests/unit/test_volume.py
index e5a9a20aa2a586191673bdfc271f990444b0efe2..266858c5300b9b35303ebe409dac40d2140610c3 100644 (file)
--- a/cinder/tests/unit/test_volume.py
+++ b/cinder/tests/unit/test_volume.py
@@ -3515,7 +3515,7 @@ class VolumeTestCase(BaseVolumeTestCase):
         """Test function of create_volume_from_image.
 
         Test cases call this function to create a volume from image, caller
-        can choose whether to fake out copy_image_to_volume and conle_image,
+        can choose whether to fake out copy_image_to_volume and clone_image,
         after calling this, test cases should check status of the volume.
         """
         def fake_local_path(volume):
@@ -3541,7 +3541,7 @@ class VolumeTestCase(BaseVolumeTestCase):
             self.stubs.Set(self.volume.driver, 'clone_image', fake_clone_image)
         self.stubs.Set(image_utils, 'fetch_to_raw', fake_fetch_to_raw)
         if fakeout_copy_image_to_volume:
-            self.stubs.Set(self.volume, '_copy_image_to_volume',
+            self.stubs.Set(self.volume.driver, 'copy_image_to_volume',
                            fake_copy_image_to_volume)
         mock_clone_image_volume.return_value = ({}, clone_image_volume)
         mock_fetch_img.return_value = mock.MagicMock(
@@ -3640,6 +3640,32 @@ class VolumeTestCase(BaseVolumeTestCase):
         self.assertDictEqual(self.volume.stats['pools'],
                              {'_pool0': {'allocated_capacity_gb': 1}})
 
+    @mock.patch('cinder.utils.brick_get_connector_properties')
+    @mock.patch('cinder.utils.brick_get_connector')
+    @mock.patch('cinder.volume.driver.BaseVD.secure_file_operations_enabled')
+    @mock.patch('cinder.volume.driver.BaseVD._detach_volume')
+    def test_create_volume_from_image_unavailable(self, mock_detach,
+                                                  mock_secure, *args):
+        """Test create volume with ImageCopyFailure
+
+        We'll raise an exception inside _connect_device after volume has
+        already been attached to confirm that it detaches the volume.
+        """
+        mock_secure.side_effect = NameError
+
+        # We want to test BaseVD copy_image_to_volume and since FakeISCSIDriver
+        # inherits from LVM it overwrites it, so we'll mock it to use the
+        # BaseVD implementation.
+        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)
+
     def test_create_volume_from_image_clone_image_volume(self):
         """Test create volume from image via image volume.
 

diff --git a/cinder/volume/driver.py b/cinder/volume/driver.py
index 901f9450c7316b011595575d55f74b38fcb9cd6a..2eba2cca3014924a41c3630b57e51d4e5126a898 100644 (file)
--- a/cinder/volume/driver.py
+++ b/cinder/volume/driver.py
@@ -964,7 +964,26 @@ class BaseVD(object):
                     LOG.error(err_msg)
                     raise exception.VolumeBackendAPIException(data=ex_msg)
                 raise exception.VolumeBackendAPIException(data=err_msg)
-        return (self._connect_device(conn), volume)
+
+        try:
+            attach_info = self._connect_device(conn)
+        except exception.DeviceUnavailable 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(_LE('Error detaching volume %s'),
+                                  volume['id'])
+            raise
+
+        return (attach_info, volume)
 
     def _attach_snapshot(self, context, snapshot, properties, remote=False):
         """Attach the snapshot."""
@@ -1037,17 +1056,26 @@ class BaseVD(object):
         device = connector.connect_volume(conn['data'])
         host_device = device['path']
 
-        # Secure network file systems will NOT run as root.
-        root_access = not self.secure_file_operations_enabled()
+        attach_info = {'conn': conn, 'device': device, 'connector': connector}
+
+        unavailable = True
+        try:
+            # Secure network file systems will NOT run as root.
+            root_access = not self.secure_file_operations_enabled()
+            unavailable = not connector.check_valid_device(host_device,
+                                                           root_access)
+        except Exception:
+            LOG.exception(_LE('Could not validate device %s'), host_device)
 
-        if not connector.check_valid_device(host_device, root_access):
+        if unavailable:
             raise exception.DeviceUnavailable(path=host_device,
+                                              attach_info=attach_info,
                                               reason=(_("Unable to access "
                                                         "the backend storage "
                                                         "via the path "
                                                         "%(path)s.") %
                                                       {'path': host_device}))
-        return {'conn': conn, 'device': device, 'connector': connector}
+        return attach_info
 
     def clone_image(self, context, volume,
                     image_location, image_meta,