On Wed, 2025-02-12 at 15:20 +0100, Felix Huettner wrote: > On Wed, Feb 12, 2025 at 02:43:08AM +0100, Martin Kalcok wrote: > > This version attempts to split northd engine processing > > of advertised routes. The main motivation is to avoid > > logical flow recomputation when NAT/LB routes change. > > Hi Martin, > > i took a look at the changes and i'll add my findings below. > > Note that i squashed part 3 and 4 together locally for reviewing. > I'll > try to put my comments to the appropriate patch, but i am already > sorry > for when i mess that up :) >
Thanks for the review and hints Felix, I thought I was helping when I kept the part 2 of the change in separate commit, but maybe I wasn't :D > > > > Signed-off-by: Martin Kalcok <[email protected]> > > --- > > lib/stopwatch-names.h | 1 + > > northd/en-advertised-route-sync.c | 167 +++++++++++++++++++++++--- > > ---- > > northd/en-advertised-route-sync.h | 4 + > > northd/en-northd-output.c | 8 ++ > > northd/en-northd-output.h | 2 + > > northd/inc-proc-northd.c | 8 ++ > > northd/northd.c | 38 +++++++ > > northd/northd.h | 15 ++- > > 8 files changed, 205 insertions(+), 38 deletions(-) > > > > diff --git a/lib/stopwatch-names.h b/lib/stopwatch-names.h > > index c12dd28d0..13aa5e7bf 100644 > > --- a/lib/stopwatch-names.h > > +++ b/lib/stopwatch-names.h > > @@ -36,5 +36,6 @@ > > #define LS_STATEFUL_RUN_STOPWATCH_NAME "ls_stateful" > > #define ADVERTISED_ROUTE_SYNC_RUN_STOPWATCH_NAME > > "advertised_route_sync" > > #define LEARNED_ROUTE_SYNC_RUN_STOPWATCH_NAME "learned_route_sync" > > +#define DYNAMIC_ROUTES_RUN_STOPWATCH_NAME "dynamic_routes" > > > > #endif > > diff --git a/northd/en-advertised-route-sync.c b/northd/en- > > advertised-route-sync.c > > index 9e8636806..9d4fb308d 100644 > > --- a/northd/en-advertised-route-sync.c > > +++ b/northd/en-advertised-route-sync.c > > @@ -25,12 +25,14 @@ > > #include "openvswitch/hmap.h" > > #include "ovn-util.h" > > > > + > > static void > > advertised_route_table_sync( > > struct ovsdb_idl_txn *ovnsb_txn, > > const struct sbrec_advertised_route_table > > *sbrec_advertised_route_table, > > const struct lr_stateful_table *lr_stateful_table, > > - const struct hmap *parsed_routes, > > + const struct hmap *routes, > > + const struct hmap *dynamic_routes, > > struct advertised_route_sync_data *data); > > > > bool > > @@ -141,6 +143,8 @@ en_advertised_route_sync_run(struct engine_node > > *node, void *data OVS_UNUSED) > > struct advertised_route_sync_data *routes_sync_data = data; > > struct routes_data *routes_data > > = engine_get_input_data("routes", node); > > + struct dynamic_routes_data *dynamic_routes_data > > + = engine_get_input_data("dynamic_routes", node); > > const struct engine_context *eng_ctx = engine_get_context(); > > const struct sbrec_advertised_route_table > > *sbrec_advertised_route_table = > > EN_OVSDB_GET(engine_get_input("SB_advertised_route", > > node)); > > @@ -153,12 +157,75 @@ en_advertised_route_sync_run(struct > > engine_node *node, void *data OVS_UNUSED) > > sbrec_advertised_route_table, > > &lr_stateful_data->table, > > &routes_data->parsed_routes, > > + &dynamic_routes_data- > > >parsed_routes, > > routes_sync_data); > > > > stopwatch_stop(ADVERTISED_ROUTE_SYNC_RUN_STOPWATCH_NAME, > > time_msec()); > > engine_set_node_state(node, EN_UPDATED); > > } > > > > +void > > +*en_dynamic_routes_init(struct engine_node *node OVS_UNUSED, > > + struct engine_arg *arg OVS_UNUSED) > > +{ > > + struct dynamic_routes_data *data = xmalloc(sizeof *data); > > + *data = (struct dynamic_routes_data) { > > + .parsed_routes = HMAP_INITIALIZER(&data->parsed_routes), > > + }; > > + > > + return data; > > +} > > + > > +void > > +en_dynamic_routes_cleanup(void *data_) > > +{ > > + struct dynamic_routes_data *data = data_; > > + > > + struct parsed_route *r; > > + HMAP_FOR_EACH_POP (r, key_node, &data->parsed_routes) { > > + parsed_route_free(r); > > + } > > + hmap_destroy(&data->parsed_routes); > > +} > > + > > +void > > +en_dynamic_routes_run(struct engine_node *node, void *data) > > +{ > > + struct dynamic_routes_data *dynamic_routes_data = data; > > + struct northd_data *northd_data = > > engine_get_input_data("northd", node); > > + struct ed_type_lr_stateful *lr_stateful_data = > > + engine_get_input_data("lr_stateful", node); > > + > > + const struct lr_stateful_record *lr_stateful_rec; > > + HMAP_FOR_EACH (lr_stateful_rec, key_node, > > + &lr_stateful_data->table.entries) { > > + const struct ovn_datapath *od = > > + ovn_datapaths_find_by_index(&northd_data- > > >lr_datapaths, > > + lr_stateful_rec- > > >lr_index); > > + if (!od->dynamic_routing) { > > + continue; > > + } > > + build_lb_nat_parsed_routes(od, lr_stateful_rec->lrnat_rec, > > + &dynamic_routes_data- > > >parsed_routes); > > + > > + } > > + engine_set_node_state(node, EN_UPDATED); > > +} > > + > > +bool > > +dynamic_routes_change_handler(struct engine_node *node OVS_UNUSED, > > + void *data OVS_UNUSED) > > +{ > > + /* XXX: It was suggested to iterate through data in lr_stateful > > node > > + * (ed_type_lr_stateful). However the trk_data is only > > populated when NAT/LB > > + * changes. While this works for us when NAT/LB is is changed, > > we also need > > + * this handler to trigger when dynamic routing options are > > changed. > > + * I didn't find a node that would give us only the LR on which > > options > > + * changed, so for now I set it to recompute every time. */ > > + return false; > > +} > > + > > + > > struct ar_entry { > > struct hmap_node hmap_node; > > > > @@ -335,12 +402,59 @@ publish_host_routes(struct hmap *sync_routes, > > } > > } > > > > +static void > > +advertised_route_table_sync_route_add( > > + const struct lr_stateful_table *lr_stateful_table, > > + struct advertised_route_sync_data *data, > > + struct uuidset *host_route_lrps, > > + struct hmap *sync_routes, > > + const struct parsed_route *route) > > +{ > > + if (route->is_discard_route) { > > + return; > > + } > > + if (prefix_is_link_local(&route->prefix, route->plen)) { > > + return; > > + } > > + if (!route->od->dynamic_routing) { > > + return; > > + } > > + > > + enum dynamic_routing_redistribute_mode drr = > > + route->out_port->dynamic_routing_redistribute; > > + if (route->source == ROUTE_SOURCE_CONNECTED) { > > + if ((drr & DRRM_CONNECTED) == 0) { > > + return; > > + } > > + /* If we advertise host routes, we only need to do so once > > per > > + * LRP. */ > > + const struct uuid *lrp_uuid = &route->out_port->nbrp- > > >header_.uuid; > > + if (drr & DRRM_CONNECTED_AS_HOST && > > + !uuidset_contains(host_route_lrps, lrp_uuid)) { > > + uuidset_insert(host_route_lrps, lrp_uuid); > > + publish_host_routes(sync_routes, lr_stateful_table, > > route, data); > > + return; > > + } > > + } > > + if (route->source == ROUTE_SOURCE_STATIC && (drr & > > DRRM_STATIC) == 0) { > > + return; > > + } > > + if (route->source == ROUTE_SOURCE_NAT && (drr & DRRM_NAT) == > > 0) { > > + return; > > + } > > + > > + char *ip_prefix = normalize_v46_prefix(&route->prefix, route- > > >plen); > > + ar_add_entry(sync_routes, route->od->sb, route->out_port->sb, > > + ip_prefix, NULL); > > +} > > + > > static void > > advertised_route_table_sync( > > struct ovsdb_idl_txn *ovnsb_txn, > > const struct sbrec_advertised_route_table > > *sbrec_advertised_route_table, > > const struct lr_stateful_table *lr_stateful_table, > > - const struct hmap *parsed_routes, > > + const struct hmap *routes, > > + const struct hmap *dynamic_routes, > > struct advertised_route_sync_data *data) > > { > > struct hmap sync_routes = HMAP_INITIALIZER(&sync_routes); > > @@ -348,46 +462,25 @@ advertised_route_table_sync( > > const struct parsed_route *route; > > > > struct ar_entry *route_e; > > - const struct sbrec_advertised_route *sb_route; > > - HMAP_FOR_EACH (route, key_node, parsed_routes) { > > - if (route->is_discard_route) { > > - continue; > > - } > > - if (prefix_is_link_local(&route->prefix, route->plen)) { > > - continue; > > - } > > - if (!route->od->dynamic_routing) { > > - continue; > > - } > > > > - enum dynamic_routing_redistribute_mode drr = > > - route->out_port->dynamic_routing_redistribute; > > - if (route->source == ROUTE_SOURCE_CONNECTED) { > > - if ((drr & DRRM_CONNECTED) == 0) { > > - continue; > > - } > > - /* If we advertise host routes, we only need to do so > > once per > > - * LRP. */ > > - const struct uuid *lrp_uuid = > > - &route->out_port->nbrp->header_.uuid; > > - if (drr & DRRM_CONNECTED_AS_HOST && > > - !uuidset_contains(&host_route_lrps, lrp_uuid)) { > > - uuidset_insert(&host_route_lrps, lrp_uuid); > > - publish_host_routes(&sync_routes, > > lr_stateful_table, > > - route, data); > > - continue; > > - } > > - } > > - if (route->source == ROUTE_SOURCE_STATIC && (drr & > > DRRM_STATIC) == 0) { > > - continue; > > - } > > + /* First build the set of non-dynamic routes that need sync- > > ing. */ > > + HMAP_FOR_EACH (route, key_node, routes) { > > + advertised_route_table_sync_route_add(lr_stateful_table, > > + data, > > &host_route_lrps, > > + &sync_routes, > > + route); > > + } > > > > - char *ip_prefix = normalize_v46_prefix(&route->prefix, > > route->plen); > > - route_e = ar_add_entry(&sync_routes, route->od->sb, > > - route->out_port->sb, ip_prefix, > > NULL); > > + /* First add the set of dynamic routes that need sync-ing. */ > > + HMAP_FOR_EACH (route, key_node, dynamic_routes) { > > + advertised_route_table_sync_route_add(lr_stateful_table, > > + data, > > &host_route_lrps, > > + &sync_routes, > > + route); > > } > > uuidset_destroy(&host_route_lrps); > > > > + const struct sbrec_advertised_route *sb_route; > > SBREC_ADVERTISED_ROUTE_TABLE_FOR_EACH_SAFE (sb_route, > > > > sbrec_advertised_route_table) { > > route_e = ar_find(&sync_routes, sb_route->datapath, > > diff --git a/northd/en-advertised-route-sync.h b/northd/en- > > advertised-route-sync.h > > index 1f24fd329..94eee0eee 100644 > > --- a/northd/en-advertised-route-sync.h > > +++ b/northd/en-advertised-route-sync.h > > @@ -36,4 +36,8 @@ void *en_advertised_route_sync_init(struct > > engine_node *, struct engine_arg *); > > void en_advertised_route_sync_cleanup(void *data); > > void en_advertised_route_sync_run(struct engine_node *, void > > *data); > > > > +bool dynamic_routes_change_handler(struct engine_node *, void > > *data); > > +void *en_dynamic_routes_init(struct engine_node *, struct > > engine_arg *); > > +void en_dynamic_routes_cleanup(void *data); > > +void en_dynamic_routes_run(struct engine_node *, void *data); > > #endif /* EN_ADVERTISED_ROUTE_SYNC_H */ > > diff --git a/northd/en-northd-output.c b/northd/en-northd-output.c > > index 715abd3ab..81c796974 100644 > > --- a/northd/en-northd-output.c > > +++ b/northd/en-northd-output.c > > @@ -97,3 +97,11 @@ > > northd_output_advertised_route_sync_handler(struct engine_node > > *node, > > return true; > > } > > > > +bool > > +northd_output_dynamic_routes_handler(struct engine_node *node, > > + void *data OVS_UNUSED) > > +{ > > + engine_set_node_state(node, EN_UPDATED); > > + return true; > > +} > > + > > diff --git a/northd/en-northd-output.h b/northd/en-northd-output.h > > index 783587cb6..689640118 100644 > > --- a/northd/en-northd-output.h > > +++ b/northd/en-northd-output.h > > @@ -23,5 +23,7 @@ bool northd_output_acl_id_handler(struct > > engine_node *node, > > void *data OVS_UNUSED); > > bool northd_output_advertised_route_sync_handler(struct > > engine_node *node, > > void *data > > OVS_UNUSED); > > +bool northd_output_dynamic_routes_handler(struct engine_node > > *node, > > + void *data OVS_UNUSED); > > > > #endif > > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c > > index ce7a89ec4..dc88ebd99 100644 > > --- a/northd/inc-proc-northd.c > > +++ b/northd/inc-proc-northd.c > > @@ -175,6 +175,7 @@ static ENGINE_NODE(multicast_igmp, > > "multicast_igmp"); > > static ENGINE_NODE(acl_id, "acl_id"); > > static ENGINE_NODE(advertised_route_sync, > > "advertised_route_sync"); > > static ENGINE_NODE(learned_route_sync, "learned_route_sync"); > > +static ENGINE_NODE(dynamic_routes, "dynamic_routes"); > > > > void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > > struct ovsdb_idl_loop *sb) > > @@ -287,7 +288,12 @@ void inc_proc_northd_init(struct > > ovsdb_idl_loop *nb, > > engine_add_input(&en_ecmp_nexthop, &en_sb_mac_binding, > > ecmp_nexthop_mac_binding_handler); > > > > + engine_add_input(&en_dynamic_routes, &en_lr_stateful, > > + dynamic_routes_change_handler); > > + engine_add_input(&en_dynamic_routes, &en_northd, > > engine_noop_handler); > > + > > engine_add_input(&en_advertised_route_sync, &en_routes, NULL); > > + engine_add_input(&en_advertised_route_sync, > > &en_dynamic_routes, NULL); > > engine_add_input(&en_advertised_route_sync, > > &en_sb_advertised_route, > > NULL); > > engine_add_input(&en_advertised_route_sync, &en_lr_stateful, > > @@ -399,6 +405,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop > > *nb, > > northd_output_acl_id_handler); > > engine_add_input(&en_northd_output, &en_advertised_route_sync, > > northd_output_advertised_route_sync_handler); > > + engine_add_input(&en_northd_output, &en_dynamic_routes, > > + northd_output_dynamic_routes_handler); > > I think you only need to add something to en_northd_output if there > is > no transitive dependency to it. > As en_advertised_route_sync now depends on en_dynamic_routes and > en_advertised_route_sync already has a dependency on en_northd_output > i > guess you don't need an additional one. Hmm, this may be a leftover from the previous node design where it was necessary. I'll try to remove it, see if it works. > > > > > struct engine_arg engine_arg = { > > .nb_idl = nb->idl, > > diff --git a/northd/northd.c b/northd/northd.c > > index a6eb70948..4b5708059 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -846,6 +846,14 @@ parse_dynamic_routing_redistribute( > > out |= DRRM_STATIC; > > continue; > > } > > + if (!strcmp(token, "nat")) { > > + out |= DRRM_NAT; > > + continue; > > + } > > + if (!strcmp(token, "lb")) { > > + out |= DRRM_LB; > > + continue; > > + } > > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, > > 1); > > VLOG_WARN_RL(&rl, "unkown dynamic-routing-redistribute > > option '%s'", > > token); > > @@ -11272,6 +11280,34 @@ build_parsed_routes(const struct > > ovn_datapath *od, const struct hmap *lr_ports, > > } > > } > > > > +void > > +build_lb_nat_parsed_routes(const struct ovn_datapath *od, > > + const struct lr_nat_record *lr_nat, > > + struct hmap *routes) > > +{ > > + const struct ovn_port *op; > > + HMAP_FOR_EACH (op, dp_node, &od->ports) { > > + enum dynamic_routing_redistribute_mode drrm = op- > > >dynamic_routing_redistribute; > > + if (!(drrm & DRRM_NAT)) { > > + continue; > > + } > > + /* I'm thinking of extending parsed_route struct with > > "tracked_port". > > + * Since we are already parsing/iterating NATs here, it > > feels more > > + * efficinet to figure out the tracked_port here, rather > > than > > + * re-parsing NATs down the line in the > > advertised_route_table_sync > > + * function before calling "ar_add_entry".*/ > > + for (size_t i = 0; i < lr_nat->n_nat_entries; i++) { > > + const struct ovn_nat *nat = &lr_nat->nat_entries[i]; > > + int plen = nat_entry_is_v6(nat) ? 128 : 32; > > + struct in6_addr prefix; > > + ip46_parse(nat->nb->external_ip, &prefix); > > + parsed_route_add(od, NULL, &prefix, plen, false, > > + nat->nb->external_ip, op, 0, false, > > false, > > + NULL, ROUTE_SOURCE_NAT, &op->nbrp- > > >header_, routes); > > + } > > + } > > + > > +} > > struct ecmp_route_list_node { > > struct ovs_list list_node; > > uint16_t id; /* starts from 1 */ > > @@ -11441,6 +11477,8 @@ route_source_to_offset(enum route_source > > source) > > { > > switch (source) { > > case ROUTE_SOURCE_CONNECTED: > > + case ROUTE_SOURCE_NAT: > > + case ROUTE_SOURCE_LB: > > These two route sources should never end up here, right? > Otherwise we would add flows for them. > So maybe they should rather be a separate case with > OVS_NOT_REACHED()? Seems reasonable. > > > return ROUTE_PRIO_OFFSET_CONNECTED; > > case ROUTE_SOURCE_STATIC: > > return ROUTE_PRIO_OFFSET_STATIC; > > diff --git a/northd/northd.h b/northd/northd.h > > index 11efa0136..95f2fe010 100644 > > --- a/northd/northd.h > > +++ b/northd/northd.h > > @@ -186,11 +186,15 @@ struct route_policy { > > }; > > > > struct routes_data { > > - struct hmap parsed_routes; > > + struct hmap parsed_routes; /* Stores struct parsed_route. */ > > struct simap route_tables; > > struct hmap bfd_active_connections; > > }; > > > > +struct dynamic_routes_data { > > + struct hmap parsed_routes; /* Stores struct parsed_route. */ > > +}; > > + > > struct route_policies_data { > > struct hmap route_policies; > > struct hmap bfd_active_connections; > > @@ -312,6 +316,8 @@ enum dynamic_routing_redistribute_mode_bits { > > DRRM_CONNECTED_BIT = 0, > > DRRM_CONNECTED_AS_HOST_BIT = 1, > > DRRM_STATIC_BIT = 2, > > + DRRM_NAT_BIT = 3, > > + DRRM_LB_BIT = 4, > > After a rebase that can be switched to our new way of defining bits. > That also allows you then to use the "drr_mode_*_is_set". Thanks, will do. Martin. > > Thanks a lot, > Felix > > > }; > > > > enum dynamic_routing_redistribute_mode { > > @@ -319,6 +325,8 @@ enum dynamic_routing_redistribute_mode { > > DRRM_CONNECTED = (1 << DRRM_CONNECTED_BIT), > > DRRM_CONNECTED_AS_HOST = (1 << DRRM_CONNECTED_AS_HOST_BIT), > > DRRM_STATIC = (1 << DRRM_STATIC_BIT), > > + DRRM_NAT = (1 << DRRM_NAT_BIT), > > + DRRM_LB = (1 << DRRM_LB_BIT), > > }; > > > > /* The 'key' comes from nbs->header_.uuid or nbr->header_.uuid or > > @@ -730,6 +738,10 @@ enum route_source { > > ROUTE_SOURCE_STATIC, > > /* The route is dynamically learned by an ovn-controller. */ > > ROUTE_SOURCE_LEARNED, > > + /* The route is derived from a NAT's external IP. */ > > + ROUTE_SOURCE_NAT, > > + /* The route is derived from a LB's VIP. */ > > + ROUTE_SOURCE_LB, > > }; > > > > struct parsed_route { > > @@ -811,6 +823,7 @@ void route_policies_destroy(struct > > route_policies_data *); > > void build_parsed_routes(const struct ovn_datapath *, const struct > > hmap *, > > const struct hmap *, struct hmap *, > > struct simap *, > > struct hmap *); > > +void build_lb_nat_parsed_routes(const struct ovn_datapath *, const > > struct lr_nat_record *, struct hmap *); > > uint32_t get_route_table_id(struct simap *, const char *); > > void routes_init(struct routes_data *); > > void routes_destroy(struct routes_data *); > > -- > > 2.43.0 > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
