[ovs-dev] [PATCH ovn v2] ovn-controller: Add missing port group lflow references.
Han Zhou
hzhou at ovn.org
Mon Dec 2 23:05:17 UTC 2019
On Mon, Dec 2, 2019 at 2:38 PM Han Zhou <hzhou at ovn.org> wrote:
>
>
>
> On Mon, Dec 2, 2019 at 5:20 AM Dumitru Ceara <dceara at 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 <dalvarez at redhat.com>
> > Reported-at: https://bugzilla.redhat.com/1778164
> > CC: Han Zhou <hzhou at ovn.org>
> > Fixes: 978f5e90af0a ("ovn-controller: Incremental processing for
port-group changes.")
> > Signed-off-by: Dumitru Ceara <dceara at 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.
Could you also send a backport patch for 2.12? Thanks!
More information about the dev
mailing list