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

[Ben Pfaff]

On Thu, Jul 28, 2016 at 05:10:56PM -0500, Ryan Moats wrote:
> Ben Pfaff <b...@ovn.org> wrote on 07/28/2016 04:06:09 PM:
> 
> > From: Ben Pfaff <b...@ovn.org>
> > To: Ryan Moats/Omaha/IBM@IBMUS, Gurucharan Shetty <g...@ovn.org>
> > Cc: dev@openvswitch.org
> > Date: 07/28/2016 04:06 PM
> > Subject: Re: [ovs-dev] [PATCH v2] ovn-controller: Persist desired
> > conntrack groups.
> >
> > On Thu, Jul 28, 2016 at 02:18:45PM +, Ryan Moats wrote:
> > > 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 <rmo...@us.ibm.com>
> > > Reported-by: Guru Shetty <g...@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")
> > > ---
> > >  v1->v2 addressed review comments
> > >updated commit message
> > >changed name of ds_copy to ds_clone
> > >moved lflow uuid storage to action_params for cleaner code
> >
> > It seems odd that group_clone() doesn't copy the UUID.
> >
> > I'm not sure why group_info's 'group' member is a struct ds instead of
> > just a char *.  (This isn't anything that you introduced.)
> >
> > Coding style:
> > > +static struct group_info *
> > > +group_info_clone(struct group_info *source) {
> >
> > But I'll hold off on further review since Guru reports that this causes
> > test failures in the system tests.
> >
> 
> If you want to hit me with the rest of the style comments, I have the
> crash issue fixed and I can fold the style into v3...

That was the style comment.  The { should be on its own line.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

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

[Ryan Moats]

Ben Pfaff <b...@ovn.org> wrote on 07/28/2016 04:06:09 PM:

> From: Ben Pfaff <b...@ovn.org>
> To: Ryan Moats/Omaha/IBM@IBMUS, Gurucharan Shetty <g...@ovn.org>
> Cc: dev@openvswitch.org
> Date: 07/28/2016 04:06 PM
> Subject: Re: [ovs-dev] [PATCH v2] ovn-controller: Persist desired
> conntrack groups.
>
> On Thu, Jul 28, 2016 at 02:18:45PM +, Ryan Moats wrote:
> > 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 <rmo...@us.ibm.com>
> > Reported-by: Guru Shetty <g...@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")
> > ---
> >  v1->v2 addressed review comments
> >updated commit message
> >changed name of ds_copy to ds_clone
> >moved lflow uuid storage to action_params for cleaner code
>
> It seems odd that group_clone() doesn't copy the UUID.
>
> I'm not sure why group_info's 'group' member is a struct ds instead of
> just a char *.  (This isn't anything that you introduced.)
>
> Coding style:
> > +static struct group_info *
> > +group_info_clone(struct group_info *source) {
>
> But I'll hold off on further review since Guru reports that this causes
> test failures in the system tests.
>

If you want to hit me with the rest of the style comments, I have the
crash issue fixed and I can fold the style into v3...

Ryan
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

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

[Ben Pfaff]

On Thu, Jul 28, 2016 at 02:18:45PM +, Ryan Moats wrote:
> 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 
> Reported-by: Guru Shetty 
> Reported-at: http://openvswitch.org/pipermail/dev/2016-July/076320.html
> Fixes: 70c7cfe ("ovn-controller: Add incremental processing to lflow_run and 
> physical_run")
> ---
>  v1->v2 addressed review comments
>updated commit message
>changed name of ds_copy to ds_clone
>moved lflow uuid storage to action_params for cleaner code

It seems odd that group_clone() doesn't copy the UUID.

I'm not sure why group_info's 'group' member is a struct ds instead of
just a char *.  (This isn't anything that you introduced.)

Coding style:
> +static struct group_info *
> +group_info_clone(struct group_info *source) {

But I'll hold off on further review since Guru reports that this causes
test failures in the system tests.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

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

[Ryan Moats]

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 
Reported-by: Guru Shetty 
Reported-at: http://openvswitch.org/pipermail/dev/2016-July/076320.html
Fixes: 70c7cfe ("ovn-controller: Add incremental processing to lflow_run and 
physical_run")
---
 v1->v2 addressed review comments
   updated commit message
   changed name of ds_copy to ds_clone
   moved lflow uuid storage to action_params for cleaner code

 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  | 32 +++-
 ovn/controller/ofctrl.h  |  3 +++
 ovn/lib/actions.c|  1 +
 7 files changed, 45 insertions(+), 9 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 = ,
 .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..de019b0 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);
@@ -680,6 +678,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, >desired_groups) {
+if (uuid_equals(>lflow_uuid, uuid)) {
+hmap_remove(>desired_groups, >hmap_node);
+ds_destroy(>group);
+free(g);
+}
+}
 }
 
 /* Shortcut to remove all flows matching the supplied UUID and add this
@@ -833,6 +841,15 @@ add_flow_mod(struct ofputil_flow_mod *fm, struct ovs_list 
*msgs)
 
 /* group_table. */
 
+static struct group_info *
+group_info_clone(struct group_info