On 2/11/25 2:12 AM, 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.
>
> Signed-off-by: Martin Kalcok <[email protected]>
> ---
Hi Martin,
This is in the right direction but it's not exactly what I initially had
in mind. :)
I'll paste the dependency diagram I was aiming for here too:
en_northd
|
+-------------+------------+-------------+----+
| | | | |
en_routes en_lr_nat en_lb_data | |
| | | | | |
| | | | | |
| | +------------+-------------+ |
| | | |
| | | |
| | en_lr_stateful |
| | | |
| | + +-------------+
| | | |
| | en_dynamic_routes
| | |
| | |
+---+ +---------+------------+
| |
| |
en_lflow en_advertised_route_sync
With this the only incremental processing node that writes
SB.Advertised_Route records to the database should be
en_advertised_route_sync.
It should have two input route maps:
- one from en_routes
- one from en_dynamic_routes
That also solves one of the problems with syncing that you listed in the
patch (see below).
> lib/stopwatch-names.h | 1 +
> northd/en-advertised-route-sync.c | 84 +++++++++++++++++++++++++++++++
> 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 | 9 ++++
> 8 files changed, 154 insertions(+)
>
> 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..e526706e0 100644
> --- a/northd/en-advertised-route-sync.c
> +++ b/northd/en-advertised-route-sync.c
> @@ -25,6 +25,7 @@
> #include "openvswitch/hmap.h"
> #include "ovn-util.h"
>
> +
> static void
> advertised_route_table_sync(
> struct ovsdb_idl_txn *ovnsb_txn,
> @@ -159,6 +160,76 @@ en_advertised_route_sync_run(struct engine_node *node,
> void *data OVS_UNUSED)
> 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 hmap *data = xzalloc(sizeof *data);
I'd prefer if we have a more explicit node data structure, e.g.:
struct dynamic_routes_data {
struct hmap parsed_routes; /* Stores struct parsed_route. */
};
> +
> + hmap_init(data);
> + return data;
> +}
> +
> +void
> +en_dynamic_routes_cleanup(void *data OVS_UNUSED)
> +{
> + hmap_destroy(data);
> +}
> +
> +void
> +en_dynamic_routes_run(struct engine_node *node, void *data)
> +{
> + struct hmap *parsed_routes = 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,
> parsed_routes);
> +
> + }
> + 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));
> +
> + stopwatch_start(DYNAMIC_ROUTES_RUN_STOPWATCH_NAME, time_msec());
> +
> + /* XXX: The last NULL arg feels bit sketchy. advertised_route_table_sync
> + * is using it only it when creating connected host routes. Should I
> + * create function similar to advertised_route_table_sync that would
> + * handle only nat/lb routes, or would a null-check be enough?*/
> + advertised_route_table_sync(eng_ctx->ovnsb_idl_txn,
> + sbrec_advertised_route_table,
> + &lr_stateful_data->table,
> + parsed_routes,
> + NULL);
This I-P node (en_dynamic_routes) should not write to the SB database.
Instead its "output" data should be a map of parsed_route built
containing all dynamic (NAT/LB) routes we want to advertise.
> + stopwatch_stop(DYNAMIC_ROUTES_RUN_STOPWATCH_NAME, time_msec());
> + 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. */
We could do this in the following way:
In the dynamic_routes_data structure store the set of routers with
dynamic routing enabled (and what they redistribute).
In this handler we could check if the lr_stateful trk_data changed
lr_stateful_records (these are one per router) ovn datapaths have a
different dynamic routing configuration than what we had in our current
dynamic_routes_data. If that's the case, return false, trigger
recompute. This should be acceptable because we don't expect a lot of
changes in the dynamic routing configuration.
However, for now, we don't really do much incremental processing for
dynamic routing in general (Felix's series doesn't do that either) so
it's fine to just always return false like you did here. We should,
however tackle the incremental processing for dynamic routing in 25.09.
> + return false;
> +}
> +
> +
> struct ar_entry {
> struct hmap_node hmap_node;
>
> @@ -381,6 +452,9 @@ advertised_route_table_sync(
> if (route->source == ROUTE_SOURCE_STATIC && (drr & DRRM_STATIC) ==
> 0) {
> continue;
> }
> + if (route->source == ROUTE_SOURCE_NAT && (drr & DRRM_NAT) == 0) {
> + continue;
> + }
>
> char *ip_prefix = normalize_v46_prefix(&route->prefix, route->plen);
> route_e = ar_add_entry(&sync_routes, route->od->sb,
> @@ -388,6 +462,16 @@ advertised_route_table_sync(
> }
> uuidset_destroy(&host_route_lrps);
>
> + /* XXX: This cleanup loop proved to be problematic after splitting
> + * pipelines for connected and NAT/LB routes. The "parsed_routes"
> argument
> + * of this function needs to contain complete set of routes, because rest
> + * of them get cleaned up.
> + * My first idea was to turn "route_source enum" into bitmask and add
> + * another argument to this function that would specify which
> + * route_source(s) are being updated. However, since records in the
> + * Advertised_Route SB table don't carry information about their
> + * route source, this approach wouldn't work.
> + * I think this is currently my biggest blocker.*/
If you follow the dependency graph I pasted in the beginning of this
email you shouldn't have this problem anymore.
> 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..6f164bc0e 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)
> @@ -295,6 +296,11 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> engine_add_input(&en_advertised_route_sync, &en_northd,
> advertised_route_sync_northd_change_handler);
>
> + engine_add_input(&en_dynamic_routes, &en_lr_stateful,
> dynamic_routes_change_handler);
> + engine_add_input(&en_dynamic_routes, &en_sb_advertised_route,
> + NULL);
> + engine_add_input(&en_dynamic_routes, &en_northd, engine_noop_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_northd,
> @@ -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);
>
> 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".*/
This is exactly what I had in mind when I was reviewing v3.
tracked_port should be populated in the parsed_route when the route is
created (like you said, we have all the information we need at that point).
> + 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:
> 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..98351c243 100644
> --- a/northd/northd.h
> +++ b/northd/northd.h
> @@ -312,6 +312,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,
> };
>
> enum dynamic_routing_redistribute_mode {
> @@ -319,6 +321,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 +734,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 +819,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 *);
I went ahead and pushed an incremental change that applies on top of
this patch here:
https://github.com/dceara/ovn/commit/462dab62352ffe7fabb0f5f77255d8630cfc7b50
What it does is add the missing link between en_dynamic_routes and
en_advertised_routes_sync. It also solves the problem of having to
write to the SB from different I-P nodes.
If this looks OK to you, feel free to integrate it in v5. I'm going to
review Felix's "OVN Fabric integration." [v8] series next and hopefully
I can apply it to main. If that's the case, you can probably drop the
RFC tag in your v5.
Also, if you face any issues, please let me know. I'll do my best to
help out so we get this feature supported in 25.03.
Thanks,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
