[ovs-dev] [PATCH ovn] Split SB Port_Group per datapath.

Numan Siddique nusiddiq at redhat.com
Mon Jun 29 07:39:32 UTC 2020


On Fri, Jun 26, 2020 at 6:50 PM Dumitru Ceara <dceara at 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 at ovn.org>
> Reported-by: Venkata Anil <anilvenkata at redhat.com>
> Reported-at: https://bugzilla.redhat.com/1818128
> Signed-off-by: Dumitru Ceara <dceara at redhat.com>
>

Thanks Dumitru for this patch.

Thi patch has turned out to be much simpler than I thought.

Can you please add a few test cases in ovn-northd.at to make sure
that the SB Port_Group rows are created as expected when
a NB Port_Group is created and it is referenced by multiple logical
switches.

If you could cover cases like, delete a logical switch and make sure that
the PG
for that logical switch in SB DB is deleted etc.

Thanks
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/test-ovn.c      | 10 +++----
>  utilities/ovn-trace.c |  3 +-
>  9 files changed, 119 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 6983038..c288607 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -4468,7 +4468,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 {
> @@ -4478,14 +4482,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 *
> @@ -4502,6 +4506,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. */
> @@ -5249,6 +5265,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);
> @@ -5286,9 +5303,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);
>          }
>      }
>  }
> @@ -10506,7 +10524,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);
>
> @@ -10515,26 +10533,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) {
> @@ -11141,7 +11168,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/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 at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>


More information about the dev mailing list