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