diff mbox series

[ovs-dev,ovn,v2,1/4] ovn-controller: A new action "select".

Message ID 1579560985-36919-1-git-send-email-hzhou@ovn.org
State New
Headers show
Series [ovs-dev,ovn,v2,1/4] ovn-controller: A new action "select". | expand

Commit Message

Han Zhou Jan. 20, 2020, 10:56 p.m. UTC
Support a new logical flow action "select", which can be used to
implement features such as ECMP.  The action uses OpenFlow group
action to select an integer (uint16_t) from a list of integers,
and assign it to specified field, e.g.:
    reg0 = select(1, 2, 3)
A weight can be specified for each member as well, e.g.:
    reg0 = select(1=20, 2=30, 3=50)

Signed-off-by: Han Zhou <hzhou@ovn.org>
---
v1 -> v2: updated according to Numan's comment. Changed the select
    action format from select(<result>, ...) to <result> = select(...).

 include/ovn/actions.h     |  15 ++++++
 lib/actions.c             | 130 ++++++++++++++++++++++++++++++++++++++++++++--
 ovn-sb.xml                |  34 ++++++++++++
 tests/ovn.at              |  23 ++++++++
 tests/test-ovn.c          |  26 +++++++++-
 utilities/ovn-trace.8.xml |   9 ++++
 utilities/ovn-trace.c     |  66 ++++++++++++++++++++++-
 7 files changed, 296 insertions(+), 7 deletions(-)

Comments

Numan Siddique Jan. 22, 2020, 9:16 a.m. UTC | #1
On Tue, Jan 21, 2020 at 4:27 AM Han Zhou <hzhou@ovn.org> wrote:
>
> Support a new logical flow action "select", which can be used to
> implement features such as ECMP.  The action uses OpenFlow group
> action to select an integer (uint16_t) from a list of integers,
> and assign it to specified field, e.g.:
>     reg0 = select(1, 2, 3)
> A weight can be specified for each member as well, e.g.:
>     reg0 = select(1=20, 2=30, 3=50)
>
> Signed-off-by: Han Zhou <hzhou@ovn.org>

Hi Han,

Thanks for v2.

I have one comment. Please see below.

With that addressed - Acked-by: Numan Siddique <numans@ovn.org> for
the entire series.

Thanks
Numan

> ---
> v1 -> v2: updated according to Numan's comment. Changed the select
>     action format from select(<result>, ...) to <result> = select(...).
>
>  include/ovn/actions.h     |  15 ++++++
>  lib/actions.c             | 130 ++++++++++++++++++++++++++++++++++++++++++++--
>  ovn-sb.xml                |  34 ++++++++++++
>  tests/ovn.at              |  23 ++++++++
>  tests/test-ovn.c          |  26 +++++++++-
>  utilities/ovn-trace.8.xml |   9 ++++
>  utilities/ovn-trace.c     |  66 ++++++++++++++++++++++-
>  7 files changed, 296 insertions(+), 7 deletions(-)
>
> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> index 047a8d7..2d4b05b 100644
> --- a/include/ovn/actions.h
> +++ b/include/ovn/actions.h
> @@ -61,6 +61,7 @@ struct ovn_extend_table;
>      OVNACT(CT_DNAT,           ovnact_ct_nat)          \
>      OVNACT(CT_SNAT,           ovnact_ct_nat)          \
>      OVNACT(CT_LB,             ovnact_ct_lb)           \
> +    OVNACT(SELECT,            ovnact_select)          \
>      OVNACT(CT_CLEAR,          ovnact_null)            \
>      OVNACT(CLONE,             ovnact_nest)            \
>      OVNACT(ARP,               ovnact_nest)            \
> @@ -251,6 +252,20 @@ struct ovnact_ct_lb {
>      uint8_t ltable;             /* Logical table ID of next table. */
>  };
>
> +struct ovnact_select_dst {
> +    uint16_t id;
> +    uint16_t weight;
> +};
> +
> +/* OVNACT_SELECT. */
> +struct ovnact_select {
> +    struct ovnact ovnact;
> +    struct ovnact_select_dst *dsts;
> +    size_t n_dsts;
> +    uint8_t ltable;             /* Logical table ID of next table. */
> +    struct expr_field res_field;
> +};
> +
>  /* OVNACT_ARP, OVNACT_ND_NA, OVNACT_CLONE. */
>  struct ovnact_nest {
>      struct ovnact ovnact;
> diff --git a/lib/actions.c b/lib/actions.c
> index 051e6c8..b4743bd 100644
> --- a/lib/actions.c
> +++ b/lib/actions.c
> @@ -218,17 +218,18 @@ action_parse_field(struct action_context *ctx,
>  }
>
>  static bool
> -action_parse_port(struct action_context *ctx, uint16_t *port)
> +action_parse_uint16(struct action_context *ctx, uint16_t *_value,
> +                    const char *msg)
>  {
>      if (lexer_is_int(ctx->lexer)) {
>          int value = ntohll(ctx->lexer->token.value.integer);
>          if (value <= UINT16_MAX) {
> -            *port = value;
> +            *_value = value;
>              lexer_get(ctx->lexer);
>              return true;
>          }
>      }
> -    lexer_syntax_error(ctx->lexer, "expecting port number");
> +    lexer_syntax_error(ctx->lexer, "expecting %s", msg);
>      return false;
>  }
>
> @@ -927,7 +928,7 @@ parse_ct_lb_action(struct action_context *ctx)
>                  }
>                  dst.port = 0;
>                  if (lexer_match(ctx->lexer, LEX_T_COLON)
> -                    && !action_parse_port(ctx, &dst.port)) {
> +                    && !action_parse_uint16(ctx, &dst.port, "port number")) {
>                      free(dsts);
>                      return;
>                  }
> @@ -957,7 +958,8 @@ parse_ct_lb_action(struct action_context *ctx)
>                          lexer_syntax_error(ctx->lexer, "IPv6 address needs "
>                                  "square brackets if port is included");
>                          return;
> -                    } else if (!action_parse_port(ctx, &dst.port)) {
> +                    } else if (!action_parse_uint16(ctx, &dst.port,
> +                                                    "port number")) {
>                          free(dsts);
>                          return;
>                      }
> @@ -1099,6 +1101,121 @@ ovnact_ct_lb_free(struct ovnact_ct_lb *ct_lb)
>  }
>
>  static void
> +parse_select_action(struct action_context *ctx, struct expr_field *res_field)
> +{
> +    if (ctx->pp->cur_ltable >= ctx->pp->n_tables) {
> +        lexer_error(ctx->lexer,
> +                    "\"select\" action not allowed in last table.");
> +        return;
> +    }
> +
> +    struct ovnact_select_dst *dsts = NULL;
> +    size_t allocated_dsts = 0;
> +    size_t n_dsts = 0;

I think it is better to validate the result field to make sure that it
is a writable field with at least 16-bits in it ?

Something like -

char *error = expr_type_check(dst, 16, true);
if (error) {
lexer_error(ctx->lexer, "%s", error);
free(error);
return;
}

I think it's  better  to expect the result register to be at least
16-bit writable field.

I added the below in ovn.at in the action parsing test case and I see
the below output.
If the result field is not a register, is it expected for the
parse_select_action()  to fail ?
I think it should.

****
diff --git a/tests/ovn.at b/tests/ovn.at
index 25ce34d34..241dd26cd 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -1504,6 +1504,9 @@ reg0 = select(1=123456, 2);
 reg0 = select(123);
     Syntax error at `;' expecting at least 2 group members.

+ip4.dst = select(1, 2);
+    Syntax error at `;' expecting at least 2 group members.
+
 # Miscellaneous negative tests.
 ;
     Syntax error at `;'.
****

And the output of testsuite.log is

*********
ip4.dst = select(1, 2);
-    Syntax error at `;' expecting at least 2 group members.
+    formats as ip4.dst = select(1=100, 2=100);
+    encodes as group:6
+    uses group: id(6),
name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:100,actions=load:1->ip_dst[0..31],resubmit(,19),bucket=bucket_id=1,weight:100,actions=load:2->ip_dst[0..31],resubmit(,19))
+    has prereqs eth.type == 0x800
********

Although ovn-northd will use the proper result field, it is still
better to test with invalid result fields.

Thanks
Numan




> +
> +    lexer_get(ctx->lexer); /* Skip "select". */
> +    lexer_get(ctx->lexer); /* Skip '('. */
> +
> +    while (!lexer_match(ctx->lexer, LEX_T_RPAREN)) {
> +        struct ovnact_select_dst dst;
> +        if (!action_parse_uint16(ctx, &dst.id, "id")) {
> +            free(dsts);
> +            return;
> +        }
> +
> +        dst.weight = 0;
> +        if (lexer_match(ctx->lexer, LEX_T_EQUALS)) {
> +            if (!action_parse_uint16(ctx, &dst.weight, "weight")) {
> +                free(dsts);
> +                return;
> +            }
> +            if (dst.weight == 0) {
> +                lexer_syntax_error(ctx->lexer, "weight can't be 0");
> +            }
> +        }
> +        lexer_match(ctx->lexer, LEX_T_COMMA);
> +
> +        /* Append to dsts. */
> +        if (n_dsts >= allocated_dsts) {
> +            dsts = x2nrealloc(dsts, &allocated_dsts, sizeof *dsts);
> +        }
> +        dsts[n_dsts++] = dst;
> +    }
> +    if (n_dsts <= 1) {
> +        lexer_syntax_error(ctx->lexer, "expecting at least 2 group members");
> +        return;
> +    }
> +
> +    struct ovnact_select *select = ovnact_put_SELECT(ctx->ovnacts);
> +    select->ltable = ctx->pp->cur_ltable + 1;
> +    select->dsts = dsts;
> +    select->n_dsts = n_dsts;
> +    select->res_field = *res_field;
> +}
> +
> +static void
> +format_SELECT(const struct ovnact_select *select, struct ds *s)
> +{
> +    expr_field_format(&select->res_field, s);
> +    ds_put_cstr(s, " = ");
> +    ds_put_cstr(s, "select");
> +    ds_put_char(s, '(');
> +    for (size_t i = 0; i < select->n_dsts; i++) {
> +        if (i) {
> +            ds_put_cstr(s, ", ");
> +        }
> +
> +        const struct ovnact_select_dst *dst = &select->dsts[i];
> +        ds_put_format(s, "%"PRIu16, dst->id);
> +        ds_put_format(s, "=%"PRIu16, dst->weight ? dst->weight : 100);
> +    }
> +    ds_put_char(s, ')');
> +    ds_put_char(s, ';');
> +}
> +
> +static void
> +encode_SELECT(const struct ovnact_select *select,
> +             const struct ovnact_encode_params *ep,
> +             struct ofpbuf *ofpacts)
> +{
> +    ovs_assert(select->n_dsts >= 1);
> +    uint8_t resubmit_table = select->ltable + first_ptable(ep, ep->pipeline);
> +    uint32_t table_id = 0;
> +    struct ofpact_group *og;
> +
> +    struct ds ds = DS_EMPTY_INITIALIZER;
> +    ds_put_format(&ds, "type=select,selection_method=dp_hash");
> +
> +    struct mf_subfield sf = expr_resolve_field(&select->res_field);
> +
> +    for (size_t bucket_id = 0; bucket_id < select->n_dsts; bucket_id++) {
> +        const struct ovnact_select_dst *dst = &select->dsts[bucket_id];
> +        ds_put_format(&ds, ",bucket=bucket_id=%"PRIuSIZE",weight:%"PRIu16
> +                      ",actions=", bucket_id, dst->weight ? dst->weight : 100);
> +        ds_put_format(&ds, "load:%u->%s[%u..%u],", dst->id, sf.field->name,
> +                      sf.ofs, sf.ofs + sf.n_bits - 1);
> +        ds_put_format(&ds, "resubmit(,%d)", resubmit_table);
> +    }
> +
> +    table_id = ovn_extend_table_assign_id(ep->group_table, ds_cstr(&ds),
> +                                          ep->lflow_uuid);
> +    ds_destroy(&ds);
> +    if (table_id == EXT_TABLE_ID_INVALID) {
> +        return;
> +    }
> +
> +    /* Create an action to set the group. */
> +    og = ofpact_put_GROUP(ofpacts);
> +    og->group_id = table_id;
> +}
> +
> +static void
> +ovnact_select_free(struct ovnact_select *select)
> +{
> +    free(select->dsts);
> +}
> +
> +static void
>  format_CT_CLEAR(const struct ovnact_null *null OVS_UNUSED, struct ds *s)
>  {
>      ds_put_cstr(s, "ct_clear;");
> @@ -2868,6 +2985,9 @@ parse_set_action(struct action_context *ctx)
>      } else if (lexer_match(ctx->lexer, LEX_T_EQUALS)) {
>          if (ctx->lexer->token.type != LEX_T_ID) {
>              parse_LOAD(ctx, &lhs);
> +        } else if (!strcmp(ctx->lexer->token.s, "select")
> +                   && lexer_lookahead(ctx->lexer) == LEX_T_LPAREN) {
> +            parse_select_action(ctx, &lhs);
>          } else if (!strcmp(ctx->lexer->token.s, "put_dhcp_opts")
>                     && lexer_lookahead(ctx->lexer) == LEX_T_LPAREN) {
>              parse_put_dhcp_opts(ctx, &lhs, ovnact_put_PUT_DHCPV4_OPTS(
> diff --git a/ovn-sb.xml b/ovn-sb.xml
> index 82167c4..9635dcc 100644
> --- a/ovn-sb.xml
> +++ b/ovn-sb.xml
> @@ -2114,6 +2114,40 @@ tcp.flags = RST;
>
>            <p><b>Example:</b> <code>handle_svc_check(inport);</code></p>
>          </dd>
> +
> +        <dt><code><var>R</var> = select(<var>N1</var>[=<var>W1</var>], <var>N2</var>[=<var>W2</var>], ...);</code></dt>
> +        <dd>
> +          <p>
> +            <b>Parameters</b>: Integer <var>N1</var>, <var>N2</var>..., with
> +            optional weight <var>W1</var>, <var>W2</var>, ...
> +          </p>
> +
> +          <p>
> +            <b>Result</b>: stored to a logical field or subfield <var>R</var>.
> +          </p>
> +
> +          <p>
> +            Select from a list of integers <var>N1</var>, <var>N2</var>...,
> +            each within the range 0 ~ 65535, and store the selected one in the
> +            field <var>R</var>.  There must be 2 or more integers listed, each
> +            with an optional weight, which is an integer within the range 1 ~
> +            65535.  If weight is not specified, it defaults to 100. The
> +            selection method is based on the 5-tuple hash of packet header.
> +          </p>
> +
> +          <p>
> +            Processing automatically moves on to the next table, as if
> +            <code>next;</code> were specified.  The <code>select</code> action
> +            must be put as the last action of the logical flow when there are
> +            multiple actions (actions put after <code>select</code> will not
> +            take effect).
> +          </p>
> +
> +          <p>
> +            <b>Example:</b> <code>reg8[16..31] = select(1=20, 2=30, 3=50);</code>
> +          </p>
> +        </dd>
> +
>        </dl>
>      </column>
>
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 411b768..de80bce 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -965,14 +965,17 @@ ct_lb();
>      has prereqs ip
>  ct_lb(192.168.1.2:80, 192.168.1.3:80);
>      encodes as group:1
> +    uses group: id(1), name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:100,actions=ct(nat(dst=192.168.1.2:80),commit,table=19,zone=NXM_NX_REG13[0..15]),bucket=bucket_id=1,weight:100,actions=ct(nat(dst=192.168.1.3:80),commit,table=19,zone=NXM_NX_REG13[0..15]))
>      has prereqs ip
>  ct_lb(192.168.1.2, 192.168.1.3, );
>      formats as ct_lb(192.168.1.2, 192.168.1.3);
>      encodes as group:2
> +    uses group: id(2), name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:100,actions=ct(nat(dst=192.168.1.2),commit,table=19,zone=NXM_NX_REG13[0..15]),bucket=bucket_id=1,weight:100,actions=ct(nat(dst=192.168.1.3),commit,table=19,zone=NXM_NX_REG13[0..15]))
>      has prereqs ip
>  ct_lb(fd0f::2, fd0f::3, );
>      formats as ct_lb(fd0f::2, fd0f::3);
>      encodes as group:3
> +    uses group: id(3), name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:100,actions=ct(nat(dst=fd0f::2),commit,table=19,zone=NXM_NX_REG13[0..15]),bucket=bucket_id=1,weight:100,actions=ct(nat(dst=fd0f::3),commit,table=19,zone=NXM_NX_REG13[0..15]))
>      has prereqs ip
>
>  ct_lb(192.168.1.2:);
> @@ -1481,6 +1484,26 @@ handle_svc_check();
>  handle_svc_check(reg0);
>      Cannot use numeric field reg0 where string field is required.
>
> +# select
> +reg9[16..31] = select(1=50, 2=100, 3, );
> +    formats as reg9[16..31] = select(1=50, 2=100, 3=100);
> +    encodes as group:4
> +    uses group: id(4), name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:50,actions=load:1->xreg4[16..31],resubmit(,19),bucket=bucket_id=1,weight:100,actions=load:2->xreg4[16..31],resubmit(,19),bucket=bucket_id=2,weight:100,actions=load:3->xreg4[16..31],resubmit(,19))
> +
> +reg0 = select(1, 2);
> +    formats as reg0 = select(1=100, 2=100);
> +    encodes as group:5
> +    uses group: id(5), name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:100,actions=load:1->xxreg0[96..127],resubmit(,19),bucket=bucket_id=1,weight:100,actions=load:2->xxreg0[96..127],resubmit(,19))
> +
> +reg0 = select(1=, 2);
> +    Syntax error at `,' expecting weight.
> +reg0 = select(1=0, 2);
> +    Syntax error at `,' weight can't be 0.
> +reg0 = select(1=123456, 2);
> +    Syntax error at `123456' expecting weight.
> +reg0 = select(123);
> +    Syntax error at `;' expecting at least 2 group members.
> +
>  # Miscellaneous negative tests.
>  ;
>      Syntax error at `;'.
> diff --git a/tests/test-ovn.c b/tests/test-ovn.c
> index 5366905..476bc33 100644
> --- a/tests/test-ovn.c
> +++ b/tests/test-ovn.c
> @@ -1227,6 +1227,28 @@ test_expr_to_packets(struct ovs_cmdl_context *ctx OVS_UNUSED)
>  /* Actions. */
>
>  static void
> +print_group_info(struct ovn_extend_table *group_table, const char *action)
> +{
> +    struct ovn_extend_table_info *g;
> +    HMAP_FOR_EACH (g, hmap_node, &group_table->desired) {
> +        char buf[64];
> +        sprintf(buf, "group:%"PRIu32, g->table_id);
> +        char *match = strstr(buf, action);
> +        if (match) {
> +            if (match[strlen(buf)] != '\0') {
> +                /* Add ',' and match again. */
> +                sprintf(buf, "group:%"PRIu32",", g->table_id);
> +                match = strstr(buf, action);
> +            }
> +        }
> +        if (match) {
> +            printf("    uses group: id(%"PRIu32"), name(%s)\n",
> +                   g->table_id, g->name);
> +        }
> +    }
> +}
> +
> +static void
>  test_parse_actions(struct ovs_cmdl_context *ctx OVS_UNUSED)
>  {
>      struct shash symtab;
> @@ -1305,7 +1327,9 @@ test_parse_actions(struct ovs_cmdl_context *ctx OVS_UNUSED)
>              struct ds ofpacts_s = DS_EMPTY_INITIALIZER;
>              struct ofpact_format_params fp = { .s = &ofpacts_s };
>              ofpacts_format(ofpacts.data, ofpacts.size, &fp);
> -            printf("    encodes as %s\n", ds_cstr(&ofpacts_s));
> +            char *ofpacts_cstr = ds_cstr(&ofpacts_s);
> +            printf("    encodes as %s\n", ofpacts_cstr);
> +            print_group_info(&group_table, ofpacts_cstr);
>              ds_destroy(&ofpacts_s);
>              ofpbuf_uninit(&ofpacts);
>
> diff --git a/utilities/ovn-trace.8.xml b/utilities/ovn-trace.8.xml
> index 01e7411..485928c 100644
> --- a/utilities/ovn-trace.8.xml
> +++ b/utilities/ovn-trace.8.xml
> @@ -438,6 +438,15 @@
>        <code>--lb-dst</code> is not available in daemon mode.
>      </dd>
>
> +    <dt><code>--select-id=</code><var>id</var></dt>
> +    <dd>
> +      Specify the <var>id</var> to be selected by the <code>select</code>
> +      action. <var>id</var> must be one of the values listed in the
> +      <code>select</code> action.  Otherwise, a random id is selected from
> +      the list, as if <code>--select-id</code> were not specified.
> +      <code>--select-id</code> is not available in daemon mode.
> +    </dd>
> +
>      <dt><code>--friendly-names</code></dt>
>      <dt><code>--no-friendly-names</code></dt>
>      <dd>
> diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
> index 2645438..a97195b 100644
> --- a/utilities/ovn-trace.c
> +++ b/utilities/ovn-trace.c
> @@ -81,6 +81,9 @@ static size_t ct_state_idx;
>  /* --lb-dst: load balancer destination info. */
>  static struct ovnact_ct_lb_dst lb_dst;
>
> +/* --select-id: "select" action member id. */
> +static uint16_t select_id;
> +
>  /* --friendly-names, --no-friendly-names: Whether to substitute human-friendly
>   * port and datapath names for the awkward UUIDs typically used in the actual
>   * logical flows. */
> @@ -209,6 +212,16 @@ parse_lb_option(const char *s)
>  }
>
>  static void
> +parse_select_option(const char *s)
> +{
> +    unsigned long int integer = strtoul(s, NULL, 10);
> +    if (!integer) {
> +        ovs_fatal(0, "%s: bad select-id", s);
> +    }
> +    select_id = integer;
> +}
> +
> +static void
>  parse_options(int argc, char *argv[])
>  {
>      enum {
> @@ -225,7 +238,8 @@ parse_options(int argc, char *argv[])
>          DAEMON_OPTION_ENUMS,
>          SSL_OPTION_ENUMS,
>          VLOG_OPTION_ENUMS,
> -        OPT_LB_DST
> +        OPT_LB_DST,
> +        OPT_SELECT_ID
>      };
>      static const struct option long_options[] = {
>          {"db", required_argument, NULL, OPT_DB},
> @@ -241,6 +255,7 @@ parse_options(int argc, char *argv[])
>          {"help", no_argument, NULL, 'h'},
>          {"version", no_argument, NULL, 'V'},
>          {"lb-dst", required_argument, NULL, OPT_LB_DST},
> +        {"select-id", required_argument, NULL, OPT_SELECT_ID},
>          DAEMON_LONG_OPTIONS,
>          VLOG_LONG_OPTIONS,
>          STREAM_SSL_LONG_OPTIONS,
> @@ -302,6 +317,10 @@ parse_options(int argc, char *argv[])
>              parse_lb_option(optarg);
>              break;
>
> +        case OPT_SELECT_ID:
> +            parse_select_option(optarg);
> +            break;
> +
>          case 'h':
>              usage();
>
> @@ -1991,6 +2010,46 @@ execute_ct_lb(const struct ovnact_ct_lb *ct_lb,
>  }
>
>  static void
> +execute_select(const struct ovnact_select *select,
> +              const struct ovntrace_datapath *dp, struct flow *uflow,
> +              enum ovnact_pipeline pipeline, struct ovs_list *super)
> +{
> +    struct flow select_flow = *uflow;
> +
> +    const struct ovnact_select_dst *dst = NULL;
> +    ovs_assert(select->n_dsts > 1);
> +    bool user_specified = false;
> +    for (int i = 0; i < select->n_dsts; i++) {
> +        const struct ovnact_select_dst *d = &select->dsts[i];
> +
> +        /* Check for the dst specified by --select-id, if any. */
> +        if (select_id == d->id) {
> +            dst = d;
> +            user_specified = true;
> +            break;
> +        }
> +    }
> +
> +    if (!dst) {
> +        /* Select a random dst as a fallback. */
> +        dst = &select->dsts[random_range(select->n_dsts)];
> +    }
> +
> +    struct mf_subfield sf = expr_resolve_field(&select->res_field);
> +    union mf_subvalue sv = { .be16_int = htons(dst->id) };
> +    mf_write_subfield_flow(&sf, &sv, &select_flow);
> +    struct ds sf_str = DS_EMPTY_INITIALIZER;
> +    expr_field_format(&select->res_field, &sf_str);
> +    struct ovntrace_node *node = ovntrace_node_append(
> +        super, OVNTRACE_NODE_TRANSFORMATION,
> +        "select: %s = %"PRIu16"%s",
> +        ds_cstr(&sf_str), dst->id, user_specified ?  "" :
> +        " /* Randomly selected. Use --select-id to specify. */");
> +    ds_destroy(&sf_str);
> +    trace__(dp, &select_flow, select->ltable, pipeline, &node->subs);
> +}
> +
> +static void
>  execute_log(const struct ovnact_log *log, struct flow *uflow,
>              struct ovs_list *super)
>  {
> @@ -2100,6 +2159,11 @@ trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len,
>              execute_ct_lb(ovnact_get_CT_LB(a), dp, uflow, pipeline, super);
>              break;
>
> +        case OVNACT_SELECT:
> +            execute_select(ovnact_get_SELECT(a), dp, uflow,
> +                                   pipeline, super);
> +            break;
> +
>          case OVNACT_CT_CLEAR:
>              flow_clear_conntrack(uflow);
>              break;
> --
> 2.1.0
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Han Zhou Jan. 22, 2020, 4:15 p.m. UTC | #2
On Wed, Jan 22, 2020 at 1:17 AM Numan Siddique <numans@ovn.org> wrote:
>
> On Tue, Jan 21, 2020 at 4:27 AM Han Zhou <hzhou@ovn.org> wrote:
> >
> > Support a new logical flow action "select", which can be used to
> > implement features such as ECMP.  The action uses OpenFlow group
> > action to select an integer (uint16_t) from a list of integers,
> > and assign it to specified field, e.g.:
> >     reg0 = select(1, 2, 3)
> > A weight can be specified for each member as well, e.g.:
> >     reg0 = select(1=20, 2=30, 3=50)
> >
> > Signed-off-by: Han Zhou <hzhou@ovn.org>
>
> Hi Han,
>
> Thanks for v2.
>
> I have one comment. Please see below.
>
> With that addressed - Acked-by: Numan Siddique <numans@ovn.org> for
> the entire series.
>
> Thanks
> Numan
>
> > ---
> > v1 -> v2: updated according to Numan's comment. Changed the select
> >     action format from select(<result>, ...) to <result> = select(...).
> >
> >  include/ovn/actions.h     |  15 ++++++
> >  lib/actions.c             | 130
++++++++++++++++++++++++++++++++++++++++++++--
> >  ovn-sb.xml                |  34 ++++++++++++
> >  tests/ovn.at              |  23 ++++++++
> >  tests/test-ovn.c          |  26 +++++++++-
> >  utilities/ovn-trace.8.xml |   9 ++++
> >  utilities/ovn-trace.c     |  66 ++++++++++++++++++++++-
> >  7 files changed, 296 insertions(+), 7 deletions(-)
> >
> > diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> > index 047a8d7..2d4b05b 100644
> > --- a/include/ovn/actions.h
> > +++ b/include/ovn/actions.h
> > @@ -61,6 +61,7 @@ struct ovn_extend_table;
> >      OVNACT(CT_DNAT,           ovnact_ct_nat)          \
> >      OVNACT(CT_SNAT,           ovnact_ct_nat)          \
> >      OVNACT(CT_LB,             ovnact_ct_lb)           \
> > +    OVNACT(SELECT,            ovnact_select)          \
> >      OVNACT(CT_CLEAR,          ovnact_null)            \
> >      OVNACT(CLONE,             ovnact_nest)            \
> >      OVNACT(ARP,               ovnact_nest)            \
> > @@ -251,6 +252,20 @@ struct ovnact_ct_lb {
> >      uint8_t ltable;             /* Logical table ID of next table. */
> >  };
> >
> > +struct ovnact_select_dst {
> > +    uint16_t id;
> > +    uint16_t weight;
> > +};
> > +
> > +/* OVNACT_SELECT. */
> > +struct ovnact_select {
> > +    struct ovnact ovnact;
> > +    struct ovnact_select_dst *dsts;
> > +    size_t n_dsts;
> > +    uint8_t ltable;             /* Logical table ID of next table. */
> > +    struct expr_field res_field;
> > +};
> > +
> >  /* OVNACT_ARP, OVNACT_ND_NA, OVNACT_CLONE. */
> >  struct ovnact_nest {
> >      struct ovnact ovnact;
> > diff --git a/lib/actions.c b/lib/actions.c
> > index 051e6c8..b4743bd 100644
> > --- a/lib/actions.c
> > +++ b/lib/actions.c
> > @@ -218,17 +218,18 @@ action_parse_field(struct action_context *ctx,
> >  }
> >
> >  static bool
> > -action_parse_port(struct action_context *ctx, uint16_t *port)
> > +action_parse_uint16(struct action_context *ctx, uint16_t *_value,
> > +                    const char *msg)
> >  {
> >      if (lexer_is_int(ctx->lexer)) {
> >          int value = ntohll(ctx->lexer->token.value.integer);
> >          if (value <= UINT16_MAX) {
> > -            *port = value;
> > +            *_value = value;
> >              lexer_get(ctx->lexer);
> >              return true;
> >          }
> >      }
> > -    lexer_syntax_error(ctx->lexer, "expecting port number");
> > +    lexer_syntax_error(ctx->lexer, "expecting %s", msg);
> >      return false;
> >  }
> >
> > @@ -927,7 +928,7 @@ parse_ct_lb_action(struct action_context *ctx)
> >                  }
> >                  dst.port = 0;
> >                  if (lexer_match(ctx->lexer, LEX_T_COLON)
> > -                    && !action_parse_port(ctx, &dst.port)) {
> > +                    && !action_parse_uint16(ctx, &dst.port, "port
number")) {
> >                      free(dsts);
> >                      return;
> >                  }
> > @@ -957,7 +958,8 @@ parse_ct_lb_action(struct action_context *ctx)
> >                          lexer_syntax_error(ctx->lexer, "IPv6 address
needs "
> >                                  "square brackets if port is included");
> >                          return;
> > -                    } else if (!action_parse_port(ctx, &dst.port)) {
> > +                    } else if (!action_parse_uint16(ctx, &dst.port,
> > +                                                    "port number")) {
> >                          free(dsts);
> >                          return;
> >                      }
> > @@ -1099,6 +1101,121 @@ ovnact_ct_lb_free(struct ovnact_ct_lb *ct_lb)
> >  }
> >
> >  static void
> > +parse_select_action(struct action_context *ctx, struct expr_field
*res_field)
> > +{
> > +    if (ctx->pp->cur_ltable >= ctx->pp->n_tables) {
> > +        lexer_error(ctx->lexer,
> > +                    "\"select\" action not allowed in last table.");
> > +        return;
> > +    }
> > +
> > +    struct ovnact_select_dst *dsts = NULL;
> > +    size_t allocated_dsts = 0;
> > +    size_t n_dsts = 0;
>
> I think it is better to validate the result field to make sure that it
> is a writable field with at least 16-bits in it ?
>
> Something like -
>
> char *error = expr_type_check(dst, 16, true);
> if (error) {
> lexer_error(ctx->lexer, "%s", error);
> free(error);
> return;
> }
>
> I think it's  better  to expect the result register to be at least
> 16-bit writable field.
>
> I added the below in ovn.at in the action parsing test case and I see
> the below output.
> If the result field is not a register, is it expected for the
> parse_select_action()  to fail ?
> I think it should.
>
> ****
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 25ce34d34..241dd26cd 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -1504,6 +1504,9 @@ reg0 = select(1=123456, 2);
>  reg0 = select(123);
>      Syntax error at `;' expecting at least 2 group members.
>
> +ip4.dst = select(1, 2);
> +    Syntax error at `;' expecting at least 2 group members.
> +
>  # Miscellaneous negative tests.
>  ;
>      Syntax error at `;'.
> ****
>
> And the output of testsuite.log is
>
> *********
> ip4.dst = select(1, 2);
> -    Syntax error at `;' expecting at least 2 group members.
> +    formats as ip4.dst = select(1=100, 2=100);
> +    encodes as group:6
> +    uses group: id(6),
>
name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:100,actions=load:1->ip_dst[0..31],resubmit(,19),bucket=bucket_id=1,weight:100,actions=load:2->ip_dst[0..31],resubmit(,19))
> +    has prereqs eth.type == 0x800
> ********
>
> Although ovn-northd will use the proper result field, it is still
> better to test with invalid result fields.
>
> Thanks
> Numan

Thanks Numan for the great suggestion.
I updated the patch as suggested by slightly different, as the result field
can have 16 *or more* bits. Please check the diff below and confirm if it
looks ok:

- - - - - - -8>< - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
- - - - - - - - - ><8 - - - - - - - -
diff --git a/lib/actions.c b/lib/actions.c
index b4743bd..cd3f586 100644
--- a/lib/actions.c
+++ b/lib/actions.c
@@ -1103,6 +1103,23 @@ ovnact_ct_lb_free(struct ovnact_ct_lb *ct_lb)
 static void
 parse_select_action(struct action_context *ctx, struct expr_field
*res_field)
 {
+    /* Check if the result field is modifiable. */
+    char *error = expr_type_check(res_field, res_field->n_bits, true);
+    if (error) {
+        lexer_error(ctx->lexer, "%s", error);
+        free(error);
+        return;
+    }
+
+    if (res_field->n_bits < 16) {
+        lexer_error(ctx->lexer, "cannot use %d-bit field %s[%d..%d] "
+                    "for \"select\", which requires at least 16 bits.",
+                    res_field->n_bits, res_field->symbol->name,
+                    res_field->ofs,
+                    res_field->ofs + res_field->n_bits - 1);
+        return;
+    }
+
     if (ctx->pp->cur_ltable >= ctx->pp->n_tables) {
         lexer_error(ctx->lexer,
                     "\"select\" action not allowed in last table.");
diff --git a/tests/ovn.at b/tests/ovn.at
index 25ce34d..f245083 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -1503,6 +1503,10 @@ reg0 = select(1=123456, 2);
     Syntax error at `123456' expecting weight.
 reg0 = select(123);
     Syntax error at `;' expecting at least 2 group members.
+ip.proto = select(1, 2, 3);
+    Field ip.proto is not modifiable.
+reg0[0..14] = select(1, 2, 3);
+    cannot use 15-bit field reg0[0..14] for "select", which requires at
least 16 bits.

 # Miscellaneous negative tests.
 ;
Numan Siddique Jan. 22, 2020, 6:10 p.m. UTC | #3
On Wed, Jan 22, 2020 at 9:46 PM Han Zhou <hzhou@ovn.org> wrote:
>
> On Wed, Jan 22, 2020 at 1:17 AM Numan Siddique <numans@ovn.org> wrote:
> >
> > On Tue, Jan 21, 2020 at 4:27 AM Han Zhou <hzhou@ovn.org> wrote:
> > >
> > > Support a new logical flow action "select", which can be used to
> > > implement features such as ECMP.  The action uses OpenFlow group
> > > action to select an integer (uint16_t) from a list of integers,
> > > and assign it to specified field, e.g.:
> > >     reg0 = select(1, 2, 3)
> > > A weight can be specified for each member as well, e.g.:
> > >     reg0 = select(1=20, 2=30, 3=50)
> > >
> > > Signed-off-by: Han Zhou <hzhou@ovn.org>
> >
> > Hi Han,
> >
> > Thanks for v2.
> >
> > I have one comment. Please see below.
> >
> > With that addressed - Acked-by: Numan Siddique <numans@ovn.org> for
> > the entire series.
> >
> > Thanks
> > Numan
> >
> > > ---
> > > v1 -> v2: updated according to Numan's comment. Changed the select
> > >     action format from select(<result>, ...) to <result> = select(...).
> > >
> > >  include/ovn/actions.h     |  15 ++++++
> > >  lib/actions.c             | 130
> ++++++++++++++++++++++++++++++++++++++++++++--
> > >  ovn-sb.xml                |  34 ++++++++++++
> > >  tests/ovn.at              |  23 ++++++++
> > >  tests/test-ovn.c          |  26 +++++++++-
> > >  utilities/ovn-trace.8.xml |   9 ++++
> > >  utilities/ovn-trace.c     |  66 ++++++++++++++++++++++-
> > >  7 files changed, 296 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> > > index 047a8d7..2d4b05b 100644
> > > --- a/include/ovn/actions.h
> > > +++ b/include/ovn/actions.h
> > > @@ -61,6 +61,7 @@ struct ovn_extend_table;
> > >      OVNACT(CT_DNAT,           ovnact_ct_nat)          \
> > >      OVNACT(CT_SNAT,           ovnact_ct_nat)          \
> > >      OVNACT(CT_LB,             ovnact_ct_lb)           \
> > > +    OVNACT(SELECT,            ovnact_select)          \
> > >      OVNACT(CT_CLEAR,          ovnact_null)            \
> > >      OVNACT(CLONE,             ovnact_nest)            \
> > >      OVNACT(ARP,               ovnact_nest)            \
> > > @@ -251,6 +252,20 @@ struct ovnact_ct_lb {
> > >      uint8_t ltable;             /* Logical table ID of next table. */
> > >  };
> > >
> > > +struct ovnact_select_dst {
> > > +    uint16_t id;
> > > +    uint16_t weight;
> > > +};
> > > +
> > > +/* OVNACT_SELECT. */
> > > +struct ovnact_select {
> > > +    struct ovnact ovnact;
> > > +    struct ovnact_select_dst *dsts;
> > > +    size_t n_dsts;
> > > +    uint8_t ltable;             /* Logical table ID of next table. */
> > > +    struct expr_field res_field;
> > > +};
> > > +
> > >  /* OVNACT_ARP, OVNACT_ND_NA, OVNACT_CLONE. */
> > >  struct ovnact_nest {
> > >      struct ovnact ovnact;
> > > diff --git a/lib/actions.c b/lib/actions.c
> > > index 051e6c8..b4743bd 100644
> > > --- a/lib/actions.c
> > > +++ b/lib/actions.c
> > > @@ -218,17 +218,18 @@ action_parse_field(struct action_context *ctx,
> > >  }
> > >
> > >  static bool
> > > -action_parse_port(struct action_context *ctx, uint16_t *port)
> > > +action_parse_uint16(struct action_context *ctx, uint16_t *_value,
> > > +                    const char *msg)
> > >  {
> > >      if (lexer_is_int(ctx->lexer)) {
> > >          int value = ntohll(ctx->lexer->token.value.integer);
> > >          if (value <= UINT16_MAX) {
> > > -            *port = value;
> > > +            *_value = value;
> > >              lexer_get(ctx->lexer);
> > >              return true;
> > >          }
> > >      }
> > > -    lexer_syntax_error(ctx->lexer, "expecting port number");
> > > +    lexer_syntax_error(ctx->lexer, "expecting %s", msg);
> > >      return false;
> > >  }
> > >
> > > @@ -927,7 +928,7 @@ parse_ct_lb_action(struct action_context *ctx)
> > >                  }
> > >                  dst.port = 0;
> > >                  if (lexer_match(ctx->lexer, LEX_T_COLON)
> > > -                    && !action_parse_port(ctx, &dst.port)) {
> > > +                    && !action_parse_uint16(ctx, &dst.port, "port
> number")) {
> > >                      free(dsts);
> > >                      return;
> > >                  }
> > > @@ -957,7 +958,8 @@ parse_ct_lb_action(struct action_context *ctx)
> > >                          lexer_syntax_error(ctx->lexer, "IPv6 address
> needs "
> > >                                  "square brackets if port is included");
> > >                          return;
> > > -                    } else if (!action_parse_port(ctx, &dst.port)) {
> > > +                    } else if (!action_parse_uint16(ctx, &dst.port,
> > > +                                                    "port number")) {
> > >                          free(dsts);
> > >                          return;
> > >                      }
> > > @@ -1099,6 +1101,121 @@ ovnact_ct_lb_free(struct ovnact_ct_lb *ct_lb)
> > >  }
> > >
> > >  static void
> > > +parse_select_action(struct action_context *ctx, struct expr_field
> *res_field)
> > > +{
> > > +    if (ctx->pp->cur_ltable >= ctx->pp->n_tables) {
> > > +        lexer_error(ctx->lexer,
> > > +                    "\"select\" action not allowed in last table.");
> > > +        return;
> > > +    }
> > > +
> > > +    struct ovnact_select_dst *dsts = NULL;
> > > +    size_t allocated_dsts = 0;
> > > +    size_t n_dsts = 0;
> >
> > I think it is better to validate the result field to make sure that it
> > is a writable field with at least 16-bits in it ?
> >
> > Something like -
> >
> > char *error = expr_type_check(dst, 16, true);
> > if (error) {
> > lexer_error(ctx->lexer, "%s", error);
> > free(error);
> > return;
> > }
> >
> > I think it's  better  to expect the result register to be at least
> > 16-bit writable field.
> >
> > I added the below in ovn.at in the action parsing test case and I see
> > the below output.
> > If the result field is not a register, is it expected for the
> > parse_select_action()  to fail ?
> > I think it should.
> >
> > ****
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 25ce34d34..241dd26cd 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -1504,6 +1504,9 @@ reg0 = select(1=123456, 2);
> >  reg0 = select(123);
> >      Syntax error at `;' expecting at least 2 group members.
> >
> > +ip4.dst = select(1, 2);
> > +    Syntax error at `;' expecting at least 2 group members.
> > +
> >  # Miscellaneous negative tests.
> >  ;
> >      Syntax error at `;'.
> > ****
> >
> > And the output of testsuite.log is
> >
> > *********
> > ip4.dst = select(1, 2);
> > -    Syntax error at `;' expecting at least 2 group members.
> > +    formats as ip4.dst = select(1=100, 2=100);
> > +    encodes as group:6
> > +    uses group: id(6),
> >
> name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:100,actions=load:1->ip_dst[0..31],resubmit(,19),bucket=bucket_id=1,weight:100,actions=load:2->ip_dst[0..31],resubmit(,19))
> > +    has prereqs eth.type == 0x800
> > ********
> >
> > Although ovn-northd will use the proper result field, it is still
> > better to test with invalid result fields.
> >
> > Thanks
> > Numan
>
> Thanks Numan for the great suggestion.
> I updated the patch as suggested by slightly different, as the result field
> can have 16 *or more* bits. Please check the diff below and confirm if it
> looks ok:

Looks good to me.

Acked-by: Numan Siddique <numans@ovn.org>

Thanks
Numan

>
> - - - - - - -8>< - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
> - - - - - - - - - ><8 - - - - - - - -
> diff --git a/lib/actions.c b/lib/actions.c
> index b4743bd..cd3f586 100644
> --- a/lib/actions.c
> +++ b/lib/actions.c
> @@ -1103,6 +1103,23 @@ ovnact_ct_lb_free(struct ovnact_ct_lb *ct_lb)
>  static void
>  parse_select_action(struct action_context *ctx, struct expr_field
> *res_field)
>  {
> +    /* Check if the result field is modifiable. */
> +    char *error = expr_type_check(res_field, res_field->n_bits, true);
> +    if (error) {
> +        lexer_error(ctx->lexer, "%s", error);
> +        free(error);
> +        return;
> +    }
> +
> +    if (res_field->n_bits < 16) {
> +        lexer_error(ctx->lexer, "cannot use %d-bit field %s[%d..%d] "
> +                    "for \"select\", which requires at least 16 bits.",
> +                    res_field->n_bits, res_field->symbol->name,
> +                    res_field->ofs,
> +                    res_field->ofs + res_field->n_bits - 1);
> +        return;
> +    }
> +
>      if (ctx->pp->cur_ltable >= ctx->pp->n_tables) {
>          lexer_error(ctx->lexer,
>                      "\"select\" action not allowed in last table.");
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 25ce34d..f245083 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -1503,6 +1503,10 @@ reg0 = select(1=123456, 2);
>      Syntax error at `123456' expecting weight.
>  reg0 = select(123);
>      Syntax error at `;' expecting at least 2 group members.
> +ip.proto = select(1, 2, 3);
> +    Field ip.proto is not modifiable.
> +reg0[0..14] = select(1, 2, 3);
> +    cannot use 15-bit field reg0[0..14] for "select", which requires at
> least 16 bits.
>
>  # Miscellaneous negative tests.
>  ;
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Han Zhou Jan. 22, 2020, 7:56 p.m. UTC | #4
On Wed, Jan 22, 2020 at 10:10 AM Numan Siddique <numans@ovn.org> wrote:
>
> On Wed, Jan 22, 2020 at 9:46 PM Han Zhou <hzhou@ovn.org> wrote:
> >
> > On Wed, Jan 22, 2020 at 1:17 AM Numan Siddique <numans@ovn.org> wrote:
> > >
> > > On Tue, Jan 21, 2020 at 4:27 AM Han Zhou <hzhou@ovn.org> wrote:
> > > >
> > > > Support a new logical flow action "select", which can be used to
> > > > implement features such as ECMP.  The action uses OpenFlow group
> > > > action to select an integer (uint16_t) from a list of integers,
> > > > and assign it to specified field, e.g.:
> > > >     reg0 = select(1, 2, 3)
> > > > A weight can be specified for each member as well, e.g.:
> > > >     reg0 = select(1=20, 2=30, 3=50)
> > > >
> > > > Signed-off-by: Han Zhou <hzhou@ovn.org>
> > >
> > > Hi Han,
> > >
> > > Thanks for v2.
> > >
> > > I have one comment. Please see below.
> > >
> > > With that addressed - Acked-by: Numan Siddique <numans@ovn.org> for
> > > the entire series.
> > >
> > > Thanks
> > > Numan
> > >
> > > > ---
> > > > v1 -> v2: updated according to Numan's comment. Changed the select
> > > >     action format from select(<result>, ...) to <result> =
select(...).
> > > >
> > > >  include/ovn/actions.h     |  15 ++++++
> > > >  lib/actions.c             | 130
> > ++++++++++++++++++++++++++++++++++++++++++++--
> > > >  ovn-sb.xml                |  34 ++++++++++++
> > > >  tests/ovn.at              |  23 ++++++++
> > > >  tests/test-ovn.c          |  26 +++++++++-
> > > >  utilities/ovn-trace.8.xml |   9 ++++
> > > >  utilities/ovn-trace.c     |  66 ++++++++++++++++++++++-
> > > >  7 files changed, 296 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> > > > index 047a8d7..2d4b05b 100644
> > > > --- a/include/ovn/actions.h
> > > > +++ b/include/ovn/actions.h
> > > > @@ -61,6 +61,7 @@ struct ovn_extend_table;
> > > >      OVNACT(CT_DNAT,           ovnact_ct_nat)          \
> > > >      OVNACT(CT_SNAT,           ovnact_ct_nat)          \
> > > >      OVNACT(CT_LB,             ovnact_ct_lb)           \
> > > > +    OVNACT(SELECT,            ovnact_select)          \
> > > >      OVNACT(CT_CLEAR,          ovnact_null)            \
> > > >      OVNACT(CLONE,             ovnact_nest)            \
> > > >      OVNACT(ARP,               ovnact_nest)            \
> > > > @@ -251,6 +252,20 @@ struct ovnact_ct_lb {
> > > >      uint8_t ltable;             /* Logical table ID of next table.
*/
> > > >  };
> > > >
> > > > +struct ovnact_select_dst {
> > > > +    uint16_t id;
> > > > +    uint16_t weight;
> > > > +};
> > > > +
> > > > +/* OVNACT_SELECT. */
> > > > +struct ovnact_select {
> > > > +    struct ovnact ovnact;
> > > > +    struct ovnact_select_dst *dsts;
> > > > +    size_t n_dsts;
> > > > +    uint8_t ltable;             /* Logical table ID of next table.
*/
> > > > +    struct expr_field res_field;
> > > > +};
> > > > +
> > > >  /* OVNACT_ARP, OVNACT_ND_NA, OVNACT_CLONE. */
> > > >  struct ovnact_nest {
> > > >      struct ovnact ovnact;
> > > > diff --git a/lib/actions.c b/lib/actions.c
> > > > index 051e6c8..b4743bd 100644
> > > > --- a/lib/actions.c
> > > > +++ b/lib/actions.c
> > > > @@ -218,17 +218,18 @@ action_parse_field(struct action_context *ctx,
> > > >  }
> > > >
> > > >  static bool
> > > > -action_parse_port(struct action_context *ctx, uint16_t *port)
> > > > +action_parse_uint16(struct action_context *ctx, uint16_t *_value,
> > > > +                    const char *msg)
> > > >  {
> > > >      if (lexer_is_int(ctx->lexer)) {
> > > >          int value = ntohll(ctx->lexer->token.value.integer);
> > > >          if (value <= UINT16_MAX) {
> > > > -            *port = value;
> > > > +            *_value = value;
> > > >              lexer_get(ctx->lexer);
> > > >              return true;
> > > >          }
> > > >      }
> > > > -    lexer_syntax_error(ctx->lexer, "expecting port number");
> > > > +    lexer_syntax_error(ctx->lexer, "expecting %s", msg);
> > > >      return false;
> > > >  }
> > > >
> > > > @@ -927,7 +928,7 @@ parse_ct_lb_action(struct action_context *ctx)
> > > >                  }
> > > >                  dst.port = 0;
> > > >                  if (lexer_match(ctx->lexer, LEX_T_COLON)
> > > > -                    && !action_parse_port(ctx, &dst.port)) {
> > > > +                    && !action_parse_uint16(ctx, &dst.port, "port
> > number")) {
> > > >                      free(dsts);
> > > >                      return;
> > > >                  }
> > > > @@ -957,7 +958,8 @@ parse_ct_lb_action(struct action_context *ctx)
> > > >                          lexer_syntax_error(ctx->lexer, "IPv6
address
> > needs "
> > > >                                  "square brackets if port is
included");
> > > >                          return;
> > > > -                    } else if (!action_parse_port(ctx, &dst.port))
{
> > > > +                    } else if (!action_parse_uint16(ctx, &dst.port,
> > > > +                                                    "port
number")) {
> > > >                          free(dsts);
> > > >                          return;
> > > >                      }
> > > > @@ -1099,6 +1101,121 @@ ovnact_ct_lb_free(struct ovnact_ct_lb
*ct_lb)
> > > >  }
> > > >
> > > >  static void
> > > > +parse_select_action(struct action_context *ctx, struct expr_field
> > *res_field)
> > > > +{
> > > > +    if (ctx->pp->cur_ltable >= ctx->pp->n_tables) {
> > > > +        lexer_error(ctx->lexer,
> > > > +                    "\"select\" action not allowed in last
table.");
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    struct ovnact_select_dst *dsts = NULL;
> > > > +    size_t allocated_dsts = 0;
> > > > +    size_t n_dsts = 0;
> > >
> > > I think it is better to validate the result field to make sure that it
> > > is a writable field with at least 16-bits in it ?
> > >
> > > Something like -
> > >
> > > char *error = expr_type_check(dst, 16, true);
> > > if (error) {
> > > lexer_error(ctx->lexer, "%s", error);
> > > free(error);
> > > return;
> > > }
> > >
> > > I think it's  better  to expect the result register to be at least
> > > 16-bit writable field.
> > >
> > > I added the below in ovn.at in the action parsing test case and I see
> > > the below output.
> > > If the result field is not a register, is it expected for the
> > > parse_select_action()  to fail ?
> > > I think it should.
> > >
> > > ****
> > > diff --git a/tests/ovn.at b/tests/ovn.at
> > > index 25ce34d34..241dd26cd 100644
> > > --- a/tests/ovn.at
> > > +++ b/tests/ovn.at
> > > @@ -1504,6 +1504,9 @@ reg0 = select(1=123456, 2);
> > >  reg0 = select(123);
> > >      Syntax error at `;' expecting at least 2 group members.
> > >
> > > +ip4.dst = select(1, 2);
> > > +    Syntax error at `;' expecting at least 2 group members.
> > > +
> > >  # Miscellaneous negative tests.
> > >  ;
> > >      Syntax error at `;'.
> > > ****
> > >
> > > And the output of testsuite.log is
> > >
> > > *********
> > > ip4.dst = select(1, 2);
> > > -    Syntax error at `;' expecting at least 2 group members.
> > > +    formats as ip4.dst = select(1=100, 2=100);
> > > +    encodes as group:6
> > > +    uses group: id(6),
> > >
> >
name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:100,actions=load:1->ip_dst[0..31],resubmit(,19),bucket=bucket_id=1,weight:100,actions=load:2->ip_dst[0..31],resubmit(,19))
> > > +    has prereqs eth.type == 0x800
> > > ********
> > >
> > > Although ovn-northd will use the proper result field, it is still
> > > better to test with invalid result fields.
> > >
> > > Thanks
> > > Numan
> >
> > Thanks Numan for the great suggestion.
> > I updated the patch as suggested by slightly different, as the result
field
> > can have 16 *or more* bits. Please check the diff below and confirm if
it
> > looks ok:
>
> Looks good to me.
>
> Acked-by: Numan Siddique <numans@ovn.org>
>
> Thanks
> Numan
>

Thanks Numan. I applied this to master.

> >
> > - - - - - - -8>< - - - - - - - - - - - - - - - - - - - - - - - - - - -
- -
> > - - - - - - - - - ><8 - - - - - - - -
> > diff --git a/lib/actions.c b/lib/actions.c
> > index b4743bd..cd3f586 100644
> > --- a/lib/actions.c
> > +++ b/lib/actions.c
> > @@ -1103,6 +1103,23 @@ ovnact_ct_lb_free(struct ovnact_ct_lb *ct_lb)
> >  static void
> >  parse_select_action(struct action_context *ctx, struct expr_field
> > *res_field)
> >  {
> > +    /* Check if the result field is modifiable. */
> > +    char *error = expr_type_check(res_field, res_field->n_bits, true);
> > +    if (error) {
> > +        lexer_error(ctx->lexer, "%s", error);
> > +        free(error);
> > +        return;
> > +    }
> > +
> > +    if (res_field->n_bits < 16) {
> > +        lexer_error(ctx->lexer, "cannot use %d-bit field %s[%d..%d] "
> > +                    "for \"select\", which requires at least 16 bits.",
> > +                    res_field->n_bits, res_field->symbol->name,
> > +                    res_field->ofs,
> > +                    res_field->ofs + res_field->n_bits - 1);
> > +        return;
> > +    }
> > +
> >      if (ctx->pp->cur_ltable >= ctx->pp->n_tables) {
> >          lexer_error(ctx->lexer,
> >                      "\"select\" action not allowed in last table.");
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 25ce34d..f245083 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -1503,6 +1503,10 @@ reg0 = select(1=123456, 2);
> >      Syntax error at `123456' expecting weight.
> >  reg0 = select(123);
> >      Syntax error at `;' expecting at least 2 group members.
> > +ip.proto = select(1, 2, 3);
> > +    Field ip.proto is not modifiable.
> > +reg0[0..14] = select(1, 2, 3);
> > +    cannot use 15-bit field reg0[0..14] for "select", which requires at
> > least 16 bits.
> >
> >  # Miscellaneous negative tests.
> >  ;
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
diff mbox series

Patch

diff --git a/include/ovn/actions.h b/include/ovn/actions.h
index 047a8d7..2d4b05b 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -61,6 +61,7 @@  struct ovn_extend_table;
     OVNACT(CT_DNAT,           ovnact_ct_nat)          \
     OVNACT(CT_SNAT,           ovnact_ct_nat)          \
     OVNACT(CT_LB,             ovnact_ct_lb)           \
+    OVNACT(SELECT,            ovnact_select)          \
     OVNACT(CT_CLEAR,          ovnact_null)            \
     OVNACT(CLONE,             ovnact_nest)            \
     OVNACT(ARP,               ovnact_nest)            \
@@ -251,6 +252,20 @@  struct ovnact_ct_lb {
     uint8_t ltable;             /* Logical table ID of next table. */
 };
 
+struct ovnact_select_dst {
+    uint16_t id;
+    uint16_t weight;
+};
+
+/* OVNACT_SELECT. */
+struct ovnact_select {
+    struct ovnact ovnact;
+    struct ovnact_select_dst *dsts;
+    size_t n_dsts;
+    uint8_t ltable;             /* Logical table ID of next table. */
+    struct expr_field res_field;
+};
+
 /* OVNACT_ARP, OVNACT_ND_NA, OVNACT_CLONE. */
 struct ovnact_nest {
     struct ovnact ovnact;
diff --git a/lib/actions.c b/lib/actions.c
index 051e6c8..b4743bd 100644
--- a/lib/actions.c
+++ b/lib/actions.c
@@ -218,17 +218,18 @@  action_parse_field(struct action_context *ctx,
 }
 
 static bool
-action_parse_port(struct action_context *ctx, uint16_t *port)
+action_parse_uint16(struct action_context *ctx, uint16_t *_value,
+                    const char *msg)
 {
     if (lexer_is_int(ctx->lexer)) {
         int value = ntohll(ctx->lexer->token.value.integer);
         if (value <= UINT16_MAX) {
-            *port = value;
+            *_value = value;
             lexer_get(ctx->lexer);
             return true;
         }
     }
-    lexer_syntax_error(ctx->lexer, "expecting port number");
+    lexer_syntax_error(ctx->lexer, "expecting %s", msg);
     return false;
 }
 
@@ -927,7 +928,7 @@  parse_ct_lb_action(struct action_context *ctx)
                 }
                 dst.port = 0;
                 if (lexer_match(ctx->lexer, LEX_T_COLON)
-                    && !action_parse_port(ctx, &dst.port)) {
+                    && !action_parse_uint16(ctx, &dst.port, "port number")) {
                     free(dsts);
                     return;
                 }
@@ -957,7 +958,8 @@  parse_ct_lb_action(struct action_context *ctx)
                         lexer_syntax_error(ctx->lexer, "IPv6 address needs "
                                 "square brackets if port is included");
                         return;
-                    } else if (!action_parse_port(ctx, &dst.port)) {
+                    } else if (!action_parse_uint16(ctx, &dst.port,
+                                                    "port number")) {
                         free(dsts);
                         return;
                     }
@@ -1099,6 +1101,121 @@  ovnact_ct_lb_free(struct ovnact_ct_lb *ct_lb)
 }
 
 static void
+parse_select_action(struct action_context *ctx, struct expr_field *res_field)
+{
+    if (ctx->pp->cur_ltable >= ctx->pp->n_tables) {
+        lexer_error(ctx->lexer,
+                    "\"select\" action not allowed in last table.");
+        return;
+    }
+
+    struct ovnact_select_dst *dsts = NULL;
+    size_t allocated_dsts = 0;
+    size_t n_dsts = 0;
+
+    lexer_get(ctx->lexer); /* Skip "select". */
+    lexer_get(ctx->lexer); /* Skip '('. */
+
+    while (!lexer_match(ctx->lexer, LEX_T_RPAREN)) {
+        struct ovnact_select_dst dst;
+        if (!action_parse_uint16(ctx, &dst.id, "id")) {
+            free(dsts);
+            return;
+        }
+
+        dst.weight = 0;
+        if (lexer_match(ctx->lexer, LEX_T_EQUALS)) {
+            if (!action_parse_uint16(ctx, &dst.weight, "weight")) {
+                free(dsts);
+                return;
+            }
+            if (dst.weight == 0) {
+                lexer_syntax_error(ctx->lexer, "weight can't be 0");
+            }
+        }
+        lexer_match(ctx->lexer, LEX_T_COMMA);
+
+        /* Append to dsts. */
+        if (n_dsts >= allocated_dsts) {
+            dsts = x2nrealloc(dsts, &allocated_dsts, sizeof *dsts);
+        }
+        dsts[n_dsts++] = dst;
+    }
+    if (n_dsts <= 1) {
+        lexer_syntax_error(ctx->lexer, "expecting at least 2 group members");
+        return;
+    }
+
+    struct ovnact_select *select = ovnact_put_SELECT(ctx->ovnacts);
+    select->ltable = ctx->pp->cur_ltable + 1;
+    select->dsts = dsts;
+    select->n_dsts = n_dsts;
+    select->res_field = *res_field;
+}
+
+static void
+format_SELECT(const struct ovnact_select *select, struct ds *s)
+{
+    expr_field_format(&select->res_field, s);
+    ds_put_cstr(s, " = ");
+    ds_put_cstr(s, "select");
+    ds_put_char(s, '(');
+    for (size_t i = 0; i < select->n_dsts; i++) {
+        if (i) {
+            ds_put_cstr(s, ", ");
+        }
+
+        const struct ovnact_select_dst *dst = &select->dsts[i];
+        ds_put_format(s, "%"PRIu16, dst->id);
+        ds_put_format(s, "=%"PRIu16, dst->weight ? dst->weight : 100);
+    }
+    ds_put_char(s, ')');
+    ds_put_char(s, ';');
+}
+
+static void
+encode_SELECT(const struct ovnact_select *select,
+             const struct ovnact_encode_params *ep,
+             struct ofpbuf *ofpacts)
+{
+    ovs_assert(select->n_dsts >= 1);
+    uint8_t resubmit_table = select->ltable + first_ptable(ep, ep->pipeline);
+    uint32_t table_id = 0;
+    struct ofpact_group *og;
+
+    struct ds ds = DS_EMPTY_INITIALIZER;
+    ds_put_format(&ds, "type=select,selection_method=dp_hash");
+
+    struct mf_subfield sf = expr_resolve_field(&select->res_field);
+
+    for (size_t bucket_id = 0; bucket_id < select->n_dsts; bucket_id++) {
+        const struct ovnact_select_dst *dst = &select->dsts[bucket_id];
+        ds_put_format(&ds, ",bucket=bucket_id=%"PRIuSIZE",weight:%"PRIu16
+                      ",actions=", bucket_id, dst->weight ? dst->weight : 100);
+        ds_put_format(&ds, "load:%u->%s[%u..%u],", dst->id, sf.field->name,
+                      sf.ofs, sf.ofs + sf.n_bits - 1);
+        ds_put_format(&ds, "resubmit(,%d)", resubmit_table);
+    }
+
+    table_id = ovn_extend_table_assign_id(ep->group_table, ds_cstr(&ds),
+                                          ep->lflow_uuid);
+    ds_destroy(&ds);
+    if (table_id == EXT_TABLE_ID_INVALID) {
+        return;
+    }
+
+    /* Create an action to set the group. */
+    og = ofpact_put_GROUP(ofpacts);
+    og->group_id = table_id;
+}
+
+static void
+ovnact_select_free(struct ovnact_select *select)
+{
+    free(select->dsts);
+}
+
+static void
 format_CT_CLEAR(const struct ovnact_null *null OVS_UNUSED, struct ds *s)
 {
     ds_put_cstr(s, "ct_clear;");
@@ -2868,6 +2985,9 @@  parse_set_action(struct action_context *ctx)
     } else if (lexer_match(ctx->lexer, LEX_T_EQUALS)) {
         if (ctx->lexer->token.type != LEX_T_ID) {
             parse_LOAD(ctx, &lhs);
+        } else if (!strcmp(ctx->lexer->token.s, "select")
+                   && lexer_lookahead(ctx->lexer) == LEX_T_LPAREN) {
+            parse_select_action(ctx, &lhs);
         } else if (!strcmp(ctx->lexer->token.s, "put_dhcp_opts")
                    && lexer_lookahead(ctx->lexer) == LEX_T_LPAREN) {
             parse_put_dhcp_opts(ctx, &lhs, ovnact_put_PUT_DHCPV4_OPTS(
diff --git a/ovn-sb.xml b/ovn-sb.xml
index 82167c4..9635dcc 100644
--- a/ovn-sb.xml
+++ b/ovn-sb.xml
@@ -2114,6 +2114,40 @@  tcp.flags = RST;
 
           <p><b>Example:</b> <code>handle_svc_check(inport);</code></p>
         </dd>
+
+        <dt><code><var>R</var> = select(<var>N1</var>[=<var>W1</var>], <var>N2</var>[=<var>W2</var>], ...);</code></dt>
+        <dd>
+          <p>
+            <b>Parameters</b>: Integer <var>N1</var>, <var>N2</var>..., with
+            optional weight <var>W1</var>, <var>W2</var>, ...
+          </p>
+
+          <p>
+            <b>Result</b>: stored to a logical field or subfield <var>R</var>.
+          </p>
+
+          <p>
+            Select from a list of integers <var>N1</var>, <var>N2</var>...,
+            each within the range 0 ~ 65535, and store the selected one in the
+            field <var>R</var>.  There must be 2 or more integers listed, each
+            with an optional weight, which is an integer within the range 1 ~
+            65535.  If weight is not specified, it defaults to 100. The
+            selection method is based on the 5-tuple hash of packet header.
+          </p>
+
+          <p>
+            Processing automatically moves on to the next table, as if
+            <code>next;</code> were specified.  The <code>select</code> action
+            must be put as the last action of the logical flow when there are
+            multiple actions (actions put after <code>select</code> will not
+            take effect).
+          </p>
+
+          <p>
+            <b>Example:</b> <code>reg8[16..31] = select(1=20, 2=30, 3=50);</code>
+          </p>
+        </dd>
+
       </dl>
     </column>
 
diff --git a/tests/ovn.at b/tests/ovn.at
index 411b768..de80bce 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -965,14 +965,17 @@  ct_lb();
     has prereqs ip
 ct_lb(192.168.1.2:80, 192.168.1.3:80);
     encodes as group:1
+    uses group: id(1), name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:100,actions=ct(nat(dst=192.168.1.2:80),commit,table=19,zone=NXM_NX_REG13[0..15]),bucket=bucket_id=1,weight:100,actions=ct(nat(dst=192.168.1.3:80),commit,table=19,zone=NXM_NX_REG13[0..15]))
     has prereqs ip
 ct_lb(192.168.1.2, 192.168.1.3, );
     formats as ct_lb(192.168.1.2, 192.168.1.3);
     encodes as group:2
+    uses group: id(2), name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:100,actions=ct(nat(dst=192.168.1.2),commit,table=19,zone=NXM_NX_REG13[0..15]),bucket=bucket_id=1,weight:100,actions=ct(nat(dst=192.168.1.3),commit,table=19,zone=NXM_NX_REG13[0..15]))
     has prereqs ip
 ct_lb(fd0f::2, fd0f::3, );
     formats as ct_lb(fd0f::2, fd0f::3);
     encodes as group:3
+    uses group: id(3), name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:100,actions=ct(nat(dst=fd0f::2),commit,table=19,zone=NXM_NX_REG13[0..15]),bucket=bucket_id=1,weight:100,actions=ct(nat(dst=fd0f::3),commit,table=19,zone=NXM_NX_REG13[0..15]))
     has prereqs ip
 
 ct_lb(192.168.1.2:);
@@ -1481,6 +1484,26 @@  handle_svc_check();
 handle_svc_check(reg0);
     Cannot use numeric field reg0 where string field is required.
 
+# select
+reg9[16..31] = select(1=50, 2=100, 3, );
+    formats as reg9[16..31] = select(1=50, 2=100, 3=100);
+    encodes as group:4
+    uses group: id(4), name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:50,actions=load:1->xreg4[16..31],resubmit(,19),bucket=bucket_id=1,weight:100,actions=load:2->xreg4[16..31],resubmit(,19),bucket=bucket_id=2,weight:100,actions=load:3->xreg4[16..31],resubmit(,19))
+
+reg0 = select(1, 2);
+    formats as reg0 = select(1=100, 2=100);
+    encodes as group:5
+    uses group: id(5), name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:100,actions=load:1->xxreg0[96..127],resubmit(,19),bucket=bucket_id=1,weight:100,actions=load:2->xxreg0[96..127],resubmit(,19))
+
+reg0 = select(1=, 2);
+    Syntax error at `,' expecting weight.
+reg0 = select(1=0, 2);
+    Syntax error at `,' weight can't be 0.
+reg0 = select(1=123456, 2);
+    Syntax error at `123456' expecting weight.
+reg0 = select(123);
+    Syntax error at `;' expecting at least 2 group members.
+
 # Miscellaneous negative tests.
 ;
     Syntax error at `;'.
diff --git a/tests/test-ovn.c b/tests/test-ovn.c
index 5366905..476bc33 100644
--- a/tests/test-ovn.c
+++ b/tests/test-ovn.c
@@ -1227,6 +1227,28 @@  test_expr_to_packets(struct ovs_cmdl_context *ctx OVS_UNUSED)
 /* Actions. */
 
 static void
+print_group_info(struct ovn_extend_table *group_table, const char *action)
+{
+    struct ovn_extend_table_info *g;
+    HMAP_FOR_EACH (g, hmap_node, &group_table->desired) {
+        char buf[64];
+        sprintf(buf, "group:%"PRIu32, g->table_id);
+        char *match = strstr(buf, action);
+        if (match) {
+            if (match[strlen(buf)] != '\0') {
+                /* Add ',' and match again. */
+                sprintf(buf, "group:%"PRIu32",", g->table_id);
+                match = strstr(buf, action);
+            }
+        }
+        if (match) {
+            printf("    uses group: id(%"PRIu32"), name(%s)\n",
+                   g->table_id, g->name);
+        }
+    }
+}
+
+static void
 test_parse_actions(struct ovs_cmdl_context *ctx OVS_UNUSED)
 {
     struct shash symtab;
@@ -1305,7 +1327,9 @@  test_parse_actions(struct ovs_cmdl_context *ctx OVS_UNUSED)
             struct ds ofpacts_s = DS_EMPTY_INITIALIZER;
             struct ofpact_format_params fp = { .s = &ofpacts_s };
             ofpacts_format(ofpacts.data, ofpacts.size, &fp);
-            printf("    encodes as %s\n", ds_cstr(&ofpacts_s));
+            char *ofpacts_cstr = ds_cstr(&ofpacts_s);
+            printf("    encodes as %s\n", ofpacts_cstr);
+            print_group_info(&group_table, ofpacts_cstr);
             ds_destroy(&ofpacts_s);
             ofpbuf_uninit(&ofpacts);
 
diff --git a/utilities/ovn-trace.8.xml b/utilities/ovn-trace.8.xml
index 01e7411..485928c 100644
--- a/utilities/ovn-trace.8.xml
+++ b/utilities/ovn-trace.8.xml
@@ -438,6 +438,15 @@ 
       <code>--lb-dst</code> is not available in daemon mode.
     </dd>
 
+    <dt><code>--select-id=</code><var>id</var></dt>
+    <dd>
+      Specify the <var>id</var> to be selected by the <code>select</code>
+      action. <var>id</var> must be one of the values listed in the
+      <code>select</code> action.  Otherwise, a random id is selected from
+      the list, as if <code>--select-id</code> were not specified.
+      <code>--select-id</code> is not available in daemon mode.
+    </dd>
+
     <dt><code>--friendly-names</code></dt>
     <dt><code>--no-friendly-names</code></dt>
     <dd>
diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
index 2645438..a97195b 100644
--- a/utilities/ovn-trace.c
+++ b/utilities/ovn-trace.c
@@ -81,6 +81,9 @@  static size_t ct_state_idx;
 /* --lb-dst: load balancer destination info. */
 static struct ovnact_ct_lb_dst lb_dst;
 
+/* --select-id: "select" action member id. */
+static uint16_t select_id;
+
 /* --friendly-names, --no-friendly-names: Whether to substitute human-friendly
  * port and datapath names for the awkward UUIDs typically used in the actual
  * logical flows. */
@@ -209,6 +212,16 @@  parse_lb_option(const char *s)
 }
 
 static void
+parse_select_option(const char *s)
+{
+    unsigned long int integer = strtoul(s, NULL, 10);
+    if (!integer) {
+        ovs_fatal(0, "%s: bad select-id", s);
+    }
+    select_id = integer;
+}
+
+static void
 parse_options(int argc, char *argv[])
 {
     enum {
@@ -225,7 +238,8 @@  parse_options(int argc, char *argv[])
         DAEMON_OPTION_ENUMS,
         SSL_OPTION_ENUMS,
         VLOG_OPTION_ENUMS,
-        OPT_LB_DST
+        OPT_LB_DST,
+        OPT_SELECT_ID
     };
     static const struct option long_options[] = {
         {"db", required_argument, NULL, OPT_DB},
@@ -241,6 +255,7 @@  parse_options(int argc, char *argv[])
         {"help", no_argument, NULL, 'h'},
         {"version", no_argument, NULL, 'V'},
         {"lb-dst", required_argument, NULL, OPT_LB_DST},
+        {"select-id", required_argument, NULL, OPT_SELECT_ID},
         DAEMON_LONG_OPTIONS,
         VLOG_LONG_OPTIONS,
         STREAM_SSL_LONG_OPTIONS,
@@ -302,6 +317,10 @@  parse_options(int argc, char *argv[])
             parse_lb_option(optarg);
             break;
 
+        case OPT_SELECT_ID:
+            parse_select_option(optarg);
+            break;
+
         case 'h':
             usage();
 
@@ -1991,6 +2010,46 @@  execute_ct_lb(const struct ovnact_ct_lb *ct_lb,
 }
 
 static void
+execute_select(const struct ovnact_select *select,
+              const struct ovntrace_datapath *dp, struct flow *uflow,
+              enum ovnact_pipeline pipeline, struct ovs_list *super)
+{
+    struct flow select_flow = *uflow;
+
+    const struct ovnact_select_dst *dst = NULL;
+    ovs_assert(select->n_dsts > 1);
+    bool user_specified = false;
+    for (int i = 0; i < select->n_dsts; i++) {
+        const struct ovnact_select_dst *d = &select->dsts[i];
+
+        /* Check for the dst specified by --select-id, if any. */
+        if (select_id == d->id) {
+            dst = d;
+            user_specified = true;
+            break;
+        }
+    }
+
+    if (!dst) {
+        /* Select a random dst as a fallback. */
+        dst = &select->dsts[random_range(select->n_dsts)];
+    }
+
+    struct mf_subfield sf = expr_resolve_field(&select->res_field);
+    union mf_subvalue sv = { .be16_int = htons(dst->id) };
+    mf_write_subfield_flow(&sf, &sv, &select_flow);
+    struct ds sf_str = DS_EMPTY_INITIALIZER;
+    expr_field_format(&select->res_field, &sf_str);
+    struct ovntrace_node *node = ovntrace_node_append(
+        super, OVNTRACE_NODE_TRANSFORMATION,
+        "select: %s = %"PRIu16"%s",
+        ds_cstr(&sf_str), dst->id, user_specified ?  "" :
+        " /* Randomly selected. Use --select-id to specify. */");
+    ds_destroy(&sf_str);
+    trace__(dp, &select_flow, select->ltable, pipeline, &node->subs);
+}
+
+static void
 execute_log(const struct ovnact_log *log, struct flow *uflow,
             struct ovs_list *super)
 {
@@ -2100,6 +2159,11 @@  trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len,
             execute_ct_lb(ovnact_get_CT_LB(a), dp, uflow, pipeline, super);
             break;
 
+        case OVNACT_SELECT:
+            execute_select(ovnact_get_SELECT(a), dp, uflow,
+                                   pipeline, super);
+            break;
+
         case OVNACT_CT_CLEAR:
             flow_clear_conntrack(uflow);
             break;