From: RYAN D. MOATS Date: Tue, 7 Jun 2016 18:52:51 +0000 (-0500) Subject: Convert binding_run to incremental processing. X-Git-Url: http://git.cascardo.eti.br/?p=cascardo%2Fovs.git;a=commitdiff_plain;h=263064aeaa31e758538773fac571dff0cb246cde Convert binding_run to incremental processing. Ensure that the entire port binding table is processed when chassis are added/removed or when get_local_iface_ids finds new ports on the local vswitch. Side effects: - Persist local_datapaths and patch_datapaths across runs so that changes to either can be used as a trigger to reset incremental flow processing. - Persist all_lports structure - Revert commit 9baaabfff3c7df014e9acbd4c68189b568552ca9 (ovn: Fix localnet ports deletion and recreation sometimes after restart.) as these changes are not desirable once local_datatpath is persisted. Signed-off-by: Ryan Moats Signed-off-by: Ben Pfaff --- diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c index a07c3275b..e36c8f4af 100644 --- a/ovn/controller/binding.c +++ b/ovn/controller/binding.c @@ -27,6 +27,16 @@ VLOG_DEFINE_THIS_MODULE(binding); +static struct sset all_lports = SSET_INITIALIZER(&all_lports); + +static bool process_full_binding = false; + +void +binding_reset_processing(void) +{ + process_full_binding = true; +} + void binding_register_ovs_idl(struct ovsdb_idl *ovs_idl) { @@ -72,13 +82,67 @@ get_local_iface_ids(const struct ovsrec_bridge *br_int, struct shash *lports) continue; } shash_add(lports, iface_id, iface_rec); + if (!sset_find(&all_lports, iface_id)) { + sset_add(&all_lports, iface_id); + binding_reset_processing(); + } + } + } +} + +/* Contains "struct local_datpath" nodes whose hash values are the + * row uuids of datapaths with at least one local port binding. */ +static struct hmap local_datapaths_by_uuid = + HMAP_INITIALIZER(&local_datapaths_by_uuid); + +static struct local_datapath * +local_datapath_lookup_by_uuid(struct hmap *hmap_p, const struct uuid *uuid) +{ + struct local_datapath *ld; + HMAP_FOR_EACH_WITH_HASH(ld, uuid_hmap_node, uuid_hash(uuid), hmap_p) { + if (uuid_equals(ld->uuid, uuid)) { + return ld; + } + } + return NULL; +} + +static void +remove_local_datapath(struct hmap *local_datapaths, struct local_datapath *ld) +{ + if (ld->logical_port) { + sset_find_and_delete(&all_lports, ld->logical_port); + free(ld->logical_port); + ld->logical_port = NULL; + } + hmap_remove(local_datapaths, &ld->hmap_node); + hmap_remove(&local_datapaths_by_uuid, &ld->uuid_hmap_node); + free(ld); +} + +static void +remove_local_datapath_by_binding(struct hmap *local_datapaths, + const struct sbrec_port_binding *binding_rec) +{ + const struct uuid *uuid = &binding_rec->header_.uuid; + struct local_datapath *ld = local_datapath_lookup_by_uuid(local_datapaths, + uuid); + if (ld) { + remove_local_datapath(local_datapaths, ld); + } else { + struct local_datapath *ld; + HMAP_FOR_EACH (ld, hmap_node, local_datapaths) { + if (ld->localnet_port == binding_rec) { + ld->localnet_port = NULL; + } } } } 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 uuid *uuid) { if (get_local_datapath(local_datapaths, binding_rec->datapath->tunnel_key)) { @@ -86,8 +150,12 @@ add_local_datapath(struct hmap *local_datapaths, } struct local_datapath *ld = xzalloc(sizeof *ld); + ld->logical_port = xstrdup(binding_rec->logical_port); + ld->uuid = &binding_rec->header_.uuid; hmap_insert(local_datapaths, &ld->hmap_node, binding_rec->datapath->tunnel_key); + hmap_insert(&local_datapaths_by_uuid, &ld->uuid_hmap_node, + uuid_hash(uuid)); } static void @@ -101,15 +169,69 @@ update_qos(const struct ovsrec_interface *iface_rec, ovsrec_interface_set_ingress_policing_burst(iface_rec, MAX(0, burst)); } +static void +consider_local_datapath(struct controller_ctx *ctx, struct shash *lports, + const struct sbrec_chassis *chassis_rec, + const struct sbrec_port_binding *binding_rec, + struct hmap *local_datapaths) +{ + const struct ovsrec_interface *iface_rec + = shash_find_and_delete(lports, binding_rec->logical_port); + if (iface_rec + || (binding_rec->parent_port && binding_rec->parent_port[0] && + sset_contains(&all_lports, binding_rec->parent_port))) { + if (binding_rec->parent_port && binding_rec->parent_port[0]) { + /* 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, + &binding_rec->header_.uuid); + if (iface_rec && ctx->ovs_idl_txn) { + update_qos(iface_rec, binding_rec); + } + if (binding_rec->chassis == chassis_rec) { + return; + } + if (ctx->ovnsb_idl_txn) { + if (binding_rec->chassis) { + VLOG_INFO("Changing chassis for lport %s from %s to %s.", + binding_rec->logical_port, + binding_rec->chassis->name, + chassis_rec->name); + } else { + VLOG_INFO("Claiming lport %s for this chassis.", + binding_rec->logical_port); + } + sbrec_port_binding_set_chassis(binding_rec, chassis_rec); + } + } else if (chassis_rec && binding_rec->chassis == chassis_rec + && strcmp(binding_rec->type, "gateway")) { + if (ctx->ovnsb_idl_txn) { + VLOG_INFO("Releasing lport %s from this chassis.", + binding_rec->logical_port); + sbrec_port_binding_set_chassis(binding_rec, NULL); + } + } else if (!binding_rec->chassis + && !strcmp(binding_rec->type, "localnet")) { + /* Localnet ports will never be bound to a chassis, but we want + * to list them in all_lports because we want to allocate + * a conntrack zone ID for each one, as we'll be creating + * a patch port for each one. */ + sset_add(&all_lports, binding_rec->logical_port); + } +} + void binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, - const char *chassis_id, struct sset *all_lports, - struct hmap *local_datapaths) + const char *chassis_id, struct hmap *local_datapaths) { const struct sbrec_chassis *chassis_rec; const struct sbrec_port_binding *binding_rec; chassis_rec = get_chassis(ctx->ovnsb_idl, chassis_id); + if (!chassis_rec) { + return; + } struct shash lports = SHASH_INITIALIZER(&lports); if (br_int) { @@ -119,60 +241,41 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, * We'll remove our chassis from all port binding records below. */ } - struct shash_node *node; - SHASH_FOR_EACH (node, &lports) { - sset_add(all_lports, node->name); - } - /* Run through each binding record to see if it is resident on this * chassis and update the binding accordingly. This includes both * directly connected logical ports and children of those ports. */ - SBREC_PORT_BINDING_FOR_EACH(binding_rec, ctx->ovnsb_idl) { - const struct ovsrec_interface *iface_rec - = shash_find_and_delete(&lports, binding_rec->logical_port); - if (iface_rec - || (binding_rec->parent_port && binding_rec->parent_port[0] && - sset_contains(all_lports, binding_rec->parent_port))) { - if (binding_rec->parent_port && binding_rec->parent_port[0]) { - /* 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); - if (iface_rec && ctx->ovs_idl_txn) { - update_qos(iface_rec, binding_rec); - } - if (ctx->ovnsb_idl_txn && chassis_rec - && binding_rec->chassis != chassis_rec) { - if (binding_rec->chassis) { - VLOG_INFO("Changing chassis for lport %s from %s to %s.", - binding_rec->logical_port, - binding_rec->chassis->name, - chassis_rec->name); - } else { - VLOG_INFO("Claiming lport %s for this chassis.", - binding_rec->logical_port); - } - sbrec_port_binding_set_chassis(binding_rec, chassis_rec); - } - } else if (chassis_rec && binding_rec->chassis == chassis_rec - && strcmp(binding_rec->type, "gateway")) { - if (ctx->ovnsb_idl_txn) { - VLOG_INFO("Releasing lport %s from this chassis.", - binding_rec->logical_port); - sbrec_port_binding_set_chassis(binding_rec, NULL); + if (process_full_binding) { + struct hmap keep_local_datapath_by_uuid = + HMAP_INITIALIZER(&keep_local_datapath_by_uuid); + SBREC_PORT_BINDING_FOR_EACH(binding_rec, ctx->ovnsb_idl) { + consider_local_datapath(ctx, &lports, chassis_rec, binding_rec, + local_datapaths); + struct local_datapath *ld = xzalloc(sizeof *ld); + ld->uuid = &binding_rec->header_.uuid; + hmap_insert(&keep_local_datapath_by_uuid, &ld->uuid_hmap_node, + uuid_hash(ld->uuid)); + } + struct local_datapath *old_ld, *next; + HMAP_FOR_EACH_SAFE (old_ld, next, hmap_node, local_datapaths) { + if (!local_datapath_lookup_by_uuid(&keep_local_datapath_by_uuid, + old_ld->uuid)) { + remove_local_datapath(local_datapaths, old_ld); } - } else if (!binding_rec->chassis - && !strcmp(binding_rec->type, "localnet")) { - /* localnet ports will never be bound to a chassis, but we want - * to list them in all_lports because we want to allocate - * a conntrack zone ID for each one, as we'll be creating - * a patch port for each one. */ - sset_add(all_lports, binding_rec->logical_port); } - } + hmap_destroy(&keep_local_datapath_by_uuid); + process_full_binding = false; + } else { + SBREC_PORT_BINDING_FOR_EACH_TRACKED(binding_rec, ctx->ovnsb_idl) { + bool is_deleted = sbrec_port_binding_row_get_seqno(binding_rec, + OVSDB_IDL_CHANGE_DELETE) > 0; - SHASH_FOR_EACH (node, &lports) { - VLOG_DBG("No port binding record for lport %s", node->name); + if (is_deleted) { + remove_local_datapath_by_binding(local_datapaths, binding_rec); + continue; + } + consider_local_datapath(ctx, &lports, chassis_rec, binding_rec, + local_datapaths); + } } shash_destroy(&lports); diff --git a/ovn/controller/binding.h b/ovn/controller/binding.h index 25f8989a4..8753d4423 100644 --- a/ovn/controller/binding.h +++ b/ovn/controller/binding.h @@ -27,9 +27,9 @@ struct simap; struct sset; void binding_register_ovs_idl(struct ovsdb_idl *); +void binding_reset_processing(void); void binding_run(struct controller_ctx *, const struct ovsrec_bridge *br_int, - const char *chassis_id, struct sset *all_lports, - struct hmap *local_datapaths); + const char *chassis_id, struct hmap *local_datapaths); bool binding_cleanup(struct controller_ctx *, const char *chassis_id); #endif /* ovn/binding.h */ diff --git a/ovn/controller/encaps.c b/ovn/controller/encaps.c index bd5650af8..495f8f6f2 100644 --- a/ovn/controller/encaps.c +++ b/ovn/controller/encaps.c @@ -15,6 +15,7 @@ #include #include "encaps.h" +#include "binding.h" #include "lflow.h" #include "lib/hash.h" @@ -234,6 +235,7 @@ tunnel_add(const struct sbrec_chassis *chassis_rec, sset_add(&tc.port_names, port_name); free(port_name); free(ports); + binding_reset_processing(); process_full_encaps = true; } @@ -420,6 +422,7 @@ encaps_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, hmap_remove(&tc.tunnel_hmap_by_uuid, &port_hash->uuid_node); free(port_hash); + binding_reset_processing(); } continue; } diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c index 5506d3b4f..8620a7129 100644 --- a/ovn/controller/ovn-controller.c +++ b/ovn/controller/ovn-controller.c @@ -322,6 +322,11 @@ update_ct_zones(struct sset *lports, struct hmap *patched_datapaths, sset_destroy(&all_users); } +/* Contains "struct local_datapath" nodes whose hash values are the + * tunnel_key of datapaths with at least one local port binding. */ +static struct hmap local_datapaths = HMAP_INITIALIZER(&local_datapaths); +static struct hmap patched_datapaths = HMAP_INITIALIZER(&patched_datapaths); + int main(int argc, char *argv[]) { @@ -411,6 +416,7 @@ main(int argc, char *argv[]) free(ovnsb_remote); ovnsb_remote = new_ovnsb_remote; ovsdb_idl_set_remote(ovnsb_idl_loop.idl, ovnsb_remote, true); + binding_reset_processing(); } else { free(new_ovnsb_remote); } @@ -422,11 +428,6 @@ main(int argc, char *argv[]) .ovnsb_idl_txn = ovsdb_idl_loop_run(&ovnsb_idl_loop), }; - /* Contains "struct local_datpath" nodes whose hash values are the - * tunnel_key of datapaths with at least one local port binding. */ - struct hmap local_datapaths = HMAP_INITIALIZER(&local_datapaths); - - struct hmap patched_datapaths = HMAP_INITIALIZER(&patched_datapaths); struct sset all_lports = SSET_INITIALIZER(&all_lports); const struct ovsrec_bridge *br_int = get_br_int(&ctx); @@ -435,8 +436,7 @@ main(int argc, char *argv[]) if (chassis_id) { chassis_run(&ctx, chassis_id); encaps_run(&ctx, br_int, chassis_id); - binding_run(&ctx, br_int, chassis_id, &all_lports, - &local_datapaths); + binding_run(&ctx, br_int, chassis_id, &local_datapaths); } if (br_int && chassis_id) { @@ -470,21 +470,6 @@ main(int argc, char *argv[]) sset_destroy(&all_lports); - struct local_datapath *cur_node, *next_node; - HMAP_FOR_EACH_SAFE (cur_node, next_node, hmap_node, &local_datapaths) { - hmap_remove(&local_datapaths, &cur_node->hmap_node); - free(cur_node); - } - hmap_destroy(&local_datapaths); - - struct patched_datapath *pd_cur_node, *pd_next_node; - HMAP_FOR_EACH_SAFE (pd_cur_node, pd_next_node, hmap_node, - &patched_datapaths) { - hmap_remove(&patched_datapaths, &pd_cur_node->hmap_node); - free(pd_cur_node); - } - hmap_destroy(&patched_datapaths); - unixctl_server_run(unixctl); unixctl_server_wait(unixctl); diff --git a/ovn/controller/ovn-controller.h b/ovn/controller/ovn-controller.h index ba50a98cf..f0c4e6341 100644 --- a/ovn/controller/ovn-controller.h +++ b/ovn/controller/ovn-controller.h @@ -38,6 +38,9 @@ struct controller_ctx { * the localnet port */ struct local_datapath { struct hmap_node hmap_node; + struct hmap_node uuid_hmap_node; + const struct uuid *uuid; + char *logical_port; const struct sbrec_port_binding *localnet_port; }; diff --git a/ovn/controller/patch.c b/ovn/controller/patch.c index 652466bb6..fa4e62428 100644 --- a/ovn/controller/patch.c +++ b/ovn/controller/patch.c @@ -185,7 +185,15 @@ add_bridge_mappings(struct controller_ctx *ctx, * to create patch ports for it. */ continue; } - if (ld->localnet_port) { + + /* Under incremental processing, it is possible to re-enter the + * following block with a logical port that has already been + * recorded in binding->logical_port. Rather than emit spurious + * warnings, add a check to see if the logical port name has + * actually changed. */ + + if (ld->localnet_port && strcmp(ld->localnet_port->logical_port, + binding->logical_port)) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); VLOG_WARN_RL(&rl, "localnet port '%s' already set for datapath " "'%"PRId64"', skipping the new port '%s'.",