[ovs-dev] [PATCH ovn v6 4/9] ofctrl: Replace the actions of an existing flow if actions have changed.

[numans at ovn.org]

From: Numan Siddique <numans at ovn.org>

If ofctrl_check_and_add_flow(F') is called where flow F' has match-actions (M, A2)
and if there already exists a flow F with match-actions (M, A1) in the desired flow
table, then
   (1) If F and F' share the same 'sb_uuid', this function doesn't update the
       existing flow F with new actions A2.

   (2) If F and F' don't share the same 'sb_uuid', this function adds F'
       also into the flow table with F and F' having the same hash. While installing
       the flows only one will be considered.

This patch fixes (1) by updating the F with the new actions A2.
This is required for the upcoming patch in this series which adds incremental
processing for OVS interface in the flow output stage. Since we will be not be
clearing the flow output data if these changes are handled incrementally, some
of the existing flows will be updated with new actions. One such example would
be flows in physical table OFTABLE_LOCAL_OUTPUT (table 33). This patch is
required to update such flows. Otherwise we will have incomplete actions installed.

We should also address (2) and it should be fixed
  - by logging the duplicate flow F'
  - and discarding F'.

Scenario (2) can happen when
 - either ovn-northd installs 2 logical flows with same match but with
   different actions due to some bug in ovn-northd

 - CMS adds 2 ACLs with the same match and priority, but with different
   actions.

However this patch doesn't attempt to fix (2).

Signed-off-by: Numan Siddique <numans at ovn.org>
---
 controller/ofctrl.c | 34 +++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/controller/ofctrl.c b/controller/ofctrl.c
index b8a9c2da8..edc22824f 100644
--- a/controller/ofctrl.c
+++ b/controller/ofctrl.c
@@ -642,6 +642,19 @@ ofctrl_recv(const struct ofp_header *oh, enum ofptype type)
     }
 }
 
+static void
+ofctrl_swap_flow_actions(struct ovn_flow *a, struct ovn_flow *b)
+{
+    struct ofpact *tmp = a->ofpacts;
+    size_t tmp_len = a->ofpacts_len;
+
+    a->ofpacts = b->ofpacts;
+    a->ofpacts_len = b->ofpacts_len;
+
+    b->ofpacts = tmp;
+    b->ofpacts_len = tmp_len;
+}
+
 /* Flow table interfaces to the rest of ovn-controller. */
 
 /* Adds a flow to 'desired_flows' with the specified 'match' and 'actions' to
@@ -667,14 +680,21 @@ ofctrl_check_and_add_flow(struct ovn_desired_flow_table *flow_table,
 
     ovn_flow_log(f, "ofctrl_add_flow");
 
-    if (ovn_flow_lookup(&flow_table->match_flow_table, f, true)) {
-        if (log_duplicate_flow) {
-            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
-            if (!VLOG_DROP_DBG(&rl)) {
-                char *s = ovn_flow_to_string(f);
-                VLOG_DBG("dropping duplicate flow: %s", s);
-                free(s);
+    struct ovn_flow *existing_f =
+        ovn_flow_lookup(&flow_table->match_flow_table, f, true);
+    if (existing_f) {
+        if (ofpacts_equal(f->ofpacts, f->ofpacts_len,
+                          existing_f->ofpacts, existing_f->ofpacts_len)) {
+            if (log_duplicate_flow) {
+                static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
+                if (!VLOG_DROP_DBG(&rl)) {
+                    char *s = ovn_flow_to_string(f);
+                    VLOG_DBG("dropping duplicate flow: %s", s);
+                    free(s);
+                }
             }
+        } else {
+            ofctrl_swap_flow_actions(f, existing_f);
         }
         ovn_flow_destroy(f);
         return;
-- 
2.26.2