| Message ID | 1593444281-19811-1-git-send-email-dceara@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,ovn,v2] Split SB Port_Group per datapath. | expand |
On Mon, Jun 29, 2020 at 8:55 PM Dumitru Ceara <dceara@redhat.com> wrote: > In order to avoid ovn-controller reinstalling all logical flows that > refer a port_group when some ports are added/removed from the port group > we now change the way ovn-northd populates the Southbound DB Port_Group > table. > > Instead of copying NB.Port_Group.name to SB.Port_Group.name we now > create one SB.Port_Group record for every datapath that has ports > referenced by the NB.Port_Group.ports field. In order to maintain the > SB.Port_Group.name uniqueness constraint, ovn-northd populates the field > with the value: <SB.Logical_Datapath.tunnel_key>_<NB.Port_Group.name>. > > In specific scenarios we see significant improvements in time to > install/remove all logical flows to/from OVS. One such scenario, in the > BZ referenced below has: > > $ ovn-nbctl acl-list pg > from-lport 1001 (inport == @pg && ip) drop > to-lport 1001 (outport == @pg && ip) drop > > Then, incrementally, creates new logical ports on different logical > switches, binds them to OVS interfaces and adds them to the port_group. > > Measuring the total time to perform the above steps 500 times (for 500 > new ports attached to 100 switches, 5 per switch) on a test setup > we observe an improvement of 50% in time it takes to install all > openflow rules when port_groups are split in the SB database. > > Suggested-by: Numan Siddique <numans@ovn.org> > Reported-by: Venkata Anil <anilvenkata@redhat.com> > Reported-at: https://bugzilla.redhat.com/1818128 > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > > --- > v2: > - add tests in ovn-northd.at as suggested by Numan. > Thanks for v2 and adding tests. Acked-by: Numan Siddique <numans@ovn.org> Numan > --- > TODO.rst | 8 ++++++ > controller/lflow.c | 4 ++- > include/ovn/expr.h | 4 ++- > lib/actions.c | 2 +- > lib/expr.c | 48 ++++++++++++++++++++++++------- > lib/ovn-util.h | 7 +++++ > northd/ovn-northd.c | 79 > ++++++++++++++++++++++++++++++++++----------------- > tests/ovn-northd.at | 79 > +++++++++++++++++++++++++++++++++++++++++++++++++++ > tests/test-ovn.c | 10 +++---- > utilities/ovn-trace.c | 3 +- > 10 files changed, 198 insertions(+), 46 deletions(-) > > diff --git a/TODO.rst b/TODO.rst > index 6df6b84..4b2fc2d 100644 > --- a/TODO.rst > +++ b/TODO.rst > @@ -140,3 +140,11 @@ OVN To-do List > * ovn-controller: Stop copying the local OVS configuration into the > Chassis external_ids column (same for the "is-remote" configuration from > ovn-ic) a few releases after the 20.06 version (21.06 maybe ?). > + > +* ovn-controller: Remove backwards compatibility for Southbound DB > Port_Group > + names in expr.c a few releases after the 20.09 version. Right now > + ovn-controller maintains backwards compatibility when connecting to a > + SB database that doesn't store Port_Group.name as > + <Logical_Datapath.tunnel_key_NB-Port_Group.name>. This causes an > additional > + hashtable lookup in parse_port_group() which can be avoided when we are > sure > + that the Southbound DB uses the new format. > diff --git a/controller/lflow.c b/controller/lflow.c > index c850a0d..5454361 100644 > --- a/controller/lflow.c > +++ b/controller/lflow.c > @@ -566,7 +566,9 @@ consider_logical_flow(const struct sbrec_logical_flow > *lflow, > struct sset port_groups_ref = SSET_INITIALIZER(&port_groups_ref); > expr = expr_parse_string(lflow->match, &symtab, l_ctx_in->addr_sets, > l_ctx_in->port_groups, > - &addr_sets_ref, &port_groups_ref, &error); > + &addr_sets_ref, &port_groups_ref, > + lflow->logical_datapath->tunnel_key, > + &error); > const char *addr_set_name; > SSET_FOR_EACH (addr_set_name, &addr_sets_ref) { > lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_ADDRSET, > addr_set_name, > diff --git a/include/ovn/expr.h b/include/ovn/expr.h > index 21bf51c..9838251 100644 > --- a/include/ovn/expr.h > +++ b/include/ovn/expr.h > @@ -391,12 +391,14 @@ 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 *port_groups_ref); > + struct sset *port_groups_ref, > + int64_t dp_id); > 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, > + int64_t dp_id, > char **errorp); > > struct expr *expr_clone(struct expr *); > diff --git a/lib/actions.c b/lib/actions.c > index ee7c825..fc6e191 100644 > --- a/lib/actions.c > +++ b/lib/actions.c > @@ -242,7 +242,7 @@ add_prerequisite(struct action_context *ctx, const > char *prerequisite) > char *error; > > expr = expr_parse_string(prerequisite, ctx->pp->symtab, NULL, NULL, > - NULL, NULL, &error); > + NULL, NULL, 0, &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 078d178..497b2ac 100644 > --- a/lib/expr.c > +++ b/lib/expr.c > @@ -29,6 +29,7 @@ > #include "simap.h" > #include "sset.h" > #include "util.h" > +#include "ovn-util.h" > > VLOG_DEFINE_THIS_MODULE(expr); > > @@ -482,6 +483,10 @@ struct expr_context { > const struct shash *port_groups; /* Port group table. */ > struct sset *addr_sets_ref; /* The set of address set > referenced. */ > struct sset *port_groups_ref; /* The set of port groups > referenced. */ > + int64_t dp_id; /* The tunnel_key of the datapath for > + which we're parsing the current > + expression. */ > + > bool not; /* True inside odd number of NOT > operators. */ > unsigned int paren_depth; /* Depth of nested parentheses. */ > }; > @@ -783,14 +788,32 @@ static bool > parse_port_group(struct expr_context *ctx, struct expr_constant_set *cs, > size_t *allocated_values) > { > + struct ds sb_name = DS_EMPTY_INITIALIZER; > + > + get_sb_port_group_name(ctx->lexer->token.s, ctx->dp_id, &sb_name); > if (ctx->port_groups_ref) { > - sset_add(ctx->port_groups_ref, ctx->lexer->token.s); > + sset_add(ctx->port_groups_ref, ds_cstr(&sb_name)); > + } > + > + struct expr_constant_set *port_group = NULL; > + > + if (ctx->port_groups) { > + port_group = shash_find_data(ctx->port_groups, ds_cstr(&sb_name)); > + if (!port_group) { > + /* For backwards compatibility (e.g., ovn-controller was > + * upgraded but ovn-northd not yet), perform an additional > + * lookup because the NB Port_Group.name might have been > + * stored as is in the SB Port_Group.name field. > + */ > + port_group = shash_find_data(ctx->port_groups, > + ctx->lexer->token.s); > + if (ctx->port_groups_ref) { > + sset_add(ctx->port_groups_ref, ctx->lexer->token.s); > + } > + } > } > + ds_destroy(&sb_name); > > - struct expr_constant_set *port_group > - = (ctx->port_groups > - ? shash_find_data(ctx->port_groups, ctx->lexer->token.s) > - : NULL); > if (!port_group) { > lexer_syntax_error(ctx->lexer, "expecting port group name"); > return false; > @@ -1302,14 +1325,16 @@ 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 *port_groups_ref) > + struct sset *port_groups_ref, > + int64_t dp_id) > { > struct expr_context ctx = { .lexer = lexer, > .symtab = symtab, > .addr_sets = addr_sets, > .port_groups = port_groups, > .addr_sets_ref = addr_sets_ref, > - .port_groups_ref = port_groups_ref }; > + .port_groups_ref = port_groups_ref, > + .dp_id = dp_id }; > return lexer->error ? NULL : expr_parse__(&ctx); > } > > @@ -1325,6 +1350,7 @@ expr_parse_string(const char *s, const struct shash > *symtab, > const struct shash *port_groups, > struct sset *addr_sets_ref, > struct sset *port_groups_ref, > + int64_t dp_id, > char **errorp) > { > struct lexer lexer; > @@ -1332,7 +1358,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, port_groups_ref); > + addr_sets_ref, port_groups_ref, dp_id); > lexer_force_end(&lexer); > *errorp = lexer_steal_error(&lexer); > if (*errorp) { > @@ -1558,7 +1584,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, NULL, NULL, NULL, > NULL, > + struct expr *expr = expr_parse_string(s, symtab, NULL, NULL, NULL, > NULL, 0, > errorp); > enum expr_level level = expr ? expr_get_level(expr) : EXPR_L_NOMINAL; > expr_destroy(expr); > @@ -1730,7 +1756,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, NULL, &error); > + expr = expr_parse_string(s, symtab, NULL, NULL, NULL, NULL, 0, > &error); > if (expr) { > expr = expr_annotate_(expr, symtab, nesting, &error); > } > @@ -3456,7 +3482,7 @@ expr_parse_microflow(const char *s, const struct > shash *symtab, > lexer_get(&lexer); > > struct expr *e = expr_parse(&lexer, symtab, addr_sets, port_groups, > - NULL, NULL); > + NULL, NULL, 0); > lexer_force_end(&lexer); > > if (e) { > diff --git a/lib/ovn-util.h b/lib/ovn-util.h > index eba2948..4e08ee0 100644 > --- a/lib/ovn-util.h > +++ b/lib/ovn-util.h > @@ -122,6 +122,13 @@ get_unique_lport_key(uint64_t dp_tunnel_key, uint64_t > lport_tunnel_key, > lport_tunnel_key); > } > > +static inline void > +get_sb_port_group_name(const char *nb_pg_name, int64_t dp_tunnel_key, > + struct ds *sb_pg_name) > +{ > + ds_put_format(sb_pg_name, "%"PRId64"_%s", dp_tunnel_key, nb_pg_name); > +} > + > char *ovn_chassis_redirect_name(const char *port_name); > void ovn_set_pidfile(const char *name); > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index f49e4f2..e1dc491 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -4471,7 +4471,11 @@ build_dhcpv6_action(struct ovn_port *op, struct > in6_addr *offer_ip, > struct ovn_port_group_ls { > struct hmap_node key_node; /* Index on 'key'. */ > struct uuid key; /* nb_ls->header_.uuid. */ > - const struct nbrec_logical_switch *nb_ls; > + struct ovn_datapath *od; > + > + struct ovn_port **ports; /* Ports in 'od' referrenced by the PG. */ > + size_t n_ports; > + size_t n_allocated_ports; > }; > > struct ovn_port_group { > @@ -4481,14 +4485,14 @@ struct ovn_port_group { > struct hmap nb_lswitches; /* NB lswitches related to the port group > */ > }; > > -static void > -ovn_port_group_ls_add(struct ovn_port_group *pg, > - const struct nbrec_logical_switch *nb_ls) > +static struct ovn_port_group_ls * > +ovn_port_group_ls_add(struct ovn_port_group *pg, struct ovn_datapath *od) > { > struct ovn_port_group_ls *pg_ls = xzalloc(sizeof *pg_ls); > - pg_ls->key = nb_ls->header_.uuid; > - pg_ls->nb_ls = nb_ls; > + pg_ls->key = od->nbs->header_.uuid; > + pg_ls->od = od; > hmap_insert(&pg->nb_lswitches, &pg_ls->key_node, > uuid_hash(&pg_ls->key)); > + return pg_ls; > } > > static struct ovn_port_group_ls * > @@ -4505,6 +4509,18 @@ ovn_port_group_ls_find(struct ovn_port_group *pg, > const struct uuid *ls_uuid) > return NULL; > } > > +static void > +ovn_port_group_ls_add_port(struct ovn_port_group_ls *pg_ls, > + struct ovn_port *op) > +{ > + if (pg_ls->n_ports == pg_ls->n_allocated_ports) { > + pg_ls->ports = x2nrealloc(pg_ls->ports, > + &pg_ls->n_allocated_ports, > + sizeof *pg_ls->ports); > + } > + pg_ls->ports[pg_ls->n_ports++] = op; > +} > + > struct ovn_ls_port_group { > struct hmap_node key_node; /* Index on 'key'. */ > struct uuid key; /* nb_pg->header_.uuid. */ > @@ -5252,6 +5268,7 @@ ovn_port_group_destroy(struct hmap *pgs, struct > ovn_port_group *pg) > hmap_remove(pgs, &pg->key_node); > struct ovn_port_group_ls *ls; > HMAP_FOR_EACH_POP (ls, key_node, &pg->nb_lswitches) { > + free(ls->ports); > free(ls); > } > hmap_destroy(&pg->nb_lswitches); > @@ -5289,9 +5306,10 @@ build_port_group_lswitches(struct northd_context > *ctx, struct hmap *pgs, > struct ovn_port_group_ls *pg_ls = > ovn_port_group_ls_find(pg, &op->od->nbs->header_.uuid); > if (!pg_ls) { > - ovn_port_group_ls_add(pg, op->od->nbs); > + pg_ls = ovn_port_group_ls_add(pg, op->od); > ovn_ls_port_group_add(&op->od->nb_pgs, nb_pg); > } > + ovn_port_group_ls_add_port(pg_ls, op); > } > } > } > @@ -10509,7 +10527,7 @@ sync_address_sets(struct northd_context *ctx) > * contains lport uuids, while in OVN_Southbound we store the lport names. > */ > static void > -sync_port_groups(struct northd_context *ctx) > +sync_port_groups(struct northd_context *ctx, struct hmap *pgs) > { > struct shash sb_port_groups = SHASH_INITIALIZER(&sb_port_groups); > > @@ -10518,26 +10536,35 @@ sync_port_groups(struct northd_context *ctx) > shash_add(&sb_port_groups, sb_port_group->name, sb_port_group); > } > > - const struct nbrec_port_group *nb_port_group; > - NBREC_PORT_GROUP_FOR_EACH (nb_port_group, ctx->ovnnb_idl) { > - sb_port_group = shash_find_and_delete(&sb_port_groups, > - nb_port_group->name); > - if (!sb_port_group) { > - sb_port_group = sbrec_port_group_insert(ctx->ovnsb_txn); > - sbrec_port_group_set_name(sb_port_group, nb_port_group->name); > - } > + struct ds sb_name = DS_EMPTY_INITIALIZER; > > - const char **nb_port_names = xcalloc(nb_port_group->n_ports, > - sizeof *nb_port_names); > - int i; > - for (i = 0; i < nb_port_group->n_ports; i++) { > - nb_port_names[i] = nb_port_group->ports[i]->name; > + struct ovn_port_group *pg; > + HMAP_FOR_EACH (pg, key_node, pgs) { > + > + struct ovn_port_group_ls *pg_ls; > + HMAP_FOR_EACH (pg_ls, key_node, &pg->nb_lswitches) { > + ds_clear(&sb_name); > + get_sb_port_group_name(pg->nb_pg->name, > pg_ls->od->sb->tunnel_key, > + &sb_name); > + sb_port_group = shash_find_and_delete(&sb_port_groups, > + ds_cstr(&sb_name)); > + if (!sb_port_group) { > + sb_port_group = sbrec_port_group_insert(ctx->ovnsb_txn); > + sbrec_port_group_set_name(sb_port_group, > ds_cstr(&sb_name)); > + } > + > + const char **nb_port_names = xcalloc(pg_ls->n_ports, > + sizeof *nb_port_names); > + for (size_t i = 0; i < pg_ls->n_ports; i++) { > + nb_port_names[i] = pg_ls->ports[i]->nbsp->name; > + } > + sbrec_port_group_set_ports(sb_port_group, > + nb_port_names, > + pg_ls->n_ports); > + free(nb_port_names); > } > - sbrec_port_group_set_ports(sb_port_group, > - nb_port_names, > - nb_port_group->n_ports); > - free(nb_port_names); > } > + ds_destroy(&sb_name); > > struct shash_node *node, *next; > SHASH_FOR_EACH_SAFE (node, next, &sb_port_groups) { > @@ -11140,7 +11167,7 @@ ovnnb_db_run(struct northd_context *ctx, > ovn_update_ipv6_prefix(ports); > > sync_address_sets(ctx); > - sync_port_groups(ctx); > + sync_port_groups(ctx, &port_groups); > sync_meters(ctx); > sync_dns_entries(ctx, datapaths); > destroy_ovn_lbs(&lbs); > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 80bda0a..d7a940f 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -1472,3 +1472,82 @@ AT_CHECK([ovn-nbctl --wait=sb sync], [0]) > AT_CHECK([test 0 = $(ovn-sbctl list Ha_Chassis_Group | wc -l)]) > > AT_CLEANUP > + > +AT_SETUP([ovn -- check NB/SB Port_Group translation (lsp add/del)]) > +ovn_start > + > +ovn-nbctl ls-add ls1 > +ovn-nbctl ls-add ls2 > +ovn-nbctl lsp-add ls1 lsp1 > +ovn-nbctl lsp-add ls2 lsp2 > +ovn-nbct --wait=sb sync > +ls1_key=$(ovn-sbctl --columns tunnel_key --bare list Datapath_Binding ls1) > +ls2_key=$(ovn-sbctl --columns tunnel_key --bare list Datapath_Binding ls2) > + > +# Add an empty port group. This should generate no entry in the SB. > +ovn-nbctl --wait=sb pg-add pg_test > +AT_CHECK([test 0 = $(ovn-sbctl --columns _uuid list Port_Group | grep > uuid -c)]) > + > +# Add lsp1 to the port group. This should generate an entry in the SB only > +# for ls1. > +ovn-nbctl --wait=sb pg-set-ports pg_test lsp1 > +AT_CHECK([test 1 = $(ovn-sbctl --columns _uuid list Port_Group | grep > uuid -c)]) > +AT_CHECK([ovn-sbctl --columns ports --bare find Port_Group > name=${ls1_key}_pg_test], [0], [dnl > +lsp1 > +]) > + > +# Add lsp2 to the port group. This should generate a new entry in the SB, > for > +# ls2. > +ovn-nbctl --wait=sb pg-set-ports pg_test lsp1 lsp2 > +AT_CHECK([test 2 = $(ovn-sbctl --columns _uuid list Port_Group | grep > uuid -c)]) > +AT_CHECK([ovn-sbctl --columns ports --bare find Port_Group > name=${ls1_key}_pg_test], [0], [dnl > +lsp1 > +]) > +AT_CHECK([ovn-sbctl --columns ports --bare find Port_Group > name=${ls2_key}_pg_test], [0], [dnl > +lsp2 > +]) > + > +# Remove lsp1 from the port group. The SB Port_Group for ls1 should be > +# removed. > +ovn-nbctl --wait=sb pg-set-ports pg_test lsp2 > +AT_CHECK([test 1 = $(ovn-sbctl --columns _uuid list Port_Group | grep > uuid -c)]) > +AT_CHECK([ovn-sbctl --columns ports --bare find Port_Group > name=${ls2_key}_pg_test], [0], [dnl > +lsp2 > +]) > + > +# Remove lsp2 from the port group. All SB Port_Groups should be purged. > +ovn-nbctl --wait=sb clear Port_Group pg_test ports > +AT_CHECK([test 0 = $(ovn-sbctl --columns _uuid list Port_Group | grep > uuid -c)]) > + > +AT_CLEANUP > + > +AT_SETUP([ovn -- check NB/SB Port_Group translation (ls del)]) > +ovn_start > + > +ovn-nbctl ls-add ls1 > +ovn-nbctl ls-add ls2 > +ovn-nbctl lsp-add ls1 lsp1 > +ovn-nbctl lsp-add ls2 lsp2 > +ovn-nbct --wait=sb sync > +ls1_key=$(ovn-sbctl --columns tunnel_key --bare list Datapath_Binding ls1) > +ls2_key=$(ovn-sbctl --columns tunnel_key --bare list Datapath_Binding ls2) > + > +# Add lsp1 & lsp2 to a port group. This should generate two entries in the > +# SB (one per logical switch). > +ovn-nbctl --wait=sb pg-add pg_test lsp1 lsp2 > +AT_CHECK([test 2 = $(ovn-sbctl --columns _uuid list Port_Group | grep > uuid -c)]) > +AT_CHECK([ovn-sbctl --columns ports --bare find Port_Group > name=${ls1_key}_pg_test], [0], [dnl > +lsp1 > +]) > +AT_CHECK([ovn-sbctl --columns ports --bare find Port_Group > name=${ls2_key}_pg_test], [0], [dnl > +lsp2 > +]) > + > +# Delete logical switch ls1. This should remove the associated SB > Port_Group. > +ovn-nbctl --wait=sb ls-del ls1 > +AT_CHECK([test 1 = $(ovn-sbctl --columns _uuid list Port_Group | grep > uuid -c)]) > +AT_CHECK([ovn-sbctl --columns ports --bare find Port_Group > name=${ls2_key}_pg_test], [0], [dnl > +lsp2 > +]) > + > +AT_CLEANUP > diff --git a/tests/test-ovn.c b/tests/test-ovn.c > index b43f67f..c3bfd20 100644 > --- a/tests/test-ovn.c > +++ b/tests/test-ovn.c > @@ -238,8 +238,8 @@ create_port_groups(struct shash *port_groups) > }; > static const char *const pg2[] = { NULL }; > > - expr_const_sets_add(port_groups, "pg1", pg1, 3, false); > - expr_const_sets_add(port_groups, "pg_empty", pg2, 0, false); > + expr_const_sets_add(port_groups, "0_pg1", pg1, 3, false); > + expr_const_sets_add(port_groups, "0_pg_empty", pg2, 0, false); > } > > static bool > @@ -305,7 +305,7 @@ test_parse_expr__(int steps) > char *error; > > expr = expr_parse_string(ds_cstr(&input), &symtab, &addr_sets, > - &port_groups, NULL, NULL, &error); > + &port_groups, NULL, NULL, 0, &error); > if (!error && steps > 0) { > expr = expr_annotate(expr, &symtab, &error); > } > @@ -431,7 +431,7 @@ test_evaluate_expr(struct ovs_cmdl_context *ctx) > struct expr *expr; > > expr = expr_parse_string(ds_cstr(&input), &symtab, NULL, NULL, > - NULL, NULL, &error); > + NULL, NULL, 0, &error); > if (!error) { > expr = expr_annotate(expr, &symtab, &error); > } > @@ -906,7 +906,7 @@ test_tree_shape_exhaustively(struct expr *expr, struct > shash *symtab, > > char *error; > modified = expr_parse_string(ds_cstr(&s), symtab, NULL, > - NULL, NULL, NULL, &error); > + NULL, NULL, NULL, 0, &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 d7251e7..2666c10 100644 > --- a/utilities/ovn-trace.c > +++ b/utilities/ovn-trace.c > @@ -889,7 +889,8 @@ read_flows(void) > char *error; > struct expr *match; > match = expr_parse_string(sblf->match, &symtab, &address_sets, > - &port_groups, NULL, NULL, &error); > + &port_groups, NULL, NULL, > dp->tunnel_key, > + &error); > if (error) { > VLOG_WARN("%s: parsing expression failed (%s)", > sblf->match, error); > -- > 1.8.3.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On 6/30/20 4:42 AM, Numan Siddique wrote: > On Mon, Jun 29, 2020 at 8:55 PM Dumitru Ceara <dceara@redhat.com> wrote: > >> In order to avoid ovn-controller reinstalling all logical flows that >> refer a port_group when some ports are added/removed from the port group >> we now change the way ovn-northd populates the Southbound DB Port_Group >> table. >> >> Instead of copying NB.Port_Group.name to SB.Port_Group.name we now >> create one SB.Port_Group record for every datapath that has ports >> referenced by the NB.Port_Group.ports field. In order to maintain the >> SB.Port_Group.name uniqueness constraint, ovn-northd populates the field >> with the value: <SB.Logical_Datapath.tunnel_key>_<NB.Port_Group.name>. >> >> In specific scenarios we see significant improvements in time to >> install/remove all logical flows to/from OVS. One such scenario, in the >> BZ referenced below has: >> >> $ ovn-nbctl acl-list pg >> from-lport 1001 (inport == @pg && ip) drop >> to-lport 1001 (outport == @pg && ip) drop >> >> Then, incrementally, creates new logical ports on different logical >> switches, binds them to OVS interfaces and adds them to the port_group. >> >> Measuring the total time to perform the above steps 500 times (for 500 >> new ports attached to 100 switches, 5 per switch) on a test setup >> we observe an improvement of 50% in time it takes to install all >> openflow rules when port_groups are split in the SB database. >> >> Suggested-by: Numan Siddique <numans@ovn.org> >> Reported-by: Venkata Anil <anilvenkata@redhat.com> >> Reported-at: https://bugzilla.redhat.com/1818128 >> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >> >> --- >> v2: >> - add tests in ovn-northd.at as suggested by Numan. >> > > Thanks for v2 and adding tests. > > Acked-by: Numan Siddique <numans@ovn.org> > > Numan I pushed this to master and branch-20.06 > > >> --- >> TODO.rst | 8 ++++++ >> controller/lflow.c | 4 ++- >> include/ovn/expr.h | 4 ++- >> lib/actions.c | 2 +- >> lib/expr.c | 48 ++++++++++++++++++++++++------- >> lib/ovn-util.h | 7 +++++ >> northd/ovn-northd.c | 79 >> ++++++++++++++++++++++++++++++++++----------------- >> tests/ovn-northd.at | 79 >> +++++++++++++++++++++++++++++++++++++++++++++++++++ >> tests/test-ovn.c | 10 +++---- >> utilities/ovn-trace.c | 3 +- >> 10 files changed, 198 insertions(+), 46 deletions(-) >> >> diff --git a/TODO.rst b/TODO.rst >> index 6df6b84..4b2fc2d 100644 >> --- a/TODO.rst >> +++ b/TODO.rst >> @@ -140,3 +140,11 @@ OVN To-do List >> * ovn-controller: Stop copying the local OVS configuration into the >> Chassis external_ids column (same for the "is-remote" configuration from >> ovn-ic) a few releases after the 20.06 version (21.06 maybe ?). >> + >> +* ovn-controller: Remove backwards compatibility for Southbound DB >> Port_Group >> + names in expr.c a few releases after the 20.09 version. Right now >> + ovn-controller maintains backwards compatibility when connecting to a >> + SB database that doesn't store Port_Group.name as >> + <Logical_Datapath.tunnel_key_NB-Port_Group.name>. This causes an >> additional >> + hashtable lookup in parse_port_group() which can be avoided when we are >> sure >> + that the Southbound DB uses the new format. >> diff --git a/controller/lflow.c b/controller/lflow.c >> index c850a0d..5454361 100644 >> --- a/controller/lflow.c >> +++ b/controller/lflow.c >> @@ -566,7 +566,9 @@ consider_logical_flow(const struct sbrec_logical_flow >> *lflow, >> struct sset port_groups_ref = SSET_INITIALIZER(&port_groups_ref); >> expr = expr_parse_string(lflow->match, &symtab, l_ctx_in->addr_sets, >> l_ctx_in->port_groups, >> - &addr_sets_ref, &port_groups_ref, &error); >> + &addr_sets_ref, &port_groups_ref, >> + lflow->logical_datapath->tunnel_key, >> + &error); >> const char *addr_set_name; >> SSET_FOR_EACH (addr_set_name, &addr_sets_ref) { >> lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_ADDRSET, >> addr_set_name, >> diff --git a/include/ovn/expr.h b/include/ovn/expr.h >> index 21bf51c..9838251 100644 >> --- a/include/ovn/expr.h >> +++ b/include/ovn/expr.h >> @@ -391,12 +391,14 @@ 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 *port_groups_ref); >> + struct sset *port_groups_ref, >> + int64_t dp_id); >> 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, >> + int64_t dp_id, >> char **errorp); >> >> struct expr *expr_clone(struct expr *); >> diff --git a/lib/actions.c b/lib/actions.c >> index ee7c825..fc6e191 100644 >> --- a/lib/actions.c >> +++ b/lib/actions.c >> @@ -242,7 +242,7 @@ add_prerequisite(struct action_context *ctx, const >> char *prerequisite) >> char *error; >> >> expr = expr_parse_string(prerequisite, ctx->pp->symtab, NULL, NULL, >> - NULL, NULL, &error); >> + NULL, NULL, 0, &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 078d178..497b2ac 100644 >> --- a/lib/expr.c >> +++ b/lib/expr.c >> @@ -29,6 +29,7 @@ >> #include "simap.h" >> #include "sset.h" >> #include "util.h" >> +#include "ovn-util.h" >> >> VLOG_DEFINE_THIS_MODULE(expr); >> >> @@ -482,6 +483,10 @@ struct expr_context { >> const struct shash *port_groups; /* Port group table. */ >> struct sset *addr_sets_ref; /* The set of address set >> referenced. */ >> struct sset *port_groups_ref; /* The set of port groups >> referenced. */ >> + int64_t dp_id; /* The tunnel_key of the datapath for >> + which we're parsing the current >> + expression. */ >> + >> bool not; /* True inside odd number of NOT >> operators. */ >> unsigned int paren_depth; /* Depth of nested parentheses. */ >> }; >> @@ -783,14 +788,32 @@ static bool >> parse_port_group(struct expr_context *ctx, struct expr_constant_set *cs, >> size_t *allocated_values) >> { >> + struct ds sb_name = DS_EMPTY_INITIALIZER; >> + >> + get_sb_port_group_name(ctx->lexer->token.s, ctx->dp_id, &sb_name); >> if (ctx->port_groups_ref) { >> - sset_add(ctx->port_groups_ref, ctx->lexer->token.s); >> + sset_add(ctx->port_groups_ref, ds_cstr(&sb_name)); >> + } >> + >> + struct expr_constant_set *port_group = NULL; >> + >> + if (ctx->port_groups) { >> + port_group = shash_find_data(ctx->port_groups, ds_cstr(&sb_name)); >> + if (!port_group) { >> + /* For backwards compatibility (e.g., ovn-controller was >> + * upgraded but ovn-northd not yet), perform an additional >> + * lookup because the NB Port_Group.name might have been >> + * stored as is in the SB Port_Group.name field. >> + */ >> + port_group = shash_find_data(ctx->port_groups, >> + ctx->lexer->token.s); >> + if (ctx->port_groups_ref) { >> + sset_add(ctx->port_groups_ref, ctx->lexer->token.s); >> + } >> + } >> } >> + ds_destroy(&sb_name); >> >> - struct expr_constant_set *port_group >> - = (ctx->port_groups >> - ? shash_find_data(ctx->port_groups, ctx->lexer->token.s) >> - : NULL); >> if (!port_group) { >> lexer_syntax_error(ctx->lexer, "expecting port group name"); >> return false; >> @@ -1302,14 +1325,16 @@ 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 *port_groups_ref) >> + struct sset *port_groups_ref, >> + int64_t dp_id) >> { >> struct expr_context ctx = { .lexer = lexer, >> .symtab = symtab, >> .addr_sets = addr_sets, >> .port_groups = port_groups, >> .addr_sets_ref = addr_sets_ref, >> - .port_groups_ref = port_groups_ref }; >> + .port_groups_ref = port_groups_ref, >> + .dp_id = dp_id }; >> return lexer->error ? NULL : expr_parse__(&ctx); >> } >> >> @@ -1325,6 +1350,7 @@ expr_parse_string(const char *s, const struct shash >> *symtab, >> const struct shash *port_groups, >> struct sset *addr_sets_ref, >> struct sset *port_groups_ref, >> + int64_t dp_id, >> char **errorp) >> { >> struct lexer lexer; >> @@ -1332,7 +1358,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, port_groups_ref); >> + addr_sets_ref, port_groups_ref, dp_id); >> lexer_force_end(&lexer); >> *errorp = lexer_steal_error(&lexer); >> if (*errorp) { >> @@ -1558,7 +1584,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, NULL, NULL, NULL, >> NULL, >> + struct expr *expr = expr_parse_string(s, symtab, NULL, NULL, NULL, >> NULL, 0, >> errorp); >> enum expr_level level = expr ? expr_get_level(expr) : EXPR_L_NOMINAL; >> expr_destroy(expr); >> @@ -1730,7 +1756,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, NULL, &error); >> + expr = expr_parse_string(s, symtab, NULL, NULL, NULL, NULL, 0, >> &error); >> if (expr) { >> expr = expr_annotate_(expr, symtab, nesting, &error); >> } >> @@ -3456,7 +3482,7 @@ expr_parse_microflow(const char *s, const struct >> shash *symtab, >> lexer_get(&lexer); >> >> struct expr *e = expr_parse(&lexer, symtab, addr_sets, port_groups, >> - NULL, NULL); >> + NULL, NULL, 0); >> lexer_force_end(&lexer); >> >> if (e) { >> diff --git a/lib/ovn-util.h b/lib/ovn-util.h >> index eba2948..4e08ee0 100644 >> --- a/lib/ovn-util.h >> +++ b/lib/ovn-util.h >> @@ -122,6 +122,13 @@ get_unique_lport_key(uint64_t dp_tunnel_key, uint64_t >> lport_tunnel_key, >> lport_tunnel_key); >> } >> >> +static inline void >> +get_sb_port_group_name(const char *nb_pg_name, int64_t dp_tunnel_key, >> + struct ds *sb_pg_name) >> +{ >> + ds_put_format(sb_pg_name, "%"PRId64"_%s", dp_tunnel_key, nb_pg_name); >> +} >> + >> char *ovn_chassis_redirect_name(const char *port_name); >> void ovn_set_pidfile(const char *name); >> >> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c >> index f49e4f2..e1dc491 100644 >> --- a/northd/ovn-northd.c >> +++ b/northd/ovn-northd.c >> @@ -4471,7 +4471,11 @@ build_dhcpv6_action(struct ovn_port *op, struct >> in6_addr *offer_ip, >> struct ovn_port_group_ls { >> struct hmap_node key_node; /* Index on 'key'. */ >> struct uuid key; /* nb_ls->header_.uuid. */ >> - const struct nbrec_logical_switch *nb_ls; >> + struct ovn_datapath *od; >> + >> + struct ovn_port **ports; /* Ports in 'od' referrenced by the PG. */ >> + size_t n_ports; >> + size_t n_allocated_ports; >> }; >> >> struct ovn_port_group { >> @@ -4481,14 +4485,14 @@ struct ovn_port_group { >> struct hmap nb_lswitches; /* NB lswitches related to the port group >> */ >> }; >> >> -static void >> -ovn_port_group_ls_add(struct ovn_port_group *pg, >> - const struct nbrec_logical_switch *nb_ls) >> +static struct ovn_port_group_ls * >> +ovn_port_group_ls_add(struct ovn_port_group *pg, struct ovn_datapath *od) >> { >> struct ovn_port_group_ls *pg_ls = xzalloc(sizeof *pg_ls); >> - pg_ls->key = nb_ls->header_.uuid; >> - pg_ls->nb_ls = nb_ls; >> + pg_ls->key = od->nbs->header_.uuid; >> + pg_ls->od = od; >> hmap_insert(&pg->nb_lswitches, &pg_ls->key_node, >> uuid_hash(&pg_ls->key)); >> + return pg_ls; >> } >> >> static struct ovn_port_group_ls * >> @@ -4505,6 +4509,18 @@ ovn_port_group_ls_find(struct ovn_port_group *pg, >> const struct uuid *ls_uuid) >> return NULL; >> } >> >> +static void >> +ovn_port_group_ls_add_port(struct ovn_port_group_ls *pg_ls, >> + struct ovn_port *op) >> +{ >> + if (pg_ls->n_ports == pg_ls->n_allocated_ports) { >> + pg_ls->ports = x2nrealloc(pg_ls->ports, >> + &pg_ls->n_allocated_ports, >> + sizeof *pg_ls->ports); >> + } >> + pg_ls->ports[pg_ls->n_ports++] = op; >> +} >> + >> struct ovn_ls_port_group { >> struct hmap_node key_node; /* Index on 'key'. */ >> struct uuid key; /* nb_pg->header_.uuid. */ >> @@ -5252,6 +5268,7 @@ ovn_port_group_destroy(struct hmap *pgs, struct >> ovn_port_group *pg) >> hmap_remove(pgs, &pg->key_node); >> struct ovn_port_group_ls *ls; >> HMAP_FOR_EACH_POP (ls, key_node, &pg->nb_lswitches) { >> + free(ls->ports); >> free(ls); >> } >> hmap_destroy(&pg->nb_lswitches); >> @@ -5289,9 +5306,10 @@ build_port_group_lswitches(struct northd_context >> *ctx, struct hmap *pgs, >> struct ovn_port_group_ls *pg_ls = >> ovn_port_group_ls_find(pg, &op->od->nbs->header_.uuid); >> if (!pg_ls) { >> - ovn_port_group_ls_add(pg, op->od->nbs); >> + pg_ls = ovn_port_group_ls_add(pg, op->od); >> ovn_ls_port_group_add(&op->od->nb_pgs, nb_pg); >> } >> + ovn_port_group_ls_add_port(pg_ls, op); >> } >> } >> } >> @@ -10509,7 +10527,7 @@ sync_address_sets(struct northd_context *ctx) >> * contains lport uuids, while in OVN_Southbound we store the lport names. >> */ >> static void >> -sync_port_groups(struct northd_context *ctx) >> +sync_port_groups(struct northd_context *ctx, struct hmap *pgs) >> { >> struct shash sb_port_groups = SHASH_INITIALIZER(&sb_port_groups); >> >> @@ -10518,26 +10536,35 @@ sync_port_groups(struct northd_context *ctx) >> shash_add(&sb_port_groups, sb_port_group->name, sb_port_group); >> } >> >> - const struct nbrec_port_group *nb_port_group; >> - NBREC_PORT_GROUP_FOR_EACH (nb_port_group, ctx->ovnnb_idl) { >> - sb_port_group = shash_find_and_delete(&sb_port_groups, >> - nb_port_group->name); >> - if (!sb_port_group) { >> - sb_port_group = sbrec_port_group_insert(ctx->ovnsb_txn); >> - sbrec_port_group_set_name(sb_port_group, nb_port_group->name); >> - } >> + struct ds sb_name = DS_EMPTY_INITIALIZER; >> >> - const char **nb_port_names = xcalloc(nb_port_group->n_ports, >> - sizeof *nb_port_names); >> - int i; >> - for (i = 0; i < nb_port_group->n_ports; i++) { >> - nb_port_names[i] = nb_port_group->ports[i]->name; >> + struct ovn_port_group *pg; >> + HMAP_FOR_EACH (pg, key_node, pgs) { >> + >> + struct ovn_port_group_ls *pg_ls; >> + HMAP_FOR_EACH (pg_ls, key_node, &pg->nb_lswitches) { >> + ds_clear(&sb_name); >> + get_sb_port_group_name(pg->nb_pg->name, >> pg_ls->od->sb->tunnel_key, >> + &sb_name); >> + sb_port_group = shash_find_and_delete(&sb_port_groups, >> + ds_cstr(&sb_name)); >> + if (!sb_port_group) { >> + sb_port_group = sbrec_port_group_insert(ctx->ovnsb_txn); >> + sbrec_port_group_set_name(sb_port_group, >> ds_cstr(&sb_name)); >> + } >> + >> + const char **nb_port_names = xcalloc(pg_ls->n_ports, >> + sizeof *nb_port_names); >> + for (size_t i = 0; i < pg_ls->n_ports; i++) { >> + nb_port_names[i] = pg_ls->ports[i]->nbsp->name; >> + } >> + sbrec_port_group_set_ports(sb_port_group, >> + nb_port_names, >> + pg_ls->n_ports); >> + free(nb_port_names); >> } >> - sbrec_port_group_set_ports(sb_port_group, >> - nb_port_names, >> - nb_port_group->n_ports); >> - free(nb_port_names); >> } >> + ds_destroy(&sb_name); >> >> struct shash_node *node, *next; >> SHASH_FOR_EACH_SAFE (node, next, &sb_port_groups) { >> @@ -11140,7 +11167,7 @@ ovnnb_db_run(struct northd_context *ctx, >> ovn_update_ipv6_prefix(ports); >> >> sync_address_sets(ctx); >> - sync_port_groups(ctx); >> + sync_port_groups(ctx, &port_groups); >> sync_meters(ctx); >> sync_dns_entries(ctx, datapaths); >> destroy_ovn_lbs(&lbs); >> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at >> index 80bda0a..d7a940f 100644 >> --- a/tests/ovn-northd.at >> +++ b/tests/ovn-northd.at >> @@ -1472,3 +1472,82 @@ AT_CHECK([ovn-nbctl --wait=sb sync], [0]) >> AT_CHECK([test 0 = $(ovn-sbctl list Ha_Chassis_Group | wc -l)]) >> >> AT_CLEANUP >> + >> +AT_SETUP([ovn -- check NB/SB Port_Group translation (lsp add/del)]) >> +ovn_start >> + >> +ovn-nbctl ls-add ls1 >> +ovn-nbctl ls-add ls2 >> +ovn-nbctl lsp-add ls1 lsp1 >> +ovn-nbctl lsp-add ls2 lsp2 >> +ovn-nbct --wait=sb sync >> +ls1_key=$(ovn-sbctl --columns tunnel_key --bare list Datapath_Binding ls1) >> +ls2_key=$(ovn-sbctl --columns tunnel_key --bare list Datapath_Binding ls2) >> + >> +# Add an empty port group. This should generate no entry in the SB. >> +ovn-nbctl --wait=sb pg-add pg_test >> +AT_CHECK([test 0 = $(ovn-sbctl --columns _uuid list Port_Group | grep >> uuid -c)]) >> + >> +# Add lsp1 to the port group. This should generate an entry in the SB only >> +# for ls1. >> +ovn-nbctl --wait=sb pg-set-ports pg_test lsp1 >> +AT_CHECK([test 1 = $(ovn-sbctl --columns _uuid list Port_Group | grep >> uuid -c)]) >> +AT_CHECK([ovn-sbctl --columns ports --bare find Port_Group >> name=${ls1_key}_pg_test], [0], [dnl >> +lsp1 >> +]) >> + >> +# Add lsp2 to the port group. This should generate a new entry in the SB, >> for >> +# ls2. >> +ovn-nbctl --wait=sb pg-set-ports pg_test lsp1 lsp2 >> +AT_CHECK([test 2 = $(ovn-sbctl --columns _uuid list Port_Group | grep >> uuid -c)]) >> +AT_CHECK([ovn-sbctl --columns ports --bare find Port_Group >> name=${ls1_key}_pg_test], [0], [dnl >> +lsp1 >> +]) >> +AT_CHECK([ovn-sbctl --columns ports --bare find Port_Group >> name=${ls2_key}_pg_test], [0], [dnl >> +lsp2 >> +]) >> + >> +# Remove lsp1 from the port group. The SB Port_Group for ls1 should be >> +# removed. >> +ovn-nbctl --wait=sb pg-set-ports pg_test lsp2 >> +AT_CHECK([test 1 = $(ovn-sbctl --columns _uuid list Port_Group | grep >> uuid -c)]) >> +AT_CHECK([ovn-sbctl --columns ports --bare find Port_Group >> name=${ls2_key}_pg_test], [0], [dnl >> +lsp2 >> +]) >> + >> +# Remove lsp2 from the port group. All SB Port_Groups should be purged. >> +ovn-nbctl --wait=sb clear Port_Group pg_test ports >> +AT_CHECK([test 0 = $(ovn-sbctl --columns _uuid list Port_Group | grep >> uuid -c)]) >> + >> +AT_CLEANUP >> + >> +AT_SETUP([ovn -- check NB/SB Port_Group translation (ls del)]) >> +ovn_start >> + >> +ovn-nbctl ls-add ls1 >> +ovn-nbctl ls-add ls2 >> +ovn-nbctl lsp-add ls1 lsp1 >> +ovn-nbctl lsp-add ls2 lsp2 >> +ovn-nbct --wait=sb sync >> +ls1_key=$(ovn-sbctl --columns tunnel_key --bare list Datapath_Binding ls1) >> +ls2_key=$(ovn-sbctl --columns tunnel_key --bare list Datapath_Binding ls2) >> + >> +# Add lsp1 & lsp2 to a port group. This should generate two entries in the >> +# SB (one per logical switch). >> +ovn-nbctl --wait=sb pg-add pg_test lsp1 lsp2 >> +AT_CHECK([test 2 = $(ovn-sbctl --columns _uuid list Port_Group | grep >> uuid -c)]) >> +AT_CHECK([ovn-sbctl --columns ports --bare find Port_Group >> name=${ls1_key}_pg_test], [0], [dnl >> +lsp1 >> +]) >> +AT_CHECK([ovn-sbctl --columns ports --bare find Port_Group >> name=${ls2_key}_pg_test], [0], [dnl >> +lsp2 >> +]) >> + >> +# Delete logical switch ls1. This should remove the associated SB >> Port_Group. >> +ovn-nbctl --wait=sb ls-del ls1 >> +AT_CHECK([test 1 = $(ovn-sbctl --columns _uuid list Port_Group | grep >> uuid -c)]) >> +AT_CHECK([ovn-sbctl --columns ports --bare find Port_Group >> name=${ls2_key}_pg_test], [0], [dnl >> +lsp2 >> +]) >> + >> +AT_CLEANUP >> diff --git a/tests/test-ovn.c b/tests/test-ovn.c >> index b43f67f..c3bfd20 100644 >> --- a/tests/test-ovn.c >> +++ b/tests/test-ovn.c >> @@ -238,8 +238,8 @@ create_port_groups(struct shash *port_groups) >> }; >> static const char *const pg2[] = { NULL }; >> >> - expr_const_sets_add(port_groups, "pg1", pg1, 3, false); >> - expr_const_sets_add(port_groups, "pg_empty", pg2, 0, false); >> + expr_const_sets_add(port_groups, "0_pg1", pg1, 3, false); >> + expr_const_sets_add(port_groups, "0_pg_empty", pg2, 0, false); >> } >> >> static bool >> @@ -305,7 +305,7 @@ test_parse_expr__(int steps) >> char *error; >> >> expr = expr_parse_string(ds_cstr(&input), &symtab, &addr_sets, >> - &port_groups, NULL, NULL, &error); >> + &port_groups, NULL, NULL, 0, &error); >> if (!error && steps > 0) { >> expr = expr_annotate(expr, &symtab, &error); >> } >> @@ -431,7 +431,7 @@ test_evaluate_expr(struct ovs_cmdl_context *ctx) >> struct expr *expr; >> >> expr = expr_parse_string(ds_cstr(&input), &symtab, NULL, NULL, >> - NULL, NULL, &error); >> + NULL, NULL, 0, &error); >> if (!error) { >> expr = expr_annotate(expr, &symtab, &error); >> } >> @@ -906,7 +906,7 @@ test_tree_shape_exhaustively(struct expr *expr, struct >> shash *symtab, >> >> char *error; >> modified = expr_parse_string(ds_cstr(&s), symtab, NULL, >> - NULL, NULL, NULL, &error); >> + NULL, NULL, NULL, 0, &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 d7251e7..2666c10 100644 >> --- a/utilities/ovn-trace.c >> +++ b/utilities/ovn-trace.c >> @@ -889,7 +889,8 @@ read_flows(void) >> char *error; >> struct expr *match; >> match = expr_parse_string(sblf->match, &symtab, &address_sets, >> - &port_groups, NULL, NULL, &error); >> + &port_groups, NULL, NULL, >> dp->tunnel_key, >> + &error); >> if (error) { >> VLOG_WARN("%s: parsing expression failed (%s)", >> sblf->match, error); >> -- >> 1.8.3.1 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 6/30/20 2:08 PM, Mark Michelson wrote: > On 6/30/20 4:42 AM, Numan Siddique wrote: >> On Mon, Jun 29, 2020 at 8:55 PM Dumitru Ceara <dceara@redhat.com> wrote: >> >>> In order to avoid ovn-controller reinstalling all logical flows that >>> refer a port_group when some ports are added/removed from the port group >>> we now change the way ovn-northd populates the Southbound DB Port_Group >>> table. >>> >>> Instead of copying NB.Port_Group.name to SB.Port_Group.name we now >>> create one SB.Port_Group record for every datapath that has ports >>> referenced by the NB.Port_Group.ports field. In order to maintain the >>> SB.Port_Group.name uniqueness constraint, ovn-northd populates the field >>> with the value: <SB.Logical_Datapath.tunnel_key>_<NB.Port_Group.name>. >>> >>> In specific scenarios we see significant improvements in time to >>> install/remove all logical flows to/from OVS. One such scenario, in the >>> BZ referenced below has: >>> >>> $ ovn-nbctl acl-list pg >>> from-lport 1001 (inport == @pg && ip) drop >>> to-lport 1001 (outport == @pg && ip) drop >>> >>> Then, incrementally, creates new logical ports on different logical >>> switches, binds them to OVS interfaces and adds them to the port_group. >>> >>> Measuring the total time to perform the above steps 500 times (for 500 >>> new ports attached to 100 switches, 5 per switch) on a test setup >>> we observe an improvement of 50% in time it takes to install all >>> openflow rules when port_groups are split in the SB database. >>> >>> Suggested-by: Numan Siddique <numans@ovn.org> >>> Reported-by: Venkata Anil <anilvenkata@redhat.com> >>> Reported-at: https://bugzilla.redhat.com/1818128 >>> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >>> >>> --- >>> v2: >>> - add tests in ovn-northd.at as suggested by Numan. >>> >> >> Thanks for v2 and adding tests. >> >> Acked-by: Numan Siddique <numans@ovn.org> >> >> Numan > > I pushed this to master and branch-20.06 > Thanks, Numan and Mark, for the reviews. Regards, Dumitru
diff --git a/TODO.rst b/TODO.rst index 6df6b84..4b2fc2d 100644 --- a/TODO.rst +++ b/TODO.rst @@ -140,3 +140,11 @@ OVN To-do List * ovn-controller: Stop copying the local OVS configuration into the Chassis external_ids column (same for the "is-remote" configuration from ovn-ic) a few releases after the 20.06 version (21.06 maybe ?). + +* ovn-controller: Remove backwards compatibility for Southbound DB Port_Group + names in expr.c a few releases after the 20.09 version. Right now + ovn-controller maintains backwards compatibility when connecting to a + SB database that doesn't store Port_Group.name as + <Logical_Datapath.tunnel_key_NB-Port_Group.name>. This causes an additional + hashtable lookup in parse_port_group() which can be avoided when we are sure + that the Southbound DB uses the new format. diff --git a/controller/lflow.c b/controller/lflow.c index c850a0d..5454361 100644 --- a/controller/lflow.c +++ b/controller/lflow.c @@ -566,7 +566,9 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow, struct sset port_groups_ref = SSET_INITIALIZER(&port_groups_ref); expr = expr_parse_string(lflow->match, &symtab, l_ctx_in->addr_sets, l_ctx_in->port_groups, - &addr_sets_ref, &port_groups_ref, &error); + &addr_sets_ref, &port_groups_ref, + lflow->logical_datapath->tunnel_key, + &error); const char *addr_set_name; SSET_FOR_EACH (addr_set_name, &addr_sets_ref) { lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_ADDRSET, addr_set_name, diff --git a/include/ovn/expr.h b/include/ovn/expr.h index 21bf51c..9838251 100644 --- a/include/ovn/expr.h +++ b/include/ovn/expr.h @@ -391,12 +391,14 @@ 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 *port_groups_ref); + struct sset *port_groups_ref, + int64_t dp_id); 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, + int64_t dp_id, char **errorp); struct expr *expr_clone(struct expr *); diff --git a/lib/actions.c b/lib/actions.c index ee7c825..fc6e191 100644 --- a/lib/actions.c +++ b/lib/actions.c @@ -242,7 +242,7 @@ add_prerequisite(struct action_context *ctx, const char *prerequisite) char *error; expr = expr_parse_string(prerequisite, ctx->pp->symtab, NULL, NULL, - NULL, NULL, &error); + NULL, NULL, 0, &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 078d178..497b2ac 100644 --- a/lib/expr.c +++ b/lib/expr.c @@ -29,6 +29,7 @@ #include "simap.h" #include "sset.h" #include "util.h" +#include "ovn-util.h" VLOG_DEFINE_THIS_MODULE(expr); @@ -482,6 +483,10 @@ struct expr_context { const struct shash *port_groups; /* Port group table. */ struct sset *addr_sets_ref; /* The set of address set referenced. */ struct sset *port_groups_ref; /* The set of port groups referenced. */ + int64_t dp_id; /* The tunnel_key of the datapath for + which we're parsing the current + expression. */ + bool not; /* True inside odd number of NOT operators. */ unsigned int paren_depth; /* Depth of nested parentheses. */ }; @@ -783,14 +788,32 @@ static bool parse_port_group(struct expr_context *ctx, struct expr_constant_set *cs, size_t *allocated_values) { + struct ds sb_name = DS_EMPTY_INITIALIZER; + + get_sb_port_group_name(ctx->lexer->token.s, ctx->dp_id, &sb_name); if (ctx->port_groups_ref) { - sset_add(ctx->port_groups_ref, ctx->lexer->token.s); + sset_add(ctx->port_groups_ref, ds_cstr(&sb_name)); + } + + struct expr_constant_set *port_group = NULL; + + if (ctx->port_groups) { + port_group = shash_find_data(ctx->port_groups, ds_cstr(&sb_name)); + if (!port_group) { + /* For backwards compatibility (e.g., ovn-controller was + * upgraded but ovn-northd not yet), perform an additional + * lookup because the NB Port_Group.name might have been + * stored as is in the SB Port_Group.name field. + */ + port_group = shash_find_data(ctx->port_groups, + ctx->lexer->token.s); + if (ctx->port_groups_ref) { + sset_add(ctx->port_groups_ref, ctx->lexer->token.s); + } + } } + ds_destroy(&sb_name); - struct expr_constant_set *port_group - = (ctx->port_groups - ? shash_find_data(ctx->port_groups, ctx->lexer->token.s) - : NULL); if (!port_group) { lexer_syntax_error(ctx->lexer, "expecting port group name"); return false; @@ -1302,14 +1325,16 @@ 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 *port_groups_ref) + struct sset *port_groups_ref, + int64_t dp_id) { struct expr_context ctx = { .lexer = lexer, .symtab = symtab, .addr_sets = addr_sets, .port_groups = port_groups, .addr_sets_ref = addr_sets_ref, - .port_groups_ref = port_groups_ref }; + .port_groups_ref = port_groups_ref, + .dp_id = dp_id }; return lexer->error ? NULL : expr_parse__(&ctx); } @@ -1325,6 +1350,7 @@ expr_parse_string(const char *s, const struct shash *symtab, const struct shash *port_groups, struct sset *addr_sets_ref, struct sset *port_groups_ref, + int64_t dp_id, char **errorp) { struct lexer lexer; @@ -1332,7 +1358,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, port_groups_ref); + addr_sets_ref, port_groups_ref, dp_id); lexer_force_end(&lexer); *errorp = lexer_steal_error(&lexer); if (*errorp) { @@ -1558,7 +1584,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, NULL, NULL, NULL, NULL, + struct expr *expr = expr_parse_string(s, symtab, NULL, NULL, NULL, NULL, 0, errorp); enum expr_level level = expr ? expr_get_level(expr) : EXPR_L_NOMINAL; expr_destroy(expr); @@ -1730,7 +1756,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, NULL, &error); + expr = expr_parse_string(s, symtab, NULL, NULL, NULL, NULL, 0, &error); if (expr) { expr = expr_annotate_(expr, symtab, nesting, &error); } @@ -3456,7 +3482,7 @@ expr_parse_microflow(const char *s, const struct shash *symtab, lexer_get(&lexer); struct expr *e = expr_parse(&lexer, symtab, addr_sets, port_groups, - NULL, NULL); + NULL, NULL, 0); lexer_force_end(&lexer); if (e) { diff --git a/lib/ovn-util.h b/lib/ovn-util.h index eba2948..4e08ee0 100644 --- a/lib/ovn-util.h +++ b/lib/ovn-util.h @@ -122,6 +122,13 @@ get_unique_lport_key(uint64_t dp_tunnel_key, uint64_t lport_tunnel_key, lport_tunnel_key); } +static inline void +get_sb_port_group_name(const char *nb_pg_name, int64_t dp_tunnel_key, + struct ds *sb_pg_name) +{ + ds_put_format(sb_pg_name, "%"PRId64"_%s", dp_tunnel_key, nb_pg_name); +} + char *ovn_chassis_redirect_name(const char *port_name); void ovn_set_pidfile(const char *name); diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index f49e4f2..e1dc491 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -4471,7 +4471,11 @@ build_dhcpv6_action(struct ovn_port *op, struct in6_addr *offer_ip, struct ovn_port_group_ls { struct hmap_node key_node; /* Index on 'key'. */ struct uuid key; /* nb_ls->header_.uuid. */ - const struct nbrec_logical_switch *nb_ls; + struct ovn_datapath *od; + + struct ovn_port **ports; /* Ports in 'od' referrenced by the PG. */ + size_t n_ports; + size_t n_allocated_ports; }; struct ovn_port_group { @@ -4481,14 +4485,14 @@ struct ovn_port_group { struct hmap nb_lswitches; /* NB lswitches related to the port group */ }; -static void -ovn_port_group_ls_add(struct ovn_port_group *pg, - const struct nbrec_logical_switch *nb_ls) +static struct ovn_port_group_ls * +ovn_port_group_ls_add(struct ovn_port_group *pg, struct ovn_datapath *od) { struct ovn_port_group_ls *pg_ls = xzalloc(sizeof *pg_ls); - pg_ls->key = nb_ls->header_.uuid; - pg_ls->nb_ls = nb_ls; + pg_ls->key = od->nbs->header_.uuid; + pg_ls->od = od; hmap_insert(&pg->nb_lswitches, &pg_ls->key_node, uuid_hash(&pg_ls->key)); + return pg_ls; } static struct ovn_port_group_ls * @@ -4505,6 +4509,18 @@ ovn_port_group_ls_find(struct ovn_port_group *pg, const struct uuid *ls_uuid) return NULL; } +static void +ovn_port_group_ls_add_port(struct ovn_port_group_ls *pg_ls, + struct ovn_port *op) +{ + if (pg_ls->n_ports == pg_ls->n_allocated_ports) { + pg_ls->ports = x2nrealloc(pg_ls->ports, + &pg_ls->n_allocated_ports, + sizeof *pg_ls->ports); + } + pg_ls->ports[pg_ls->n_ports++] = op; +} + struct ovn_ls_port_group { struct hmap_node key_node; /* Index on 'key'. */ struct uuid key; /* nb_pg->header_.uuid. */ @@ -5252,6 +5268,7 @@ ovn_port_group_destroy(struct hmap *pgs, struct ovn_port_group *pg) hmap_remove(pgs, &pg->key_node); struct ovn_port_group_ls *ls; HMAP_FOR_EACH_POP (ls, key_node, &pg->nb_lswitches) { + free(ls->ports); free(ls); } hmap_destroy(&pg->nb_lswitches); @@ -5289,9 +5306,10 @@ build_port_group_lswitches(struct northd_context *ctx, struct hmap *pgs, struct ovn_port_group_ls *pg_ls = ovn_port_group_ls_find(pg, &op->od->nbs->header_.uuid); if (!pg_ls) { - ovn_port_group_ls_add(pg, op->od->nbs); + pg_ls = ovn_port_group_ls_add(pg, op->od); ovn_ls_port_group_add(&op->od->nb_pgs, nb_pg); } + ovn_port_group_ls_add_port(pg_ls, op); } } } @@ -10509,7 +10527,7 @@ sync_address_sets(struct northd_context *ctx) * contains lport uuids, while in OVN_Southbound we store the lport names. */ static void -sync_port_groups(struct northd_context *ctx) +sync_port_groups(struct northd_context *ctx, struct hmap *pgs) { struct shash sb_port_groups = SHASH_INITIALIZER(&sb_port_groups); @@ -10518,26 +10536,35 @@ sync_port_groups(struct northd_context *ctx) shash_add(&sb_port_groups, sb_port_group->name, sb_port_group); } - const struct nbrec_port_group *nb_port_group; - NBREC_PORT_GROUP_FOR_EACH (nb_port_group, ctx->ovnnb_idl) { - sb_port_group = shash_find_and_delete(&sb_port_groups, - nb_port_group->name); - if (!sb_port_group) { - sb_port_group = sbrec_port_group_insert(ctx->ovnsb_txn); - sbrec_port_group_set_name(sb_port_group, nb_port_group->name); - } + struct ds sb_name = DS_EMPTY_INITIALIZER; - const char **nb_port_names = xcalloc(nb_port_group->n_ports, - sizeof *nb_port_names); - int i; - for (i = 0; i < nb_port_group->n_ports; i++) { - nb_port_names[i] = nb_port_group->ports[i]->name; + struct ovn_port_group *pg; + HMAP_FOR_EACH (pg, key_node, pgs) { + + struct ovn_port_group_ls *pg_ls; + HMAP_FOR_EACH (pg_ls, key_node, &pg->nb_lswitches) { + ds_clear(&sb_name); + get_sb_port_group_name(pg->nb_pg->name, pg_ls->od->sb->tunnel_key, + &sb_name); + sb_port_group = shash_find_and_delete(&sb_port_groups, + ds_cstr(&sb_name)); + if (!sb_port_group) { + sb_port_group = sbrec_port_group_insert(ctx->ovnsb_txn); + sbrec_port_group_set_name(sb_port_group, ds_cstr(&sb_name)); + } + + const char **nb_port_names = xcalloc(pg_ls->n_ports, + sizeof *nb_port_names); + for (size_t i = 0; i < pg_ls->n_ports; i++) { + nb_port_names[i] = pg_ls->ports[i]->nbsp->name; + } + sbrec_port_group_set_ports(sb_port_group, + nb_port_names, + pg_ls->n_ports); + free(nb_port_names); } - sbrec_port_group_set_ports(sb_port_group, - nb_port_names, - nb_port_group->n_ports); - free(nb_port_names); } + ds_destroy(&sb_name); struct shash_node *node, *next; SHASH_FOR_EACH_SAFE (node, next, &sb_port_groups) { @@ -11140,7 +11167,7 @@ ovnnb_db_run(struct northd_context *ctx, ovn_update_ipv6_prefix(ports); sync_address_sets(ctx); - sync_port_groups(ctx); + sync_port_groups(ctx, &port_groups); sync_meters(ctx); sync_dns_entries(ctx, datapaths); destroy_ovn_lbs(&lbs); diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 80bda0a..d7a940f 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -1472,3 +1472,82 @@ AT_CHECK([ovn-nbctl --wait=sb sync], [0]) AT_CHECK([test 0 = $(ovn-sbctl list Ha_Chassis_Group | wc -l)]) AT_CLEANUP + +AT_SETUP([ovn -- check NB/SB Port_Group translation (lsp add/del)]) +ovn_start + +ovn-nbctl ls-add ls1 +ovn-nbctl ls-add ls2 +ovn-nbctl lsp-add ls1 lsp1 +ovn-nbctl lsp-add ls2 lsp2 +ovn-nbct --wait=sb sync +ls1_key=$(ovn-sbctl --columns tunnel_key --bare list Datapath_Binding ls1) +ls2_key=$(ovn-sbctl --columns tunnel_key --bare list Datapath_Binding ls2) + +# Add an empty port group. This should generate no entry in the SB. +ovn-nbctl --wait=sb pg-add pg_test +AT_CHECK([test 0 = $(ovn-sbctl --columns _uuid list Port_Group | grep uuid -c)]) + +# Add lsp1 to the port group. This should generate an entry in the SB only +# for ls1. +ovn-nbctl --wait=sb pg-set-ports pg_test lsp1 +AT_CHECK([test 1 = $(ovn-sbctl --columns _uuid list Port_Group | grep uuid -c)]) +AT_CHECK([ovn-sbctl --columns ports --bare find Port_Group name=${ls1_key}_pg_test], [0], [dnl +lsp1 +]) + +# Add lsp2 to the port group. This should generate a new entry in the SB, for +# ls2. +ovn-nbctl --wait=sb pg-set-ports pg_test lsp1 lsp2 +AT_CHECK([test 2 = $(ovn-sbctl --columns _uuid list Port_Group | grep uuid -c)]) +AT_CHECK([ovn-sbctl --columns ports --bare find Port_Group name=${ls1_key}_pg_test], [0], [dnl +lsp1 +]) +AT_CHECK([ovn-sbctl --columns ports --bare find Port_Group name=${ls2_key}_pg_test], [0], [dnl +lsp2 +]) + +# Remove lsp1 from the port group. The SB Port_Group for ls1 should be +# removed. +ovn-nbctl --wait=sb pg-set-ports pg_test lsp2 +AT_CHECK([test 1 = $(ovn-sbctl --columns _uuid list Port_Group | grep uuid -c)]) +AT_CHECK([ovn-sbctl --columns ports --bare find Port_Group name=${ls2_key}_pg_test], [0], [dnl +lsp2 +]) + +# Remove lsp2 from the port group. All SB Port_Groups should be purged. +ovn-nbctl --wait=sb clear Port_Group pg_test ports +AT_CHECK([test 0 = $(ovn-sbctl --columns _uuid list Port_Group | grep uuid -c)]) + +AT_CLEANUP + +AT_SETUP([ovn -- check NB/SB Port_Group translation (ls del)]) +ovn_start + +ovn-nbctl ls-add ls1 +ovn-nbctl ls-add ls2 +ovn-nbctl lsp-add ls1 lsp1 +ovn-nbctl lsp-add ls2 lsp2 +ovn-nbct --wait=sb sync +ls1_key=$(ovn-sbctl --columns tunnel_key --bare list Datapath_Binding ls1) +ls2_key=$(ovn-sbctl --columns tunnel_key --bare list Datapath_Binding ls2) + +# Add lsp1 & lsp2 to a port group. This should generate two entries in the +# SB (one per logical switch). +ovn-nbctl --wait=sb pg-add pg_test lsp1 lsp2 +AT_CHECK([test 2 = $(ovn-sbctl --columns _uuid list Port_Group | grep uuid -c)]) +AT_CHECK([ovn-sbctl --columns ports --bare find Port_Group name=${ls1_key}_pg_test], [0], [dnl +lsp1 +]) +AT_CHECK([ovn-sbctl --columns ports --bare find Port_Group name=${ls2_key}_pg_test], [0], [dnl +lsp2 +]) + +# Delete logical switch ls1. This should remove the associated SB Port_Group. +ovn-nbctl --wait=sb ls-del ls1 +AT_CHECK([test 1 = $(ovn-sbctl --columns _uuid list Port_Group | grep uuid -c)]) +AT_CHECK([ovn-sbctl --columns ports --bare find Port_Group name=${ls2_key}_pg_test], [0], [dnl +lsp2 +]) + +AT_CLEANUP diff --git a/tests/test-ovn.c b/tests/test-ovn.c index b43f67f..c3bfd20 100644 --- a/tests/test-ovn.c +++ b/tests/test-ovn.c @@ -238,8 +238,8 @@ create_port_groups(struct shash *port_groups) }; static const char *const pg2[] = { NULL }; - expr_const_sets_add(port_groups, "pg1", pg1, 3, false); - expr_const_sets_add(port_groups, "pg_empty", pg2, 0, false); + expr_const_sets_add(port_groups, "0_pg1", pg1, 3, false); + expr_const_sets_add(port_groups, "0_pg_empty", pg2, 0, false); } static bool @@ -305,7 +305,7 @@ test_parse_expr__(int steps) char *error; expr = expr_parse_string(ds_cstr(&input), &symtab, &addr_sets, - &port_groups, NULL, NULL, &error); + &port_groups, NULL, NULL, 0, &error); if (!error && steps > 0) { expr = expr_annotate(expr, &symtab, &error); } @@ -431,7 +431,7 @@ test_evaluate_expr(struct ovs_cmdl_context *ctx) struct expr *expr; expr = expr_parse_string(ds_cstr(&input), &symtab, NULL, NULL, - NULL, NULL, &error); + NULL, NULL, 0, &error); if (!error) { expr = expr_annotate(expr, &symtab, &error); } @@ -906,7 +906,7 @@ test_tree_shape_exhaustively(struct expr *expr, struct shash *symtab, char *error; modified = expr_parse_string(ds_cstr(&s), symtab, NULL, - NULL, NULL, NULL, &error); + NULL, NULL, NULL, 0, &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 d7251e7..2666c10 100644 --- a/utilities/ovn-trace.c +++ b/utilities/ovn-trace.c @@ -889,7 +889,8 @@ read_flows(void) char *error; struct expr *match; match = expr_parse_string(sblf->match, &symtab, &address_sets, - &port_groups, NULL, NULL, &error); + &port_groups, NULL, NULL, dp->tunnel_key, + &error); if (error) { VLOG_WARN("%s: parsing expression failed (%s)", sblf->match, error);
In order to avoid ovn-controller reinstalling all logical flows that refer a port_group when some ports are added/removed from the port group we now change the way ovn-northd populates the Southbound DB Port_Group table. Instead of copying NB.Port_Group.name to SB.Port_Group.name we now create one SB.Port_Group record for every datapath that has ports referenced by the NB.Port_Group.ports field. In order to maintain the SB.Port_Group.name uniqueness constraint, ovn-northd populates the field with the value: <SB.Logical_Datapath.tunnel_key>_<NB.Port_Group.name>. In specific scenarios we see significant improvements in time to install/remove all logical flows to/from OVS. One such scenario, in the BZ referenced below has: $ ovn-nbctl acl-list pg from-lport 1001 (inport == @pg && ip) drop to-lport 1001 (outport == @pg && ip) drop Then, incrementally, creates new logical ports on different logical switches, binds them to OVS interfaces and adds them to the port_group. Measuring the total time to perform the above steps 500 times (for 500 new ports attached to 100 switches, 5 per switch) on a test setup we observe an improvement of 50% in time it takes to install all openflow rules when port_groups are split in the SB database. Suggested-by: Numan Siddique <numans@ovn.org> Reported-by: Venkata Anil <anilvenkata@redhat.com> Reported-at: https://bugzilla.redhat.com/1818128 Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- v2: - add tests in ovn-northd.at as suggested by Numan. --- TODO.rst | 8 ++++++ controller/lflow.c | 4 ++- include/ovn/expr.h | 4 ++- lib/actions.c | 2 +- lib/expr.c | 48 ++++++++++++++++++++++++------- lib/ovn-util.h | 7 +++++ northd/ovn-northd.c | 79 ++++++++++++++++++++++++++++++++++----------------- tests/ovn-northd.at | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++ tests/test-ovn.c | 10 +++---- utilities/ovn-trace.c | 3 +- 10 files changed, 198 insertions(+), 46 deletions(-)