From 7e95b05b937e632ae8aee82cfa8f6f31835e6316 Mon Sep 17 00:00:00 2001 From: Zhiteng Huang Date: Sun, 7 Sep 2014 10:22:52 -0700 Subject: [PATCH] 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 --- cinder/tests/api/contrib/test_volume_transfer.py | 6 ++++++ cinder/tests/test_volume_rpcapi.py | 2 +- cinder/tests/test_volume_transfer.py | 3 +++ cinder/volume/manager.py | 4 +++- cinder/volume/rpcapi.py | 4 ++-- 5 files changed, 15 insertions(+), 4 deletions(-) diff --git a/cinder/tests/api/contrib/test_volume_transfer.py b/cinder/tests/api/contrib/test_volume_transfer.py index 50605a7b7..f1d5b6d68 100644 --- 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 b5460cd16..82fe6396d 100644 --- 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 eac3db1bf..183b063b1 100644 --- 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 2c5ce6a90..03d8dc36f 100644 --- 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 d5dab4825..6c7638c35 100644 --- 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']) -- 2.45.2