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 b2fd36fc8e2277824061fdc39efa360845458acd..67bc182a2a25e587440314d2599315f832918437 100644 (file)
--- 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 4afd9bbef4759de3381417e22aafbb7a02286890..21431c613e2cb0a4c3f2ecb3de7c53e12e4ac22d 100644 (file)
--- 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.