From 81e043f72135682510727c9fa9bafe7efa676717 Mon Sep 17 00:00:00 2001 From: Kevin Benton Date: Thu, 30 Apr 2015 17:14:44 -0700 Subject: [PATCH] Don't delete port from bridge on delete_port event Commit d6a55c17360d1aa8ca91849199987ae71e8600ee 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. Conflicts: neutron/plugins/openvswitch/agent/ovs_neutron_agent.py neutron/tests/unit/plugins/openvswitch/agent/test_ovs_neutron_agent.py neutron/tests/unit/plugins/openvswitch/test_ovs_tunnel.py Related-Bug: #1333365 Closes-Bug: #1448148 Change-Id: I7ae7750b7ac7d15325ed9f2d517ca171543b53be (cherry picked from commit e007167a700aa5b80ecb48adff0ac36bb330591d) --- neutron/agent/common/ovs_lib.py | 9 +++--- .../openvswitch/agent/ovs_neutron_agent.py | 32 +++++++++++++++---- .../agent/test_ovs_neutron_agent.py | 31 ++++++++++-------- .../plugins/openvswitch/test_ovs_tunnel.py | 6 ++-- 4 files changed, 52 insertions(+), 26 deletions(-) diff --git a/neutron/agent/common/ovs_lib.py b/neutron/agent/common/ovs_lib.py index f73f73114..7b1d400c3 100644 --- a/neutron/agent/common/ovs_lib.py +++ b/neutron/agent/common/ovs_lib.py @@ -127,16 +127,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): diff --git a/neutron/plugins/openvswitch/agent/ovs_neutron_agent.py b/neutron/plugins/openvswitch/agent/ovs_neutron_agent.py index c7aabf7f1..87c2c9788 100644 --- a/neutron/plugins/openvswitch/agent/ovs_neutron_agent.py +++ b/neutron/plugins/openvswitch/agent/ovs_neutron_agent.py @@ -201,6 +201,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() @@ -338,10 +340,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") @@ -777,16 +790,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.add_flow(priority=2, in_port=port.ofport, actions="drop") @@ -1571,6 +1585,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. " diff --git a/neutron/tests/unit/plugins/openvswitch/agent/test_ovs_neutron_agent.py b/neutron/tests/unit/plugins/openvswitch/agent/test_ovs_neutron_agent.py index 483155955..8f9d45d38 100644 --- a/neutron/tests/unit/plugins/openvswitch/agent/test_ovs_neutron_agent.py +++ b/neutron/tests/unit/plugins/openvswitch/agent/test_ovs_neutron_agent.py @@ -212,13 +212,15 @@ class TestOvsNeutronAgent(base.BaseTestCase): mock.patch.object(self.agent.int_br, 'add_flow') ) as (set_ovs_db_func, get_ovs_db_func, add_flow_func): self.agent.port_dead(port) - get_ovs_db_func.assert_called_once_with("Port", mock.ANY, "tag") + get_ovs_db_func.assert_called_once_with("Port", mock.ANY, "tag", + log_errors=True) if cur_tag == ovs_neutron_agent.DEAD_VLAN_TAG: self.assertFalse(set_ovs_db_func.called) self.assertFalse(add_flow_func.called) else: set_ovs_db_func.assert_called_once_with( - "Port", mock.ANY, "tag", ovs_neutron_agent.DEAD_VLAN_TAG) + "Port", mock.ANY, "tag", ovs_neutron_agent.DEAD_VLAN_TAG, + log_errors=True) add_flow_func.assert_called_once_with( priority=2, in_port=port.ofport, actions="drop") @@ -523,18 +525,21 @@ class TestOvsNeutronAgent(base.BaseTestCase): self.assertEqual(set(['123']), self.agent.updated_ports) def test_port_delete(self): - port_id = "123" - port_name = "foo" - with contextlib.nested( - mock.patch.object(self.agent.int_br, 'get_vif_port_by_id', - return_value=mock.MagicMock( - port_name=port_name)), - mock.patch.object(self.agent.int_br, "delete_port") - ) as (get_vif_func, 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', ovs_neutron_agent.DEAD_VLAN_TAG, + log_errors=False) + int_br.add_flow.assert_any_call(priority=mock.ANY, + in_port=vif.ofport, + actions='drop') def test_setup_physical_bridges(self): with contextlib.nested( diff --git a/neutron/tests/unit/plugins/openvswitch/test_ovs_tunnel.py b/neutron/tests/unit/plugins/openvswitch/test_ovs_tunnel.py index cd21d0477..dbbbdef8b 100644 --- a/neutron/tests/unit/plugins/openvswitch/test_ovs_tunnel.py +++ b/neutron/tests/unit/plugins/openvswitch/test_ovs_tunnel.py @@ -463,10 +463,12 @@ class TunnelTest(base.BaseTestCase): 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', ovs_neutron_agent.DEAD_VLAN_TAG), + 'tag', ovs_neutron_agent.DEAD_VLAN_TAG, + log_errors=True), mock.call.add_flow(priority=2, in_port=VIF_PORT.ofport, actions='drop') ] -- 2.45.2