[ovs-dev] [PATCH V2] OF1.5/EXT-334 OXS/Extensible Flow Entry Statistics Support

Ben Pfaff blp at ovn.org
Fri Feb 2 17:26:18 UTC 2018


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:blp at ovn.org]
> > Sent: Friday, 02 February, 2018 16:52
> > To: Satyavalli Rama <satyavalli.rama at tcs.com>
> > Cc: SatyaValli <satyavalli.rama at gmail.com>; dev at openvswitch.org; manasa.cherukupally at tcs.com; p.pavani1 at tcs.com; Harivelam
> > Lavanya <harivelam.lavanya at tcs.com>; muttamsetty.surya at tcs.com; Jan Scheurich <jan.scheurich at 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.rama at tcs.com
> > > Website: http://www.tcs.com
> > > ____________________________________________
> > > Experience certainty.	IT Services
> > > Business Solutions
> > > Consulting
> > > ____________________________________________
> > >
> > >
> > > -----Ben Pfaff <blp at ovn.org> wrote: -----
> > > To: Satyavalli Rama <satyavalli.rama at tcs.com>
> > > From: Ben Pfaff <blp at ovn.org>
> > > Date: 02/02/2018 03:43AM
> > > Cc: SatyaValli <satyavalli.rama at gmail.com>, "dev at openvswitch.org" <dev at openvswitch.org>, manasa.cherukupally at tcs.com,
> > p.pavani1 at tcs.com, Harivelam Lavanya <harivelam.lavanya at tcs.com>, muttamsetty.surya at tcs.com, Jan Scheurich
> > <jan.scheurich at 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.rama at tcs.com
> > > > Website: http://www.tcs.com
> > > > ____________________________________________
> > > > Experience certainty.	IT Services
> > > > Business Solutions
> > > > Consulting
> > > > ____________________________________________
> > > >
> > > >
> > > > -----Jan Scheurich <jan.scheurich at ericsson.com> wrote: -----
> > > > To: Satyavalli Rama <satyavalli.rama at tcs.com>, Ben Pfaff <blp at ovn.org>
> > > > From: Jan Scheurich <jan.scheurich at ericsson.com>
> > > > Date: 01/08/2018 06:31PM
> > > > Cc: SatyaValli <satyavalli.rama at gmail.com>, "dev at openvswitch.org" <dev at openvswitch.org>, "manasa.cherukupally at tcs.com"
> > <manasa.cherukupally at tcs.com>, "p.pavani1 at tcs.com" <p.pavani1 at tcs.com>, Harivelam Lavanya <harivelam.lavanya at tcs.com>,
> > "muttamsetty.surya at tcs.com" <muttamsetty.surya at 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.rama at tcs.com]
> > > > Sent: Friday, 05 January, 2018 12:25
> > > > To: Ben Pfaff <blp at ovn.org>; Jan Scheurich <jan.scheurich at ericsson.com>
> > > > Cc: SatyaValli <satyavalli.rama at gmail.com>; dev at openvswitch.org; manasa.cherukupally at tcs.com; p.pavani1 at tcs.com; Harivelam
> > Lavanya <harivelam.lavanya at tcs.com>; muttamsetty.surya at 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 <blp at ovn.org> wrote: -----
> > > > To: Jan Scheurich <jan.scheurich at ericsson.com>
> > > > From: Ben Pfaff <blp at ovn.org>
> > > > Date: 01/05/2018 02:35AM
> > > > Cc: SatyaValli <satyavalli.rama at gmail.com>, "dev at openvswitch.org" <dev at openvswitch.org>, Manasa Cherukupally
> > <manasa.cherukupally at tcs.com>, Pavani Panthagada <p.pavani1 at tcs.com>, Lavanya Harivelam <harivelam.lavanya at tcs.com>, Surya
> > Muttamsetty <muttamsetty.surya at tcs.com>, SatyaValli <satyavalli.rama at 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
> > > >
> > > >


More information about the dev mailing list