From: Swaminathan Vasudevan Date: Fri, 25 Sep 2015 16:54:44 +0000 (-0700) Subject: Static routes not added to qrouter namespace for DVR X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=158f9eabe20824b2c91eaac795dad8b8a773611d;p=openstack-build%2Fneutron-build.git Static routes not added to qrouter namespace for DVR Today static routes are added to the SNAT namespace for DVR routers. But they are not added to the qrouter namespace. Also while configuring the static routes to SNAT namespace, the router is not checked for the existence of the gateway. When routes are added to a router without a gateway the routes are only configured in the router namespace, but when a gateway is set later, those routes have to be populated in the snat_namespace as well. This patch addresses the above mentioned issues. Closes-Bug: #1499785 Closes-Bug: #1499787 Change-Id: I37e0d0d723fcc727faa09028045b776957c75a82 --- diff --git a/neutron/agent/l3/dvr_edge_router.py b/neutron/agent/l3/dvr_edge_router.py index d2424096d..4e45bebde 100644 --- a/neutron/agent/l3/dvr_edge_router.py +++ b/neutron/agent/l3/dvr_edge_router.py @@ -14,6 +14,7 @@ from oslo_log import log as logging +from neutron._i18n import _LE from neutron.agent.l3 import dvr_local_router from neutron.agent.l3 import dvr_snat_ns from neutron.agent.l3 import router_info as router @@ -35,6 +36,11 @@ class DvrEdgeRouter(dvr_local_router.DvrLocalRouter): ex_gw_port, interface_name) if self._is_this_snat_host(): self._create_dvr_gateway(ex_gw_port, interface_name) + # NOTE: When a router is created without a gateway the routes get + # added to the router namespace, but if we wanted to populate + # the same routes to the snat namespace after the gateway port + # is added, we need to call routes_updated here. + self.routes_updated([], self.router['routes']) def external_gateway_updated(self, ex_gw_port, interface_name): if not self._is_this_snat_host(): @@ -182,10 +188,20 @@ class DvrEdgeRouter(dvr_local_router.DvrLocalRouter): self._add_snat_rules(ex_gw_port, self.snat_iptables_manager, interface_name) - def update_routing_table(self, operation, route, namespace=None): - ns_name = dvr_snat_ns.SnatNamespace.get_snat_ns_name(self.router['id']) - super(DvrEdgeRouter, self).update_routing_table(operation, route, - namespace=ns_name) + def update_routing_table(self, operation, route): + if self.get_ex_gw_port() and self._is_this_snat_host(): + ns_name = dvr_snat_ns.SnatNamespace.get_snat_ns_name( + self.router['id']) + # NOTE: For now let us apply the static routes both in SNAT + # namespace and Router Namespace, to reduce the complexity. + ip_wrapper = ip_lib.IPWrapper(namespace=ns_name) + if ip_wrapper.netns.exists(ns_name): + super(DvrEdgeRouter, self)._update_routing_table( + operation, route, namespace=ns_name) + else: + LOG.error(_LE("The SNAT namespace %s does not exist for " + "the router."), ns_name) + super(DvrEdgeRouter, self).update_routing_table(operation, route) def delete(self, agent): super(DvrEdgeRouter, self).delete(agent) diff --git a/neutron/agent/l3/ha_router.py b/neutron/agent/l3/ha_router.py index d1d1a1f39..6d54ddfab 100644 --- a/neutron/agent/l3/ha_router.py +++ b/neutron/agent/l3/ha_router.py @@ -179,15 +179,12 @@ class HaRouter(router.RouterInfo): def get_router_cidrs(self, device): return set(self._get_cidrs_from_keepalived(device.name)) - def routes_updated(self): - new_routes = self.router['routes'] - + def routes_updated(self, old_routes, new_routes): instance = self._get_keepalived_instance() instance.virtual_routes.extra_routes = [ keepalived.KeepalivedVirtualRoute( route['destination'], route['nexthop']) for route in new_routes] - self.routes = new_routes def _add_default_gw_virtual_route(self, ex_gw_port, interface_name): gateway_ips = self._get_external_gw_ips(ex_gw_port) diff --git a/neutron/agent/l3/router_info.py b/neutron/agent/l3/router_info.py index 968bd9d36..4a4917a0c 100644 --- a/neutron/agent/l3/router_info.py +++ b/neutron/agent/l3/router_info.py @@ -107,15 +107,10 @@ class RouterInfo(object): ip_wrapper = ip_lib.IPWrapper(namespace=namespace) ip_wrapper.netns.execute(cmd, check_exit_code=False) - def update_routing_table(self, operation, route, namespace=None): - if namespace is None: - namespace = self.ns_name - self._update_routing_table(operation, route, namespace) + def update_routing_table(self, operation, route): + self._update_routing_table(operation, route, self.ns_name) - def routes_updated(self): - new_routes = self.router['routes'] - - old_routes = self.routes + def routes_updated(self, old_routes, new_routes): adds, removes = common_utils.diff_list_of_dict(old_routes, new_routes) for route in adds: @@ -129,7 +124,6 @@ class RouterInfo(object): for route in removes: LOG.debug("Removed route entry is '%s'", route) self.update_routing_table('delete', route) - self.routes = new_routes def get_ex_gw_port(self): return self.router.get('gw_port') @@ -707,7 +701,8 @@ class RouterInfo(object): agent.pd.sync_router(self.router['id']) self.process_external(agent, delete) # Process static routes for router - self.routes_updated() + self.routes_updated(self.routes, self.router['routes']) + self.routes = self.router['routes'] # Update ex_gw_port and enable_snat on the router info cache self.ex_gw_port = self.get_ex_gw_port() diff --git a/neutron/tests/functional/agent/l3/framework.py b/neutron/tests/functional/agent/l3/framework.py index 465338a90..797dac7c7 100644 --- a/neutron/tests/functional/agent/l3/framework.py +++ b/neutron/tests/functional/agent/l3/framework.py @@ -423,8 +423,10 @@ class L3AgentTestFramework(base.BaseSudoTestCase): self.assertTrue(self.device_exists_with_ips_and_mac( device, router.get_internal_device_name, router.ns_name)) - def _assert_extra_routes(self, router): - routes = ip_lib.get_routing_table(4, namespace=router.ns_name) + def _assert_extra_routes(self, router, namespace=None): + if namespace is None: + namespace = router.ns_name + routes = ip_lib.get_routing_table(4, namespace=namespace) routes = [{'nexthop': route['nexthop'], 'destination': route['destination']} for route in routes] diff --git a/neutron/tests/functional/agent/l3/test_dvr_router.py b/neutron/tests/functional/agent/l3/test_dvr_router.py index 831d670a6..482c3f284 100644 --- a/neutron/tests/functional/agent/l3/test_dvr_router.py +++ b/neutron/tests/functional/agent/l3/test_dvr_router.py @@ -115,7 +115,7 @@ class TestDvrRouter(framework.L3AgentTestFramework): # We get the router info particular to a dvr router router_info = self.generate_dvr_router_info( - enable_ha, enable_snat) + enable_ha, enable_snat, extra_routes=True) # We need to mock the get_agent_gateway_port return value # because the whole L3PluginApi is mocked and we need the port @@ -164,7 +164,6 @@ class TestDvrRouter(framework.L3AgentTestFramework): self._assert_snat_chains(router) self._assert_floating_ip_chains(router) self._assert_metadata_chains(router) - self._assert_extra_routes(router) self._assert_rfp_fpr_mtu(router, custom_mtu) if enable_snat: ip_versions = [4, 6] if (ip_version == 6 or dual_stack) else [4] @@ -172,7 +171,7 @@ class TestDvrRouter(framework.L3AgentTestFramework): router.router_id) self._assert_onlink_subnet_routes( router, ip_versions, snat_ns_name) - + self._assert_extra_routes(router, namespace=snat_ns_name) self._delete_router(self.agent, router.router_id) self._assert_fip_namespace_deleted(ext_gateway_port) self._assert_router_does_not_exist(router) @@ -182,6 +181,7 @@ class TestDvrRouter(framework.L3AgentTestFramework): enable_ha=False, enable_snat=False, agent=None, + extra_routes=False, **kwargs): if not agent: agent = self.agent @@ -189,6 +189,8 @@ class TestDvrRouter(framework.L3AgentTestFramework): enable_snat=enable_snat, enable_floating_ip=True, enable_ha=enable_ha, + extra_routes=extra_routes, + num_internal_ports=2, **kwargs) internal_ports = router.get(l3_constants.INTERFACE_KEY, []) router['distributed'] = True @@ -644,6 +646,24 @@ class TestDvrRouter(framework.L3AgentTestFramework): self._assert_ip_addresses_in_dvr_ha_snat_namespace(router2) self._assert_no_ip_addresses_in_dvr_ha_snat_namespace(router1) + def test_dvr_router_static_routes(self): + """Test to validate the extra routes on dvr routers.""" + self.agent.conf.agent_mode = 'dvr_snat' + router_info = self.generate_dvr_router_info(enable_snat=True) + router1 = self.manage_router(self.agent, router_info) + self.assertTrue(self._namespace_exists(router1.ns_name)) + self._assert_snat_namespace_exists(router1) + snat_ns_name = dvr_snat_ns.SnatNamespace.get_snat_ns_name( + router1.router_id) + # Now try to add routes that are suitable for both the + # router namespace and the snat namespace. + router1.router['routes'] = [{'destination': '8.8.4.0/24', + 'nexthop': '35.4.0.20'}] + self.agent._process_updated_router(router1.router) + router_updated = self.agent.router_info[router_info['id']] + self._assert_extra_routes(router_updated, namespace=snat_ns_name) + self._assert_extra_routes(router_updated) + def _assert_fip_namespace_deleted(self, ext_gateway_port): ext_net_id = ext_gateway_port['network_id'] self.agent.fipnamespace_delete_on_ext_net( diff --git a/neutron/tests/unit/agent/l3/test_agent.py b/neutron/tests/unit/agent/l3/test_agent.py index f187f1e41..2953125e5 100644 --- a/neutron/tests/unit/agent/l3/test_agent.py +++ b/neutron/tests/unit/agent/l3/test_agent.py @@ -1096,14 +1096,14 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): router) self.assertEqual(1, self.send_adv_notif.call_count) - def test_update_routing_table(self): - # Just verify the correct namespace was used in the call + def _test_update_routing_table(self, is_snat_host=True): router = l3_test_common.prepare_router_data() uuid = router['id'] - netns = 'snat-' + uuid + s_netns = 'snat-' + uuid + q_netns = 'qrouter-' + uuid fake_route1 = {'destination': '135.207.0.0/16', - 'nexthop': '1.2.3.4'} - + 'nexthop': '19.4.4.200'} + calls = [mock.call('replace', fake_route1, q_netns)] agent = l3_agent.L3NATAgent(HOSTNAME, self.conf) ri = dvr_router.DvrEdgeRouter( agent, @@ -1113,10 +1113,19 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): **self.ri_kwargs) ri._update_routing_table = mock.Mock() - ri.update_routing_table('replace', fake_route1) - ri._update_routing_table.assert_called_once_with('replace', - fake_route1, - netns) + with mock.patch.object(ri, '_is_this_snat_host') as snat_host: + snat_host.return_value = is_snat_host + ri.update_routing_table('replace', fake_route1) + if is_snat_host: + ri._update_routing_table('replace', fake_route1, s_netns) + calls += [mock.call('replace', fake_route1, s_netns)] + ri._update_routing_table.assert_has_calls(calls, any_order=True) + + def test_process_update_snat_routing_table(self): + self._test_update_routing_table() + + def test_process_not_update_snat_routing_table(self): + self._test_update_routing_table(is_snat_host=False) def test_process_router_interface_added(self): agent = l3_agent.L3NATAgent(HOSTNAME, self.conf) diff --git a/neutron/tests/unit/agent/l3/test_router_info.py b/neutron/tests/unit/agent/l3/test_router_info.py index f1378af2d..4921d1519 100644 --- a/neutron/tests/unit/agent/l3/test_router_info.py +++ b/neutron/tests/unit/agent/l3/test_router_info.py @@ -97,7 +97,7 @@ class TestRouterInfo(base.BaseTestCase): 'nexthop': "10.100.10.30"}] ri.routes = fake_old_routes ri.router['routes'] = fake_new_routes - ri.routes_updated() + ri.routes_updated(fake_old_routes, fake_new_routes) expected = [['ip', 'route', 'replace', 'to', '110.100.30.0/24', 'via', '10.100.10.30'], @@ -105,18 +105,18 @@ class TestRouterInfo(base.BaseTestCase): 'via', '10.100.10.30']] self._check_agent_method_called(expected) - + ri.routes = fake_new_routes fake_new_routes = [{'destination': "110.100.30.0/24", 'nexthop': "10.100.10.30"}] ri.router['routes'] = fake_new_routes - ri.routes_updated() + ri.routes_updated(ri.routes, fake_new_routes) expected = [['ip', 'route', 'delete', 'to', '110.100.31.0/24', 'via', '10.100.10.30']] self._check_agent_method_called(expected) fake_new_routes = [] ri.router['routes'] = fake_new_routes - ri.routes_updated() + ri.routes_updated(ri.routes, fake_new_routes) expected = [['ip', 'route', 'delete', 'to', '110.100.30.0/24', 'via', '10.100.10.30']]