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

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)

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)

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()

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]
 

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(

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)

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']]