Dont raise RouterInterfaceNotFound on overlap check router ports

A corner case of the fix done in [1] could happend if, as a race
scenario, parallel requests evaluate other ports that could be deleted
during the process if they had already determined a overlapping, in that
case a RouterInterfaceNotFound exception was raised and the request
finished with that exception and a 404 status code.

This patch removes the exception due to a port not found, because if the
port is not found, the related subnet should not participate in the
overlap evaluation, so it makes no sense to break the process for a port
that no longer exists. It also improves the previous validation to
perform the overlapping check, being performed only when we have
at least more than one subnet, as the overlapping check with only one
subnet did not make sense.

Closes-Bug: #1998226

[1] https://review.opendev.org/c/openstack/neutron/+/859143

Change-Id: If4afe6f525c46f9cf7f02d8aae27dfc56144fd62
(cherry picked from commit 92efd8e)

Author: fernandoroyosanchez

neutron/db/l3_db.py

@@ -1025,10 +1025,15 @@ def _add_router_port(self, context, port_id, router, device_owner):
                     subnets_id.extend([fixed_ip['subnet_id']
                                        for fixed_ip in port['fixed_ips']])
                 else:
-                    raise l3_exc.RouterInterfaceNotFound(
-                        router_id=router.id, port_id=rp.port_id)
-
-            if subnets_id:
+                    # due to race conditions maybe the port under analysis is
+                    # deleted, so instead returning a RouterInterfaceNotFound
+                    # we continue the analysis avoiding that port
+                    LOG.debug("Port %s could not be found, it might have been "
+                              "deleted concurrently. Will not be checked for "
+                              "an overlapping router interface.",
+                              rp.port_id)
+
+            if len(subnets_id) > 1:
                 id_filter = {'id': subnets_id}
                 subnets = self._core_plugin.get_subnets(context.elevated(),
                                                         filters=id_filter)

neutron/tests/base.py

@@ -492,7 +492,11 @@ def setup_rootwrap(self):
                     root_helper_daemon=get_rootwrap_daemon_cmd())
 
     def _simulate_concurrent_requests_process_and_raise(self, calls, args):
+        self._simulate_concurrent_requests_process(calls, args,
+                                                   raise_on_exception=True)
 
+    def _simulate_concurrent_requests_process(self, calls, args,
+                                              raise_on_exception=False):
         class SimpleThread(threading.Thread):
             def __init__(self, q):
                 super(SimpleThread, self).__init__()
@@ -529,10 +533,16 @@ def get_exception(self):
             t.start()
         q.join()
 
+        threads_exceptions = []
         for t in threads:
             e = t.get_exception()
             if e:
-                raise e
+                if raise_on_exception:
+                    raise e
+                else:
+                    threads_exceptions.append(e)
+
+        return threads_exceptions
 
 
 class PluginFixture(fixtures.Fixture):

neutron/tests/fullstack/test_l3_agent.py

@@ -270,25 +270,37 @@ def _is_filter_set(direction):
     def _test_concurrent_router_subnet_attachment_overlapping_cidr(self,
                                                                    ha=False):
         tenant_id = uuidutils.generate_uuid()
-        subnet_cidr = '10.100.0.0/24'
-        network1 = self.safe_client.create_network(
-            tenant_id, name='foo-network1')
-        subnet1 = self.safe_client.create_subnet(
-            tenant_id, network1['id'], subnet_cidr)
-        network2 = self.safe_client.create_network(
-            tenant_id, name='foo-network2')
-        subnet2 = self.safe_client.create_subnet(
-            tenant_id, network2['id'], subnet_cidr)
+        subnet_cidr = '10.200.0.0/24'
+        # to have many port interactions where race conditions would happen
+        # deleting ports meanwhile find operations to evaluate the overlapping
+        subnets = 10
+
+        funcs = []
+        args = []
         router = self.safe_client.create_router(tenant_id, ha=ha)
 
-        funcs = [self.safe_client.add_router_interface,
-                 self.safe_client.add_router_interface]
-        args = [(router['id'], subnet1['id']), (router['id'], subnet2['id'])]
-        self.assertRaises(
-            exceptions.BadRequest,
-            self._simulate_concurrent_requests_process_and_raise,
-            funcs,
-            args)
+        for i in range(subnets):
+            network_tmp = self.safe_client.create_network(
+                tenant_id, name='foo-network' + str(i))
+            subnet_tmp = self.safe_client.create_subnet(
+                tenant_id, network_tmp['id'], subnet_cidr)
+            funcs.append(self.safe_client.add_router_interface)
+            args.append((router['id'], subnet_tmp['id']))
+
+        exception_requests = self._simulate_concurrent_requests_process(
+            funcs, args)
+
+        if not all(type(e) == exceptions.BadRequest
+                   for e in exception_requests):
+            self.fail('Unexpected exception adding interfaces to router from '
+                      'different subnets overlapping')
+
+        if not len(exception_requests) >= (subnets - 1):
+            self.fail('If we have tried to associate %s subnets overlapping '
+                      'cidr to the router, we should have received at least '
+                      '%s or %s rejected requests, but we have only received '
+                      '%s', (str(subnets), str(subnets - 1), str(subnets),
+                             str(len(exception_requests))))
 
 
 class TestLegacyL3Agent(TestL3Agent):
@@ -441,7 +453,7 @@ def test_external_subnet_changed(self):
     def test_router_fip_qos_after_admin_state_down_up(self):
         self._router_fip_qos_after_admin_state_down_up()
 
-    def test_concurrent_router_subnet_attachment_overlapping_cidr_(self):
+    def test_concurrent_router_subnet_attachment_overlapping_cidr(self):
         self._test_concurrent_router_subnet_attachment_overlapping_cidr()
 
 
@@ -601,6 +613,6 @@ def test_external_subnet_changed(self):
     def test_router_fip_qos_after_admin_state_down_up(self):
         self._router_fip_qos_after_admin_state_down_up(ha=True)
 
-    def test_concurrent_router_subnet_attachment_overlapping_cidr_(self):
+    def test_concurrent_router_subnet_attachment_overlapping_cidr(self):
         self._test_concurrent_router_subnet_attachment_overlapping_cidr(
             ha=True)