From: Nikola Dipanov <ndipanov@redhat.com>
Date: Fri, 25 Apr 2014 11:38:31 +0000 (+0200)
Subject: Make begin_detaching fail if volume not "in-use"
X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=d955c1da4490987233e252f94dffcfa106746c14;p=openstack-build%2Fcinder-build.git

Make begin_detaching fail if volume not "in-use"

Like it's counterpart from Nova's volume-attach functionality standpoint
- reserve_volume, begin_detaching should fail if the volume is not in
  the correct state to be detached. This will prevent nova from
attempting to check and then detach, which is inherently racy.

Change-Id: Ie87eb0c9aea068affc94032ab2e53a91888d272a
Partial-bug: #1302774
---

diff --git a/cinder/tests/test_volume.py b/cinder/tests/test_volume.py
index e94d04cc2..e2d1b9f32 100644
--- a/cinder/tests/test_volume.py
+++ b/cinder/tests/test_volume.py
@@ -2060,9 +2060,19 @@ class VolumeTestCase(BaseVolumeTestCase):
                           'name',
                           'description')
 
+    def test_begin_detaching_fails_available(self):
+        volume_api = cinder.volume.api.API()
+        volume = tests_utils.create_volume(self.context, **self.volume_params)
+        self.assertRaises(exception.InvalidVolume, volume_api.begin_detaching,
+                          self.context, volume)
+
     def test_begin_roll_detaching_volume(self):
         """Test begin_detaching and roll_detaching functions."""
+
+        instance_uuid = '12345678-1234-5678-1234-567812345678'
         volume = tests_utils.create_volume(self.context, **self.volume_params)
+        volume = db.volume_attached(
+            self.context, volume['id'], instance_uuid, 'fake-host', 'vdb')
         volume_api = cinder.volume.api.API()
         volume_api.begin_detaching(self.context, volume)
         volume = db.volume_get(self.context, volume['id'])
diff --git a/cinder/volume/api.py b/cinder/volume/api.py
index 1ce4ecbb0..33e5dc8a4 100644
--- a/cinder/volume/api.py
+++ b/cinder/volume/api.py
@@ -382,8 +382,20 @@ class API(base.Base):
         # If we are in the middle of a volume migration, we don't want the user
         # to see that the volume is 'detaching'. Having 'migration_status' set
         # will have the same effect internally.
-        if not volume['migration_status']:
-            self.update(context, volume, {"status": "detaching"})
+        if volume['migration_status']:
+            return
+
+        if (volume['status'] != 'in-use' and
+                volume['attach_status'] != 'attached'):
+            msg = (_("Unable to detach volume. Volume status must be 'in-use' "
+                     "and attached_status must be 'attached' to detach. "
+                     "Currently: status: '%(status)s', "
+                     "attach_status: '%(attach_status)s'") %
+                   {'status': volume['status'],
+                    'attach_status': volume['attach_status']})
+            LOG.error(msg)
+            raise exception.InvalidVolume(reason=msg)
+        self.update(context, volume, {"status": "detaching"})
 
     @wrap_check_policy
     def roll_detaching(self, context, volume):