From ab07b91b1adf9c7fe3d94a67aeb689788dfee09a Mon Sep 17 00:00:00 2001 From: Bence Romsics Date: Mon, 15 Jul 2019 15:18:46 +0200 Subject: [PATCH] Implement extension: extraroute-atomic I avoided changing the pre-existing extraroute implementation to also avoid any breakage of legacy behavior. Alternatively we can refactor the extraroute code to better fit both APIs: * the old: updates to the 'routes' attribute and * the new: actions 'add_extraroutes' / 'remove_extraroutes'. Change-Id: Ia78e0e02de4a42a056e3ff654d8b6976a4e52616 Closes-Bug: #1826396 (rfe) Depends-On: https://review.opendev.org/679259 Related-Change: https://review.opendev.org/655680 (spec) --- neutron/db/extraroute_db.py | 82 +++++++++++++++++++ .../services/l3_router/l3_router_plugin.py | 2 + neutron/tests/unit/db/test_extraroute_db.py | 82 +++++++++++++++++++ 3 files changed, 166 insertions(+) diff --git a/neutron/db/extraroute_db.py b/neutron/db/extraroute_db.py index 9149fad5bde..45f6f9aea8e 100644 --- a/neutron/db/extraroute_db.py +++ b/neutron/db/extraroute_db.py @@ -151,6 +151,88 @@ class ExtraRoute_dbonly_mixin(l3_db.L3_NAT_dbonly_mixin): raise xroute_exc.RouterInterfaceInUseByRoute( router_id=router_id, subnet_id=subnet_id) + @staticmethod + def _add_extra_routes(old, add): + """Add two lists of extra routes. + + Exact duplicates (both destination and nexthop) in old and add are + merged into one item. + Same destinations with different nexthops are accepted and all of + them are returned. + Overlapping destinations are accepted and all of them are returned. + """ + routes_dict = {} # its values are sets of nexthops + for r in old + add: + dst = r['destination'] + nexthop = r['nexthop'] + if dst not in routes_dict: + routes_dict[dst] = set() + routes_dict[dst].add(nexthop) + routes_list = [] + for dst, nexthops in routes_dict.items(): + for nexthop in nexthops: + routes_list.append({'destination': dst, 'nexthop': nexthop}) + return routes_list + + @staticmethod + def _remove_extra_routes(old, remove): + """Remove the 2nd list of extra routes from the first. + + Since we care about the end state if an extra route to be removed + is already missing from old, that's not an error, but accepted. + """ + routes_dict = {} # its values are sets of nexthops + for r in old: + dst = r['destination'] + nexthop = r['nexthop'] + if dst not in routes_dict: + routes_dict[dst] = set() + routes_dict[dst].add(nexthop) + for r in remove: + dst = r['destination'] + nexthop = r['nexthop'] + if dst in routes_dict: + routes_dict[dst].discard(nexthop) + routes_list = [] + for dst, nexthops in routes_dict.items(): + for nexthop in nexthops: + routes_list.append({'destination': dst, 'nexthop': nexthop}) + return routes_list + + @db_api.retry_if_session_inactive() + def add_extraroutes(self, context, router_id, body=None): + # NOTE(bence romsics): The input validation is delayed until + # update_router() validates the whole set of routes. Until then + # do not trust 'routes'. + routes = body['router']['routes'] + with db_api.CONTEXT_WRITER.using(context): + old_routes = self._get_extra_routes_by_router_id( + context, router_id) + router = self.update_router( + context, + router_id, + {'router': + {'routes': + self._add_extra_routes(old_routes, routes)}}) + return {'router': router} + + @db_api.retry_if_session_inactive() + def remove_extraroutes(self, context, router_id, body=None): + # NOTE(bence romsics): The input validation is delayed until + # update_router() validates the whole set of routes. Until then + # do not trust 'routes'. + routes = body['router']['routes'] + with db_api.CONTEXT_WRITER.using(context): + old_routes = self._get_extra_routes_by_router_id( + context, router_id) + router = self.update_router( + context, + router_id, + {'router': + {'routes': + self._remove_extra_routes(old_routes, routes)}}) + return {'router': router} + class ExtraRoute_db_mixin(ExtraRoute_dbonly_mixin, l3_db.L3_NAT_db_mixin): """Mixin class to support extra route configuration on router with rpc.""" diff --git a/neutron/services/l3_router/l3_router_plugin.py b/neutron/services/l3_router/l3_router_plugin.py index 6e5aba717e9..2e8a7627648 100644 --- a/neutron/services/l3_router/l3_router_plugin.py +++ b/neutron/services/l3_router/l3_router_plugin.py @@ -16,6 +16,7 @@ from neutron_lib.agent import topics from neutron_lib.api.definitions import dvr from neutron_lib.api.definitions import extraroute +from neutron_lib.api.definitions import extraroute_atomic from neutron_lib.api.definitions import fip_port_details from neutron_lib.api.definitions import floatingip_pools from neutron_lib.api.definitions import l3 as l3_apidef @@ -94,6 +95,7 @@ class L3RouterPlugin(service_base.ServicePluginBase, _supported_extension_aliases = [dvr.ALIAS, l3_apidef.ALIAS, l3_ext_gw_mode.ALIAS, extraroute.ALIAS, + extraroute_atomic.ALIAS, n_const.L3_AGENT_SCHEDULER_EXT_ALIAS, l3_ext_ha_mode.ALIAS, router_availability_zone.ALIAS, diff --git a/neutron/tests/unit/db/test_extraroute_db.py b/neutron/tests/unit/db/test_extraroute_db.py index 828cc07f7bc..d34dc9380b0 100644 --- a/neutron/tests/unit/db/test_extraroute_db.py +++ b/neutron/tests/unit/db/test_extraroute_db.py @@ -73,3 +73,85 @@ class TestExtraRouteDb(testlib_api.SqlTestCase): self.assertItemsEqual(updated_router['routes'], routes) got_router = self._plugin.get_router(ctx, router_id) self.assertItemsEqual(got_router['routes'], routes) + + def assertEqualRoutes(self, a, b): + """Compare a list of routes without caring for the list order.""" + return self.assertSetEqual( + set(frozenset(r.items()) for r in a), + set(frozenset(r.items()) for r in b)) + + def test_add_extra_routes(self): + self.assertEqual( + [], + self._plugin._add_extra_routes([], []), + ) + + old = [{"destination": "10.0.10.0/24", "nexthop": "10.0.0.10"}] + add = [] + self.assertEqual(old, self._plugin._add_extra_routes(old, add)) + + old = [] + add = [{"destination": "10.0.10.0/24", "nexthop": "10.0.0.10"}] + self.assertEqual(add, self._plugin._add_extra_routes(old, add)) + + old = [{"destination": "10.0.10.0/24", "nexthop": "10.0.0.10"}] + add = [{"destination": "10.0.10.0/24", "nexthop": "10.0.0.10"}] + self.assertEqual(old, self._plugin._add_extra_routes(old, add)) + + old = [{"destination": "10.0.10.0/24", "nexthop": "10.0.0.10"}] + add = [{"destination": "10.0.10.0/24", "nexthop": "10.0.0.11"}] + self.assertEqualRoutes( + old + add, self._plugin._add_extra_routes(old, add)) + + def test_remove_extra_routes(self): + old = [] + remove = [] + self.assertEqual(old, self._plugin._remove_extra_routes(old, remove)) + + old = [{"destination": "10.0.10.0/24", "nexthop": "10.0.0.10"}] + remove = [] + self.assertEqual(old, self._plugin._remove_extra_routes(old, remove)) + + old = [] + remove = [{"destination": "10.0.10.0/24", "nexthop": "10.0.0.10"}] + self.assertEqual(old, self._plugin._remove_extra_routes(old, remove)) + + old = [{"destination": "10.0.10.0/24", "nexthop": "10.0.0.10"}] + remove = [{"destination": "10.0.10.0/24", "nexthop": "10.0.0.11"}] + self.assertEqual(old, self._plugin._remove_extra_routes(old, remove)) + + old = [{"destination": "10.0.10.0/24", "nexthop": "10.0.0.10"}] + remove = old + self.assertEqual([], self._plugin._remove_extra_routes(old, remove)) + + old = [ + {"destination": "10.0.10.0/24", "nexthop": "10.0.0.10"}, + {"destination": "10.0.11.0/24", "nexthop": "10.0.0.11"}, + ] + remove = old[1:] + self.assertEqual( + old[:1], self._plugin._remove_extra_routes(old, remove)) + + old = [ + {"destination": "10.0.10.0/24", "nexthop": "10.0.0.10"}, + {"destination": "10.0.10.0/24", "nexthop": "10.0.0.11"}, + ] + remove = old[1:] + self.assertEqual( + old[:1], self._plugin._remove_extra_routes(old, remove)) + + old = [] + remove = [ + {"destination": "10.0.10.0/24", "nexthop": "10.0.0.10"}, + {"destination": "10.0.10.0/24", "nexthop": "10.0.0.10"}, + ] + self.assertEqual([], self._plugin._remove_extra_routes(old, remove)) + + old = [ + {"destination": "10.0.10.0/24", "nexthop": "10.0.0.10"}, + ] + remove = [ + {"destination": "10.0.10.0/24", "nexthop": "10.0.0.10"}, + {"destination": "10.0.10.0/24", "nexthop": "10.0.0.10"}, + ] + self.assertEqual([], self._plugin._remove_extra_routes(old, remove))