From 530c1d30875c0ca2936303ed6e396d83018aaed6 Mon Sep 17 00:00:00 2001 From: Swaminathan Vasudevan Date: Wed, 20 Jan 2016 13:35:52 -0800 Subject: [PATCH] DVR: Fix Duplicate IPtables rule detected warning message in l3agent Duplicate IPtables rule detected warning message is seen in the l3 agent logs for sometime. This will be seen when multiple floatingips are created on the same node for different routers or when a floatingip is disassociated and re-associated to a fixed-ip on the same node. The fip namespace is retained in the compute node even though the floatingip is disassociated, but when we try to re-associate or create a new floatingip the code in l3agent is trying to check, if this is the 'first' floatingip and if so tries to re-create the floatingip namespace and the rules within it. This happens because we are unsubscribing the fip namespace count for every associated routers that we are deleting. This duplicate call to create the fip namespace should be restricted if there is already a fip namespace in the compute node and the fip namespace should be unsubscribed only when the external network is removed before the actual fip namespace is deleted. The change proposed in this fix, will only unsubscribe the fip namespace before it is deleted. Closes-Bug: #1535928 Change-Id: I24016382091cad485f65e7753972f4b71702ff9f --- neutron/agent/l3/dvr.py | 1 + neutron/agent/l3/dvr_fip_ns.py | 8 +-- neutron/agent/l3/dvr_local_router.py | 5 +- .../functional/agent/l3/test_dvr_router.py | 7 +- neutron/tests/unit/agent/l3/test_agent.py | 68 ++++++++++++++++++- .../tests/unit/agent/l3/test_dvr_fip_ns.py | 16 ++--- .../unit/agent/l3/test_dvr_local_router.py | 3 +- 7 files changed, 88 insertions(+), 20 deletions(-) diff --git a/neutron/agent/l3/dvr.py b/neutron/agent/l3/dvr.py index bb4c1a43a58..38f01b4a69f 100644 --- a/neutron/agent/l3/dvr.py +++ b/neutron/agent/l3/dvr.py @@ -74,4 +74,5 @@ class AgentMixin(object): """Delete fip namespace after external network removed.""" fip_ns = self.get_fip_ns(ext_net_id) if fip_ns.agent_gateway_port and not fip_ns.destroyed: + fip_ns.unsubscribe(ext_net_id) fip_ns.delete() diff --git a/neutron/agent/l3/dvr_fip_ns.py b/neutron/agent/l3/dvr_fip_ns.py index 6d4cf7bc3dc..654a7d24365 100644 --- a/neutron/agent/l3/dvr_fip_ns.py +++ b/neutron/agent/l3/dvr_fip_ns.py @@ -79,13 +79,13 @@ class FipNamespace(namespaces.Namespace): def has_subscribers(self): return len(self._subscribers) != 0 - def subscribe(self, router_id): + def subscribe(self, external_net_id): is_first = not self.has_subscribers() - self._subscribers.add(router_id) + self._subscribers.add(external_net_id) return is_first - def unsubscribe(self, router_id): - self._subscribers.discard(router_id) + def unsubscribe(self, external_net_id): + self._subscribers.discard(external_net_id) return not self.has_subscribers() def allocate_rule_priority(self, floating_ip): diff --git a/neutron/agent/l3/dvr_local_router.py b/neutron/agent/l3/dvr_local_router.py index 563c88a9220..93101edf66a 100644 --- a/neutron/agent/l3/dvr_local_router.py +++ b/neutron/agent/l3/dvr_local_router.py @@ -133,7 +133,6 @@ class DvrLocalRouter(dvr_router_base.DvrRouterBase): self.fip_ns.local_subnets.release(self.router_id) self.rtr_fip_subnet = None ns_ip.del_veth(fip_2_rtr_name) - self.fip_ns.unsubscribe(self.router_id) # NOTE (Swami): The fg interface and the namespace will be deleted # when the external gateway port is removed. This will be # initiated from the server through an RPC call. @@ -429,7 +428,7 @@ class DvrLocalRouter(dvr_router_base.DvrRouterBase): "plugin: %s", fip_agent_port) is_first = False if floating_ips: - is_first = self.fip_ns.subscribe(self.router_id) + is_first = self.fip_ns.subscribe(ex_gw_port['network_id']) if is_first and not fip_agent_port: LOG.debug("No FloatingIP agent gateway port possibly due to " "late binding of the private port to the host, " @@ -448,7 +447,7 @@ class DvrLocalRouter(dvr_router_base.DvrRouterBase): self.fip_ns.create_gateway_port(fip_agent_port) if (self.fip_ns.agent_gateway_port and - (self.dist_fip_count == 0 or is_first)): + (self.dist_fip_count == 0)): self.fip_ns.create_rtr_2_fip_link(self) # kicks the FW Agent to add rules for the IR namespace if diff --git a/neutron/tests/functional/agent/l3/test_dvr_router.py b/neutron/tests/functional/agent/l3/test_dvr_router.py index e7c5beb59bb..9ef421714b1 100644 --- a/neutron/tests/functional/agent/l3/test_dvr_router.py +++ b/neutron/tests/functional/agent/l3/test_dvr_router.py @@ -19,7 +19,6 @@ import mock import netaddr from neutron.agent.l3 import agent as neutron_l3_agent -from neutron.agent.l3 import dvr_fip_ns from neutron.agent.l3 import dvr_snat_ns from neutron.agent.l3 import namespaces from neutron.agent.linux import ip_lib @@ -673,8 +672,12 @@ class TestDvrRouter(framework.L3AgentTestFramework): def _assert_fip_namespace_deleted(self, ext_gateway_port): ext_net_id = ext_gateway_port['network_id'] + fip_ns = self.agent.get_fip_ns(ext_net_id) + fip_ns.unsubscribe = mock.Mock() self.agent.fipnamespace_delete_on_ext_net( self.agent.context, ext_net_id) self._assert_interfaces_deleted_from_ovs() - fip_ns_name = dvr_fip_ns.FipNamespace._get_ns_name(ext_net_id) + fip_ns_name = fip_ns.get_name() self.assertFalse(self._namespace_exists(fip_ns_name)) + self.assertTrue(fip_ns.destroyed) + self.assertTrue(fip_ns.unsubscribe.called) diff --git a/neutron/tests/unit/agent/l3/test_agent.py b/neutron/tests/unit/agent/l3/test_agent.py index 229114a2156..0f01570a0d6 100644 --- a/neutron/tests/unit/agent/l3/test_agent.py +++ b/neutron/tests/unit/agent/l3/test_agent.py @@ -889,6 +889,72 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): ri.add_floating_ip.assert_called_once_with( floating_ips[0], mock.sentinel.interface_name, device) + @mock.patch.object(lla.LinkLocalAllocator, '_write') + def test_create_dvr_fip_interfaces_if_fipnamespace_exist(self, lla_write): + fake_network_id = _uuid() + subnet_id = _uuid() + fake_floatingips = {'floatingips': [ + {'id': _uuid(), + 'floating_ip_address': '20.0.0.3', + 'fixed_ip_address': '192.168.0.1', + 'floating_network_id': _uuid(), + 'port_id': _uuid(), + 'host': HOSTNAME}]} + agent_gateway_port = ( + [{'fixed_ips': [ + {'ip_address': '20.0.0.30', + 'prefixlen': 24, + 'subnet_id': subnet_id}], + 'subnets': [ + {'id': subnet_id, + 'gateway_ip': '20.0.0.1'}], + 'id': _uuid(), + 'network_id': fake_network_id, + 'mac_address': 'ca:fe:de:ad:be:ef'}] + ) + + router = l3_test_common.prepare_router_data(enable_snat=True) + router[l3_constants.FLOATINGIP_KEY] = fake_floatingips['floatingips'] + router[l3_constants.FLOATINGIP_AGENT_INTF_KEY] = agent_gateway_port + router['distributed'] = True + agent = l3_agent.L3NATAgent(HOSTNAME, self.conf) + ri = dvr_router.DvrEdgeRouter( + agent, HOSTNAME, router['id'], router, **self.ri_kwargs) + ext_gw_port = ri.router.get('gw_port') + ri.fip_ns = agent.get_fip_ns(ext_gw_port['network_id']) + ri.dist_fip_count = 0 + agent.process_router_add = mock.Mock() + ri.fip_ns.create_rtr_2_fip_link = mock.Mock() + with mock.patch.object(ri, 'get_floating_ips') as fips, \ + mock.patch.object(ri.fip_ns, + 'create') as create_fip, \ + mock.patch.object(ri, 'get_floating_agent_gw_interface' + ) as fip_gw_port: + fips.return_value = fake_floatingips + fip_gw_port.return_value = agent_gateway_port[0] + ri.create_dvr_fip_interfaces(ext_gw_port) + self.assertTrue(fip_gw_port.called) + self.assertTrue(fips.called) + self.assertTrue(create_fip.called) + self.assertEqual(agent_gateway_port[0], + ri.fip_ns.agent_gateway_port) + # Now let us associate the fip to the router + ri.floating_ip_added_dist(fips, "192.168.0.1/32") + self.assertEqual(1, ri.dist_fip_count) + # Now let us disassociate the fip from the router + ri.floating_ip_removed_dist("192.168.0.1/32") + self.assertEqual(0, ri.dist_fip_count) + # Calling create_dvr_fip_interfaces again to make sure + # that the fip namespace create is not called again. + # If the create is not called again, that would contain + # the duplicate rules configuration in the fip namespace. + ri.create_dvr_fip_interfaces(ext_gw_port) + self.assertTrue(fip_gw_port.called) + self.assertTrue(fips.called) + create_fip.assert_called_once_with() + self.assertEqual(2, agent.process_router_add.call_count) + self.assertEqual(2, ri.fip_ns.create_rtr_2_fip_link.call_count) + @mock.patch.object(lla.LinkLocalAllocator, '_write') def test_create_dvr_fip_interfaces_for_late_binding(self, lla_write): fake_network_id = _uuid() @@ -1020,7 +1086,7 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): ri.fip_ns.subscribe = mock.Mock(return_value=True) ri.fip_ns.agent_router_gateway = mock.Mock() ri.rtr_fip_subnet = None - ri.dist_fip_count = 1 + ri.dist_fip_count = 0 with mock.patch.object(ri, 'get_floating_ips') as fips,\ mock.patch.object(ri, 'get_floating_agent_gw_interface' diff --git a/neutron/tests/unit/agent/l3/test_dvr_fip_ns.py b/neutron/tests/unit/agent/l3/test_dvr_fip_ns.py index e5b2f9ac188..5b67e56787d 100644 --- a/neutron/tests/unit/agent/l3/test_dvr_fip_ns.py +++ b/neutron/tests/unit/agent/l3/test_dvr_fip_ns.py @@ -39,23 +39,23 @@ class TestDvrFipNs(base.BaseTestCase): use_ipv6=True) def test_subscribe(self): - is_first = self.fip_ns.subscribe(mock.sentinel.router_id) + is_first = self.fip_ns.subscribe(mock.sentinel.external_net_id) self.assertTrue(is_first) def test_subscribe_not_first(self): - self.fip_ns.subscribe(mock.sentinel.router_id) - is_first = self.fip_ns.subscribe(mock.sentinel.router_id2) + self.fip_ns.subscribe(mock.sentinel.external_net_id) + is_first = self.fip_ns.subscribe(mock.sentinel.external_net_id2) self.assertFalse(is_first) def test_unsubscribe(self): - self.fip_ns.subscribe(mock.sentinel.router_id) - is_last = self.fip_ns.unsubscribe(mock.sentinel.router_id) + self.fip_ns.subscribe(mock.sentinel.external_net_id) + is_last = self.fip_ns.unsubscribe(mock.sentinel.external_net_id) self.assertTrue(is_last) def test_unsubscribe_not_last(self): - self.fip_ns.subscribe(mock.sentinel.router_id) - self.fip_ns.subscribe(mock.sentinel.router_id2) - is_last = self.fip_ns.unsubscribe(mock.sentinel.router_id2) + self.fip_ns.subscribe(mock.sentinel.external_net_id) + self.fip_ns.subscribe(mock.sentinel.external_net_id2) + is_last = self.fip_ns.unsubscribe(mock.sentinel.external_net_id2) self.assertFalse(is_last) def test_allocate_rule_priority(self): diff --git a/neutron/tests/unit/agent/l3/test_dvr_local_router.py b/neutron/tests/unit/agent/l3/test_dvr_local_router.py index b200cf57e87..95229a93c77 100644 --- a/neutron/tests/unit/agent/l3/test_dvr_local_router.py +++ b/neutron/tests/unit/agent/l3/test_dvr_local_router.py @@ -252,7 +252,6 @@ class TestDvrRouterOperations(base.BaseTestCase): mIPRule().rule.delete.assert_called_with( ip=str(netaddr.IPNetwork(fip_cidr).ip), table=16, priority=FIP_PRI) mIPDevice().route.delete_route.assert_called_with(fip_cidr, str(s.ip)) - self.assertFalse(ri.fip_ns.unsubscribe.called) ri.fip_ns.local_subnets.allocate.assert_not_called() ri.dist_fip_count = 1 @@ -267,7 +266,7 @@ class TestDvrRouterOperations(base.BaseTestCase): fip_ns.get_int_device_name(router['id'])) mIPDevice().route.delete_gateway.assert_called_once_with( str(fip_to_rtr.ip), table=16) - fip_ns.unsubscribe.assert_called_once_with(ri.router_id) + self.assertFalse(ri.fip_ns.unsubscribe.called) ri.fip_ns.local_subnets.allocate.assert_called_once_with(ri.router_id) def _test_add_floating_ip(self, ri, fip, is_failure):