Use weak ref to avoid deleting fip namespace through agent

By using a weak ref in the agent to keep track of the fip namespaces,
we avoid having to call a method directly on the agent to delete a fip
namespace.  When the last router removes the last floating ip, the
fip_ns is marked destroyed and the router's strong reference is
removed.  This allows the garbage collector to reap it.  When the
agent goes looking for the fip namespace instance again, it will check
to see that it has not been garbage collected and that it has not been
destroyed before using it.

The goal here is to avoid having to ask the agent to delete a fip
namespace.  We know to delete a fip namespace when all of the floating
ips are gone.  We delete floating ips in the context of processing a
router.  So, having to call back out to the agent to destroy the fip
namespace is preventing some of this code from being moved in to the
router context.

Change-Id: I1b10f5d3233378ef9d8ef7b82659d3e775dac6b7
Partially-Implements: bp/restructure-l3-agent

neutron/agent/l3/agent.py

@@ -735,8 +735,7 @@ class L3NATAgent(firewall_l3_agent.FWaaSL3AgentRpcCallback,
                                                namespace=ri.ns_name,
                                                ip=ip_cidr)
             if ri.router['distributed']:
-                #TODO(Carl) Call this method on ri.  Needs namespace work.
-                self.floating_ip_removed_dist(ri, ip_cidr)
+                ri.floating_ip_removed_dist(ip_cidr)
 
     def _get_router_cidrs(self, ri, device):
         if ri.is_ha:

neutron/agent/l3/dvr.py

@@ -14,6 +14,7 @@
 
 import binascii
 import netaddr
+import weakref
 
 from neutron.agent.l3 import dvr_fip_ns
 from neutron.agent.linux import ip_lib
@@ -33,7 +34,7 @@ MASK_30 = 0x3fffffff
 class AgentMixin(object):
     def __init__(self, host):
         # dvr data
-        self._fip_namespaces = {}
+        self._fip_namespaces = weakref.WeakValueDictionary()
         super(AgentMixin, self).__init__(host)
 
     def get_fip_ns(self, ext_net_id):
@@ -41,15 +42,18 @@ class AgentMixin(object):
         # convert these to string like this so I preserved that.
         ext_net_id = str(ext_net_id)
 
-        if ext_net_id not in self._fip_namespaces:
-            fip_ns = dvr_fip_ns.FipNamespace(ext_net_id,
-                                             self.conf,
-                                             self.driver,
-                                             self.root_helper,
-                                             self.use_ipv6)
-            self._fip_namespaces[ext_net_id] = fip_ns
+        fip_ns = self._fip_namespaces.get(ext_net_id)
+        if fip_ns and not fip_ns.destroyed:
+            return fip_ns
 
-        return self._fip_namespaces[ext_net_id]
+        fip_ns = dvr_fip_ns.FipNamespace(ext_net_id,
+                                         self.conf,
+                                         self.driver,
+                                         self.root_helper,
+                                         self.use_ipv6)
+        self._fip_namespaces[ext_net_id] = fip_ns
+
+        return fip_ns
 
     def _destroy_snat_namespace(self, ns):
         ns_ip = ip_lib.IPWrapper(self.root_helper, namespace=ns)
@@ -68,7 +72,6 @@ class AgentMixin(object):
     def _destroy_fip_namespace(self, ns):
         ex_net_id = ns[len(dvr_fip_ns.FIP_NS_PREFIX):]
         fip_ns = self.get_fip_ns(ex_net_id)
-        del self._fip_namespaces[ex_net_id]
         fip_ns.destroy()
 
     def _set_subnet_arp_info(self, ri, port):
@@ -168,13 +171,6 @@ class AgentMixin(object):
         # kicks the FW Agent to add rules for the snat namespace
         self.process_router_add(ri)
 
-    def floating_ip_removed_dist(self, ri, fip_cidr):
-        """Remove floating IP from FIP namespace."""
-        is_last = ri.floating_ip_removed_dist(fip_cidr)
-        # clean up fip-namespace if this is the last FIP
-        if is_last:
-            self._destroy_fip_namespace(ri.fip_ns.get_name())
-
     def _snat_redirect_add(self, ri, gateway, sn_port, sn_int):
         """Adds rules and routes for SNAT redirection."""
         try:

neutron/agent/l3/dvr_fip_ns.py

@@ -52,6 +52,7 @@ class FipNamespace(object):
             use_ipv6=self.use_ipv6)
         path = os.path.join(agent_conf.state_path, 'fip-linklocal-networks')
         self.local_subnets = lla.LinkLocalAllocator(path, FIP_LL_SUBNET)
+        self.destroyed = False
 
     def get_name(self):
         return (FIP_NS_PREFIX + self._ext_net_id)
@@ -135,6 +136,7 @@ class FipNamespace(object):
         self._iptables_manager.apply()
 
     def destroy(self):
+        self.destroyed = True
         ns = self.get_name()
         # TODO(carl) Reconcile this with mlavelle's namespace work
         # TODO(carl) mlavelle's work has self.ip_wrapper

neutron/agent/l3/dvr_router.py

@@ -99,7 +99,6 @@ class DvrRouter(router.RouterInfo):
         device.route.delete_route(fip_cidr, str(rtr_2_fip.ip))
         # check if this is the last FIP for this router
         self.dist_fip_count = self.dist_fip_count - 1
-        is_last = False
         if self.dist_fip_count == 0:
             #remove default route entry
             device = ip_lib.IPDevice(rtr_2_fip_name,
@@ -113,4 +112,11 @@ class DvrRouter(router.RouterInfo):
             self.rtr_fip_subnet = None
             ns_ip.del_veth(fip_2_rtr_name)
             is_last = self.fip_ns.unsubscribe(self.router_id)
-        return is_last
+            if is_last:
+                # TODO(Carl) I can't help but think that another router could
+                # come in and want to start using this namespace while this is
+                # destroying it.  The two could end up conflicting on
+                # creating/destroying interfaces and such.  I think I'd like a
+                # semaphore to sync creation/deletion of this namespace.
+                self.fip_ns.destroy()
+                self.fip_ns = None

neutron/tests/unit/test_dvr_router.py

@@ -106,9 +106,13 @@ class TestDvrRouterOperations(base.BaseTestCase):
         ri.dist_fip_count = 1
         ri.rtr_fip_subnet = lla.LinkLocalAddressPair('15.1.2.3/32')
         _, fip_to_rtr = ri.rtr_fip_subnet.get_pair()
+        fip_ns = ri.fip_ns
+
         ri.floating_ip_removed_dist(fip_cidr)
+
+        self.assertTrue(fip_ns.destroyed)
         mIPWrapper().del_veth.assert_called_once_with(
-            ri.fip_ns.get_int_device_name(router['id']))
+            fip_ns.get_int_device_name(router['id']))
         mIPDevice().route.delete_gateway.assert_called_once_with(
             str(fip_to_rtr.ip), table=16)
-        ri.fip_ns.unsubscribe.assert_called_once_with(ri.router_id)
+        fip_ns.unsubscribe.assert_called_once_with(ri.router_id)

neutron/tests/unit/test_l3_agent.py

@@ -1845,20 +1845,22 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
             {'cidr': '19.4.4.1/24'}]
         self.device_exists.return_value = True
 
+        fip_ns = ri.fip_ns
         agent.external_gateway_removed(
             ri, ri.ex_gw_port,
             agent.get_external_device_name(ri.ex_gw_port['id']))
 
         self.mock_ip.del_veth.assert_called_once_with(
-            ri.fip_ns.get_int_device_name(ri.router['id']))
+            fip_ns.get_int_device_name(ri.router['id']))
         self.mock_ip_dev.route.delete_gateway.assert_called_once_with(
             str(fip_to_rtr.ip), table=dvr_fip_ns.FIP_RT_TBL)
 
         self.assertEqual(ri.dist_fip_count, 0)
-        self.assertFalse(ri.fip_ns.has_subscribers())
+        self.assertFalse(fip_ns.has_subscribers())
         self.assertEqual(self.mock_driver.unplug.call_count, 1)
-        self.assertIsNone(ri.fip_ns.agent_gateway_port)
-        self.mock_ip.netns.delete.assert_called_once_with(ri.fip_ns.get_name())
+        self.assertIsNone(fip_ns.agent_gateway_port)
+        self.assertTrue(fip_ns.destroyed)
+        self.mock_ip.netns.delete.assert_called_once_with(fip_ns.get_name())
         self.assertFalse(nat.add_rule.called)
         nat.clear_rules_by_tag.assert_called_once_with('floating_ip')