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]> 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]] > *Sent:* Tuesday, July 26, 2016 1:21 PM > *To:* Anil Vishnoi > *Cc:* Abhijit Kumbhare; [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]> > 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
