On Tue, Jan 21, 2020 at 4:27 AM Han Zhou <hz...@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 <hz...@ovn.org>
Hi Han,
Thanks for v2.
I have one comment. Please see below.
With that addressed - Acked-by: Numan Siddique <num...@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
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
