From: Salvatore Orlando Date: Sat, 21 Dec 2013 00:59:05 +0000 (-0800) Subject: Avoid re-wiring ports unnecessarily X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=a9050757d94009c68f06c9f374de33eefbf06aa9;p=openstack-build%2Fneutron-build.git Avoid re-wiring ports unnecessarily In the majority of cases a port_update notification pertains a change in the properties affecting port filter, and does not affect port wiring, ie: the local vlan tag. This patch simply avoids doing port wiring/unwiring if the local vlan tag did not change. The extra overhead for the ovs-db get operation is offset by the fact that get commands are generally faster than set commands, and by avoiding executing the ovs-ofctl operation. Partial-Bug: #1253993 Partially implements blueprint: neutron-tempest-parallel Change-Id: Ia0bd2dc4e5a2634a4c863ff32ccc5cabe8e21337 --- diff --git a/neutron/plugins/openvswitch/agent/ovs_neutron_agent.py b/neutron/plugins/openvswitch/agent/ovs_neutron_agent.py index 17aa55f49..22467a71a 100644 --- a/neutron/plugins/openvswitch/agent/ovs_neutron_agent.py +++ b/neutron/plugins/openvswitch/agent/ovs_neutron_agent.py @@ -594,11 +594,13 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin, physical_network, segmentation_id) lvm = self.local_vlan_map[net_uuid] lvm.vif_ports[port.vif_id] = port - - self.int_br.set_db_attribute("Port", port.port_name, "tag", - str(lvm.vlan)) - if int(port.ofport) != -1: - self.int_br.delete_flows(in_port=port.ofport) + # Do not bind a port if it's already bound + cur_tag = self.int_br.db_get_val("Port", port.port_name, "tag") + if cur_tag != str(lvm.vlan): + self.int_br.set_db_attribute("Port", port.port_name, "tag", + str(lvm.vlan)) + if int(port.ofport) != -1: + self.int_br.delete_flows(in_port=port.ofport) def port_unbound(self, vif_id, net_uuid=None): '''Unbind port. @@ -628,9 +630,13 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin, :param port: a ovs_lib.VifPort object. ''' - self.int_br.set_db_attribute("Port", port.port_name, "tag", - DEAD_VLAN_TAG) - self.int_br.add_flow(priority=2, in_port=port.ofport, actions="drop") + # Don't kill a port if it's already dead + cur_tag = self.int_br.db_get_val("Port", port.port_name, "tag") + if cur_tag != DEAD_VLAN_TAG: + self.int_br.set_db_attribute("Port", port.port_name, "tag", + DEAD_VLAN_TAG) + self.int_br.add_flow(priority=2, in_port=port.ofport, + actions="drop") def setup_integration_br(self): '''Setup the integration bridge. @@ -1024,8 +1030,6 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin, # VIF wiring needs to be performed always for 'new' devices. # For updated ports, re-wiring is not needed in most cases, but needs # to be performed anyway when the admin state of a device is changed. - # TODO(salv-orlando): Optimize for avoiding unnecessary VIF - # processing for updated ports whose admin state is left unchanged # A device might be both in the 'added' and 'updated' # list at the same time; avoid processing it twice. devices_added_updated = (port_info.get('added', set()) | diff --git a/neutron/tests/unit/openvswitch/test_ovs_neutron_agent.py b/neutron/tests/unit/openvswitch/test_ovs_neutron_agent.py index 76eb976be..18f1c6001 100644 --- a/neutron/tests/unit/openvswitch/test_ovs_neutron_agent.py +++ b/neutron/tests/unit/openvswitch/test_ovs_neutron_agent.py @@ -126,32 +126,70 @@ class TestOvsNeutronAgent(base.BaseTestCase): self.agent.tun_br = mock.Mock() self.agent.sg_agent = mock.Mock() - def _mock_port_bound(self, ofport=None): + def _mock_port_bound(self, ofport=None, new_local_vlan=None, + old_local_vlan=None): port = mock.Mock() port.ofport = ofport net_uuid = 'my-net-uuid' - with mock.patch('neutron.agent.linux.ovs_lib.OVSBridge.' - 'set_db_attribute', - return_value=True): - with mock.patch.object(self.agent.int_br, - 'delete_flows') as delete_flows_func: - self.agent.port_bound(port, net_uuid, 'local', None, None) - self.assertEqual(delete_flows_func.called, ofport != -1) + if old_local_vlan is not None: + self.agent.local_vlan_map[net_uuid] = ( + ovs_neutron_agent.LocalVLANMapping( + old_local_vlan, None, None, None)) + with contextlib.nested( + mock.patch('neutron.agent.linux.ovs_lib.OVSBridge.' + 'set_db_attribute', return_value=True), + mock.patch('neutron.agent.linux.ovs_lib.OVSBridge.' + 'db_get_val', return_value=str(old_local_vlan)), + mock.patch.object(self.agent.int_br, 'delete_flows') + ) as (set_ovs_db_func, get_ovs_db_func, delete_flows_func): + self.agent.port_bound(port, net_uuid, 'local', None, None) + get_ovs_db_func.assert_called_once_with("Port", mock.ANY, "tag") + if new_local_vlan != old_local_vlan: + set_ovs_db_func.assert_called_once_with( + "Port", mock.ANY, "tag", str(new_local_vlan)) + if ofport != -1: + delete_flows_func.assert_called_once_with(in_port=port.ofport) + else: + self.assertFalse(delete_flows_func.called) + else: + self.assertFalse(set_ovs_db_func.called) + self.assertFalse(delete_flows_func.called) def test_port_bound_deletes_flows_for_valid_ofport(self): - self._mock_port_bound(ofport=1) + self._mock_port_bound(ofport=1, new_local_vlan=1) def test_port_bound_ignores_flows_for_invalid_ofport(self): - self._mock_port_bound(ofport=-1) + self._mock_port_bound(ofport=-1, new_local_vlan=1) + + def test_port_bound_does_not_rewire_if_already_bound(self): + self._mock_port_bound(ofport=-1, new_local_vlan=1, old_local_vlan=1) + + def _test_port_dead(self, cur_tag=None): + port = mock.Mock() + port.ofport = 1 + with contextlib.nested( + mock.patch('neutron.agent.linux.ovs_lib.OVSBridge.' + 'set_db_attribute', return_value=True), + mock.patch('neutron.agent.linux.ovs_lib.OVSBridge.' + 'db_get_val', return_value=cur_tag), + 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") + 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", str(ovs_neutron_agent.DEAD_VLAN_TAG)) + add_flow_func.assert_called_once_with( + priority=2, in_port=port.ofport, actions="drop") def test_port_dead(self): - with mock.patch('neutron.agent.linux.ovs_lib.OVSBridge.' - 'set_db_attribute', - return_value=True): - with mock.patch.object(self.agent.int_br, - 'add_flow') as add_flow_func: - self.agent.port_dead(mock.Mock()) - self.assertTrue(add_flow_func.called) + self._test_port_dead() + + def test_port_dead_with_port_already_dead(self): + self._test_port_dead(ovs_neutron_agent.DEAD_VLAN_TAG) def mock_scan_ports(self, vif_port_set=None, registered_ports=None, updated_ports=None): diff --git a/neutron/tests/unit/openvswitch/test_ovs_tunnel.py b/neutron/tests/unit/openvswitch/test_ovs_tunnel.py index 335a33f0c..63c4b455d 100644 --- a/neutron/tests/unit/openvswitch/test_ovs_tunnel.py +++ b/neutron/tests/unit/openvswitch/test_ovs_tunnel.py @@ -417,6 +417,7 @@ class TunnelTest(base.BaseTestCase): def test_port_bound(self): self.mock_int_bridge_expected += [ + mock.call.db_get_val('Port', VIF_PORT.port_name, 'tag'), mock.call.set_db_attribute('Port', VIF_PORT.port_name, 'tag', str(LVM.vlan)), mock.call.delete_flows(in_port=VIF_PORT.ofport) @@ -447,6 +448,7 @@ 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.set_db_attribute( 'Port', VIF_PORT.port_name, 'tag', ovs_neutron_agent.DEAD_VLAN_TAG),