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: &#8220;ovs-ofctl dump-flows&#8221; 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 &#8220;ovs-ofctl -
> Oopenflow15 dump-flow-stats&#8221; 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 &#8220;--no-stats&#8221; 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

Reply via email to