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
