I made a correction inline
On Tue, Oct 4, 2016 at 10:04 PM, Darrell Ball <dlu...@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 <b...@ovn.org>
> CC: Liran Schour <lir...@il.ibm.com>
> Signed-off-by: Darrell Ball <dlu...@gmail.com>
> ---
>
> v4->v5: Correct cleanup of monitors.
> Fix warning.
>
> 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 | 103 ++++++++++++++++++++++++++++++
> ++++++++++
> ovn/controller/ovn-controller.h | 5 ++
> ovn/controller/patch.c | 7 ++-
> ovn/northd/ovn-northd.c | 76 +++++++++++++++++++++++++++++
> ovn/ovn-sb.ovsschema | 11 +++--
> ovn/ovn-sb.xml | 9 ++++
> tests/ovn.at | 8 ++++
> 8 files changed, 221 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..02bafa0 100644
> --- a/ovn/controller/ovn-controller.c
> +++ b/ovn/controller/ovn-controller.c
> @@ -69,6 +69,14 @@ 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;
> + bool stale;
> +};
> +
> +static struct hmap dps_of_interest = HMAP_INITIALIZER(&dps_of_interest);
> +
> struct local_datapath *
> get_local_datapath(const struct hmap *local_datapaths, uint32_t
> tunnel_key)
> {
> @@ -128,6 +136,84 @@ 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) {
> + node->stale = false;
> + if (uuid_equals(&node->sb_db->header_.uuid, &dp->header_.uuid)) {
> + return false;
> + }
> + }
Changed to
+ 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)) {
+ node->stale = false;
+ 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 +603,12 @@ main(int argc, char *argv[])
> struct hmap patched_datapaths = HMAP_INITIALIZER(&patched_
> datapaths);
> struct sset all_lports = SSET_INITIALIZER(&all_lports);
>
> + 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) {
> + dp_cur_node->stale = true;
> + }
> +
> const struct ovsrec_bridge *br_int = get_br_int(&ctx);
> const char *chassis_id = get_chassis_id(ctx.ovs_idl);
>
> @@ -565,6 +657,17 @@ main(int argc, char *argv[])
> }
> mcgroup_index_destroy(&mcgroups);
> lport_index_destroy(&lports);
> +
> + HMAP_FOR_EACH_SAFE (dp_cur_node, dp_next_node, hmap_node,
> + &dps_of_interest) {
> + if(dp_cur_node->stale == true) {
> + sbrec_logical_flow_remove_clause_logical_datapath(
> + ctx.ovnsb_idl, OVSDB_F_EQ,
> + &dp_cur_node->sb_db->header_.uuid);
> + hmap_remove(&dps_of_interest,
> &dp_cur_node->hmap_node);
> + free(dp_cur_node);
> + }
> + }
> }
>
> sset_destroy(&all_lports);
> 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 eeeb41d..0c045d6 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,28 @@ 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] = CONST_CAST(
> + struct sbrec_datapath_binding *,
> 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 +1316,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 +1353,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 +4705,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 2b193dd..a6e914f 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -4159,6 +4159,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
>
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
