> > From: Seamus Ryan <[email protected]> > > > > This adds 4 new unit tests to the 'check-dpdk' subsystem that will > > test rate limiting functionality. > > > > Thanks for the patch Michael, a few comments below. > > I'm still conducting some testing so may have some follow up comments for full > review.
Hi Michael, had some more time for testing today and have a few more follow up comments inline below for the v2. > > > Signed-off-by: Seamus Ryan <[email protected]> > Probably for the v2 you should add yourself as a Co-author and add your own > sign off tag as well. > > > --- > > NEWS | 3 +- > > tests/system-dpdk.at | 128 > > +++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 130 insertions(+), 1 deletion(-) > > > > diff --git a/NEWS b/NEWS > > index 8fa57836a..e1fbc39ed 100644 > > --- a/NEWS > > +++ b/NEWS > > @@ -3,7 +3,8 @@ Post-v2.17.0 > > - OVSDB: > > * 'relay' service model now supports transaction history, i.e. honors > > the > > 'last-txn-id' field in 'monitor_cond_since' requests from clients. > > - > No need to remove the white space above, You can just add the DPDK section > straight after the last line above. > There should still be 2 white spaces between this section and the v2.17 > section > below. > > > + - DPDK: > > + * Add rate limiting unit tests to DPDK testsuite > Minor, missing period at end of the sentence above. > > > > > v2.17.0 - 17 Feb 2022 > > --------------------- > > diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at > > index 7d2715c4a..6d4707f84 100644 > > --- a/tests/system-dpdk.at > > +++ b/tests/system-dpdk.at > > @@ -222,6 +222,134 @@ OVS_VSWITCHD_STOP("m4_join([], > > [SYSTEM_DPDK_ALLOWED_LOGS], [ > > AT_CLEANUP > > dnl > > -------------------------------------------------------------------------- > > > Just to keep with the existing style for the majority of the unit tests please > ensure 3 whitespaces between the dnl above and below here. > > > +dnl > > -------------------------------------------------------------------------- > > +dn1 Rate create delete phy port > Should this not be dnl instead of dn1? > > As this is ingress policing I think it would be better to reflect that in the > text > throughout, you can have rate limiting technically on the QoS (Egress) and on > ingress policing also so it would be good to reflect which is being checked > here. > > > +AT_SETUP([OVS-DPDK - Rate create delete phy port]) > > +AT_KEYWORDS([dpdk]) > > + > > +OVS_DPDK_PRE_PHY_SKIP() > > +OVS_DPDK_START() > > + > > +dnl Add userspace bridge and attach it to OVS and add 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]) > > +AT_CHECK([ovs-vsctl set interface phy0 ingress_policing_rate=10000 > > ingress_policing_burst=1000]) > > +AT_CHECK([ovs-vsctl show], [], [stdout]) > > What's the purpose of this check? I'm guessing that you are probably looking > to > confirm the rate and burst levels but I don't see an explicit check for it? > If so maybe you need to search for the required fields explicitly rather than > just > the show command itself? So just to confirm, show command here wont provide useful infor for the inress policer. However we could check the values for burst and rate with something like ovs-vsctl get interface dpdk0 ingress_policing_burst ovs-vsctl get interface dpdk0 ingress_policing_rate To ensure the expected values are there for the given interface? > > > +sleep 2 > > + > > +dn1 remove policer > dn1 should be dnl > > > +AT_CHECK([ovs-vsctl set interface phy0 ingress_policing_rate=0 > > ingress_policing_burst=0]) Again we could confirm the rate/burst after it's set to 0 here to ensure it's after being set without error. > > + > > +dnl Clean up > > +AT_CHECK([ovs-vsctl del-port br10 phy0], [], [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 > > +])") > Not sure why we need the OVS_VSWITCHD_STOP command above to check for > vhost client connection error, this test only looks at using a standard DPDK > PHY > device so we shouldn't see this message would be my thinking as we are not > adding a vhost port? > > > +AT_CLEANUP > > +dnl > > -------------------------------------------------------------------------- > > + > > +dnl > > -------------------------------------------------------------------------- > > +dn1 Rate create delete vport port > > dn1 -> dnl > > > +AT_SETUP([OVS-DPDK - Rate 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 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]) > > +AT_CHECK([ovs-vsctl set interface dpdkvhostuserclient0 > > ingress_policing_rate=10000 ingress_policing_burst=1000]) > > +AT_CHECK([ovs-vsctl show], [], [stdout]) Again here this will not confirm the rate/burst, just the interfaces attached to the bridge. Might be an idea to check the rate/burst as flagged above here also. > > +sleep 2 > > + > > +dn1 remove policer > > dn1 -> dnl > > > +AT_CHECK([ovs-vsctl set interface dpdkvhostuserclient0 > > ingress_policing_rate=0 ingress_policing_burst=0]) Same comment as above to confirm the rate/burst here has been reset to 0. > > + > > +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 > > +])") > > +AT_CLEANUP > > +dnl > > -------------------------------------------------------------------------- > > + > 3 whitspaces between dnl above and below to separate the unit tests > > > +dnl > > -------------------------------------------------------------------------- > > +dn1 Rate no policing rate > dn1 - > dnl > > > +AT_SETUP([OVS-DPDK - Rate no policing rate]) > > +AT_KEYWORDS([dpdk]) > > + > > +OVS_DPDK_PRE_CHECK() > > +OVS_DPDK_START() > > + > > +dnl Add userspace bridge and attach it to OVS and add 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]) > > +AT_CHECK([ovs-vsctl set interface dpdkvhostuserclient0 > > ingress_policing_burst=1000]) So the behavior for no value being set for rate is that the existing value for rate remains the same. We could try and confirm this here. > > +AT_CHECK([ovs-vsctl show], [], [stdout]) > > +sleep 2 > > + > > +dn1 check policer was created correctly > dn1 -> dnl > > > +AT_CHECK([ovs-vsctl list interface dpdkvhostuserclient0], [], [stdout]) > > +AT_CHECK([egrep 'ingress_policing_burst: 1000' stdout], [], [stdout]) > > + > > +AT_CHECK([ovs-vsctl list interface dpdkvhostuserclient0], [], [stdout]) > > +AT_CHECK([egrep 'ingress_policing_rate: 0' 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 > > +])") > > +AT_CLEANUP > > +dnl > > -------------------------------------------------------------------------- > > + > 3 whitspaces between dnl above and below to separate the unit tests > > > +dnl > > -------------------------------------------------------------------------- > > +dn1 Rate no policing burst > > +AT_SETUP([OVS-DPDK - Rate no policing burst]) > > +AT_KEYWORDS([dpdk]) > > + > > +OVS_DPDK_PRE_CHECK() > > +OVS_DPDK_START() > > + > > +dnl Add userspace bridge and attach it to OVS and add 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]) > > +AT_CHECK([ovs-vsctl set interface dpdkvhostuserclient0 > > ingress_policing_rate=10000]) > > +AT_CHECK([ovs-vsctl show], [], [stdout]) > > +sleep 2 > > + > > +dn1 check policer was created correctly > > +AT_CHECK([ovs-vsctl list interface dpdkvhostuserclient0], [], [stdout]) > > +AT_CHECK([egrep 'ingress_policing_burst: 0' stdout], [], [stdout]) > > + > > +AT_CHECK([ovs-vsctl list interface dpdkvhostuserclient0], [], [stdout]) > > +AT_CHECK([egrep 'ingress_policing_rate: 10000' stdout], [], [stdout]) > > + > So when burst is not defined it should default to 8000 from what I remember, > it > would be good if we could check this and confirm it in the test? Actually I'm not sure how we can check this as the value will be 0 in the database so its difficult to confirm (I believe from the code the default value will be set to 8000 but this will not be reflected back in the database). Thanks Ian > > Thanks > Ian > > > +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 > > +])") > > +AT_CLEANUP > > +dnl > > -------------------------------------------------------------------------- > > + > > dnl > > -------------------------------------------------------------------------- > > dnl Add standard DPDK PHY port > > AT_SETUP([OVS-DPDK - MFEX Autovalidator]) > > -- > > 2.25.1 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
