Sure. Am debugging the match comparison patch . These flows are installed via rpc(not from the config Ds)
From: Anil Vishnoi [mailto:[email protected]] Sent: Sunday, July 31, 2016 11:35 PM To: Shuva Jyoti Kar Cc: Josh Hershberg; Abhijit Kumbhare; [email protected]; Sam Hague; Jozef Bacigal -X (jbacigal - PANTHEON TECHNOLOGIES at Cisco); Muthukumaran K; Andrej Leitner -X (anleitne - PANTHEON TECHNOLOGIES at Cisco); Manohar SL; Hema Gopalkrishnan Subject: Re: Flow comparison appending priority is not a good idea. We need to see why it's generating the same key for the flow given that these flows are different. On Sun, Jul 31, 2016 at 7:04 AM, Shuva Jyoti Kar <[email protected]<mailto:[email protected]>> wrote: Even with the latest code base I do find that 2 flows of the same table , with different match are being allocated the same flow-key. This is resulting in the bug 5822 (https://bugs.opendaylight.org/show_bug.cgi?id=5822) Flows on switch : ininet> sh ovs-ofctl -O Openflow13 dump-flows s1 OFPST_FLOW reply (OF1.3) (xid=0x2): cookie=0x2b00000000000001, duration=796.236s, table=0, n_packets=0, n_bytes=0, priority=100,dl_type=0x88cc actions=CONTROLLER:65535 cookie=0x2b00000000000001, duration=796.254s, table=0, n_packets=6, n_bytes=480, priority=0 actions=drop flow in the operational DS: <flow> <id>Uri [_value=1]100</id> <flow-statistics xmlns="urn:opendaylight:flow:statistics"> <packet-count>0</packet-count> <duration> <nanosecond>877000000</nanosecond> <second>814</second> </duration> <byte-count>0</byte-count> </flow-statistics> <priority>100</priority> <table_id>0</table_id> <hard-timeout>0</hard-timeout> <match> <ethernet-match> <ethernet-type> <type>35020</type> </ethernet-type> </ethernet-match> </match> <cookie>3098476543630901249</cookie> <flags></flags> <instructions> <instruction> <order>0</order> <apply-actions> <action> <order>0</order> <output-action> <max-length>65535</max-length> <output-node-connector>CONTROLLER</output-node-connector> </output-action> </action> </apply-actions> </instruction> </instructions> <idle-timeout>0</idle-timeout> </flow> <flow> <id>Uri [_value=1]0</id> <flow-statistics xmlns="urn:opendaylight:flow:statistics"> <packet-count>6</packet-count> <duration> <nanosecond>895000000</nanosecond> <second>814</second> </duration> <byte-count>480</byte-count> </flow-statistics> <priority>0</priority> <table_id>0</table_id> <hard-timeout>0</hard-timeout> <match></match> <cookie>3098476543630901249</cookie> <flags></flags> <idle-timeout>0</idle-timeout> </flow> As you can see the flowid is same flow-id is been given Uri [_value=1] . I have used a patch to append the flow-priority along with the flow-id thus making the flowkey unique. This solves a part of the problem and bug 5822 but we need to revisit the match comparison part. Thanks Shuva From: Josh Hershberg [mailto:[email protected]<mailto:[email protected]>] Sent: Tuesday, July 26, 2016 1:21 PM To: Anil Vishnoi Cc: Abhijit Kumbhare; [email protected]<mailto:[email protected]>; Sam Hague; Shuva Jyoti Kar; Jozef Bacigal -X (jbacigal - PANTHEON TECHNOLOGIES at Cisco); Muthukumaran K; Andrej Leitner -X (anleitne - PANTHEON TECHNOLOGIES at Cisco); Manohar SL Subject: Re: Flow comparison 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]<mailto:[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]<mailto:[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]<mailto:[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]<mailto:[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]<mailto:[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]<mailto:[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]<mailto:[email protected]>> wrote: On Wed, Jul 20, 2016 at 8:28 PM, Josh Hershberg <[email protected]<mailto:[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]<mailto:[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]<mailto:[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]<mailto:[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]<mailto:[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]<mailto:[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]<mailto:[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
