From: Lorenzo Bianconi <lorenzo.bianc...@redhat.com> Introduce Incremetal processing logic to datapath-sync node for en_datapath_logical_switch and en_datapath_logical_router inputs.
One test change that may seem unrelated is the propage sb_cfg test in ovn-northd.at. That test checked for a specific sequence of events to happen based on northbound database manipulation. The test had used the insertion of a logical switch to test this. Now, inserting a logical switch causes more than one engine run to occur because of the way en-datapath-sync works. The easiest way to fix the test was to insert a different type of row into the database, so this commit changes to using a port group now. Reported-at: https://issues.redhat.com/browse/FDP-1519 Signed-off-by: Lorenzo Bianconi <lorenzo.bianc...@redhat.com> --- v13 * Rebased. * The previous version of this patch introduced a delay for SB propagation of synced datapaths until we had a permanent pointer to the datapath binding. This no longer happens with this patch. We now pass the synced datapath when the northbound logical datapath is created. We then use a new collection, new_sb, to communicate when the datapath pointer needs to be updated. This allows for ovn-nbctl --wait=sb to work the same as it always has. v12 * Rebased * Use hmapx_is_empty() instead of hmapx_count() to determine if there are unsynced datapaths that need to be handled. * Fix typo where EN_UPDATED was being returned in the case where EN_UNCHANGED should have been returned. * The previous version of this patch changed en_northd to no longer have a handler for en_sb_datapath_binding. This caused a bunch of test failures. This change is now delayed until patch 8 in the series. --- northd/datapath-sync.h | 30 ++++ northd/en-datapath-sync.c | 283 +++++++++++++++++++++++++++++++++++++- northd/en-datapath-sync.h | 7 + northd/en-northd.c | 2 - northd/inc-proc-northd.c | 11 +- northd/northd.h | 1 - tests/ovn-northd.at | 40 ++++++ 7 files changed, 360 insertions(+), 14 deletions(-) diff --git a/northd/datapath-sync.h b/northd/datapath-sync.h index f3725617e..7e9f3e860 100644 --- a/northd/datapath-sync.h +++ b/northd/datapath-sync.h @@ -74,11 +74,41 @@ struct ovn_synced_datapath { struct hmap_node hmap_node; const struct ovsdb_idl_row *nb_row; const struct sbrec_datapath_binding *sb_dp; + /* This boolean indicates if the synced datapath + * has a transient sb_dp pointer. If "true", then + * it means the sb_dp field is the return value of + * sbrec_datapath_binding_insert(). If "false", then + * it means the sb_dp field was provided by an IDL + * update. + * + * This field is only necessary in order to distinguish + * duplicate southbound datapath binding records. If a + * mischievous agent inserts a datapath binding that walks, + * looks, and quacks like an existing datapath binding, + * this is what helps us distinguish that the inserted + * datapath is actually a duplicate of a known datapath. + */ + bool pending_sb_dp; }; struct ovn_synced_datapaths { struct hmap synced_dps; struct hmap dp_tnlids; + + struct hmapx new; + struct hmapx updated; + struct hmapx deleted; + + /* The new_sb hmapx contains synced datapaths whose southbound + * datapath pointer was detected as "new" from the datapath sync + * node. This means that the southbound datapath binding pointer + * has been updated to a new value, but the contents of the southbound + * datapath binding are the same as they had been in the previous + * engine run. Synced datapath binding handlers know to update + * their pointer but not to present the synced datapath as "new" or + * "updated" in this situation. + */ + struct hmapx new_sb; }; /* This is a simple wrapper for a southbound datapath binding. Its purpose diff --git a/northd/en-datapath-sync.c b/northd/en-datapath-sync.c index db84448e0..19b9a3264 100644 --- a/northd/en-datapath-sync.c +++ b/northd/en-datapath-sync.c @@ -36,6 +36,10 @@ en_datapath_sync_init(struct engine_node *node OVS_UNUSED, *synced_datapaths = (struct ovn_synced_datapaths) { .synced_dps = HMAP_INITIALIZER(&synced_datapaths->synced_dps), .dp_tnlids = HMAP_INITIALIZER(&synced_datapaths->dp_tnlids), + .new = HMAPX_INITIALIZER(&synced_datapaths->new), + .new_sb = HMAPX_INITIALIZER(&synced_datapaths->new_sb), + .deleted = HMAPX_INITIALIZER(&synced_datapaths->deleted), + .updated = HMAPX_INITIALIZER(&synced_datapaths->updated), }; return synced_datapaths; @@ -70,6 +74,38 @@ find_unsynced_datapath(const struct ovn_unsynced_datapath_map **maps, return NULL; } +static struct ovn_synced_datapath * +find_synced_datapath_from_udp( + const struct ovn_synced_datapaths *synced_datapaths, + const struct ovn_unsynced_datapath *udp) +{ + struct ovn_synced_datapath *sdp; + uint32_t hash = uuid_hash(&udp->nb_row->uuid); + HMAP_FOR_EACH_WITH_HASH (sdp, hmap_node, hash, + &synced_datapaths->synced_dps) { + if (uuid_equals(&sdp->nb_row->uuid, &udp->nb_row->uuid)) { + return sdp; + } + } + + return NULL; +} + +static struct ovn_synced_datapath * +find_synced_datapath_from_sb(const struct hmap *datapaths, + const struct sbrec_datapath_binding *sb_dp) +{ + struct ovn_synced_datapath *sdp; + uint32_t hash = uuid_hash(sb_dp->nb_uuid); + HMAP_FOR_EACH_WITH_HASH (sdp, hmap_node, hash, datapaths) { + if (uuid_equals(&sdp->nb_row->uuid, sb_dp->nb_uuid)) { + return sdp; + } + } + + return NULL; +} + struct candidate_sdp { struct ovn_synced_datapath *sdp; uint32_t requested_tunnel_key; @@ -79,21 +115,43 @@ struct candidate_sdp { static struct ovn_synced_datapath * synced_datapath_alloc(const struct ovn_unsynced_datapath *udp, - const struct sbrec_datapath_binding *sb_dp) + const struct sbrec_datapath_binding *sb_dp, + bool pending_sb_dp) { struct ovn_synced_datapath *sdp; sdp = xmalloc(sizeof *sdp); *sdp = (struct ovn_synced_datapath) { .sb_dp = sb_dp, .nb_row = udp->nb_row, + .pending_sb_dp = pending_sb_dp }; + return sdp; +} + +static void +synced_datapath_set_sb_fields(const struct sbrec_datapath_binding *sb_dp, + const struct ovn_unsynced_datapath *udp) +{ sbrec_datapath_binding_set_external_ids(sb_dp, &udp->external_ids); sbrec_datapath_binding_set_nb_uuid(sb_dp, &udp->nb_row->uuid, 1); sbrec_datapath_binding_set_type(sb_dp, ovn_datapath_type_to_string(udp->type)); - return sdp; +} + +static void +clear_tracked_data(struct ovn_synced_datapaths *synced_datapaths) +{ + hmapx_clear(&synced_datapaths->new); + hmapx_clear(&synced_datapaths->new_sb); + hmapx_clear(&synced_datapaths->updated); + + struct hmapx_node *node; + HMAPX_FOR_EACH_SAFE (node, &synced_datapaths->deleted) { + free(node->data); + hmapx_delete(&synced_datapaths->deleted, node); + } } static void @@ -104,6 +162,7 @@ reset_synced_datapaths(struct ovn_synced_datapaths *synced_datapaths) free(sdp); } ovn_destroy_tnlids(&synced_datapaths->dp_tnlids); + clear_tracked_data(synced_datapaths); hmap_init(&synced_datapaths->dp_tnlids); } @@ -135,10 +194,11 @@ create_synced_datapath_candidates_from_sb( } struct candidate_sdp candidate = { - .sdp = synced_datapath_alloc(udp, sb_dp), + .sdp = synced_datapath_alloc(udp, sb_dp, false), .requested_tunnel_key = udp->requested_tunnel_key, .existing_tunnel_key = sb_dp->tunnel_key, }; + synced_datapath_set_sb_fields(sb_dp, udp); vector_push(candidate_sdps, &candidate); uuidset_insert(visited, &udp->nb_row->uuid); } @@ -161,10 +221,11 @@ create_synced_datapath_candidates_from_nb( struct sbrec_datapath_binding *sb_dp; sb_dp = sbrec_datapath_binding_insert(ovnsb_idl_txn); struct candidate_sdp candidate = { - .sdp = synced_datapath_alloc(udp, sb_dp), + .sdp = synced_datapath_alloc(udp, sb_dp, true), .requested_tunnel_key = udp->requested_tunnel_key, .existing_tunnel_key = sb_dp->tunnel_key, }; + synced_datapath_set_sb_fields(sb_dp, udp); vector_push(candidate_sdps, &candidate); } } @@ -190,7 +251,8 @@ assign_requested_tunnel_keys(struct vector *candidate_sdps, } sbrec_datapath_binding_set_tunnel_key(candidate->sdp->sb_dp, candidate->requested_tunnel_key); - hmap_insert(&synced_datapaths->synced_dps, &candidate->sdp->hmap_node, + hmap_insert(&synced_datapaths->synced_dps, + &candidate->sdp->hmap_node, uuid_hash(candidate->sdp->sb_dp->nb_uuid)); candidate->tunnel_key_assigned = true; } @@ -239,7 +301,8 @@ allocate_tunnel_keys(struct vector *candidate_sdps, } sbrec_datapath_binding_set_tunnel_key(candidate->sdp->sb_dp, tunnel_key); - hmap_insert(&synced_datapaths->synced_dps, &candidate->sdp->hmap_node, + hmap_insert(&synced_datapaths->synced_dps, + &candidate->sdp->hmap_node, uuid_hash(candidate->sdp->sb_dp->nb_uuid)); candidate->tunnel_key_assigned = true; } @@ -258,6 +321,199 @@ delete_unassigned_candidates(struct vector *candidate_sdps) } } +static enum engine_input_handler_result +datapath_sync_unsynced_datapath_handler( + const struct ovn_unsynced_datapath_map *map, + const struct ed_type_global_config *global_config, + struct ovsdb_idl_txn *ovnsb_idl_txn, void *data) +{ + enum engine_input_handler_result ret = EN_HANDLED_UNCHANGED; + struct ovn_synced_datapaths *synced_datapaths = data; + struct ovn_unsynced_datapath *udp; + struct ovn_synced_datapath *sdp; + + if (hmapx_is_empty(&map->new) && + hmapx_is_empty(&map->deleted) && + hmapx_is_empty(&map->updated)) { + return EN_UNHANDLED; + } + + struct hmapx_node *n; + HMAPX_FOR_EACH (n, &map->deleted) { + udp = n->data; + sdp = find_synced_datapath_from_udp(synced_datapaths, udp); + if (!sdp) { + return EN_UNHANDLED; + } + hmap_remove(&synced_datapaths->synced_dps, &sdp->hmap_node); + hmapx_add(&synced_datapaths->deleted, sdp); + ovn_free_tnlid(&synced_datapaths->dp_tnlids, + sdp->sb_dp->tunnel_key); + sbrec_datapath_binding_delete(sdp->sb_dp); + ret = EN_HANDLED_UPDATED; + } + + HMAPX_FOR_EACH (n, &map->new) { + udp = n->data; + uint32_t tunnel_key; + + if (find_synced_datapath_from_udp(synced_datapaths, udp)) { + return EN_UNHANDLED; + } + + if (udp->requested_tunnel_key) { + tunnel_key = udp->requested_tunnel_key; + if (!ovn_add_tnlid(&synced_datapaths->dp_tnlids, tunnel_key)) { + return EN_UNHANDLED; + } + } else { + uint32_t hint = 0; + tunnel_key = ovn_allocate_tnlid(&synced_datapaths->dp_tnlids, + "datapath", OVN_MIN_DP_KEY_LOCAL, + global_config->max_dp_tunnel_id, + &hint); + if (!tunnel_key) { + return EN_UNHANDLED; + } + } + + struct sbrec_datapath_binding *sb_dp = + sbrec_datapath_binding_insert(ovnsb_idl_txn); + sbrec_datapath_binding_set_tunnel_key(sb_dp, tunnel_key); + sdp = synced_datapath_alloc(udp, sb_dp, true); + synced_datapath_set_sb_fields(sb_dp, udp); + hmap_insert(&synced_datapaths->synced_dps, &sdp->hmap_node, + uuid_hash(sb_dp->nb_uuid)); + hmapx_add(&synced_datapaths->new, sdp); + ret = EN_HANDLED_UPDATED; + } + + HMAPX_FOR_EACH (n, &map->updated) { + udp = n->data; + sdp = find_synced_datapath_from_udp(synced_datapaths, udp); + if (!sdp || !sdp->sb_dp) { + return EN_UNHANDLED; + } + if (udp->requested_tunnel_key && + udp->requested_tunnel_key != sdp->sb_dp->tunnel_key) { + ovn_free_tnlid(&synced_datapaths->dp_tnlids, + sdp->sb_dp->tunnel_key); + if (!ovn_add_tnlid(&synced_datapaths->dp_tnlids, + udp->requested_tunnel_key)) { + return EN_UNHANDLED; + } + sbrec_datapath_binding_set_tunnel_key(sdp->sb_dp, + udp->requested_tunnel_key); + } + if (!smap_equal(&udp->external_ids, &sdp->sb_dp->external_ids)) { + sbrec_datapath_binding_set_external_ids(sdp->sb_dp, + &udp->external_ids); + } + hmapx_add(&synced_datapaths->updated, sdp); + ret = EN_HANDLED_UPDATED; + } + + return ret; +} + +enum engine_input_handler_result +datapath_sync_logical_switch_handler(struct engine_node *node, void *data) +{ + const struct ovn_unsynced_datapath_map *map = + engine_get_input_data("datapath_logical_switch", node); + const struct engine_context *eng_ctx = engine_get_context(); + const struct ed_type_global_config *global_config = + engine_get_input_data("global_config", node); + + return datapath_sync_unsynced_datapath_handler(map, global_config, + eng_ctx->ovnsb_idl_txn, + data); +} + +enum engine_input_handler_result +datapath_sync_logical_router_handler(struct engine_node *node, void *data) +{ + const struct ovn_unsynced_datapath_map *map = + engine_get_input_data("datapath_logical_router", node); + const struct engine_context *eng_ctx = engine_get_context(); + const struct ed_type_global_config *global_config = + engine_get_input_data("global_config", node); + + return datapath_sync_unsynced_datapath_handler(map, global_config, + eng_ctx->ovnsb_idl_txn, + data); +} + +enum engine_input_handler_result +datapath_sync_sb_datapath_binding(struct engine_node *node, void *data) +{ + const struct sbrec_datapath_binding_table *sb_dp_table = + EN_OVSDB_GET(engine_get_input("SB_datapath_binding", node)); + enum engine_input_handler_result ret = EN_HANDLED_UNCHANGED; + struct ovn_synced_datapaths *synced_datapaths = data; + + const struct sbrec_datapath_binding *sb_dp; + SBREC_DATAPATH_BINDING_TABLE_FOR_EACH_TRACKED (sb_dp, sb_dp_table) { + struct ovn_synced_datapath *sdp = + find_synced_datapath_from_sb(&synced_datapaths->synced_dps, sb_dp); + if (sbrec_datapath_binding_is_deleted(sb_dp)) { + if (sdp) { + /* The SB datapath binding was deleted, but we still have a + * record of it locally. This implies the SB datapath binding + * was deleted by something other than ovn-northd. We need + * to recompute in this case. + */ + return EN_UNHANDLED; + } + continue; + } + + if (sbrec_datapath_binding_is_new(sb_dp)) { + if (!sdp) { + /* There is a new datapath binding, but we have no record + * of the synced datapath. This indicates that + * something other than ovn-northd added this datapath + * binding to the database, and we need to recompute. + */ + return EN_UNHANDLED; + } else { + if (sdp->pending_sb_dp) { + /* Update the existing synced datapath pointer to the safer + * version to cache. + */ + sdp->sb_dp = sb_dp; + sdp->pending_sb_dp = false; + hmapx_add(&synced_datapaths->new_sb, sdp); + ret = EN_HANDLED_UPDATED; + } else { + /* Some jerk has inserted a duplicate datapath to try to + * fool us. Not on my watch! + */ + return EN_UNHANDLED; + } + } + continue; + } + + /* Datapath binding is updated. This happens if the northbound + * logical datapath is updated and we make changes to the existing + * southbound datapath binding. In this case, the handler for the + * unsynced datapath will add sdp to synced_datapaths->updated, so + * we don't need to do anything here. + */ + } + + return ret; +} + +void +en_datapath_sync_clear_tracked_data(void *data) +{ + struct ovn_synced_datapaths *synced_datapaths = data; + + clear_tracked_data(synced_datapaths); +} + enum engine_node_state en_datapath_sync_run(struct engine_node *node , void *data) { @@ -304,7 +560,11 @@ en_datapath_sync_run(struct engine_node *node , void *data) delete_unassigned_candidates(&candidate_sdps); vector_destroy(&candidate_sdps); - return EN_UPDATED; + if (hmap_count(&synced_datapaths->synced_dps)) { + return EN_UPDATED; + } else { + return EN_UNCHANGED; + } } void en_datapath_sync_cleanup(void *data) @@ -312,6 +572,15 @@ void en_datapath_sync_cleanup(void *data) struct ovn_synced_datapaths *synced_datapaths = data; struct ovn_synced_datapath *sdp; + hmapx_destroy(&synced_datapaths->new); + hmapx_destroy(&synced_datapaths->new_sb); + hmapx_destroy(&synced_datapaths->updated); + struct hmapx_node *node; + HMAPX_FOR_EACH_SAFE (node, &synced_datapaths->deleted) { + free(node->data); + } + hmapx_destroy(&synced_datapaths->deleted); + HMAP_FOR_EACH_POP (sdp, hmap_node, &synced_datapaths->synced_dps) { free(sdp); } diff --git a/northd/en-datapath-sync.h b/northd/en-datapath-sync.h index 3b3262304..bed2d5a7b 100644 --- a/northd/en-datapath-sync.h +++ b/northd/en-datapath-sync.h @@ -21,7 +21,14 @@ void *en_datapath_sync_init(struct engine_node *, struct engine_arg *); +enum engine_input_handler_result +datapath_sync_logical_switch_handler(struct engine_node *, void *data); +enum engine_input_handler_result +datapath_sync_logical_router_handler(struct engine_node *, void *data); +enum engine_input_handler_result +datapath_sync_sb_datapath_binding(struct engine_node *, void *data); enum engine_node_state en_datapath_sync_run(struct engine_node *, void *data); void en_datapath_sync_cleanup(void *data); +void en_datapath_sync_clear_tracked_data(void *data); #endif /* EN_DATAPATH_SYNC_H */ diff --git a/northd/en-northd.c b/northd/en-northd.c index b991d3a1c..b2986edf1 100644 --- a/northd/en-northd.c +++ b/northd/en-northd.c @@ -81,8 +81,6 @@ northd_get_input_data(struct engine_node *node, input_data->nbrec_port_group_table = EN_OVSDB_GET(engine_get_input("NB_port_group", node)); - input_data->sbrec_datapath_binding_table = - EN_OVSDB_GET(engine_get_input("SB_datapath_binding", node)); input_data->sbrec_port_binding_table = EN_OVSDB_GET(engine_get_input("SB_port_binding", node)); input_data->sbrec_mac_binding_table = diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c index 0dfbf00b5..243c0c0c0 100644 --- a/northd/inc-proc-northd.c +++ b/northd/inc-proc-northd.c @@ -184,7 +184,7 @@ static ENGINE_NODE(dynamic_routes); static ENGINE_NODE(group_ecmp_route, CLEAR_TRACKED_DATA); static ENGINE_NODE(datapath_logical_router, CLEAR_TRACKED_DATA); static ENGINE_NODE(datapath_logical_switch, CLEAR_TRACKED_DATA); -static ENGINE_NODE(datapath_sync); +static ENGINE_NODE(datapath_sync, CLEAR_TRACKED_DATA); static ENGINE_NODE(datapath_synced_logical_router); static ENGINE_NODE(datapath_synced_logical_switch); @@ -225,10 +225,13 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, engine_add_input(&en_datapath_logical_router, &en_nb_logical_router, en_datapath_logical_router_logical_router_handler); - engine_add_input(&en_datapath_sync, &en_datapath_logical_switch, NULL); - engine_add_input(&en_datapath_sync, &en_datapath_logical_router, NULL); - engine_add_input(&en_datapath_sync, &en_sb_datapath_binding, NULL); engine_add_input(&en_datapath_sync, &en_global_config, NULL); + engine_add_input(&en_datapath_sync, &en_sb_datapath_binding, + datapath_sync_sb_datapath_binding); + engine_add_input(&en_datapath_sync, &en_datapath_logical_switch, + datapath_sync_logical_switch_handler); + engine_add_input(&en_datapath_sync, &en_datapath_logical_router, + datapath_sync_logical_router_handler); engine_add_input(&en_datapath_synced_logical_router, &en_datapath_sync, NULL); diff --git a/northd/northd.h b/northd/northd.h index e223a54da..3541a9160 100644 --- a/northd/northd.h +++ b/northd/northd.h @@ -42,7 +42,6 @@ struct northd_input { const struct nbrec_port_group_table *nbrec_port_group_table; /* Southbound table references */ - const struct sbrec_datapath_binding_table *sbrec_datapath_binding_table; const struct sbrec_port_binding_table *sbrec_port_binding_table; const struct sbrec_mac_binding_table *sbrec_mac_binding_table; const struct sbrec_ha_chassis_group_table *sbrec_ha_chassis_group_table; diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 8620c5382..c919735cc 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -17822,3 +17822,43 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE AT_CLEANUP ]) +OVN_FOR_EACH_NORTHD_NO_HV([ +AT_SETUP([Datapath Sync incremental processing]) +ovn_start + +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats +check ovn-nbctl --wait=sb ls-add sw0 +check_engine_stats datapath_sync norecompute compute +CHECK_NO_CHANGE_AFTER_RECOMPUTE + +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats +check ovn-nbctl --wait=sb set logical_switch sw0 other_config:fdb_age_threshold=5 +check_engine_stats datapath_sync norecompute compute +CHECK_NO_CHANGE_AFTER_RECOMPUTE + +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats +check ovn-nbctl --wait=sb set logical_switch sw0 other_config:requested-tnl-key=123 +check_engine_stats datapath_sync norecompute compute +CHECK_NO_CHANGE_AFTER_RECOMPUTE + +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats +check ovn-nbctl --wait=sb ls-del sw0 +check_engine_stats datapath_sync norecompute compute +CHECK_NO_CHANGE_AFTER_RECOMPUTE + +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats +check ovn-nbctl --wait=sb lr-add lr0 +check_engine_stats datapath_sync norecompute compute +CHECK_NO_CHANGE_AFTER_RECOMPUTE + +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats +check ovn-nbctl set Logical_Router lr0 options:ct-zone-limit=10 +check_engine_stats datapath_sync norecompute compute + +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats +check ovn-nbctl --wait=sb lr-del lr0 +check_engine_stats datapath_sync norecompute compute +CHECK_NO_CHANGE_AFTER_RECOMPUTE + +AT_CLEANUP +]) -- 2.49.0 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
