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

Darrell Ball dlu998 at gmail.com
Tue Oct 4 16:19:26 UTC 2016


This patch does not include cleanup of stale datapaths
of interest, with this post incremental processing code base.
I will add the cleanup today.

On Tue, Oct 4, 2016 at 12:49 AM, Darrell Ball <dlu998 at gmail.com> wrote:

> 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