From 924f19e8f16aebf103a3b70f2bd236afc846933b Mon Sep 17 00:00:00 2001 From: Ann Kamyshnikova Date: Sat, 12 Dec 2015 22:11:37 +0300 Subject: [PATCH] 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 | 34 ++++++++++++++ neutron/db/common_db_mixin.py | 42 ++++++++++++++++- neutron/db/l3_hamode_db.py | 44 +++++++++--------- neutron/tests/unit/db/test_common_db_mixin.py | 45 +++++++++++++++++++ neutron/tests/unit/db/test_l3_hamode_db.py | 16 ++++--- 5 files changed, 152 insertions(+), 29 deletions(-) create mode 100644 neutron/tests/unit/db/test_common_db_mixin.py diff --git a/doc/source/devref/effective_neutron.rst b/doc/source/devref/effective_neutron.rst index 6d4e4a5dbc3..9083680c7d9 100644 --- a/doc/source/devref/effective_neutron.rst +++ b/doc/source/devref/effective_neutron.rst @@ -132,6 +132,40 @@ Document common pitfalls as well as good practices done during database developm `patch 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 ~~~~~~~~~~~~~~~~~~ diff --git a/neutron/db/common_db_mixin.py b/neutron/db/common_db_mixin.py index b504cfbb5a7..9258771eeef 100644 --- a/neutron/db/common_db_mixin.py +++ b/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 diff --git a/neutron/db/l3_hamode_db.py b/neutron/db/l3_hamode_db.py index 14d07b96ad1..f7a49b9c5a1 100644 --- a/neutron/db/l3_hamode_db.py +++ b/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() diff --git a/neutron/tests/unit/db/test_common_db_mixin.py b/neutron/tests/unit/db/test_common_db_mixin.py new file mode 100644 index 00000000000..1cf7d957470 --- /dev/null +++ b/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) diff --git a/neutron/tests/unit/db/test_l3_hamode_db.py b/neutron/tests/unit/db/test_l3_hamode_db.py index a416c31b8fc..acbee11ecae 100644 --- a/neutron/tests/unit/db/test_l3_hamode_db.py +++ b/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)