Hey Sunil, Thanks for the comments, I've left some responses inline. Let me know what you think.
> -----Original Message----- > From: Pai G, Sunil <[email protected]> > Sent: Wednesday 13 July 2022 14:51 > To: Phelan, Michael <[email protected]>; [email protected] > Cc: [email protected]; [email protected] > 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 <[email protected]> On Behalf Of Michael > > Phelan > > Sent: Wednesday, June 29, 2022 4:19 PM > > To: [email protected] > > Cc: [email protected]; [email protected] > > 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 <[email protected]> > > > > --- > > 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. > > > > +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? > > > > +--- > > + > > + > > + > > +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 😊 > > > Thanks and regards > Sunil Thanks, Michael. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
