Sounds good (discussing over email till the meeting on Thursday). On Sun, Jul 24, 2016 at 1:14 AM, Anil Vishnoi <vishnoia...@gmail.com> 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 <vishnoia...@gmail.com> > 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 <vishnoia...@gmail.com> >> wrote: >> >>> >>> >>> On Wed, Jul 20, 2016 at 8:28 PM, Josh Hershberg <jhers...@redhat.com> >>> 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 < >>>> abhijitk...@gmail.com> 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 <jhers...@redhat.com> >>>>> 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 <sha...@redhat.com> >>>>>> 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" <jhers...@redhat.com> >>>>>>> wrote: >>>>>>> >>>>>>> Adding Sam. >>>>>>> >>>>>>> Sam, this is the alien flow issue. >>>>>>> >>>>>>> On Tue, Jul 19, 2016 at 5:35 AM, Anil Vishnoi <vishnoia...@gmail.com >>>>>>> > 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 < >>>>>>>> jhers...@redhat.com> 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 >
_______________________________________________ openflowplugin-dev mailing list openflowplugin-dev@lists.opendaylight.org https://lists.opendaylight.org/mailman/listinfo/openflowplugin-dev
