> Hey Sunil,
> Thanks for the comments, I've left some responses inline. Let me know
> what you think.
>
> > -----Original Message-----
> > From: Pai G, Sunil <sunil.pa...@intel.com>
> > Sent: Wednesday 13 July 2022 14:51
> > To: Phelan, Michael <michael.phe...@intel.com>; d...@openvswitch.org
> > Cc: maxime.coque...@redhat.com; i.maxim...@ovn.org
> > Subject: RE: [ovs-dev] [PATCH v2] tests: Add OVS-DPDK QoS unit tests
> >
> > Hi Mike,
> >
> > Thanks for adding more testcases !
> > Patch generally looks good, although it might require a rebase.
>
> Yep, will definitely need a rebase before merging.
>
> > Minor nits inline.
> >
> >
> > > -----Original Message-----
> > > From: dev <ovs-dev-boun...@openvswitch.org> On Behalf Of Michael
> > > Phelan
> > > Sent: Wednesday, June 29, 2022 4:19 PM
> > > To: d...@openvswitch.org
> > > Cc: maxime.coque...@redhat.com; i.maxim...@ovn.org
> > > Subject: [ovs-dev] [PATCH v2] tests: Add OVS-DPDK QoS unit tests
> > >
> > > This adds 4 new unit tests to the 'check-dpdk' subsystem that will
> > > test Quality of Service (QoS) functionality.
> > >
> > > Signed-off-by: Michael Phelan <michael.phe...@intel.com>
> > >
> > > ---
> > > v2:
> > > - Added check to confirm policer was removed correctly in phy and
> > > vport tests.
> > > - Removed unnecessary error catch in phy test.
> > > - Added egress to various comments to improve clarity.
> > > ---
> > > ---
> > > tests/system-dpdk.at | 147
> > > +++++++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 147 insertions(+)
> > >
> > > diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at index
> > > 7d2715c4a..41987b7b5 100644
> > > --- a/tests/system-dpdk.at
> > > +++ b/tests/system-dpdk.at
> > > @@ -222,6 +222,153 @@ OVS_VSWITCHD_STOP("m4_join([],
> > > [SYSTEM_DPDK_ALLOWED_LOGS], [ AT_CLEANUP dnl
> > > ---------------------------
> > > -----------------------------------------------
> > >
> > > +
> > > +
> > > +dnl
> > > +---------------------------------------------------------------------
> > > +--
> > > +---
> > > +dnl QoS create delete phy port
> > > +AT_SETUP([OVS-DPDK - QoS create delete phy port])
> >
> > Do we want to call this QOS or egress policing ? or both ? (QoS / Egress
> policing)
> > Don’t have a strong preference here but just to make the life of a tester
> easier I
> > would include egress policing in the name.
> > This way: make check-dpdk TESTSUITEFLAGS="-k policing" would run all
> policing
> > test cases(ingress/rate limiting as well as egress/QOS)
>
> I agree with you here, I think it make sense to call them egress policing
> tests
> to keep it in line with the ingress tests and hopefully make it a bit easier
> for
> end users. I will change this is in the next version.
Not sure I agree with this, egress policing is specifically part of QoS which
is handled separately to rate-limiting, even as part of the vswitch.xml these
are separate concepts and separate in the code base for netdev-dpdk, it's just
coincidence that there happens to be an egress policer implementation for QoS.
I would prefer to see a section in the future for QoS specifically that runs
all QoS tests if that makes sense rather than mixing the policers.
> >
> >
> > > +AT_KEYWORDS([dpdk])
> > > +
> > > +OVS_DPDK_PRE_PHY_SKIP()
> > > +OVS_DPDK_START()
> > > +
> > > +dnl Add userspace bridge and attach it to OVS and add egress policer
> > > +AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10
> > > +datapath_type=netdev]) AT_CHECK([ovs-vsctl add-port br10 phy0 --
> set
> > > +Interface phy0 type=dpdk options:dpdk-devargs=$(cat PCI_ADDR)], [],
> > > +[stdout], [stderr]) OVS_WAIT_UNTIL([ovs-vsctl set port phy0
> qos=@newqos
> > > +-- --id=@newqos create qos type=egress-policer other-
> config:cir=1250000
> > > +other-config:cbs=2048]) AT_CHECK([ovs-appctl -t ovs-vswitchd
> qos/show
> > > +phy0], [], [stdout]) sleep 2
> > > +
> > > +dnl Fail if egress policer could not be created AT_FAIL_IF([grep "Could
> > > +not create rte meter for egress policer" ovs-vswitchd.log], [],
> > > +[stdout])
> > > +
> > > +dnl remove egress policer
> >
> > Please capitalize the first word. [please check this on the entire patch]
> >
> > > +AT_CHECK([ovs-vsctl destroy QoS phy0 -- clear Port phy0 qos])
> > > +
> > > +dnl check egress policer was removed correctly AT_CHECK([ovs-appctl -
> t
> > > +ovs-vswitchd qos/show phy0], [], [stdout]) AT_CHECK([egrep 'QoS not
> > > +configured on phy0' stdout], [], [stdout])
> > > +
> > > +dnl Clean up
> > > +AT_CHECK([ovs-vsctl del-port br10 phy0], [], [stdout], [stderr])
> > > +OVS_VSWITCHD_STOP("[SYSTEM_DPDK_ALLOWED_LOGS]")
> > > +AT_CLEANUP
> > > +dnl
> > > +-----------------------------------------------------------------------
> > > +---
> > > +
> > > +
> > > +
> > > +dnl
> > > +-----------------------------------------------------------------------
> > > +---
> > > +dnl QoS create delete vport port
> > > +AT_SETUP([OVS-DPDK - QoS create delete vport port])
> > > +AT_KEYWORDS([dpdk])
> > > +
> > > +OVS_DPDK_PRE_CHECK()
> > > +OVS_DPDK_START()
> > > +
> > > +dnl Add userspace bridge and attach it to OVS and add egress policer
> > > +AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10
> > > +datapath_type=netdev]) AT_CHECK([ovs-vsctl add-port br10
> > > +dpdkvhostuserclient0 -- set Interface dpdkvhostuserclient0
> > > type=dpdkvhostuserclient options:vhost-server-
> > > path=$OVS_RUNDIR/dpdkvhostclient0], [], [stdout], [stderr])
> > > OVS_WAIT_UNTIL([ovs-vsctl set port dpdkvhostuserclient0
> qos=@newqos -- --
> > > id=@newqos create qos type=egress-policer other-config:cir=1250000 \
> > > + other-config:cbs=2048]) AT_CHECK([ovs-appctl -t
> > > +ovs-vswitchd qos/show dpdkvhostuserclient0], [], [stdout]) sleep 2
> > > +
> > > +dnl Fail if egress policer could not be created AT_FAIL_IF([grep "Could
> > > +not create rte meter for egress policer" ovs-vswitchd.log], [],
> > > +[stdout])
> > > +
> > > +dnl remove egress policer
> > > +AT_CHECK([ovs-vsctl destroy QoS dpdkvhostuserclient0 -- clear Port
> > > +dpdkvhostuserclient0 qos])
> > > +
> > > +dnl check egress policer was removed correctly AT_CHECK([ovs-appctl -
> t
> > > +ovs-vswitchd qos/show dpdkvhostuserclient0], [], [stdout])
> > > +AT_CHECK([egrep 'QoS not configured on dpdkvhostuserclient0'
> stdout],
> > > +[], [stdout])
> > > +
> > > +dnl Parse log file
> > > +AT_CHECK([grep "VHOST_CONFIG: vhost-user client: socket created"
> > > +ovs-vswitchd.log], [], [stdout]) AT_CHECK([grep "vHost User device
> > > +'dpdkvhostuserclient0' created in 'client' mode, using client socket"
> > > +ovs-vswitchd.log], [], [stdout]) AT_CHECK([grep "VHOST_CONFIG:
> > > +$OVS_RUNDIR/dpdkvhostclient0: reconnecting..." ovs-vswitchd.log], [],
> > > +[stdout])
> >
> > Although this sequence works just fine, we could move the above
> checks(Parse
> > log file) just after adding the vhost port.
> > Followed by qos port add and the checks for this addition.
>
> Makes sense, I will rearrange these in the next version.
>
> >
> >
> > > +
> > > +dnl Clean up
> > > +AT_CHECK([ovs-vsctl del-port br10 dpdkvhostuserclient0], [], [stdout],
> > > +[stderr]) OVS_VSWITCHD_STOP("m4_join([],
> > [SYSTEM_DPDK_ALLOWED_LOGS], [
> > > +\@VHOST_CONFIG: failed to connect to
> $OVS_RUNDIR/dpdkvhostclient0:
> > No
> > > +such file or directory@d
> > > +])")
> > > +AT_CLEANUP
> > > +dnl
> > > +-----------------------------------------------------------------------
> > > +---
> > > +
> > > +
> > > +
> > > +dnl
> > > +-----------------------------------------------------------------------
> > > +---
> > > +dnl QoS no cir
> > > +AT_SETUP([OVS-DPDK - QoS no cir])
> > > +AT_KEYWORDS([dpdk])
> > > +
> > > +OVS_DPDK_PRE_CHECK()
> > > +OVS_DPDK_START()
> > > +
> > > +dnl Add userspace bridge and attach it to OVS and add egress policer
> > > +AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10
> > > +datapath_type=netdev]) AT_CHECK([ovs-vsctl add-port br10
> > > +dpdkvhostuserclient0 -- set Interface dpdkvhostuserclient0
> > > +type=dpdkvhostuserclient
> > > +options:vhost-server-path=$OVS_RUNDIR/dpdkvhostclient0], [],
> [stdout],
> > > +[stderr]) OVS_WAIT_UNTIL([ovs-vsctl set port dpdkvhostuserclient0
> > > +qos=@newqos -- --id=@newqos create qos type=egress-policer
> > > +other-config:cbs=2048]) sleep 2
> > > +
> > > +dnl check egress policer was not created AT_CHECK([ovs-appctl -t
> > > +ovs-vswitchd qos/show dpdkvhostuserclient0], [], [stdout])
> > > +AT_CHECK([egrep 'QoS not configured on dpdkvhostuserclient0'
> stdout],
> > > +[], [stdout])
> > > +
> > > +dnl Parse log file
> > > +AT_CHECK([grep "VHOST_CONFIG: vhost-user client: socket created"
> > > +ovs-vswitchd.log], [], [stdout]) AT_CHECK([grep "vHost User device
> > > +'dpdkvhostuserclient0' created in 'client' mode, using client socket"
> > > +ovs-vswitchd.log], [], [stdout]) AT_CHECK([grep "VHOST_CONFIG:
> > > +$OVS_RUNDIR/dpdkvhostclient0: reconnecting..." ovs-vswitchd.log], [],
> > > +[stdout])
> > > +
> > > +dnl Clean up
> > > +AT_CHECK([ovs-vsctl del-port br10 dpdkvhostuserclient0], [], [stdout],
> > > +[stderr]) OVS_VSWITCHD_STOP("m4_join([],
> > [SYSTEM_DPDK_ALLOWED_LOGS], [
> > > +\@VHOST_CONFIG: failed to connect to
> $OVS_RUNDIR/dpdkvhostclient0:
> > No
> > > +such file or directory@d \@Could not create rte meter for egress
> > > +policer@d \@Failed to set QoS type egress-policer on port
> > > +dpdkvhostuserclient0: Invalid argument@d
> > > +])")
> > > +AT_CLEANUP
> > > +dnl
> > > +-----------------------------------------------------------------------
> >
> > Maybe we should combine last three test cases into 1 ? WDYT ?
> > But here we must be careful while we grep the string in ovs-vswitchd.log
> as logs
> > might be repetitive.
> > We could borrow the following trick used for few testscases in pmd.at to
> grep
> > from the logical end of the prev part of the test:
> > TMP=$(($(cat ovs-vswitchd.log | wc -l | tr -d [[:blank:]])+1))
> > OVS_WAIT_UNTIL([tail -n +$TMP ovs-vswitchd.log | grep "<any_string>"])
>
> I'm open to doing that as there is a lot of repetition of commands in the 3
> tests but I thought it was better to make unit tests test a specific
> functionality to allow for easy debugging on a failure. Ilya or Ian do you
> have any thoughts on the best course of action?
+1 on making unit tests specific functionality, this follows what we have
already for the rate-limiting tests so is in keeping with what is already in
place.
>
> >
> >
> > > +---
> > > +
> > > +
> > > +
> > > +dnl
> > > +-----------------------------------------------------------------------
> > > +---
> > > +dnl QoS no cbs
> > > +AT_SETUP([OVS-DPDK - QoS no cbs])
> > > +AT_KEYWORDS([dpdk])
> > > +
> > > +OVS_DPDK_PRE_CHECK()
> > > +OVS_DPDK_START()
> > > +
> > > +dnl Add userspace bridge and attach it to OVS and add egress policer
> > > +AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10
> > > +datapath_type=netdev]) AT_CHECK([ovs-vsctl add-port br10
> > > +dpdkvhostuserclient0 -- set Interface dpdkvhostuserclient0
> > > +type=dpdkvhostuserclient
> > > +options:vhost-server-path=$OVS_RUNDIR/dpdkvhostclient0], [],
> [stdout],
> > > +[stderr]) OVS_WAIT_UNTIL([ovs-vsctl set port dpdkvhostuserclient0
> > > +qos=@newqos -- --id=@newqos create qos type=egress-policer
> > > +other-config:cir=1250000]) sleep 2
> > > +
> > > +dnl check egress policer was not created AT_CHECK([ovs-appctl -t
> > > +ovs-vswitchd qos/show dpdkvhostuserclient0], [], [stdout])
> > > +AT_CHECK([egrep 'QoS not configured on dpdkvhostuserclient0'
> stdout],
> > > +[], [stdout])
> > > +
> > > +
> > > +dnl Parse log file
> > > +AT_CHECK([grep "VHOST_CONFIG: vhost-user client: socket created"
> > > +ovs-vswitchd.log], [], [stdout]) AT_CHECK([grep "vHost User device
> > > +'dpdkvhostuserclient0' created in 'client' mode, using client socket"
> > > +ovs-vswitchd.log], [], [stdout]) AT_CHECK([grep "VHOST_CONFIG:
> > > +$OVS_RUNDIR/dpdkvhostclient0: reconnecting..." ovs-vswitchd.log], [],
> > > +[stdout])
> > > +
> > > +dnl Clean up
> > > +AT_CHECK([ovs-vsctl del-port br10 dpdkvhostuserclient0], [], [stdout],
> > > +[stderr]) OVS_VSWITCHD_STOP("m4_join([],
> > [SYSTEM_DPDK_ALLOWED_LOGS], [
> > > +\@VHOST_CONFIG: failed to connect to
> $OVS_RUNDIR/dpdkvhostclient0:
> > No
> > > +such file or directory@d \@Could not create rte meter for egress
> > > +policer@d \@Failed to set QoS type egress-policer on port
> > > +dpdkvhostuserclient0: Invalid argument@d
> > > +])")
> > > +AT_CLEANUP
> > > +dnl
> > > +-----------------------------------------------------------------------
> > > +---
> > > +
> >
> >
> > We should also consider adding trtcm-policer tests too in future. But
> these are
> > good additions for now 😊
Agree here, I think this will be targeted for 2.19.
Thanks
Ian
> >
> >
> > Thanks and regards
> > Sunil
> Thanks,
> Michael.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
