From: Gorka Eguileor Date: Fri, 2 Oct 2015 13:08:31 +0000 (+0200) Subject: Detach volume on device validation failure X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=c5323c6d6c9ec6628cd38e21f11ba45bfeb7867f;p=openstack-build%2Fcinder-build.git 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 e5a9a20aa..266858c53 100644 --- 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 901f9450c..2eba2cca3 100644 --- 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,