openstack
/
neutron
Code Issues Proposed changes

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
(cherry picked from commit 924f19e8f1)
This commit is contained in:
Ann Kamyshnikova 2015-12-12 22:11:37 +03:00
parent cc6b16558e
commit 229856666a
5 changed files with 152 additions and 28 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

34
doc/source/devref/effective_neutron.rst
View File

@ -65,6 +65,40 @@ Document common pitfalls as well as good practices done during database developm
on the query object. Read the warnings for more details about in-python cascades.
* ...
* 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
~~~~~~~~~~~~~~~~~~

41
neutron/db/common_db_mixin.py
View File

@ -15,14 +15,55 @@
import weakref
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 _, _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

44
neutron/db/l3_hamode_db.py
View File

@ -13,6 +13,8 @@
# under the License.
#
import functools
import netaddr
from oslo_config import cfg
from oslo_db import exception as db_exc
@ -26,6 +28,7 @@ from neutron.common import constants
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 import common_db_mixin
from neutron.db import l3_attrs_db
from neutron.db import l3_db
from neutron.db import l3_dvr_db
@ -237,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(subtransactions=True):
with context.session.begin(nested=True):
ha_network = L3HARouterNetwork(tenant_id=tenant_id,
network_id=network_id)
context.session.add(ha_network)
@ -260,16 +263,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:
@ -305,8 +307,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)
@ -320,15 +322,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()

45
neutron/tests/unit/db/test_common_db_mixin.py Normal file
View File

@ -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)

16
neutron/tests/unit/db/test_l3_hamode_db.py
View File

@ -113,6 +113,7 @@ class L3HATestFramework(testlib_api.SqlTestCase):
class L3HATestCase(L3HATestFramework):
def test_verify_configuration_succeed(self):
# Default configuration should pass
self.plugin._verify_configuration()
@ -440,7 +441,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,
@ -454,8 +455,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())
@ -467,9 +468,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)
@ -483,7 +485,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)