Merge "create_router: Report the original exception"

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
@@ -230,18 +232,23 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase,
                             router_id=router['id'], router_db=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'])
+        create = functools.partial(self._create_router_db, context, r,
-        try:
+                                   r['tenant_id'])
-            if gw_info:
+        delete = functools.partial(self.delete_router, context)
-                self._update_router_gw_info(context, router_db['id'],
+        update_gw = functools.partial(self._update_gw_for_create_router,
-                                            gw_info, router=router_db)
+                                      context, gw_info)
-        except Exception:
+        router_db, _unused = common_db_mixin.safe_creation(context, create,
-            with excutils.save_and_reraise_exception():
+                                                           delete, update_gw,
-                LOG.debug("Could not update gateway info, deleting router.")
+                                                           transaction=False)
-                self.delete_router(context, router_db.id)
 
         return self._make_router_dict(router_db)
 

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:
 

neutron/tests/unit/extensions/test_l3.py

@@ -2795,6 +2795,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.