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 :) > > 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. > > 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()? > 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 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
