On 9/2/25 9:43 PM, Mark Michelson wrote:
> On 9/2/25 12:14 PM, Dumitru Ceara wrote:
>> On 9/2/25 3:25 PM, Mark Michelson wrote:
>>> On 9/1/25 10:48 AM, 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
>>>> b. try to update existing dynamic MAC bindings when their datapath is
>>>> not accurate anymore (similar to what we do for static mac bindings)
>>>> c. revert all the Datpath_Binding schema changes (type and UUID
>>>> syncing)
>>>
>>> Sorry for the late reply (four day weekend for me).
>>>
>>
>> Hi Mark,
>>
>> No worries.
>>
>>> A fourth option would be to go with what I had done prior to v16 of the
>>> datapath refactor series. Have a look at v15's patch here: https://
>>> patchwork.ozlabs.org/project/ovn/patch/20250811171752.3916543-2-
>>> mmich...@redhat.com/
>>>
>>> In that version, instead of syncing UUIDs from NB to SB, the SB datapath
>>> binding has an "nb_uuid" column that contains the NB logical datapath
>>> UUID. The datapath_get_nb_*() functions can distinguish "legacy"
>>> southbound datapaths from new ones based on whether the nb_uuid column
>>> is NULL or not. If it's NULL, then it's a legacy datapath binding, which
>>> means we need to get the UUID and type from external-ids. If the column
>>> is non-NULL, then we can guarantee that both the NB UUID and type are
>>> populated and use those.
>>>
>>
>> We'd still have to ensure the new nb_uuid column is optional to avoid
>> active-backup upgrade issues (see patch 1/4 of this series).
>>
>>> Going with this option means that any SB records (logical flows,
>>> mac_bindings, etc.) that point to datapath bindings do not need to be
>>> rewritten during an upgrade, since the actual SB UUIDs will not change.
>>> You also still have a simple way to link the SB datapath binding to the
>>> NB logical datapath using the "nb_uuid" column. There's an extra
>>> debugging step to go from the SB datapath binding UUID to the NB logical
>>> datpath UUID, but IMO, it's better and more consistent than what we have
>>> prior to 25.09 with the external-ids.
>>>
>>
>> I think I'm OK with this approach. The only worry I have is how much of
>> code we'd have to change given that the release is currently scheduled
>> to happen this Friday, September 5th.
>>
>> I guess it would help if we had an implementation of "option d" you
>> describe above in order to allow us to compare against "option b" I have
>> implemented here https://github.com/dceara/ovn/commits/fix-northd-
>> loop-v3/.
>
> I have a patch here: https://github.com/putnopvut/ovn/
> commit/9bc21f85a933f82d02a324cea58844e9b1b9320e .
>
> Github CI is green, and I don't see the looping behavior when I run a
> sandbox test. You can use this as a comparison to option b.
>
Thanks, Mark, for sharing this! I see a couple of (small) issues with
the patch:
1. We can probably skip the SB schema version bump as we don't have any
released version with the previous schema. OR if you want to be extra
sure then we should increment to 21.5.0 (.z is bumped only for cosmetic
issues normally, when adding columns we need to bump .y in the version).
2. We still need patch 1/4 of my series:
https://patchwork.ozlabs.org/project/ovn/patch/20250901112248.76646-2-dce...@redhat.com/
OR, maybe better, your patch should make the "type" column optional too.
3. There's a crash we hit in specific cases, e.g., when a stale old-style
SB.Datapath_binding (for a router/switch that doesn't exist in NB anymore).
The binding is deleted by ovn-northd; the SB transaction that removes it
generates an OVSDB update towards ovn-northd. When northd processes that
update, in:
enum engine_input_handler_result
datapath_sync_sb_datapath_binding(struct engine_node *node, void *data)
{
const struct sbrec_datapath_binding_table *sb_dp_table =
EN_OVSDB_GET(engine_get_input("SB_datapath_binding", node));
enum engine_input_handler_result ret = EN_HANDLED_UNCHANGED;
struct ovn_synced_datapaths *synced_datapaths = data;
const struct sbrec_datapath_binding *sb_dp;
SBREC_DATAPATH_BINDING_TABLE_FOR_EACH_TRACKED (sb_dp, sb_dp_table) {
struct ovn_synced_datapath *sdp =
find_synced_datapath_from_sb(&synced_datapaths->synced_dps, sb_dp);
if (sbrec_datapath_binding_is_deleted(sb_dp)) {
if (sdp) {
we crash in find_synced_datapath_from_sb() because there we expect all
deleted sbrec_datapath_binding records to have a valid nb_uuid set.
I think the following diff needs to be squashed into your patch:
diff --git a/northd/en-datapath-sync.c b/northd/en-datapath-sync.c
index 6331dbc5f1..a1adebd008 100644
--- a/northd/en-datapath-sync.c
+++ b/northd/en-datapath-sync.c
@@ -95,9 +95,15 @@ find_synced_datapath_from_sb(const struct hmap *datapaths,
const struct sbrec_datapath_binding *sb_dp)
{
struct ovn_synced_datapath *sdp;
- uint32_t hash = uuid_hash(sb_dp->nb_uuid);
+ struct uuid nb_uuid;
+
+ if (!datapath_get_nb_uuid(sb_dp, &nb_uuid)) {
+ return NULL;
+ }
+
+ uint32_t hash = uuid_hash(&nb_uuid);
HMAP_FOR_EACH_WITH_HASH (sdp, hmap_node, hash, datapaths) {
- if (uuid_equals(&sdp->nb_row->uuid, sb_dp->nb_uuid)) {
+ if (uuid_equals(&sdp->nb_row->uuid, &nb_uuid)) {
return sdp;
}
}
---
The rest looks good to me. I didn't see any other issues when testing
your patch. I tried with some smaller test databases but also with
databases from actual (larger) deployments where we had seen problems
without your patch.
I know Ales is running another scale test with your change (and my incremental
suggestion squashed in). Assuming that his tests don't show any issues either
my vote goes towards using your patch, option d, with the 3 things I
highlighted above fixed. I guess it would be great if you could post it as
a formal patch.
Thanks,
Dumitru
>>
>> What I don't like about "option b" is that it does rewrite most of the
>> SB database on upgrade.
>
> Same here.
>
>>
>>>>
>>>> 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
>>>> - Learned_Route - ovn-controller will recreate them too but we don't
>>>> really expect any customer deployments pre 25.09 with BGP enabled.
>>>>
>>>>>>>
>>>>>>>>> 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
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>> Regards,
>> Dumitru
>>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
