From 1584650a0db1620ebd3fe6fe7e45b9f0a045dc4b Mon Sep 17 00:00:00 2001 From: Carl Baldwin Date: Mon, 2 Feb 2015 23:34:24 +0000 Subject: [PATCH] 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 | 3 +-- neutron/agent/l3/dvr.py | 30 ++++++++++++--------------- neutron/agent/l3/dvr_fip_ns.py | 2 ++ neutron/agent/l3/dvr_router.py | 10 +++++++-- neutron/tests/unit/test_dvr_router.py | 8 +++++-- neutron/tests/unit/test_l3_agent.py | 10 +++++---- 6 files changed, 36 insertions(+), 27 deletions(-) diff --git a/neutron/agent/l3/agent.py b/neutron/agent/l3/agent.py index dc40135e4..5641da8e5 100644 --- 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 0e4fa981a..64eed2552 100644 --- 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 52bd59b3f..b11dc1345 100644 --- 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 7b44e7df8..2eca3da65 100644 --- 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 4552d73d7..e44328348 100644 --- 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 b066a1761..820a12435 100644 --- 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') -- 2.45.2