ovn-controller: Persist desired conntrack groups.

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@us.ibm.com>
Reported-by: Guru Shetty <guru@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")
Acked-by: Flavio Fernandes <flavio@flaviof.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>

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. */
 

include/ovn/actions.h

@@ -22,6 +22,7 @@
 #include "compiler.h"
 #include "openvswitch/hmap.h"
 #include "openvswitch/dynamic-string.h"
+#include "openvswitch/uuid.h"
 #include "util.h"
 
 struct expr;
@@ -43,6 +44,7 @@ struct group_table {
 struct group_info {
     struct hmap_node hmap_node;
     struct ds group;
+    struct uuid lflow_uuid;
     uint32_t group_id;
 };
 
@@ -117,6 +119,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:

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);
+}

ovn/controller/lflow.c

@@ -263,6 +263,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;
@@ -395,6 +396,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,

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,13 @@ 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);
+    groups = group_table;
 }
 
 /* S_NEW, for a new connection.
@@ -680,6 +679,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 +842,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 +871,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;
@@ -881,30 +901,24 @@ add_group_mod(const struct ofputil_group_mod *gm, struct ovs_list *msgs)
 }
 
 
-/* Replaces the flow table on the switch, if possible, by the flows in added
+/* Replaces the flow table on the switch, if possible, by the flows added
  * with ofctrl_add_flow().
  *
- * Replaces the group table on the switch, if possible, by the groups in
- * 'group_table->desired_groups'. Regardless of whether the group table
- * is updated, this deletes all the groups from the
- * 'group_table->desired_groups' and frees them. (The hmap itself isn't
- * destroyed.)
+ * Replaces the group table on the switch, if possible, by the groups added to
+ * the group table.  Regardless of whether the group table is updated, clears
+ * the gruop table.
  *
  * This should be called after ofctrl_run() within the main loop. */
 void
-ofctrl_put(struct group_table *group_table, int64_t nb_cfg)
+ofctrl_put(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
      * between ovn-controller and OVS provides some buffering.) */
     if (state != S_UPDATE_FLOWS
         || rconn_packet_counter_n_packets(tx_counter)) {
-        ovn_group_table_clear(group_table, false);
+        ovn_group_table_clear(groups, false);
         return;
     }
 
@@ -914,8 +928,8 @@ ofctrl_put(struct group_table *group_table, int64_t nb_cfg)
     /* Iterate through all the desired groups. If there are new ones,
      * add them to the switch. */
     struct group_info *desired;
-    HMAP_FOR_EACH(desired, hmap_node, &group_table->desired_groups) {
-        if (!ovn_group_lookup(&group_table->existing_groups, desired)) {
+    HMAP_FOR_EACH(desired, hmap_node, &groups->desired_groups) {
+        if (!ovn_group_lookup(&groups->existing_groups, desired)) {
             /* Create and install new group. */
             struct ofputil_group_mod gm;
             enum ofputil_protocol usable_protocols;
@@ -1030,8 +1044,8 @@ ofctrl_put(struct group_table *group_table, int64_t nb_cfg)
      * are not needed delete them. */
     struct group_info *installed, *next_group;
     HMAP_FOR_EACH_SAFE(installed, next_group, hmap_node,
-                       &group_table->existing_groups) {
-        if (!ovn_group_lookup(&group_table->desired_groups, installed)) {
+                       &groups->existing_groups) {
+        if (!ovn_group_lookup(&groups->desired_groups, installed)) {
             /* Delete the group. */
             struct ofputil_group_mod gm;
             enum ofputil_protocol usable_protocols;
@@ -1053,26 +1067,23 @@ ofctrl_put(struct group_table *group_table, int64_t nb_cfg)
             ds_destroy(&group_string);
             ofputil_uninit_group_mod(&gm);
 
-            /* Remove 'installed' from 'group_table->existing_groups' */
-            hmap_remove(&group_table->existing_groups, &installed->hmap_node);
+            /* Remove 'installed' from 'groups->existing_groups' */
+            hmap_remove(&groups->existing_groups, &installed->hmap_node);
             ds_destroy(&installed->group);
 
             /* Dealloc group_id. */
-            bitmap_set0(group_table->group_ids, installed->group_id);
+            bitmap_set0(groups->group_ids, installed->group_id);
             free(installed);
         }
     }
 
     /* 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);
+                       &groups->desired_groups) {
+        if (!ovn_group_lookup(&groups->existing_groups, desired)) {
+            struct group_info *clone = group_info_clone(desired);
+            hmap_insert(&groups->existing_groups, &clone->hmap_node,
+                        clone->hmap_node.hash);
         }
     }
 

ovn/controller/ofctrl.h

@@ -30,9 +30,9 @@ 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_put(int64_t nb_cfg);
 void ofctrl_wait(void);
 void ofctrl_destroy(void);
 int64_t ofctrl_get_cur_cfg(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 */

ovn/controller/ovn-controller.c

@@ -350,7 +350,7 @@ main(int argc, char *argv[])
     ovsrec_init();
     sbrec_init();
 
-    ofctrl_init();
+    ofctrl_init(&group_table);
     pinctrl_init();
     lflow_init();
 
@@ -459,7 +459,7 @@ main(int argc, char *argv[])
                          br_int, chassis_id, &ct_zones,
                          &local_datapaths, &patched_datapaths);
 
-            ofctrl_put(&group_table, get_nb_cfg(ctx.ovnsb_idl));
+            ofctrl_put(get_nb_cfg(ctx.ovnsb_idl));
             if (ctx.ovnsb_idl_txn) {
                 int64_t cur_cfg = ofctrl_get_cur_cfg();
                 if (cur_cfg && cur_cfg != chassis->nb_cfg) {

ovn/lib/actions.c

@@ -762,6 +762,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,