I'm not going to change the established semantics of dump-flows, but I want a patch that applies and doesn't break tests before I provide any more detailed feedback.
On Fri, Feb 02, 2018 at 04:00:44PM +0000, Jan Scheurich wrote: > Hi Ben, > > I would appreciate if you could comment on my general concerns with this > patch. > > I think it is unwise to sacrifice the semantics of the established ovs-ofctl > dump-flow command just in order to align the CLI commands with the particular > re-arrangement of multipart requests messages in OpenFlow 1.5. > > There is no need for one-to-one correspondence between ovs-ofctl commands and > underlying OF message types. Especially not if these change between OF > versions. The CLI commands should have a well-defined stable semantics and > use whatever message is appropriate for the OF protocol version in question > to implement that. > > Thanks, Jan > > > -----Original Message----- > > From: Ben Pfaff [mailto:b...@ovn.org] > > Sent: Friday, 02 February, 2018 16:52 > > To: Satyavalli Rama <satyavalli.r...@tcs.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; Jan > > Scheurich <jan.scheur...@ericsson.com> > > Subject: Re: [ovs-dev] [PATCH V2] OF1.5/EXT-334 OXS/Extensible Flow Entry > > Statistics Support > > > > 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