| Message ID | 1478887432-46836-1-git-send-email-dlu998@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, Nov 11, 2016 at 10:03 AM, Darrell Ball <dlu998@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@ovn.org> > CC: Liran Schour <LIRANS@il.ibm.com> > Signed-off-by: Darrell Ball <dlu998@gmail.com> > --- > > v6->v7: Rebase > > v5->v6: Rebase; fix stale handling. > > 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 | 9 +++- > ovn/ovn-sb.xml | 9 ++++ > tests/ovn.at | 8 ++++ > 8 files changed, 220 insertions(+), 8 deletions(-) <snip> > diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn- > controller.c > index 52eb491..2cf6c7e 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) { > + 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; > + } > I assume the checks against get_local_datapath and get_patched_datapath are there in order to prevent addition of a clause for the gateway router on a chassis other than the gateway chassis. The problem with these checks is if you ever want to expand the conditional monitoring to port_binding. You stated above: "This will be re-evaluated later to possibly add additional conditional monitoring such as for logical ports." These checks are not compatible with such an extension. You have to have the port bindings in the first place, in order to add a datapath to local_datapaths and/or patched_datapaths. + > + 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); > + } > + } > +} > + > <snip> diff --git a/ovn/ovn-sb.ovsschema b/ovn/ovn-sb.ovsschema > index 89342fe..7522694 100644 > --- a/ovn/ovn-sb.ovsschema > +++ b/ovn/ovn-sb.ovsschema > @@ -1,7 +1,7 @@ > { > "name": "OVN_Southbound", > "version": "1.9.0", > Don't you need to bump the version? > - "cksum": "239060528 9012", > + "cksum": "3711226361 9319", > "tables": { > "SB_Global": { > "columns": { > @@ -93,7 +93,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 45c473c..4f14066 100644 > --- a/ovn/ovn-sb.xml > +++ b/ovn/ovn-sb.xml > @@ -1541,6 +1541,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 6ae4247..84fb290 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -4193,6 +4193,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]) > This test does not check whether flows were downloaded to the ovn-controller, it just checks whether the flows were programmed in OpenFlow. I have been staring at the code for local_datapaths and patched_datapaths recently. I cannot figure out why line 170 in lflow.c exists. Perhaps before gateway routers all datapaths for logical routers were patched_datapaths. I was thinking of submitting a patch removing line 170 of lflow.c, so that the check for either local_datapaths or patched_datapaths applies to datapaths for logical routers as well as datapaths for logical switches. If that were done, this test would pass even without conditional monitoring. Mickey > + > echo $expected > expected > OVN_CHECK_PACKETS([hv2/vif1-tx.pcap], [expected]) > > -- > 1.9.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
Darrell Ball <dlu998@gmail.com> wrote on 11/11/2016 08:03:52 PM: > 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@ovn.org> > CC: Liran Schour <LIRANS@il.ibm.com> > Signed-off-by: Darrell Ball <dlu998@gmail.com> > --- > I have two comments for this: 1. This patch choose to starts the monitor session by retrieving everything. I prefer to start the monitor session with a false clause and by that start with an empty table. 2. Originally my patch monitored conditionally the following tables: Port_Binding, Logical_Flow, Multicast_Group, MAC_Binding. In the commit message I included the performance impact of that patch. I think that we should evaluate what is the effect of using conditional monitor only for the Logical_Flow table and what will be the benefit of including other tables under conditional monitoring too. Acked-by: Liran Schour <lirans@il.ibm.com> > v6->v7: Rebase > > v5->v6: Rebase; fix stale handling. > > 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 | 9 +++- > ovn/ovn-sb.xml | 9 ++++ > tests/ovn.at | 8 ++++ > 8 files changed, 220 insertions(+), 8 deletions(-) > > diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c > index d7b175e..1fbebe8 100644 > --- a/ovn/controller/binding.c > +++ b/ovn/controller/binding.c > @@ -106,7 +106,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)) { > @@ -118,6 +119,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 > @@ -295,7 +298,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 && qos_map && ctx->ovs_idl_txn) { > get_qos_params(binding_rec, qos_map); > } > @@ -330,7 +333,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; > } > @@ -344,7 +347,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 52eb491..2cf6c7e 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) { > + 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, > @@ -509,6 +595,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); > > @@ -559,6 +651,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 437da9f..b714799 100644 > --- a/ovn/northd/ovn-northd.c > +++ b/ovn/northd/ovn-northd.c > @@ -231,6 +231,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) > { > @@ -376,6 +412,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; > @@ -426,6 +464,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; > @@ -441,6 +480,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); > } > @@ -624,6 +664,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); > } > @@ -1359,6 +1421,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); > @@ -1411,6 +1479,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; > @@ -4825,6 +4899,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 89342fe..7522694 100644 > --- a/ovn/ovn-sb.ovsschema > +++ b/ovn/ovn-sb.ovsschema > @@ -1,7 +1,7 @@ > { > "name": "OVN_Southbound", > "version": "1.9.0", > - "cksum": "239060528 9012", > + "cksum": "3711226361 9319", > "tables": { > "SB_Global": { > "columns": { > @@ -93,7 +93,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 45c473c..4f14066 100644 > --- a/ovn/ovn-sb.xml > +++ b/ovn/ovn-sb.xml > @@ -1541,6 +1541,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 6ae4247..84fb290 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -4193,6 +4193,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 >
On 11/11/16, 7:00 PM, "ovs-dev-bounces@openvswitch.org on behalf of Mickey Spiegel" <ovs-dev-bounces@openvswitch.org on behalf of mickeys.dev@gmail.com> wrote:
On Fri, Nov 11, 2016 at 10:03 AM, Darrell Ball <dlu998@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@ovn.org>
> CC: Liran Schour <LIRANS@il.ibm.com>
> Signed-off-by: Darrell Ball <dlu998@gmail.com>
> ---
>
> v6->v7: Rebase
>
> v5->v6: Rebase; fix stale handling.
>
> 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 | 9 +++-
> ovn/ovn-sb.xml | 9 ++++
> tests/ovn.at | 8 ++++
> 8 files changed, 220 insertions(+), 8 deletions(-)
<snip>
> diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-
> controller.c
> index 52eb491..2cf6c7e 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) {
> + 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;
> + }
>
I assume the checks against get_local_datapath and get_patched_datapath
are there in order to prevent addition of a clause for the gateway router
on a
chassis other than the gateway chassis.
These checks were on one of my todo lists - thanks for mentioning.
The two checks were meant to be defensive, in the context of the previous incremental
processing code base; they don’t have any utility right now; I can remove them and revisit
as needed.
The problem with these checks is if you ever want to expand the
conditional monitoring to port_binding. You stated above: "This will be
re-evaluated later to possibly add additional conditional monitoring such
as for logical ports."
These checks are not compatible with such an extension. You have to
have the port bindings in the first place, in order to add a datapath to
local_datapaths and/or patched_datapaths.
+
> + 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);
> + }
> + }
> +}
> +
>
<snip>
diff --git a/ovn/ovn-sb.ovsschema b/ovn/ovn-sb.ovsschema
> index 89342fe..7522694 100644
> --- a/ovn/ovn-sb.ovsschema
> +++ b/ovn/ovn-sb.ovsschema
> @@ -1,7 +1,7 @@
> {
> "name": "OVN_Southbound",
> "version": "1.9.0",
>
Don't you need to bump the version?
hmm, V5 incremented the version to 1.9.0.
However, version 1.9.0 was claimed by another patch in the interim on Oct. 24 b/w v5 and v6.
> - "cksum": "239060528 9012",
> + "cksum": "3711226361 9319",
> "tables": {
> "SB_Global": {
> "columns": {
> @@ -93,7 +93,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 45c473c..4f14066 100644
> --- a/ovn/ovn-sb.xml
> +++ b/ovn/ovn-sb.xml
> @@ -1541,6 +1541,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 6ae4247..84fb290 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -4193,6 +4193,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])
>
This test does not check whether flows were downloaded to the
ovn-controller, it just checks whether the flows were programmed in
OpenFlow.
I have been staring at the code for local_datapaths and
patched_datapaths recently. I cannot figure out why line 170 in
lflow.c exists. Perhaps before gateway routers all datapaths for
logical routers were patched_datapaths. I was thinking of
submitting a patch removing line 170 of lflow.c, so that the check
for either local_datapaths or patched_datapaths applies to
datapaths for logical routers as well as datapaths for logical
switches. If that were done, this test would pass even without
conditional monitoring.
The test was a summary surrogate test that was quicker to implement to demonstrate
using the existing code base that the patch code works.
However, I am fine to add a more definitive check; besides interpretation benefits, I think we can gain
some added debug benefit by adding a debug point after the ovsdb client update (another location I considered)
but before the translation to OF rules.
Mickey
> +
> echo $expected > expected
> OVN_CHECK_PACKETS([hv2/vif1-tx.pcap], [expected])
>
> --
> 1.9.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=CwICAg&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=dGZmbKhBG9tJHY4odedsGA&m=W7hF3rchizFq6wWh_Q3FoBcDT6O2UYaDccX0fKU-RSY&s=qjsj-H5X-gC3LPtI6vZPKc4cIJM3Vg5hWR5aPoWAGg8&e=
>
_______________________________________________
dev mailing list
dev@openvswitch.org
https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=CwICAg&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=dGZmbKhBG9tJHY4odedsGA&m=W7hF3rchizFq6wWh_Q3FoBcDT6O2UYaDccX0fKU-RSY&s=qjsj-H5X-gC3LPtI6vZPKc4cIJM3Vg5hWR5aPoWAGg8&e=
On 11/17/16, 4:25 AM, "ovs-dev-bounces@openvswitch.org on behalf of Liran Schour" <ovs-dev-bounces@openvswitch.org on behalf of LIRANS@il.ibm.com> wrote:
Darrell Ball <dlu998@gmail.com> wrote on 11/11/2016 08:03:52 PM:
> 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@ovn.org>
> CC: Liran Schour <LIRANS@il.ibm.com>
> Signed-off-by: Darrell Ball <dlu998@gmail.com>
> ---
>
I have two comments for this:
1. This patch choose to starts the monitor session by retrieving
everything. I prefer to start the monitor session with a false clause and
by that start with an empty table.
As we discussed, I also debated this aspect including considering possible future HA scenarios.
Let us go with your recommendation. We can always revisit to if and when the situations arise in the future.
2. Originally my patch monitored conditionally the following tables:
Port_Binding, Logical_Flow, Multicast_Group, MAC_Binding. In the commit
message I included the performance impact of that patch. I think that we
should evaluate what is the effect of using conditional monitor only for
the Logical_Flow table and what will be the benefit of including other
tables under conditional monitoring too.
Based on the proportionate volume of work, we think most of the benefit should come
logical flow conditional monitoring. However, it would be ideal if we could get the
numbers with logical flow conditional monitoring in isolation.
Acked-by: Liran Schour <lirans@il.ibm.com>
> v6->v7: Rebase
>
> v5->v6: Rebase; fix stale handling.
>
> 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 | 9 +++-
> ovn/ovn-sb.xml | 9 ++++
> tests/ovn.at | 8 ++++
> 8 files changed, 220 insertions(+), 8 deletions(-)
>
> diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
> index d7b175e..1fbebe8 100644
> --- a/ovn/controller/binding.c
> +++ b/ovn/controller/binding.c
> @@ -106,7 +106,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)) {
> @@ -118,6 +119,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
> @@ -295,7 +298,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 && qos_map && ctx->ovs_idl_txn) {
> get_qos_params(binding_rec, qos_map);
> }
> @@ -330,7 +333,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;
> }
> @@ -344,7 +347,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 52eb491..2cf6c7e 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) {
> + 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,
> @@ -509,6 +595,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);
>
> @@ -559,6 +651,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 437da9f..b714799 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -231,6 +231,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)
> {
> @@ -376,6 +412,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;
> @@ -426,6 +464,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;
> @@ -441,6 +480,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);
> }
> @@ -624,6 +664,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);
> }
> @@ -1359,6 +1421,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);
> @@ -1411,6 +1479,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;
> @@ -4825,6 +4899,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 89342fe..7522694 100644
> --- a/ovn/ovn-sb.ovsschema
> +++ b/ovn/ovn-sb.ovsschema
> @@ -1,7 +1,7 @@
> {
> "name": "OVN_Southbound",
> "version": "1.9.0",
> - "cksum": "239060528 9012",
> + "cksum": "3711226361 9319",
> "tables": {
> "SB_Global": {
> "columns": {
> @@ -93,7 +93,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 45c473c..4f14066 100644
> --- a/ovn/ovn-sb.xml
> +++ b/ovn/ovn-sb.xml
> @@ -1541,6 +1541,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 6ae4247..84fb290 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -4193,6 +4193,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
https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=CwICAg&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=dGZmbKhBG9tJHY4odedsGA&m=SQ0dsPAD381lgGANv_WScYJ9KqPmTGoDxL3nkBZSIXg&s=0FM9JgWZCI6ILcEwYj0x5q-o_9754EPDOjaOl1dte8k&e=
diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c index d7b175e..1fbebe8 100644 --- a/ovn/controller/binding.c +++ b/ovn/controller/binding.c @@ -106,7 +106,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)) { @@ -118,6 +119,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 @@ -295,7 +298,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 && qos_map && ctx->ovs_idl_txn) { get_qos_params(binding_rec, qos_map); } @@ -330,7 +333,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; } @@ -344,7 +347,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 52eb491..2cf6c7e 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) { + 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, @@ -509,6 +595,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); @@ -559,6 +651,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 437da9f..b714799 100644 --- a/ovn/northd/ovn-northd.c +++ b/ovn/northd/ovn-northd.c @@ -231,6 +231,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) { @@ -376,6 +412,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; @@ -426,6 +464,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; @@ -441,6 +480,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); } @@ -624,6 +664,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); } @@ -1359,6 +1421,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); @@ -1411,6 +1479,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; @@ -4825,6 +4899,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 89342fe..7522694 100644 --- a/ovn/ovn-sb.ovsschema +++ b/ovn/ovn-sb.ovsschema @@ -1,7 +1,7 @@ { "name": "OVN_Southbound", "version": "1.9.0", - "cksum": "239060528 9012", + "cksum": "3711226361 9319", "tables": { "SB_Global": { "columns": { @@ -93,7 +93,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 45c473c..4f14066 100644 --- a/ovn/ovn-sb.xml +++ b/ovn/ovn-sb.xml @@ -1541,6 +1541,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 6ae4247..84fb290 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -4193,6 +4193,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])
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@ovn.org> CC: Liran Schour <LIRANS@il.ibm.com> Signed-off-by: Darrell Ball <dlu998@gmail.com> --- v6->v7: Rebase v5->v6: Rebase; fix stale handling. 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 | 9 +++- ovn/ovn-sb.xml | 9 ++++ tests/ovn.at | 8 ++++ 8 files changed, 220 insertions(+), 8 deletions(-)