From: Vipin Balachandran <vbala@vmware.com>
Date: Tue, 16 Jun 2015 12:15:18 +0000 (+0530)
Subject: Get updated volume status in begin_detaching
X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=8fdfac7177b2fd03d86c1bbe0513026492e46341;p=openstack-build%2Fcinder-build.git

Get updated volume status in begin_detaching

Heat stack deletion followed by an unsuccessful heat stack delete
attempt resulted in calling begin_detaching on a volume which was
already in detaching state. It was observed that the API returned
success instead of failing the operation. There is a check which
verifies the volume status, but it is not using the updated volume
status. This patch fixes the problem by getting updated volume
status from DB. But this fix  won't eliminate the possibility of
above mentioned inconsistent behavior if multiple begin_detaching
happens concurrently.

Closes-Bug: #1466060
Change-Id: I2a4cacef193e6cfa68026d97292370a895b291a9
---

diff --git a/cinder/tests/unit/test_volume.py b/cinder/tests/unit/test_volume.py
index b2fd36fc8..67bc182a2 100644
--- a/cinder/tests/unit/test_volume.py
+++ b/cinder/tests/unit/test_volume.py
@@ -3570,19 +3570,29 @@ class VolumeTestCase(BaseVolumeTestCase):
                           'name',
                           'description')
 
-    def test_begin_detaching_fails_available(self):
+    @mock.patch.object(db, 'volume_get')
+    def test_begin_detaching_fails_available(self, volume_get):
         volume_api = cinder.volume.api.API()
         volume = tests_utils.create_volume(self.context, **self.volume_params)
+        volume_get.return_value = volume
+        # Volume status is 'available'.
         self.assertRaises(exception.InvalidVolume, volume_api.begin_detaching,
                           self.context, volume)
+        volume_get.assert_called_once_with(self.context, volume['id'])
+
+        volume_get.reset_mock()
         volume['status'] = "in-use"
         volume['attach_status'] = "detached"
         # Should raise an error since not attached
         self.assertRaises(exception.InvalidVolume, volume_api.begin_detaching,
                           self.context, volume)
+        volume_get.assert_called_once_with(self.context, volume['id'])
+
+        volume_get.reset_mock()
         volume['attach_status'] = "attached"
         # Ensure when attached no exception raised
         volume_api.begin_detaching(self.context, volume)
+        volume_get.assert_called_once_with(self.context, volume['id'])
 
     def test_begin_roll_detaching_volume(self):
         """Test begin_detaching and roll_detaching functions."""
diff --git a/cinder/volume/api.py b/cinder/volume/api.py
index 4afd9bbef..21431c613 100644
--- a/cinder/volume/api.py
+++ b/cinder/volume/api.py
@@ -563,6 +563,10 @@ class API(base.Base):
 
     @wrap_check_policy
     def begin_detaching(self, context, volume):
+        # NOTE(vbala): The volume status might be 'detaching' already due to
+        # a previous begin_detaching call. Get updated volume status so that
+        # we fail such cases.
+        volume = self.db.volume_get(context, volume['id'])
         # 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.