[ovs-dev] [PATCH 1/1] tests: Add PMD auto load balance unit tests.

Kevin Traynor ktraynor at redhat.com
Mon Mar 15 18:33:24 UTC 2021


On 09/03/2021 15:21, David Marchand wrote:
> On Thu, Feb 18, 2021 at 11:48 AM Kevin Traynor <ktraynor at redhat.com> wrote:
>>
>> These tests focus on enabling/disabling and user parameters.
>>
>> Signed-off-by: Kevin Traynor <ktraynor at redhat.com>
> 
> The patch looks good to me and I tested it with no issue.
> 
> Just two small comments, see below.
> 
> 
>> ---
>>  tests/alb.at       | 256 +++++++++++++++++++++++++++++++++++++++++++++
>>  tests/automake.mk  |   1 +
>>  tests/testsuite.at |   1 +
>>  3 files changed, 258 insertions(+)
>>  create mode 100644 tests/alb.at
>>
>> diff --git a/tests/alb.at b/tests/alb.at
>> new file mode 100644
>> index 000000000..136c26c44
>> --- /dev/null
>> +++ b/tests/alb.at
>> @@ -0,0 +1,256 @@
>> +AT_BANNER([PMD Auto Load Balance])
>> +
>> +m4_divert_push([PREPARE_TESTS])
>> +
>> +get_log_line_num () {
>> +    LINENUM=$(cat ovs-vswitchd.log | wc -l | tr -d [[:blank:]])
>> +}
>> +
>> +m4_divert_pop([PREPARE_TESTS])
>> +
>> +m4_define([DUMMY_NUMA], [--dummy-numa="0,0"])
>> +
>> +dnl CHECK_INTERVAL_PARAM([interval_time],  [+line])
>> +dnl
>> +dnl Waits for alb interval time in logs and checks if time matches
>> +dnl $1. Checking starts from line number 'line' in ovs-vswithd.log .
>> +m4_define([CHECK_INTERVAL_PARAM], [
>> +    PATTERN="PMD auto load balance interval set to [[0-9]]* mins"
>> +    line_st=$2
>> +    if [[ -z "$line_st" ]]
>> +    then
>> +        line_st="+0"
>> +    fi
>> +    OVS_WAIT_UNTIL([tail -n $line_st ovs-vswitchd.log | grep "$PATTERN"])
>> +    TIME=$(tail -n $line_st ovs-vswitchd.log | grep "$PATTERN" | tail -1 | sed -e 's/.* \([[0-9]]*\) mins/\1/')
>> +    AT_CHECK([test "$TIME" -eq "$1"])
> 
> The ALB log messages follow a common format, so I'd suggest combining
> those 3 helpers as a single.
> That is with the hope we will conform to this format for new parameters later.
> 
> dnl CHECK_ALB_PARAM([param], [value], [+line])
> dnl
> dnl Waits for ALB logs for 'param' in logs and checks if value matches
> dnl 'value'. Checking starts from line number 'line' in ovs-vswithd.log.
> m4_define([CHECK_ALB_PARAM], [
>     line_st=$3
>     if [[ -z "$line_st" ]]
>     then
>         line_st="+0"
>     fi
>     OVS_WAIT_UNTIL([tail -n $line_st ovs-vswitchd.log | grep "PMD auto
> load balance $1 set to"])
>     AT_CHECK([tail -n $line_st ovs-vswitchd.log | sed -n "s#.*\(PMD
> auto load balance $1 set to.*\)#\1#p" | tail -1], [0], [dnl
> PMD auto load balance $1 set to $2
> ])
> ])
> 
> [snip]
> 
> 
>> +AT_SETUP([ALB - interval param])
>> +OVS_VSWITCHD_START
>> +OVS_WAIT_UNTIL([grep "PMD auto load balance is disabled" ovs-vswitchd.log])
>> +
>> +# Check default
>> +CHECK_INTERVAL_PARAM([1], [])
> 
> Here, it becomes:
> CHECK_ALB_PARAM([interval], [1 mins], [])
> 
> etc..
> 
> [snip]
> 

Thanks, this is neater. I'll incorporate it into v2.

>> +# TODO 20000 is documented as max value but it seems arbitary
>> +# Setting to 20001 works, should it be checked and rejected?
>> +# For now add a dummy test above the max value that accepts it
> 
> Afaiu, from ovsdb pov, other_config is a simple map of strings with no
> constraint on the content of the strings.
> I understand the constraints expressed for sub fields like
> pmd-auto-lb-XX parameters in vswitchd/vswitchd.xml as only for
> documentation.
> 
> I don't see the benefit of checking values 20000 or 20001.
> 
> 

As it stands, without this limit enforced, it is implicitly limited to
INT_MAX and even that should not cause any problems with usage, though
traffic patterns may have changed a bit during the 4K years, so it may
not be optimal :-)

I'll remove this these tests and if we add enforcement on this limit in
the future we can add a UT at that point.



More information about the dev mailing list