Remove stale ofport drop-rule upon port-delete

When a port is deleted, that port is set to a dead-vlan, and
an ofport drop-flow is added in port_dead().

The ofport drop-flow gets removed only in some cases
in _bind_devices() - depending on the timing of the
concurrent port-deletion. In other cases, the drop-flow
never gets removed, and such garbage drop-flow rules
accumulate forever until the ovs-agent restarts.

The fix is to use the function update_stale_ofport_rules which
solves this problem of tracking stale ofport flows
in deleted ports, but currently only applies only to
prevent_arp_spoofing.

Change-Id: I0d1dbe3918cc7d7b3d0cdc49d7b6ff85f9b02a17
Closes-Bug: #1493414

neutron/plugins/ml2/drivers/openvswitch/agent/ovs_neutron_agent.py

@@ -1124,11 +1124,8 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin,
                                     'options:peer', int_if_name)
 
     def update_stale_ofport_rules(self):
-        # right now the ARP spoofing rules are the only thing that utilizes
-        # ofport-based rules, so make arp_spoofing protection a conditional
-        # until something else uses ofport
-        if not self.prevent_arp_spoofing:
-            return []
+        # ARP spoofing rules and drop-flow upon port-delete
+        # use ofport-based rules
         previous = self.vifname_to_ofport_map
         current = self.int_br.get_vif_port_to_ofport_map()
 
@@ -1139,8 +1136,9 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin,
         # delete any stale rules based on removed ofports
         ofports_deleted = set(previous.values()) - set(current.values())
         for ofport in ofports_deleted:
-            self.int_br.delete_arp_spoofing_protection(port=ofport)
-
+            if self.prevent_arp_spoofing:
+                self.int_br.delete_arp_spoofing_protection(port=ofport)
+            self.int_br.delete_flows(in_port=ofport)
         # store map for next iteration
         self.vifname_to_ofport_map = current
         return moved_ports

neutron/tests/functional/agent/test_l2_ovs_agent.py

@@ -17,7 +17,7 @@
 import time
 
 from eventlet.timeout import Timeout
-
+from neutron.agent.linux import utils as agent_utils
 from neutron.plugins.ml2.drivers.openvswitch.agent.common import constants
 from neutron.tests.common import net_helpers
 from neutron.tests.functional.agent.l2 import base
@@ -35,6 +35,53 @@ class TestOVSAgent(base.OVSAgentTestFramework):
 
         self.wait_until_ports_state(self.ports, up=False)
 
+    def test_no_stale_flows_after_port_delete(self):
+        def find_drop_flow(ofport, flows):
+            for flow in flows.split("\n"):
+                if "in_port=%d" % ofport in flow and "actions=drop" in flow:
+                    return True
+            return False
+
+        def num_ports_with_drop_flows(ofports, flows):
+            count = 0
+            for ofport in ofports:
+                if find_drop_flow(ofport, flows):
+                    count = count + 1
+            return count
+        # setup
+        self.setup_agent_and_ports(
+            port_dicts=self.create_test_ports())
+        self.wait_until_ports_state(self.ports, up=True)
+
+        # call port_delete first
+        for port in self.ports:
+            self.agent.port_delete([], port_id=port['id'])
+        portnames = [port["vif_name"] for port in self.ports]
+        ofports = [port.ofport for port in self.agent.int_br.get_vif_ports()
+                   if port.port_name in portnames]
+
+        #wait until ports are marked dead, with drop flow
+        agent_utils.wait_until_true(
+            lambda: num_ports_with_drop_flows(
+                ofports,
+                self.agent.int_br.dump_flows(
+                    constants.LOCAL_SWITCHING
+                )) == len(ofports))
+
+        #delete the ports on bridge
+        for port in self.ports:
+            self.agent.int_br.delete_port(port['vif_name'])
+        self.wait_until_ports_state(self.ports, up=False)
+
+        #verify no stale drop flows
+        self.assertEqual(0,
+            num_ports_with_drop_flows(
+                ofports,
+                self.agent.int_br.dump_flows(
+                    constants.LOCAL_SWITCHING
+                )
+            ))
+
     def _check_datapath_type_netdev(self, expected, default=False):
         if not default:
             self.config.set_override('datapath_type',

neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_neutron_agent.py

@@ -1762,6 +1762,25 @@ class TestOvsNeutronAgent(object):
         ofport_changed_ports = self.agent.update_stale_ofport_rules()
         self.assertEqual(['port1'], ofport_changed_ports)
 
+    def test_update_stale_ofport_rules_removes_drop_flow(self):
+        self.agent.prevent_arp_spoofing = False
+        self.agent.vifname_to_ofport_map = {'port1': 1, 'port2': 2}
+        self.agent.int_br = mock.Mock()
+        # simulate port1 was removed
+        newmap = {'port2': 2}
+        self.agent.int_br.get_vif_port_to_ofport_map.return_value = newmap
+        self.agent.update_stale_ofport_rules()
+        # drop flow rule matching port 1 should have been deleted
+        ofport_changed_ports = self.agent.update_stale_ofport_rules()
+        expected = [
+            mock.call(in_port=1)
+        ]
+        self.assertEqual(expected, self.agent.int_br.delete_flows.mock_calls)
+        self.assertEqual(newmap, self.agent.vifname_to_ofport_map)
+        self.assertFalse(
+            self.agent.int_br.delete_arp_spoofing_protection.called)
+        self.assertEqual([], ofport_changed_ports)
+
     def test__setup_tunnel_port_while_new_mapping_is_added(self):
         """
         Test that _setup_tunnel_port doesn't fail if new vlan mapping is