Hi Ales,
On Fri, Jan 12, 2024 at 11:12 AM Ales Musil <amu...@redhat.com> wrote:
>
>
> On Tue, Jan 9, 2024 at 2:29 PM Mohammad Heib <mh...@redhat.com> wrote:
>
>> add unit test that check validate that sync command
>> sync ISB properly
>>
>> Signed-off-by: Mohammad Heib <mh...@redhat.com>
>> ---
>>
>
> Hi Mohammad,
>
> I have a few comments down below.
>
>
>> tests/ovn-ic.at | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 47 insertions(+)
>>
>> diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at
>> index d4c436f84..f55ffa6cd 100644
>> --- a/tests/ovn-ic.at
>> +++ b/tests/ovn-ic.at
>> @@ -1274,3 +1274,50 @@ OVN_CLEANUP_IC([az1], [az2])
>>
>> AT_CLEANUP
>> ])
>> +
>> +AT_SETUP([ovn-ic -- sync ISB status to INB])
>> +ovn_init_ic_db
>> +net_add n1
>> +
>> +ovn_start az1
>> +sim_add gw-az1
>> +as gw-az1
>> +
>> +check ovs-vsctl add-br br-phys
>> +ovn_az_attach az1 n1 br-phys 192.168.1.1
>> +check ovs-vsctl set open . external-ids:ovn-is-interconn=true
>> +as az1
>> +
>> +# pause ovn-ic instance
>> +check ovn-appctl -t ic/ovn-ic pause
>> +
>> +# run sync command in the background this commands
>> +# supposed to stuck since ovn-ic is paused.
>> +$(ovn-ic-nbctl --wait=sb sync &)
>>
>
> The extra $() around shouldn't be needed.
>
>
>> +
>> +for i in {1..5}; do
>> + set -- $(ovn-ic-nbctl get ic_nb_global . nb_ic_cfg sb_ic_cfg)
>> + AS_VAR_SET([ic_nb_cfg], [$1])
>> + AS_VAR_SET([ic_sb_cfg], [$2])
>> + if test $ic_nb_cfg -gt $ic_sb_cfg; then
>> + sleep 1
>> + else
>> + break
>> + fi
>> +done
>>
>
> Why not to use OVS_WAIT_UNTIL?
>
>
>> +
>> +# if ic_nb_cfg equal to ic_sb_cfg that mean both zero
>> +# or both 1 which is a not correct and the test must fail.
>> +AT_FAIL_IF([test $ic_nb_cfg == $ic_sb_cfg])
>>
>
> AT_CHECK would be better suited for this.
>
>
>> +
>> +# resume ovn-ic instance
>> +check ovn-appctl -t ic/ovn-ic resume
>> +set -- $(ovn-ic-nbctl get ic_nb_global . nb_ic_cfg sb_ic_cfg)
>>
>
> We should wait after "resume" for the values to settle down.
>
>
>> +AS_VAR_SET([ic_nb_cfg], [$1])
>> +AS_VAR_SET([ic_sb_cfg], [$2])
>> +AT_FAIL_IF([test $ic_nb_cfg != 1])
>> +AT_FAIL_IF([test $ic_nb_cfg != 1])
>>
>
> Same as above, AT_CHECK would be better.
>
>
>> +
>> +OVN_CLEANUP_IC([az1])
>> +AT_CLEANUP
>> +])
>> --
>> 2.34.3
>>
>> _______________________________________________
>> dev mailing list
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>>
> The test seems to be overly complicated for what it tries to achieve. I
> would suggest the following diff:
>
Thank you so much for reviewing the patch,
I agree with you the test is complicated, I will go with your suggestions
in v5,
Thanks :)
>
> diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at
> index f55ffa6cd..32df3be2c 100644
> --- a/tests/ovn-ic.at
> +++ b/tests/ovn-ic.at
> @@ -1293,30 +1293,25 @@ check ovn-appctl -t ic/ovn-ic pause
>
> # run sync command in the background this commands
> # supposed to stuck since ovn-ic is paused.
> -$(ovn-ic-nbctl --wait=sb sync &)
> -
> -for i in {1..5}; do
> - set -- $(ovn-ic-nbctl get ic_nb_global . nb_ic_cfg sb_ic_cfg)
> - AS_VAR_SET([ic_nb_cfg], [$1])
> - AS_VAR_SET([ic_sb_cfg], [$2])
> - if test $ic_nb_cfg -gt $ic_sb_cfg; then
> - sleep 1
> - else
> - break
> - fi
> -done
> +ovn-ic-nbctl --wait=sb sync &
>
> -# if ic_nb_cfg equal to ic_sb_cfg that mean both zero
> -# or both 1 which is a not correct and the test must fail.
> -AT_FAIL_IF([test $ic_nb_cfg == $ic_sb_cfg])
> +OVS_WAIT_UNTIL([test $(ovn-ic-nbctl get ic_nb_global . nb_ic_cfg) -gt
> $(ovn-ic-nbctl get ic_nb_global . sb_ic_cfg)])
> +AT_CHECK([ovn-ic-nbctl get ic_nb_global . nb_ic_cfg], [0], [dnl
> +1
> +])
> +AT_CHECK([ovn-ic-nbctl get ic_nb_global . sb_ic_cfg], [0], [dnl
> +0
> +])
>
> # resume ovn-ic instance
> check ovn-appctl -t ic/ovn-ic resume
> -set -- $(ovn-ic-nbctl get ic_nb_global . nb_ic_cfg sb_ic_cfg)
> -AS_VAR_SET([ic_nb_cfg], [$1])
> -AS_VAR_SET([ic_sb_cfg], [$2])
> -AT_FAIL_IF([test $ic_nb_cfg != 1])
> -AT_FAIL_IF([test $ic_nb_cfg != 1])
> +OVS_WAIT_UNTIL([test $(ovn-ic-nbctl get ic_nb_global . nb_ic_cfg) -eq
> $(ovn-ic-nbctl get ic_nb_global . sb_ic_cfg)])
> +AT_CHECK([ovn-ic-nbctl get ic_nb_global . nb_ic_cfg], [0], [dnl
> +1
> +])
> +AT_CHECK([ovn-ic-nbctl get ic_nb_global . sb_ic_cfg], [0], [dnl
> +1
> +])
>
> OVN_CLEANUP_IC([az1])
> AT_CLEANUP
>
>
> Thanks,
> Ales
>
> --
>
> Ales Musil
>
> Senior Software Engineer - OVN Core
>
> Red Hat EMEA <https://www.redhat.com>
>
> amu...@redhat.com
> <https://red.ht/sig>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
