On Mon, Feb 03, 2025 at 02:40:28PM +0100, Dumitru Ceara wrote: > 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,
Hi Dumitru, > > > 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. I would also say no. "parsed_routes" is populated by the en_routes which uses parsed_route_add. So whatever is in this hmap is already deduplicated. Since parsed_route_lookup also takes pr->source into account it can also not conflict with the learned routes since they have a different source. So from my perspective we are safe here. > > >> > >>> + } > >>> + > >>> + 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. Thanks a lot, Felix > > >> > >> 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
