josh, is it possible to get the flow details for these flows? that will give us some idea why it's failing.
On Tue, Jul 26, 2016 at 12:50 AM, Josh Hershberg <[email protected]> wrote: > 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 >> > > -- Thanks Anil
_______________________________________________ openflowplugin-dev mailing list [email protected] https://lists.opendaylight.org/mailman/listinfo/openflowplugin-dev
