Make object creation methods in l3_hamode_db atomic

Methods _create_ha_network, add_ha_port don't have wrapping
transaction in them, so they are prone to race conditions.
This commit adds a transaction to them. To avoid problem with
rolling back outmost transaction during exception handling,
internal methods have been wrapped in nested transaction.

Nested transaction is required in places like this:

def create():
      create_something()
      try:
            _do_other_thing()
      except Exception:
             with excutils.save_and_reraise_exception():
                     delete_something()

def _do_other_thing():
     with context.session.begin(subtransactions=True):
         ....

When exception is raised in _do_other_thing it
is caught in except block, but the object cannot be deleted in
except section because internal transaction has been rolled back.

A new method safe_creation and has been added
that provides a common way of handling such situations.

Closes-bug: #1501686

Change-Id: I952f6f7f8684743aa7f829bd92b1dc41b2c6aecf

doc/source/devref/effective_neutron.rst

@@ -132,6 +132,40 @@ Document common pitfalls as well as good practices done during database developm
   `patch 257086 <https://review.openstack.org/#/c/257086/>`_ which changed the
   availability zone code from the incorrect style to a database friendly one.
 
+* Sometimes in code we use the following structures:
+
+  .. code:: python
+
+     def create():
+        with context.session.begin(subtransactions=True):
+            create_something()
+            try:
+                _do_other_thing_with_created_object()
+            except Exception:
+                with excutils.save_and_reraise_exception():
+                    delete_something()
+
+     def _do_other_thing_with_created_object():
+        with context.session.begin(subtransactions=True):
+            ....
+
+  The problem is that when exception is raised in ``_do_other_thing_with_created_object``
+  it is caught in except block, but the object cannot be deleted in except
+  section because internal transaction from ``_do_other_thing_with_created_object``
+  has been rolled back. To avoid this nested transactions should be used.
+  For such cases help function ``safe_creation`` has been created in
+  ``neutron/db/common_db_mixin.py``.
+  So, the example above should be replaced with:
+
+  .. code:: python
+
+     _safe_creation(context, create_something, delete_someting,
+                    _do_other_thing_with_created_object)
+
+  Where nested transaction is used in _do_other_thing_with_created_object
+  function.
+
+
 System development
 ~~~~~~~~~~~~~~~~~~
 

neutron/db/common_db_mixin.py

@@ -16,15 +16,55 @@
 import weakref
 
 from debtcollector import removals
+from oslo_log import log as logging
+from oslo_utils import excutils
 import six
 from sqlalchemy import and_
 from sqlalchemy import or_
 from sqlalchemy import sql
 
-from neutron._i18n import _
+from neutron._i18n import _, _LE
 from neutron.common import exceptions as n_exc
 from neutron.db import sqlalchemyutils
 
+LOG = logging.getLogger(__name__)
+
+
+def safe_creation(context, create_fn, delete_fn, create_bindings):
+    '''This function wraps logic of object creation in safe atomic way.
+
+    In case of exception, object is deleted.
+
+    More information when this method could be used can be found in
+    developer guide - Effective Neutron: Database interaction section.
+    http://docs.openstack.org/developer/neutron/devref/effective_neutron.html
+
+    :param context: context
+
+    :param create_fn: function without arguments that is called to create
+        object and returns this object.
+
+    :param delete_fn: function that is called to delete an object. It is
+        called with object's id field as an argument.
+
+    :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):
+        obj = create_fn()
+        try:
+            value = create_bindings(obj['id'])
+        except Exception:
+            with excutils.save_and_reraise_exception():
+                try:
+                    delete_fn(obj['id'])
+                except Exception as e:
+                    LOG.error(_LE("Cannot clean up created object %(obj)s. "
+                                  "Exception: %(exc)s"), {'obj': obj['id'],
+                                                          'exc': e})
+        return obj, value
+
 
 def model_query_scope(context, model):
     # Unless a context has 'admin' or 'advanced-service' rights the

neutron/db/l3_hamode_db.py

@@ -13,6 +13,8 @@
 # under the License.
 #
 
+import functools
+
 import netaddr
 from oslo_config import cfg
 from oslo_db import exception as db_exc
@@ -28,6 +30,7 @@ from neutron.common import exceptions as n_exc
 from neutron.common import utils as n_utils
 from neutron.db import agents_db
 from neutron.db.availability_zone import router as router_az_db
+from neutron.db import common_db_mixin
 from neutron.db import l3_attrs_db
 from neutron.db import l3_db
 from neutron.db import l3_dvr_db
@@ -242,7 +245,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(subtransactions=True):
+        with context.session.begin(nested=True):
             ha_network = L3HARouterNetwork(tenant_id=tenant_id,
                                            network_id=network_id)
             context.session.add(ha_network)
@@ -265,16 +268,15 @@ class L3_HA_NAT_db_mixin(l3_dvr_db.L3_NAT_with_dvr_db_mixin,
                  'shared': False,
                  'admin_state_up': True}}
         self._add_ha_network_settings(args['network'])
-        network = p_utils.create_network(self._core_plugin, admin_ctx, args)
-
-        try:
-            ha_network = self._create_ha_network_tenant_binding(admin_ctx,
-                                                                tenant_id,
-                                                                network['id'])
-        except Exception:
-            with excutils.save_and_reraise_exception():
-                self._core_plugin.delete_network(admin_ctx, network['id'])
+        creation = functools.partial(p_utils.create_network,
+                                     self._core_plugin, admin_ctx, args)
+        content = functools.partial(self._create_ha_network_tenant_binding,
+                                    admin_ctx, tenant_id)
+        deletion = functools.partial(self._core_plugin.delete_network,
+                                     admin_ctx)
 
+        network, ha_network = common_db_mixin.safe_creation(
+            context, creation, deletion, content)
         try:
             self._create_ha_subnet(admin_ctx, network['id'], tenant_id)
         except Exception:
@@ -310,8 +312,8 @@ 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, port_id, router_id):
-        with context.session.begin(subtransactions=True):
+    def _create_ha_port_binding(self, context, router_id, port_id):
+        with context.session.begin(nested=True):
             portbinding = L3HARouterAgentPortBinding(port_id=port_id,
                                                      router_id=router_id)
             context.session.add(portbinding)
@@ -334,15 +336,15 @@ class L3_HA_NAT_db_mixin(l3_dvr_db.L3_NAT_with_dvr_db_mixin,
                 'device_id': router_id,
                 'device_owner': constants.DEVICE_OWNER_ROUTER_HA_INTF,
                 'name': constants.HA_PORT_NAME % tenant_id}
-        port = p_utils.create_port(self._core_plugin, context,
-                                 {'port': args})
-
-        try:
-            return self._create_ha_port_binding(context, port['id'], router_id)
-        except Exception:
-            with excutils.save_and_reraise_exception():
-                self._core_plugin.delete_port(context, port['id'],
-                                              l3_port_check=False)
+        creation = functools.partial(p_utils.create_port, self._core_plugin,
+                                     context, {'port': args})
+        content = functools.partial(self._create_ha_port_binding, context,
+                                    router_id)
+        deletion = functools.partial(self._core_plugin.delete_port, context,
+                                     l3_port_check=False)
+        port, bindings = common_db_mixin.safe_creation(context, creation,
+                                                       deletion, content)
+        return bindings
 
     def _create_ha_interfaces(self, context, router, ha_network):
         admin_ctx = context.elevated()

neutron/tests/unit/db/test_common_db_mixin.py

@@ -0,0 +1,45 @@
+# Copyright 2016
+# All Rights Reserved.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+import mock
+
+from neutron import context
+from neutron.db import common_db_mixin
+from neutron.tests.unit import testlib_api
+
+
+class TestCommonHelpFunctions(testlib_api.SqlTestCase):
+
+    def setUp(self):
+        super(TestCommonHelpFunctions, self).setUp()
+        self.admin_ctx = context.get_admin_context()
+
+    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()
+        self.assertRaises(ValueError, common_db_mixin.safe_creation,
+                          self.admin_ctx, create_fn, delete_fn,
+                          create_bindings)
+        delete_fn.assert_called_once_with(1234)
+
+    def test__safe_creation_deletion_fails(self):
+        create_fn = mock.Mock(return_value={'id': 1234})
+        create_bindings = mock.Mock(side_effect=ValueError)
+        delete_fn = mock.Mock(side_effect=EnvironmentError)
+        self.assertRaises(ValueError, common_db_mixin.safe_creation,
+                          self.admin_ctx, create_fn, delete_fn,
+                          create_bindings)
+        delete_fn.assert_called_once_with(1234)

neutron/tests/unit/db/test_l3_hamode_db.py

@@ -105,6 +105,7 @@ class L3HATestFramework(testlib_api.SqlTestCase):
 
 
 class L3HATestCase(L3HATestFramework):
+
     def test_verify_configuration_succeed(self):
         # Default configuration should pass
         self.plugin._verify_configuration()
@@ -469,7 +470,7 @@ class L3HATestCase(L3HATestFramework):
         network = self.plugin.get_ha_network(self.admin_ctx,
                                              router['tenant_id'])
 
-        with mock.patch.object(self.plugin, '_create_ha_port_binding',
+        with mock.patch.object(l3_hamode_db, 'L3HARouterAgentPortBinding',
                                side_effect=ValueError):
             self.assertRaises(ValueError, self.plugin.add_ha_port,
                               self.admin_ctx, router['id'], network.network_id,
@@ -483,8 +484,8 @@ class L3HATestCase(L3HATestFramework):
     def test_create_ha_network_binding_failure_rolls_back_network(self):
         networks_before = self.core_plugin.get_networks(self.admin_ctx)
 
-        with mock.patch.object(self.plugin,
-                               '_create_ha_network_tenant_binding',
+        with mock.patch.object(l3_hamode_db,
+                               'L3HARouterNetwork',
                                side_effect=ValueError):
             self.assertRaises(ValueError, self.plugin._create_ha_network,
                               self.admin_ctx, _uuid())
@@ -496,9 +497,10 @@ 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.assertRaises(ValueError, self.plugin._create_ha_network,
-                              self.admin_ctx, _uuid())
+                               side_effect=ValueError),\
+            self.admin_ctx._session.begin():
+                self.assertRaises(ValueError, self.plugin._create_ha_network,
+                                  self.admin_ctx, _uuid())
 
         networks_after = self.core_plugin.get_networks(self.admin_ctx)
         self.assertEqual(networks_before, networks_after)
@@ -512,7 +514,7 @@ class L3HATestCase(L3HATestFramework):
             self.admin_ctx, filters=device_filter)
 
         router_db = self.plugin._get_router(self.admin_ctx, router['id'])
-        with mock.patch.object(self.plugin, '_create_ha_port_binding',
+        with mock.patch.object(l3_hamode_db, 'L3HARouterAgentPortBinding',
                                side_effect=ValueError):
             self.assertRaises(ValueError, self.plugin._create_ha_interfaces,
                               self.admin_ctx, router_db, network)