From e25151be3d71abc9b51cfc85b92b42b3e1162354 Mon Sep 17 00:00:00 2001 From: YAMAMOTO Takashi Date: Tue, 1 Sep 2015 19:07:50 +0900 Subject: [PATCH] create_router: Report the original exception In case delete_router, which is an attempt to revert the operation, failed, report the original exception rather than the one from delete_router. The former is more likely useful for users. Besides that, it's critical to preserve the original exception for special exceptions like RetryRequest. Also, add some relevant unit tests. Closes-Bug: #1490917 Change-Id: I1cef944115c1911d4b7e60fb4dff389b1911ddbb --- neutron/db/l3_db.py | 25 ++++++--- neutron/tests/unit/db/test_l3_db.py | 2 + neutron/tests/unit/extensions/test_l3.py | 70 ++++++++++++++++++++++++ 3 files changed, 88 insertions(+), 9 deletions(-) diff --git a/neutron/db/l3_db.py b/neutron/db/l3_db.py index 6f8fbf49fac..2cd2b689c41 100644 --- a/neutron/db/l3_db.py +++ b/neutron/db/l3_db.py @@ -12,6 +12,7 @@ # License for the specific language governing permissions and limitations # under the License. +import functools import itertools import netaddr @@ -37,6 +38,7 @@ from neutron.common import constants as n_const from neutron.common import ipv6_utils from neutron.common import rpc as n_rpc from neutron.common import utils +from neutron.db import common_db_mixin from neutron.db import l3_agentschedulers_db as l3_agt from neutron.db import model_base from neutron.db import models_v2 @@ -226,18 +228,23 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase, context.session.add(router_db) return router_db + def _update_gw_for_create_router(self, context, gw_info, router_id): + if gw_info: + router_db = self._get_router(context, router_id) + self._update_router_gw_info(context, router_id, + gw_info, router=router_db) + def create_router(self, context, router): r = router['router'] gw_info = r.pop(EXTERNAL_GW_INFO, None) - router_db = self._create_router_db(context, r, r['tenant_id']) - try: - if gw_info: - self._update_router_gw_info(context, router_db['id'], - gw_info, router=router_db) - except Exception: - with excutils.save_and_reraise_exception(): - LOG.debug("Could not update gateway info, deleting router.") - self.delete_router(context, router_db.id) + create = functools.partial(self._create_router_db, context, r, + r['tenant_id']) + delete = functools.partial(self.delete_router, context) + update_gw = functools.partial(self._update_gw_for_create_router, + context, gw_info) + router_db, _unused = common_db_mixin.safe_creation(context, create, + delete, update_gw, + transaction=False) return self._make_router_dict(router_db) diff --git a/neutron/tests/unit/db/test_l3_db.py b/neutron/tests/unit/db/test_l3_db.py index d710efc15b8..9867d7bb333 100644 --- a/neutron/tests/unit/db/test_l3_db.py +++ b/neutron/tests/unit/db/test_l3_db.py @@ -228,6 +228,8 @@ class L3_NAT_db_mixin(base.BaseTestCase): return_value=router_dict),\ mock.patch.object(l3_db.L3_NAT_dbonly_mixin, '_update_router_gw_info') as urgi,\ + mock.patch.object(l3_db.L3_NAT_dbonly_mixin, '_get_router', + return_value=router_db),\ mock.patch.object(l3_db.L3_NAT_db_mixin, 'notify_router_updated')\ as nru: diff --git a/neutron/tests/unit/extensions/test_l3.py b/neutron/tests/unit/extensions/test_l3.py index c6a15f16c8f..36c751e3534 100644 --- a/neutron/tests/unit/extensions/test_l3.py +++ b/neutron/tests/unit/extensions/test_l3.py @@ -2797,6 +2797,76 @@ class L3NatTestCaseBase(L3NatTestCaseMixin): fip['floatingip']['floating_ip_address']) self.assertEqual(4, floating_ip.version) + def test_create_router_gateway_fails_nested(self): + # Force _update_router_gw_info failure + plugin = manager.NeutronManager.get_service_plugins()[ + service_constants.L3_ROUTER_NAT] + if not isinstance(plugin, l3_db.L3_NAT_dbonly_mixin): + self.skipTest("Plugin is not L3_NAT_dbonly_mixin") + ctx = context.Context('', 'foo') + data = {'router': { + 'name': 'router1', 'admin_state_up': True, + 'external_gateway_info': {'network_id': 'some_uuid'}, + 'tenant_id': 'some_tenant'}} + + def mock_fail__update_router_gw_info(ctx, router_id, info, + router=None): + # Fail with breaking transaction + with ctx.session.begin(subtransactions=True): + raise n_exc.NeutronException + + mock.patch.object(plugin, '_update_router_gw_info', + side_effect=mock_fail__update_router_gw_info).start() + + def create_router_with_transaction(ctx, data): + # Emulates what many plugins do + with ctx.session.begin(subtransactions=True): + plugin.create_router(ctx, data) + + # Verify router doesn't persist on failure + self.assertRaises(n_exc.NeutronException, + create_router_with_transaction, ctx, data) + routers = plugin.get_routers(ctx) + self.assertEqual(0, len(routers)) + + def test_create_router_gateway_fails_nested_delete_router_failed(self): + # Force _update_router_gw_info failure + plugin = manager.NeutronManager.get_service_plugins()[ + service_constants.L3_ROUTER_NAT] + if not isinstance(plugin, l3_db.L3_NAT_dbonly_mixin): + self.skipTest("Plugin is not L3_NAT_dbonly_mixin") + ctx = context.Context('', 'foo') + data = {'router': { + 'name': 'router1', 'admin_state_up': True, + 'external_gateway_info': {'network_id': 'some_uuid'}, + 'tenant_id': 'some_tenant'}} + + def mock_fail__update_router_gw_info(ctx, router_id, info, + router=None): + # Fail with breaking transaction + with ctx.session.begin(subtransactions=True): + raise n_exc.NeutronException + + def mock_fail_delete_router(ctx, router_id): + with ctx.session.begin(subtransactions=True): + raise Exception() + + mock.patch.object(plugin, '_update_router_gw_info', + side_effect=mock_fail__update_router_gw_info).start() + mock.patch.object(plugin, 'delete_router', + mock_fail_delete_router).start() + + def create_router_with_transaction(ctx, data): + # Emulates what many plugins do + with ctx.session.begin(subtransactions=True): + plugin.create_router(ctx, data) + + # Verify router doesn't persist on failure + self.assertRaises(n_exc.NeutronException, + create_router_with_transaction, ctx, data) + routers = plugin.get_routers(ctx) + self.assertEqual(0, len(routers)) + def test_update_subnet_gateway_for_external_net(self): """Test to make sure notification to routers occurs when the gateway ip address of a subnet of the external network is changed.