On Thursday, June 30, 2016, Kyle Mestery <mest...@mestery.com> wrote:
> On Tue, Jun 28, 2016 at 3:50 AM, <bscha...@redhat.com <javascript:;>>
> wrote:
> > From: Russel Bryant <russ...@ovn.org <javascript:;>>
> >
> > Update the OVN expression parser to support address sets. Previously,
> > you could have a set of IP or MAC addresses in this form:
> >
> > {addr1, addr2, ..., addrN}
> >
> > This patch adds support for a bit of indirection where we can define a
> > set of addresses and refer to them by name.
> >
> > $name
> >
> > This '$name' can be used in the expresssions like
> >
> > {addr1, addr2, $name, ... }
> > {$name}
> > $name
> >
> > A future patch will expose the ability to define address sets for use.
> >
> > Signed-off-by: Russell Bryant <russ...@ovn.org <javascript:;>>
> > Co-authored-by: Babu Shanmugam <bscha...@redhat.com <javascript:;>>
> > Signed-off-by: Babu Shanmugam <bscha...@redhat.com <javascript:;>>
>
> Tested-by: Kyle Mestery <mest...@mestery.com <javascript:;>>
>
> I've run these through the ovn-scale-test CI setup [1]. We have a test
> which creates ports and puts ACLs on them. This is a pure OVN control
> plane test with Rally driving creating and managing the resources
> under test. The test uses the following parameters:
>
> * 5 networks
> * 200 ports per network
> * 5 ACLs per port
>
> Before the address sets patches, these tests took an average of 820
> seconds to run. With the address sets patches, they take an average of
> 460 seconds. That's quite the improvement! Thanks for the work here
> Russell and Babu!
>
> [1] https://github.com/openvswitch/ovn-scale-test/tree/master/ci
Wow, that's a nice speed up. What do the ACLs look like that get created?
What did they look like after changing them to use address sets?
Russell
>
> > ---
> > ovn/controller/lflow.c | 2 +-
> > ovn/lib/actions.c | 2 +-
> > ovn/lib/expr.c | 126
> ++++++++++++++++++++++++++++++++++++++++++++++---
> > ovn/lib/expr.h | 17 +++++++
> > ovn/lib/lex.c | 16 +++++++
> > ovn/lib/lex.h | 1 +
> > tests/ovn.at | 50 ++++++++++++++++++++
> > tests/test-ovn.c | 31 ++++++++++--
> > 8 files changed, 234 insertions(+), 11 deletions(-)
> >
> > diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
> > index 52e6131..433df70 100644
> > --- a/ovn/controller/lflow.c
> > +++ b/ovn/controller/lflow.c
> > @@ -306,7 +306,7 @@ add_logical_flows(struct controller_ctx *ctx, const
> struct lport_index *lports,
> > struct hmap matches;
> > struct expr *expr;
> >
> > - expr = expr_parse_string(lflow->match, &symtab, &error);
> > + expr = expr_parse_string(lflow->match, &symtab, NULL, &error);
> > if (!error) {
> > if (prereqs) {
> > expr = expr_combine(EXPR_T_AND, expr, prereqs);
> > diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
> > index 569970e..ebd7159 100644
> > --- a/ovn/lib/actions.c
> > +++ b/ovn/lib/actions.c
> > @@ -208,7 +208,7 @@ add_prerequisite(struct action_context *ctx, const
> char *prerequisite)
> > struct expr *expr;
> > char *error;
> >
> > - expr = expr_parse_string(prerequisite, ctx->ap->symtab, &error);
> > + expr = expr_parse_string(prerequisite, ctx->ap->symtab, NULL,
> &error);
> > ovs_assert(!error);
> > ctx->prereqs = expr_combine(EXPR_T_AND, ctx->prereqs, expr);
> > }
> > diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c
> > index 91bff07..2883ca9 100644
> > --- a/ovn/lib/expr.c
> > +++ b/ovn/lib/expr.c
> > @@ -409,6 +409,7 @@ expr_print(const struct expr *e)
> > struct expr_context {
> > struct lexer *lexer; /* Lexer for pulling more tokens. */
> > const struct shash *symtab; /* Symbol table. */
> > + const struct shash *macros; /* Table of macros. */
> > char *error; /* Error, if any, otherwise NULL. */
> > bool not; /* True inside odd number of NOT
> operators. */
> > };
> > @@ -729,6 +730,44 @@ assign_constant_set_type(struct expr_context *ctx,
> > }
> >
> > static bool
> > +parse_macros(struct expr_context *ctx, struct expr_constant_set *cs,
> > + size_t *allocated_values)
> > +{
> > + if (!ctx->macros) {
> > + expr_syntax_error(ctx, "No macros defined.");
> > + return false;
> > + }
> > +
> > + struct expr_constant_set *addr_set
> > + = shash_find_data(ctx->macros, ctx->lexer->token.s);
> > + if (!addr_set) {
> > + expr_syntax_error(ctx, "Unknown macro: '%s'",
> ctx->lexer->token.s);
> > + return false;
> > + }
> > +
> > + if (!assign_constant_set_type(ctx, cs, EXPR_C_INTEGER)) {
> > + return false;
> > + }
> > +
> > + size_t n_values = cs->n_values + addr_set->n_values;
> > + if (n_values >= *allocated_values) {
> > + cs->values = xrealloc(cs->values, n_values * sizeof
> *cs->values);
> > + *allocated_values = n_values;
> > + }
> > + size_t i;
> > + for (i = 0; i < addr_set->n_values; i++) {
> > + union expr_constant *c1 = &cs->values[cs->n_values++];
> > + union expr_constant *c2 = &addr_set->values[i];
> > + c1->value = c2->value;
> > + c1->format = c2->format;
> > + c1->masked = c2->masked;
> > + c1->mask = c2->mask;
> > + }
> > +
> > + return true;
> > +}
> > +
> > +static bool
> > parse_constant(struct expr_context *ctx, struct expr_constant_set *cs,
> > size_t *allocated_values)
> > {
> > @@ -759,6 +798,12 @@ parse_constant(struct expr_context *ctx, struct
> expr_constant_set *cs,
> > }
> > lexer_get(ctx->lexer);
> > return true;
> > + } else if (ctx->lexer->token.type == LEX_T_MACRO) {
> > + if (!parse_macros(ctx, cs, allocated_values)) {
> > + return false;
> > + }
> > + lexer_get(ctx->lexer);
> > + return true;
> > } else {
> > expr_syntax_error(ctx, "expecting constant.");
> > return false;
> > @@ -808,6 +853,72 @@ expr_constant_set_destroy(struct expr_constant_set
> *cs)
> > }
> > }
> >
> > +void
> > +expr_macros_add(struct shash *macros, const char *name,
> > + const char * const *values, size_t n_values)
> > +{
> > + /* Replace any existing entry for this name. */
> > + expr_macros_remove(macros, name);
> > +
> > + struct expr_constant_set *cset = xzalloc(sizeof *cset);
> > + cset->type = EXPR_C_INTEGER;
> > + cset->in_curlies = true;
> > + cset->n_values = n_values;
> > + cset->values = xmalloc(cset->n_values * sizeof *cset->values);
> > + size_t i, errors = 0;
> > + for (i = 0; i < n_values; i++) {
> > + /* Use the lexer to convert each macro into the proper
> > + * integer format. */
> > + struct lexer lex;
> > + lexer_init(&lex, values[i]);
> > + lexer_get(&lex);
> > + if (lex.token.type != LEX_T_INTEGER
> > + && lex.token.type != LEX_T_MASKED_INTEGER) {
> > + VLOG_WARN("Invalid address set entry: '%s', token type: %d",
> > + values[i], lex.token.type);
> > + errors += 1;
> > + } else {
> > + union expr_constant *c = &cset->values[i - errors];
> > + c->value = lex.token.value;
> > + c->format = lex.token.format;
> > + c->masked = lex.token.type == LEX_T_MASKED_INTEGER;
> > + if (c->masked) {
> > + c->mask = lex.token.mask;
> > + }
> > + }
> > + lexer_destroy(&lex);
> > + }
> > + cset->n_values -= errors;
> > +
> > + shash_add(macros, name, cset);
> > +}
> > +
> > +void
> > +expr_macros_remove(struct shash *macros, const char *name)
> > +{
> > + struct expr_constant_set *cset
> > + = shash_find_and_delete(macros, name);
> > +
> > + if (cset) {
> > + expr_constant_set_destroy(cset);
> > + free(cset);
> > + }
> > +}
> > +
> > +/* Destroy all contents of macros. */
> > +void
> > +expr_macros_destroy(struct shash *macros)
> > +{
> > + struct shash_node *node, *next;
> > +
> > + SHASH_FOR_EACH_SAFE (node, next, macros) {
> > + struct expr_constant_set *cset = node->data;
> > +
> > + shash_delete(macros, node);
> > + expr_constant_set_destroy(cset);
> > + }
> > +}
> > +
> > static struct expr *
> > expr_parse_primary(struct expr_context *ctx, bool *atomic)
> > {
> > @@ -980,9 +1091,11 @@ expr_parse__(struct expr_context *ctx)
> > * The caller must eventually free the returned expression (with
> > * expr_destroy()) or error (with free()). */
> > struct expr *
> > -expr_parse(struct lexer *lexer, const struct shash *symtab, char
> **errorp)
> > +expr_parse(struct lexer *lexer, const struct shash *symtab,
> > + const struct shash *macros, char **errorp)
> > {
> > - struct expr_context ctx = { .lexer = lexer, .symtab = symtab };
> > + struct expr_context ctx = { .lexer = lexer, .symtab = symtab,
> > + .macros = macros };
> > struct expr *e = expr_parse__(&ctx);
> > *errorp = ctx.error;
> > ovs_assert((ctx.error != NULL) != (e != NULL));
> > @@ -991,14 +1104,15 @@ expr_parse(struct lexer *lexer, const struct
> shash *symtab, char **errorp)
> >
> > /* Like expr_parse(), but the expression is taken from 's'. */
> > struct expr *
> > -expr_parse_string(const char *s, const struct shash *symtab, char
> **errorp)
> > +expr_parse_string(const char *s, const struct shash *symtab,
> > + const struct shash *macros, char **errorp)
> > {
> > struct lexer lexer;
> > struct expr *expr;
> >
> > lexer_init(&lexer, s);
> > lexer_get(&lexer);
> > - expr = expr_parse(&lexer, symtab, errorp);
> > + expr = expr_parse(&lexer, symtab, macros, errorp);
> > if (!*errorp && lexer.token.type != LEX_T_END) {
> > *errorp = xstrdup("Extra tokens at end of input.");
> > expr_destroy(expr);
> > @@ -1149,7 +1263,7 @@ expr_get_level(const struct expr *expr)
> > static enum expr_level
> > expr_parse_level(const char *s, const struct shash *symtab, char
> **errorp)
> > {
> > - struct expr *expr = expr_parse_string(s, symtab, errorp);
> > + struct expr *expr = expr_parse_string(s, symtab, NULL, errorp);
> > enum expr_level level = expr ? expr_get_level(expr) :
> EXPR_L_NOMINAL;
> > expr_destroy(expr);
> > return level;
> > @@ -1292,7 +1406,7 @@ parse_and_annotate(const char *s, const struct
> shash *symtab,
> > char *error;
> > struct expr *expr;
> >
> > - expr = expr_parse_string(s, symtab, &error);
> > + expr = expr_parse_string(s, symtab, NULL, &error);
> > if (expr) {
> > expr = expr_annotate__(expr, symtab, nesting, &error);
> > }
> > diff --git a/ovn/lib/expr.h b/ovn/lib/expr.h
> > index d6f8489..56f6bbb 100644
> > --- a/ovn/lib/expr.h
> > +++ b/ovn/lib/expr.h
> > @@ -352,8 +352,10 @@ expr_from_node(const struct ovs_list *node)
> > void expr_format(const struct expr *, struct ds *);
> > void expr_print(const struct expr *);
> > struct expr *expr_parse(struct lexer *, const struct shash *symtab,
> > + const struct shash *macros,
> > char **errorp);
> > struct expr *expr_parse_string(const char *, const struct shash *symtab,
> > + const struct shash *macros,
> > char **errorp);
> >
> > struct expr *expr_clone(struct expr *);
> > @@ -453,4 +455,19 @@ char *expr_parse_constant_set(struct lexer *, const
> struct shash *symtab,
> > OVS_WARN_UNUSED_RESULT;
> > void expr_constant_set_destroy(struct expr_constant_set *cs);
> >
> > +
> > +/* MACRO Variables
> > + *
> > + * Instead of referring to a set of value as:
> > + * {addr1, addr2, ..., addrN}
> > + * You can register a set of values and refer to them as:
> > + * $name
> > + * The macros should all have integer/masked-integer values.
> > + * The values that don't qualify are ingnored
> > + */
> > +void expr_macros_add(struct shash *macros, const char *name,
> > + const char * const *values, size_t n_values);
> > +void expr_macros_remove(struct shash *macros, const char *name);
> > +void expr_macros_destroy(struct shash *macros);
> > +
> > #endif /* ovn/expr.h */
> > diff --git a/ovn/lib/lex.c b/ovn/lib/lex.c
> > index 556288f..bbbc7fc 100644
> > --- a/ovn/lib/lex.c
> > +++ b/ovn/lib/lex.c
> > @@ -227,6 +227,10 @@ lex_token_format(const struct lex_token *token,
> struct ds *s)
> > lex_token_format_masked_integer(token, s);
> > break;
> >
> > + case LEX_T_MACRO:
> > + ds_put_cstr(s, token->s);
> > + break;
> > +
> > case LEX_T_LPAREN:
> > ds_put_cstr(s, "(");
> > break;
> > @@ -527,6 +531,14 @@ lex_parse_id(const char *p, struct lex_token *token)
> > return p;
> > }
> >
> > +static const char *
> > +lex_parse_macro(const char *p, struct lex_token *token)
> > +{
> > + p = lex_parse_id(++p, token);
> > + token->type = LEX_T_MACRO;
> > + return p;
> > +}
> > +
> > /* Initializes 'token' and parses the first token from the beginning of
> > * null-terminated string 'p' into 'token'. Stores a pointer to the
> start of
> > * the token (after skipping white space and comments, if any) into
> '*startp'.
> > @@ -697,6 +709,10 @@ next:
> > }
> > break;
> >
> > + case '$':
> > + p = lex_parse_macro(p, token);
> > + break;
> > +
> > case '0': case '1': case '2': case '3': case '4':
> > case '5': case '6': case '7': case '8': case '9':
> > case ':':
> > diff --git a/ovn/lib/lex.h b/ovn/lib/lex.h
> > index 578ef40..826465c 100644
> > --- a/ovn/lib/lex.h
> > +++ b/ovn/lib/lex.h
> > @@ -36,6 +36,7 @@ enum lex_type {
> > LEX_T_STRING, /* "foo" */
> > LEX_T_INTEGER, /* 12345 or 1.2.3.4 or ::1 or
> 01:02:03:04:05 */
> > LEX_T_MASKED_INTEGER, /* 12345/10 or 1.2.0.0/16 or ::2/127
> or... */
> > + LEX_T_MACRO, /* $NAME */
> > LEX_T_ERROR, /* invalid input */
> >
> > /* Bare tokens. */
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 297070c..840d0ef 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -434,6 +434,56 @@ AT_CHECK([expr_to_flow 'inport == "eth0" && inport
> == "eth1"'], [0], [dnl
> > ])
> > AT_CLEANUP
> >
> > +AT_SETUP([ovn -- converting expressions to flows -- address sets])
> > +expr_to_flow () {
> > + echo "$1" | ovstest test-ovn expr-to-flows | sort
> > +}
> > +AT_CHECK([expr_to_flow 'ip4.src == {10.0.0.1, 10.0.0.2, 10.0.0.3}'],
> [0], [dnl
> > +ip,nw_src=10.0.0.1
> > +ip,nw_src=10.0.0.2
> > +ip,nw_src=10.0.0.3
> > +])
> > +AT_CHECK([expr_to_flow 'ip4.src == $set1'], [0], [dnl
> > +ip,nw_src=10.0.0.1
> > +ip,nw_src=10.0.0.2
> > +ip,nw_src=10.0.0.3
> > +])
> > +AT_CHECK([expr_to_flow 'ip4.src == {1.2.3.4, $set1}'], [0], [dnl
> > +ip,nw_src=1.2.3.4
> > +ip,nw_src=10.0.0.1
> > +ip,nw_src=10.0.0.2
> > +ip,nw_src=10.0.0.3
> > +])
> > +AT_CHECK([expr_to_flow 'ip4.src == {1.2.0.0/20, 5.5.5.0/24, $set1}'],
> [0], [dnl
> > +ip,nw_src=1.2.0.0/20
> > +ip,nw_src=10.0.0.1
> > +ip,nw_src=10.0.0.2
> > +ip,nw_src=10.0.0.3
> > +ip,nw_src=5.5.5.0/24
> > +])
> > +AT_CHECK([expr_to_flow 'ip6.src == {::1, ::2, ::3}'], [0], [dnl
> > +ipv6,ipv6_src=::1
> > +ipv6,ipv6_src=::2
> > +ipv6,ipv6_src=::3
> > +])
> > +AT_CHECK([expr_to_flow 'ip6.src == {::1, $set2, ::4}'], [0], [dnl
> > +ipv6,ipv6_src=::1
> > +ipv6,ipv6_src=::2
> > +ipv6,ipv6_src=::3
> > +ipv6,ipv6_src=::4
> > +])
> > +AT_CHECK([expr_to_flow 'eth.src == {00:00:00:00:00:01,
> 00:00:00:00:00:02, 00:00:00:00:00:03}'], [0], [dnl
> > +dl_src=00:00:00:00:00:01
> > +dl_src=00:00:00:00:00:02
> > +dl_src=00:00:00:00:00:03
> > +])
> > +AT_CHECK([expr_to_flow 'eth.src == {$set3}'], [0], [dnl
> > +dl_src=00:00:00:00:00:01
> > +dl_src=00:00:00:00:00:02
> > +dl_src=00:00:00:00:00:03
> > +])
> > +AT_CLEANUP
> > +
> > AT_SETUP([ovn -- action parsing])
> > dnl Text before => is input, text after => is expected output.
> > AT_DATA([test-cases.txt], [[
> > diff --git a/tests/test-ovn.c b/tests/test-ovn.c
> > index 18e5aca..15ebba7 100644
> > --- a/tests/test-ovn.c
> > +++ b/tests/test-ovn.c
> > @@ -268,6 +268,26 @@ create_dhcp_opts(struct hmap *dhcp_opts)
> > dhcp_opt_add(dhcp_opts, "lease_time", 51, "uint32");
> > }
> >
> > +static void
> > +create_macros(struct shash *macros)
> > +{
> > + shash_init(macros);
> > +
> > + const char * const addrs1[] = {
> > + "10.0.0.1", "10.0.0.2", "10.0.0.3",
> > + };
> > + const char * const addrs2[] = {
> > + "::1", "::2", "::3",
> > + };
> > + const char * const addrs3[] = {
> > + "00:00:00:00:00:01", "00:00:00:00:00:02", "00:00:00:00:00:03",
> > + };
> > +
> > + expr_macros_add(macros, "set1", addrs1, 3);
> > + expr_macros_add(macros, "set2", addrs2, 3);
> > + expr_macros_add(macros, "set3", addrs3, 3);
> > +}
> > +
> > static bool
> > lookup_port_cb(const void *ports_, const char *port_name, unsigned int
> *portp)
> > {
> > @@ -284,10 +304,12 @@ static void
> > test_parse_expr__(int steps)
> > {
> > struct shash symtab;
> > + struct shash macros;
> > struct simap ports;
> > struct ds input;
> >
> > create_symtab(&symtab);
> > + create_macros(¯os);
> >
> > simap_init(&ports);
> > simap_put(&ports, "eth0", 5);
> > @@ -299,7 +321,8 @@ test_parse_expr__(int steps)
> > struct expr *expr;
> > char *error;
> >
> > - expr = expr_parse_string(ds_cstr(&input), &symtab, &error);
> > + expr = expr_parse_string(ds_cstr(&input), &symtab, ¯os,
> > + &error);
> > if (!error && steps > 0) {
> > expr = expr_annotate(expr, &symtab, &error);
> > }
> > @@ -336,6 +359,8 @@ test_parse_expr__(int steps)
> > simap_destroy(&ports);
> > expr_symtab_destroy(&symtab);
> > shash_destroy(&symtab);
> > + expr_macros_destroy(¯os);
> > + shash_destroy(¯os);
> > }
> >
> > static void
> > @@ -480,7 +505,7 @@ test_evaluate_expr(struct ovs_cmdl_context *ctx)
> > struct expr *expr;
> > char *error;
> >
> > - expr = expr_parse_string(ds_cstr(&input), &symtab, &error);
> > + expr = expr_parse_string(ds_cstr(&input), &symtab, NULL,
> &error);
> > if (!error) {
> > expr = expr_annotate(expr, &symtab, &error);
> > }
> > @@ -954,7 +979,7 @@ test_tree_shape_exhaustively(struct expr *expr,
> struct shash *symtab,
> > expr_format(expr, &s);
> >
> > char *error;
> > - modified = expr_parse_string(ds_cstr(&s), symtab, &error);
> > + modified = expr_parse_string(ds_cstr(&s), symtab, NULL,
> &error);
> > if (error) {
> > fprintf(stderr, "%s fails to parse (%s)\n",
> > ds_cstr(&s), error);
> > --
> > 2.5.5
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org <javascript:;>
> > http://openvswitch.org/mailman/listinfo/dev
> _______________________________________________
> dev mailing list
> dev@openvswitch.org <javascript:;>
> http://openvswitch.org/mailman/listinfo/dev
>
--
Russell Bryant
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
