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 <i.maxim...@ovn.org>
> Sent: Wednesday 4 May 2022 11:33
> To: Phelan, Michael <michael.phe...@intel.com>; d...@openvswitch.org;
> Stokes, Ian <ian.sto...@intel.com>
> Cc: maxime.coque...@redhat.com; david.march...@redhat.com; Ryan,
> Seamus <seamus.r...@intel.com>; i.maxim...@ovn.org
> 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 <seamus.r...@intel.com>
> >
> > This adds 4 new unit tests to the 'check-dpdk' subsystem that will
> > test rate limiting functionality.
> >
> > Signed-off-by: Seamus Ryan <seamus.r...@intel.com>
> > Signed-off-by: Michael Phelan <michael.phe...@intel.com>
> > Co-authored-by: Michael Phelan <michael.phe...@intel.com>
> >
> > ---
> > 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.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
