On 23/06/2021 21:49, Ilya Maximets wrote:
> On 3/16/21 4:45 PM, Kevin Traynor wrote:
>> These tests focus on enabling/disabling and user parameters.
>>
>> Co-Authored-by: David Marchand <david.march...@redhat.com>
>> Signed-off-by: David Marchand <david.march...@redhat.com>
>> Signed-off-by: Kevin Traynor <ktray...@redhat.com>
>>
>> ---
>> v2:
>> - Remove above max documented interval test
>> - Add David's code to combine param checks and add as co-author
>> ---
>> tests/alb.at | 218 +++++++++++++++++++++++++++++++++++++++++++++
>> tests/automake.mk | 1 +
>> tests/testsuite.at | 1 +
>> 3 files changed, 220 insertions(+)
>> create mode 100644 tests/alb.at
>
> Hi, Kevin. While testing these tests I noticed one thing:
>
> get_log_line_num() returns current line and not the next one, so if log
> didn't change, several subsequent get_log_line_num + OVS_WAIT_UNTIL
> will succeed. Meaning that it maybe unreliable to test for the same
> text in a log two times in a row with some command in-between, because
> command may return faster than logs printed to a file and the check
> will be performed with the previous line in a log. Suggesting to
> increase the line number by one to avoid that. I understand that you
> ported this part from the pmd.at, so we, probably, need to fix that
> there too in a separate change.
>
Hi Ilya, that makes sense. Even if it was ok for the current tests due
to the particular logs, there is a danger that it would be reused again
and cause an issue then. I will update here and send a patch for pmd.at.
> Suggesting following incremental:
>
> diff --git a/tests/alb.at b/tests/alb.at
> index 0ea1bbdd1..1331b742c 100644
> --- a/tests/alb.at
> +++ b/tests/alb.at
> @@ -3,7 +3,7 @@ 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:]])
> + LINENUM=$(($(cat ovs-vswitchd.log | wc -l | tr -d [[:blank:]])+1))
> }
>
> m4_divert_pop([PREPARE_TESTS])
> @@ -21,7 +21,8 @@ m4_define([CHECK_ALB_PARAM], [
> 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
> + AT_CHECK([tail -n $line_st ovs-vswitchd.log dnl
> + | sed -n "s#.*\(PMD auto load balance $1 set to.*\)#\1#p" |
> tail -1], [0], [dnl
> PMD auto load balance $1 set to $2
> ])
> ])
> @@ -107,7 +108,9 @@ OVS_WAIT_UNTIL([tail -n +$LINENUM ovs-vswitchd.log | grep
> "PMD auto load balance
> get_log_line_num
> AT_CHECK([ovs-vsctl set open_vswitch . other_config:pmd-auto-lb="false"])
> OVS_WAIT_UNTIL([tail -n +$LINENUM ovs-vswitchd.log | grep "PMD auto load
> balance is disabled"])
> +get_log_line_num
> AT_CHECK([ovs-vsctl set Open_vSwitch .
> other_config:pmd-rxq-assign=roundrobin])
> +OVS_WAIT_UNTIL([tail -n +$LINENUM ovs-vswitchd.log | grep "mode changed to:
> 'roundrobin'"])
> get_log_line_num
> AT_CHECK([ovs-vsctl set open_vswitch . other_config:pmd-auto-lb="true"])
> OVS_WAIT_UNTIL([tail -n +$LINENUM ovs-vswitchd.log | grep "PMD auto load
> balance is disabled"])
> ---
>
> What do you think?
> The check around 'roundrobin' is just in case, to be sure that
> log actually updated.
>
Yeah, good idea. In earlier cases it is changing between enable/disable,
but I also added similar check on assignment mode change, just to be
explicit that it is the reason.
thanks,
Kevin.
> Best regards, Ilya Maximets.
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
