Merge "DVR: Clean stale snat-ns by checking its existence when agent restarts" into stable/liberty

8 files changed, 117 insertions, 18 deletions

diff --git a/neutron/agent/l3/agent.py b/neutron/agent/l3/agent.py
index 7f97a9979e..b0960910f6 100644
--- a/neutron/agent/l3/agent.py
+++ b/neutron/agent/l3/agent.py
@@ -573,8 +573,12 @@ class L3NATAgent(firewall_l3_agent.FWaaSL3AgentRpcCallback,
                     # need to keep fip namespaces as well
                     ext_net_id = (r['external_gateway_info'] or {}).get(
                         'network_id')
+                    is_snat_agent = (self.conf.agent_mode ==
+                        l3_constants.L3_AGENT_MODE_DVR_SNAT)
                     if ext_net_id:
                         ns_manager.keep_ext_net(ext_net_id)
+                    elif is_snat_agent:
+                            ns_manager.ensure_snat_cleanup(r['id'])
                 update = queue.RouterUpdate(r['id'],
                                             queue.PRIORITY_SYNC_ROUTERS_TASK,
                                             router=r,
diff --git a/neutron/agent/l3/dvr_edge_router.py b/neutron/agent/l3/dvr_edge_router.py
index 33b8673bc2..37a165d40f 100644
--- a/neutron/agent/l3/dvr_edge_router.py
+++ b/neutron/agent/l3/dvr_edge_router.py
@@ -28,7 +28,8 @@ class DvrEdgeRouter(dvr_local_router.DvrLocalRouter):
 
     def __init__(self, agent, host, *args, **kwargs):
         super(DvrEdgeRouter, self).__init__(agent, host, *args, **kwargs)
-        self.snat_namespace = None
+        self.snat_namespace = dvr_snat_ns.SnatNamespace(
+            self.router_id, self.agent_conf, self.driver, self.use_ipv6)
         self.snat_iptables_manager = None
 
     def external_gateway_added(self, ex_gw_port, interface_name):
@@ -41,19 +42,27 @@ class DvrEdgeRouter(dvr_local_router.DvrLocalRouter):
             # 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'])
+        elif self.snat_namespace.exists():
+            # This is the case where the snat was moved manually or
+            # rescheduled to a different agent when the agent was dead.
+            LOG.debug("SNAT was moved or rescheduled to a different host "
+                      "and does not match with the current host. This is "
+                      "a stale namespace %s and will be cleared from the "
+                      "current dvr_snat host.", self.snat_namespace.name)
+            self.external_gateway_removed(ex_gw_port, interface_name)
 
     def external_gateway_updated(self, ex_gw_port, interface_name):
         if not self._is_this_snat_host():
             # no centralized SNAT gateway for this node/agent
             LOG.debug("not hosting snat for router: %s", self.router['id'])
-            if self.snat_namespace:
+            if self.snat_namespace.exists():
                 LOG.debug("SNAT was rescheduled to host %s. Clearing snat "
                           "namespace.", self.router.get('gw_port_host'))
                 return self.external_gateway_removed(
                     ex_gw_port, interface_name)
             return
 
-        if not self.snat_namespace:
+        if not self.snat_namespace.exists():
             # SNAT might be rescheduled to this agent; need to process like
             # newly created gateway
             return self.external_gateway_added(ex_gw_port, interface_name)
@@ -66,7 +75,7 @@ class DvrEdgeRouter(dvr_local_router.DvrLocalRouter):
     def external_gateway_removed(self, ex_gw_port, interface_name):
         super(DvrEdgeRouter, self).external_gateway_removed(ex_gw_port,
                                                             interface_name)
-        if not self._is_this_snat_host() and not self.snat_namespace:
+        if not self._is_this_snat_host() and not self.snat_namespace.exists():
             # no centralized SNAT gateway for this node/agent
             LOG.debug("not hosting snat for router: %s", self.router['id'])
             return
@@ -75,8 +84,9 @@ class DvrEdgeRouter(dvr_local_router.DvrLocalRouter):
                            bridge=self.agent_conf.external_network_bridge,
                            namespace=self.snat_namespace.name,
                            prefix=router.EXTERNAL_DEV_PREFIX)
-        self.snat_namespace.delete()
-        self.snat_namespace = None
+
+        if self.snat_namespace.exists():
+            self.snat_namespace.delete()
 
     def internal_network_added(self, port):
         super(DvrEdgeRouter, self).internal_network_added(port)
@@ -147,10 +157,6 @@ class DvrEdgeRouter(dvr_local_router.DvrLocalRouter):
         # TODO(mlavalle): in the near future, this method should contain the
         # code in the L3 agent that creates a gateway for a dvr. The first step
         # is to move the creation of the snat namespace here
-        self.snat_namespace = dvr_snat_ns.SnatNamespace(self.router['id'],
-                                                        self.agent_conf,
-                                                        self.driver,
-                                                        self.use_ipv6)
         self.snat_namespace.create()
         return self.snat_namespace
 
@@ -185,12 +191,10 @@ class DvrEdgeRouter(dvr_local_router.DvrLocalRouter):
 
     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'])
+            ns_name = self.snat_namespace.name
             # 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):
+            if self.snat_namespace.exists():
                 super(DvrEdgeRouter, self)._update_routing_table(
                     operation, route, namespace=ns_name)
             else:
@@ -200,5 +204,5 @@ class DvrEdgeRouter(dvr_local_router.DvrLocalRouter):
 
     def delete(self, agent):
         super(DvrEdgeRouter, self).delete(agent)
-        if self.snat_namespace:
+        if self.snat_namespace.exists():
             self.snat_namespace.delete()
diff --git a/neutron/agent/l3/dvr_router_base.py b/neutron/agent/l3/dvr_router_base.py
index 704be6ce6e..53f10abfbd 100644
--- a/neutron/agent/l3/dvr_router_base.py
+++ b/neutron/agent/l3/dvr_router_base.py
@@ -25,6 +25,7 @@ class DvrRouterBase(router.RouterInfo):
 
         self.agent = agent
         self.host = host
+        self.snat_ports = None
 
     def process(self, agent):
         super(DvrRouterBase, self).process(agent)
diff --git a/neutron/agent/l3/namespace_manager.py b/neutron/agent/l3/namespace_manager.py
index 24e665533f..a58fe9bea2 100644
--- a/neutron/agent/l3/namespace_manager.py
+++ b/neutron/agent/l3/namespace_manager.py
@@ -132,6 +132,10 @@ class NamespaceManager(object):
                 ns_prefix, ns_id = self.get_prefix_and_id(ns)
                 self._cleanup(ns_prefix, ns_id)
 
+    def ensure_snat_cleanup(self, router_id):
+        prefix = dvr_snat_ns.SNAT_NS_PREFIX
+        self._cleanup(prefix, router_id)
+
     def _cleanup(self, ns_prefix, ns_id):
         ns_class = self.ns_prefix_to_class_map[ns_prefix]
         ns = ns_class(ns_id, self.agent_conf, self.driver, use_ipv6=False)
diff --git a/neutron/agent/l3/namespaces.py b/neutron/agent/l3/namespaces.py
index 1f85fc149d..e2f2f3a5cf 100644
--- a/neutron/agent/l3/namespaces.py
+++ b/neutron/agent/l3/namespaces.py
@@ -83,6 +83,9 @@ class Namespace(object):
                 msg = _LE('Failed trying to delete namespace: %s')
                 LOG.exception(msg, self.name)
 
+    def exists(self):
+        return self.ip_wrapper_root.netns.exists(self.name)
+
 
 class RouterNamespace(Namespace):
 
diff --git a/neutron/tests/functional/agent/test_l3_agent.py b/neutron/tests/functional/agent/test_l3_agent.py
index f5b9185e86..879645fe38 100644
--- a/neutron/tests/functional/agent/test_l3_agent.py
+++ b/neutron/tests/functional/agent/test_l3_agent.py
@@ -1199,6 +1199,23 @@ class TestDvrRouter(L3AgentTestFramework):
         self._delete_router(self.agent, router.router_id)
         self._assert_fip_namespace_deleted(ext_gateway_port)
 
+    def test_dvr_unused_snat_ns_deleted_when_agent_restarts_after_move(self):
+        """Test to validate the stale snat namespace delete with snat move.
+
+        This test validates the stale snat namespace cleanup when
+        the agent restarts after the gateway port has been moved
+        from the agent.
+        """
+        self.agent.conf.agent_mode = 'dvr_snat'
+        router_info = self.generate_dvr_router_info()
+        router1 = self.manage_router(self.agent, router_info)
+        self._assert_snat_namespace_exists(router1)
+        restarted_agent = neutron_l3_agent.L3NATAgentWithStateReport(
+            self.agent.host, self.agent.conf)
+        router1.router['gw_port_host'] = "my-new-host"
+        restarted_router = self.manage_router(restarted_agent, router1.router)
+        self._assert_snat_namespace_does_not_exist(restarted_router)
+
     def test_dvr_update_floatingip_statuses(self):
         self.agent.conf.agent_mode = 'dvr'
         self._test_update_floatingip_statuses(self.generate_dvr_router_info())
diff --git a/neutron/tests/unit/agent/l3/test_agent.py b/neutron/tests/unit/agent/l3/test_agent.py
index 1376adaca0..17f05e2e82 100644
--- a/neutron/tests/unit/agent/l3/test_agent.py
+++ b/neutron/tests/unit/agent/l3/test_agent.py
@@ -617,6 +617,68 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
     def test_external_gateway_updated_dual_stack(self):
         self._test_external_gateway_updated(dual_stack=True)
 
+    def test_dvr_edge_router_init_for_snat_namespace_object(self):
+        router = {'id': _uuid()}
+        ri = dvr_router.DvrEdgeRouter(mock.Mock(),
+                                      HOSTNAME,
+                                      router['id'],
+                                      router,
+                                      **self.ri_kwargs)
+        # Make sure that ri.snat_namespace object is created when the
+        # router is initialized
+        self.assertIsNotNone(ri.snat_namespace)
+
+    def test_ext_gw_updated_calling_snat_ns_delete_if_gw_port_host_none(
+        self):
+        """Test to check the impact of snat_namespace object.
+
+        This function specifically checks the impact of the snat
+        namespace object value on external_gateway_removed for deleting
+        snat_namespace when the gw_port_host mismatches or none.
+        """
+        router = l3_test_common.prepare_router_data(num_internal_ports=2)
+        ri = dvr_router.DvrEdgeRouter(mock.Mock(),
+                                      HOSTNAME,
+                                      router['id'],
+                                      router,
+                                      **self.ri_kwargs)
+        with mock.patch.object(dvr_snat_ns.SnatNamespace,
+                               'delete') as snat_ns_delete:
+            interface_name, ex_gw_port = l3_test_common.prepare_ext_gw_test(
+                self, ri)
+            router['gw_port_host'] = ''
+            ri.external_gateway_updated(ex_gw_port, interface_name)
+            if router['gw_port_host'] != ri.host:
+                self.assertEqual(1, snat_ns_delete.call_count)
+
+    @mock.patch.object(namespaces.Namespace, 'delete')
+    def test_snat_ns_delete_not_called_when_snat_namespace_does_not_exist(
+        self, mock_ns_del):
+        """Test to check the impact of snat_namespace object.
+
+        This function specifically checks the impact of the snat
+        namespace object initialization without the actual creation
+        of snat_namespace. When deletes are issued to the snat
+        namespace based on the snat namespace object existence, it
+        should be checking for the valid namespace existence before
+        it tries to delete.
+        """
+        router = l3_test_common.prepare_router_data(num_internal_ports=2)
+        ri = dvr_router.DvrEdgeRouter(mock.Mock(),
+                                      HOSTNAME,
+                                      router['id'],
+                                      router,
+                                      **self.ri_kwargs)
+        # Make sure we set a return value to emulate the non existence
+        # of the namespace.
+        self.mock_ip.netns.exists.return_value = False
+        self.assertIsNotNone(ri.snat_namespace)
+        interface_name, ex_gw_port = l3_test_common.prepare_ext_gw_test(self,
+                                                                        ri)
+        ri._external_gateway_removed = mock.Mock()
+        ri.external_gateway_removed(ex_gw_port, interface_name)
+        self.assertFalse(mock_ns_del.called)
+
     def _test_ext_gw_updated_dvr_edge_router(self, host_match,
                                              snat_hosted_before=True):
         """
@@ -635,8 +697,6 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
         if snat_hosted_before:
             ri._create_snat_namespace()
             snat_ns_name = ri.snat_namespace.name
-        else:
-            self.assertIsNone(ri.snat_namespace)
 
         interface_name, ex_gw_port = l3_test_common.prepare_ext_gw_test(self,
                                                                         ri)
@@ -656,7 +716,6 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
                     bridge=self.conf.external_network_bridge,
                     namespace=snat_ns_name,
                     prefix=l3_agent.EXTERNAL_DEV_PREFIX)
-                self.assertIsNone(ri.snat_namespace)
         else:
             if not snat_hosted_before:
                 self.assertIsNotNone(ri.snat_namespace)
diff --git a/neutron/tests/unit/agent/l3/test_namespace_manager.py b/neutron/tests/unit/agent/l3/test_namespace_manager.py
index 228ffdad4a..a3caa27b1e 100644
--- a/neutron/tests/unit/agent/l3/test_namespace_manager.py
+++ b/neutron/tests/unit/agent/l3/test_namespace_manager.py
@@ -92,6 +92,13 @@ class TestNamespaceManager(NamespaceManagerTestCaseFramework):
             retrieved_ns_names = self.ns_manager.list_all()
         self.assertFalse(retrieved_ns_names)
 
+    def test_ensure_snat_cleanup(self):
+        router_id = _uuid()
+        with mock.patch.object(self.ns_manager, '_cleanup') as mock_cleanup:
+            self.ns_manager.ensure_snat_cleanup(router_id)
+            mock_cleanup.assert_called_once_with(dvr_snat_ns.SNAT_NS_PREFIX,
+                                                 router_id)
+
     def test_ensure_router_cleanup(self):
         router_id = _uuid()
         ns_names = [namespaces.NS_PREFIX + _uuid() for _ in range(5)]