Split SB Port_Group per datapath.

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 <[email protected]> Reported-by: Venkata Anil <[email protected]> Reported-at: https://bugzilla.redhat.com/1818128 Signed-off-by: Dumitru Ceara <[email protected]> Acked-by: Numan Siddique <[email protected]> Signed-off-by: Mark Michelson <[email protected]>
almusil · Jun 30, 2020 · bff01d4 · bff01d4
1 parent 48ada61
commit bff01d4
Show file tree

Hide file tree

Showing 10 changed files with 198 additions and 46 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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,14 +1350,15 @@ 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;
 
     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
@@ -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
@@ -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);