[ovs-dev] [PATCH v3] ovn-controller: Persist desired conntrack groups.

Ryan Moats rmoats at us.ibm.com
Thu Jul 28 22:17:41 UTC 2016


With incremental processing of logical flows desired conntrack groups
are not being persisted.  This patch adds this capability, with the
side effect of adding a ds_clone method that this capability leverages.

Signed-off-by: Ryan Moats <rmoats at us.ibm.com>
Reported-by: Guru Shetty <guru at ovn.org>
Reported-at: http://openvswitch.org/pipermail/dev/2016-July/076320.html
Fixes: 70c7cfe ("ovn-controller: Add incremental processing to lflow_run and physical_run")
---
 include/openvswitch/dynamic-string.h |  1 +
 include/ovn/actions.h                |  6 +++++
 lib/dynamic-string.c                 |  9 ++++++++
 ovn/controller/lflow.c               |  2 ++
 ovn/controller/ofctrl.c              | 43 ++++++++++++++++++++++++------------
 ovn/controller/ofctrl.h              |  5 ++++-
 ovn/controller/ovn-controller.c      |  2 +-
 ovn/lib/actions.c                    |  1 +
 8 files changed, 53 insertions(+), 16 deletions(-)

diff --git a/include/openvswitch/dynamic-string.h b/include/openvswitch/dynamic-string.h
index dfe2688..bf1f64a 100644
--- a/include/openvswitch/dynamic-string.h
+++ b/include/openvswitch/dynamic-string.h
@@ -73,6 +73,7 @@ void ds_swap(struct ds *, struct ds *);
 
 int ds_last(const struct ds *);
 bool ds_chomp(struct ds *, int c);
+void ds_clone(struct ds *, struct ds *);
 
 /* Inline functions. */
 
diff --git a/include/ovn/actions.h b/include/ovn/actions.h
index 114c71e..55720ce 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -22,7 +22,9 @@
 #include "compiler.h"
 #include "openvswitch/hmap.h"
 #include "openvswitch/dynamic-string.h"
+#include "openvswitch/uuid.h"
 #include "util.h"
+#include "uuid.h"
 
 struct expr;
 struct lexer;
@@ -43,6 +45,7 @@ struct group_table {
 struct group_info {
     struct hmap_node hmap_node;
     struct ds group;
+    struct uuid lflow_uuid;
     uint32_t group_id;
 };
 
@@ -107,6 +110,9 @@ struct action_params {
     /* A struct to figure out the group_id for group actions. */
     struct group_table *group_table;
 
+    /* The logical flow uuid that drove this action. */
+    struct uuid lflow_uuid;
+
     /* OVN maps each logical flow table (ltable), one-to-one, onto a physical
      * OpenFlow flow table (ptable).  A number of parameters describe this
      * mapping and data related to flow tables:
diff --git a/lib/dynamic-string.c b/lib/dynamic-string.c
index 1f17a9f..6f7b610 100644
--- a/lib/dynamic-string.c
+++ b/lib/dynamic-string.c
@@ -456,3 +456,12 @@ ds_chomp(struct ds *ds, int c)
         return false;
     }
 }
+
+void
+ds_clone(struct ds *dst, struct ds *source)
+{
+    dst->length = source->length;
+    dst->allocated = dst->length;
+    dst->string = xmalloc(dst->allocated + 1);
+    memcpy(dst->string, source->string, dst->allocated + 1);
+}
diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
index a4f3322..e38c32a 100644
--- a/ovn/controller/lflow.c
+++ b/ovn/controller/lflow.c
@@ -383,6 +383,7 @@ add_logical_flows(struct controller_ctx *ctx, const struct lport_index *lports,
 
     if (full_flow_processing) {
         ovn_flow_table_clear();
+        ovn_group_table_clear(group_table, false);
         full_logical_flow_processing = true;
         full_neighbor_flow_processing = true;
         full_flow_processing = false;
@@ -515,6 +516,7 @@ consider_logical_flow(const struct lport_index *lports,
         .aux = &aux,
         .ct_zones = ct_zones,
         .group_table = group_table,
+        .lflow_uuid = lflow->header_.uuid,
 
         .n_tables = LOG_PIPELINE_LEN,
         .first_ptable = first_ptable,
diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
index dd9f5ec..54bea99 100644
--- a/ovn/controller/ofctrl.c
+++ b/ovn/controller/ofctrl.c
@@ -140,8 +140,6 @@ static ovs_be32 queue_msg(struct ofpbuf *);
 static void ovn_flow_table_destroy(void);
 static struct ofpbuf *encode_flow_mod(struct ofputil_flow_mod *);
 
-static void ovn_group_table_clear(struct group_table *group_table,
-                                  bool existing);
 static struct ofpbuf *encode_group_mod(const struct ofputil_group_mod *);
 
 static void ofctrl_recv(const struct ofp_header *, enum ofptype);
@@ -150,12 +148,15 @@ static struct hmap match_flow_table = HMAP_INITIALIZER(&match_flow_table);
 static struct hindex uuid_flow_table = HINDEX_INITIALIZER(&uuid_flow_table);
 
 void
-ofctrl_init(void)
+ofctrl_init(struct group_table *group_table)
 {
     swconn = rconn_create(5, 0, DSCP_DEFAULT, 1 << OFP13_VERSION);
     tx_counter = rconn_packet_counter_create();
     hmap_init(&installed_flows);
     ovs_list_init(&flow_updates);
+    if (!groups) {
+        groups = group_table;
+    }
 }
 
 /* S_NEW, for a new connection.
@@ -680,6 +681,16 @@ ofctrl_remove_flows(const struct uuid *uuid)
             ovn_flow_destroy(f);
         }
     }
+
+    /* Remove any group_info information created by this logical flow. */
+    struct group_info *g, *next_g;
+    HMAP_FOR_EACH_SAFE (g, next_g, hmap_node, &groups->desired_groups) {
+        if (uuid_equals(&g->lflow_uuid, uuid)) {
+            hmap_remove(&groups->desired_groups, &g->hmap_node);
+            ds_destroy(&g->group);
+            free(g);
+        }
+    }
 }
 
 /* Shortcut to remove all flows matching the supplied UUID and add this
@@ -833,6 +844,17 @@ add_flow_mod(struct ofputil_flow_mod *fm, struct ovs_list *msgs)
 
 /* group_table. */
 
+static struct group_info *
+group_info_clone(struct group_info *source)
+{
+    struct group_info *clone = xmalloc(sizeof *clone);
+    ds_clone(&clone->group, &source->group);
+    clone->group_id = source->group_id;
+    clone->lflow_uuid = source->lflow_uuid;
+    clone->hmap_node.hash = source->hmap_node.hash;
+    return clone;
+}
+
 /* Finds and returns a group_info in 'existing_groups' whose key is identical
  * to 'target''s key, or NULL if there is none. */
 static struct group_info *
@@ -851,7 +873,7 @@ ovn_group_lookup(struct hmap *exisiting_groups,
 }
 
 /* Clear either desired_groups or existing_groups in group_table. */
-static void
+void
 ovn_group_table_clear(struct group_table *group_table, bool existing)
 {
     struct group_info *g, *next;
@@ -894,10 +916,6 @@ add_group_mod(const struct ofputil_group_mod *gm, struct ovs_list *msgs)
 void
 ofctrl_put(struct group_table *group_table, int64_t nb_cfg)
 {
-    if (!groups) {
-        groups = group_table;
-    }
-
     /* The flow table can be updated if the connection to the switch is up and
      * in the correct state and not backlogged with existing flow_mods.  (Our
      * criteria for being backlogged appear very conservative, but the socket
@@ -1066,13 +1084,10 @@ ofctrl_put(struct group_table *group_table, int64_t nb_cfg)
     /* Move the contents of desired_groups to existing_groups. */
     HMAP_FOR_EACH_SAFE(desired, next_group, hmap_node,
                        &group_table->desired_groups) {
-        hmap_remove(&group_table->desired_groups, &desired->hmap_node);
         if (!ovn_group_lookup(&group_table->existing_groups, desired)) {
-            hmap_insert(&group_table->existing_groups, &desired->hmap_node,
-                        desired->hmap_node.hash);
-        } else {
-            ds_destroy(&desired->group);
-            free(desired);
+            struct group_info *clone = group_info_clone(desired);
+            hmap_insert(&group_table->existing_groups, &clone->hmap_node,
+                        clone->hmap_node.hash);
         }
     }
 
diff --git a/ovn/controller/ofctrl.h b/ovn/controller/ofctrl.h
index befae01..da4ebb4 100644
--- a/ovn/controller/ofctrl.h
+++ b/ovn/controller/ofctrl.h
@@ -30,7 +30,7 @@ struct ovsrec_bridge;
 struct group_table;
 
 /* Interface for OVN main loop. */
-void ofctrl_init(void);
+void ofctrl_init(struct group_table *group_table);
 enum mf_field_id ofctrl_run(const struct ovsrec_bridge *br_int);
 void ofctrl_put(struct group_table *group_table, int64_t nb_cfg);
 void ofctrl_wait(void);
@@ -54,4 +54,7 @@ void ofctrl_flow_table_clear(void);
 
 void ovn_flow_table_clear(void);
 
+void ovn_group_table_clear(struct group_table *group_table,
+                           bool existing);
+
 #endif /* ovn/ofctrl.h */
diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index 5c74186..3cdfcdf 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -347,7 +347,7 @@ main(int argc, char *argv[])
     ovsrec_init();
     sbrec_init();
 
-    ofctrl_init();
+    ofctrl_init(&group_table);
     pinctrl_init();
     lflow_init();
 
diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
index fd5a867..aef5c75 100644
--- a/ovn/lib/actions.c
+++ b/ovn/lib/actions.c
@@ -761,6 +761,7 @@ parse_ct_lb_action(struct action_context *ctx)
         group_info = xmalloc(sizeof *group_info);
         group_info->group = ds;
         group_info->group_id = group_id;
+        group_info->lflow_uuid = ctx->ap->lflow_uuid;
         group_info->hmap_node.hash = hash;
 
         hmap_insert(&ctx->ap->group_table->desired_groups,
-- 
1.9.1




More information about the dev mailing list