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 e94d04cc23da24f9bf69883061da075d74c83f18..e2d1b9f3268de91755c195804bb9c1f75657054c 100644 (file)
--- 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 1ce4ecbb0f05c6fdf727417d42fe0faf7a1cd6d8..33e5dc8a4bdf068345a689c8465e134a70cfaa93 100644 (file)
--- 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):