On 2/3/25 2:29 PM, Felix Huettner wrote: > On Fri, Jan 31, 2025 at 11:11:09AM +0100, Dumitru Ceara wrote: >> Hi Felix, >> >> On 1/21/25 4:47 PM, Felix Huettner via dev wrote: >>> Here we expand the previous routes-sync engine node to not only >>> advertise routes to the southbound table, but also learn received routes >>> from this table. >>> >>> These routes are then passed to the same logic that connected and static >>> routes are using for flow generation. >>> However we prioritize these routes lower than connected or static routes >>> as information in cluster (for the same prefix length) should always be >>> more correct then learned routes. >>> This is also consistent with the behaviour of phyiscal routers. >> >> Nit: I'd change this to "with the default behaviour of physical >> routers". As discussed on the previous version, traditional routers >> often allow users to change the default administrative distances. > > Hi Dumitru, >
Hi Felix, > thanks a lot for the review. > The topics will be addressed in the next version, except for one where i > am unsure how to proceed. > >> >>> >>> Signed-off-by: Felix Huettner <[email protected]> >>> --- >>> v2->v3: >>> * A lot of minor review comments. >>> * Support learning routes over other address families >>> >>> NEWS | 4 + >>> lib/stopwatch-names.h | 1 + >>> northd/automake.mk | 2 + >>> northd/en-learned-route-sync.c | 214 ++++++++++++++++++++++++++++ >>> northd/en-learned-route-sync.h | 33 +++++ >>> northd/en-lflow.c | 5 +- >>> northd/inc-proc-northd.c | 14 +- >>> northd/northd.c | 239 ++++++++++++++++++------------- >>> northd/northd.h | 28 +++- >>> northd/ovn-northd.c | 1 + >>> tests/ovn-northd.at | 253 ++++++++++++++++++++++++++++----- >>> 11 files changed, 661 insertions(+), 133 deletions(-) >>> create mode 100644 northd/en-learned-route-sync.c >>> create mode 100644 northd/en-learned-route-sync.h >>> [...] >>> + >>> +static void >>> +routes_table_sync( >>> + const struct sbrec_learned_route_table *sbrec_learned_route_table, >>> + const struct hmap *parsed_routes, >>> + const struct hmap *lr_ports, >>> + const struct ovn_datapaths *lr_datapaths, >>> + struct hmap *parsed_routes_out) >>> +{ >>> + struct hmap sync_routes = HMAP_INITIALIZER(&sync_routes); >>> + >> >> We don't use 'sync_routes' anywhere, it can be removed. >> >>> + const struct parsed_route *route; >>> + >>> + const struct sbrec_learned_route *sb_route; >>> + SBREC_LEARNED_ROUTE_TABLE_FOR_EACH (sb_route, >>> sbrec_learned_route_table) { >>> + parse_route_from_sbrec_route(parsed_routes_out, lr_ports, >>> + &lr_datapaths->datapaths, >>> + sb_route); >>> + >>> + } >>> + >>> + HMAP_FOR_EACH (route, key_node, parsed_routes) { >>> + hmap_insert(parsed_routes_out, >>> &parsed_route_clone(route)->key_node, >>> + parsed_route_hash(route)); >> >> Should we be extra careful here and check if a duplicate already exists >> in parsed_routes_out()? I guess we don't ever expect one to be there so >> we could log a warning if that's the case, what do you think? > > Do you mean duplicate in the sense of the prefix + plen? I think we can not do > that here, since parsed_routes can already contain multiple routes with > the same prefix for ECMP. > Or do you mean something else for "duplicate"? > I meant: is there a chance that parsed_route_lookup(parsed_routes_out, parsed_route_clone(route)) will ever return true? I assume not but I wanted to double check. >> >>> + } >>> + >>> + hmap_destroy(&sync_routes); >>> +} >>> + >> >> One newline too many here. >> >>> diff --git a/northd/en-learned-route-sync.h b/northd/en-learned-route-sync.h >>> new file mode 100644 >>> index 000000000..0d007e932 >>> --- /dev/null >>> +++ b/northd/en-learned-route-sync.h >>> @@ -0,0 +1,33 @@ >>> +/* >>> + * Copyright (c) 2024, STACKIT GmbH & Co. KG >> >> Nit: 2025 >> >>> + * >>> + * Licensed under the Apache License, Version 2.0 (the "License"); >>> + * you may not use this file except in compliance with the License. >>> + * You may obtain a copy of the License at: >>> + * >>> + * http://www.apache.org/licenses/LICENSE-2.0 >>> + * >>> + * Unless required by applicable law or agreed to in writing, software >>> + * distributed under the License is distributed on an "AS IS" BASIS, >>> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. >>> + * See the License for the specific language governing permissions and >>> + * limitations under the License. >>> + */ >>> +#ifndef EN_LEARNED_ROUTE_SYNC_H >>> +#define EN_LEARNED_ROUTE_SYNC_H 1 >>> + >>> +#include "lib/inc-proc-eng.h" >>> +#include "openvswitch/hmap.h" >>> + >>> +struct learned_route_sync_data { >>> + struct hmap parsed_routes; >>> +}; >>> + >>> +bool learned_route_sync_northd_change_handler(struct engine_node *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_run(struct engine_node *, void *data); >>> + >>> + >> >> Nit: no need for the second empty line. >> >>> +#endif /* EN_LEARNED_ROUTE_SYNC_H */ >>> diff --git a/northd/en-lflow.c b/northd/en-lflow.c >>> index fa1f0236d..62224eb63 100644 >>> --- a/northd/en-lflow.c >>> +++ b/northd/en-lflow.c >>> @@ -26,6 +26,7 @@ >>> #include "en-northd.h" >>> #include "en-meters.h" >>> #include "en-sampling-app.h" >>> +#include "en-learned-route-sync.h" >>> #include "lflow-mgr.h" >>> >>> #include "lib/inc-proc-eng.h" >>> @@ -46,6 +47,8 @@ lflow_get_input_data(struct engine_node *node, >>> engine_get_input_data("bfd_sync", node); >>> struct routes_data *routes_data = >>> engine_get_input_data("routes", node); >>> + struct learned_route_sync_data *learned_route_sync_data = >>> + engine_get_input_data("learned_route_sync", node); >>> struct route_policies_data *route_policies_data = >>> engine_get_input_data("route_policies", node); >>> struct port_group_data *pg_data = >>> @@ -82,7 +85,7 @@ lflow_get_input_data(struct engine_node *node, >>> lflow_input->lb_datapaths_map = &northd_data->lb_datapaths_map; >>> lflow_input->svc_monitor_map = &northd_data->svc_monitor_map; >>> lflow_input->bfd_ports = &bfd_sync_data->bfd_ports; >>> - lflow_input->parsed_routes = &routes_data->parsed_routes; >>> + lflow_input->parsed_routes = &learned_route_sync_data->parsed_routes; >>> lflow_input->route_tables = &routes_data->route_tables; >>> lflow_input->route_policies = &route_policies_data->route_policies; >>> >>> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c >>> index 12956a5de..5f5f36b45 100644 >>> --- a/northd/inc-proc-northd.c >>> +++ b/northd/inc-proc-northd.c >>> @@ -42,6 +42,7 @@ >>> #include "en-sync-sb.h" >>> #include "en-sync-from-sb.h" >>> #include "en-advertised-route-sync.h" >>> +#include "en-learned-route-sync.h" >>> #include "unixctl.h" >>> #include "util.h" >>> >>> @@ -104,7 +105,8 @@ static unixctl_cb_func chassis_features_list; >>> SB_NODE(static_mac_binding, "static_mac_binding") \ >>> SB_NODE(chassis_template_var, "chassis_template_var") \ >>> SB_NODE(logical_dp_group, "logical_dp_group") \ >>> - SB_NODE(advertised_route, "advertised_route") >>> + SB_NODE(advertised_route, "advertised_route") \ >>> + SB_NODE(learned_route, "learned_route") >>> >>> enum sb_engine_node { >>> #define SB_NODE(NAME, NAME_STR) SB_##NAME, >>> @@ -164,6 +166,7 @@ static ENGINE_NODE(routes, "routes"); >>> static ENGINE_NODE(bfd, "bfd"); >>> static ENGINE_NODE(bfd_sync, "bfd_sync"); >>> static ENGINE_NODE(advertised_route_sync, "advertised_route_sync"); >>> +static ENGINE_NODE(learned_route_sync, "learned_route_sync"); >>> >>> void inc_proc_northd_init(struct ovsdb_idl_loop *nb, >>> struct ovsdb_idl_loop *sb) >>> @@ -270,6 +273,11 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, >>> engine_add_input(&en_advertised_route_sync, &en_sb_advertised_route, >>> NULL); >>> >>> + 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, >>> + learned_route_sync_northd_change_handler); >>> + >>> engine_add_input(&en_sync_meters, &en_nb_acl, NULL); >>> engine_add_input(&en_sync_meters, &en_nb_meter, NULL); >>> engine_add_input(&en_sync_meters, &en_sb_meter, NULL); >>> @@ -283,6 +291,10 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, >>> engine_add_input(&en_lflow, &en_bfd_sync, NULL); >>> engine_add_input(&en_lflow, &en_route_policies, NULL); >>> engine_add_input(&en_lflow, &en_routes, NULL); >>> + /* XXX: This causes a full lflow recompute on each change to any route. >>> + * At least for learned routes we should add incremental processing >>> here. >>> + * */ >>> + engine_add_input(&en_lflow, &en_learned_route_sync, NULL); >>> engine_add_input(&en_lflow, &en_global_config, >>> node_global_config_handler); >>> >>> diff --git a/northd/northd.c b/northd/northd.c >>> index 06fcb0bde..0eff92125 100644 >>> --- a/northd/northd.c >>> +++ b/northd/northd.c >>> @@ -302,11 +302,14 @@ BUILD_ASSERT_DECL(ACL_OBS_STAGE_MAX < (1 << 2)); >>> /* >>> * Route offsets implement logic to prioritize traffic for routes with >>> * same ip_prefix values: >>> - * - connected route overrides static one; >>> - * - static route overrides src-ip route. */ >>> -#define ROUTE_PRIO_OFFSET_MULTIPLIER 5 >>> -#define ROUTE_PRIO_OFFSET_STATIC 2 >>> -#define ROUTE_PRIO_OFFSET_CONNECTED 4 >>> + * 1. (highest priority) connected routes >>> + * 2. static routes >>> + * 3. routes learned from the outside via ovn-controller (e.g. bgp) >>> + * 4. (lowest priority) src-ip routes */ >>> +#define ROUTE_PRIO_OFFSET_MULTIPLIER 8 >>> +#define ROUTE_PRIO_OFFSET_LEARNED 2 >>> +#define ROUTE_PRIO_OFFSET_STATIC 4 >>> +#define ROUTE_PRIO_OFFSET_CONNECTED 6 >>> >>> /* Returns the type of the datapath to which a flow with the given 'stage' >>> may >>> * be added. */ >>> @@ -11134,7 +11137,7 @@ build_route_table_lflow(struct ovn_datapath *od, >>> struct lflow_table *lflows, >>> } >>> >>> static uint32_t >>> -route_hash(struct parsed_route *route) >>> +route_hash(const struct parsed_route *route) >>> { >>> return hash_bytes(&route->prefix, sizeof route->prefix, >>> (uint32_t)route->plen); >>> @@ -11185,7 +11188,7 @@ parsed_route_lookup(struct hmap *routes, size_t >>> hash, >>> continue; >>> } >>> >>> - if (pr->route != new_pr->route) { >>> + if (pr->source_hint != new_pr->source_hint) { >>> continue; >>> } >>> >>> @@ -11212,15 +11215,44 @@ parsed_route_lookup(struct hmap *routes, size_t >>> hash, >>> return NULL; >>> } >>> >>> -static void >>> +struct parsed_route *parsed_route_clone(const struct parsed_route *pr) { >>> + struct parsed_route *new_pr = xzalloc(sizeof *new_pr); >> >> Nit: newline here would make it more readable IMO. >> >>> + new_pr->prefix = pr->prefix; >>> + new_pr->plen = pr->plen; >>> + if (pr->nexthop) { >>> + new_pr->nexthop = xmemdup(pr->nexthop, sizeof *pr->nexthop); >>> + } >>> + new_pr->route_table_id = pr->route_table_id; >>> + new_pr->is_src_route = pr->is_src_route; >>> + new_pr->hash = route_hash(pr); >> >> This is not the same hash as parsed_route_hash(). We use the latter >> when inserting in the routes map in parsed_route_add(). While we're at >> it, parsed_route_hash() is wrong: >> >> size_t parsed_route_hash(const struct parsed_route *pr) { >> return uuid_hash(&pr->od->key); >> } >> >> This uses the datapath uuid as hash for any route in that datapath. >> That means all routes for a given datapath will hash to the same bucket >> creating long collision lists. >> >> Shouldn't we just use route_hash() everywhere? > > In build_route_flows_for_lrouter we seem to use the hash of the datapath > to find all routes associated with that datapath. > > Iterating through all routes to find the appropriate datapath feels > worse to me than having long collision lists, since we would then need > to run through all routes of the entire deployment once for each LR. > Ah, I missed that part in build_route_flows_for_lrouter(). OK. > So it seems that route->hash should be a good hash over the route based > on prefix. It is mainly used in efficiently building ecmp groups. > However it is not the hash used for entries in "struct hmap *parsed_routes" > as this is searched by datapath and not by prefix. > > While it is confusing the general approach sounds reasonable to me. I am > just not sure how to make this understandable. > > I'll add a comment to parsed_route_hash, maybe that helps. > OK, let's try that for now. >> >> And for cloning we can just copy the hash value, we don't need to >> recompute it. >> >>> + new_pr->ecmp_symmetric_reply = pr->ecmp_symmetric_reply; >>> + new_pr->is_discard_route = pr->is_discard_route; >>> + new_pr->od = pr->od; >>> + new_pr->stale = pr->stale; >>> + new_pr->source = pr->source; >>> + new_pr->source_hint = pr->source_hint; >>> + new_pr->lrp_addr_s = nullable_xstrdup(pr->lrp_addr_s); >>> + new_pr->out_port = pr->out_port; >>> + sset_clone(&new_pr->ecmp_selection_fields, &pr->ecmp_selection_fields); >> >> This whole block is almost identical to what we do in >> parsed_route_add(). Let's factor initialization of a parsed route into >> an internal parsed_route_init() function that we call in both places? >> What do you think? > > Yep looks nicer now. > > Thanks a lot, > Felix > Thanks, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
