Given that we crossed the M5, and the time we have in hand, long term solution of generating equals/hashcode and getAllAugmentation doesn't look feasible. So in my opinion, this looks like a good short term solution.
On Mon, Jul 25, 2016 at 7:23 PM, Josh Hershberg <[email protected]> wrote: > Will do a little later. However, our current code does not set cookies and > if we do not really think that is the correct long term solution, are we > sure we want it now? > > On Tue, Jul 26, 2016 at 1:15 AM, Anil Vishnoi <[email protected]> > wrote: > >> So i hooked up the He plugin custom comparator code to the Li plugin - >> FlowRegistryKeyFactory. >> Custom comparator will only trigger if it won't find the FlowRegistryKey >> in the flowRegistery TrieMap. >> With this all the operational flow related regressions will be address. >> >> Josh, can you please test with this patch and see if it resolves the >> issue. It does not use augmentation comparator, rather it uses cookie value >> in comparison, so as far as you are using unique cookie, operational flow >> should be present at the correct location in operational data store. >> >> Anil >> >> On Mon, Jul 25, 2016 at 9:10 AM, Abhijit Kumbhare <[email protected]> >> wrote: >> >>> Sounds good (discussing over email till the meeting on Thursday). >>> >>> On Sun, Jul 24, 2016 at 1:14 AM, Anil Vishnoi <[email protected]> >>> wrote: >>> >>>> I just realised that next meeting is on Thursday (still i feel >>>> Openflowplugin meeting is on Monday), which is i think going to be bit >>>> late. Can we discuss on the short term plan in this mail thread so that we >>>> can fix it on time ? There are couple of more bugs that is reported related >>>> to the same issue as well. >>>> >>>> On Sun, Jul 24, 2016 at 12:55 AM, Anil Vishnoi <[email protected]> >>>> wrote: >>>> >>>>> Abhijit, can we please put this on agenda for next weeks meeting, we >>>>> need to resolve this issue because it probably will surface many >>>>> regressions with Li plugin. >>>>> >>>>> On Sun, Jul 24, 2016 at 12:54 AM, Anil Vishnoi <[email protected]> >>>>> wrote: >>>>> >>>>>> >>>>>> >>>>>> On Wed, Jul 20, 2016 at 8:28 PM, Josh Hershberg <[email protected]> >>>>>> wrote: >>>>>> >>>>>>> Yeah. >>>>>>> >>>>>>> The Problem >>>>>>> ======================== >>>>>>> Openflowplugin has a "registry" of flows within which flows are >>>>>>> stored in a hash table. When a flow comes in an update (OFST_FLOW) it >>>>>>> does >>>>>>> not have the flow id assigned to it when it was created via the md-sal. >>>>>>> Openflowplugin looks up the flow in the registry to find the id of the >>>>>>> flow. The problem is, that there are multiple representations of the >>>>>>> same >>>>>>> flow in our yang models and depending on where and how the flow entered >>>>>>> the >>>>>>> system it will be represented syntactically different but semantically >>>>>>> identical. As far as I could tell the syntactic differences were around >>>>>>> "container" structures, not the Match objects themselves. The .hashCode >>>>>>> and >>>>>>> .equals methods generated for the md-sal objects are effected by the >>>>>>> syntactic elements and two semantically identical flows will have >>>>>>> different >>>>>>> .hashCodes and will not be .equals and will therefor not be found in the >>>>>>> registry and added to the OPERATIONAL data store with "alien" flow IDs. >>>>>>> You >>>>>>> can see an example of the toString'ed md-sal objects and how they differ >>>>>>> syntactically but are identical semantically in the description of the >>>>>>> bug >>>>>>> [1]. >>>>>>> >>>>>>> HE Solution [2] >>>>>>> ======================== >>>>>>> In the He plugin the solution was to write a custom compare function >>>>>>> (I assume there's a hash too but didn't check). The compare function >>>>>>> manually compares the fields of the match which is what the generated >>>>>>> .equals function does but with one big difference, the custom compare >>>>>>> function skips any extension Match objects. >>>>>>> >>>>>> Below problem is solved by adding cookie in the flow comparison, so >>>>>> if you use different cookie value for these flows, their operational flow >>>>>> will be augmented at the correct id. It's not the solution, but rather a >>>>>> workaround thats been used in HE plugin. >>>>>> >>>>>> >>>>>>> What this means is that two flows that differ only in an extenstion >>>>>>> match, e.g., NxmNxTunIpv4DstGrouping, will be considered equal and one >>>>>>> will >>>>>>> overwrite the other in OPERATIONAL. >>>>>>> >>>>>>> My Patch [3] >>>>>>> ======================== >>>>>>> I pushed a patch that walks the augmentations and compares the match >>>>>>> objects themselves, ignoring the containers but comparing the extension >>>>>>> Match objects. It does this by looping through a hard coded list of >>>>>>> augmentations and then comparing the Match objects contained therein. >>>>>>> Jozef >>>>>>> Becigal pointed out, correctly, that this will break if there is a new >>>>>>> extension Match added because the list is hard coded. >>>>>>> >>>>>>> A possible solution to the extension problem is to change the code >>>>>>> gen so that Augmentable gets a new method called getAllAugmentations >>>>>>> which >>>>>>> would return a list of all the attached augmentations. That way we could >>>>>>> dynamically compare all matches, removing the need for a hard coded >>>>>>> list of >>>>>>> possible extension augmentations. This is a relatively simple fix, >>>>>>> though >>>>>>> (as Sam pointed out) the mdsal project is post code freeze so it would >>>>>>> take >>>>>>> a little maneuvering. >>>>>>> >>>>>>> Another issue with this solution, though this can be solved >>>>>>> relatively easily is that the current implementation relies on the >>>>>>> ordering >>>>>>> of the Matches which I'm not sure is safe. >>>>>>> >>>>>>> >>>>>>> Decisions >>>>>>> ======================== >>>>>>> 1. We could fall back on the HE solution >>>>>>> 2. We could continue with my patch which is imperfect but seems to >>>>>>> cover more cases than the He plugin solution (though less "tried and >>>>>>> true" >>>>>>> as He was running for quite a while) >>>>>>> 3. Take the Augmentable.getAllAugmentations route. >>>>>>> 4. ?? >>>>>>> >>>>>> I think jozef mention that we should leave the comparison of >>>>>> extension to the extensions itself, so extensions should implement their >>>>>> own hashcode/equals. Probably that's what you mean by (3)? >>>>>> >>>>>>> >>>>>>> -J >>>>>>> >>>>>>> [1] https://bugs.opendaylight.org/show_bug.cgi?id=6146 >>>>>>> [2] >>>>>>> openflowplugin/applications/statistics-manager/src/main/java/org/opendaylight/openflowplugin/applications/statistics/manager/impl/helper/FlowComparatorFactory.java >>>>>>> [3] https://git.opendaylight.org/gerrit/#/c/41542/ >>>>>>> >>>>>>> On Thu, Jul 21, 2016 at 4:20 AM, Abhijit Kumbhare < >>>>>>> [email protected]> wrote: >>>>>>> >>>>>>>> Hi folks, >>>>>>>> >>>>>>>> Getting this on the OpenFlow mailing list. Also changed the subject >>>>>>>> to be slightly more related to the actual topic than "Gerrit is stuck" >>>>>>>> :) >>>>>>>> >>>>>>>> Anyway Josh - do you want to summarize your solution (and the >>>>>>>> problem) for the folks new to this discussion? >>>>>>>> >>>>>>>> Thanks, >>>>>>>> Abhijit >>>>>>>> >>>>>>>> On Tue, Jul 19, 2016 at 4:19 PM, Josh Hershberg < >>>>>>>> [email protected]> wrote: >>>>>>>> >>>>>>>>> Which I think my patch is! The differences between the Match >>>>>>>>> objects in question are syntactic and not semantic. I added a compare >>>>>>>>> method that ignores the structural differences. >>>>>>>>> >>>>>>>>> On Tue, Jul 19, 2016 at 12:45 PM, Sam Hague <[email protected]> >>>>>>>>> wrote: >>>>>>>>> >>>>>>>>>> As Josh mentions the issue is that the match object coming back >>>>>>>>>> is not the same as what was written to mdsal so it ends up as a >>>>>>>>>> different >>>>>>>>>> flow. We need a better way to compare the flows. >>>>>>>>>> >>>>>>>>>> On Jul 19, 2016 2:43 AM, "Josh Hershberg" <[email protected]> >>>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>> Adding Sam. >>>>>>>>>> >>>>>>>>>> Sam, this is the alien flow issue. >>>>>>>>>> >>>>>>>>>> On Tue, Jul 19, 2016 at 5:35 AM, Anil Vishnoi < >>>>>>>>>> [email protected]> wrote: >>>>>>>>>> >>>>>>>>>>> +adding other folks >>>>>>>>>>> >>>>>>>>>>> Hi Josh, >>>>>>>>>>> >>>>>>>>>>> Thanks for reminding me of the patch. >>>>>>>>>>> >>>>>>>>>>> Andrej/Jozef, This is the patch, regarding which i asked a >>>>>>>>>>> question on how we currently compare the operational flow and >>>>>>>>>>> config flow >>>>>>>>>>> in lithium plugin? >>>>>>>>>>> >>>>>>>>>>> In the helium plugin we have custom comparators to compare the >>>>>>>>>>> config and operational flow that do explicit match using the >>>>>>>>>>> matching >>>>>>>>>>> construct of the flow ( + cookie). Looks like in lithium plugin we >>>>>>>>>>> are >>>>>>>>>>> using hashcode/equals for the same ( i didn't get a chance to look >>>>>>>>>>> at the >>>>>>>>>>> code yet), which i am afraid won't work for all the scenario, >>>>>>>>>>> specially >>>>>>>>>>> augmentation. Using custom comparator for matching flow for >>>>>>>>>>> augmentation is >>>>>>>>>>> also not really a good idea, because there can be many extension >>>>>>>>>>> (openflow >>>>>>>>>>> and vendor specific) and handling them in the plugin is not a good >>>>>>>>>>> idea. In >>>>>>>>>>> helium plugin, i added cookie as a part of the flow comparison, so >>>>>>>>>>> if you >>>>>>>>>>> have two flows that is different only by the extension matches, >>>>>>>>>>> then user >>>>>>>>>>> can use cookie to differentiate the flows, so custom comparator >>>>>>>>>>> don't have >>>>>>>>>>> to match based on the augmentation. >>>>>>>>>>> >>>>>>>>>>> Can you please share some more details on the existing >>>>>>>>>>> comparison mechanism and lets discuss how we can resolve this issue >>>>>>>>>>> in >>>>>>>>>>> better way. >>>>>>>>>>> >>>>>>>>>>> Thanks >>>>>>>>>>> Anil >>>>>>>>>>> >>>>>>>>>>> On Sat, Jul 16, 2016 at 9:49 PM, Josh Hershberg < >>>>>>>>>>> [email protected]> wrote: >>>>>>>>>>> >>>>>>>>>>>> Hi Anil, >>>>>>>>>>>> >>>>>>>>>>>> Could you please take a look at this gerrit? Just want to make >>>>>>>>>>>> sure it's not buried or stuck or something. >>>>>>>>>>>> >>>>>>>>>>>> thanks, >>>>>>>>>>>> >>>>>>>>>>>> Josh >>>>>>>>>>>> >>>>>>>>>>>> https://git.opendaylight.org/gerrit/#/c/41542/ >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> -- >>>>>>>>>>> Thanks >>>>>>>>>>> Anil >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> Thanks >>>>>> Anil >>>>>> >>>>> >>>>> >>>>> >>>>> -- >>>>> Thanks >>>>> Anil >>>>> >>>> >>>> >>>> >>>> -- >>>> Thanks >>>> Anil >>>> >>> >>> >> >> >> -- >> Thanks >> Anil >> > > -- Thanks Anil
_______________________________________________ openflowplugin-dev mailing list [email protected] https://lists.opendaylight.org/mailman/listinfo/openflowplugin-dev
