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.
>>>>
>>>> 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
