Don't delete port from bridge on delete_port event

Commit d6a55c1736 added
logic to the OVS agent to delete a port from the integration
bridge when a port was deleted on the Neutron side. However,
this led to several races where whoever created the initial
port (e.g. Nova, L3 agent, DHCP agent) would be trying to
remove the port from the bridge at the same time. These
would result in ugly exceptions on one side or the other.

The original commit was trying to address the problem where
the port would maintain connectivity even though it was removed
from the integration bridge.

This patch addresses both cases by removing the iptables rules
for the deleted port and putting it in the dead VLAN so it loses
connectivity. However, it still leaves the port attached to the
integration bridge so the original creator can delete it.

Related-Bug: #1333365
Closes-Bug: #1448148
Change-Id: I7ae7750b7ac7d15325ed9f2d517ca171543b53be

neutron/agent/common/ovs_lib.py

@@ -129,16 +129,17 @@ class BaseOVS(object):
         return self.ovsdb.br_get_external_id(bridge, 'bridge-id').execute()
 
     def set_db_attribute(self, table_name, record, column, value,
-                         check_error=False):
+                         check_error=False, log_errors=True):
         self.ovsdb.db_set(table_name, record, (column, value)).execute(
-            check_error=check_error)
+            check_error=check_error, log_errors=log_errors)
 
     def clear_db_attribute(self, table_name, record, column):
         self.ovsdb.db_clear(table_name, record, column).execute()
 
-    def db_get_val(self, table, record, column, check_error=False):
+    def db_get_val(self, table, record, column, check_error=False,
+                   log_errors=True):
         return self.ovsdb.db_get(table, record, column).execute(
-            check_error=check_error)
+            check_error=check_error, log_errors=log_errors)
 
 
 class OVSBridge(BaseOVS):

neutron/plugins/openvswitch/agent/ovs_neutron_agent.py

@@ -207,6 +207,8 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin,
         self.setup_integration_br()
         # Stores port update notifications for processing in main rpc loop
         self.updated_ports = set()
+        # Stores port delete notifications
+        self.deleted_ports = set()
         # keeps association between ports and ofports to detect ofport change
         self.vifname_to_ofport_map = {}
         self.setup_rpc()
@@ -366,10 +368,21 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin,
 
     def port_delete(self, context, **kwargs):
         port_id = kwargs.get('port_id')
-        port = self.int_br.get_vif_port_by_id(port_id)
-        # If port exists, delete it
-        if port:
-            self.int_br.delete_port(port.port_name)
+        self.deleted_ports.add(port_id)
+        LOG.debug("port_delete message processed for port %s", port_id)
+
+    def process_deleted_ports(self):
+        while self.deleted_ports:
+            port_id = self.deleted_ports.pop()
+            # Flush firewall rules and move to dead VLAN so deleted ports no
+            # longer have access to the network
+            self.sg_agent.remove_devices_filter([port_id])
+            port = self.int_br.get_vif_port_by_id(port_id)
+            if port:
+                # don't log errors since there is a chance someone will be
+                # removing the port from the bridge at the same time
+                self.port_dead(port, log_errors=False)
+            self.port_unbound(port_id)
 
     def tunnel_update(self, context, **kwargs):
         LOG.debug("tunnel_update received")
@@ -791,16 +804,17 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin,
         if not lvm.vif_ports:
             self.reclaim_local_vlan(net_uuid)
 
-    def port_dead(self, port):
+    def port_dead(self, port, log_errors=True):
         '''Once a port has no binding, put it on the "dead vlan".
 
         :param port: a ovs_lib.VifPort object.
         '''
         # Don't kill a port if it's already dead
-        cur_tag = self.int_br.db_get_val("Port", port.port_name, "tag")
+        cur_tag = self.int_br.db_get_val("Port", port.port_name, "tag",
+                                         log_errors=log_errors)
         if cur_tag != DEAD_VLAN_TAG:
             self.int_br.set_db_attribute("Port", port.port_name, "tag",
-                                         DEAD_VLAN_TAG)
+                                         DEAD_VLAN_TAG, log_errors=log_errors)
             self.int_br.drop_port(in_port=port.ofport)
 
     def setup_integration_br(self):
@@ -1502,6 +1516,10 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin,
                     self.updated_ports = set()
                     reg_ports = (set() if ovs_restarted else ports)
                     port_info = self.scan_ports(reg_ports, updated_ports_copy)
+                    # don't try to process removed ports as deleted ports since
+                    # they are already gone
+                    self.deleted_ports -= port_info['removed']
+                    self.process_deleted_ports()
                     self.update_stale_ofport_rules()
                     LOG.debug("Agent rpc_loop - iteration:%(iter_num)d - "
                               "port information retrieved. "

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

@@ -221,7 +221,8 @@ class TestOvsNeutronAgent(object):
         else:
             int_br.assert_has_calls([
                 mock.call.set_db_attribute("Port", mock.ANY, "tag",
-                                           self.mod_agent.DEAD_VLAN_TAG),
+                                           self.mod_agent.DEAD_VLAN_TAG,
+                                           log_errors=True),
                 mock.call.drop_port(in_port=port.ofport),
             ])
 
@@ -532,18 +533,19 @@ class TestOvsNeutronAgent(object):
         self.assertEqual(set(['123']), self.agent.updated_ports)
 
     def test_port_delete(self):
-        port_id = "123"
-        port_name = "foo"
-        with mock.patch.object(
-            self.agent.int_br,
-            'get_vif_port_by_id',
-            return_value=mock.MagicMock(port_name=port_name)) as get_vif_func,\
-                mock.patch.object(self.agent.int_br,
-                                  "delete_port") as del_port_func:
+        vif = FakeVif()
+        with mock.patch.object(self.agent, 'int_br') as int_br:
+            int_br.get_vif_by_port_id.return_value = vif.port_name
+            int_br.get_vif_port_by_id.return_value = vif
             self.agent.port_delete("unused_context",
-                                   port_id=port_id)
-            self.assertTrue(get_vif_func.called)
-            del_port_func.assert_called_once_with(port_name)
+                                   port_id='id')
+            self.agent.process_deleted_ports()
+            # the main things we care about are that it gets put in the
+            # dead vlan and gets blocked
+            int_br.set_db_attribute.assert_any_call(
+                'Port', vif.port_name, 'tag', self.mod_agent.DEAD_VLAN_TAG,
+                log_errors=False)
+            int_br.drop_port.assert_called_once_with(in_port=vif.ofport)
 
     def test_setup_physical_bridges(self):
         with mock.patch.object(ip_lib, "device_exists") as devex_fn,\

neutron/tests/unit/plugins/openvswitch/test_ovs_tunnel.py

@@ -443,10 +443,12 @@ class TunnelTest(object):
 
     def test_port_dead(self):
         self.mock_int_bridge_expected += [
-            mock.call.db_get_val('Port', VIF_PORT.port_name, 'tag'),
+            mock.call.db_get_val('Port', VIF_PORT.port_name, 'tag',
+                                 log_errors=True),
             mock.call.set_db_attribute(
                 'Port', VIF_PORT.port_name,
-                'tag', self.mod_agent.DEAD_VLAN_TAG),
+                'tag', self.mod_agent.DEAD_VLAN_TAG,
+                log_errors=True),
             mock.call.drop_port(in_port=VIF_PORT.ofport),
         ]