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