diff mbox

[ovs-dev,RFC,8/8] ovn-controller: Handle logical flows of loadbalancers.

Message ID 1456727604-15784-9-git-send-email-guru@ovn.org
State Changes Requested
Headers show

Commit Message

Gurucharan Shetty Feb. 29, 2016, 6:33 a.m. UTC
Signed-off-by: Gurucharan Shetty <guru@ovn.org>
---
 ovn/controller/lflow.c          |   3 +-
 ovn/controller/lflow.h          |   2 +
 ovn/controller/ofctrl.c         | 134 +++++++++++++++++++++++++++++++++++++-
 ovn/controller/ofctrl.h         |   3 +-
 ovn/controller/ovn-controller.c |  22 ++++++-
 ovn/lib/actions.c               | 141 ++++++++++++++++++++++++++++++++++++++++
 ovn/lib/actions.h               |  16 +++++
 ovn/lib/lex.c                   |  13 ++++
 ovn/lib/lex.h                   |   1 +
 9 files changed, 330 insertions(+), 5 deletions(-)

Comments

Ben Pfaff March 15, 2016, 6:32 p.m. UTC | #1
On Sun, Feb 28, 2016 at 10:33:24PM -0800, Gurucharan Shetty wrote:
> Signed-off-by: Gurucharan Shetty <guru@ovn.org>

GCC complains about strtok_r() in parse_ct_lb_action():

    ../ovn/lib/actions.c: In function 'actions_parse':
    ../ovn/lib/actions.c:251:5: error: 'save' may be used uninitialized in this function [-Werror=maybe-uninitialized]
         ds_put_format(ds, ",bucket=bucket_id=%u,weight:100,actions="
         ^
    ../ovn/lib/actions.c:227:22: note: 'save' was declared here
         char *ips, *ip, *save;
                          ^

This is a common GCC complaint; we usually end up with an initializer on
the save pointer, e.g.:
    char *ips, *ip, *save = NULL;
even though I'm pretty sure it's not really necessary.

I'm uncomfortable with the form of the argument to ct_lb.  It seems odd
that it would be a string, since it is naturally a list of IP addresses
and the OVN match/action language is well suited for lists of IP
addresses.

I don't see any documentation for the new actions in ovn-sb.xml.

s/paranthesis/parenthesis/ here:

    static void
    parse_ct_lb_action(struct action_context *ctx)
    {
        if (!lexer_match(ctx->lexer, LEX_T_LPAREN)) {
            action_error(ctx, "\"ct_lb\" needs paranthesis.");
            return;
        }

OFPG_MAX is 0xffffff00.  This allocates a bitmap with that many bits, or
about 512 MB of RAM.  I think that's overkill.

It's probably better to use bitmap_scan() than to open-code a loop.

I think that parse_ct_lb_action() leaks the string it constructs in the
case where it succeeds but doesn't create a new group.

parse_ct_lb_action() ends with a plain "return;", which can be removed.

There is a typo in the parameter name for ovn_group_lookup():
s/exisiting/existing/.

run_S_CLEAR_FLOWS() calls ovn_flow_table_clear().  I think it should
also clear existing_groups, so that the desired_groups get reinstalled.

Thanks,

Ben.
Gurucharan Shetty March 15, 2016, 9:14 p.m. UTC | #2
On 15 March 2016 at 11:32, Ben Pfaff <blp@ovn.org> wrote:

> On Sun, Feb 28, 2016 at 10:33:24PM -0800, Gurucharan Shetty wrote:
> > Signed-off-by: Gurucharan Shetty <guru@ovn.org>
>


Thank you for looking through the series. I agree with all your comments so
far in the series and I will get them right in the non-RFC series. I have
one comment below.

I'm uncomfortable with the form of the argument to ct_lb.  It seems odd
> that it would be a string, since it is naturally a list of IP addresses
> and the OVN match/action language is well suited for lists of IP
> addresses.
>

I went with the string as that was what needs to be given to the created
group string of the
form: type=select,bucket=bucket_id=%u,weight:100,actions=ct(nat(dst=%s),commit,table=%d,zone=NXM_NX_REG5[0..15])
  ......

So if I understand you right, you want the ip reconverted again to string?
So the advantage with your approach is that if a wrong string is given, the
translation to IP addresses will catch it early?


>
>
Ben Pfaff March 16, 2016, 11:53 p.m. UTC | #3
On Tue, Mar 15, 2016 at 02:14:42PM -0700, Guru Shetty wrote:
> On 15 March 2016 at 11:32, Ben Pfaff <blp@ovn.org> wrote:
> 
> > On Sun, Feb 28, 2016 at 10:33:24PM -0800, Gurucharan Shetty wrote:
> > > Signed-off-by: Gurucharan Shetty <guru@ovn.org>
> >
> 
> 
> Thank you for looking through the series. I agree with all your comments so
> far in the series and I will get them right in the non-RFC series. I have
> one comment below.
> 
> I'm uncomfortable with the form of the argument to ct_lb.  It seems odd
> > that it would be a string, since it is naturally a list of IP addresses
> > and the OVN match/action language is well suited for lists of IP
> > addresses.
> >
> 
> I went with the string as that was what needs to be given to the created
> group string of the
> form: type=select,bucket=bucket_id=%u,weight:100,actions=ct(nat(dst=%s),commit,table=%d,zone=NXM_NX_REG5[0..15])
>   ......
> 
> So if I understand you right, you want the ip reconverted again to string?
> So the advantage with your approach is that if a wrong string is given, the
> translation to IP addresses will catch it early?

I want the syntax of the OVN logical match/action language to make sense
independent of their implementation.  The logical matches and actions
don't currently have anything where a string needs to be parsed into
tokens to understand what's going on, and unless you're very familiar
with the implementation then it doesn't make sense.  I don't want
whoever writes the OVN logical flows to need to be familiar with the
implementation to understand.

I also agree with the reason you suggest.
diff mbox

Patch

diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
index 32491cf..9aed437 100644
--- a/ovn/controller/lflow.c
+++ b/ovn/controller/lflow.c
@@ -267,7 +267,7 @@  lflow_init(void)
  * into OpenFlow flows.  See ovn-architecture(7) for more information. */
 void
 lflow_run(struct controller_ctx *ctx, struct hmap *flow_table,
-          const struct simap *ct_zones)
+          struct group_table *group_table, const struct simap *ct_zones)
 {
     struct hmap flows = HMAP_INITIALIZER(&flows);
     uint32_t conj_id_ofs = 1;
@@ -309,6 +309,7 @@  lflow_run(struct controller_ctx *ctx, struct hmap *flow_table,
             .symtab = &symtab,
             .ports = &ldp->ports,
             .ct_zones = ct_zones,
+            .group_table = group_table,
 
             .n_tables = LOG_PIPELINE_LEN,
             .first_ptable = first_ptable,
diff --git a/ovn/controller/lflow.h b/ovn/controller/lflow.h
index 4a4fa83..5c93be8 100644
--- a/ovn/controller/lflow.h
+++ b/ovn/controller/lflow.h
@@ -37,6 +37,7 @@ 
 
 struct controller_ctx;
 struct hmap;
+struct group_table;
 struct simap;
 struct uuid;
 
@@ -57,6 +58,7 @@  struct uuid;
 
 void lflow_init(void);
 void lflow_run(struct controller_ctx *, struct hmap *flow_table,
+               struct group_table *group_table,
                const struct simap *ct_zones);
 void lflow_destroy(void);
 
diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
index 3297fb3..b6be206 100644
--- a/ovn/controller/ofctrl.c
+++ b/ovn/controller/ofctrl.c
@@ -21,12 +21,14 @@ 
 #include "match.h"
 #include "ofp-actions.h"
 #include "ofp-msgs.h"
+#include "ofp-parse.h"
 #include "ofp-print.h"
 #include "ofp-util.h"
 #include "ofpbuf.h"
 #include "openflow/openflow.h"
 #include "openvswitch/vlog.h"
 #include "ovn-controller.h"
+#include "ovn/lib/actions.h"
 #include "physical.h"
 #include "rconn.h"
 #include "socket-util.h"
@@ -60,6 +62,8 @@  static void queue_flow_mod(struct ofputil_flow_mod *);
 /* OpenFlow connection to the switch. */
 static struct rconn *swconn;
 
+static void queue_group_mod(struct ofputil_group_mod *);
+
 /* Last seen sequence number for 'swconn'.  When this differs from
  * rconn_get_connection_seqno(rconn), 'swconn' has reconnected. */
 static unsigned int seqno;
@@ -309,6 +313,15 @@  run_S_CLEAR_FLOWS(void)
     queue_flow_mod(&fm);
     VLOG_DBG("clearing all flows");
 
+    struct ofputil_group_mod gm;
+    memset(&gm, 0, sizeof gm);
+    gm.command = OFPGC11_DELETE;
+    gm.group_id = OFPG_ALL;
+    gm.command_bucket_id = OFPG15_BUCKET_ALL;
+    list_init(&gm.buckets);
+    queue_group_mod(&gm);
+    ofputil_bucket_list_destroy(&gm.buckets);
+
     /* Clear installed_flows, to match the state of the switch. */
     ovn_flow_table_clear(&installed_flows);
 
@@ -589,15 +602,61 @@  queue_flow_mod(struct ofputil_flow_mod *fm)
     queue_msg(ofputil_encode_flow_mod(fm, OFPUTIL_P_OF13_OXM));
 }
 
+
+/* group_table. */
+
+/* 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 *
+ovn_group_lookup(struct hmap *exisiting_groups,
+                 const struct group_info *target)
+{
+    struct group_info *e;
+
+    HMAP_FOR_EACH_WITH_HASH(e, hmap_node, target->hmap_node.hash,
+                            exisiting_groups) {
+        if (e->group_id == target->group_id) {
+            return e;
+        }
+    }
+    return NULL;
+}
+
+/* Clear desired_groups in group_table. */
+static void
+ovn_group_table_clear(struct group_table *group_table)
+{
+    struct group_info *g, *next;
+    HMAP_FOR_EACH_SAFE (g, next, hmap_node, &group_table->desired_groups) {
+        hmap_remove(&group_table->desired_groups, &g->hmap_node);
+        bitmap_set0(group_table->group_ids, g->group_id);
+        ds_destroy(g->group);
+        free(g->group);
+        free(g);
+    }
+}
+
+static void
+queue_group_mod(struct ofputil_group_mod *gm)
+{
+    queue_msg(ofputil_encode_group_mod(OFP13_VERSION, gm));
+}
+
 /* Replaces the flow table on the switch, if possible, by the flows in
  * 'flow_table', which should have been added with ofctrl_add_flow().
  * Regardless of whether the flow table is updated, this deletes all of the
  * flows from 'flow_table' and frees them.  (The hmap itself isn't
  * destroyed.)
  *
+ * 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.)
+ *
  * This called be called be ofctrl_run() within the main loop. */
 void
-ofctrl_put(struct hmap *flow_table)
+ofctrl_put(struct hmap *flow_table, struct group_table *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
@@ -608,9 +667,37 @@  ofctrl_put(struct hmap *flow_table)
     if (state != S_UPDATE_FLOWS
         || rconn_packet_counter_n_packets(tx_counter)) {
         ovn_flow_table_clear(flow_table);
+        ovn_group_table_clear(group_table);
         return;
     }
 
+    /* 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)) {
+            /* Create and install new group. */
+            struct ofputil_group_mod gm;
+            enum ofputil_protocol usable_protocols;
+            char *error;
+            struct ds group_string = DS_EMPTY_INITIALIZER;
+            ds_put_format(&group_string, "group_id=%u,%s",
+                          desired->group_id, ds_cstr(desired->group));
+
+            error = parse_ofp_group_mod_str(&gm, OFPGC11_ADD,
+                                            ds_cstr(&group_string),
+                                            &usable_protocols);
+            if (!error) {
+                queue_group_mod(&gm);
+            } else {
+                VLOG_ERR("new group %s %s", error, ds_cstr(&group_string));
+                free(error);
+            }
+            ds_destroy(&group_string);
+            ofputil_bucket_list_destroy(&gm.buckets);
+        }
+    }
+
     /* Iterate through all of the installed flows.  If any of them are no
      * longer desired, delete them; if any of them should have different
      * actions, update them. */
@@ -680,4 +767,49 @@  ofctrl_put(struct hmap *flow_table)
         hmap_remove(flow_table, &d->hmap_node);
         hmap_insert(&installed_flows, &d->hmap_node, d->hmap_node.hash);
     }
+
+    /* Iterate through the installed groups from previous runs. If they
+     * 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)) {
+            /* Delete the group. */
+            struct ofputil_group_mod gm;
+            enum ofputil_protocol usable_protocols;
+            char *error;
+            struct ds group_string = DS_EMPTY_INITIALIZER;
+            ds_put_format(&group_string, "group_id=%u", installed->group_id);
+
+            error = parse_ofp_group_mod_str(&gm, OFPGC11_DELETE,
+                                            ds_cstr(&group_string),
+                                            &usable_protocols);
+            if (!error) {
+                queue_group_mod(&gm);
+            } else {
+                VLOG_ERR("%s", error);
+                free(error);
+            }
+            ds_destroy(&group_string);
+            ofputil_bucket_list_destroy(&gm.buckets);
+
+            /* Remove 'installed' from 'group_table->existing_groups' */
+            hmap_remove(&group_table->existing_groups, &installed->hmap_node);
+            ds_destroy(installed->group);
+
+            /* Dealloc group_id. */
+            bitmap_set0(group_table->group_ids, installed->group_id);
+        }
+    }
+
+    /* 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);
+        }
+    }
+
 }
diff --git a/ovn/controller/ofctrl.h b/ovn/controller/ofctrl.h
index 93ef8ea..dc0324e 100644
--- a/ovn/controller/ofctrl.h
+++ b/ovn/controller/ofctrl.h
@@ -26,11 +26,12 @@  struct hmap;
 struct match;
 struct ofpbuf;
 struct ovsrec_bridge;
+struct group_table;
 
 /* Interface for OVN main loop. */
 void ofctrl_init(void);
 enum mf_field_id ofctrl_run(const struct ovsrec_bridge *br_int);
-void ofctrl_put(struct hmap *flows);
+void ofctrl_put(struct hmap *flows, struct group_table *group_table);
 void ofctrl_wait(void);
 void ofctrl_destroy(void);
 
diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index 02ecb3e..34a4850 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -30,6 +30,7 @@ 
 #include "dynamic-string.h"
 #include "openvswitch/vconn.h"
 #include "openvswitch/vlog.h"
+#include "ovn/lib/actions.h"
 #include "ovn/lib/ovn-sb-idl.h"
 #include "poll-loop.h"
 #include "fatal-signal.h"
@@ -267,6 +268,13 @@  main(int argc, char *argv[])
     unixctl_command_register("ct-zone-list", "", 0, 0,
                              ct_zone_list, &ct_zones);
 
+    /* Initialize group ids for loadbalancing. */
+    struct group_table group_table;
+    group_table.group_ids = bitmap_allocate(OFPG_MAX);
+    bitmap_set1(group_table.group_ids, 0); /* Group id 0 is invalid. */
+    hmap_init(&group_table.desired_groups);
+    hmap_init(&group_table.existing_groups);
+
     /* Main loop. */
     exiting = false;
     while (!exiting) {
@@ -297,12 +305,12 @@  main(int argc, char *argv[])
             pinctrl_run(&ctx, br_int);
 
             struct hmap flow_table = HMAP_INITIALIZER(&flow_table);
-            lflow_run(&ctx, &flow_table, &ct_zones);
+            lflow_run(&ctx, &flow_table, &group_table, &ct_zones);
             if (chassis_id) {
                 physical_run(&ctx, mff_ovn_geneve,
                              br_int, chassis_id, &ct_zones, &flow_table);
             }
-            ofctrl_put(&flow_table);
+            ofctrl_put(&flow_table, &group_table);
             hmap_destroy(&flow_table);
         }
 
@@ -360,6 +368,16 @@  main(int argc, char *argv[])
 
     simap_destroy(&ct_zones);
 
+    bitmap_free(group_table.group_ids);
+    hmap_destroy(&group_table.desired_groups);
+
+    struct group_info *installed, *next_group;
+    HMAP_FOR_EACH_SAFE(installed, next_group, hmap_node,
+                       &group_table.existing_groups) {
+        hmap_remove(&group_table.existing_groups, &installed->hmap_node);
+        ds_destroy(installed->group);
+    }
+
     ovsdb_idl_loop_destroy(&ovs_idl_loop);
     ovsdb_idl_loop_destroy(&ovnsb_idl_loop);
 
diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
index 42e7f3b..88bfec5 100644
--- a/ovn/lib/actions.c
+++ b/ovn/lib/actions.c
@@ -21,6 +21,7 @@ 
 #include "compiler.h"
 #include "dynamic-string.h"
 #include "expr.h"
+#include "hmap.h"
 #include "lex.h"
 #include "logical-fields.h"
 #include "ofp-actions.h"
@@ -184,6 +185,142 @@  add_prerequisite(struct action_context *ctx, const char *prerequisite)
 }
 
 static void
+parse_ct_nat_action(struct action_context *ctx)
+{
+    struct ofpact_conntrack *ct = ofpact_put_CT(ctx->ofpacts);
+    struct ofpact_nat *nat;
+    size_t nat_offset;
+    if (ctx->ap->cur_ltable < ctx->ap->n_tables) {
+        ct->recirc_table = ctx->ap->first_ptable + ctx->ap->cur_ltable + 1;
+    } else {
+        action_error(ctx, "\"ct_nat\" action not allowed in last table.");
+        return;
+    }
+    ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE);
+    ct->zone_src.ofs = 0;
+    ct->zone_src.n_bits = 16;
+    ct->flags = 0;
+    ct->alg = 0;
+
+    add_prerequisite(ctx, "ip");
+
+    nat_offset = ctx->ofpacts->size;
+    ofpbuf_pull(ctx->ofpacts, nat_offset);
+
+    nat = ofpact_put_NAT(ctx->ofpacts);
+    nat->flags = 0;
+    nat->range_af = AF_UNSPEC;
+
+    ctx->ofpacts->header = ofpbuf_push_uninit(ctx->ofpacts, nat_offset);
+    ct = ctx->ofpacts->header;
+    ofpact_finish(ctx->ofpacts, &ct->ofpact);
+}
+
+static void
+parse_ct_lb_action(struct action_context *ctx)
+{
+    if (!lexer_match(ctx->lexer, LEX_T_LPAREN)) {
+        action_error(ctx, "\"ct_lb\" needs paranthesis.");
+        return;
+    }
+
+    char *ips, *ip, *save;
+    uint8_t recirc_table;
+    uint32_t group_id = 0, bucket_id = 0, hash;
+    struct group_info *group_info;
+    struct ofpact_group *og;
+    struct ds *ds;
+
+    if (ctx->ap->cur_ltable < ctx->ap->n_tables) {
+        recirc_table = ctx->ap->first_ptable + ctx->ap->cur_ltable + 1;
+    } else {
+        action_error(ctx, "\"ct_lb()\" action not allowed in last table.");
+        return;
+    }
+
+    if (!lexer_get_string(ctx->lexer, &ips)) {
+        action_error(ctx, "ct_lb has missing ip parameters.");
+        return;
+    }
+
+    ds = xmalloc(sizeof *ds);
+    ds_init(ds);
+    ds_put_format(ds, "type=select");
+
+    ip = strtok_r(ips, ",", &save);
+    ds_put_format(ds, ",bucket=bucket_id=%u,weight:100,actions="
+                  "ct(nat(dst=%s),commit,table=%d,zone=NXM_NX_REG5[0..15])"
+                  ,bucket_id, ip, recirc_table);
+    while ((ip = strtok_r(NULL, ",", &save)) != NULL) {
+        bucket_id++;
+        ds_put_format(ds, ",bucket=bucket_id=%u,weight:100,actions="
+                      "ct(nat(dst=%s),commit,table=%d,zone="
+                      "NXM_NX_REG5[0..15])", bucket_id, ip, recirc_table);
+    }
+
+    free(ips);
+    if (!lexer_match(ctx->lexer, LEX_T_RPAREN)) {
+        ds_destroy(ds);
+        free(ds);
+        action_syntax_error(ctx, "expecting `)'");
+        return;
+    }
+    hash = hash_string(ds_cstr(ds), 0);
+
+    /* Check whether we have non installed but allocated group_id. */
+    HMAP_FOR_EACH_WITH_HASH (group_info, hmap_node, hash,
+                             &ctx->ap->group_table->desired_groups) {
+        if (!strcmp(ds_cstr(group_info->group), ds_cstr(ds))) {
+            group_id = group_info->group_id;
+            break;
+        }
+    }
+
+    if (!group_id) {
+        /* Check whether we already have an installed entry for this
+         * combination. */
+        HMAP_FOR_EACH_WITH_HASH (group_info, hmap_node, hash,
+                                 &ctx->ap->group_table->existing_groups) {
+            if (!strcmp(ds_cstr(group_info->group), ds_cstr(ds))) {
+                group_id = group_info->group_id;
+            }
+        }
+
+        if (!group_id) {
+            /* Reserve a new group_id. */
+            for (group_id = 1; group_id < OFPG_MAX; group_id++) {
+                if (!bitmap_is_set(ctx->ap->group_table->group_ids,
+                    group_id)) {
+                    bitmap_set1(ctx->ap->group_table->group_ids, group_id);
+                    break;
+                }
+            }
+        }
+
+        if (group_id == OFPG_MAX) {
+            ds_destroy(ds);
+            free(ds);
+            action_error(ctx, "out of group ids.");
+            return;
+        }
+
+        group_info = xmalloc(sizeof *group_info);
+        group_info->group = ds;
+        group_info->group_id = group_id;
+        group_info->hmap_node.hash = hash;
+
+        hmap_insert(&ctx->ap->group_table->desired_groups,
+                    &group_info->hmap_node, group_info->hmap_node.hash);
+    }
+
+    /* Create an action to set the group. */
+    og = ofpact_put_GROUP(ctx->ofpacts);
+    og->group_id = group_id;
+
+    return;
+}
+
+static void
 emit_ct(struct action_context *ctx, bool recirc_next, bool commit)
 {
     struct ofpact_conntrack *ct = ofpact_put_CT(ctx->ofpacts);
@@ -239,6 +376,10 @@  parse_action(struct action_context *ctx)
         emit_ct(ctx, true, false);
     } else if (lexer_match_id(ctx->lexer, "ct_commit")) {
         emit_ct(ctx, false, true);
+    } else if (lexer_match_id(ctx->lexer, "ct_nat")) {
+        parse_ct_nat_action(ctx);
+    } else if (lexer_match_id(ctx->lexer, "ct_lb")) {
+        parse_ct_lb_action(ctx);
     } else {
         action_syntax_error(ctx, "expecting action");
     }
diff --git a/ovn/lib/actions.h b/ovn/lib/actions.h
index 2c3644a..65d8a93 100644
--- a/ovn/lib/actions.h
+++ b/ovn/lib/actions.h
@@ -19,6 +19,7 @@ 
 
 #include <stdint.h>
 #include "compiler.h"
+#include "hmap.h"
 
 struct expr;
 struct lexer;
@@ -26,6 +27,18 @@  struct ofpbuf;
 struct shash;
 struct simap;
 
+struct group_table {
+    unsigned long *group_ids;
+    struct hmap desired_groups;
+    struct hmap existing_groups;
+};
+
+struct group_info {
+    struct hmap_node hmap_node;
+    struct ds *group;
+    uint32_t group_id;
+};
+
 struct action_params {
     /* A table of "struct expr_symbol"s to support (as one would provide to
      * expr_parse()). */
@@ -39,6 +52,9 @@  struct action_params {
     /* A map from a port name to its connection tracking zone. */
     const struct simap *ct_zones;
 
+    /* A struct to figure out the group_id for group actions. */
+    struct group_table *group_table;
+
     /* 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/ovn/lib/lex.c b/ovn/lib/lex.c
index 481f11e..530bfbb 100644
--- a/ovn/lib/lex.c
+++ b/ovn/lib/lex.c
@@ -781,3 +781,16 @@  lexer_get_int(struct lexer *lexer, int *value)
         return false;
     }
 }
+
+bool
+lexer_get_string(struct lexer *lexer, char **str)
+{
+    if (lexer->token.type == LEX_T_STRING) {
+        *str = xstrdup(lexer->token.s);
+        lexer_get(lexer);
+        return true;
+    } else {
+        *str = NULL;
+        return false;
+    }
+}
diff --git a/ovn/lib/lex.h b/ovn/lib/lex.h
index 3c2bb6a..b0a70dc 100644
--- a/ovn/lib/lex.h
+++ b/ovn/lib/lex.h
@@ -112,5 +112,6 @@  bool lexer_match(struct lexer *, enum lex_type);
 bool lexer_match_id(struct lexer *, const char *id);
 bool lexer_is_int(const struct lexer *);
 bool lexer_get_int(struct lexer *, int *value);
+bool lexer_get_string(struct lexer *, char  **str);
 
 #endif /* ovn/lex.h */