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