From 9b2983743bacf6feb95a60d1b6fbc46e2427df88 Mon Sep 17 00:00:00 2001 From: XiaoYu Zhu Date: Tue, 28 Jul 2020 17:34:27 +0800 Subject: [PATCH] L3 router support ECMP This patch changes the policy for updating routes to support ECMP, and will now add ECMP routes to the Neutron router namespace when there are multiple routes pointing to the same destination address. Change-Id: I842c1408ee0235bc54441e9ed69c8b87ea30651b Related-Bug: #1880532 --- neutron/agent/l3/router_info.py | 71 ++++++++++++++++--- neutron/extensions/ecmp_routes.py | 20 ++++++ .../functional/agent/l3/test_dvr_router.py | 71 +++++++++++++++++++ .../tests/unit/agent/l3/test_router_info.py | 68 ++++++++++++++++++ .../notes/ecmp-routes-771ff34beafee370.yaml | 7 ++ 5 files changed, 227 insertions(+), 10 deletions(-) create mode 100644 neutron/extensions/ecmp_routes.py create mode 100644 releasenotes/notes/ecmp-routes-771ff34beafee370.yaml diff --git a/neutron/agent/l3/router_info.py b/neutron/agent/l3/router_info.py index 85c4829a03a..09bcf8f5e84 100644 --- a/neutron/agent/l3/router_info.py +++ b/neutron/agent/l3/router_info.py @@ -187,20 +187,71 @@ class RouterInfo(BaseRouterInfo): def update_routing_table(self, operation, route): self._update_routing_table(operation, route, self.ns_name) + def update_routing_table_ecmp(self, route_list): + multipath = [dict(via=route['nexthop']) + for route in route_list] + try: + ip_lib.add_ip_route(self.ns_name, route_list[0]['destination'], + via=multipath) + except (RuntimeError, OSError, pyroute2_exc.NetlinkError): + pass + + def check_and_remove_ecmp_route(self, old_routes, remove_route): + route_list = [] + for route in old_routes: + if route['destination'] == remove_route['destination']: + route_list.append(route) + # An ECMP route is composed of multiple routes with the same + # destination address, and two scenarios should be considered + # when removing a nexthop address from an ECMP route. + # a. The original ECMP route has only two nexthops, deleting + # one of them will make it a normal route. + # b. The original ECMP route has more than two nexthops, + # delete one of the nexthops, it is still an ECMP route. + if len(route_list) == 2: + for r in route_list: + if r['nexthop'] != remove_route['nexthop']: + self.update_routing_table('replace', r) + return True + + if len(route_list) > 2: + route_list.remove(remove_route) + self.update_routing_table_ecmp(route_list) + return True + + return False + + def check_and_add_ecmp_route(self, old_routes, new_route): + route_list = [] + for route in old_routes: + if route['destination'] == new_route['destination']: + route_list.append(route) + + if route_list: + route_list.append(new_route) + self.update_routing_table_ecmp(route_list) + return True + + return False + def routes_updated(self, old_routes, new_routes): adds, removes = helpers.diff_list_of_dict(old_routes, new_routes) - for route in adds: - LOG.debug("Added route entry is '%s'", route) - # remove replaced route from deleted route - for del_route in removes: - if route['destination'] == del_route['destination']: - removes.remove(del_route) - # replace success even if there is no existing route - self.update_routing_table('replace', route) for route in removes: - LOG.debug("Removed route entry is '%s'", route) - self.update_routing_table('delete', route) + # Judge if modifying an ECMP route or not, if not, + # just delete it, if it is, replace it + # update old_routes after modify + if not self.check_and_remove_ecmp_route(old_routes, route): + LOG.debug("Removed route entry is '%s'", route) + self.update_routing_table('delete', route) + old_routes.remove(route) + + for route in adds: + if not self.check_and_add_ecmp_route(old_routes, route): + LOG.debug("Added route entry is '%s'", route) + # replace success even if there is no existing route + self.update_routing_table('replace', route) + old_routes.append(route) def get_floating_ips(self): """Filter Floating IPs to be hosted on this agent.""" diff --git a/neutron/extensions/ecmp_routes.py b/neutron/extensions/ecmp_routes.py new file mode 100644 index 00000000000..d05a3270b18 --- /dev/null +++ b/neutron/extensions/ecmp_routes.py @@ -0,0 +1,20 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +from neutron_lib.api.definitions import ecmp_routes as apidef +from neutron_lib.api import extensions + + +class Ecmp_routes(extensions.APIExtensionDescriptor): + """Extension class supporting configuration of ECMP routes.""" + + api_definition = apidef diff --git a/neutron/tests/functional/agent/l3/test_dvr_router.py b/neutron/tests/functional/agent/l3/test_dvr_router.py index e250f86961c..dfd18679d9c 100644 --- a/neutron/tests/functional/agent/l3/test_dvr_router.py +++ b/neutron/tests/functional/agent/l3/test_dvr_router.py @@ -2185,3 +2185,74 @@ class TestDvrRouter(DvrRouterTestFramework, framework.L3AgentTestFramework): in fip_agent_gw_port['extra_subnets']) routes_cidr = set(route['cidr'] for route in routes) self.assertEqual(extra_subnet_cidr, routes_cidr) + + def test_dvr_router_update_ecmp_routes(self): + self.agent.conf.agent_mode = 'dvr' + router_info = self.generate_dvr_router_info() + print(router_info) + router1 = self.manage_router(self.agent, router_info) + router1.router['routes'] = [{'destination': '20.0.10.10/32', + 'nexthop': '35.4.0.11'}, + {'destination': '20.0.10.10/32', + 'nexthop': '35.4.0.22'}, + {'destination': '20.0.10.10/32', + 'nexthop': '35.4.0.33'}] + self.agent._process_updated_router(router1.router) + + updated_route = ip_lib.list_ip_routes( + router1.ns_name, + ip_version=lib_constants.IP_VERSION_4,) + expected_route = [{'cidr': '20.0.10.10/32', + 'table': 'main', + 'via': [{'via': '35.4.0.11'}, + {'via': '35.4.0.22'}, + {'via': '35.4.0.33'}] + }] + actual_routes = [{key: route[key] for key in expected_route[0].keys()} + for route in updated_route] + for entry in actual_routes: + if entry['via']: + if isinstance(entry['via'], (list, tuple)): + via_list = [{'via': hop['via']} + for hop in entry['via']] + entry['via'] = sorted(via_list, key=lambda i: i['via']) + self.assertIn(expected_route[0], actual_routes) + + # delete one route + router1.router['routes'] = [{'destination': '20.0.10.10/32', + 'nexthop': '35.4.0.11'}, + {'destination': '20.0.10.10/32', + 'nexthop': '35.4.0.22'}] + self.agent._process_updated_router(router1.router) + updated_route = ip_lib.list_ip_routes( + router1.ns_name, + ip_version=lib_constants.IP_VERSION_4, ) + expected_route = [{'cidr': '20.0.10.10/32', + 'table': 'main', + 'via': [{'via': '35.4.0.11'}, + {'via': '35.4.0.22'}] + }] + actual_routes = [{key: route[key] for key in expected_route[0].keys()} + for route in updated_route] + for entry in actual_routes: + if entry['via']: + if isinstance(entry['via'], (list, tuple)): + via_list = [{'via': hop['via']} + for hop in entry['via']] + entry['via'] = sorted(via_list, key=lambda i: i['via']) + self.assertIn(expected_route[0], actual_routes) + + # delete one route again + router1.router['routes'] = [{'destination': '20.0.10.10/32', + 'nexthop': '35.4.0.11'}] + self.agent._process_updated_router(router1.router) + updated_route = ip_lib.list_ip_routes( + router1.ns_name, + ip_version=lib_constants.IP_VERSION_4, ) + expected_route = [{'cidr': '20.0.10.10/32', + 'table': 'main', + 'via': '35.4.0.11' + }] + actual_routes = [{key: route[key] for key in expected_route[0].keys()} + for route in updated_route] + self.assertIn(expected_route[0], actual_routes) diff --git a/neutron/tests/unit/agent/l3/test_router_info.py b/neutron/tests/unit/agent/l3/test_router_info.py index 34f68913e0d..58344d623f6 100644 --- a/neutron/tests/unit/agent/l3/test_router_info.py +++ b/neutron/tests/unit/agent/l3/test_router_info.py @@ -52,6 +52,74 @@ class TestRouterInfo(base.BaseTestCase): self.mock_delete_ip_route) mock_method.assert_has_calls(mock_calls, any_order=True) + def _check_ip_wrapper_method_called(self, calls): + self.mock_ip.netns.execute.assert_has_calls( + [mock.call(call, check_exit_code=False) for call in calls], + any_order=True) + + def test_update_routing_table_ecmp(self): + ri = router_info.RouterInfo(mock.Mock(), _uuid(), {}, **self.ri_kwargs) + ri.router = {} + fake_route_list = [{'destination': '135.207.111.111/32', + 'nexthop': '135.207.111.112'}, + {'destination': '135.207.111.111/32', + 'nexthop': '135.207.111.113'}] + expected_dst = '135.207.111.111/32' + expected_next_hops = [{'via': '135.207.111.112'}, + {'via': '135.207.111.113'}] + ri.update_routing_table_ecmp(fake_route_list) + self.mock_add_ip_route.assert_called_once_with( + ri.ns_name, + expected_dst, + via=expected_next_hops) + + def test_check_and_remove_ecmp_route(self): + ri = router_info.RouterInfo(mock.Mock(), _uuid(), {}, **self.ri_kwargs) + ri.router = {} + fake_old_routes1 = [{'destination': '135.207.111.111/32', + 'nexthop': '135.207.111.112'}, + {'destination': '135.207.111.111/32', + 'nexthop': '135.207.111.113'}] + fake_route1 = {'destination': '135.207.111.111/32', + 'nexthop': '135.207.111.113'} + fake_old_routes2 = [{'destination': '135.207.111.111/32', + 'nexthop': '135.207.111.112'}, + {'destination': '135.207.111.111/32', + 'nexthop': '135.207.111.113'}, + {'destination': '135.207.111.111/32', + 'nexthop': '135.207.111.114'}] + fake_route2 = [{'destination': '135.207.111.111/32', + 'nexthop': '135.207.111.113'}, + {'destination': '135.207.111.111/32', + 'nexthop': '135.207.111.114'}] + fake_remove_route = {'destination': '135.207.111.111/32', + 'nexthop': '135.207.111.112'} + ri.update_routing_table = mock.Mock() + + ri.check_and_remove_ecmp_route(fake_old_routes1, fake_remove_route) + ri.update_routing_table.assert_called_once_with('replace', + fake_route1) + + ri.update_routing_table_ecmp = mock.Mock() + ri.check_and_remove_ecmp_route(fake_old_routes2, fake_remove_route) + ri.update_routing_table_ecmp.assert_called_once_with(fake_route2) + + def test_check_and_add_ecmp_route(self): + ri = router_info.RouterInfo(mock.Mock(), _uuid(), {}, **self.ri_kwargs) + ri.router = {} + fake_old_routes = [{'destination': '135.207.111.111/32', + 'nexthop': '135.207.111.112'}] + fake_new_route = {'destination': '135.207.111.111/32', + 'nexthop': '135.207.111.113'} + ri.update_routing_table_ecmp = mock.Mock() + + ri.check_and_add_ecmp_route(fake_old_routes, fake_new_route) + expected_routes = [{'destination': '135.207.111.111/32', + 'nexthop': '135.207.111.112'}, + {'destination': '135.207.111.111/32', + 'nexthop': '135.207.111.113'}] + ri.update_routing_table_ecmp.assert_called_once_with(expected_routes) + def test_routing_table_update(self): ri = router_info.RouterInfo(mock.Mock(), _uuid(), {}, **self.ri_kwargs) ri.router = {} diff --git a/releasenotes/notes/ecmp-routes-771ff34beafee370.yaml b/releasenotes/notes/ecmp-routes-771ff34beafee370.yaml new file mode 100644 index 00000000000..146624337fd --- /dev/null +++ b/releasenotes/notes/ecmp-routes-771ff34beafee370.yaml @@ -0,0 +1,7 @@ +--- +features: + - | + Neutron supports ECMP routes now, with this change, neutron will + consolidate multiple routes with the same destination address into + a single ECMP route. For more information see bug + `1880532 `_.