On 9/1/25 9:15 PM, Dumitru Ceara wrote:
> On 9/1/25 5:13 PM, Ilya Maximets wrote:
>> On 9/1/25 4:48 PM, Dumitru Ceara wrote:
>>> On 9/1/25 4:25 PM, Ilya Maximets wrote:
>>>> On 9/1/25 4:19 PM, Dumitru Ceara wrote:
>>>>> On 9/1/25 4:17 PM, Dumitru Ceara wrote:
>>>>>> On 9/1/25 4:12 PM, Ilya Maximets wrote:
>>>>>>> On 9/1/25 3:33 PM, Dumitru Ceara wrote:
>>>>>>>> Hi Ilya,
>>>>>>>>
>>>>>>>> Thanks for the review!
>>>>>>>>
>>>>>>>> On 9/1/25 3:23 PM, Ilya Maximets wrote:
>>>>>>>>> On 9/1/25 1:22 PM, Dumitru Ceara wrote:
>>>>>>>>>> Since 6919992d8781 ("Datapath_Binding: Separate type column and sync
>>>>>>>>>> NB.UUID to SB."), a new type column is added to SB datapaths to
>>>>>>>>>> differentiate between datapath types (switch vs router). The same
>>>>>>>>>> commit also changed northd such that newly created SB datapaths have
>>>>>>>>>> as
>>>>>>>>>> UUID the same value as the NB switch/router UUID. All SB.Datapath
>>>>>>>>>> records were also updated and their 'type' field populated
>>>>>>>>>>
>>>>>>>>>> It also added a helper function, datapath_get_nb_uuid_and_type(), to
>>>>>>>>>> abstract out the extraction of a SB datapath's corresponding NB UUID
>>>>>>>>>> and
>>>>>>>>>> type in the following way:
>>>>>>>>>> - if the SB.Datapath.type field has a value, the helper assumed that
>>>>>>>>>> the
>>>>>>>>>> SB.Datapath.UUID is equal to the NB.Logical_Switch/Router.UUID so
>>>>>>>>>> it
>>>>>>>>>> returned the 'type' value and the SB UUID
>>>>>>>>>> - else (for "old style") datapaths it extracted the NB UUID and type
>>>>>>>>>> from the SB.Datapath.external_ids (as used to be done before the
>>>>>>>>>> commit).
>>>>>>>>>>
>>>>>>>>>> This creates an upgrade issue thoughi: older (already existing before
>>>>>>>>>> upgrade) SB datapaths are not immediately recreated and their 'type'
>>>>>>>>>> is set; so the NB UUID value returned by
>>>>>>>>>> datapath_get_nb_uuid_and_type()
>>>>>>>>>> for these records is incorrect.
>>>>>>>>>>
>>>>>>>>>> That stays like that until datapaths are finally recreated (due to
>>>>>>>>>> other
>>>>>>>>>> events, e.g., full recompute of the datapath-sync nodes). All that
>>>>>>>>>> time
>>>>>>>>>> the lflow manager will incorrectly determine that the existing lflows
>>>>>>>>>> that are stale. That's because the lflow manager uses
>>>>>>>>>> ovn_datapath_from_sbrec() which in turn relied on
>>>>>>>>>> datapath_get_nb_uuid_and_type() to determine the in-memory
>>>>>>>>>> ovn_datapath
>>>>>>>>>> corresponding to the logical flow.
>>>>>>>>>>
>>>>>>>>>> There's no need to avoid re-creating SB.Datapath bindings as soon as
>>>>>>>>>> ovn-northd determines that they're not using the new scheme. In
>>>>>>>>>> order
>>>>>>>>>> to achieve that we add two sets of helpers:
>>>>>>>>>> a. one to be used by ovn-northd, ovn_datapath_get_nb_uuid_and_type()
>>>>>>>>>> which only parses the new type of datapaths (everything else, i.e.
>>>>>>>>>> old-style datapaths, will be considered stale)
>>>>>>>>>> b. one to be used by all other readers of SB datapaths (e.g.,
>>>>>>>>>> ovn-controller, ovn-ic), datapath_get_nb_uuid_and_type_legacy()
>>>>>>>>>> which relies on parsing external IDs (even in the new type of
>>>>>>>>>> datapaths external IDs are still populated with the NB UUID in
>>>>>>>>>> order
>>>>>>>>>> to maintain backwards compatibility).
>>>>>>>>>
>>>>>>>>> Can we ever switch all users to the non-leagacy? I'm not sure...
>>>>>>>>> Should probably be a TODO entry for this.
>>>>>>>>>
>>>>>>>>
>>>>>>>> We can, it should happen in the next LTS (26.03) though. And it should
>>>>>>>> be the plan.
>>>>>>>
>>>>>>> How the transition is done? i.e. is that guaranteed that the database
>>>>>>> is fully translated to the new format after the next upgrade? Or was it
>>>>>>> decided that the full database rewrite on upgrade is not an issue and it
>>>>>>> works this way?
>>>>>>>
>>>>>>
>>>>>> Yes, it's guaranteed that after upgrade to 25.09.0 (or more recent) the
>>>>>> database is fully translated. It was decided that it's not a concern to
>>>>>> do that. Discussed here:
>>>>>>
>>>>>> https://mail.openvswitch.org/pipermail/ovs-dev/2025-August/425327.html
>>>>
>>>> OK. FWIW, in comparison with the schema upgrade situation, this datapath
>>>> binding re-creation business may cause actual temporary traffic disruption
>>>> as we'll delete all the dynamic MAC bindings. Is that the case? I didn't
>>>> read it very carefully, but I don't see this being discussed in the linked
>>>> thread.
>>>>
>>>
>>> You're right, dynamic MAC bindings have not been discussed. I guess we
>>> have a few options:
>>> a. leave it as is, dynamic MAC bindings will be recreated with the
>>> correct datapaths when needed, traffic will be buffered (for a bit) by
>>> ovn-controller
>>
>> The main problem here is that all that traffic is going to ovn-controller
>> and, as practice shows (we had an issue like this earlier this year),
>> CoPP starts dropping a fair share of this traffic causing a noticeable
>> amount of lost packets and visible delays on TCP connections.
>>
>
> True, this sounds like the riskiest option.
>
>>> b. try to update existing dynamic MAC bindings when their datapath is
>>> not accurate anymore (similar to what we do for static mac bindings)
>>
>> Sounds risky in way that it's not that simple to track. We may have a
>> chance of moving entries to a wrong datapath. Do you have ideas on how
>> to do the transition cleanly in northd?
>>
>
> I was thinking of tracking the set of SB.datapath_bindings that were
> recreated (due to old-style format) and if we have any of those iterate
> through other types SB records that might need update (MAC_Binding,
> IGMP_Group and Learned_Route), update them and potentially schedule a
> forced full recompute for the next ovn-northd run (and trigger one).
> It's a bit compute heavy but only when we recreate old-style datapaths
> and it should cover all dependencies.
>
> It's my impression that if we do that, the only potential traffic impact
> might be on IP multicast traffic. That's because we might generate
> slightly different logical flows and SB.multicast_groups next time we
> run northd. In my opinion, an acceptable compromise.
>
> The other two types of records (dynamic mac bindings and learned routes)
> don't actually need the second (forced recompute) run of northd.
>
After a closer look we might not even need the forced recompute.
>>> c. revert all the Datpath_Binding schema changes (type and UUID syncing)
>>
>> Maybe the safest option, but I'm not sure how much reverting this involves.
>>
>
> I agree this is the safest option. But that also means that we'll
> likely make it practically impossible to sync UUIDs from NB to SB in the
> future. I'm not against that, I think the current incremental
> processing infrastructure in northd doesn't really need the UUIDs to be
> in sync. But there were opinions from others that it would simplify the
> work if they're in sync.
>
> In conclusion I think I'd try "b" update SB records that need update.
> I'm going to try to prepare something along those lines but it would be
> nice to hear opinions from Han, Mark, Ales and others too.
>
>>>
>>> Other SB records created by ovn-controller and referring to
>>> Datapath_bindings are:
>>>
>>> - IGMP Group - ovn-controller will recreate this from memory immediately
>>> after the datapath binding update so there might be some disruption but
>>> minimal
>>
>> We should also remember that ovn-controller is handling the whole database
>> rewrite here as well. So, it may take a bit longer than usual.
>>
>>> - Learned_Route - ovn-controller will recreate them too but we don't
>>> really expect any customer deployments pre 25.09 with BGP enabled.
>>
>> The feature was there since 25.03, so we may have some early users, I guess.
>>
>
> Yes, possible but unlikely. In any case, if we go with "b" or "c" we
> can we won't impact such users.
>
>>>
>>>>>>
>>>>>>>> Good point about adding a TODO. I can post a v3 or if
>>>>>>>> this is the only change that needs to happen maybe whoever merges this
>>>>>>>> series can add the following incremental:
>>>>>>>>
>>>>>>>> diff --git a/TODO.rst b/TODO.rst
>>>>>>>> index a9fe3ec4e8..4c7aa0c772 100644
>>>>>>>> --- a/TODO.rst
>>>>>>>> +++ b/TODO.rst
>>>>>>>> @@ -99,6 +99,12 @@ OVN To-do List
>>>>>>>> * Implement I-P for datapath groups.
>>>>>>>> * Implement I-P for route exchange relevant ports.
>>>>>>>>
>>>>>>>> +* ovn-northd Incremental processing
>>>>>>>> +
>>>>>>>> + * Remove datapath_get_nb_uuid_and_type_legacy() in the next LTS
>>>>>>>> (26.03) as
>>>>>
>>>>> On second thought, this should be "after the next LTS (in 26.09)".
>>>>
>>>> Yep.
>>>>
>>>>>
>>>>>>>> + all deployments should be using the new SB.Datapath_Binding.type
>>>>>>>> column
>>>>>>>> + at that point.
>>>>>>>> +
>>>>>>>> * ovn-northd parallel logical flow processing
>>>>>>>>
>>>>>>>> * Multi-threaded logical flow computation was optimized for the case
>>>>>>>> ---
>>>>>>>>
>>>>>>>>> If not, then we should just drop the "new" way and stop duplicating
>>>>>>>>> the same information in two places.
>>>>>>>>>
>>>>>>>>> Best regards, Ilya Maximets.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Dumitru
>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>>
>>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
