Hi, Thanks for your review. Em sex., 10 de abr. de 2026 às 05:29, Ales Musil <[email protected]> escreveu:
> > > On Tue, Mar 10, 2026 at 6:36 PM Lucas Vargas Dias via dev < > [email protected]> wrote: > >> Create a handler for deleted and updated static routes, >> and change the handler from engine route which check if >> it has a new static route created. >> Test with 2000 static routes created in the same logical router >> and add a new one: >> Without the incremental processing: >> ovn-nbctl --print-wait-time --wait=sb lr-route-add lr1-2 10.0.0.1/32 >> 192.168.20.2 >> Time spent on processing nb_cfg 4: >> ovn-northd delay before processing: 4ms >> ovn-northd completion: 62ms >> >> With the incremental processing: >> ovn-nbctl --print-wait-time --wait=sb lr-route-add lr1-2 10.0.0.1/32 >> 192.168.20.2 >> Time spent on processing nb_cfg 6: >> ovn-northd delay before processing: 4ms >> ovn-northd completion: 21ms >> >> Test with 2000 static routes created in the same logical router >> and delete one: >> Without the incremental processing: >> ovn-nbctl --print-wait-time --wait=sb lr-route-del lr1-2 10.0.0.1/32 >> 192.168.20.2 >> Time spent on processing nb_cfg 5: >> ovn-northd delay before processing: 3ms >> ovn-northd completion: 62ms >> >> With the incremental processing: >> ovn-nbctl --print-wait-time --wait=sb lr-route-del lr1-2 10.0.0.1/32 >> 192.168.20.2 >> Time spent on processing nb_cfg 9: >> ovn-northd delay before processing: 2ms >> ovn-northd completion: 32ms >> >> Signed-off-by: Lucas Vargas Dias <[email protected]> >> --- >> > > Hi Lucas, > > sorry for the review delay, I have a couple of comments down below. > > >> northd/en-group-ecmp-route.c | 55 ++++++++++++ >> northd/en-group-ecmp-route.h | 4 + >> northd/en-lflow.c | 20 +++++ >> northd/en-lflow.h | 2 + >> northd/en-northd.c | 144 +++++++++++++++++++++++++++---- >> northd/en-northd.h | 5 +- >> northd/inc-proc-northd.c | 13 ++- >> northd/northd.c | 106 +++++++++++++++++------ >> northd/northd.h | 37 +++++++- >> tests/ovn-inc-proc-graph-dump.at | 6 +- >> tests/ovn-northd.at | 18 ++-- >> 11 files changed, 349 insertions(+), 61 deletions(-) >> >> diff --git a/northd/en-group-ecmp-route.c b/northd/en-group-ecmp-route.c >> index cd4cdb991..0405fb5d8 100644 >> --- a/northd/en-group-ecmp-route.c >> +++ b/northd/en-group-ecmp-route.c >> @@ -510,3 +510,58 @@ group_ecmp_route_learned_route_change_handler(struct >> engine_node *eng_node, >> } >> return EN_HANDLED_UNCHANGED; >> } >> + >> +enum engine_input_handler_result >> +group_ecmp_static_route_change_handler(struct engine_node *eng_node, >> + void *_data) >> > > nit: Wrong indentation. > > >> +{ >> + struct routes_data *routes_data >> + = engine_get_input_data("routes", eng_node); >> + struct group_ecmp_route_data *data = _data; >> + if (!routes_data->tracked) { >> + data->tracked = false; >> + return EN_UNHANDLED; >> + } >> + >> + struct parsed_route *pr; >> + struct hmapx updated_routes = HMAPX_INITIALIZER(&updated_routes); >> + >> + const struct hmapx_node *hmapx_node; >> + HMAPX_FOR_EACH (hmapx_node, >> + &routes_data->trk_data.trk_deleted_parsed_route) { >> + pr = hmapx_node->data; >> + if (!handle_deleted_route(data, pr, &updated_routes)) { >> + hmapx_destroy(&updated_routes); >> + return EN_UNHANDLED; >> + } >> + hmap_remove(&routes_data->parsed_routes, &pr->key_node); >> + parsed_route_free(pr); >> + } >> + >> + HMAPX_FOR_EACH (hmapx_node, >> + &routes_data->trk_data.trk_crupdated_parsed_route) { >> + pr = hmapx_node->data; >> + handle_added_route(data, pr, &updated_routes); >> + } >> + >> > > nit: Extra line. > > >> + >> + 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(&data->trk_data.deleted_datapath_routes, node); >> + hmap_remove(&data->datapaths, &node->hmap_node); >> + } else { >> + hmapx_add(&data->trk_data.crupdated_datapath_routes, node); >> + } >> + } >> + >> + hmapx_destroy(&updated_routes); >> + >> + if (!(hmapx_is_empty(&data->trk_data.crupdated_datapath_routes) && >> + hmapx_is_empty(&data->trk_data.deleted_datapath_routes))) { >> + data->tracked = true; >> + 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 784004347..874c8ddd7 100644 >> --- a/northd/en-group-ecmp-route.h >> +++ b/northd/en-group-ecmp-route.h >> @@ -97,6 +97,10 @@ enum engine_input_handler_result >> group_ecmp_route_learned_route_change_handler(struct engine_node *, >> void *data); >> >> +enum engine_input_handler_result >> +group_ecmp_static_route_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/en-lflow.c b/northd/en-lflow.c >> index d4351edb9..22cd8fe91 100644 >> --- a/northd/en-lflow.c >> +++ b/northd/en-lflow.c >> @@ -297,6 +297,21 @@ lflow_multicast_igmp_handler(struct engine_node >> *node, void *data) >> return EN_HANDLED_UPDATED; >> } >> >> +enum engine_input_handler_result >> +lflow_group_route_change_handler(struct engine_node *node, >> + void *data OVS_UNUSED) >> > > nit: Wrong indentation. > > >> +{ >> + struct routes_data *route_data = >> + engine_get_input_data("routes", node); >> + >> + /* If we do not have tracked data we need to recompute. */ >> + if (!route_data->tracked) { >> + return EN_UNHANDLED; >> + } >> + >> + return EN_HANDLED_UNCHANGED; >> +} >> + >> enum engine_input_handler_result >> lflow_group_ecmp_route_change_handler(struct engine_node *node, >> void *data OVS_UNUSED) >> @@ -346,6 +361,11 @@ lflow_group_ecmp_route_change_handler(struct >> engine_node *node, >> route_node->od, lflow_data->lflow_table, >> route_node, lflow_input.bfd_ports); >> >> + build_arp_request_flows_for_lrouter(route_node->od, >> + lflow_data->lflow_table, >> + lflow_input.meter_groups, >> + route_node->lflow_ref); >> + >> bool handled = lflow_ref_sync_lflows( >> route_node->lflow_ref, lflow_data->lflow_table, >> eng_ctx->ovnsb_idl_txn, lflow_input.dps, >> diff --git a/northd/en-lflow.h b/northd/en-lflow.h >> index d2a92e49f..aa320615f 100644 >> --- a/northd/en-lflow.h >> +++ b/northd/en-lflow.h >> @@ -31,5 +31,7 @@ lflow_multicast_igmp_handler(struct engine_node *node, >> void *data); >> enum engine_input_handler_result >> lflow_group_ecmp_route_change_handler(struct engine_node *node, void >> *data); >> enum engine_input_handler_result >> +lflow_group_route_change_handler(struct engine_node *node, void *data); >> +enum engine_input_handler_result >> lflow_ic_learned_svc_mons_handler(struct engine_node *node, void *data); >> #endif /* EN_LFLOW_H */ >> diff --git a/northd/en-northd.c b/northd/en-northd.c >> index a828f9a5f..af8e8effd 100644 >> --- a/northd/en-northd.c >> +++ b/northd/en-northd.c >> @@ -203,7 +203,8 @@ northd_nb_logical_router_handler(struct engine_node >> *node, >> } >> >> if (northd_has_lr_nats_in_tracked_data(&nd->trk_data) || >> - northd_has_lrouters_in_tracked_data(&nd->trk_data)) { >> + northd_has_lrouters_in_tracked_data(&nd->trk_data) || >> + northd_has_lr_route_in_tracked_data(&nd->trk_data)) { >> return EN_HANDLED_UPDATED; >> } >> >> @@ -325,30 +326,130 @@ en_route_policies_run(struct engine_node *node, >> void *data) >> >> enum engine_input_handler_result >> routes_northd_change_handler(struct engine_node *node, >> - void *data OVS_UNUSED) >> + void *data) >> { >> struct northd_data *northd_data = engine_get_input_data("northd", >> node); >> if (!northd_has_tracked_data(&northd_data->trk_data)) { >> return EN_UNHANDLED; >> } >> >> - /* This node uses the below data from the en_northd engine node. >> - * See (lr_stateful_get_input_data()) >> - * 1. northd_data->lr_datapaths >> - * 2. northd_data->lr_ports >> - * This data gets updated when a logical router or logical >> router port >> - * is created or deleted. >> - * Northd engine node presently falls back to full recompute >> when >> - * this happens and so does this node. >> - * Note: When we add I-P to the created/deleted logical routers >> or >> - * logical router ports, we need to revisit this handler. >> - * >> - * This node also accesses the static routes of the logical >> router. >> - * When these static routes gets updated, en_northd engine >> recomputes >> - * and so does this node. >> - * Note: When we add I-P to handle static routes changes, we >> need >> - * to revisit this handler. >> - */ >> + if (!northd_has_lr_route_in_tracked_data(&northd_data->trk_data)) { >> + return EN_HANDLED_UNCHANGED; >> + } >> + >> + struct bfd_data *bfd_data = engine_get_input_data("bfd", node); >> + struct routes_data *routes_data = data; >> + struct hmapx_node *hmapx_node; >> + struct ovn_datapath *od; >> + HMAPX_FOR_EACH (hmapx_node, &northd_data->trk_data.trk_lrs_routes) { >> + od = hmapx_node->data; >> + struct parsed_route *pr; >> + >> + for (int i = 0; i < od->nbr->n_static_routes; i++) { >> + struct nbrec_logical_router_static_route *static_route = >> + od->nbr->static_routes[i]; >> + if (nbrec_logical_router_static_route_is_new(static_route)) { >> > > This can only be used in the context of tracked changes. We should do the > parsed_routes lookup instead. > > >> + pr = parsed_routes_add_static( >> + od, &northd_data->lr_ports, >> + static_route, >> + &bfd_data->bfd_connections, >> + &routes_data->parsed_routes, >> + &routes_data->route_tables, >> + >> &routes_data->bfd_active_connections); >> + if (!pr) { >> + continue; >> + } >> + pr->stale = false; >> + >> + >> hmapx_add(&routes_data->trk_data.trk_crupdated_parsed_route, >> + pr); >> + } >> + } >> + } >> + >> + if >> (!hmapx_is_empty(&routes_data->trk_data.trk_crupdated_parsed_route)) { >> + routes_data->tracked = true; >> + return EN_HANDLED_UPDATED; >> + } >> + >> + if >> (hmapx_is_empty(&routes_data->trk_data.trk_crupdated_parsed_route) && >> + hmapx_is_empty(&routes_data->trk_data.trk_deleted_parsed_route)) >> { >> > > The other handler is the only one that populated the deleted routes. > This is order-dependent; if this handler runs first, it will always be > empty. Which would lead to a full recompute in that case there isn't > any new static route added here, is that really correct? > Do you think it's more appropriate to check just trk_crupdated_parsed_route here? I didn't find a easy to handle the created routes together to updated or deleted route in the same handler. Maybe it makes sense to remove the check for trk_deleted_parsed_route here. > > >> + return EN_UNHANDLED; >> + } >> + >> + return EN_HANDLED_UNCHANGED; >> +} >> +enum engine_input_handler_result >> +routes_static_route_change_handler(struct engine_node *node, >> + void *data) >> +{ >> + struct routes_data *routes_data = data; >> + >> + const struct nbrec_logical_router_static_route_table * >> + nb_lr_static_route_table = >> + EN_OVSDB_GET(engine_get_input("NB_logical_router_static_route", >> node)); >> + >> + struct northd_data *northd_data = engine_get_input_data("northd", >> node); >> + struct bfd_data *bfd_data = engine_get_input_data("bfd", node); >> + >> + const struct nbrec_logical_router_static_route *changed_static_route; >> + NBREC_LOGICAL_ROUTER_STATIC_ROUTE_TABLE_FOR_EACH ( >> > > This should be FOR_EACH_TRACKED. > I agree > > >> + changed_static_route, >> nb_lr_static_route_table) { >> + >> + if >> (nbrec_logical_router_static_route_is_new(changed_static_route)) { >> + continue; >> + } >> > > We should check is_deleted first as the row can be both new and deleted. > I agree > > + bool is_deleted = nbrec_logical_router_static_route_is_deleted( >> + >> changed_static_route); >> + if (is_deleted) { >> + struct parsed_route *pr = parsed_route_lookup_by_source( >> + ROUTE_SOURCE_STATIC, >> + >> &changed_static_route->header_, >> + &routes_data->parsed_routes); >> + if (pr) { >> + pr->stale = true; >> + } >> + hmapx_add(&routes_data->trk_data.trk_deleted_parsed_route, >> pr); >> > > Shouldn't the hmapx_add be inside the if (pr) to prevent adding a NULL? > > >> + continue; >> + } >> + if (nbrec_logical_router_static_route_is_updated( >> + changed_static_route, >> + NBREC_LOGICAL_ROUTER_STATIC_ROUTE_COL_NEXTHOP) >> + || nbrec_logical_router_static_route_is_updated( >> + changed_static_route, >> + NBREC_LOGICAL_ROUTER_STATIC_ROUTE_COL_OUTPUT_PORT) >> + || nbrec_logical_router_static_route_is_updated( >> + changed_static_route, >> + NBREC_LOGICAL_ROUTER_STATIC_ROUTE_COL_POLICY) >> + || nbrec_logical_router_static_route_is_updated( >> + changed_static_route, >> + NBREC_LOGICAL_ROUTER_STATIC_ROUTE_COL_ROUTE_TABLE) >> + || nbrec_logical_router_static_route_is_updated( >> + changed_static_route, >> + >> NBREC_LOGICAL_ROUTER_STATIC_ROUTE_COL_SELECTION_FIELDS)) { >> > > What about the other columns? For example, "ip_prefix" wouldn't trigger > the I-P at all. > I forgot ip_prefix here > > >> + struct parsed_route *pr = parsed_route_lookup_by_source( >> + ROUTE_SOURCE_STATIC, >> + >> &changed_static_route->header_, >> + &routes_data->parsed_routes); >> + if (!pr || !pr->od || !northd_data || !bfd_data) { >> + continue; >> + } >> + parsed_routes_add_static(pr->od, &northd_data->lr_ports, >> + changed_static_route, >> + &bfd_data->bfd_connections, >> + &routes_data->parsed_routes, >> + &routes_data->route_tables, >> + >> &routes_data->bfd_active_connections); >> + hmapx_add(&routes_data->trk_data.trk_crupdated_parsed_route, >> + pr); >> > > I'm slightly confused by this part, so we will try to find a route by > source, > if it returns something we insert a new one. The new one might have > a different nexthop, so the inner lookup won't find the old. The old is now > stale. The new one is never added to the crupdated so it's never handled > by the ecmp, while the old one is? > You're right, the logic is wrong. This part must be: Search pr, if it exists, remove it from route_tables and add a new one parsed route with the values updated. > > >> + } >> + } >> + if >> (!hmapx_is_empty(&routes_data->trk_data.trk_crupdated_parsed_route) || >> + >> !hmapx_is_empty(&routes_data->trk_data.trk_deleted_parsed_route)) { >> + routes_data->tracked = true; >> + return EN_HANDLED_UPDATED; >> + } >> + >> return EN_HANDLED_UNCHANGED; >> } >> >> @@ -586,6 +687,11 @@ en_routes_cleanup(void *data) >> routes_destroy(data); >> } >> >> +void >> +en_routes_clear_tracked_data(void *data) >> +{ >> + routes_clear_tracked(data); >> +} >> void >> en_bfd_cleanup(void *data) >> { >> diff --git a/northd/en-northd.h b/northd/en-northd.h >> index 7794739b9..5247f3e11 100644 >> --- a/northd/en-northd.h >> +++ b/northd/en-northd.h >> @@ -39,9 +39,12 @@ enum engine_node_state en_route_policies_run(struct >> engine_node *node, >> void *data); >> void *en_route_policies_init(struct engine_node *node OVS_UNUSED, >> struct engine_arg *arg OVS_UNUSED); >> +void en_routes_clear_tracked_data(void *data); >> void en_routes_cleanup(void *data); >> enum engine_input_handler_result >> -routes_northd_change_handler(struct engine_node *node, void *data >> OVS_UNUSED); >> +routes_northd_change_handler(struct engine_node *node, void *data); >> +enum engine_input_handler_result >> +routes_static_route_change_handler(struct engine_node *node, void *data); >> enum engine_node_state en_routes_run(struct engine_node *node, void >> *data); >> void *en_bfd_init(struct engine_node *node OVS_UNUSED, >> struct engine_arg *arg OVS_UNUSED); >> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c >> index 10bad4bab..7d03dd7ef 100644 >> --- a/northd/inc-proc-northd.c >> +++ b/northd/inc-proc-northd.c >> @@ -76,7 +76,9 @@ static unixctl_cb_func chassis_features_list; >> NB_NODE(sampling_app) \ >> NB_NODE(network_function) \ >> NB_NODE(network_function_group) \ >> - NB_NODE(logical_switch_port_health_check) >> + NB_NODE(logical_switch_port_health_check) \ >> + NB_NODE(logical_router_static_route) >> + >> >> enum nb_engine_node { >> #define NB_NODE(NAME) NB_##NAME, >> @@ -178,7 +180,7 @@ static ENGINE_NODE(lr_stateful, CLEAR_TRACKED_DATA); >> static ENGINE_NODE(ls_stateful, CLEAR_TRACKED_DATA); >> static ENGINE_NODE(ls_arp, CLEAR_TRACKED_DATA); >> static ENGINE_NODE(route_policies); >> -static ENGINE_NODE(routes); >> +static ENGINE_NODE(routes, CLEAR_TRACKED_DATA); >> static ENGINE_NODE(bfd); >> static ENGINE_NODE(bfd_sync, SB_WRITE); >> static ENGINE_NODE(ecmp_nexthop, SB_WRITE); >> @@ -335,6 +337,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, >> engine_add_input(&en_routes, &en_bfd, NULL); >> engine_add_input(&en_routes, &en_northd, >> routes_northd_change_handler); >> + engine_add_input(&en_routes, &en_nb_logical_router_static_route, >> + routes_static_route_change_handler); >> >> engine_add_input(&en_bfd_sync, &en_bfd, NULL); >> engine_add_input(&en_bfd_sync, &en_nb_bfd, NULL); >> @@ -374,7 +378,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, >> engine_add_input(&en_learned_route_sync, &en_northd, >> learned_route_sync_northd_change_handler); >> >> - engine_add_input(&en_group_ecmp_route, &en_routes, NULL); >> + engine_add_input(&en_group_ecmp_route, &en_routes, >> + group_ecmp_static_route_change_handler); >> engine_add_input(&en_group_ecmp_route, &en_learned_route_sync, >> group_ecmp_route_learned_route_change_handler); >> >> @@ -393,7 +398,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, >> engine_add_input(&en_lflow, &en_sb_logical_dp_group, NULL); >> engine_add_input(&en_lflow, &en_bfd_sync, NULL); >> engine_add_input(&en_lflow, &en_route_policies, NULL); >> - engine_add_input(&en_lflow, &en_routes, NULL); >> + engine_add_input(&en_lflow, &en_routes, >> lflow_group_route_change_handler); >> /* XXX: The incremental processing only supports changes to learned >> routes. >> * All other changes trigger a full recompute. */ >> engine_add_input(&en_lflow, &en_group_ecmp_route, >> diff --git a/northd/northd.c b/northd/northd.c >> index 05702bb49..602b84fc6 100644 >> --- a/northd/northd.c >> +++ b/northd/northd.c >> @@ -4411,6 +4411,7 @@ destroy_northd_data_tracked_changes(struct >> northd_data *nd) >> destroy_tracked_ovn_ports(&trk_changes->trk_lsps); >> destroy_tracked_lbs(&trk_changes->trk_lbs); >> hmapx_clear(&trk_changes->trk_nat_lrs); >> + hmapx_clear(&trk_changes->trk_lrs_routes); >> hmapx_clear(&trk_changes->ls_with_changed_lbs); >> hmapx_clear(&trk_changes->ls_with_changed_acls); >> hmapx_clear(&trk_changes->ls_with_changed_ipam); >> @@ -4434,6 +4435,7 @@ init_northd_tracked_data(struct northd_data *nd) >> hmapx_init(&trk_data->trk_lbs.crupdated); >> hmapx_init(&trk_data->trk_lbs.deleted); >> hmapx_init(&trk_data->trk_nat_lrs); >> + hmapx_init(&trk_data->trk_lrs_routes); >> hmapx_init(&trk_data->ls_with_changed_lbs); >> hmapx_init(&trk_data->ls_with_changed_acls); >> hmapx_init(&trk_data->ls_with_changed_ipam); >> @@ -4452,6 +4454,7 @@ destroy_northd_tracked_data(struct northd_data *nd) >> hmapx_destroy(&trk_data->trk_lbs.crupdated); >> hmapx_destroy(&trk_data->trk_lbs.deleted); >> hmapx_destroy(&trk_data->trk_nat_lrs); >> + hmapx_destroy(&trk_data->trk_lrs_routes); >> hmapx_destroy(&trk_data->ls_with_changed_lbs); >> hmapx_destroy(&trk_data->ls_with_changed_acls); >> hmapx_destroy(&trk_data->ls_with_changed_ipam); >> @@ -5257,7 +5260,8 @@ lr_changes_can_be_handled(const struct >> nbrec_logical_router *lr) >> if (nbrec_logical_router_is_updated(lr, col)) { >> if (col == NBREC_LOGICAL_ROUTER_COL_LOAD_BALANCER >> || col == NBREC_LOGICAL_ROUTER_COL_LOAD_BALANCER_GROUP >> - || col == NBREC_LOGICAL_ROUTER_COL_NAT) { >> + || col == NBREC_LOGICAL_ROUTER_COL_NAT >> + || col == NBREC_LOGICAL_ROUTER_COL_STATIC_ROUTES) { >> continue; >> } >> return false; >> @@ -5313,6 +5317,12 @@ is_lr_nats_changed(const struct >> nbrec_logical_router *nbr) { >> || is_lr_nats_seqno_changed(nbr)); >> } >> >> +static bool >> +is_lr_static_routes_changed(const struct nbrec_logical_router *nbr) { >> + return nbrec_logical_router_is_updated(nbr, >> + >> NBREC_LOGICAL_ROUTER_COL_STATIC_ROUTES); >> +} >> + >> /* Return true if changes are handled incrementally, false otherwise. >> * >> * Note: Changes to load balancer and load balancer groups associated >> with >> @@ -5380,6 +5390,19 @@ northd_handle_lr_changes(const struct northd_input >> *ni, >> } >> >> hmapx_add(&nd->trk_data.trk_nat_lrs, od); >> + } else if (is_lr_static_routes_changed(changed_lr)) { >> > > Can't both thing happen at the same time? This way we will > skip the routes if there is a nat. Also this duplicates a lot of code > we should consider making the lookup common for both. > I think it can happen. I'll adjust. > > >> + struct ovn_datapath *od = ovn_datapath_find_( >> + &nd->lr_datapaths.datapaths, >> + &changed_lr->header_.uuid); >> + >> + if (!od) { >> + static struct vlog_rate_limit rl = >> VLOG_RATE_LIMIT_INIT(1, 1); >> + VLOG_WARN_RL(&rl, "Internal error: a tracked updated LR " >> + "doesn't exist in lr_datapaths: "UUID_FMT, >> + UUID_ARGS(&changed_lr->header_.uuid)); >> + goto fail; >> + } >> + hmapx_add(&nd->trk_data.trk_lrs_routes, od); >> } >> } >> >> @@ -5421,6 +5444,9 @@ northd_handle_lr_changes(const struct northd_input >> *ni, >> if (!hmapx_is_empty(&nd->trk_data.trk_nat_lrs)) { >> nd->trk_data.type |= NORTHD_TRACKED_LR_NATS; >> } >> + if (!hmapx_is_empty(&nd->trk_data.trk_lrs_routes)) { >> + nd->trk_data.type |= NORTHD_TRACKED_LR_ROUTES; >> + } >> if (!hmapx_is_empty(&nd->trk_data.trk_routers.crupdated) || >> !hmapx_is_empty(&nd->trk_data.trk_routers.deleted)) { >> nd->trk_data.type |= NORTHD_TRACKED_ROUTERS; >> @@ -11926,8 +11952,17 @@ parsed_route_lookup(struct hmap *routes, size_t >> hash, >> continue; >> } >> >> - if (pr->nexthop && ipv6_addr_equals(pr->nexthop, >> new_pr->nexthop)) { >> - continue; >> + if (pr->nexthop) { >> + if (IN6_IS_ADDR_V4MAPPED(pr->nexthop)) { >> + if (in6_addr_get_mapped_ipv4(pr->nexthop) != >> + in6_addr_get_mapped_ipv4(new_pr->nexthop)) { >> + continue; >> + } >> + } else { >> + if (!ipv6_addr_equals(pr->nexthop, new_pr->nexthop)) { >> + continue; >> + } >> + } >> > > This logic was originally wrong, but not in the sense of matching > the mapped address, it should have been with > (!ipv6_addr_equals(...)). Also this a bug fix of the original logic. > Let's make it a separate patch with proper Fixes tag please. > > Looking again, I think it's not necessary to change this. I'll check. > } >> >> if (memcmp(&pr->prefix, &new_pr->prefix, sizeof(struct >> in6_addr))) { >> @@ -12109,7 +12144,7 @@ parsed_route_add(const struct ovn_datapath *od, >> } >> } >> >> -static void >> +struct parsed_route * >> parsed_routes_add_static(const struct ovn_datapath *od, >> const struct hmap *lr_ports, >> const struct nbrec_logical_router_static_route >> *route, >> @@ -12130,8 +12165,9 @@ parsed_routes_add_static(const struct >> ovn_datapath *od, >> UUID_FMT, route->nexthop, >> UUID_ARGS(&route->header_.uuid)); >> free(nexthop); >> - return; >> + return NULL; >> } >> + >> if ((IN6_IS_ADDR_V4MAPPED(nexthop) && plen != 32) || >> (!IN6_IS_ADDR_V4MAPPED(nexthop) && plen != 128)) { >> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, >> 1); >> @@ -12139,7 +12175,7 @@ parsed_routes_add_static(const struct >> ovn_datapath *od, >> UUID_FMT, route->nexthop, >> UUID_ARGS(&route->header_.uuid)); >> free(nexthop); >> - return; >> + return NULL; >> } >> } >> >> @@ -12151,7 +12187,7 @@ parsed_routes_add_static(const struct >> ovn_datapath *od, >> UUID_FMT, route->ip_prefix, >> UUID_ARGS(&route->header_.uuid)); >> free(nexthop); >> - return; >> + return NULL; >> } >> >> /* Verify that ip_prefix and nexthop are on the same network. */ >> @@ -12162,7 +12198,7 @@ parsed_routes_add_static(const struct >> ovn_datapath *od, >> IN6_IS_ADDR_V4MAPPED(&prefix), >> &lrp_addr_s, &out_port)) { >> free(nexthop); >> - return; >> + return NULL; >> } >> >> const struct nbrec_bfd *nb_bt = route->bfd; >> @@ -12172,7 +12208,7 @@ parsed_routes_add_static(const struct >> ovn_datapath *od, >> nb_bt->dst_ip); >> if (!bfd_e) { >> free(nexthop); >> - return; >> + return NULL; >> } >> >> /* This static route is linked to an active bfd session. */ >> @@ -12191,8 +12227,8 @@ parsed_routes_add_static(const struct >> ovn_datapath *od, >> >> >> if (!strcmp(bfd_sr->status, "down")) { >> - free(nexthop); >> - return; >> + free(nexthop); >> + return NULL; >> } >> } >> >> @@ -12229,11 +12265,15 @@ parsed_routes_add_static(const struct >> ovn_datapath *od, >> source = ROUTE_SOURCE_STATIC; >> } >> >> - parsed_route_add(od, nexthop, &prefix, plen, is_discard_route, >> lrp_addr_s, >> - out_port, route_table_id, is_src_route, >> - ecmp_symmetric_reply, &ecmp_selection_fields, >> source, >> - &route->header_, NULL, routes); >> + struct parsed_route *pr = parsed_route_add(od, nexthop, &prefix, >> plen, >> + is_discard_route, >> lrp_addr_s, >> + out_port, route_table_id, >> + is_src_route, >> + ecmp_symmetric_reply, >> + &ecmp_selection_fields, >> source, >> + &route->header_, NULL, >> routes); >> sset_destroy(&ecmp_selection_fields); >> + return pr; >> } >> >> static void >> @@ -16097,13 +16137,14 @@ build_lr_gateway_redirect_flows_for_nats( >> * In the common case where the Ethernet destination has been resolved, >> * this table outputs the packet (priority 0). Otherwise, it composes >> * and sends an ARP/IPv6 NA request (priority 100). */ >> -static void >> +void >> build_arp_request_flows_for_lrouter( >> - struct ovn_datapath *od, struct lflow_table *lflows, >> - struct ds *match, struct ds *actions, >> + const struct ovn_datapath *od, struct lflow_table *lflows, >> const struct shash *meter_groups, >> struct lflow_ref *lflow_ref) >> { >> + struct ds match = DS_EMPTY_INITIALIZER; >> + struct ds actions = DS_EMPTY_INITIALIZER; >> ovs_assert(od->nbr); >> for (int i = 0; i < od->nbr->n_static_routes; i++) { >> const struct nbrec_logical_router_static_route *route; >> @@ -16117,8 +16158,8 @@ build_arp_request_flows_for_lrouter( >> continue; >> } >> >> - ds_clear(match); >> - ds_put_format(match, "eth.dst == 00:00:00:00:00:00 && " >> + ds_clear(&match); >> + ds_put_format(&match, "eth.dst == 00:00:00:00:00:00 && " >> REGBIT_NEXTHOP_IS_IPV4" == 0 && " >> REG_NEXT_HOP_IPV6 " == %s", >> route->nexthop); >> @@ -16130,8 +16171,8 @@ build_arp_request_flows_for_lrouter( >> char sn_addr_s[INET6_ADDRSTRLEN + 1]; >> ipv6_string_mapped(sn_addr_s, &sn_addr); >> >> - ds_clear(actions); >> - ds_put_format(actions, >> + ds_clear(&actions); >> + ds_put_format(&actions, >> "nd_ns { " >> "eth.dst = "ETH_ADDR_FMT"; " >> "ip6.dst = %s; " >> @@ -16141,7 +16182,7 @@ build_arp_request_flows_for_lrouter( >> route->nexthop); >> >> ovn_lflow_add(lflows, od, S_ROUTER_IN_ARP_REQUEST, 200, >> - ds_cstr(match), ds_cstr(actions), lflow_ref, >> + ds_cstr(&match), ds_cstr(&actions), lflow_ref, >> WITH_CTRL_METER(copp_meter_get(COPP_ND_NS_RESOLVE, >> od->nbr->copp, >> meter_groups)), >> @@ -16173,6 +16214,8 @@ build_arp_request_flows_for_lrouter( >> >> meter_groups))); >> ovn_lflow_add(lflows, od, S_ROUTER_IN_ARP_REQUEST, 0, "1", "next;", >> lflow_ref); >> + ds_destroy(&match); >> + ds_destroy(&actions); >> } >> >> static void >> @@ -19296,8 +19339,7 @@ build_lswitch_and_lrouter_iterate_by_lr(struct >> ovn_datapath *od, >> build_gateway_redirect_flows_for_lrouter(od, lsi->lflows, >> &lsi->match, >> &lsi->actions, >> od->datapath_lflows); >> - build_arp_request_flows_for_lrouter(od, lsi->lflows, &lsi->match, >> - &lsi->actions, >> + build_arp_request_flows_for_lrouter(od, lsi->lflows, >> lsi->meter_groups, >> od->datapath_lflows); >> build_ecmp_stateful_egr_flows_for_lrouter(od, lsi->lflows, >> @@ -20855,6 +20897,9 @@ routes_init(struct routes_data *data) >> hmap_init(&data->parsed_routes); >> simap_init(&data->route_tables); >> hmap_init(&data->bfd_active_connections); >> + data->tracked = false; >> + hmapx_init(&data->trk_data.trk_deleted_parsed_route); >> + hmapx_init(&data->trk_data.trk_crupdated_parsed_route); >> } >> >> void >> @@ -20985,6 +21030,17 @@ routes_destroy(struct routes_data *data) >> >> simap_destroy(&data->route_tables); >> __bfd_destroy(&data->bfd_active_connections); >> + data->tracked = false; >> + hmapx_destroy(&data->trk_data.trk_crupdated_parsed_route); >> + hmapx_destroy(&data->trk_data.trk_deleted_parsed_route); >> +} >> + >> +void >> +routes_clear_tracked(struct routes_data *data) >> +{ >> + data->tracked = false; >> + hmapx_clear(&data->trk_data.trk_crupdated_parsed_route); >> + hmapx_clear(&data->trk_data.trk_deleted_parsed_route); >> } >> >> void >> diff --git a/northd/northd.h b/northd/northd.h >> index 4165b6fdd..ba8ddc430 100644 >> --- a/northd/northd.h >> +++ b/northd/northd.h >> @@ -159,6 +159,7 @@ enum northd_tracked_data_type { >> NORTHD_TRACKED_LS_ACLS = (1 << 4), >> NORTHD_TRACKED_SWITCHES = (1 << 5), >> NORTHD_TRACKED_ROUTERS = (1 << 6), >> + NORTHD_TRACKED_LR_ROUTES = (1 << 7), >> > > nit: Double space. > > >> }; >> >> /* Track what's changed in the northd engine node. >> @@ -176,6 +177,10 @@ struct northd_tracked_data { >> * hmapx node is 'struct ovn_datapath *'. */ >> struct hmapx trk_nat_lrs; >> >> + /* Tracked logical routers whose static routes have changed. >> + * hmapx node is 'struct ovn_datapath *'. */ >> + struct hmapx trk_lrs_routes; >> + >> /* Tracked logical switches whose load balancers have changed. >> * hmapx node is 'struct ovn_datapath *'. */ >> struct hmapx ls_with_changed_lbs; >> @@ -216,10 +221,23 @@ struct route_policy { >> uint32_t jump_chain_id; >> }; >> >> +struct route_tracked_data { >> + /* Contains references to group_ecmp_route_node. Each of the >> referenced >> + * datapaths contains at least one route. */ >> + struct hmapx trk_crupdated_parsed_route; >> + >> + /* Contains references to group_ecmp_route_node. Each of the >> referenced >> + * datapath previously had some routes. The datapath now no longer >> + * contains any route.*/ >> + struct hmapx trk_deleted_parsed_route; >> > > Both comments are wrong, this does contain references > to the struct parsed_route. > I agree > > >> +}; >> + >> struct routes_data { >> struct hmap parsed_routes; /* Stores struct parsed_route. */ >> struct simap route_tables; >> struct hmap bfd_active_connections; >> + bool tracked; >> + struct route_tracked_data trk_data; >> }; >> >> struct route_policies_data { >> @@ -874,6 +892,14 @@ struct parsed_route *parsed_route_add( >> const struct ovn_port *tracked_port, >> struct hmap *routes); >> >> +struct parsed_route * parsed_routes_add_static( >> > > nit: Double space between struct and parsed_route. > > >> + const struct ovn_datapath *od, >> + const struct hmap *lr_ports, >> + const struct nbrec_logical_router_static_route *route, >> + const struct hmap *bfd_connections, >> + struct hmap *routes, struct simap *route_tables, >> + struct hmap *bfd_active_connections); >> + >> > > >> struct svc_monitors_map_data { >> const struct hmap *local_svc_monitors_map; >> const struct hmap *ic_learned_svc_monitors_map; >> @@ -918,7 +944,7 @@ void build_parsed_routes(const struct ovn_datapath *, >> const struct hmap *, >> uint32_t get_route_table_id(struct simap *, const char *); >> void routes_init(struct routes_data *); >> void routes_destroy(struct routes_data *); >> - >> +void routes_clear_tracked(struct routes_data *); >> void bfd_init(struct bfd_data *); >> void bfd_destroy(struct bfd_data *); >> >> @@ -944,6 +970,10 @@ void build_route_data_flows_for_lrouter( >> const struct ovn_datapath *od, struct lflow_table *lflows, >> const struct group_ecmp_datapath *route_node, >> const struct sset *bfd_ports); >> +void build_arp_request_flows_for_lrouter( >> + const struct ovn_datapath *od, struct lflow_table *lflows, >> + const struct shash *meter_groups, >> + struct lflow_ref *lflow_ref); >> >> bool lflow_handle_northd_lr_changes(struct ovsdb_idl_txn *ovnsh_txn, >> struct tracked_dps *, >> @@ -1034,6 +1064,11 @@ northd_has_lr_nats_in_tracked_data(struct >> northd_tracked_data *trk_nd_changes) >> { >> return trk_nd_changes->type & NORTHD_TRACKED_LR_NATS; >> } >> +static inline bool >> +northd_has_lr_route_in_tracked_data(struct northd_tracked_data >> *trk_nd_changes) >> +{ >> + return trk_nd_changes->type & NORTHD_TRACKED_LR_ROUTES; >> +} >> >> static inline bool >> northd_has_ls_lbs_in_tracked_data(struct northd_tracked_data >> *trk_nd_changes) >> diff --git a/tests/ovn-inc-proc-graph-dump.at b/tests/ >> ovn-inc-proc-graph-dump.at >> index 69b4bee98..de1c4696b 100644 >> --- a/tests/ovn-inc-proc-graph-dump.at >> +++ b/tests/ovn-inc-proc-graph-dump.at >> @@ -149,9 +149,11 @@ digraph "Incremental-Processing-Engine" { >> bfd [[style=filled, shape=box, fillcolor=white, label="bfd"]]; >> NB_bfd -> bfd [[label=""]]; >> SB_bfd -> bfd [[label=""]]; >> + NB_logical_router_static_route [[style=filled, shape=box, >> fillcolor=white, label="NB_logical_router_static_route"]]; >> routes [[style=filled, shape=box, fillcolor=white, >> label="routes"]]; >> bfd -> routes [[label=""]]; >> northd -> routes [[label="routes_northd_change_handler"]]; >> + NB_logical_router_static_route -> routes >> [[label="routes_static_route_change_handler"]]; >> route_policies [[style=filled, shape=box, fillcolor=white, >> label="route_policies"]]; >> bfd -> route_policies [[label=""]]; >> northd -> route_policies >> [[label="route_policies_northd_change_handler"]]; >> @@ -166,7 +168,7 @@ digraph "Incremental-Processing-Engine" { >> SB_learned_route -> learned_route_sync >> [[label="learned_route_sync_sb_learned_route_change_handler"]]; >> northd -> learned_route_sync >> [[label="learned_route_sync_northd_change_handler"]]; >> group_ecmp_route [[style=filled, shape=box, fillcolor=white, >> label="group_ecmp_route"]]; >> - routes -> group_ecmp_route [[label=""]]; >> + routes -> group_ecmp_route >> [[label="group_ecmp_static_route_change_handler"]]; >> learned_route_sync -> group_ecmp_route >> [[label="group_ecmp_route_learned_route_change_handler"]]; >> ls_stateful [[style=filled, shape=box, fillcolor=white, >> label="ls_stateful"]]; >> northd -> ls_stateful [[label="ls_stateful_northd_handler"]]; >> @@ -187,7 +189,7 @@ digraph "Incremental-Processing-Engine" { >> SB_logical_dp_group -> lflow [[label=""]]; >> bfd_sync -> lflow [[label=""]]; >> route_policies -> lflow [[label=""]]; >> - routes -> lflow [[label=""]]; >> + routes -> lflow [[label="lflow_group_route_change_handler"]]; >> group_ecmp_route -> lflow >> [[label="lflow_group_ecmp_route_change_handler"]]; >> global_config -> lflow [[label="node_global_config_handler"]]; >> sampling_app -> lflow [[label=""]]; >> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at >> index fd049d096..305d6d160 100644 >> --- a/tests/ovn-northd.at >> +++ b/tests/ovn-northd.at >> @@ -4153,7 +4153,7 @@ check ovn-nbctl --bfd=$uuid lr-route-add r0 >> 100.0.0.0/8 192.168.1.2 >> wait_column down bfd status logical_port=r0-sw1 >> AT_CHECK([ovn-nbctl lr-route-list r0 | grep 192.168.1.2 | grep -q bfd], >> [0], [], [ignore]) >> >> -check_engine_stats northd recompute nocompute >> +check_engine_stats northd recompute incremental >> check_engine_stats bfd recompute nocompute >> check_engine_stats routes recompute nocompute >> check_engine_stats lflow recompute nocompute >> @@ -4169,7 +4169,7 @@ check ovn-nbctl --bfd lr-route-add r0 240.0.0.0/8 >> 192.168.5.2 r0-sw5 >> wait_column down bfd status logical_port=r0-sw5 >> AT_CHECK([ovn-nbctl lr-route-list r0 | grep 192.168.5.2 | grep -q bfd], >> [0], [], [ignore]) >> >> -check_engine_stats northd recompute nocompute >> +check_engine_stats northd recompute incremental >> check_engine_stats bfd recompute nocompute >> check_engine_stats routes recompute nocompute >> check_engine_stats lflow recompute nocompute >> @@ -4181,7 +4181,7 @@ check ovn-nbctl --bfd --policy=src-ip lr-route-add >> r0 192.168.6.1/32 192.168.10. >> wait_column down bfd status logical_port=r0-sw6 >> AT_CHECK([ovn-nbctl lr-route-list r0 | grep 192.168.6.1 | grep -q bfd], >> [0], [], [ignore]) >> >> -check_engine_stats northd recompute nocompute >> +check_engine_stats northd recompute incremental >> check_engine_stats bfd recompute nocompute >> check_engine_stats route_policies recompute nocompute >> check_engine_stats lflow recompute nocompute >> @@ -4216,7 +4216,7 @@ wait_column down bfd status logical_port=r0-sw8 >> bfd_route_policy_uuid=$(fetch_column nb:bfd _uuid logical_port=r0-sw8) >> AT_CHECK([ovn-nbctl list logical_router_policy | grep -q >> $bfd_route_policy_uuid]) >> >> -check_engine_stats northd recompute nocompute >> +check_engine_stats northd recompute incremental >> check_engine_stats bfd recompute nocompute >> check_engine_stats routes recompute nocompute >> check_engine_stats lflow recompute nocompute >> @@ -16150,12 +16150,12 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE >> >> check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >> check ovn-nbctl --wait=sb lr-route-add lr0 192.168.0.0/24 10.0.0.10 >> -check_engine_compute northd recompute >> -check_engine_compute routes recompute >> +check_engine_compute northd incremental >> +check_engine_compute routes incremental >> check_engine_compute advertised_route_sync recompute >> -check_engine_compute learned_route_sync recompute >> -check_engine_compute group_ecmp_route recompute >> -check_engine_compute lflow recompute >> +check_engine_compute learned_route_sync incremental >> +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 >> > > It might be a good idea to add more test cases when the > route actually changes. > I agree Regards, Lucas > > >> -- >> 2.43.0 >> >> >> -- >> >> >> >> >> _'Esta mensagem é direcionada apenas para os endereços constantes no >> cabeçalho inicial. Se você não está listado nos endereços constantes no >> cabeçalho, pedimos-lhe que desconsidere completamente o conteúdo dessa >> mensagem e cuja cópia, encaminhamento e/ou execução das ações citadas >> estão >> imediatamente anuladas e proibidas'._ >> >> >> * **'Apesar do Magazine Luiza tomar >> todas as precauções razoáveis para assegurar que nenhum vírus esteja >> presente nesse e-mail, a empresa não poderá aceitar a responsabilidade >> por >> quaisquer perdas ou danos causados por esse e-mail ou por seus anexos'.* >> >> >> >> _______________________________________________ >> dev mailing list >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> > Regards, > Ales > -- _‘Esta mensagem é direcionada apenas para os endereços constantes no cabeçalho inicial. Se você não está listado nos endereços constantes no cabeçalho, pedimos-lhe que desconsidere completamente o conteúdo dessa mensagem e cuja cópia, encaminhamento e/ou execução das ações citadas estão imediatamente anuladas e proibidas’._ * **‘Apesar do Magazine Luiza tomar todas as precauções razoáveis para assegurar que nenhum vírus esteja presente nesse e-mail, a empresa não poderá aceitar a responsabilidade por quaisquer perdas ou danos causados por esse e-mail ou por seus anexos’.* _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
