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: [email protected] > Website: http://www.tcs.com > ____________________________________________ > Experience certainty. IT Services > Business Solutions > Consulting > ____________________________________________ > > > -----Jan Scheurich <[email protected]> wrote: ----- > To: Satyavalli Rama <[email protected]>, Ben Pfaff <[email protected]> > From: Jan Scheurich <[email protected]> > Date: 01/08/2018 06:31PM > Cc: SatyaValli <[email protected]>, "[email protected]" > <[email protected]>, "[email protected]" > <[email protected]>, "[email protected]" <[email protected]>, > Harivelam Lavanya <[email protected]>, "[email protected]" > <[email protected]> > 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:[email protected]] > Sent: Friday, 05 January, 2018 12:25 > To: Ben Pfaff <[email protected]>; Jan Scheurich <[email protected]> > Cc: SatyaValli <[email protected]>; [email protected]; > [email protected]; [email protected]; Harivelam Lavanya > <[email protected]>; [email protected] > 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 <[email protected]> wrote: ----- > To: Jan Scheurich <[email protected]> > From: Ben Pfaff <[email protected]> > Date: 01/05/2018 02:35AM > Cc: SatyaValli <[email protected]>, "[email protected]" > <[email protected]>, Manasa Cherukupally <[email protected]>, > Pavani Panthagada <[email protected]>, Lavanya Harivelam > <[email protected]>, Surya Muttamsetty <[email protected]>, > SatyaValli <[email protected]> > 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 [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
