OVS lib defer apply doesn't handle concurrency

The OVS lib deferred apply methods use a dict to save flows to add,
modify or delete when deffered apply is switched off.
If another thread adds, modifies or deletes flows on that dict during
another process called deffered_apply_off, its flows could be ignored.

This fix stash reference flows list and point the flows list to a new
cleared flows list. Then, it applies flows from the stashed flows list.

Closes-bug: #1263866
Change-Id: Ia3c6ce181e1599d1474da7eb944feff7d84f1d73

diff --git a/neutron/agent/linux/ovs_lib.py b/neutron/agent/linux/ovs_lib.py
index 207218399296df546f6aac92c24a5ed59d1257fe..5caa42045143c25fc3a310412b1b15bfc1e9593f 100644 (file)
--- a/neutron/agent/linux/ovs_lib.py
+++ b/neutron/agent/linux/ovs_lib.py
@@ -208,7 +208,14 @@ class OVSBridge(BaseOVS):
 
     def defer_apply_off(self):
         LOG.debug(_('defer_apply_off'))
-        for action, flows in self.deferred_flows.items():
+        # Note(ethuleau): stash flows and disable deferred mode. Then apply
+        # flows from the stashed reference to be sure to not purge flows that
+        # were added between two ofctl commands.
+        stashed_deferred_flows, self.deferred_flows = (
+            self.deferred_flows, {'add': '', 'mod': '', 'del': ''}
+        )
+        self.defer_apply_flows = False
+        for action, flows in stashed_deferred_flows.items():
             if flows:
                 LOG.debug(_('Applying following deferred flows '
                             'to bridge %s'), self.br_name)
@@ -216,8 +223,6 @@ class OVSBridge(BaseOVS):
                     LOG.debug(_('%(action)s: %(flow)s'),
                               {'action': action, 'flow': line})
                 self.run_ofctl('%s-flows' % action, ['-'], flows)
-        self.defer_apply_flows = False
-        self.deferred_flows = {'add': '', 'mod': '', 'del': ''}
 
     def add_tunnel_port(self, port_name, remote_ip, local_ip,
                         tunnel_type=p_const.TYPE_GRE,

diff --git a/neutron/tests/unit/agent/linux/test_ovs_lib.py b/neutron/tests/unit/agent/linux/test_ovs_lib.py
index 96a7e8ee64bdb56cb83888ec21c953222c82df39..4b4231790d1c4ebe71cd3a8353520e46004489f4 100644 (file)
--- a/neutron/tests/unit/agent/linux/test_ovs_lib.py
+++ b/neutron/tests/unit/agent/linux/test_ovs_lib.py
@@ -407,6 +407,50 @@ class OVS_Lib_Test(base.BaseTestCase):
             mock.call('del-flows', ['-'], 'deleted_flow_1\n')
         ])
 
+    def test_defer_apply_flows_concurrently(self):
+        flow_expr = mock.patch.object(ovs_lib, '_build_flow_expr_str').start()
+        flow_expr.side_effect = ['added_flow_1', 'deleted_flow_1',
+                                 'modified_flow_1', 'added_flow_2',
+                                 'deleted_flow_2', 'modified_flow_2']
+
+        run_ofctl = mock.patch.object(self.br, 'run_ofctl').start()
+
+        def run_ofctl_fake(cmd, args, process_input=None):
+            self.br.defer_apply_on()
+            if cmd == 'add-flows':
+                self.br.add_flow(flow='added_flow_2')
+            elif cmd == 'del-flows':
+                self.br.delete_flows(flow='deleted_flow_2')
+            elif cmd == 'mod-flows':
+                self.br.mod_flow(flow='modified_flow_2')
+        run_ofctl.side_effect = run_ofctl_fake
+
+        self.br.defer_apply_on()
+        self.br.add_flow(flow='added_flow_1')
+        self.br.delete_flows(flow='deleted_flow_1')
+        self.br.mod_flow(flow='modified_flow_1')
+        self.br.defer_apply_off()
+
+        run_ofctl.side_effect = None
+        self.br.defer_apply_off()
+
+        flow_expr.assert_has_calls([
+            mock.call({'flow': 'added_flow_1'}, 'add'),
+            mock.call({'flow': 'deleted_flow_1'}, 'del'),
+            mock.call({'flow': 'modified_flow_1'}, 'mod'),
+            mock.call({'flow': 'added_flow_2'}, 'add'),
+            mock.call({'flow': 'deleted_flow_2'}, 'del'),
+            mock.call({'flow': 'modified_flow_2'}, 'mod')
+        ])
+        run_ofctl.assert_has_calls([
+            mock.call('add-flows', ['-'], 'added_flow_1\n'),
+            mock.call('del-flows', ['-'], 'deleted_flow_1\n'),
+            mock.call('mod-flows', ['-'], 'modified_flow_1\n'),
+            mock.call('add-flows', ['-'], 'added_flow_2\n'),
+            mock.call('del-flows', ['-'], 'deleted_flow_2\n'),
+            mock.call('mod-flows', ['-'], 'modified_flow_2\n')
+        ])
+
     def test_add_tunnel_port(self):
         pname = "tap99"
         local_ip = "1.1.1.1"