On 2/6/25 6:40 PM, [email protected] wrote: > On Thu, 2025-02-06 at 18:02 +0100, Dumitru Ceara wrote: >> On 2/5/25 10:21 AM, Martin Kalcok wrote: >>> This change builds on top of the new "dynamic routing" OVN feature >>> that allows advertising routes to the fabric network. When LR >>> option >>> "dynamic-routing" is set on the router, following two new LRP >>> options >>> become available: >>> >>> * dynamic-routing-nat - When set to "true", ovn-controller will >>> advertise >>> routes for external NAT IPs valid for the >>> LRP. >>> * dynamic-routing-lb-vips - When set to "true", ovn-controller will >>> advertise >>> host routes to LB VIPs via the LRP. >>> >>> Co-authored-by: Frode Nordahl <[email protected]> >>> Signed-off-by: Frode Nordahl <[email protected]> >>> Signed-off-by: Martin Kalcok <[email protected]> >>> --- >>> NEWS | 17 + >>> northd/en-advertised-route-sync.c | 198 +++++++++++ >>> northd/en-northd.c | 5 +- >>> northd/inc-proc-northd.c | 5 + >>> northd/northd.c | 134 ++++++- >>> northd/northd.h | 8 +- >>> ovn-nb.xml | 32 ++ >>> ovs | 2 +- >>> tests/ovn-northd.at | 556 >>> ++++++++++++++++++++++++++++++ >>> tests/system-ovn.at | 379 ++++++++++++++++++++ >>> 10 files changed, 1332 insertions(+), 4 deletions(-) >> >> Hi Martin, >> >> Not a full review but I'll drop this here for now (and continue >> reviewing). >> >>> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c >>> index 86b7b999e..d74c2e3dc 100644 >>> --- a/northd/inc-proc-northd.c >>> +++ b/northd/inc-proc-northd.c >>> @@ -267,6 +267,11 @@ void inc_proc_northd_init(struct >>> ovsdb_idl_loop *nb, >>> engine_add_input(&en_routes, &en_bfd, NULL); >>> engine_add_input(&en_routes, &en_northd, >>> routes_northd_change_handler); >>> + engine_add_input(&en_routes, &en_lr_nat, >>> + NULL); >> >> This means we trigger a recompute of en_routes every time a nat is >> changed which in turn triggers a recompute of the lflow node. I >> think >> we should add a handler that at least restricts it to recompute if >> changed nats/lbs are part of routers that have dynamic routing >> enabled. >> >> We can refine it further later and add real incremental processing >> (routes per datapath) but for now we shouldn't affect >> non-dynamic-routing deployment performance. > > ack, I'll refine it. > >> >>> + engine_add_input(&en_routes, &en_lb_data, >>> + NULL); >> >> Do we need this dependency? I don't think we use en_lb_data in the >> en_routes node anywhere. > > maybe I misused it, my intention was to trigger en_routes_run when LB > data changes. >
Ah, ok, you're right. But then we should probably do the same thing as for NATs, i.e., only "fail" to incrementally process load balancer data if the load balancers are applied to routers that are involved in dynamic routing. Thanks, Dumitru > Thanks, > Martin. > >> >>> + engine_add_input(&en_routes, &en_lr_stateful, >>> engine_noop_handler); >>> >>> engine_add_input(&en_bfd_sync, &en_bfd, NULL); >>> engine_add_input(&en_bfd_sync, &en_nb_bfd, NULL); >> >> Regards, >> Dumitru >> >> > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
