en_dynamic_routes rebuilds its parsed route map on each run. Move the old map aside and initially mark its entries as deleted. As replacement routes are built, reuse matching objects and cancel their deletions. Record new routes as created. Entries left in the old map remain deleted.
group_ecmp_route applies the created and deleted sets incrementally. A rebuild with no route changes now reports EN_HANDLED_UNCHANGED instead of forcing a downstream recompute. Signed-off-by: Dmitrii Shcherbakov <[email protected]> --- northd/en-advertised-route-sync.c | 96 +++++++++++++++++++++++++++++-- northd/en-advertised-route-sync.h | 17 ++++++ northd/en-group-ecmp-route.c | 57 ++++++++++++++++++ northd/en-group-ecmp-route.h | 4 ++ northd/inc-proc-northd.c | 3 +- northd/northd.c | 2 +- northd/northd.h | 2 + tests/ovn-inc-proc-graph-dump.at | 2 +- tests/ovn-northd.at | 24 ++++++-- 9 files changed, 195 insertions(+), 12 deletions(-) diff --git a/northd/en-advertised-route-sync.c b/northd/en-advertised-route-sync.c index cafb3ca0c..2d38754b1 100644 --- a/northd/en-advertised-route-sync.c +++ b/northd/en-advertised-route-sync.c @@ -231,10 +231,38 @@ add_redistribute_parsed_route(struct hmap *parsed_routes_out, return; } - parsed_route_add(advertising_od, nexthop, &prefix, plen, false, - lrp_addr_s, advertising_op, 0, false, false, false, NULL, - source, false, source_hint, tracked_port, - parsed_routes_out); + size_t n_routes = hmap_count(parsed_routes_out); + struct parsed_route *new_pr = parsed_route_add( + advertising_od, nexthop, &prefix, plen, false, lrp_addr_s, + advertising_op, 0, false, false, false, NULL, source, false, + source_hint, tracked_port, parsed_routes_out); + + /* A duplicate route was already reconciled by an earlier call. */ + if (hmap_count(parsed_routes_out) == n_routes) { + return; + } + + struct dynamic_routes_data *data = CONTAINER_OF( + parsed_routes_out, struct dynamic_routes_data, parsed_routes); + size_t hash = parsed_route_hash(new_pr); + struct parsed_route *old_pr = parsed_route_lookup( + &data->old_parsed_routes, hash, new_pr); + if (!old_pr) { + hmapx_add(&data->trk_data.trk_created_parsed_routes, new_pr); + return; + } + + /* Keep the existing object so pointers held by group_ecmp_route remain + * valid, and cancel the deletion recorded at rebuild start. */ + struct hmapx_node *deleted = hmapx_find( + &data->trk_data.trk_deleted_parsed_routes, old_pr); + ovs_assert(deleted); + hmapx_delete(&data->trk_data.trk_deleted_parsed_routes, deleted); + + hmap_remove(&data->old_parsed_routes, &old_pr->key_node); + hmap_remove(parsed_routes_out, &new_pr->key_node); + hmap_insert(parsed_routes_out, &old_pr->key_node, hash); + parsed_route_free(new_pr); } /* This function adds a new route for each entry in lr_nat record @@ -643,13 +671,34 @@ en_dynamic_routes_init(struct engine_node *node OVS_UNUSED, *data = (struct dynamic_routes_data) { .routes = HMAP_INITIALIZER(&data->routes), .parsed_routes = HMAP_INITIALIZER(&data->parsed_routes), + .old_parsed_routes = HMAP_INITIALIZER(&data->old_parsed_routes), .nb_lr = UUIDSET_INITIALIZER(&data->nb_lr), .nb_ls = UUIDSET_INITIALIZER(&data->nb_ls), + .tracked = false, + .trk_data.trk_created_parsed_routes = + HMAPX_INITIALIZER(&data->trk_data.trk_created_parsed_routes), + .trk_data.trk_deleted_parsed_routes = + HMAPX_INITIALIZER(&data->trk_data.trk_deleted_parsed_routes), }; return data; } +static void +dynamic_routes_clear_tracked(struct dynamic_routes_data *data) +{ + hmapx_clear(&data->trk_data.trk_created_parsed_routes); + struct hmapx_node *hmapx_node; + HMAPX_FOR_EACH_SAFE (hmapx_node, + &data->trk_data.trk_deleted_parsed_routes) { + struct parsed_route *pr = hmapx_node->data; + hmap_remove(&data->old_parsed_routes, &pr->key_node); + parsed_route_free(pr); + hmapx_delete(&data->trk_data.trk_deleted_parsed_routes, hmapx_node); + } + data->tracked = false; +} + static void en_dynamic_routes_clear(struct dynamic_routes_data *data) { @@ -663,6 +712,34 @@ en_dynamic_routes_clear(struct dynamic_routes_data *data) parsed_route_free(pr); } + dynamic_routes_clear_tracked(data); + + uuidset_clear(&data->nb_lr); + uuidset_clear(&data->nb_ls); +} + +/* Mark the current parsed routes as deleted, then move them aside so each + * route can be reconciled as its replacement is calculated. */ +static void +dynamic_routes_prepare_rebuild(struct dynamic_routes_data *data) +{ + dynamic_routes_clear_tracked(data); + + ovs_assert(hmap_is_empty(&data->old_parsed_routes)); + hmap_swap(&data->old_parsed_routes, &data->parsed_routes); + + struct parsed_route *pr; + HMAP_FOR_EACH (pr, key_node, &data->old_parsed_routes) { + hmapx_add(&data->trk_data.trk_deleted_parsed_routes, pr); + } + + /* TODO: Track advertised-route deltas like the parsed-route deltas + * consumed by group_ecmp_route, then add a dynamic_routes change handler + * to advertised_route_sync. */ + struct ar_entry *ar; + HMAP_FOR_EACH_POP (ar, hmap_node, &data->routes) { + ar_entry_free(ar); + } uuidset_clear(&data->nb_lr); uuidset_clear(&data->nb_ls); } @@ -675,6 +752,9 @@ en_dynamic_routes_cleanup(void *data_) en_dynamic_routes_clear(data); hmap_destroy(&data->routes); hmap_destroy(&data->parsed_routes); + hmap_destroy(&data->old_parsed_routes); + hmapx_destroy(&data->trk_data.trk_created_parsed_routes); + hmapx_destroy(&data->trk_data.trk_deleted_parsed_routes); uuidset_destroy(&data->nb_lr); uuidset_destroy(&data->nb_ls); } @@ -687,7 +767,7 @@ en_dynamic_routes_run(struct engine_node *node, void *data) struct ed_type_lr_stateful *lr_stateful_data = engine_get_input_data("lr_stateful", node); - en_dynamic_routes_clear(dynamic_routes_data); + dynamic_routes_prepare_rebuild(dynamic_routes_data); const struct ovn_datapath *od; HMAP_FOR_EACH (od, key_node, &northd_data->lr_datapaths.datapaths) { @@ -720,6 +800,12 @@ en_dynamic_routes_run(struct engine_node *node, void *data) dynamic_routes_data); } + dynamic_routes_data->tracked = + !hmapx_is_empty( + &dynamic_routes_data->trk_data.trk_created_parsed_routes) + || !hmapx_is_empty( + &dynamic_routes_data->trk_data.trk_deleted_parsed_routes); + return EN_UPDATED; } diff --git a/northd/en-advertised-route-sync.h b/northd/en-advertised-route-sync.h index bebdcfddb..e7343072d 100644 --- a/northd/en-advertised-route-sync.h +++ b/northd/en-advertised-route-sync.h @@ -19,6 +19,14 @@ #include "lib/inc-proc-eng.h" #include "lib/uuidset.h" #include "openvswitch/hmap.h" +#include "hmapx.h" + +/* Track what changed in the dynamic_routes engine node's parsed_routes. + * All hmapx node data are pointers to struct parsed_route. */ +struct dynamic_routes_tracked_data { + struct hmapx trk_created_parsed_routes; + struct hmapx trk_deleted_parsed_routes; +}; struct dynamic_routes_data { /* Stores struct ar_entry, one for each dynamic route. Fed only to @@ -32,12 +40,21 @@ struct dynamic_routes_data { * route to. Fed to en_group_ecmp_route alongside en_routes and * en_learned_route_sync. */ struct hmap parsed_routes; + /* Holds the previous parsed routes while parsed_routes is rebuilt. + * Routes that remain here after the rebuild are owned by + * trk_deleted_parsed_routes until its tracked data is cleared. */ + struct hmap old_parsed_routes; /* Contains the uuids of all NB Logical Routers where we used a * lr_stateful_record during computation. */ struct uuidset nb_lr; /* Contains the uuids of all NB Logical Switches where we rely on port * changes for host routes. */ struct uuidset nb_ls; + + /* 'tracked' is set to true if there is information available for + * incremental processing. If true then trk_data is valid. */ + bool tracked; + struct dynamic_routes_tracked_data trk_data; }; void *en_advertised_route_sync_init(struct engine_node *, struct engine_arg *); diff --git a/northd/en-group-ecmp-route.c b/northd/en-group-ecmp-route.c index a1b121c56..aca197318 100644 --- a/northd/en-group-ecmp-route.c +++ b/northd/en-group-ecmp-route.c @@ -24,6 +24,7 @@ #include "en-advertised-route-sync.h" #include "en-group-ecmp-route.h" #include "en-learned-route-sync.h" +#include "hmapx.h" #include "openvswitch/hmap.h" VLOG_DEFINE_THIS_MODULE(en_group_ecmp_route); @@ -600,3 +601,59 @@ group_ecmp_route_routes_change_handler(struct engine_node *eng_node, } return EN_HANDLED_UNCHANGED; } + +/* dynamic_routes rebuilds parsed_routes and diffs it against its previous + * content. tracked=false therefore means that the rebuild produced no + * delta, even when parsed_routes itself is nonempty. */ +enum engine_input_handler_result +group_ecmp_route_dynamic_routes_change_handler(struct engine_node *eng_node, + void *data) +{ + struct group_ecmp_route_data *gdata = data; + struct dynamic_routes_data *dynamic_routes_data + = engine_get_input_data("dynamic_routes", eng_node); + + if (!dynamic_routes_data->tracked) { + return EN_HANDLED_UNCHANGED; + } + + gdata->tracked = true; + + struct hmapx updated_routes = HMAPX_INITIALIZER(&updated_routes); + + const struct hmapx_node *hmapx_node; + const struct parsed_route *pr; + HMAPX_FOR_EACH (hmapx_node, + &dynamic_routes_data->trk_data.trk_deleted_parsed_routes) { + pr = hmapx_node->data; + if (!handle_deleted_route(gdata, pr, &updated_routes)) { + hmapx_destroy(&updated_routes); + return EN_UNHANDLED; + } + } + + HMAPX_FOR_EACH (hmapx_node, + &dynamic_routes_data->trk_data.trk_created_parsed_routes) { + pr = hmapx_node->data; + handle_added_route(gdata, pr, &updated_routes); + } + + HMAPX_FOR_EACH (hmapx_node, &updated_routes) { + struct group_ecmp_datapath *node = hmapx_node->data; + if (hmap_is_empty(&node->unique_routes) && + hmap_is_empty(&node->ecmp_groups)) { + hmapx_add(&gdata->trk_data.deleted_datapath_routes, node); + hmap_remove(&gdata->datapaths, &node->hmap_node); + } else { + hmapx_add(&gdata->trk_data.crupdated_datapath_routes, node); + } + } + + hmapx_destroy(&updated_routes); + + if (!(hmapx_is_empty(&gdata->trk_data.crupdated_datapath_routes) && + hmapx_is_empty(&gdata->trk_data.deleted_datapath_routes))) { + return EN_HANDLED_UPDATED; + } + return EN_HANDLED_UNCHANGED; +} diff --git a/northd/en-group-ecmp-route.h b/northd/en-group-ecmp-route.h index aa6b26015..b40935be2 100644 --- a/northd/en-group-ecmp-route.h +++ b/northd/en-group-ecmp-route.h @@ -102,6 +102,10 @@ enum engine_input_handler_result group_ecmp_route_routes_change_handler(struct engine_node *, void *data); +enum engine_input_handler_result +group_ecmp_route_dynamic_routes_change_handler(struct engine_node *, + void *data); + struct group_ecmp_datapath *group_ecmp_datapath_lookup( const struct group_ecmp_route_data *data, const struct ovn_datapath *od); diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c index 25aa3bcf0..d2f28666c 100644 --- a/northd/inc-proc-northd.c +++ b/northd/inc-proc-northd.c @@ -386,7 +386,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, /* Connected-neighbour redistribute={lb,nat} also emits forwarding * parsed_routes. Consume those to compose ECMP groups alongside * routes and learned_route_sync. */ - engine_add_input(&en_group_ecmp_route, &en_dynamic_routes, NULL); + engine_add_input(&en_group_ecmp_route, &en_dynamic_routes, + group_ecmp_route_dynamic_routes_change_handler); engine_add_input(&en_sync_meters, &en_nb_acl, sync_meters_nb_acl_handler); engine_add_input(&en_sync_meters, &en_nb_meter, NULL); diff --git a/northd/northd.c b/northd/northd.c index 43be834dd..3449a3326 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -12486,7 +12486,7 @@ find_static_route_outport(const struct ovn_datapath *od, /* Parse and validate the route. Return the parsed route if successful. * Otherwise return NULL. */ -static struct parsed_route * +struct parsed_route * parsed_route_lookup(struct hmap *routes, size_t hash, struct parsed_route *new_pr) { diff --git a/northd/northd.h b/northd/northd.h index 0ae1fd607..2e3a9e00d 100644 --- a/northd/northd.h +++ b/northd/northd.h @@ -881,6 +881,8 @@ struct parsed_route { }; struct parsed_route *parsed_route_clone(const struct parsed_route *); +struct parsed_route *parsed_route_lookup(struct hmap *routes, size_t hash, + struct parsed_route *new_pr); struct parsed_route *parsed_route_lookup_by_source( enum route_source source, const struct ovsdb_idl_row *source_hint, const struct hmap *routes); diff --git a/tests/ovn-inc-proc-graph-dump.at b/tests/ovn-inc-proc-graph-dump.at index fc44a98cf..8efb994e1 100644 --- a/tests/ovn-inc-proc-graph-dump.at +++ b/tests/ovn-inc-proc-graph-dump.at @@ -174,7 +174,7 @@ digraph "Incremental-Processing-Engine" { group_ecmp_route [[style=filled, shape=box, fillcolor=white, label="group_ecmp_route"]]; routes -> group_ecmp_route [[label="group_ecmp_route_routes_change_handler"]]; learned_route_sync -> group_ecmp_route [[label="group_ecmp_route_learned_route_change_handler"]]; - dynamic_routes -> group_ecmp_route [[label=""]]; + dynamic_routes -> group_ecmp_route [[label="group_ecmp_route_dynamic_routes_change_handler"]]; ls_stateful [[style=filled, shape=box, fillcolor=white, label="ls_stateful"]]; northd -> ls_stateful [[label="ls_stateful_northd_handler"]]; port_group -> ls_stateful [[label="ls_stateful_port_group_handler"]]; diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index c1f113d56..4b3272817 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -17305,8 +17305,9 @@ check_engine_compute northd incremental check_engine_compute routes incremental check_engine_compute advertised_route_sync recompute check_engine_compute learned_route_sync incremental -check_engine_compute group_ecmp_route recompute -check_engine_compute lflow recompute +# See comment below re group_ecmp_route incremental vs unchanged. +check_engine_compute group_ecmp_route incremental +check_engine_compute lflow incremental CHECK_NO_CHANGE_AFTER_RECOMPUTE check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats @@ -17329,8 +17330,13 @@ check_engine_compute northd incremental check_engine_compute routes incremental check_engine_compute advertised_route_sync recompute check_engine_compute learned_route_sync incremental -check_engine_compute group_ecmp_route recompute -check_engine_compute lflow recompute +# dynamic_routes recomputes on every en_northd update. the handler on +# en_group_ecmp_route returns UNCHANGED when no connected-neighbour +# LB/NAT forwarding routes are produced (this LR uses =connected-as-host, +# not =lb/=nat), but the call itself increments compute_ct so the state +# is "incremental" rather than "unchanged". +check_engine_compute group_ecmp_route incremental +check_engine_compute lflow incremental CHECK_NO_CHANGE_AFTER_RECOMPUTE check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats @@ -18164,6 +18170,16 @@ AT_CHECK([grep 'lr_in_ip_routing.*172.16.1.10/32' lr0_flows | ovn_strip_lflows], table=??(lr_in_ip_routing ), priority=1935 , match=(ip4.dst == 172.16.1.10/32), action=(ip.ttl--; reg8[[0..15]] = 0; reg0 = 10.0.0.1; eth.src = 00:00:00:00:00:01; outport = "lr0-up"; flags.loopback = 1; reg9[[9]] = 1; next;) ]) +# Changing an LB option rebuilds dynamic_routes, but leaves the synthesized +# forwarding route unchanged. group_ecmp_route must consume that empty diff +# incrementally instead of rebuilding its nonempty route map. The reject +# option is arbitrary: any LB option that does not affect VIPs or backends +# would serve the same purpose. +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats +check ovn-nbctl --wait=sb set Load_Balancer lb0 options:reject=true +check_engine_compute dynamic_routes recompute +check_engine_compute group_ecmp_route incremental + # Removing the redistribution option also removes the forwarding route. check ovn-nbctl --wait=sb remove Logical_Router_Port lr0-up options dynamic-routing-redistribute ovn-sbctl lflow-list lr0 > lr0_flows_after_remove -- 2.53.0 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
