From 0abf43535d63e294f4649acff0f5e4a6e9c37275 Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Fri, 11 Oct 2019 15:56:15 -0400 Subject: [PATCH] Don't use wrap_db_retry on subtransaction in resource_create_replacement() The wrap_db_retry decorator must be used outside the outermost DB transaction. Using it on a subtransaction means that the main transaction gets marked for rollback but not closed, leaving the session in the 'inactive' state. This change ensures that the decorator is not used when we update the previous resource from within a subtransaction as part of creating a replacement resource, and instead the retry happens outside the main transaction. Change-Id: I28bfcc43b108d4d907098b2f0cf3553aab399553 Task: 36957 (cherry picked from commit 2398191be91d0dd145a4183f9a025fff1aa3727e) --- heat/db/sqlalchemy/api.py | 16 ++++++++++++---- heat/tests/db/test_sqlalchemy_api.py | 8 ++++---- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/heat/db/sqlalchemy/api.py b/heat/db/sqlalchemy/api.py index 3966b0e3a2..583e2cd787 100644 --- a/heat/db/sqlalchemy/api.py +++ b/heat/db/sqlalchemy/api.py @@ -260,6 +260,12 @@ def _add_atomic_key_to_values(values, atomic_key): retry_interval=0.5, inc_retry_interval=True) def resource_update(context, resource_id, values, atomic_key, expected_engine_id=None): + return _try_resource_update(context, resource_id, values, atomic_key, + expected_engine_id) + + +def _try_resource_update(context, resource_id, values, atomic_key, + expected_engine_id=None): session = context.session with session.begin(subtransactions=True): _add_atomic_key_to_values(values, atomic_key) @@ -448,6 +454,8 @@ def resource_create(context, values): return resource_ref +@oslo_db_api.wrap_db_retry(max_retries=3, retry_on_deadlock=True, + retry_interval=0.5, inc_retry_interval=True) def resource_create_replacement(context, existing_res_id, existing_res_values, new_res_values, @@ -458,10 +466,10 @@ def resource_create_replacement(context, new_res = resource_create(context, new_res_values) update_data = {'replaced_by': new_res.id} update_data.update(existing_res_values) - if not resource_update(context, - existing_res_id, update_data, - atomic_key, - expected_engine_id=expected_engine_id): + if not _try_resource_update(context, + existing_res_id, update_data, + atomic_key, + expected_engine_id=expected_engine_id): data = {} if 'name' in new_res_values: data['resource_name'] = new_res_values['name'] diff --git a/heat/tests/db/test_sqlalchemy_api.py b/heat/tests/db/test_sqlalchemy_api.py index 096d3b88f4..77821a7e23 100644 --- a/heat/tests/db/test_sqlalchemy_api.py +++ b/heat/tests/db/test_sqlalchemy_api.py @@ -2685,8 +2685,8 @@ class DBAPIResourceReplacementTest(common.HeatTestCase): db_api.resource_update_and_save(other_ctx, orig.id, {'atomic_key': 2}) - self.patchobject(db_api, 'resource_update', - new=mock.Mock(wraps=db_api.resource_update, + self.patchobject(db_api, '_try_resource_update', + new=mock.Mock(wraps=db_api._try_resource_update, side_effect=update_atomic_key)) self.assertRaises(exception.UpdateInProgress, @@ -2728,8 +2728,8 @@ class DBAPIResourceReplacementTest(common.HeatTestCase): {'engine_id': 'a', 'atomic_key': 2}) - self.patchobject(db_api, 'resource_update', - new=mock.Mock(wraps=db_api.resource_update, + self.patchobject(db_api, '_try_resource_update', + new=mock.Mock(wraps=db_api._try_resource_update, side_effect=lock_resource)) self.assertRaises(exception.UpdateInProgress,