[ovs-dev] [patch_v4] ovn: Add datapaths of interest filtering.

Darrell Ball dlu998 at gmail.com
Tue Oct 4 07:49:32 UTC 2016


This patch adds datapaths of interest support where only datapaths of
local interest are monitored by the ovn-controller ovsdb client.  The
idea is to do a flood fill in ovn-controller of datapath associations
calculated by northd. A new column is added to the SB database
datapath_binding table - related_datapaths to facilitate this so all
datapaths associations are known quickly in ovn-controller.  This
allows monitoring to adapt quickly with a single new monitor setting
for all datapaths of interest locally.

To reduce risk and minimize latency on network churn, only logical
flows are conditionally monitored for now.  This should provide
the bulk of the benefit.  This will be re-evaluated later to
possibly add additional conditional monitoring such as for
logical ports.

Liran Schour contributed enhancements to the conditional
monitoring granularity in ovs and also submitted patches
for ovn usage of conditional monitoring which aided this patch
though sharing of concepts through code review work.

Ben Pfaff suggested that northd could be used to pre-populate
related datapaths for ovn-controller to use.  That idea is
used as part of this patch.

CC: Ben Pfaff <blp at ovn.org>
CC: Liran Schour <LIRANS at il.ibm.com>
Signed-off-by: Darrell Ball <dlu998 at gmail.com>
---

v3->v4: Refactor after incremental processing backout.
        Limit filtering to logical flows to limit risk.

v2->v3: Line length violation fixups :/

v1->v2: Added logical port removal monitoring handling, factoring
        in recent changes for incremental processing.  Added some
        comments in the code regarding initial conditions for
        database conditional monitoring.

 ovn/controller/binding.c        | 11 +++--
 ovn/controller/ovn-controller.c | 94 +++++++++++++++++++++++++++++++++++++++++
 ovn/controller/ovn-controller.h |  5 +++
 ovn/controller/patch.c          |  7 ++-
 ovn/northd/ovn-northd.c         | 75 ++++++++++++++++++++++++++++++++
 ovn/ovn-sb.ovsschema            | 11 +++--
 ovn/ovn-sb.xml                  |  9 ++++
 tests/ovn.at                    |  8 ++++
 8 files changed, 211 insertions(+), 9 deletions(-)

diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
index 13c51da..0561b6c 100644
--- a/ovn/controller/binding.c
+++ b/ovn/controller/binding.c
@@ -84,7 +84,8 @@ get_local_iface_ids(const struct ovsrec_bridge *br_int,
 
 static void
 add_local_datapath(struct hmap *local_datapaths,
-        const struct sbrec_port_binding *binding_rec)
+        const struct sbrec_port_binding *binding_rec,
+        const struct controller_ctx *ctx)
 {
     if (get_local_datapath(local_datapaths,
                            binding_rec->datapath->tunnel_key)) {
@@ -96,6 +97,8 @@ add_local_datapath(struct hmap *local_datapaths,
     memcpy(&ld->uuid, &binding_rec->header_.uuid, sizeof ld->uuid);
     hmap_insert(local_datapaths, &ld->hmap_node,
                 binding_rec->datapath->tunnel_key);
+    do_datapath_flood_fill(ctx, binding_rec->datapath,
+                           local_datapaths, NULL);
 }
 
 static void
@@ -127,7 +130,7 @@ consider_local_datapath(struct controller_ctx *ctx,
             /* Add child logical port to the set of all local ports. */
             sset_add(all_lports, binding_rec->logical_port);
         }
-        add_local_datapath(local_datapaths, binding_rec);
+        add_local_datapath(local_datapaths, binding_rec, ctx);
         if (iface_rec && ctx->ovs_idl_txn) {
             update_qos(iface_rec, binding_rec);
         }
@@ -162,7 +165,7 @@ consider_local_datapath(struct controller_ctx *ctx,
         }
 
         sset_add(all_lports, binding_rec->logical_port);
-        add_local_datapath(local_datapaths, binding_rec);
+        add_local_datapath(local_datapaths, binding_rec, ctx);
         if (binding_rec->chassis == chassis_rec) {
             return;
         }
@@ -176,7 +179,7 @@ consider_local_datapath(struct controller_ctx *ctx,
         const char *chassis = smap_get(&binding_rec->options,
                                        "l3gateway-chassis");
         if (!strcmp(chassis, chassis_rec->name) && ctx->ovnsb_idl_txn) {
-            add_local_datapath(local_datapaths, binding_rec);
+            add_local_datapath(local_datapaths, binding_rec, ctx);
         }
     } else if (chassis_rec && binding_rec->chassis == chassis_rec) {
         if (ctx->ovnsb_idl_txn) {
diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index 00392ca..61f821e 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -69,6 +69,13 @@ OVS_NO_RETURN static void usage(void);
 
 static char *ovs_remote;
 
+struct dp_of_interest_node {
+    struct hmap_node hmap_node;
+    const struct sbrec_datapath_binding *sb_db;
+};
+
+static struct hmap dps_of_interest;
+
 struct local_datapath *
 get_local_datapath(const struct hmap *local_datapaths, uint32_t tunnel_key)
 {
@@ -128,6 +135,83 @@ get_bridge(struct ovsdb_idl *ovs_idl, const char *br_name)
     return NULL;
 }
 
+static bool
+add_datapath_of_interest(const struct controller_ctx *ctx,
+                         const struct sbrec_datapath_binding *dp,
+                         const struct hmap *local_datapaths,
+                         const struct hmap *patched_datapaths)
+{
+    struct dp_of_interest_node *node;
+    HMAP_FOR_EACH_WITH_HASH (node, hmap_node, uuid_hash(&dp->header_.uuid),
+                             &dps_of_interest) {
+        if (uuid_equals(&node->sb_db->header_.uuid, &dp->header_.uuid)) {
+            return false;
+        }
+    }
+
+    if (local_datapaths &&
+        !get_local_datapath(local_datapaths, dp->tunnel_key)) {
+        return false;
+    }
+
+    if (patched_datapaths &&
+        !get_patched_datapath(patched_datapaths, dp->tunnel_key)){
+        return false;
+    }
+
+    sbrec_logical_flow_add_clause_logical_datapath(ctx->ovnsb_idl,
+                                                   OVSDB_F_EQ,
+                                                   &dp->header_.uuid);
+    node = xzalloc(sizeof *node);
+    hmap_insert(&dps_of_interest, &node->hmap_node,
+                uuid_hash(&dp->header_.uuid));
+    node->sb_db = dp;
+    return true;
+}
+
+void
+do_datapath_flood_fill(const struct controller_ctx *ctx,
+                       const struct sbrec_datapath_binding *dp_start,
+                       struct hmap *local_datapaths,
+                       struct hmap *patched_datapaths)
+{
+    struct interest_dps_list_elem {
+        struct ovs_list list_node;
+        const struct sbrec_datapath_binding * dp;
+    };
+
+    struct ovs_list interest_datapaths;
+    ovs_list_init(&interest_datapaths);
+
+    struct interest_dps_list_elem *dp_list_elem =
+        xzalloc(sizeof *dp_list_elem);
+    dp_list_elem->dp = dp_start;
+    ovs_list_push_back(&interest_datapaths, &dp_list_elem->list_node);
+
+    while (!ovs_list_is_empty(&interest_datapaths)) {
+
+        struct ovs_list *list_node_ptr =
+            ovs_list_pop_front(&interest_datapaths);
+        dp_list_elem = CONTAINER_OF(list_node_ptr,
+            struct interest_dps_list_elem, list_node);
+
+	size_t num_related_datapaths = dp_list_elem->dp->n_related_datapaths;
+	struct sbrec_datapath_binding **related_datapaths =
+        dp_list_elem->dp->related_datapaths;
+        if (!add_datapath_of_interest(ctx, dp_list_elem->dp,
+                                      local_datapaths, patched_datapaths)) {
+            free(dp_list_elem);
+            continue;
+        }
+        free(dp_list_elem);
+        for (int i = 0; i < num_related_datapaths; i++) {
+            dp_list_elem = xzalloc(sizeof *dp_list_elem);
+            dp_list_elem->dp = related_datapaths[i];
+            ovs_list_push_back(&interest_datapaths, &dp_list_elem->list_node);
+        }
+    }
+}
+
 static const struct ovsrec_bridge *
 create_br_int(struct controller_ctx *ctx,
               const struct ovsrec_open_vswitch *cfg,
@@ -517,6 +601,8 @@ main(int argc, char *argv[])
         struct hmap patched_datapaths = HMAP_INITIALIZER(&patched_datapaths);
         struct sset all_lports = SSET_INITIALIZER(&all_lports);
 
+        hmap_init(&dps_of_interest);
+
         const struct ovsrec_bridge *br_int = get_br_int(&ctx);
         const char *chassis_id = get_chassis_id(ctx.ovs_idl);
 
@@ -585,6 +671,14 @@ main(int argc, char *argv[])
         }
         hmap_destroy(&patched_datapaths);
 
+        struct dp_of_interest_node *dp_cur_node, *dp_next_node;
+        HMAP_FOR_EACH_SAFE (dp_cur_node, dp_next_node, hmap_node,
+                            &dps_of_interest) {
+            hmap_remove(&dps_of_interest, &dp_cur_node->hmap_node);
+            free(dp_cur_node);
+        }
+        hmap_destroy(&dps_of_interest);
+
         unixctl_server_run(unixctl);
 
         unixctl_server_wait(unixctl);
diff --git a/ovn/controller/ovn-controller.h b/ovn/controller/ovn-controller.h
index 4dcf4e5..ed03e79 100644
--- a/ovn/controller/ovn-controller.h
+++ b/ovn/controller/ovn-controller.h
@@ -81,6 +81,11 @@ const struct ovsrec_bridge *get_bridge(struct ovsdb_idl *,
 const struct sbrec_chassis *get_chassis(struct ovsdb_idl *,
                                         const char *chassis_id);
 
+void do_datapath_flood_fill(const struct controller_ctx *,
+                            const struct sbrec_datapath_binding *,
+                            struct hmap *,
+                            struct hmap *);
+
 /* Must be a bit-field ordered from most-preferred (higher number) to
  * least-preferred (lower number). */
 enum chassis_tunnel_type {
diff --git a/ovn/controller/patch.c b/ovn/controller/patch.c
index c9a5dd9..7d8aefd 100644
--- a/ovn/controller/patch.c
+++ b/ovn/controller/patch.c
@@ -252,7 +252,8 @@ add_bridge_mappings(struct controller_ctx *ctx,
 
 static void
 add_patched_datapath(struct hmap *patched_datapaths,
-                     const struct sbrec_port_binding *binding_rec, bool local)
+                     const struct sbrec_port_binding *binding_rec, bool local,
+                     struct controller_ctx *ctx)
 {
     struct patched_datapath *pd = get_patched_datapath(patched_datapaths,
                                        binding_rec->datapath->tunnel_key);
@@ -269,6 +270,8 @@ add_patched_datapath(struct hmap *patched_datapaths,
     /* stale is set to false. */
     hmap_insert(patched_datapaths, &pd->hmap_node,
                 binding_rec->datapath->tunnel_key);
+    do_datapath_flood_fill(ctx, binding_rec->datapath, NULL,
+                           patched_datapaths);
 }
 
 static void
@@ -362,7 +365,7 @@ add_logical_patch_ports(struct controller_ctx *ctx,
                               existing_ports);
             free(dst_name);
             free(src_name);
-            add_patched_datapath(patched_datapaths, binding, local_port);
+            add_patched_datapath(patched_datapaths, binding, local_port, ctx);
             if (local_port) {
                 if (binding->chassis != chassis_rec && ctx->ovnsb_idl_txn) {
                     sbrec_port_binding_set_chassis(binding, chassis_rec);
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 17ea90f..9c9e73d 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -228,6 +228,42 @@ struct tnlid_node {
     uint32_t tnlid;
 };
 
+struct related_datapath_node {
+    struct hmap_node hmap_node;
+    const struct sbrec_datapath_binding *sb_db;
+};
+
+static void
+add_related_datapath(struct hmap *related_datapaths,
+                     const struct sbrec_datapath_binding *sb,
+                     size_t *n_related_datapaths)
+{
+    struct related_datapath_node *node;
+    HMAP_FOR_EACH_WITH_HASH (node, hmap_node,
+                             uuid_hash(&sb->header_.uuid),
+                             related_datapaths) {
+        if (uuid_equals(&sb->header_.uuid, &node->sb_db->header_.uuid)) {
+            return;
+        }
+    }
+
+    node = xzalloc(sizeof *node);
+    hmap_insert(related_datapaths, &node->hmap_node,
+                uuid_hash(&sb->header_.uuid));
+    node->sb_db = sb;
+    (*n_related_datapaths)++;
+}
+
+static void
+destroy_related_datapaths(struct hmap *related_datapaths)
+{
+    struct related_datapath_node *node;
+    HMAP_FOR_EACH_POP (node, hmap_node, related_datapaths) {
+        free(node);
+    }
+    hmap_destroy(related_datapaths);
+}
+
 static void
 destroy_tnlids(struct hmap *tnlids)
 {
@@ -293,6 +329,8 @@ struct ovn_datapath {
     size_t n_router_ports;
 
     struct hmap port_tnlids;
+    struct hmap related_datapaths;
+    size_t n_related_datapaths;
     uint32_t port_key_hint;
 
     bool has_unknown;
@@ -343,6 +381,7 @@ ovn_datapath_create(struct hmap *datapaths, const struct uuid *key,
     od->nbr = nbr;
     hmap_init(&od->port_tnlids);
     hmap_init(&od->ipam);
+    hmap_init(&od->related_datapaths);
     od->port_key_hint = 0;
     hmap_insert(datapaths, &od->key_node, uuid_hash(&od->key));
     return od;
@@ -358,6 +397,7 @@ ovn_datapath_destroy(struct hmap *datapaths, struct ovn_datapath *od)
         hmap_remove(datapaths, &od->key_node);
         destroy_tnlids(&od->port_tnlids);
         destroy_ipam(&od->ipam);
+        destroy_related_datapaths(&od->related_datapaths);
         free(od->router_ports);
         free(od);
     }
@@ -541,6 +581,27 @@ build_datapaths(struct northd_context *ctx, struct hmap *datapaths)
             smap_destroy(&ids);
 
             sbrec_datapath_binding_set_tunnel_key(od->sb, tunnel_key);
+
+            struct sbrec_datapath_binding **sb_related_datapaths
+                = xmalloc(sizeof(*sb_related_datapaths) * od->n_related_datapaths);
+            int rdi = 0;
+            struct related_datapath_node *related_datapath;
+            HMAP_FOR_EACH (related_datapath, hmap_node,
+                           &od->related_datapaths) {
+                if (rdi >= od->n_related_datapaths) {
+                    static struct vlog_rate_limit rl
+                        = VLOG_RATE_LIMIT_INIT(5, 1);
+                    VLOG_ERR_RL(&rl, "related datapaths accounting error"
+                                UUID_FMT, UUID_ARGS(&od->key));
+                    break;
+                }
+                sb_related_datapaths[rdi] = related_datapath->sb_db;
+                rdi++;
+            }
+            sbrec_datapath_binding_set_related_datapaths(od->sb,
+                sb_related_datapaths, od->n_related_datapaths);
+            free(sb_related_datapaths);
+
         }
         destroy_tnlids(&dp_tnlids);
     }
@@ -1254,6 +1315,12 @@ ovn_port_update_sbrec(const struct ovn_port *op)
             sbrec_port_binding_set_type(op->sb, "patch");
         }
 
+        if (op->peer && op->peer->od && op->peer->od->sb) {
+            add_related_datapath(&op->od->related_datapaths,
+                                 op->peer->od->sb,
+                                 &op->od->n_related_datapaths);
+        }
+
         const char *peer = op->peer ? op->peer->key : "<error>";
         struct smap new;
         smap_init(&new);
@@ -1285,6 +1352,12 @@ ovn_port_update_sbrec(const struct ovn_port *op)
                 sbrec_port_binding_set_type(op->sb, "patch");
             }
 
+            if (op->peer && op->peer->od && op->peer->od->sb) {
+                add_related_datapath(&op->od->related_datapaths,
+                                     op->peer->od->sb,
+                                     &op->od->n_related_datapaths);
+            }
+
             const char *router_port = smap_get_def(&op->nbsp->options,
                                                    "router-port", "<error>");
             struct smap new;
@@ -4631,6 +4704,8 @@ main(int argc, char *argv[])
     add_column_noalert(ovnsb_idl_loop.idl,
                        &sbrec_datapath_binding_col_tunnel_key);
     add_column_noalert(ovnsb_idl_loop.idl,
+                       &sbrec_datapath_binding_col_related_datapaths);
+    add_column_noalert(ovnsb_idl_loop.idl,
                        &sbrec_datapath_binding_col_external_ids);
 
     ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_port_binding);
diff --git a/ovn/ovn-sb.ovsschema b/ovn/ovn-sb.ovsschema
index 8604b4e..c48ce6d 100644
--- a/ovn/ovn-sb.ovsschema
+++ b/ovn/ovn-sb.ovsschema
@@ -1,7 +1,7 @@
 {
     "name": "OVN_Southbound",
-    "version": "1.8.0",
-    "cksum": "59582657 7376",
+    "version": "1.9.0",
+    "cksum": "2621057810 7683",
     "tables": {
         "SB_Global": {
             "columns": {
@@ -88,7 +88,12 @@
                                       "maxInteger": 16777215}}},
                 "external_ids": {
                     "type": {"key": "string", "value": "string",
-                             "min": 0, "max": "unlimited"}}},
+                             "min": 0, "max": "unlimited"}},
+                "related_datapaths": {"type": {"key": {"type": "uuid",
+                                      "refTable": "Datapath_Binding",
+                                      "refType": "weak"},
+                                      "min": 0,
+                                      "max": "unlimited"}}},
             "indexes": [["tunnel_key"]],
             "isRoot": true},
         "Port_Binding": {
diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
index 235c21c..309dc19 100644
--- a/ovn/ovn-sb.xml
+++ b/ovn/ovn-sb.xml
@@ -1505,6 +1505,15 @@ tcp.flags = RST;
       constructed for each supported encapsulation.
     </column>
 
+    <column name="related_datapaths">
+      The related_datapaths column is used to keep track of which datapaths
+      are connected to each other.  This is leveraged in ovn-controller to
+      do a flood fill from each local logical switch to determine the full
+      set of datapaths needed on a given ovn-controller.   This set of
+      datapaths can be used to determine which logical ports and logical
+      flows an ovn-controller should monitor.
+    </column>
+
     <group title="OVN_Northbound Relationship">
       <p>
         Each row in <ref table="Datapath_Binding"/> is associated with some
diff --git a/tests/ovn.at b/tests/ovn.at
index e99f77b..a6048d3 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -4157,6 +4157,14 @@ as hv2 ovs-ofctl show br-int
 as hv2 ovs-ofctl dump-flows br-int
 echo "----------------------------"
 
+# Metadata 2 represents the gateway router and the associated
+# flows should only be on hv2 not hv1 when conditional
+# monitoring of flows is being used.
+AT_CHECK([as hv2 ovs-ofctl dump-flows br-int], [0], [stdout])
+AT_CHECK([grep -q -i 'metadata=0x2' stdout], [0], [ignore-nolog])
+AT_CHECK([as hv1 ovs-ofctl dump-flows br-int], [0], [stdout])
+AT_CHECK([grep -q -i 'metadata=0x2' stdout], [1], [ignore-nolog])
+
 echo $expected > expected
 OVN_CHECK_PACKETS([hv2/vif1-tx.pcap], [expected])
 
-- 
1.9.1




More information about the dev mailing list