On Tue, Dec 3, 2019 at 12:05 AM Han Zhou <hz...@ovn.org> wrote: > > > > On Mon, Dec 2, 2019 at 2:38 PM Han Zhou <hz...@ovn.org> wrote: > > > > > > > > On Mon, Dec 2, 2019 at 5:20 AM Dumitru Ceara <dce...@redhat.com> wrote: > > > > > > The commit that adds incremental processing for port-group changes > > > doesn't store logical flow references for port groups. If a port group > > > is updated (e.g., a port is added) no logical flow recalculation will be > > > performed. > > > > > > To fix this, when parsing the flow expression also store the referenced > > > port groups and bind them to the logical flows that depend on them. If > > > the port group is updated then the logical flows referring them will > > > also be reinstalled. > > > > > > Reported-by: Daniel Alvarez <dalva...@redhat.com> > > > Reported-at: https://bugzilla.redhat.com/1778164 > > > CC: Han Zhou <hz...@ovn.org> > > > Fixes: 978f5e90af0a ("ovn-controller: Incremental processing for > > > port-group changes.") > > > Signed-off-by: Dumitru Ceara <dce...@redhat.com> > > > > > > --- > > > v2: remove Change-Id from commit message. > > > --- > > > controller/lflow.c | 9 ++++++++- > > > include/ovn/expr.h | 4 +++- > > > lib/actions.c | 4 ++-- > > > lib/expr.c | 24 +++++++++++++++++------- > > > tests/test-ovn.c | 8 ++++---- > > > utilities/ovn-trace.c | 2 +- > > > 6 files changed, 35 insertions(+), 16 deletions(-) > > > > > > diff --git a/controller/lflow.c b/controller/lflow.c > > > index 36150bd..a689320 100644 > > > --- a/controller/lflow.c > > > +++ b/controller/lflow.c > > > @@ -616,14 +616,21 @@ consider_logical_flow( > > > struct expr *expr; > > > > > > struct sset addr_sets_ref = SSET_INITIALIZER(&addr_sets_ref); > > > + struct sset port_groups_ref = SSET_INITIALIZER(&port_groups_ref); > > > expr = expr_parse_string(lflow->match, &symtab, addr_sets, > > > port_groups, > > > - &addr_sets_ref, &error); > > > + &addr_sets_ref, &port_groups_ref, &error); > > > const char *addr_set_name; > > > SSET_FOR_EACH (addr_set_name, &addr_sets_ref) { > > > lflow_resource_add(lfrr, REF_TYPE_ADDRSET, addr_set_name, > > > &lflow->header_.uuid); > > > } > > > + const char *port_group_name; > > > + SSET_FOR_EACH (port_group_name, &port_groups_ref) { > > > + lflow_resource_add(lfrr, REF_TYPE_PORTGROUP, port_group_name, > > > + &lflow->header_.uuid); > > > + } > > > sset_destroy(&addr_sets_ref); > > > + sset_destroy(&port_groups_ref); > > > > > > if (!error) { > > > if (prereqs) { > > > diff --git a/include/ovn/expr.h b/include/ovn/expr.h > > > index 22f633e..21bf51c 100644 > > > --- a/include/ovn/expr.h > > > +++ b/include/ovn/expr.h > > > @@ -390,11 +390,13 @@ void expr_print(const struct expr *); > > > struct expr *expr_parse(struct lexer *, const struct shash *symtab, > > > const struct shash *addr_sets, > > > const struct shash *port_groups, > > > - struct sset *addr_sets_ref); > > > + struct sset *addr_sets_ref, > > > + struct sset *port_groups_ref); > > > struct expr *expr_parse_string(const char *, const struct shash *symtab, > > > const struct shash *addr_sets, > > > const struct shash *port_groups, > > > struct sset *addr_sets_ref, > > > + struct sset *port_groups_ref, > > > char **errorp); > > > > > > struct expr *expr_clone(struct expr *); > > > diff --git a/lib/actions.c b/lib/actions.c > > > index 586d7b7..051e6c8 100644 > > > --- a/lib/actions.c > > > +++ b/lib/actions.c > > > @@ -240,8 +240,8 @@ add_prerequisite(struct action_context *ctx, const > > > char *prerequisite) > > > struct expr *expr; > > > char *error; > > > > > > - expr = expr_parse_string(prerequisite, ctx->pp->symtab, NULL, NULL, > > > NULL, > > > - &error); > > > + expr = expr_parse_string(prerequisite, ctx->pp->symtab, NULL, NULL, > > > + NULL, NULL, &error); > > > ovs_assert(!error); > > > ctx->prereqs = expr_combine(EXPR_T_AND, ctx->prereqs, expr); > > > } > > > diff --git a/lib/expr.c b/lib/expr.c > > > index 71de615..e5e4d21 100644 > > > --- a/lib/expr.c > > > +++ b/lib/expr.c > > > @@ -480,7 +480,8 @@ struct expr_context { > > > const struct shash *symtab; /* Symbol table. */ > > > const struct shash *addr_sets; /* Address set table. */ > > > const struct shash *port_groups; /* Port group table. */ > > > - struct sset *addr_sets_ref; /* The set of address set > > > referenced. */ > > > + struct sset *addr_sets_ref; /* The set of address set > > > referenced. */ > > > + struct sset *port_groups_ref; /* The set of port groups > > > referenced. */ > > > bool not; /* True inside odd number of NOT > > > operators. */ > > > unsigned int paren_depth; /* Depth of nested parentheses. */ > > > }; > > > @@ -782,6 +783,10 @@ static bool > > > parse_port_group(struct expr_context *ctx, struct expr_constant_set *cs, > > > size_t *allocated_values) > > > { > > > + if (ctx->port_groups_ref) { > > > + sset_add(ctx->port_groups_ref, ctx->lexer->token.s); > > > + } > > > + > > > struct expr_constant_set *port_group > > > = (ctx->port_groups > > > ? shash_find_data(ctx->port_groups, ctx->lexer->token.s) > > > @@ -1296,13 +1301,15 @@ struct expr * > > > expr_parse(struct lexer *lexer, const struct shash *symtab, > > > const struct shash *addr_sets, > > > const struct shash *port_groups, > > > - struct sset *addr_sets_ref) > > > + struct sset *addr_sets_ref, > > > + struct sset *port_groups_ref) > > > { > > > struct expr_context ctx = { .lexer = lexer, > > > .symtab = symtab, > > > .addr_sets = addr_sets, > > > .port_groups = port_groups, > > > - .addr_sets_ref = addr_sets_ref }; > > > + .addr_sets_ref = addr_sets_ref, > > > + .port_groups_ref = port_groups_ref }; > > > return lexer->error ? NULL : expr_parse__(&ctx); > > > } > > > > > > @@ -1317,6 +1324,7 @@ expr_parse_string(const char *s, const struct shash > > > *symtab, > > > const struct shash *addr_sets, > > > const struct shash *port_groups, > > > struct sset *addr_sets_ref, > > > + struct sset *port_groups_ref, > > > char **errorp) > > > { > > > struct lexer lexer; > > > @@ -1324,7 +1332,7 @@ expr_parse_string(const char *s, const struct shash > > > *symtab, > > > lexer_init(&lexer, s); > > > lexer_get(&lexer); > > > struct expr *expr = expr_parse(&lexer, symtab, addr_sets, > > > port_groups, > > > - addr_sets_ref); > > > + addr_sets_ref, port_groups_ref); > > > lexer_force_end(&lexer); > > > *errorp = lexer_steal_error(&lexer); > > > if (*errorp) { > > > @@ -1550,7 +1558,8 @@ 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, NULL, NULL, NULL, > > > errorp); > > > + struct expr *expr = expr_parse_string(s, symtab, NULL, NULL, NULL, > > > NULL, > > > + errorp); > > > enum expr_level level = expr ? expr_get_level(expr) : EXPR_L_NOMINAL; > > > expr_destroy(expr); > > > return level; > > > @@ -1721,7 +1730,7 @@ parse_and_annotate(const char *s, const struct > > > shash *symtab, > > > char *error; > > > struct expr *expr; > > > > > > - expr = expr_parse_string(s, symtab, NULL, NULL, NULL, &error); > > > + expr = expr_parse_string(s, symtab, NULL, NULL, NULL, NULL, &error); > > > if (expr) { > > > expr = expr_annotate_(expr, symtab, nesting, &error); > > > } > > > @@ -3445,7 +3454,8 @@ expr_parse_microflow(const char *s, const struct > > > shash *symtab, > > > lexer_init(&lexer, s); > > > lexer_get(&lexer); > > > > > > - struct expr *e = expr_parse(&lexer, symtab, addr_sets, port_groups, > > > NULL); > > > + struct expr *e = expr_parse(&lexer, symtab, addr_sets, port_groups, > > > + NULL, NULL); > > > lexer_force_end(&lexer); > > > > > > if (e) { > > > diff --git a/tests/test-ovn.c b/tests/test-ovn.c > > > index c16f9c5..5366905 100644 > > > --- a/tests/test-ovn.c > > > +++ b/tests/test-ovn.c > > > @@ -289,7 +289,7 @@ test_parse_expr__(int steps) > > > char *error; > > > > > > expr = expr_parse_string(ds_cstr(&input), &symtab, &addr_sets, > > > - &port_groups, NULL, &error); > > > + &port_groups, NULL, NULL, &error); > > > if (!error && steps > 0) { > > > expr = expr_annotate(expr, &symtab, &error); > > > } > > > @@ -413,8 +413,8 @@ test_evaluate_expr(struct ovs_cmdl_context *ctx) > > > while (!ds_get_test_line(&input, stdin)) { > > > struct expr *expr; > > > > > > - expr = expr_parse_string(ds_cstr(&input), &symtab, NULL, NULL, > > > NULL, > > > - &error); > > > + expr = expr_parse_string(ds_cstr(&input), &symtab, NULL, NULL, > > > + NULL, NULL, &error); > > > if (!error) { > > > expr = expr_annotate(expr, &symtab, &error); > > > } > > > @@ -889,7 +889,7 @@ test_tree_shape_exhaustively(struct expr *expr, > > > struct shash *symtab, > > > > > > char *error; > > > modified = expr_parse_string(ds_cstr(&s), symtab, NULL, > > > - NULL, NULL, &error); > > > + NULL, NULL, NULL, &error); > > > if (error) { > > > fprintf(stderr, "%s fails to parse (%s)\n", > > > ds_cstr(&s), error); > > > diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c > > > index 19b82e6..2645438 100644 > > > --- a/utilities/ovn-trace.c > > > +++ b/utilities/ovn-trace.c > > > @@ -866,7 +866,7 @@ read_flows(void) > > > char *error; > > > struct expr *match; > > > match = expr_parse_string(sblf->match, &symtab, &address_sets, > > > - &port_groups, NULL, &error); > > > + &port_groups, NULL, NULL, &error); > > > if (error) { > > > VLOG_WARN("%s: parsing expression failed (%s)", > > > sblf->match, error); > > > -- > > > 1.8.3.1 > > > > > > > Thanks for the fix. I applied to master. > > It was a big miss from me, sorry. I was suprised that the parsing wasn't > > added initially. I figured out that I didn't add test case for port-group > > I-P, although I did add the test case for address-set I-P. I think we'd > > better still add a test case similar to: "Address Set Incremental > > Processing", to avoid regression in the future.
Ack, I'll add a test case soon. > > Could you also send a backport patch for 2.12? Thanks! Sure, thanks! _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
