So it does not work well enough, see this output [1]. I have not invested too much time in analyzing why these failures have occurred although some of them look rather suspicious, especially DEFAULT_PIPELINE_FLOW_70.
[1] Failed tests: NetvirtIT.testNetVirt:421 Could not find flow in operational: Flow [_flowName=DEFAULT_PIPELINE_FLOW_70, _hardTimeout=0, _id=Uri [_value=DEFAULT_PIPELINE_FLOW_70], _idleTimeout=0, _key=FlowKey [_id=Uri [_value=DEFAULT_PIPELINE_FLOW_70]], _tableId=70, _barrier=false, _strict=true, augmentation=[]]--Node [_id=Uri [_value=openflow:106208305082963], _key=NodeKey [_id=Uri [_value=openflow:106208305082963]], augmentation=[]] NetvirtIT.testNetVirtFixedSG:493 Could not find flow in operational: Flow [_flowName=Egress_DHCP_Client_Permit_, _hardTimeout=0, _id=Uri [_value=Egress_DHCP_Client_Permit_], _idleTimeout=0, _key=FlowKey [_id=Uri [_value=Egress_DHCP_Client_Permit_]], _tableId=40, _barrier=false, _strict=true, augmentation=[]]--Node [_id=Uri [_value=openflow:106208305082963], _key=NodeKey [_id=Uri [_value=openflow:106208305082963]], augmentation=[]] NetvirtIT.testNeutronNet:640 Could not find flow in operational: Flow [_flowName=TunnelMiss_101, _hardTimeout=0, _id=Uri [_value=TunnelMiss_101], _idleTimeout=0, _key=FlowKey [_id=Uri [_value=TunnelMiss_101]], _tableId=110, _barrier=false, _strict=true, augmentation=[]]--Node [_id=Uri [_value=openflow:106208305082963], _key=NodeKey [_id=Uri [_value=openflow:106208305082963]], augmentation=[]] On Tue, Jul 26, 2016 at 4:26 AM, Anil Vishnoi <[email protected]> wrote: > 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
