On 09/03/2021 15:21, David Marchand wrote:
> On Thu, Feb 18, 2021 at 11:48 AM Kevin Traynor <ktray...@redhat.com> wrote:
>>
>> These tests focus on enabling/disabling and user parameters.
>>
>> Signed-off-by: Kevin Traynor <ktray...@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.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
