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? > + 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. > + 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. + 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. > + 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? > + } > + } > + 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. > + 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. } > > 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. > +}; > + > 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. > -- > 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
