On 2/18/25 3:02 PM, Felix Huettner via dev wrote:
> Add support for I+P handling of en-learned-route-sync for the
> sb_learned_routes input.
>
> Signed-off-by: Felix Huettner <[email protected]>
> ---
Hi Felix,
Overall the patch looks good to me. I do have some small comments below.
> v1-v2: Fix test error
>
> northd/en-learned-route-sync.c | 123 ++++++++++++++++++++++++++++++---
> northd/en-learned-route-sync.h | 25 +++++++
> northd/inc-proc-northd.c | 6 +-
> northd/northd.c | 5 +-
> northd/northd.h | 31 +++++----
> tests/ovn-northd.at | 16 ++---
> 6 files changed, 170 insertions(+), 36 deletions(-)
>
> diff --git a/northd/en-learned-route-sync.c b/northd/en-learned-route-sync.c
> index 4e87b3265..312141208 100644
> --- a/northd/en-learned-route-sync.c
> +++ b/northd/en-learned-route-sync.c
> @@ -60,6 +60,19 @@ learned_route_sync_northd_change_handler(struct
> engine_node *node,
> return true;
> }
>
> +static void
> +routes_sync_clear_tracked(struct learned_route_sync_data *data)
> +{
> + hmapx_clear(&data->trk_data.trk_created_parsed_route);
> + struct hmapx_node *hmapx_node;
> + HMAPX_FOR_EACH_SAFE (hmapx_node,
> + &data->trk_data.trk_deleted_parsed_route) {
> + parsed_route_free(hmapx_node->data);
> + hmapx_delete(&data->trk_data.trk_deleted_parsed_route, hmapx_node);
> + }
> + data->trk_data.type = LEARNED_ROUTES_TRACKED_NONE;
> +}
> +
> static void
> routes_sync_clear(struct learned_route_sync_data *data)
> {
> @@ -67,12 +80,16 @@ routes_sync_clear(struct learned_route_sync_data *data)
> HMAP_FOR_EACH_POP (r, key_node, &data->parsed_routes) {
> parsed_route_free(r);
> }
> + routes_sync_clear_tracked(data);
> }
>
> static void
> routes_sync_init(struct learned_route_sync_data *data)
> {
> hmap_init(&data->parsed_routes);
> + hmapx_init(&data->trk_data.trk_created_parsed_route);
> + hmapx_init(&data->trk_data.trk_deleted_parsed_route);
> + data->trk_data.type = LEARNED_ROUTES_TRACKED_NONE;
> }
>
> static void
> @@ -80,6 +97,8 @@ routes_sync_destroy(struct learned_route_sync_data *data)
> {
> routes_sync_clear(data);
> hmap_destroy(&data->parsed_routes);
> + hmapx_destroy(&data->trk_data.trk_created_parsed_route);
> + hmapx_destroy(&data->trk_data.trk_deleted_parsed_route);
> }
>
> void *
> @@ -97,6 +116,12 @@ en_learned_route_sync_cleanup(void *data)
> routes_sync_destroy(data);
> }
>
> +void
> +en_learned_route_sync_clear_tracked_data(void *data)
> +{
> + routes_sync_clear_tracked(data);
> +}
> +
> void
> en_learned_route_sync_run(struct engine_node *node, void *data)
> {
> @@ -122,7 +147,7 @@ en_learned_route_sync_run(struct engine_node *node, void
> *data)
> }
>
>
> -static void
> +static struct parsed_route *
> parse_route_from_sbrec_route(struct hmap *parsed_routes_out,
> const struct hmap *lr_ports,
> const struct hmap *lr_datapaths,
> @@ -132,7 +157,7 @@ parse_route_from_sbrec_route(struct hmap
> *parsed_routes_out,
> NULL, lr_datapaths, route->datapath);
>
> if (!od || ovn_datapath_is_stale(od)) {
> - return;
> + return NULL;
> }
>
> /* Verify that the next hop is an IP address with an all-ones mask. */
> @@ -144,7 +169,7 @@ parse_route_from_sbrec_route(struct hmap
> *parsed_routes_out,
> 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)) {
> @@ -153,7 +178,7 @@ parse_route_from_sbrec_route(struct hmap
> *parsed_routes_out,
> UUID_FMT, route->nexthop,
> UUID_ARGS(&route->header_.uuid));
> free(nexthop);
> - return;
> + return NULL;
> }
>
> /* Parse ip_prefix */
> @@ -164,7 +189,7 @@ parse_route_from_sbrec_route(struct hmap
> *parsed_routes_out,
> 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. */
> @@ -175,14 +200,18 @@ parse_route_from_sbrec_route(struct hmap
> *parsed_routes_out,
> IN6_IS_ADDR_V4MAPPED(&prefix),
> false,
> &out_port, &lrp_addr_s)) {
> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> + VLOG_WARN_RL(&rl, "could not find output port %s for learned route "
> + UUID_FMT, route->logical_port->logical_port,
> + UUID_ARGS(&route->header_.uuid));
> free(nexthop);
> - return;
> + return NULL;
> }
>
> - parsed_route_add(od, nexthop, &prefix, plen, false, lrp_addr_s,
> - out_port, 0, false, false, NULL,
> - ROUTE_SOURCE_LEARNED, &route->header_, NULL,
> - parsed_routes_out);
> + return parsed_route_add(od, nexthop, &prefix, plen, false, lrp_addr_s,
> + out_port, 0, false, false, NULL,
> + ROUTE_SOURCE_LEARNED, &route->header_, NULL,
> + parsed_routes_out);
> }
>
> static void
> @@ -212,3 +241,77 @@ routes_table_sync(
> parsed_route_hash(route));
> }
> }
> +
> +static struct parsed_route *
> +find_learned_route(const struct sbrec_learned_route *learned_route,
> + const struct ovn_datapaths *lr_datapaths,
> + const struct hmap *routes)
> +{
> + const struct ovn_datapath *od = ovn_datapath_from_sbrec(
> + NULL, &lr_datapaths->datapaths, learned_route->datapath);
> + if (!od) {
> + return NULL;
> + }
> + size_t hash = uuid_hash(&od->key);
Let's not use parsed_route implementation internals. Let's expose a
parsed_route_lookup_by_source(source, source_hint) or similar instead.
Then we can just use that one directly in the change handler instead of
re-implementing the logic in find_learned_route() here.
> + struct parsed_route *route;
> + HMAP_FOR_EACH_WITH_HASH (route, key_node, hash, routes) {
> + if (route->source == ROUTE_SOURCE_LEARNED &&
> + uuid_equals(&route->source_hint->uuid,
> + &learned_route->header_.uuid)) {
> + return route;
> + }
> + }
> + return NULL;
> +}
> +
> +bool
> +learned_route_sync_learned_route_change_handler(struct engine_node *node,
> + void *data_ OVS_UNUSED)
> +{
> + struct learned_route_sync_data *data = data_;
> +
> + const struct sbrec_learned_route_table *sbrec_learned_route_table =
> + EN_OVSDB_GET(engine_get_input("SB_learned_route", node));
> + struct northd_data *northd_data = engine_get_input_data("northd", node);
> +
> + const struct sbrec_learned_route *changed_route;
> + SBREC_LEARNED_ROUTE_TABLE_FOR_EACH_TRACKED (changed_route,
> + sbrec_learned_route_table) {
> + if (sbrec_learned_route_is_new(changed_route)) {
> + struct parsed_route *route = parse_route_from_sbrec_route(
> + &data->parsed_routes, &northd_data->lr_ports,
> + &northd_data->lr_datapaths.datapaths, changed_route);
> + if (route) {
> + hmapx_add(&data->trk_data.trk_created_parsed_route, route);
> + data->trk_data.type |= LEARNED_ROUTES_TRACKED_LEARNED;
> + continue;
> + }
> + }
> +
> + if (sbrec_learned_route_is_deleted(changed_route)) {
> + struct parsed_route *route = find_learned_route(
> + changed_route, &northd_data->lr_datapaths,
> + &data->parsed_routes);
> + if (!route) {
> + goto fail;
> + }
> + hmap_remove(&data->parsed_routes, &route->key_node);
> + hmapx_add(&data->trk_data.trk_deleted_parsed_route, route);
> + data->trk_data.type |= LEARNED_ROUTES_TRACKED_LEARNED;
> + continue;
> + }
> +
> + /* The learned routes should never be updated, so we just fail the
> + * handler here if that happens. */
> + goto fail;
> + }
> +
> + if ((data->trk_data.type & LEARNED_ROUTES_TRACKED_LEARNED) != 0) {
> + engine_set_node_state(node, EN_UPDATED);
> + }
> +
> + return true;
Nit: I'd add a newline here.
> +fail:
> + routes_sync_clear_tracked(data);
> + return false;
> +}
> diff --git a/northd/en-learned-route-sync.h b/northd/en-learned-route-sync.h
> index 1a5d6ff23..34baadaa1 100644
> --- a/northd/en-learned-route-sync.h
> +++ b/northd/en-learned-route-sync.h
> @@ -18,15 +18,40 @@
>
> #include "lib/inc-proc-eng.h"
> #include "openvswitch/hmap.h"
> +#include "hmapx.h"
> +
> +enum learned_routes_tracked_data_type {
> + LEARNED_ROUTES_TRACKED_NONE,
> + LEARNED_ROUTES_TRACKED_LEARNED = (1 << 0),
> +};
I'm confused, will we ever need more than NONE / LEARNED? If not can we
change this too bool?
> +
> +/* Track what's changed in the learned routes engine node.
> + * For now only tracks changed learned routes. */
> +struct learned_routes_tracked_data {
> + /* Indicates the type of data tracked. One or all of ROUTES_TRACKED_*.
> */
Nit: LEARNED_ROUTES_TRACKED_*?
> + enum learned_routes_tracked_data_type type;
> +
> + /* Tracked created routes based on sb_learned_routes.
> + * hmapx node is 'struct parsed_route *'. */
> + struct hmapx trk_created_parsed_route;
> +
> + /* Tracked deleted routes based on sb_learned_routes.
> + * hmapx node is 'struct parsed_route *'. */
> + struct hmapx trk_deleted_parsed_route;
> +};
>
> struct learned_route_sync_data {
> struct hmap parsed_routes;
> + struct learned_routes_tracked_data trk_data;
> };
>
> bool learned_route_sync_northd_change_handler(struct engine_node *,
> void *data);
> +bool learned_route_sync_learned_route_change_handler(struct engine_node *,
> + void *data);
> void *en_learned_route_sync_init(struct engine_node *, struct engine_arg *);
> void en_learned_route_sync_cleanup(void *data);
> +void en_learned_route_sync_clear_tracked_data(void *data);
> void en_learned_route_sync_run(struct engine_node *, void *data);
>
> #endif /* EN_LEARNED_ROUTE_SYNC_H */
> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> index 3fa99f637..bf0fbc62b 100644
> --- a/northd/inc-proc-northd.c
> +++ b/northd/inc-proc-northd.c
> @@ -174,7 +174,8 @@ static ENGINE_NODE(ecmp_nexthop, "ecmp_nexthop");
> 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_WITH_CLEAR_TRACK_DATA(learned_route_sync,
> + "learned_route_sync");
> static ENGINE_NODE(dynamic_routes, "dynamic_routes");
>
> void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> @@ -307,7 +308,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> advertised_route_sync_northd_change_handler);
>
> engine_add_input(&en_learned_route_sync, &en_routes, NULL);
> - engine_add_input(&en_learned_route_sync, &en_sb_learned_route, NULL);
> + engine_add_input(&en_learned_route_sync, &en_sb_learned_route,
> + learned_route_sync_learned_route_change_handler);
Nit: to be inline with the rest of the handlers this should be:
learned_route_sync_sb_learned_route_change_handler
that is:
<target_i_p_node>_<input_i_p_node>_change_handler
> engine_add_input(&en_learned_route_sync, &en_northd,
> learned_route_sync_northd_change_handler);
>
> diff --git a/northd/northd.c b/northd/northd.c
> index f1f1ede43..52bb67c0e 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -11079,7 +11079,7 @@ parsed_route_free(struct parsed_route *pr) {
> * in there.
> * Takes ownership of the provided nexthop. All other parameters are cloned.
> * Elements of the routes hmap need to be freed using parsed_route_free. */
> -void
> +struct parsed_route *
> parsed_route_add(const struct ovn_datapath *od,
> struct in6_addr *nexthop,
> const struct in6_addr *prefix,
> @@ -11108,9 +11108,11 @@ parsed_route_add(const struct ovn_datapath *od,
> struct parsed_route *pr = parsed_route_lookup(routes, hash, new_pr);
> if (!pr) {
> hmap_insert(routes, &new_pr->key_node, hash);
> + return new_pr;
> } else {
> pr->stale = false;
> parsed_route_free(new_pr);
> + return pr;
> }
> }
>
> @@ -19116,6 +19118,7 @@ routes_destroy(struct routes_data *data)
>
> simap_destroy(&data->route_tables);
> __bfd_destroy(&data->bfd_active_connections);
> +
Nit: unrelated change.
> }
>
> void
> diff --git a/northd/northd.h b/northd/northd.h
> index 1fbc93c8e..9402e4a4b 100644
> --- a/northd/northd.h
> +++ b/northd/northd.h
> @@ -783,21 +783,22 @@ struct parsed_route *parsed_route_clone(const struct
> parsed_route *);
> size_t parsed_route_hash(const struct parsed_route *);
> void parsed_route_free(struct parsed_route *);
>
> -void parsed_route_add(const struct ovn_datapath *od,
> - struct in6_addr *nexthop,
> - const struct in6_addr *prefix,
> - unsigned int plen,
> - bool is_discard_route,
> - const char *lrp_addr_s,
> - const struct ovn_port *out_port,
> - uint32_t route_table_id,
> - bool is_src_route,
> - bool ecmp_symmetric_reply,
> - const struct sset *ecmp_selection_fields,
> - enum route_source source,
> - const struct ovsdb_idl_row *source_hint,
> - const struct ovn_port *tracked_port,
> - struct hmap *routes);
> +struct parsed_route * parsed_route_add(
Nit: should be:
struct parsed_route *parsed_route_add(
> + const struct ovn_datapath *od,
> + struct in6_addr *nexthop,
> + const struct in6_addr *prefix,
> + unsigned int plen,
> + bool is_discard_route,
> + const char *lrp_addr_s,
> + const struct ovn_port *out_port,
> + uint32_t route_table_id,
> + bool is_src_route,
> + bool ecmp_symmetric_reply,
> + const struct sset *ecmp_selection_fields,
> + enum route_source source,
> + const struct ovsdb_idl_row *source_hint,
> + const struct ovn_port *tracked_port,
> + struct hmap *routes);
>
> bool
> find_route_outport(const struct hmap *lr_ports, const char *output_port,
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 6a58ccc27..bb77f2abc 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -15324,8 +15324,8 @@ ovn-sbctl dump-flows lr0 > lr0flows
> AT_CHECK([grep -w "lr_in_ip_routing" lr0flows | ovn_strip_lflows], [0], [dnl
> table=??(lr_in_ip_routing ), priority=0 , match=(1), action=(drop;)
> table=??(lr_in_ip_routing ), priority=10550, match=(nd_rs || nd_ra),
> action=(drop;)
> - table=??(lr_in_ip_routing ), priority=194 , match=(reg7 == 0 && ip4.dst
> == 192.168.1.0/24), action=(ip.ttl--; flags.loopback = 1; reg8[[0..15]] = 1;
> reg8[[16..31]] = select(1, 2);)
> - table=??(lr_in_ip_routing ), priority=196 , match=(reg7 == 0 && ip4.dst
> == 192.168.0.0/24), action=(ip.ttl--; flags.loopback = 1; reg8[[0..15]] = 2;
> reg8[[16..31]] = select(1, 2);)
> + table=??(lr_in_ip_routing ), priority=194 , match=(reg7 == 0 && ip4.dst
> == 192.168.1.0/24), action=(ip.ttl--; flags.loopback = 1; reg8[[0..15]] = 2;
> reg8[[16..31]] = select(1, 2);)
> + table=??(lr_in_ip_routing ), priority=196 , match=(reg7 == 0 && ip4.dst
> == 192.168.0.0/24), action=(ip.ttl--; flags.loopback = 1; reg8[[0..15]] = 1;
> reg8[[16..31]] = select(1, 2);)
> table=??(lr_in_ip_routing ), priority=198 , match=(ip4.dst ==
> 10.0.0.0/24), action=(ip.ttl--; reg8[[0..15]] = 0; reg0 = ip4.dst; reg5 =
> 10.0.0.1; eth.src = 00:00:00:00:ff:01; outport = "lr0-sw0"; flags.loopback =
> 1; reg9[[9]] = 1; next;)
> table=??(lr_in_ip_routing ), priority=198 , match=(ip4.dst ==
> 10.0.1.0/24), action=(ip.ttl--; reg8[[0..15]] = 0; reg0 = ip4.dst; reg5 =
> 10.0.1.1; eth.src = 00:00:00:00:ff:02; outport = "lr0-sw1"; flags.loopback =
> 1; reg9[[9]] = 1; next;)
> table=??(lr_in_ip_routing ), priority=518 , match=(inport == "lr0-sw0"
> && ip6.dst == fe80::/64), action=(ip.ttl--; reg8[[0..15]] = 0; xxreg0 =
> ip6.dst; xxreg1 = fe80::200:ff:fe00:ff01; eth.src = 00:00:00:00:ff:01;
> outport = "lr0-sw0"; flags.loopback = 1; reg9[[9]] = 0; next;)
> @@ -15333,10 +15333,10 @@ AT_CHECK([grep -w "lr_in_ip_routing" lr0flows |
> ovn_strip_lflows], [0], [dnl
> ])
> AT_CHECK([grep -e "lr_in_ip_routing_ecmp" lr0flows | sed -e
> 's/10\.0\..\./10.0.??./g' -e 's/lr0-sw./lr0-sw??/' -e 's/00:ff:0./00:ff:0?/'
> | ovn_strip_lflows], [0], [dnl
> table=??(lr_in_ip_routing_ecmp), priority=0 , match=(1), action=(drop;)
> - table=??(lr_in_ip_routing_ecmp), priority=100 , match=(reg8[[0..15]] == 1
> && reg8[[16..31]] == 1), action=(reg0 = 10.0.??.20; reg5 = 10.0.??.1; eth.src
> = 00:00:00:00:ff:0?; outport = "lr0-sw??"; reg9[[9]] = 1; next;)
> - table=??(lr_in_ip_routing_ecmp), priority=100 , match=(reg8[[0..15]] == 1
> && reg8[[16..31]] == 2), action=(reg0 = 10.0.??.20; reg5 = 10.0.??.1; eth.src
> = 00:00:00:00:ff:0?; outport = "lr0-sw??"; reg9[[9]] = 1; next;)
> - table=??(lr_in_ip_routing_ecmp), priority=100 , match=(reg8[[0..15]] == 2
> && reg8[[16..31]] == 1), action=(reg0 = 10.0.??.10; reg5 = 10.0.??.1; eth.src
> = 00:00:00:00:ff:0?; outport = "lr0-sw??"; reg9[[9]] = 1; next;)
> - table=??(lr_in_ip_routing_ecmp), priority=100 , match=(reg8[[0..15]] == 2
> && reg8[[16..31]] == 2), action=(reg0 = 10.0.??.10; reg5 = 10.0.??.1; eth.src
> = 00:00:00:00:ff:0?; outport = "lr0-sw??"; reg9[[9]] = 1; next;)
> + table=??(lr_in_ip_routing_ecmp), priority=100 , match=(reg8[[0..15]] == 1
> && reg8[[16..31]] == 1), action=(reg0 = 10.0.??.10; reg5 = 10.0.??.1; eth.src
> = 00:00:00:00:ff:0?; outport = "lr0-sw??"; reg9[[9]] = 1; next;)
> + table=??(lr_in_ip_routing_ecmp), priority=100 , match=(reg8[[0..15]] == 1
> && reg8[[16..31]] == 2), action=(reg0 = 10.0.??.10; reg5 = 10.0.??.1; eth.src
> = 00:00:00:00:ff:0?; outport = "lr0-sw??"; reg9[[9]] = 1; next;)
> + table=??(lr_in_ip_routing_ecmp), priority=100 , match=(reg8[[0..15]] == 2
> && reg8[[16..31]] == 1), action=(reg0 = 10.0.??.20; reg5 = 10.0.??.1; eth.src
> = 00:00:00:00:ff:0?; outport = "lr0-sw??"; reg9[[9]] = 1; next;)
> + table=??(lr_in_ip_routing_ecmp), priority=100 , match=(reg8[[0..15]] == 2
> && reg8[[16..31]] == 2), action=(reg0 = 10.0.??.20; reg5 = 10.0.??.1; eth.src
> = 00:00:00:00:ff:0?; outport = "lr0-sw??"; reg9[[9]] = 1; next;)
> table=??(lr_in_ip_routing_ecmp), priority=150 , match=(reg8[[0..15]] ==
> 0), action=(next;)
> ])
>
> @@ -15630,7 +15630,7 @@ check ovn-nbctl --wait=sb sync
> check_engine_compute northd unchanged
> check_engine_compute routes unchanged
> check_engine_compute advertised_route_sync unchanged
> -check_engine_compute learned_route_sync recompute
> +check_engine_compute learned_route_sync incremental
> check_engine_compute lflow recompute
> CHECK_NO_CHANGE_AFTER_RECOMPUTE
>
> @@ -15640,7 +15640,7 @@ check ovn-nbctl --wait=sb sync
> check_engine_compute northd unchanged
> check_engine_compute routes unchanged
> check_engine_compute advertised_route_sync unchanged
> -check_engine_compute learned_route_sync recompute
> +check_engine_compute learned_route_sync incremental
> check_engine_compute lflow recompute
> CHECK_NO_CHANGE_AFTER_RECOMPUTE
>
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
