Hi Mike, Thanks for adding more testcases ! Patch generally looks good, although it might require a rebase. 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) > +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. > + > +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>"]) > +--- > + > + > + > +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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
