From: Kevin Benton Date: Fri, 1 May 2015 00:14:44 +0000 (-0700) Subject: Don't delete port from bridge on delete_port event X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=e007167a700aa5b80ecb48adff0ac36bb330591d;p=openstack-build%2Fneutron-build.git 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. Related-Bug: #1333365 Closes-Bug: #1448148 Change-Id: I7ae7750b7ac7d15325ed9f2d517ca171543b53be --- diff --git a/neutron/agent/common/ovs_lib.py b/neutron/agent/common/ovs_lib.py index b316584f9..513dbc536 100644 --- a/neutron/agent/common/ovs_lib.py +++ b/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): diff --git a/neutron/plugins/openvswitch/agent/ovs_neutron_agent.py b/neutron/plugins/openvswitch/agent/ovs_neutron_agent.py index 003cbdf06..dfe156bfc 100644 --- a/neutron/plugins/openvswitch/agent/ovs_neutron_agent.py +++ b/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. " 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 d29750dba..7ac299e4a 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 @@ -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,\ diff --git a/neutron/tests/unit/plugins/openvswitch/test_ovs_tunnel.py b/neutron/tests/unit/plugins/openvswitch/test_ovs_tunnel.py index fe8d9a00e..1828e394e 100644 --- a/neutron/tests/unit/plugins/openvswitch/test_ovs_tunnel.py +++ b/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), ]