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