> -----Original Message----- > From: Phelan, Michael > Sent: Thursday 9 June 2022 11:49 > To: Ilya Maximets <[email protected]>; [email protected]; Stokes, Ian > <[email protected]> > Cc: [email protected]; [email protected]; Ryan, > Seamus <[email protected]>; Aaron Conole <[email protected]> > Subject: RE: [ovs-dev] [PATCH v2] tests: Add ovs-dpdk rate limiting unit > tests. > > Hi Ilya, > Sorry about the delay in getting back to you, I appreciate you taking the > time to > take a look at the patch, I left some responses to your comments inline below. > > > -----Original Message----- > > From: Ilya Maximets <[email protected]> > > Sent: Wednesday 4 May 2022 11:33 > > To: Phelan, Michael <[email protected]>; [email protected]; > > Stokes, Ian <[email protected]> > > Cc: [email protected]; [email protected]; Ryan, > > Seamus <[email protected]>; [email protected] > > Subject: Re: [ovs-dev] [PATCH v2] tests: Add ovs-dpdk rate limiting unit > > tests. > > > > On 4/28/22 16:56, Michael Phelan wrote: > > > From: Seamus Ryan <[email protected]> > > > > > > This adds 4 new unit tests to the 'check-dpdk' subsystem that will > > > test rate limiting functionality. > > > > > > Signed-off-by: Seamus Ryan <[email protected]> > > > Signed-off-by: Michael Phelan <[email protected]> > > > Co-authored-by: Michael Phelan <[email protected]> > > > > > > --- > > > v2: > > > - Fixed dn1 typo & spacing issues. > > > - Added check for removing burst and rate values. > > > - Renamed tests to specify ingress policing. > > > --- > > > NEWS | 2 + > > > tests/system-dpdk.at | 161 > > > +++++++++++++++++++++++++++++++++++++++++++ > > > 2 files changed, 163 insertions(+) > > > > > > diff --git a/NEWS b/NEWS > > > index 5bc8e6566..63c6a3009 100644 > > > --- a/NEWS > > > +++ b/NEWS > > > @@ -19,6 +19,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. > > > + - DPDK: > > > + * Add rate limiting unit tests to DPDK testsuite. > > > > This is not a change visible to an end user and not a big > > architectural change, so no need for a NEWS entry. > > Sure no problem, I will remove this in the next version. > > > > > > > > > > > v2.17.0 - 17 Feb 2022 > > > diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at index > > > 7d2715c4a..74174f61e 100644 > > > --- a/tests/system-dpdk.at > > > +++ b/tests/system-dpdk.at > > > @@ -222,6 +222,167 @@ OVS_VSWITCHD_STOP("m4_join([], > > > [SYSTEM_DPDK_ALLOWED_LOGS], [ AT_CLEANUP dnl > > > -------------------------------------------------------------------- > > > -- > > > ---- > > > > > > + > > > + > > > +dnl > > > +------------------------------------------------------------------- > > > +-- > > > +----- dnl Ingress policing create delete phy port > > > +AT_SETUP([OVS-DPDK > > > +- Ingress policing 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]) sleep 2 > > > + > > > +dnl check policer was created correctly AT_CHECK([ovs-vsctl list > > > +interface phy0], [], [stdout]) AT_CHECK([egrep > > > +'ingress_policing_burst: 1000' stdout], [], [stdout]) > > > + > > > +AT_CHECK([ovs-vsctl list interface phy0], [], [stdout]) > > > +AT_CHECK([egrep 'ingress_policing_rate: 10000' stdout], [], > > > +[stdout]) > > > + > > > +dnl remove policer > > > +AT_CHECK([ovs-vsctl set interface phy0 ingress_policing_rate=0 > > > +ingress_policing_burst=0]) > > > + > > > +dnl check policer was removed correctly AT_CHECK([ovs-vsctl list > > > +interface phy0], [], [stdout]) AT_CHECK([egrep > > > +'ingress_policing_burst: 0' stdout], [], [stdout]) > > > + > > > +AT_CHECK([ovs-vsctl list interface phy0], [], [stdout]) > > > +AT_CHECK([egrep 'ingress_policing_rate: 0' stdout], [], [stdout]) > > > + > > > +dnl Clean up > > > +AT_CHECK([ovs-vsctl del-port br10 phy0], [], [stdout], [stderr]) > > > +OVS_VSWITCHD_STOP("m4_join([], [SYSTEM_DPDK_ALLOWED_LOGS], > > [])") > > > +AT_CLEANUP > > > > I didn't try to play with these tests, so I'm curious how certain > > you're that these tests will catch configuration problems for the policer? > > > > Database read-write is mostly independent from the implementation and > > AFAICT the configuration failure will not be propagated to the > > database, so I'm not sure if we need to read those values back. > > tests/ovs-vsctl.at already includes database tests for the policer. > > We can keep reads here, but only for consistency, I guess. They > > doesn't seem to have a lot of test value by themselves. > > These tests were based off similar tests in VSPERF which is why I included > those > checks. You are correct in thinking that they are not testing the database but > instead just confirming the output value matches the value in the create > command. I think this is something that would be done by a typical OvS user so > maybe the check is worth keeping? > > > > The only issue that these tests should be able to catch is the ERR log > > "Could not create rte meter for ingress policer". Tests are not > > checking for it directly, but relying on the ovs-vswitch.log to not > > have any errors at the end of the test. That might be fine, but maybe > > we need to be more clear about that in the test comments as we have no > > real way right now to tell if the policer was actually created or removed. > > You're right, it is probably better to directly check and mark the test as > failed if > the error is found, rather than not explicitly checking for it and marking > the test > as passed if it is not found in the final log. I will make this change for > all 4 tests > and also add a comment explaining the process better in the next version. > > > > What do you think? > > Overall, are you satisfied that the 4 tests are different enough to each > other and > that each of them adds some value to the test suite? > > > > Best regards, Ilya Maximets. > > Kind regards, > Michael.
Hi Ilya, I have implemented your suggested changes and pushed a v3 to the mailing list which can be found here: http://patchwork.ozlabs.org/project/openvswitch/patch/[email protected]/. I would greatly appreciate it if you could take a look at the new version and give me any feedback you might have on it. Kind regards, Michael. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
