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

[David Marchand]

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]

> +# 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.


-- 
David Marchand