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

@@ -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()

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):

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

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)

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'

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):

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):