| Message ID | 20241018100930.826293-1-amusil@redhat.com |
|---|---|
| Headers | show |
| Series | Reuse UUID for SB rows if there is 1:1 mapping | expand |
On 10/18/24 12:09, Ales Musil wrote: > Some of the NB rows has only one corresponding row in SB, we can > reuse the UUID from NB for the SB row. This makes it easier to handle > changes as we can directly get the corrsponding row by UUID and is also > better for debugging, because the relation is obvious right away. > > There is still potential to do the same for Datapath_Binding and > Port_Binding tables in SB. In order to have those we would need > some form of collision detection as UUIDs are unique per table. > In theory you could have LS and LR with the same UUID which would > collide at the SB level when we try to do the mapping. The benefit of > of disallowing duplicates across LR and LS, LSP and LRP would be > slightly decresed complexity of the northd code that keeps track of > the relation. > Thanks, Ales, for the series! It would be nice if we could get this change in. However, I do have a concern about SB UUIDs changing when upgrading to ovn-northd to the new version. This will cause ovn-controller to reprogram OpenFlows (as the cookie changes) and has the potential of slightly causing dataplane impact. While the hit is probably minimal (old openflows are removed and new openflows are added in the same bundle) I'd like to first confirm that this is not causing visible problems with some CMS. We're currently running some tests with this change in OpenShift and will come back with results as soon as we have them. Thanks, Dumitru > Ales Musil (5): > northd: Use the same UUID for SB representation of Static_Mac_Binding. > northd: Use the same UUID for SB representation of > Chassis_Template_Var. > northd: Use the same UUID for SB representation of DNS. > northd: Use the same UUID for SB representation of Mirror. > northd: Use the same UUID for SB representation of Load_Balancer. > > controller/pinctrl.c | 76 +++++++++++--------- > lib/automake.mk | 2 - > lib/static-mac-binding-index.c | 43 ----------- > lib/static-mac-binding-index.h | 27 ------- > northd/en-northd.c | 4 -- > northd/en-sync-sb.c | 80 +++------------------ > northd/en-sync-sb.h | 2 - > northd/inc-proc-northd.c | 10 +-- > northd/northd.c | 126 +++++++++++---------------------- > northd/northd.h | 1 - > tests/ovn-northd.at | 7 +- > 11 files changed, 103 insertions(+), 275 deletions(-) > delete mode 100644 lib/static-mac-binding-index.c > delete mode 100644 lib/static-mac-binding-index.h >
On Wed, Oct 23, 2024 at 3:35 AM Dumitru Ceara <dceara@redhat.com> wrote: > > On 10/18/24 12:09, Ales Musil wrote: > > Some of the NB rows has only one corresponding row in SB, we can > > reuse the UUID from NB for the SB row. This makes it easier to handle > > changes as we can directly get the corrsponding row by UUID and is also > > better for debugging, because the relation is obvious right away. > > > > There is still potential to do the same for Datapath_Binding and > > Port_Binding tables in SB. In order to have those we would need > > some form of collision detection as UUIDs are unique per table. > > In theory you could have LS and LR with the same UUID which would > > collide at the SB level when we try to do the mapping. The benefit of > > of disallowing duplicates across LR and LS, LSP and LRP would be > > slightly decresed complexity of the northd code that keeps track of > > the relation. > > > > Thanks, Ales, for the series! It would be nice if we could get this > change in. > > However, I do have a concern about SB UUIDs changing when upgrading to > ovn-northd to the new version. This will cause ovn-controller to > reprogram OpenFlows (as the cookie changes) and has the potential of > slightly causing dataplane impact. > > While the hit is probably minimal (old openflows are removed and new > openflows are added in the same bundle) I'd like to first confirm that > this is not causing visible problems with some CMS. We're currently > running some tests with this change in OpenShift and will come back with > results as soon as we have them. Hi Dumitru, Do you've any updates on the test results ? Thanks Numan > > Thanks, > Dumitru > > > Ales Musil (5): > > northd: Use the same UUID for SB representation of Static_Mac_Binding. > > northd: Use the same UUID for SB representation of > > Chassis_Template_Var. > > northd: Use the same UUID for SB representation of DNS. > > northd: Use the same UUID for SB representation of Mirror. > > northd: Use the same UUID for SB representation of Load_Balancer. > > > > controller/pinctrl.c | 76 +++++++++++--------- > > lib/automake.mk | 2 - > > lib/static-mac-binding-index.c | 43 ----------- > > lib/static-mac-binding-index.h | 27 ------- > > northd/en-northd.c | 4 -- > > northd/en-sync-sb.c | 80 +++------------------ > > northd/en-sync-sb.h | 2 - > > northd/inc-proc-northd.c | 10 +-- > > northd/northd.c | 126 +++++++++++---------------------- > > northd/northd.h | 1 - > > tests/ovn-northd.at | 7 +- > > 11 files changed, 103 insertions(+), 275 deletions(-) > > delete mode 100644 lib/static-mac-binding-index.c > > delete mode 100644 lib/static-mac-binding-index.h > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 11/13/24 17:28, Numan Siddique wrote: > On Wed, Oct 23, 2024 at 3:35 AM Dumitru Ceara <dceara@redhat.com> wrote: >> >> On 10/18/24 12:09, Ales Musil wrote: >>> Some of the NB rows has only one corresponding row in SB, we can >>> reuse the UUID from NB for the SB row. This makes it easier to handle >>> changes as we can directly get the corrsponding row by UUID and is also >>> better for debugging, because the relation is obvious right away. >>> >>> There is still potential to do the same for Datapath_Binding and >>> Port_Binding tables in SB. In order to have those we would need >>> some form of collision detection as UUIDs are unique per table. >>> In theory you could have LS and LR with the same UUID which would >>> collide at the SB level when we try to do the mapping. The benefit of >>> of disallowing duplicates across LR and LS, LSP and LRP would be >>> slightly decresed complexity of the northd code that keeps track of >>> the relation. >>> >> >> Thanks, Ales, for the series! It would be nice if we could get this >> change in. >> >> However, I do have a concern about SB UUIDs changing when upgrading to >> ovn-northd to the new version. This will cause ovn-controller to >> reprogram OpenFlows (as the cookie changes) and has the potential of >> slightly causing dataplane impact. >> >> While the hit is probably minimal (old openflows are removed and new >> openflows are added in the same bundle) I'd like to first confirm that >> this is not causing visible problems with some CMS. We're currently >> running some tests with this change in OpenShift and will come back with >> results as soon as we have them. > > Hi Dumitru, > Hi Numan, > Do you've any updates on the test results ? > The OpenShift CI testing with this change didn't show any disruption (Surya can correct me if I'm wrong). So it's probably ok to go forward with the patches. Regards, Dumitru > Thanks > Numan > >> >> Thanks, >> Dumitru >> >>> Ales Musil (5): >>> northd: Use the same UUID for SB representation of Static_Mac_Binding. >>> northd: Use the same UUID for SB representation of >>> Chassis_Template_Var. >>> northd: Use the same UUID for SB representation of DNS. >>> northd: Use the same UUID for SB representation of Mirror. >>> northd: Use the same UUID for SB representation of Load_Balancer. >>> >>> controller/pinctrl.c | 76 +++++++++++--------- >>> lib/automake.mk | 2 - >>> lib/static-mac-binding-index.c | 43 ----------- >>> lib/static-mac-binding-index.h | 27 ------- >>> northd/en-northd.c | 4 -- >>> northd/en-sync-sb.c | 80 +++------------------ >>> northd/en-sync-sb.h | 2 - >>> northd/inc-proc-northd.c | 10 +-- >>> northd/northd.c | 126 +++++++++++---------------------- >>> northd/northd.h | 1 - >>> tests/ovn-northd.at | 7 +- >>> 11 files changed, 103 insertions(+), 275 deletions(-) >>> delete mode 100644 lib/static-mac-binding-index.c >>> delete mode 100644 lib/static-mac-binding-index.h >>> >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >
Some of the NB rows has only one corresponding row in SB, we can reuse the UUID from NB for the SB row. This makes it easier to handle changes as we can directly get the corrsponding row by UUID and is also better for debugging, because the relation is obvious right away. There is still potential to do the same for Datapath_Binding and Port_Binding tables in SB. In order to have those we would need some form of collision detection as UUIDs are unique per table. In theory you could have LS and LR with the same UUID which would collide at the SB level when we try to do the mapping. The benefit of of disallowing duplicates across LR and LS, LSP and LRP would be slightly decresed complexity of the northd code that keeps track of the relation. Ales Musil (5): northd: Use the same UUID for SB representation of Static_Mac_Binding. northd: Use the same UUID for SB representation of Chassis_Template_Var. northd: Use the same UUID for SB representation of DNS. northd: Use the same UUID for SB representation of Mirror. northd: Use the same UUID for SB representation of Load_Balancer. controller/pinctrl.c | 76 +++++++++++--------- lib/automake.mk | 2 - lib/static-mac-binding-index.c | 43 ----------- lib/static-mac-binding-index.h | 27 ------- northd/en-northd.c | 4 -- northd/en-sync-sb.c | 80 +++------------------ northd/en-sync-sb.h | 2 - northd/inc-proc-northd.c | 10 +-- northd/northd.c | 126 +++++++++++---------------------- northd/northd.h | 1 - tests/ovn-northd.at | 7 +- 11 files changed, 103 insertions(+), 275 deletions(-) delete mode 100644 lib/static-mac-binding-index.c delete mode 100644 lib/static-mac-binding-index.h