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