Merge "NVP plugin: Avoid timeouts if creating routers in parallel"

neutron/plugins/nicira/NeutronPlugin.py

@@ -601,6 +601,7 @@ class NvpPluginV2(addr_pair_db.AllowedAddressPairsMixin,
                          % router_id))
         return lr_port
 
+    @lockutils.synchronized('nicira', 'neutron-')
     def _nvp_create_ext_gw_port(self, context, port_data):
         """Driver for creating an external gateway port on NVP platform."""
         # TODO(salvatore-orlando): Handle NVP resource
@@ -1459,7 +1460,8 @@ class NvpPluginV2(addr_pair_db.AllowedAddressPairsMixin,
         # did not specify any value for the 'distributed' attribute
         # Platforms older than 3.x do not support the attribute
         r['distributed'] = lrouter.get('distributed', False)
-
+        # TODO(salv-orlando): Deal with backend object removal in case
+        # of db failures
         with context.session.begin(subtransactions=True):
             # Transaction nesting is needed to avoid foreign key violations
             # when processing the distributed router binding
@@ -1471,8 +1473,26 @@ class NvpPluginV2(addr_pair_db.AllowedAddressPairsMixin,
                                          status=lrouter['status'])
                 self._process_nsx_router_create(context, router_db, r)
                 context.session.add(router_db)
-            if has_gw_info:
+        if has_gw_info:
+            # NOTE(salv-orlando): This operation has been moved out of the
+            # database transaction since it performs several NVP queries,
+            # ithis ncreasing the risk of deadlocks between eventlet and
+            # sqlalchemy operations.
+            # Set external gateway and remove router in case of failure
+            try:
                 self._update_router_gw_info(context, router_db['id'], gw_info)
+            except (q_exc.NeutronException, NvpApiClient.NvpApiException):
+                with excutils.save_and_reraise_exception():
+                    # As setting gateway failed, the router must be deleted
+                    # in order to ensure atomicity
+                    router_id = router_db['id']
+                    LOG.warn(_("Failed to set gateway info for router being "
+                               "created:%s - removing router"), router_id)
+                    self.delete_router(context, router_id)
+                    LOG.info(_("Create router failed while setting external "
+                               "gateway. Router:%s has been removed from "
+                               "DB and backend"),
+                             router_id)
         router = self._make_router_dict(router_db)
         return router
 

neutron/tests/unit/nicira/test_nicira_plugin.py

@@ -622,20 +622,25 @@ class TestNiciraL3NatTestCase(NiciraL3NatTest,
             nvplib, 'create_lrouter', new=obsolete_response):
             self._test_router_create_with_distributed(None, False, '2.2')
 
+    def _create_router_with_gw_info_for_test(self, subnet):
+        data = {'router': {'tenant_id': 'whatever',
+                           'name': 'router1',
+                           'external_gateway_info':
+                           {'network_id': subnet['subnet']['network_id']}}}
+        router_req = self.new_create_request(
+            'routers', data, self.fmt)
+        return router_req.get_response(self.ext_api)
+
     def test_router_create_nvp_error_returns_500(self, vlan_id=None):
         with mock.patch.object(nvplib,
                                'create_router_lport',
                                side_effect=NvpApiClient.NvpApiException):
             with self._create_l3_ext_network(vlan_id) as net:
                 with self.subnet(network=net) as s:
-                    data = {'router': {'tenant_id': 'whatever'}}
+                    res = self._create_router_with_gw_info_for_test(s)
-                    data['router']['name'] = 'router1'
+                    self.assertEqual(
-                    data['router']['external_gateway_info'] = {
+                        webob.exc.HTTPInternalServerError.code,
-                        'network_id': s['subnet']['network_id']}
+                        res.status_int)
-                    router_req = self.new_create_request(
-                        'routers', data, self.fmt)
-                    res = router_req.get_response(self.ext_api)
-                    self.assertEqual(500, res.status_int)
 
     def test_router_add_gateway_invalid_network_returns_404(self):
         # NOTE(salv-orlando): This unit test has been overriden
@@ -647,6 +652,41 @@ class TestNiciraL3NatTestCase(NiciraL3NatTest,
                 uuidutils.generate_uuid(),
                 expected_code=webob.exc.HTTPNotFound.code)
 
+    def _verify_router_rollback(self):
+        # Check that nothing is left on DB
+        # TODO(salv-orlando): Verify whehter this is thread-safe
+        # w.r.t. sqllite and parallel testing
+        self._test_list_resources('router', [])
+        # Check that router is not in NVP
+        self.assertFalse(self.fc._fake_lrouter_dict)
+
+    def test_router_create_with_gw_info_neutron_fail_does_rollback(self):
+        # Simulate get subnet error while building list of ips with prefix
+        with mock.patch.object(self._plugin_class,
+                               '_build_ip_address_list',
+                               side_effect=ntn_exc.SubnetNotFound(
+                                   subnet_id='xxx')):
+            with self._create_l3_ext_network() as net:
+                with self.subnet(network=net) as s:
+                    res = self._create_router_with_gw_info_for_test(s)
+                    self.assertEqual(
+                        webob.exc.HTTPNotFound.code,
+                        res.status_int)
+                    self._verify_router_rollback()
+
+    def test_router_create_with_gw_info_nvp_fail_does_rollback(self):
+        # Simulate error while fetching nvp router gw port
+        with mock.patch.object(self._plugin_class,
+                               '_find_router_gw_port',
+                               side_effect=NvpApiClient.NvpApiException):
+            with self._create_l3_ext_network() as net:
+                with self.subnet(network=net) as s:
+                    res = self._create_router_with_gw_info_for_test(s)
+                    self.assertEqual(
+                        webob.exc.HTTPInternalServerError.code,
+                        res.status_int)
+                    self._verify_router_rollback()
+
     def _test_router_update_gateway_on_l3_ext_net(self, vlan_id=None,
                                                   validate_ext_gw=True):
         with self.router() as r: