Please start by rebasing and reposting.
On Fri, Feb 02, 2018 at 06:04:49PM +0530, Satyavalli Rama wrote: > Hi Ben, > > We didn't observed any test case breaks and we've updated the same with logs > in our previous conversations. > Could you please provide your inputs regarding the Jan's comments about > command syntax modifications. > > Thanks & Regards > Satya Valli > Tata Consultancy Services > Mailto: satyavalli.r...@tcs.com > Website: http://www.tcs.com > ____________________________________________ > Experience certainty. IT Services > Business Solutions > Consulting > ____________________________________________ > > > -----Ben Pfaff <b...@ovn.org> wrote: ----- > To: Satyavalli Rama <satyavalli.r...@tcs.com> > From: Ben Pfaff <b...@ovn.org> > Date: 02/02/2018 03:43AM > Cc: SatyaValli <satyavalli.r...@gmail.com>, "d...@openvswitch.org" > <d...@openvswitch.org>, manasa.cherukupa...@tcs.com, p.pava...@tcs.com, > Harivelam Lavanya <harivelam.lava...@tcs.com>, muttamsetty.su...@tcs.com, Jan > Scheurich <jan.scheur...@ericsson.com> > Subject: Re: [ovs-dev] [PATCH V2] OF1.5/EXT-334 OXS/Extensible Flow Entry > Statistics Support > > At the very minimum I can't review a patch that breaks tests. > > On Wed, Jan 31, 2018 at 05:29:12PM +0530, Satyavalli Rama wrote: > > Hi Ben, > > > > Are you also agreeing with the Jan's comments. > > > > Thanks & Regards > > Satya Valli > > Tata Consultancy Services > > Mailto: satyavalli.r...@tcs.com > > Website: http://www.tcs.com > > ____________________________________________ > > Experience certainty. IT Services > > Business Solutions > > Consulting > > ____________________________________________ > > > > > > -----Jan Scheurich <jan.scheur...@ericsson.com> wrote: ----- > > To: Satyavalli Rama <satyavalli.r...@tcs.com>, Ben Pfaff <b...@ovn.org> > > From: Jan Scheurich <jan.scheur...@ericsson.com> > > Date: 01/08/2018 06:31PM > > Cc: SatyaValli <satyavalli.r...@gmail.com>, "d...@openvswitch.org" > > <d...@openvswitch.org>, "manasa.cherukupa...@tcs.com" > > <manasa.cherukupa...@tcs.com>, "p.pava...@tcs.com" <p.pava...@tcs.com>, > > Harivelam Lavanya <harivelam.lava...@tcs.com>, "muttamsetty.su...@tcs.com" > > <muttamsetty.su...@tcs.com> > > Subject: RE: [ovs-dev] [PATCH V2] OF1.5/EXT-334 OXS/Extensible Flow Entry > > Statistics Support > > > > Hi Satyavalli, > > > > Please find my responses below. > > > > Regards, Jan > > > > From: Satyavalli Rama [mailto:satyavalli.r...@tcs.com] > > Sent: Friday, 05 January, 2018 12:25 > > To: Ben Pfaff <b...@ovn.org>; Jan Scheurich <jan.scheur...@ericsson.com> > > Cc: SatyaValli <satyavalli.r...@gmail.com>; d...@openvswitch.org; > > manasa.cherukupa...@tcs.com; p.pava...@tcs.com; Harivelam Lavanya > > <harivelam.lava...@tcs.com>; muttamsetty.su...@tcs.com > > Subject: Re: [ovs-dev] [PATCH V2] OF1.5/EXT-334 OXS/Extensible Flow Entry > > Statistics Support > > > > Hi Jan and Ben, > > > > Please find the inline responses. > > > > > > -----Ben Pfaff <b...@ovn.org> wrote: ----- > > To: Jan Scheurich <jan.scheur...@ericsson.com> > > From: Ben Pfaff <b...@ovn.org> > > Date: 01/05/2018 02:35AM > > Cc: SatyaValli <satyavalli.r...@gmail.com>, "d...@openvswitch.org" > > <d...@openvswitch.org>, Manasa Cherukupally <manasa.cherukupa...@tcs.com>, > > Pavani Panthagada <p.pava...@tcs.com>, Lavanya Harivelam > > <harivelam.lava...@tcs.com>, Surya Muttamsetty <muttamsetty.su...@tcs.com>, > > SatyaValli <satyavalli.r...@tcs.com> > > Subject: Re: [ovs-dev] [PATCH V2] OF1.5/EXT-334 OXS/Extensible Flow Entry > > Statistics Support > > > > On Wed, Jan 03, 2018 at 04:24:06PM +0000, Jan Scheurich wrote: > > > > > > > > > > > > This Patch provides implementation Existing flow entry statistics > > > > > > are > > > > > > redefined as standard OXS(OpenFlow Extensible Statistics) fields for > > > > > > displaying the arbitrary flow stats.The existing Flow Stats were > > > > > > renamed > > > > > > as Flow Description. > > > > > > > > > > > > To support this implementation below messages are newly added > > > > > > > > > > > > OFPRAW_OFPT15_FLOW_REMOVED, > > > > > > OFPRAW_OFPST15_FLOW_REQUEST, > > > > > > OFPRAW_OFPST15_FLOW_DESC_REQUEST, > > > > > > OFPRAW_OFPST15_AGGREGATE_REQUEST, > > > > > > OFPRAW_OFPST15_FLOW_REPLY, > > > > > > OFPRAW_OFPST15_FLOW_DESC_REPLY, > > > > > > OFPRAW_OFPST15_AGGREGATE_REPLY, > > > > > > > > > > > > The current commit adds support for the new feature in flow > > > > > > statistics > > > > > > multipart messages,aggregate multipart messages and OXS support for > > > > > > flow > > > > > > removal message, individual flow description messages. > > > > > > > > > > > > "ovs-ofctl dump-flows" needs to be provided with the arbitrary OXS > > > > > > fields > > > > > > for displaying the desired flow stats. > > > > > > > > > > > > Below are Commands to display OXS stats field wise > > > > > > > > > > > > Flow Statistics Multipart > > > > > > ovs-ofctl dump-flows -O OpenFlow15 <bridge> idle_time > > > > > > ovs-ofctl dump-flows -O OpenFlow15 <bridge> packet_count > > > > > > ovs-ofctl dump-flows -O OpenFlow15 <bridge> byte_count > > > > > > > > > > This would break backward compatibility for one of the most > > > > > frequently used OVS CLI commands. Why don't you introduce a new > > > > command such as "ovs-ofctl dump-flow-stats" for the new OXS stats? > > > > > > > > I think you might be misinterpreting the meaning here. It doesn't > > > > appear to break compatibility, at least not in a major way, since it > > > > doesn't do a lot of updates to the tests that would otherwise be > > > > required. > > > > > > Perhaps I am missing the point of some of these changes. I understand > > > that OVS needs to support the new extensible OXS flow stats syntax in > > > OpenFlow 1.5 and the differentiated MP request/reply pairs > > > OFPMP_FLOW_DESC (replacing the former OFPMP_FLOW) and OFPMP_FLOW_STATS > > > (just fetching flow stats per flow w/o the rest of the flow data). > > > > > > But I don't understand why this should have any impact on the existing > > > CLI command "ovs-ofctl dump-flows" and its output. This tool expressly > > > fetches and displays the complete flow dump from OVS, including match, > > > instructions/actions and statistics. When using OF 1.5 it should > > > transparently apply OFPMP_FLOW_DESC MP request/reply to fetch the data, > > > up to OF 1.4 it should use the original OFPMP_FLOW. > > > > > > I can't see any ovs-ofctl use case that would justify the use of the new > > > OFPMP_FLOW_STATS request/reply. The removed data in the reply compared to > > > the full flow description are mainly the instructions, the full match is > > > still there to identify each flow. So cutting down the transferred data > > > volume can hardly be the reason (Note, this may still be different for > > > real OF 1.5 controllers). > > > > > > If you believe we should have an ovs-ofctl command anyhow, e.g. for > > > testing purposes, I suggest to introduce a new command or add an option > > > to dump-flows to force use of this particular MP message. The output > > > would be limited to flow match and stats in that case. > > > > > > > As per our understanding and from previous review comments we treated > > OF1.5+ has two different ways to request and get replies for Flow Stats: > > FLOW_DESC and FLOW_STATS (which will be even used for Flow Stats Trigger). > > And we've supported this with the help of two commands > > > > OFPMP_FLOW_DESC - ovs-ofctl dump-flow-desc -O OpenFlow15 > > OFPMP_FLOW_STATS - ovs-ofctl dump-flows -O OpenFlow15 > > > > [Jan] My argument still holds: “ovs-ofctl dump-flows” is the > > existing command to fetch and display the entire flow data from OVS. In OF > > 1.5+ it should use the OFPMP_FLOW_DESC primitive instead of the earlier > > OFPMP_FLOW used in older OF protocols. > > > > If you want a way to exercise the new OFPMP_FLOW_STATS primitive you should > > introduce a new command “ovs-ofctl -Oopenflow15 > > dump-flow-stats” for that purpose or, alternatively, add an option to > > the dump-flows command to select using that primitive (and limit the > > output). > > > > > The new command to dump aggregate flow stats is of course a welcome > > > addition. It should work irrespectively of the used OpenFlow version with > > > the OFPMP_AGGREGATE_STATS primitive using classic flow stats prior to OF > > > 1.5 and OXS flow stats in OF 1.5. > > > > > > All in all I am NACK-ing this patch as it stands. > > > > > > Regards, Jan > > > > > > BTW: I have played a bit with the patch. The existing ovs-ofctl test > > > cases appear to not break because the changes described in the commit > > > message and the documentation are not effective. The legacy command > > > format "ovs-ofctl dump-flows -O OpenFlow15 <bridge> [<match>]" still > > > produces the complete flow dump including instructions/actions: > > > > > As OXS_OF_DURATION field is mandatory as per OF 1.5+ Spec for tracking the > > age of flow entry in the switch, we've kept it as a default field. The > > other fields OXS_OF_PACKET_COUNT and OXS_OF_BYTE_COUNT are kept as optional. > > > > ##Flow Status Dump: > > > > ## ovs-ofctl dump-flows -O OpenFlow15 br0 duration > > > > cookie=0x0, duration=235.211s, table=0, n_packets=0, n_bytes=0, > > idle_age=0, hard_age=0, icmp actions=NORMAL > > > > cookie=0x0, duration=229.483s, table=0, n_packets=0, n_bytes=0, > > idle_age=0, hard_age=0, udp,tp_dst=200 actions=NORMAL > > > > cookie=0x0, duration=225.539s, table=0, n_packets=0, n_bytes=0, > > idle_age=0, hard_age=0, tcp,tp_src=100 actions=NORMAL > > > > cookie=0x0, duration=3240.675s, table=0, n_packets=0, n_bytes=0, > > idle_age=0, hard_age=0, priority=0 actions=NORMAL > > > > The OXS field value reflection is based on the flow match > > > > ## ovs-ofctl dump-flows -O OpenFlow15 br0 packet_count > > > > cookie=0x0, duration=176.872s, table=0, n_packets=1102, n_bytes=0, > > idle_age=0, hard_age=0, icmp actions=NORMAL > > > > cookie=0x0, duration=171.144s, table=0, n_packets=544, n_bytes=0, > > idle_age=0, hard_age=0, udp,tp_dst=200 actions=NORMAL > > > > cookie=0x0, duration=167.200s, table=0, n_packets=408, n_bytes=0, > > idle_age=0, hard_age=0, tcp,tp_src=100 actions=NORMAL > > > > cookie=0x0, duration=3182.336s, table=0, n_packets=53592657, n_bytes=0, > > idle_age=0, hard_age=0, priority=0 actions=NORMAL > > > > ## ovs-ofctl dump-flows -O OpenFlow15 br0 packet_count,idle_time > > > > cookie=0x0, duration=143.448s, table=0, n_packets=1102, n_bytes=0, > > idle_age=5, hard_age=0, icmp actions=NORMAL > > > > cookie=0x0, duration=137.720s, table=0, n_packets=544, n_bytes=0, > > idle_age=5, hard_age=0, udp,tp_dst=200 actions=NORMAL > > > > cookie=0x0, duration=133.776s, table=0, n_packets=408, n_bytes=0, > > idle_age=5, hard_age=0, tcp,tp_src=100 actions=NORMAL > > > > cookie=0x0, duration=3148.912s, table=0, n_packets=53592655, n_bytes=0, > > idle_age=0, hard_age=0, priority=0 actions=NORMAL > > > > [Jan] Irrespective of the discrepancy of behavior in my test environment, I > > would like to ask what you consider the purpose of adding one or more OXS > > types to the dump-flows (and dump-flow-desc) commands. I could understand > > if the client could filter the requested flow stats but, as per the OF 1.5 > > spec, the OFPMP_FLOW_STATS request cannot selectively fetch a subset of the > > total flow stats. So the switch will always report all available statistics > > back. > > > > The only thing the patch currently does is to discard not explicitly listed > > counters (except for duration, why?) on the receiver side and print zero > > values instead. What is the point? > > > > For unit tests we typically apply some output filters to suppress > > non-deterministic or irrelevant data of a command outputs. Is that the use > > case you were thinking of? There is a “--no-stats” option > > already in ovs-ovctl that suppresses all flow stats if the user is not > > interested in those. > > > > Unless there is a real use case that I am overlooking, I think you should > > just remove the OXS fields option from the affected commands. > > > > > When specifying it in addition to some filter match it is rejected: > > > > > > # ovs-ofctl -Oopenflow15 dump-flows br0 in_port="br0-ns1" packet_count > > > ovs-ofctl: 'dump-flows' command takes at most 2 arguments > > > > There is a slight syntax mismatch, please find the below dumps with match > > field: > > > > ##Dumps with Match Field: > > ## ovs-ofctl dump-flows -O OpenFlow15 br0 tcp,packet_count > > cookie=0x0, duration=549s, table=0, n_packets=42146, n_bytes=0, > > idle_age=0, hard_age=0, tcp,tp_src=100 actions=NORMAL > > > > ## ovs-ofctl dump-flows -O OpenFlow15 br0 tcp,byte_count > > cookie=0x0, duration=565.577s, table=0, n_packets=0, n_bytes=2275884, > > idle_age=0, hard_age=0, tcp,tp_src=100 actions=NORMAL > > > > ## ovs-ofctl dump-flows -O OpenFlow15 br0 udp,packet_count > > cookie=0x0, duration=600.984s, table=0, n_packets=56200, n_bytes=0, > > idle_age=0, hard_age=0, udp,tp_dst=200 actions=NORMAL > > > > ## ovs-ofctl dump-flows -O OpenFlow15 br0 icmp,packet_count > > cookie=0x0, duration=614.544s, table=0, n_packets=112611, n_bytes=0, > > idle_age=0, hard_age=0, icmp actions=NORMAL > > > > ## ovs-ofctl dump-flow-desc -O OpenFlow15 br0 icmp,packet_count > > OFPST_FLOW_DESC reply (OF1.5) (xid=0x4): > > cookie=0x0, duration=625.217s, table=0, n_packets=112611, n_bytes=0, > > check_overlap reset_counts idle_age=0, hard_age=0, icmp actions=NORMAL > > > > ## ovs-ofctl dump-aggregate -O OpenFlow15 br0 icmp,packet_count > > OFPST_AGGREGATE reply (OF1.5) (xid=0x4): packet_count=112611 byte_count=0 > > flow_count=1 > > > > [Jan] This syntax is even more confusing as it mixes match fields and OXS > > filters. But that problem goes away when we get rid of the OXS filter > > option. > > > > =====-----=====-----===== > > Notice: The information contained in this e-mail > > message and/or attachments to it may contain > > confidential or privileged information. If you are > > not the intended recipient, any dissemination, use, > > review, distribution, printing or copying of the > > information contained in this e-mail message > > and/or attachments to it are strictly prohibited. If > > you have received this communication in error, > > please notify us by reply e-mail or telephone and > > immediately and permanently delete the message > > and any attachments. Thank you > > > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
