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

[Kevin Traynor]

On 12/03/2021 13:21, Pai G, Sunil wrote:
> Hey Kevin, 
> 
> Tested the patch , works fine for me too :)
> 
>>
>> 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
>> ])
>> ])
>>
> 
> I agree with David on this one. It might be good to create a helper like CHECK_LOAD_PARAM.
> 
>>
>>
>>> +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.
> 
> I couldn't find the doc where the max value is mentioned.
> Would you mind pointing it ?
> Or its calculated somehow ? 
> 

Thanks Sunil (and David). The max is mentioned here:
https://github.com/openvswitch/ovs/blob/master/vswitchd/vswitch.xml#L677-L679

We probably need to think a bit more about the units and max for this
parameter in general, but that's what's currently documented.

Kevin.


> The rest looks to good to me!
> 
> <snip>
>