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:
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
