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 >
_______________________________________________ openflowplugin-dev mailing list [email protected] https://lists.opendaylight.org/mailman/listinfo/openflowplugin-dev
