From: Abhijeet Malawade <Abhijeet.Malawade@nttdata.com>
Date: Mon, 22 Dec 2014 13:50:28 +0000 (-0800)
Subject: Get volume from db again before updating it's status
X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=4d6ddcb94435a20476536f4956de88ce4ab15b66;p=openstack-build%2Fcinder-build.git

Get volume from db again before updating it's status

During uploading a volume which is attached to an instance to image service,
if the instance is deleted then the volume remains in "in-use" status.
So get the volume again from db before updating it's status.

Added new db api called 'volume_update_status_based_on_attachment'.
It gets volume from db and updates its status using the same session.

Closes-Bug: #1406703
Change-Id: I01d313438f021cd2fe0d6f5c23f2029279d4b55a
---

diff --git a/cinder/db/api.py b/cinder/db/api.py
index 1717631a4..f0726fea9 100644
--- a/cinder/db/api.py
+++ b/cinder/db/api.py
@@ -262,6 +262,11 @@ def volume_attachment_get_by_instance_uuid(context, volume_id, instance_uuid):
                                                        instance_uuid)
 
 
+def volume_update_status_based_on_attachment(context, volume_id):
+    """Update volume status according to attached instance id"""
+    return IMPL.volume_update_status_based_on_attachment(context, volume_id)
+
+
 ####################
 
 
diff --git a/cinder/db/sqlalchemy/api.py b/cinder/db/sqlalchemy/api.py
index 5fc00640b..4f2b36e80 100644
--- a/cinder/db/sqlalchemy/api.py
+++ b/cinder/db/sqlalchemy/api.py
@@ -1686,8 +1686,34 @@ def volume_attachment_update(context, attachment_id, values):
         return volume_attachment_ref
 
 
+def volume_update_status_based_on_attachment(context, volume_id):
+    """Update volume status based on attachment.
+
+    Get volume and check if 'volume_attachment' parameter is present in volume.
+    If 'volume_attachment' is None then set volume status to 'available'
+    else set volume status to 'in-use'.
+
+    :param context: context to query under
+    :param volume_id: id of volume to be updated
+    :returns: updated volume
+    """
+    session = get_session()
+    with session.begin():
+        volume_ref = _volume_get(context, volume_id, session=session)
+        # We need to get and update volume using same session because
+        # there is possibility that instance is deleted between the 'get'
+        # and 'update' volume call.
+        if not volume_ref['volume_attachment']:
+            volume_ref.update({'status': 'available'})
+        else:
+            volume_ref.update({'status': 'in-use'})
+
+        return volume_ref
+
+
 ####################
 
+
 def _volume_x_metadata_get_query(context, volume_id, model, session=None):
     return model_query(context, model, session=session, read_deleted="no").\
         filter_by(volume_id=volume_id)
diff --git a/cinder/tests/test_volume.py b/cinder/tests/test_volume.py
index 68ca32ca1..818d3deac 100644
--- a/cinder/tests/test_volume.py
+++ b/cinder/tests/test_volume.py
@@ -4576,6 +4576,46 @@ class CopyVolumeToImageTestCase(BaseVolumeTestCase):
         volume = db.volume_get(self.context, self.volume_id)
         self.assertEqual(volume['status'], 'available')
 
+    def test_copy_volume_to_image_instance_deleted(self):
+        # During uploading volume to image if instance is deleted,
+        # volume should be in available status.
+        self.image_meta['id'] = 'a440c04b-79fa-479c-bed1-0b816eaec379'
+        # Creating volume testdata
+        self.volume_attrs['instance_uuid'] = 'b21f957d-a72f-4b93-b5a5-' \
+                                             '45b1161abb02'
+        db.volume_create(self.context, self.volume_attrs)
+
+        # Storing unmocked db api function reference here, because we have to
+        # update volume status (set instance_uuid to None) before calling the
+        # 'volume_update_status_based_on_attached_instance_id' db api.
+        unmocked_db_api = db.volume_update_status_based_on_attachment
+
+        def mock_volume_update_after_upload(context, volume_id):
+            # First update volume and set 'instance_uuid' to None
+            # because after deleting instance, instance_uuid of volume is
+            # set to None
+            db.volume_update(context, volume_id, {'instance_uuid': None})
+            # Calling unmocked db api
+            unmocked_db_api(context, volume_id)
+
+        with mock.patch.object(
+                db,
+                'volume_update_status_based_on_attachment',
+                side_effect=mock_volume_update_after_upload) as mock_update:
+
+            # Start test
+            self.volume.copy_volume_to_image(self.context,
+                                             self.volume_id,
+                                             self.image_meta)
+            # Check 'volume_update_status_after_copy_volume_to_image'
+            # is called 1 time
+            self.assertEqual(1, mock_update.call_count)
+
+        # Check volume status has changed to available because
+        # instance is deleted
+        volume = db.volume_get(self.context, self.volume_id)
+        self.assertEqual('available', volume['status'])
+
     def test_copy_volume_to_image_status_use(self):
         self.image_meta['id'] = 'a440c04b-79fa-479c-bed1-0b816eaec379'
         # creating volume testdata
diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py
index bd728d1e5..2dd0823d9 100644
--- a/cinder/volume/manager.py
+++ b/cinder/volume/manager.py
@@ -938,12 +938,8 @@ class VolumeManager(manager.SchedulerDependentManager):
             with excutils.save_and_reraise_exception():
                 payload['message'] = unicode(error)
         finally:
-            if not volume['volume_attachment']:
-                self.db.volume_update(context, volume_id,
-                                      {'status': 'available'})
-            else:
-                self.db.volume_update(context, volume_id,
-                                      {'status': 'in-use'})
+            self.db.volume_update_status_based_on_attachment(context,
+                                                             volume_id)
 
     def _delete_image(self, context, image_id, image_service):
         """Deletes an image stuck in queued or saving state."""