On 2/5/25 11:22 AM, Felix Huettner wrote:
> On Tue, Feb 04, 2025 at 05:41:51PM +0100, Dumitru Ceara wrote:
>> On 2/4/25 5:19 PM, Felix Huettner wrote:
>>> On Tue, Feb 04, 2025 at 11:23:08AM +0100, Dumitru Ceara wrote:
>>>> On 2/4/25 11:04 AM, Felix Huettner wrote:
>>>>> On Tue, Feb 04, 2025 at 10:35:25AM +0100, Felix Huettner via dev wrote:
>>>>>> On Mon, Feb 03, 2025 at 02:33:07PM +0100, Dumitru Ceara wrote:
>>>>>>> On 1/29/25 12:15 PM, Felix Huettner via dev wrote:
>>>>>>>> We now learn all routes inside the vrfs we also advertise routes on.
>>>>>>>> The routes are then placed in the southbound database for processing by
>>>>>>>> northd.
>>>>>>>>
>>>>>>>> Routes are only selected if matching the following rules:
>>>>>>>> 1. must not be a route advertised by us
>>>>>>>> 2. must not be a local connected route (as we want to not learn
>>>>>>>> transfer
>>>>>>>> networks)
>>>>>>>> 3. the prefix must not be a link local address
>>>>>>>>
>>>>>>>> However we can not reliably determine over which link we learned the
>>>>>>>> route in case we have two LRPs of the same LR on the same chassis.
>>>>>>>> For now we just assume the routes on both links are identical.
>>>>>>>> Future commits will refine this.
>>>>>>>>
>>>>>>>> Signed-off-by: Felix Huettner <felix.huettner@stackit.cloud>
>>>>>>>> ---
>>>>>>>
>>>>>>> Hi Felix,
>>>>>>>
>>>>>>> I have a few more (mostly minor) comments on this version.
>>>>>>
>>>>>> Hi Dumitru,
>>>>>>
>>>>>> thanks for the review.
>>>>>> The smaller things are addressed in the next version.
>>>>>>
>>>>>>>
>>>>>>>> v3->v4:
>>>>>>>> - addressed review comments.
>>>>>>>> v2->v3:
>>>>>>>> * Set monitor conditions on sb Learned_Route table.
>>>>>>>> * Do not learn routes if Learned_Route table does not exist
>>>>>>>> (upgrades).
>>>>>>>>
>>>>>>>> controller/ovn-controller.c | 64 +++++++++++--
>>>>>>>> controller/route-exchange-netlink.c | 38 +++++++-
>>>>>>>> controller/route-exchange-netlink.h | 15 ++-
>>>>>>>> controller/route-exchange.c | 138 +++++++++++++++++++++++++++-
>>>>>>>> controller/route-exchange.h | 3 +
>>>>>>>> lib/ovn-util.c | 10 ++
>>>>>>>> lib/ovn-util.h | 1 +
>>>>>>>> tests/system-ovn.at | 21 +++++
>>>>>>>> 8 files changed, 277 insertions(+), 13 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
>>>>>>>> index 1eb8d39d1..5b31f6fd2 100644
>>>>>>>> --- a/controller/ovn-controller.c
>>>>>>>> +++ b/controller/ovn-controller.c
>>>>>>>> @@ -233,7 +233,7 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
>>>>>>>> *
>>>>>>>> * Monitor Template_Var for local chassis.
>>>>>>>> *
>>>>>>>> - * Monitor Advertised_Route for local datapaths.
>>>>>>>> + * Monitor Advertised/Learned_Route for local datapaths.
>>>>>>>> *
>>>>>>>> * We always monitor patch ports because they allow us to see the
>>>>>>>> linkages
>>>>>>>> * between related logical datapaths. That way, when we know
>>>>>>>> that we have
>>>>>>>> @@ -252,6 +252,7 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
>>>>>>>> struct ovsdb_idl_condition chprv =
>>>>>>>> OVSDB_IDL_CONDITION_INIT(&chprv);
>>>>>>>> struct ovsdb_idl_condition tv = OVSDB_IDL_CONDITION_INIT(&tv);
>>>>>>>> struct ovsdb_idl_condition ar = OVSDB_IDL_CONDITION_INIT(&ar);
>>>>>>>> + struct ovsdb_idl_condition lr = OVSDB_IDL_CONDITION_INIT(&lr);
>>>>>>>>
>>>>>>>> /* Always monitor all logical datapath groups. Otherwise, DPG
>>>>>>>> updates may
>>>>>>>> * be received *after* the lflows using it are seen by
>>>>>>>> ovn-controller.
>>>>>>>> @@ -277,6 +278,7 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
>>>>>>>> ovsdb_idl_condition_add_clause_true(&chprv);
>>>>>>>> ovsdb_idl_condition_add_clause_true(&tv);
>>>>>>>> ovsdb_idl_condition_add_clause_true(&ar);
>>>>>>>> + ovsdb_idl_condition_add_clause_true(&lr);
>>>>>>>
>>>>>>> Same comment as for advertised routes in the previous patch. We might
>>>>>>> be able to move this under if (!chassis) {...}.
>>>>>
>>>>> Hi Dumitru,
>>>>>
>>>>> actually we can not do this (but i just noticed that).
>>>>>
>>>>> Assume we get a new LRP on a chassis of a running ovn-controller. That
>>>>> LRP belongs to a LR that has route-exchange set. The VRF belonging to
>>>>> that LR has already been created on the chassis and contains routes that
>>>>> ovn-controller should learn.
>>>>>
>>>>> In this case ovn-controller will in one iteration (at least if i
>>>>> understood it correctly):
>>>>> 1. claim the port
>>>>> 2. add the LR to local_datapaths
>>>>> 3. try to learn routes from the VRF
>>>>> 4. update monitoring conditions
>>>>>
>>>>> If we do only monitor learned routes for all local_datapaths then at the
>>>>> point where we learn the routes we did not yet call update_sb_monitors.
>>>>> So we would try to add a entry to Learned_Route that already exists
>>>>> there.
>>>>>
>>>>> In my understanding the options are:
>>>>> 1. monitor all Learned_Route entries
>>>>> 2. only try to learn routes after the monitoring condition has been
>>>>> updated.
>>>>>
>>>>> If you would prefer option 2, i would need some hint how to know if we
>>>>> have a monitoring condition set.
>>>>
>>>> We could decide to switch from monitoring all Learned_Routes to
>>>> monitoring a subset based on daemon_started_recently(). We do something
>>>> similar and delay deleting patch ports as long as ovn-controller has
>>>> "recently started" in the hope that we won't have to re-add them soon.
>>>>
>>>> Could that work?
>>>
>>> Hi Dumitru,
>>>
>>> i think that would only solve the issue on the startup of
>>> ovn-controller.
>>>
>>> But even later during runtime the same thing could happen if we get a
>>> new local_datapath and it already has its vrf filled with learnable
>>> routes.
>>> In this case i think we would update the monitoring condition after we
>>> would try to insert the routes to the southbound.
>>>
>>> So if we would want to use something similar to daemon_started_recently
>>> then i guess we would need that for each local datapath. Where we only
>>> start learning routes for this datapath once the monitoring condition
>>> has had sufficiently long time to update.
>>>
>>> But that still seems to be less safe than just monitoring everything.
>>>
>>
>> I agree, it sounds complicated. Let's monitor everything for now but
>> let's add a TODO.rst item and an "xxx: " comment for this. I'm worried
>> that if the SB table has a lot of records we waste bandwidth/memory/cpu
>> for (mostly) nothing.
>
> Hi Dumitru,
>
> sounds good. I'll add it in the next version.
>
> I would send out the next version tomorrow morning with all changes that
> accumulated until then, if that is ok.
>
Sure, I'll try to have another look at v6 today though.
Thanks,
Dumitru
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
