Sorry for the confusion josh, that was for shuva, shuva is already looking at few similar issues and i think it's related as well.
Shuva .. FYI :) On Mon, Aug 1, 2016 at 10:51 PM, Josh Hershberg <[email protected]> wrote: > Sure. I'll take a look. > > On Mon, Aug 1, 2016 at 9:20 PM, Anil Vishnoi <[email protected]> wrote: >> >> Thanks Josh, >> >> It looks like the null match support we just added, looks like >> openflowjava is sending no match from device and plugin internally not doing >> lookup for the key for this flow match. >> >> Can you also put this in your todo list among other flows are you are >> debugging? >> >> On Mon, Aug 1, 2016 at 8:42 AM, Josh Hershberg <[email protected]> >> wrote: >>> >>> Anil, >>> >>> As far as I can tell there's just one left in the IT that is not showing >>> up. The so called TunnelMiss_<seg_id> flow that is present on the switch.: >>> cookie=0x0, duration=3.296s, table=110, n_packets=0, n_bytes=0, >>> priority=8192,tun_id=0x65 actions=drop >>> I can't see any obvious reason why this would not work so if you'd like I >>> can try and debug it using the IT test. >>> >>> -J >>> >>> >>> On Sun, Jul 31, 2016 at 8:03 PM, Anil Vishnoi <[email protected]> >>> wrote: >>>> >>>> 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 >>> >>> >> >> >> >> -- >> Thanks >> Anil > > -- Thanks Anil _______________________________________________ openflowplugin-dev mailing list [email protected] https://lists.opendaylight.org/mailman/listinfo/openflowplugin-dev
