On Thu, Aug 14, 2025 at 1:27 AM Dumitru Ceara <dce...@redhat.com> wrote: > > On 8/14/25 7:40 AM, Ales Musil wrote: > > On Thu, Aug 14, 2025 at 1:18 AM Han Zhou <hz...@ovn.org> wrote: > > > >> > >> > >> On Wed, Aug 13, 2025 at 3:45 PM Dumitru Ceara <dce...@redhat.com> wrote: > >>> > >>> Hi Han, Mark, > >>> > >>> On 8/13/25 10:18 PM, Han Zhou wrote: > >>>> > >>>> > >>>> On Wed, Aug 13, 2025 at 12:36 PM Mark Michelson <mmich...@redhat.com > >>>> <mailto:mmich...@redhat.com>> wrote: > >>>>> > >>>>> On 8/13/25 3:10 PM, Dumitru Ceara wrote: > >>>>>> On Wednesday, August 13, 2025, Han Zhou <hz...@ovn.org > >>>> <mailto:hz...@ovn.org> > >>>>>> <mailto:hz...@ovn.org <mailto:hz...@ovn.org>>> wrote: > >>>>>> > >>>>>> > >>>>>> > >>>>>> On Wed, Aug 13, 2025 at 1:20 AM Dumitru Ceara < > >> dce...@redhat.com > >>>> <mailto:dce...@redhat.com> > >>>>>> <mailto:dce...@redhat.com <mailto:dce...@redhat.com>>> wrote: > >>>>>> > > >>>>>> > Hi Mark, Han, Numan, > >>>>>> > > >>>>>> > On 8/12/25 8:32 PM, Mark Michelson via dev wrote: > >>>>>> > > On 8/11/25 2:36 PM, Han Zhou wrote: > >>>>>> > >> > >>>>>> > >> > >>>>>> > >> On Mon, Aug 11, 2025 at 10:22 AM Numan Siddique > >>>>>> <num...@ovn.org <mailto:num...@ovn.org> <mailto:num...@ovn.org > >>>> <mailto:num...@ovn.org>> > >>>>>> > >> <mailto:num...@ovn.org <mailto:num...@ovn.org> > >>>> <mailto:num...@ovn.org <mailto:num...@ovn.org>>>> wrote: > >>>>>> > >> > > >>>>>> > >> > On Mon, Aug 11, 2025 at 10:00 PM Han Zhou > >>>> <hz...@ovn.org <mailto:hz...@ovn.org> > >>>>>> <mailto:hz...@ovn.org <mailto:hz...@ovn.org>> > >>>>>> > >> <mailto:hz...@ovn.org <mailto:hz...@ovn.org> > >>>> <mailto:hz...@ovn.org <mailto:hz...@ovn.org>>>> wrote: > >>>>>> > >> > > > >>>>>> > >> > > On Mon, Aug 11, 2025 at 7:59 AM Mark Michelson via > >> dev < > >>>>>> > >> > > ovs-dev@openvswitch.org <mailto:ovs- > >>>> d...@openvswitch.org> <mailto:ovs-dev@openvswitch.org <mailto:ovs- > >>>> d...@openvswitch.org>> > >>>>>> <mailto:ovs-dev@openvswitch.org <mailto:ovs-dev@openvswitch.org > >>> > >>>> <mailto:ovs-dev@openvswitch.org <mailto:ovs-dev@openvswitch.org>>>> > >>>>>> wrote: > >>>>>> > >> > > > > >>>>>> > >> > > > Prior to this commit, if you wanted to find the > >>>>>> corresponding > >>>>>> > >> northbound > >>>>>> > >> > > > UUID for a southbound Logical Datapath, you could > >> find > >>>>>> it in > >>>>>> > >> one of two > >>>>>> > >> > > > places: > >>>>>> > >> > > > > >>>>>> > >> > > > For logical switches, it was in > >> external-ids:logical- > >>>>>> switch. > >>>>>> > >> > > > For logical routers, it was in external- > >>>> ids:logical-router. > >>>>>> > >> > > > > >>>>>> > >> > > > With this commit, we are separating the type and > >>>> UUID into > >>>>>> > >> separate > >>>>>> > >> > > > fields. This way, no matter the type of the > >> datapath, > >>>>>> you can > >>>>>> > >> find the > >>>>>> > >> > > > UUID. And you can find the type of the datapath > >> without > >>>>>> having to > >>>>>> > >> > > > potentially check multiple external-ids keys to do > >> so. > >>>>>> > >> > > > > >>>>>> > >> > > > These fields are going to be used pretty heavily by > >>>>>> northd in > >>>>>> > >> upcoming > >>>>>> > >> > > > patches, so instead of making them external-ids, > >> these > >>>>>> are now > >>>>>> > >> > > > full-fledged columns on the southbound > >>>>>> Datapath_Binding. The > >>>>>> > >> UUID of the > >>>>>> > >> > > > northbound logical datapath is in a column called > >>>>>> "nb_uuid" and > >>>>>> > >> the type > >>>>>> > >> > > > of the logical datapath is stored in an enumerated > >>>> column > >>>>>> > >> called "type". > >>>>>> > >> > > > >>>>>> > >> > > Thanks Mark. Sorry for reviewing this late. While > >> still > >>>>>> reviewing > >>>>>> > >> the rest > >>>>>> > >> > > of the patches, I want to raise a question regarding > >> this > >>>>>> patch. > >>>>>> > >> Would it > >>>>>> > >> > > be more straightforward and efficient to use NB UUIDs > >>>>>> directly in > >>>>>> > >> SB for > >>>>>> > >> > > both LS and LR? I saw that there was an old patch > >> from > >>>>>> Ales which > >>>>>> > >> mentioned > >>>>>> > >> > > this potential change [0]. It mentioned the > >> necessity of > >>>>>> "collision > >>>>>> > >> > > detection", but I don't think it is necessary, > >> because > >>>>>> UUIDs are > >>>>>> > >> just > >>>>>> > >> > > designed to be globally unique. Did I miss any other > >>>>>> discussions > >>>>>> > >> for this? > >>>>>> > >> > > > >>>>>> > >> > > [0] https://www.mail-archive.com/ovs- <https:// > >>>> www.mail-archive.com/ovs-> > >>>>>> dev%40openvswitch.org/ <http://40openvswitch.org/> <https:// > >>>> www.mail-archive.com/ovs- <https://www.mail-archive.com/ovs-> > >>>>>> dev%40openvswitch.org/ <http://40openvswitch.org/>> > >>>>>> > >> msg89347.html <https://www.mail-archive.com/ovs- > >> <https:// > >>>> www.mail-archive.com/ovs-> > >>>>>> dev%40openvswitch.org/ <http://40openvswitch.org/> <https:// > >>>> www.mail-archive.com/ovs- <https://www.mail-archive.com/ovs-> > >>>>>> dev%40openvswitch.org/ <http://40openvswitch.org/>> > >>>>>> > >> msg89347.html> > >>>>>> > >> > > > >>>>>> > >> > > >>>>>> > >> > I had included this change of using the same NB UUIDs > >>>> in SB for > >>>>>> > >> > Datapath-bindings here [1] > >>>>>> > >> > as part of this series - [2]. Please see the comments > >>>> in [1]. > >>>>>> > >> > > >>>>>> > >> > Later Lorenzo took over this patch series and dropped > >> the > >>>>>> patch [1] > >>>>>> > >> from it. > >>>>>> > >> > > >>>>>> > >> > [1] - https://patchwork.ozlabs.org/project/ovn/ > >>>> <https://patchwork.ozlabs.org/project/ovn/> <https:// > >>>>>> patchwork.ozlabs.org/project/ovn/ <http://patchwork.ozlabs.org/ > >>>> project/ovn/>> > >>>>>> > >> patch/20250110162652.3550775-1-num...@ovn.org/ > >>>> <http://20250110162652.3550775-1-num...@ovn.org/> > >>>>>> <http://20250110162652.3550775-1-num...@ovn.org/ > >>>> <http://20250110162652.3550775-1-num...@ovn.org/>> <https:// > >>>>>> > >> patchwork.ozlabs.org/project/ovn/ <http:// > >>>> patchwork.ozlabs.org/project/ovn/> > >>>>>> patch/20250110162652.3550775-1- <http://patchwork.ozlabs.org/ > >>>> <http://patchwork.ozlabs.org/> > >>>>>> project/ovn/patch/20250110162652.3550775-1-> > >>>>>> > >> num...@ovn.org/ <http://num...@ovn.org/> <http:// > >>>> num...@ovn.org/ <http://num...@ovn.org/>>> > >>>>>> > >> > [2] - https://patchwork.ozlabs.org/project/ovn/list/ > >>>> <https://patchwork.ozlabs.org/project/ovn/list/> > >>>>>> <https://patchwork.ozlabs.org/project/ovn/list/ <https:// > >>>> patchwork.ozlabs.org/project/ovn/list/>>? > >>>>>> > >> series=439760&state=* <https://patchwork.ozlabs.org/ > >>>> project/ <https://patchwork.ozlabs.org/project/> > >>>>>> ovn/list/ <https://patchwork.ozlabs.org/project/ovn/list/ > >>>> <https://patchwork.ozlabs.org/project/ovn/list/>>? > >>>>>> > >> series=439760&state=*> > >>>>>> > >> > > >>>>>> > >> > The main concern is that both LS and LR can have the > >>>> same uuid. > >>>>>> > >> > IMO this can happen only if LS and LR were created by > >>>> passing > >>>>>> > >> > "--id=<SAME_UUID>". > >>>>>> > >> > > >>>>>> > >> Using the same explicit UUID for LS and LR creation is > >>>> clearly a > >>>>>> > >> misconfiguration. > >>>>>> > >> > >>>>>> > >> > I personally think we can overlook this corner case and > >>>>>> document it > >>>>>> > >> > and suggest the users to recreate such logical > >> switches/ > >>>>>> routers before > >>>>>> > >> > upgrading. > >>>>>> > >> > >>>>>> > >> I agree, and I assume legitimate deployments would not > >>>> have such > >>>>>> > >> conflicts. > >>>>>> > > > >>>>>> > > I have been looking at the datapath syncing code *a lot* > >>>> lately. > >>>>>> > > Existing code in northd/northd.c in the main branch already > >>>>>> complains if > >>>>>> > > UUIDs are shared between logical switches and logical > >> routers. > >>>>>> > > > >>>>>> > > This is from join_datapaths() in northd/northd.c in main: > >>>>>> > > > >>>>>> > > SBREC_DATAPATH_BINDING_TABLE_FOR_EACH_SAFE (sb, > >>>>>> sbrec_dp_table) { > >>>>>> > > struct uuid key; > >>>>>> > > if (!smap_get_uuid(&sb->external_ids, "logical- > >>>>>> switch", &key) && > >>>>>> > > !smap_get_uuid(&sb->external_ids, "logical- > >>>>>> router", &key)) { > >>>>>> > > ovsdb_idl_txn_add_comment( > >>>>>> > > ovnsb_txn, > >>>>>> > > "deleting Datapath_Binding "UUID_FMT" > >> that > >>>>>> lacks " > >>>>>> > > "external-ids:logical-switch and " > >>>>>> > > "external-ids:logical-router", > >>>>>> > > UUID_ARGS(&sb->header_.uuid)); > >>>>>> > > sbrec_datapath_binding_delete(sb); > >>>>>> > > continue; > >>>>>> > > } > >>>>>> > > > >>>>>> > > if (ovn_datapath_find_(datapaths, &key)) { > >>>>>> > > static struct vlog_rate_limit rl = > >>>>>> VLOG_RATE_LIMIT_INIT(5, > >>>>>> > > 1); > >>>>>> > > VLOG_INFO_RL( > >>>>>> > > &rl, "deleting Datapath_Binding > >>>> "UUID_FMT" with " > >>>>>> > > "duplicate > >> external-ids:logical-switch/router > >>>>>> "UUID_FMT, > >>>>>> > > UUID_ARGS(&sb->header_.uuid), > >>>> UUID_ARGS(&key)); > >>>>>> > > sbrec_datapath_binding_delete(sb); > >>>>>> > > continue; > >>>>>> > > } > >>>>>> > > > >>>>>> > > struct ovn_datapath *od = > >>>>>> ovn_datapath_create(datapaths, &key, > >>>>>> > > > >> NULL, > >>>>>> NULL, sb); > >>>>>> > > ovs_list_push_back(sb_only, &od->list); > >>>>>> > > } > >>>>>> > > > >>>>>> > > > >>>>>> > > Notice the call to `ovn_datapath_find_()` here. If you > >> have a > >>>>>> logical > >>>>>> > > switch and logical router with the same UUID, then northd > >> will > >>>>>> complain > >>>>>> > > and not create an ovn_datapath for the southbound datapath > >>>>>> binding with > >>>>>> > > the duplicated UUID. > >>>>>> > > > >>>>>> > > Therefore, I think it's perfectly fine to propagate the > >>>>>> northbound UUID > >>>>>> > > to the southbound, since the UUID collision would already > >> be > >>>>>> treated as > >>>>>> > > an error, and we would not break anything by doing so. > >>>>>> > > > >>>>>> > > >>>>>> > Unfortunately, while looking at your v16 where we actually > >>>>>> propagate the > >>>>>> > NB UUID to the SB, I realized this might introduce a > >> significant > >>>>>> problem > >>>>>> > during upgrades. > >>>>>> > > >>>>>> > Due to the "UUID sync" we now have to recreate _all_ > >>>>>> SB.Datapath_binding > >>>>>> > records when ovn-northd upgrades to a version that includes > >> that > >>>>>> change. > >>>>>> > That essentially means we need to recreate most of the SB > >>>> contents > >>>>>> > (datapaths, lflows, datapath groups). > >>>>>> > > >>>>>> > That SB change will likely be a single transaction > >> (ovn-northd -> > >>>>>> SB) so > >>>>>> > it will also be sent as a single jsonrpc update to ovn- > >>>> controllers. > >>>>>> > > >>>>>> > In large(r) setups, e.g., with a few hundred chassis, this > >> will > >>>>>> likely > >>>>>> > create a performance issue as the SB ovsdb-server will have > >> to > >>>>>> send that > >>>>>> > huge transaction to all ovn-controllers. While that could be > >>>>>> mitigated > >>>>>> > by using ovsdb-relay for example, it's quite likely that our > >>>>>> users don't > >>>>>> > have that in place. > >>>>>> > > >>>>>> > So my recommendation is to _not_ sync NB.UUID to > >>>>>> > SB.Datapath_Binding.UUID. I don't think the extra column > >>>> (the v14) > >>>>>> > approach was too much overhead, it's just one UUID per > >> datapath > >>>>>> record > >>>>>> > and the northd code doesn't seem to become way more complex. > >>>>>> > > >>>>>> > >>>>>> Hi Dumitru, > >>>>>> > >>>>>> > >>>>>> Hi Han, > >>>>>> > >>>>>> Thanks for sharing your concern. However, we did have similar > >>>>>> changes in the past that resulted in large SB updates, for > >> example, > >>>>>> "northd.c: Generate and maintain SB lflow uuid in ovn_lflow." > >> that > >>>>>> changed UUIDs of all logical flows, which is the biggest part > >> of SB > >>>>>> DB. In addition, whenever there is a SB DB schema change, > >> during the > >>>>>> upgrade it would anyway resend all the DB data to all ovn- > >>>>>> controllers, and this time we are changing the > >>>>>> > >>>>>> schema as well. > >>>>>> > >>>>>> > >>>>>> Thanks for the pointer to the precedent. I’d argue that datapath > >> binding > >>>>>> resync is slightly more costly than just lflow but you’re right it’s > >>>>>> probably comparable. > >>>>> > >>>>> Updating datapath binding UUIDs has a knock-on effect since it would > >>>>> update all > >>>>> * Datapath_Bindings > >>>>> * Logical_DP_Groups > >>>>> * Logical_Flows > >>>>> * Multicast_Groups > >>>>> * Load_Balancers > >>>>> > >>>>> So I agree with Dumitru that updating datapath bindings is going to be > >>>>> more costly. > >>>>> > >>>>>> > >>>>>> So I think we shouldn't worry too much about this. Upgrades for > >>>>>> large scale needs to be taken care of by operators regardless. > >> And I > >>>>>> think it is very > >>>>>> > >>>>>> > >>>>>> I think we’ve all been involved in complex debugging sessions for > >> issues > >>>>>> that appeared when upgrading ovn at scale. I’d rather we avoid > >> adding > >>>>>> more potential issues. > >>>>> > > > >>>>>> valuable to sync UUIDs between NB and SB. It would make code > >> more > >>>>>> efficient, simpler, especially for incremental processing, and > >> maybe > >>>>>> also easier for debugging. > >>>>>> > >>>>>> > >>>>>> However, I guess at this point we have a time issue.. we’re > >> supposed to > >>>>>> branch on Friday and this large series is still not merged. > >>>>>> > >>>>>> Both approaches have been posted, v16 which resyncs uuids, and v17 > >> which > >>>>>> doesn’t. If Mark is ok with it I can take the responsibility of > >> merging > >>>>>> either of those (or a combination of both) tomorrow. > >>>>>> > >>>>>> But we need some sort of agreement. Mark, Han, Numan, Ales, it > >> would be > >>>>>> great to hear more opinions on this. > >>>>> > >>>>> Above I agreed that this would be a bigger performance hit. However, > >>>>> this is also a one-time thing. Once the UUIDs are updated once, and > >> all > >>>>> other SB tables are updated, then it doesn't happen any more. We're > >> not > >>>>> talking about a repeated performance hit. I think the biggest > >> potential > >>>>> issue here is if the ovn-controller transaction is so large that > >>>>> ovn-controller ends up missing an OVSDB ECHO message and we end up in > >> a > >>>>> loop trying to process the large transaction over and over. > >>>>> > >>>>> As Han has pointed out, making this change has some nice benefits for > >> us > >>>>> as developers. > >>>>> > >>>>> My problem is that I can be persuaded either way :) > >>>> > >>>> I'd recommend going the "same UUID" way, because, if our only concern > >>>> here is the SB DB resync performance, I'd argue that because we already > >>>> have a SB schema change in the same patch, it would anyway cause the > >>>> whole DB data be resynced during the DB conversion, in one jsonRPC. > >>> > >>> You're right and, thinking more about it, if on upgrade the SB server is > >>> restarted, there's no fast-resync anyway and all ovn-controllers in the > >>> cluster will get the whole SB content (or at least the part that matches > >>> the monitor condition). > >>> > >> > >> (Sorry Dumitru, I replied to Mark just before this one.) > >> > >> I'd like to clarify that for regular OVSDB server restart (e.g. during > >> maintenance), fast-resync will work, because ovn-controller has the local > >> cache. > >> Fast-resync just doesn't work when there is a schema change, because the > >> server will inform the clients to cancel the existing monitor and drop the > >> cache, which is the case in this new release of OVN and also most of the > >> releases in the past. > >> > >>>> Please correct me if I am wrong with the latest OVSDB schema change > >>>> behavior. If the whole DB resync can be tolerated by operators, the > >> UUID > >>>> change should also be ok because the size of data change would be > >>>> slightly smaller, although almost at the same level. Of course we > >>>> understand that the DB conversion happens most likely at a separate > >> step > >>>> during the upgrade, so we'd expect two large updates instead of one. > >> But > >>>> if the bigger one can be handled, the smaller one should be fine. > >>>> > >>> > >>> Not necessarily, the whole system might be just on the limit of hitting > >>> a liveness probe timeout or similar and the extra time to do the > >>> "initial processing" might get it over the limit. But I'm just > >>> speculating without a real example. > >> > >> I believe between schema conversion and the data change from northd there > >> is a chance to respond to liveness probes, but I am not 100% sure. > >> On the other hand, in our practice we always do DB upgrade and northd > >> upgrade in separate steps, for which I assumed the same for any large scale > >> operators. Probably we should document this as suggestions in our upgrade > >> guide? Again, I am not 100% sure if this is necessary. In our practice with > >> separate steps it is just more controllable. > >> > >>> > >>>> Thanks, > >>>> Han > >>>> > >>>>> > >>>>> I think a good compromise is that we merge v17 for ovn25.09. In the > >>>>> ovn26.03 development cycle, we can coordinate with ovn-kubernetes > >>>>> developers to try to fix the bug that causes integer enums not to work > >>>>> with libovsdb. Then we can put out an update that essentially does > >> what > >>>>> v16 does. However, we can try to mitigate the potential performance > >> hit > >>>>> in certain ways, such as > >>>>> * Batching the updates to datapath bindings so that we don't update > >>>>> them all at once, but instead just update a few at a time each > >>>>> transaction. Or, > >>>>> * Keep backwards compatibility in the code and only sync UUIDs on > >>>>> datapaths that are added after upgrade. Existing ones can still use > >> the > >>>>> "old" way of using the nb_uuid column or > >>>>> external-ids:logical-switch/logical-router. Getting the nb_uuid from a > >>>>> southbound datapath binding is done via a helper function, so we only > >>>>> have to update the one function in order to get the nb_uuid properly. > >>>>> > >>>>> Either way, having months of development time to plan this instead of > >> 2 > >>>>> days would be better for everyone, I think. > >>>>> > >>> > >>> I'd say, also based on the discussion above, I'm now inclining towards > >>> getting the version that syncs the NB -> SB UUID in before branching. > >>> > >>> Another reason for me changing my mind about this is that we still have > >>> some time until the actual release. So we can change the schema back > >>> even after branching if we decide that's better performance-wise. > >>> > >>> However, we'd need a slightly different version of the series that also > >>> syncs NB -> SB UUID (currently v15), one that doesn't add the integer > >>> enum type. In order to work on the most recent submission I adapted the > >>> v16 patch 1/8 slightly, here's the incremental diff: > >>> > >>> https://github.com/dceara/ovn/commit/b4345db > >>> > >>> It's essentially a port of the uuid syncing from v15 1/8 (skipping the > >>> enum change). > >>> > >>> Mark, does this look correct to you? If so, I can take another look at > >>> v16 with the incremental change squashed in tomorrow morning and try to > >>> get it merged into main. > >>> > >>> In case others want to look at my current branch that's: > >>> > >> https://github.com/dceara/ovn/commits/refs/heads/review-datapath-i-p-v17-sync-uuid/ > >>> > >> > >> Thanks Mark, Dumitru and Ales for the quick revisions! > >> > >> Han > >> > > > > Hi Mark, Dumitru and Han, > > > > looking through the conversation I agree with the NB->SB uuid sync. > > There is only one minor thing I have found in the diff provided by Dumitru > > with the below fix applied it should be fine: > > > > diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c > > index 232578d64..bf9a56928 100644 > > --- a/ic/ovn-ic.c > > +++ b/ic/ovn-ic.c > > @@ -2579,8 +2579,7 @@ main(int argc, char *argv[]) > > ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_encap_col_options); > > > > ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_datapath_binding); > > - ovsdb_idl_add_column(ovnsb_idl_loop.idl, > > - &sbrec_datapath_binding_col_external_ids); > > + ovsdb_idl_add_column(ovnsb_idl_loop.idl, > > &sbrec_datapath_binding_col_type); > > > > ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_port_binding); > > ovsdb_idl_add_column(ovnsb_idl_loop.idl, > > > > Hi all, > > Thank you, everyone, for the discussion! I'll be folding the above > change in too, along with all the other minor comments Ales had on v16 > and will merge it to main today. > > Han, I know you mentioned you hadn't finished the review yet but I hope > it's fine to address any issues you might find as follow ups.
Of course. Thanks everyone! Han > > Best regards, > Dumitru _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
