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