Fix possible race condition for accept transfer

Accept transfer API workflow is currently like this:

  call volume_api.accept_transfer()
    |
    --- RPC cast to volume manager
          |
          --- volume manager calls volume driver accept_transfer()

  update volume's DB record

Given the non-blocking nature of RPC cast, what happens in volume
manager and volume driver can happen in parallel with the DB update.
If volume driver relies on original DB record to do things, then
DB record shouldn't be updated until volume driver finishes its job.

So this patch change volume RPC API accept_transfer() from cast
to call to make sure the workflow is in serialized manner.  Also
elevated the context when volume manager tries to update the DB
record when driver has done accept_transfer().

Change-Id: Ieae52e167aa02967338e0be5d78d570d682faa7a
Closes-bug: #1357432

diff --git a/cinder/tests/api/contrib/test_volume_transfer.py b/cinder/tests/api/contrib/test_volume_transfer.py
index 50605a7b70b355343d9fa47fb39e12a4f7c0bb24..f1d5b6d688c3b50558e3445a6d12f1430b6c49a7 100644 (file)
--- a/cinder/tests/api/contrib/test_volume_transfer.py
+++ b/cinder/tests/api/contrib/test_volume_transfer.py
@@ -385,6 +385,7 @@ class VolumeTransferAPITestCase(test.TestCase):
         volume_id = self._create_volume()
         transfer = self._create_transfer(volume_id)
 
+        svc = self.start_service('volume', host='fake_host')
         body = {"accept": {"id": transfer['id'],
                            "auth_key": transfer['auth_key']}}
         req = webob.Request.blank('/v2/fake/os-volume-transfer/%s/accept' %
@@ -398,10 +399,13 @@ class VolumeTransferAPITestCase(test.TestCase):
         self.assertEqual(res.status_int, 202)
         self.assertEqual(res_dict['transfer']['id'], transfer['id'])
         self.assertEqual(res_dict['transfer']['volume_id'], volume_id)
+        # cleanup
+        svc.stop()
 
     def test_accept_transfer_volume_id_specified_xml(self):
         volume_id = self._create_volume(size=5)
         transfer = self._create_transfer(volume_id)
+        svc = self.start_service('volume', host='fake_host')
 
         req = webob.Request.blank('/v2/fake/os-volume-transfer/%s/accept' %
                                   transfer['id'])
@@ -419,6 +423,8 @@ class VolumeTransferAPITestCase(test.TestCase):
         self.assertEqual(accept.item(0).getAttribute('volume_id'), volume_id)
 
         db.volume_destroy(context.get_admin_context(), volume_id)
+        # cleanup
+        svc.stop()
 
     def test_accept_transfer_with_no_body(self):
         volume_id = self._create_volume(size=5)

diff --git a/cinder/tests/test_volume_rpcapi.py b/cinder/tests/test_volume_rpcapi.py
index b5460cd161a305795a6e1020f373ff1115b731b5..82fe6396d275cc5980dbbd46ef47808aaf1caba3 100644 (file)
--- a/cinder/tests/test_volume_rpcapi.py
+++ b/cinder/tests/test_volume_rpcapi.py
@@ -234,7 +234,7 @@ class VolumeRpcAPITestCase(test.TestCase):
 
     def test_accept_transfer(self):
         self._test_volume_api('accept_transfer',
-                              rpc_method='cast',
+                              rpc_method='call',
                               volume=self.fake_volume,
                               new_user='e5565fd0-06c8-11e3-'
                                        '8ffd-0800200c9b77',

diff --git a/cinder/tests/test_volume_transfer.py b/cinder/tests/test_volume_transfer.py
index eac3db1bf6bac77e374b8c9844d9a6a29ebe5dbd..183b063b136fbcd3e1f084ee7c8d1686496b3ab5 100644 (file)
--- a/cinder/tests/test_volume_transfer.py
+++ b/cinder/tests/test_volume_transfer.py
@@ -59,6 +59,7 @@ class VolumeTransferTestCase(test.TestCase):
         self.assertEqual('in-use', volume['status'], 'Unexpected state')
 
     def test_transfer_accept(self):
+        svc = self.start_service('volume', host='test_host')
         tx_api = transfer_api.API()
         utils.create_volume(self.ctxt, id='1',
                             updated_at=self.updated_at)
@@ -97,6 +98,8 @@ class VolumeTransferTestCase(test.TestCase):
         self.assertEqual(transfer['id'], response['id'],
                          'Unexpected transfer id in response.')
 
+        svc.stop()
+
     def test_transfer_get(self):
         tx_api = transfer_api.API()
         volume = utils.create_volume(self.ctxt, id='1',

diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py
index 2c5ce6a909018b410b902798532682a093b0eebf..03d8dc36f3ef4b6b6c4c167bb0871d60e4bdd419 100644 (file)
--- a/cinder/volume/manager.py
+++ b/cinder/volume/manager.py
@@ -967,7 +967,7 @@ class VolumeManager(manager.SchedulerDependentManager):
 
         if model_update:
             try:
-                self.db.volume_update(context,
+                self.db.volume_update(context.elevated(),
                                       volume_id,
                                       model_update)
             except exception.CinderException:
@@ -982,6 +982,8 @@ class VolumeManager(manager.SchedulerDependentManager):
                                           volume_id,
                                           {'status': 'error'})
 
+        return model_update
+
     def _migrate_volume_generic(self, ctxt, volume, host, new_type_id):
         rpcapi = volume_rpcapi.VolumeAPI()
 

diff --git a/cinder/volume/rpcapi.py b/cinder/volume/rpcapi.py
index d5dab48250a38e069b6bc8a8b2cc2d3b4804706f..6c7638c35dbabf03b1196c389839c45b9da5b170 100644 (file)
--- a/cinder/volume/rpcapi.py
+++ b/cinder/volume/rpcapi.py
@@ -176,8 +176,8 @@ class VolumeAPI(object):
     def accept_transfer(self, ctxt, volume, new_user, new_project):
         new_host = utils.extract_host(volume['host'])
         cctxt = self.client.prepare(server=new_host, version='1.9')
-        cctxt.cast(ctxt, 'accept_transfer', volume_id=volume['id'],
-                   new_user=new_user, new_project=new_project)
+        return cctxt.call(ctxt, 'accept_transfer', volume_id=volume['id'],
+                          new_user=new_user, new_project=new_project)
 
     def extend_volume(self, ctxt, volume, new_size, reservations):
         new_host = utils.extract_host(volume['host'])