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 17aa55f4996bd28337d91e0d210970610fc0083b..22467a71a927d9fdf11c8505b87478440c19fe98 100644 (file)
--- 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 76eb976beb807089d929e653e4fd874c18101bfa..18f1c60015a20448344e8e16993684b24dbc113b 100644 (file)
--- 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 335a33f0cf28aac539fd5c6b10a49fc9d1c4bf01..63c4b455d71bbc65d77f530178ea38a87db7c9f1 100644 (file)
--- 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),