L3HA: Do not wrap create/delete in transaction

This is unsafe when calling ML2 because ML2 assumes that its
functions will not be called inside of a transaction. This is
not only an issue for drivers that try to do DB lookups using
a different session in the post commit operation, but it's a
big issue for the delete methods.

The delete subnet and network methods in ML2 have 'while True'
loops that catch concurrency errors and retry the operation after
looking up info. If these are called inside a transaction, the
lookups will contain stale information and it can lead to the
while True loop never terminating!

Conflicts:
	doc/source/devref/effective_neutron.rst
	neutron/db/l3_hamode_db.py

Closes-Bug: #1551958
Change-Id: I33dc084ed15e5491fdda19da712a746ca87fbc8c
(cherry picked from commit d1c501cd1d31ed3f859975b40c01cb7aaa06d26d)

5 files changed, 28 insertions, 10 deletions

diff --git a/doc/source/devref/effective_neutron.rst b/doc/source/devref/effective_neutron.rst
index 71949dba45..cdabe45351 100644
--- a/doc/source/devref/effective_neutron.rst
+++ b/doc/source/devref/effective_neutron.rst
@@ -119,6 +119,10 @@ Document common pitfalls as well as good practices done during database developm
   Where nested transaction is used in _do_other_thing_with_created_object
   function.
 
+  The ``_safe_creation function can also be passed the ``transaction=False``
+  argument to prevent any transaction from being created just to leverage
+  the automatic deletion on exception logic.
+
 
 System development
 ~~~~~~~~~~~~~~~~~~
diff --git a/neutron/db/common_db_mixin.py b/neutron/db/common_db_mixin.py
index 8efccacfd0..cf917cb41b 100644
--- a/neutron/db/common_db_mixin.py
+++ b/neutron/db/common_db_mixin.py
@@ -13,6 +13,7 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import contextlib
 import weakref
 
 from oslo_log import log as logging
@@ -29,7 +30,13 @@ from neutron.db import sqlalchemyutils
 LOG = logging.getLogger(__name__)
 
 
-def safe_creation(context, create_fn, delete_fn, create_bindings):
+@contextlib.contextmanager
+def _noop_context_manager():
+    yield
+
+
+def safe_creation(context, create_fn, delete_fn, create_bindings,
+                  transaction=True):
     '''This function wraps logic of object creation in safe atomic way.
 
     In case of exception, object is deleted.
@@ -48,9 +55,13 @@ def safe_creation(context, create_fn, delete_fn, create_bindings):
 
     :param create_bindings: function that is called to create bindings for
         an object. It is called with object's id field as an argument.
-    '''
 
-    with context.session.begin(subtransactions=True):
+    :param transaction: if true the whole operation will be wrapped in a
+        transaction. if false, no transaction will be used.
+    '''
+    cm = (context.session.begin(subtransactions=True)
+          if transaction else _noop_context_manager())
+    with cm:
         obj = create_fn()
         try:
             value = create_bindings(obj['id'])
diff --git a/neutron/db/l3_hamode_db.py b/neutron/db/l3_hamode_db.py
index eb746b1c30..fe6c1edf94 100644
--- a/neutron/db/l3_hamode_db.py
+++ b/neutron/db/l3_hamode_db.py
@@ -240,7 +240,7 @@ class L3_HA_NAT_db_mixin(l3_dvr_db.L3_NAT_with_dvr_db_mixin):
 
     def _create_ha_network_tenant_binding(self, context, tenant_id,
                                           network_id):
-        with context.session.begin(nested=True):
+        with context.session.begin():
             ha_network = L3HARouterNetwork(tenant_id=tenant_id,
                                            network_id=network_id)
             context.session.add(ha_network)
@@ -271,7 +271,7 @@ class L3_HA_NAT_db_mixin(l3_dvr_db.L3_NAT_with_dvr_db_mixin):
                                      admin_ctx)
 
         network, ha_network = common_db_mixin.safe_creation(
-            context, creation, deletion, content)
+            context, creation, deletion, content, transaction=False)
         try:
             self._create_ha_subnet(admin_ctx, network['id'], tenant_id)
         except Exception:
@@ -308,7 +308,7 @@ class L3_HA_NAT_db_mixin(l3_dvr_db.L3_NAT_with_dvr_db_mixin):
         return num_agents
 
     def _create_ha_port_binding(self, context, router_id, port_id):
-        with context.session.begin(nested=True):
+        with context.session.begin():
             portbinding = L3HARouterAgentPortBinding(port_id=port_id,
                                                      router_id=router_id)
             context.session.add(portbinding)
@@ -338,7 +338,8 @@ class L3_HA_NAT_db_mixin(l3_dvr_db.L3_NAT_with_dvr_db_mixin):
         deletion = functools.partial(self._core_plugin.delete_port, context,
                                      l3_port_check=False)
         port, bindings = common_db_mixin.safe_creation(context, creation,
-                                                       deletion, content)
+                                                       deletion, content,
+                                                       transaction=False)
         return bindings
 
     def _create_ha_interfaces(self, context, router, ha_network):
diff --git a/neutron/tests/unit/db/test_common_db_mixin.py b/neutron/tests/unit/db/test_common_db_mixin.py
index 1cf7d95747..07cb780305 100644
--- a/neutron/tests/unit/db/test_common_db_mixin.py
+++ b/neutron/tests/unit/db/test_common_db_mixin.py
@@ -29,11 +29,14 @@ class TestCommonHelpFunctions(testlib_api.SqlTestCase):
     def test__safe_creation_create_bindings_fails(self):
         create_fn = mock.Mock(return_value={'id': 1234})
         create_bindings = mock.Mock(side_effect=ValueError)
-        delete_fn = mock.Mock()
+        tx_check = lambda i: setattr(self, '_active',
+                                     self.admin_ctx.session.is_active)
+        delete_fn = mock.Mock(side_effect=tx_check)
         self.assertRaises(ValueError, common_db_mixin.safe_creation,
                           self.admin_ctx, create_fn, delete_fn,
                           create_bindings)
         delete_fn.assert_called_once_with(1234)
+        self.assertTrue(self._active)
 
     def test__safe_creation_deletion_fails(self):
         create_fn = mock.Mock(return_value={'id': 1234})
diff --git a/neutron/tests/unit/db/test_l3_hamode_db.py b/neutron/tests/unit/db/test_l3_hamode_db.py
index 0fe302d275..74b8dc8b2f 100644
--- a/neutron/tests/unit/db/test_l3_hamode_db.py
+++ b/neutron/tests/unit/db/test_l3_hamode_db.py
@@ -513,8 +513,7 @@ class L3HATestCase(L3HATestFramework):
         networks_before = self.core_plugin.get_networks(self.admin_ctx)
 
         with mock.patch.object(self.plugin, '_create_ha_subnet',
-                               side_effect=ValueError),\
-            self.admin_ctx._session.begin():
+                               side_effect=ValueError):
                 self.assertRaises(ValueError, self.plugin._create_ha_network,
                                   self.admin_ctx, _uuid())