From 74618128bc37b8277505d44b1cdea69191930258 Mon Sep 17 00:00:00 2001 From: Fernando Royo Date: Thu, 1 Dec 2022 11:47:58 +0100 Subject: [PATCH] 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 92efd8e45bef761fc464c932f1fd60d1c4d7e828) --- neutron/db/l3_db.py | 11 ++++-- neutron/tests/base.py | 12 +++++- neutron/tests/fullstack/test_l3_agent.py | 50 +++++++++++++++--------- 3 files changed, 50 insertions(+), 23 deletions(-) diff --git a/neutron/db/l3_db.py b/neutron/db/l3_db.py index 2abc2945ce8..bf121874048 100644 --- a/neutron/db/l3_db.py +++ b/neutron/db/l3_db.py @@ -1003,10 +1003,15 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase, 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) + # 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 subnets_id: + if len(subnets_id) > 1: id_filter = {'id': subnets_id} subnets = self._core_plugin.get_subnets(context.elevated(), filters=id_filter) diff --git a/neutron/tests/base.py b/neutron/tests/base.py index 14aa07a77ff..6f8bf486594 100644 --- a/neutron/tests/base.py +++ b/neutron/tests/base.py @@ -492,7 +492,11 @@ class BaseTestCase(DietTestCase): 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 @@ class BaseTestCase(DietTestCase): 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): diff --git a/neutron/tests/fullstack/test_l3_agent.py b/neutron/tests/fullstack/test_l3_agent.py index 3daf07facdc..9a7563d6d15 100644 --- a/neutron/tests/fullstack/test_l3_agent.py +++ b/neutron/tests/fullstack/test_l3_agent.py @@ -269,25 +269,37 @@ class TestL3Agent(base.BaseFullStackTestCase): 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): @@ -437,7 +449,7 @@ class TestLegacyL3Agent(TestL3Agent): 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() @@ -591,6 +603,6 @@ class TestHAL3Agent(TestL3Agent): 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)