This group is locked. No changes can be made to the group while it is locked.
Date
1 - 7 of 7
Clarification on put vs merge and createMissingParent
|
Vishal Thapar <vthapar@...>
Hi Robert, Tom, We're seeing an issue where Genius and Netvirt code both are adding a TerminationPoint/port to an OVSDB bridge. TerminationPoint is a keyed list where port name is the key. IID used in both cases is properly keyed to port name. This always happens when Genius port is the first port added to bridge. If Netvirt code is the one adding first port, all works fine. The issue we're seeing is that any ports added by Genius before Netvirt adds the port are deleted when Netvirt adds the port. Tim has a workaround [1] that seems to work. It just replaces put with a merge in the method called by Netvirt. But genius code is also doing a put and that works, it doesn't delete any existing ports. In fact Genius code most times adds multiple ports in quick succession. Only difference between two is Genius code sets createMissingParents=True. Any inputs on why Netvirt method ends up overwriting the existing ports in list and why merge works? Want to understand if we're missing something crucial and there can be a better fix than changing merge to put. Regards, Vishal. |
|
Robert Varga <nite@...>
On 27/11/2018 18:19, Vishal Thapar wrote:
Hi Robert, Tom,Hello Vishal, Pardon my ignorance, I am not familiar with the mechanics/models here. We're seeing an issue where Genius and Netvirt code both are adding a"port" the same thing as TerminationPoint, right? The issue we're seeing is that any ports added by Genius before NetvirtI suck at explaining, so bear with me :) The data being put/merged is the same: TP { key=TpId(portName) augmentations={OvsdbTerminationPointAugmentation} } If the TP does not exist, the two operations have the same outcome: the termination point is created and DTCL will see a ModificationType.WRITE on the IID(TP) path. If the TP does exist, though, there is a difference: put() will have the same effect as above and it will replace the TP content (augmentations, other fields) with the contents of that field. If you read the IID(TP), you will get back the same object. merge() will have a different effect: - it will retain any present data in the TP and only replace non-matching specified items: in this case it will have the same effect as put(IID(TP)/OvsdbTerminationPointAugmentation) - that means that any existing augmentations will be retained, so if you read IID(TP) you will see your augmentation and anything else that was there. - it also means DTCLs will see a ModificationType.SUBTREE_MODIFIED on the IID(TP) and will see ModificationType.WRITE on IID(TP/OvsdbTerminationPointAugmentation) But genius code is also doing a put and that works, it doesn't deleteGot a pointer to that code? Assuming bridgeNode exists in the data store, createMissingParents=true should not have a bearing on the effects of the overall list (but I may be missing something or there could be a bug). Any inputs on why Netvirt method ends up overwriting the existing portsBy overwriting, do you mean other entries in the TerminationPoint list disappear? If so, does that happen in the same transaction that introduced the netvirt port? (that sounds like a bug) Or could this be Genius reacting to the difference in DTCL explained above? Do the port names overlap at all? Thanks, Robert |
|
Robert Varga <nite@...>
On 27/11/2018 20:45, Robert Varga wrote:
Sorry for self-reply, it just occurred to me.But genius code is also doing a put and that works, it doesn't deleteGot a pointer to that code? There are slight differences in handling createMissingParents={true,false} between controller APIs, md-sal APIs and mdsal-3.0.2 in this particular case (keyed list entry): 1) Controller and MD-SAL < 3.0.2 (read: up until Neon right now) with createMissingParents=false will issue an empty merge to the parent list: https://github.com/opendaylight/controller/blob/master/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/md/sal/binding/impl/AbstractWriteTransaction.java#L98 https://github.com/opendaylight/mdsal/blob/11ed4d0bd7625ae2202782e0785ec15981e49130/binding/mdsal-binding-dom-adapter/src/main/java/org/opendaylight/mdsal/binding/dom/adapter/AbstractWriteTransaction.java#L101 This is needed because before yangtools-2.1.0 the list needed to be created, but there is no way to address the list itself in Binding (you can address a list's parent and you can address all its entries, but you cannot address the list itself) 2) MDSAL >= 3.0.2 (read: Neon real soon now) will take no additional steps This is noted in https://jira.opendaylight.org/browse/MDSAL-383: with yangtools-2.1.0 the list node comes as goes as needed, hence the hoopla in 1) is not necessary. 3) Controller with createMissingParents=true will walk the path and issue an empty merge for each path component: https://github.com/opendaylight/controller/blob/master/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/md/sal/binding/impl/BindingDOMWriteTransactionAdapter.java#L60 Note that this is a superset of 1). 4) MD_SAL with createMissingParents=true will create a single "empty parents" structure and merge it that as immediate descendant of root: https://github.com/opendaylight/mdsal/blob/master/binding/mdsal-binding-dom-adapter/src/main/java/org/opendaylight/mdsal/binding/dom/adapter/AbstractWriteTransaction.java#L82 The difference between 3) and 4) exists because 4) is seen as more efficient way of achieving the same goal -- but the details are sketchy, as that was Tony's patch a looong time ago. I do believe it was all about the utility 4) uses actually coming from netconf to yangtools (and not being available when 3) was implemented) and us not wanting to risk a behaviour change in the controller. Nevertheless, the expectation is that all of these different approaches produce the same resulting data tree content and same ModificationType in associated DTCLs. So if we have exhausted all other alternatives, someone can check if the behaviour changes with https://git.opendaylight.org/gerrit/#/q/topic:mdsal-3.0.2, assuming Netvirt is using MD-SAL APIs :) Regards, Robert |
|
Tim Rozet <trozet@...>
Tim Rozet Red Hat SDN Team On Tue, Nov 27, 2018 at 3:50 PM Robert Varga <nite@...> wrote: On 27/11/2018 20:45, Robert Varga wrote: Ovsdb also executes with createMissingParents=true: Sorry for self-reply, it just occurred to me. |
|
Robert Varga <nite@...>
On 27/11/2018 22:01, Tim Rozet wrote:
I think the genius interface add happens here:That does not look like the place -- it is not related to TerminationPoints at all... Is there a big picture of who does what in the datastore somewhere? Regards, Robert |
|
Vishal Thapar <vthapar@...>
Just to clarify, netvirt and genius code are adding two different terminationPoints/ports. TerminationPoint is what a port is called in network-topology model. So they both are adding two different entries to keyed list, but the code from netvirt ends up overwriting the existing one if Genius adds it first. This is the method that eventually does write from Genius: Regards, Vishal. On Wed, Nov 28, 2018 at 3:28 AM Robert Varga <nite@...> wrote: On 27/11/2018 22:01, Tim Rozet wrote: |
|
Robert Varga <nite@...>
On 28/11/2018 03:16, Vishal Thapar wrote:
Just to clarify, netvirt and genius code are adding two differentCursory look finds nothing. The basic question is: does this happen as the effect of the transaction which put the netvirt port? mdsal-trace should be able to answer that question... Regards, Robert |