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

diff --git a/neutron/agent/l3/agent.py b/neutron/agent/l3/agent.py
index dc40135e47eb9511c3f1a8fde707c32492808c82..5641da8e5c5001871c1795c627e9511820fc754d 100644 (file)
--- a/neutron/agent/l3/agent.py
+++ b/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:

diff --git a/neutron/agent/l3/dvr.py b/neutron/agent/l3/dvr.py
index 0e4fa981abfedb337b55b35470fceb2dfd0c9118..64eed255219e061d74dddfdfa87d76f636ae8320 100644 (file)
--- a/neutron/agent/l3/dvr.py
+++ b/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:

diff --git a/neutron/agent/l3/dvr_fip_ns.py b/neutron/agent/l3/dvr_fip_ns.py
index 52bd59b3fe88e030fd18eb634eb5c9520726cb71..b11dc1345843879a88aff93bb3bf8e6aea8a0c7d 100644 (file)
--- a/neutron/agent/l3/dvr_fip_ns.py
+++ b/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

diff --git a/neutron/agent/l3/dvr_router.py b/neutron/agent/l3/dvr_router.py
index 7b44e7df8c1d54fb78617f138cda2341578e1a92..2eca3da6507644d774795de875c4dcb5e287308c 100644 (file)
--- a/neutron/agent/l3/dvr_router.py
+++ b/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

diff --git a/neutron/tests/unit/test_dvr_router.py b/neutron/tests/unit/test_dvr_router.py
index 4552d73d7a2c1999f2de369315bd25c3685ffc0e..e443283487f4bc0e2c7d1f37c232e48c014c2903 100644 (file)
--- a/neutron/tests/unit/test_dvr_router.py
+++ b/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)

diff --git a/neutron/tests/unit/test_l3_agent.py b/neutron/tests/unit/test_l3_agent.py
index b066a1761b143c0dc1af891d91e2ec872cab46e1..820a1243593d93ff1e850072d3b54a6c928ad28c 100644 (file)
--- a/neutron/tests/unit/test_l3_agent.py
+++ b/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')