Message ID | 20240109132854.669822-5-mheib@redhat.com |
---|---|
State | Superseded |
Headers | show |
Series | OVN-IC: add basic sequence number status support | expand |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | success | apply and check: success |
ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
On Tue, Jan 9, 2024 at 2:29 PM Mohammad Heib <mheib@redhat.com> wrote: > add unit test that check validate that sync command > sync ISB properly > > Signed-off-by: Mohammad Heib <mheib@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 > dev@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
Hi Ales, On Fri, Jan 12, 2024 at 11:12 AM Ales Musil <amusil@redhat.com> wrote: > > > On Tue, Jan 9, 2024 at 2:29 PM Mohammad Heib <mheib@redhat.com> wrote: > >> add unit test that check validate that sync command >> sync ISB properly >> >> Signed-off-by: Mohammad Heib <mheib@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 >> dev@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> > > amusil@redhat.com > <https://red.ht/sig> >
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 &) + +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 + +# 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]) + +# 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]) + +OVN_CLEANUP_IC([az1]) +AT_CLEANUP +])
add unit test that check validate that sync command sync ISB properly Signed-off-by: Mohammad Heib <mheib@redhat.com> --- tests/ovn-ic.at | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)