On Thu, 2025-02-06 at 22:06 +0100, Dumitru Ceara wrote: > On 2/6/25 6:46 PM, Dumitru Ceara wrote: > > 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. > > On a closer look, we don't need these two direct dependencies. There > are already the following dependencies: > > en_lb_data -> en_lr_stateful > en_lr_nat -> en_lr_stateful > > and: > > en_lr_stateful -> en_routes > > So every time a NAT or LB is changed en_routes gets updated but see > below. > > > > > Thanks, > > Dumitru > > > > > Thanks, > > > Martin. > > > > > > > > > > > > + engine_add_input(&en_routes, &en_lr_stateful, > > > > > engine_noop_handler); > > The actual problem is here. We use a "no-op" handler so we just > ignore > those updates. > > But the data in the en_lr_stateful node is quite useful: > > struct lr_stateful_tracked_data { > /* Created or updated logical router with LB and/or NAT data. */ > struct hmapx crupdated; /* Stores 'struct lr_stateful_record'. */ > }; > > struct ed_type_lr_stateful { > struct lr_stateful_table table; > > /* Node's tracked data. */ > struct lr_stateful_tracked_data trk_data; > }; > > So a first try of incremental processing could be to write a handler > (instead of the no-op one) that: > > - for each lr_stateful_record (per router) in lr_stateful.trk_data: > // these are the routers whose NAT or LB config changed > - if lr_stateful_record.has_dynamic_routing: > // we need to store this info > return false // we don't have I-P yet and > // this needs route updates > else > continue > > Maybe instead of adding a "has_dynamic_routing" field we should call: > ovn_datapaths_find_by_index(northd_data->lr_datapaths, > lr_stateful_record.lr_index); > > It sounds complicated but I think it's actually not that much work. > en_routes already depends on en_northd so we can extract the > northd_data->lr_datapaths. >
Thank you for details, I'll work that in. Martin. > > > > > > > > > > engine_add_input(&en_bfd_sync, &en_bfd, NULL); > > > > > engine_add_input(&en_bfd_sync, &en_nb_bfd, NULL); > > > > > > Thanks, > Dumitru > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
