On 2/3/25 3:59 PM, Felix Huettner wrote: > 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. >
OK, let's leave it as is for now. Thanks, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
