On Tue, Jun 28, 2016 at 3:50 AM, <bscha...@redhat.com> wrote:
> From: Russel Bryant <russ...@ovn.org>
>
> 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>
> Co-authored-by: Babu Shanmugam <bscha...@redhat.com>
> Signed-off-by: Babu Shanmugam <bscha...@redhat.com>
Tested-by: Kyle Mestery <mest...@mestery.com>
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
> ---
> 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
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
