| Message ID | 20190724144224.32884-1-aserdean@ovn.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] windows, tests: Modify service test | expand |
On 7/24/19 4:42 PM, Alin Gabriel Serdean wrote: > The database is now called "_Server" so look for that instead of "Open_vSwitch". > > Modify the test so it always stops and deletes the service. > > Signed-off-by: Alin Gabriel Serdean <aserdean@ovn.org> > --- Hi. Thanks for the fix. It looks OK to me in general, but it seems like it was never accepted or even reviewed. See one comment inline. Best regards, Ilya Maximets. > tests/daemon.at | 12 ++++-------- > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/tests/daemon.at b/tests/daemon.at > index bdc8910f9..ff1953633 100644 > --- a/tests/daemon.at > +++ b/tests/daemon.at > @@ -212,17 +212,13 @@ abs_path="$(cd $(dirname `which ovsdb-server`); pwd -W; cd $OLDPWD)" > AT_CHECK([sc create ovsdb-server binpath="$abs_path/ovsdb-server --no-db --log-file=`pwd`/ovsdb-server.log --pidfile=`pwd`/ovsdb-server.pid --unixctl=`pwd`/ovsdb-server.ctl --remote=punix:`pwd`/socket --service"], > [0], [[[SC]] CreateService SUCCESS > ]) > - > -AT_CHECK([sc start ovsdb-server], [0], [ignore], [ignore], [sc delete ovsdb-server]) > -OVS_WAIT_UNTIL([test -s ovsdb-server.pid]) > +on_exit 'sc delete ovsdb-server' > +on_exit 'sc stop ovsdb-server' > +AT_CHECK([sc start ovsdb-server], [0], [ignore]) > OVS_WAIT_UNTIL([sc query ovsdb-server | grep STATE | grep RUNNING > /dev/null 2>&1]) > -AT_CHECK([kill -0 `cat ovsdb-server.pid`], [0], [ignore]) > AT_CHECK([ovs-appctl -t ovsdb-server ovsdb-server/list-dbs], [0], > -[Open_vSwitch > +[_Server > ]) > AT_CHECK([sc stop ovsdb-server], [0], [ignore]) > OVS_WAIT_UNTIL([test ! -s ovsdb-server.pid]) > -AT_CHECK([sc query ovsdb-server | grep STATE | grep STOPPED], [0], [ignore]) > -AT_CHECK([sc delete ovsdb-server], [0], [[[SC]] DeleteService SUCCESS > -]) I'd like to keep these explicit checks for 'STOPPED' state and deletion success. I see that we have 'on_exit' now, but this test was designed to test graceful service start/stop/delete and code executed during cleanup will not check for any issues. So, I think, that we need to test graseful termination and have 'on_exit' hooks just in case some part of the test failed early. BTW, It might make sense to replace AT_CHECK with OVS_WAIT_UNTIL for checking the 'STOPPED' state, the same way as it done for 'RUNNING'. What do you think? Best regards, Ilya Maximets.
Thanks a lot for the review! It’s been some time since I proposed the patch, I think I wanted to make sure the service was deleted, but I agree with your suggestions. I sent an incremental here: https://patchwork.ozlabs.org/project/openvswitch/patch/20200923112247.738-1-aserdean@ovn.org/ Alin. From: Ilya Maximets<mailto:i.maximets@ovn.org> Sent: Thursday, September 10, 2020 3:07 PM To: Alin Gabriel Serdean<mailto:aserdean@ovn.org>; dev@openvswitch.org<mailto:dev@openvswitch.org> Cc: i.maximets@ovn.org<mailto:i.maximets@ovn.org> Subject: Re: [ovs-dev] [PATCH] windows, tests: Modify service test On 7/24/19 4:42 PM, Alin Gabriel Serdean wrote: > The database is now called "_Server" so look for that instead of "Open_vSwitch". > > Modify the test so it always stops and deletes the service. > > Signed-off-by: Alin Gabriel Serdean <aserdean@ovn.org> > --- Hi. Thanks for the fix. It looks OK to me in general, but it seems like it was never accepted or even reviewed. See one comment inline. Best regards, Ilya Maximets. > tests/daemon.at | 12 ++++-------- > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/tests/daemon.at b/tests/daemon.at > index bdc8910f9..ff1953633 100644 > --- a/tests/daemon.at > +++ b/tests/daemon.at > @@ -212,17 +212,13 @@ abs_path="$(cd $(dirname `which ovsdb-server`); pwd -W; cd $OLDPWD)" > AT_CHECK([sc create ovsdb-server binpath="$abs_path/ovsdb-server --no-db --log-file=`pwd`/ovsdb-server.log --pidfile=`pwd`/ovsdb-server.pid --unixctl=`pwd`/ovsdb-server.ctl --remote=punix:`pwd`/socket --service"], > [0], [[[SC]] CreateService SUCCESS > ]) > - > -AT_CHECK([sc start ovsdb-server], [0], [ignore], [ignore], [sc delete ovsdb-server]) > -OVS_WAIT_UNTIL([test -s ovsdb-server.pid]) > +on_exit 'sc delete ovsdb-server' > +on_exit 'sc stop ovsdb-server' > +AT_CHECK([sc start ovsdb-server], [0], [ignore]) > OVS_WAIT_UNTIL([sc query ovsdb-server | grep STATE | grep RUNNING > /dev/null 2>&1]) > -AT_CHECK([kill -0 `cat ovsdb-server.pid`], [0], [ignore]) > AT_CHECK([ovs-appctl -t ovsdb-server ovsdb-server/list-dbs], [0], > -[Open_vSwitch > +[_Server > ]) > AT_CHECK([sc stop ovsdb-server], [0], [ignore]) > OVS_WAIT_UNTIL([test ! -s ovsdb-server.pid]) > -AT_CHECK([sc query ovsdb-server | grep STATE | grep STOPPED], [0], [ignore]) > -AT_CHECK([sc delete ovsdb-server], [0], [[[SC]] DeleteService SUCCESS > -]) I'd like to keep these explicit checks for 'STOPPED' state and deletion success. I see that we have 'on_exit' now, but this test was designed to test graceful service start/stop/delete and code executed during cleanup will not check for any issues. So, I think, that we need to test graseful termination and have 'on_exit' hooks just in case some part of the test failed early. BTW, It might make sense to replace AT_CHECK with OVS_WAIT_UNTIL for checking the 'STOPPED' state, the same way as it done for 'RUNNING'. What do you think? Best regards, Ilya Maximets.
On 9/23/20 6:37 PM, Alin Serdean wrote: > Thanks a lot for the review! > > > > It’s been some time since I proposed the patch, I think I wanted to make sure > > the service was deleted, but I agree with your suggestions. > > I sent an incremental here: > > https://patchwork.ozlabs.org/project/openvswitch/patch/20200923112247.738-1-aserdean@ovn.org/ Hmm. That's a different patch. Is it a wrong link? > > > > Alin. > > > > *From: *Ilya Maximets <mailto:i.maximets@ovn.org> > *Sent: *Thursday, September 10, 2020 3:07 PM > *To: *Alin Gabriel Serdean <mailto:aserdean@ovn.org>; dev@openvswitch.org <mailto:dev@openvswitch.org> > *Cc: *i.maximets@ovn.org <mailto:i.maximets@ovn.org> > *Subject: *Re: [ovs-dev] [PATCH] windows, tests: Modify service test > > > > On 7/24/19 4:42 PM, Alin Gabriel Serdean wrote: >> The database is now called "_Server" so look for that instead of "Open_vSwitch". >> >> Modify the test so it always stops and deletes the service. >> >> Signed-off-by: Alin Gabriel Serdean <aserdean@ovn.org> >> --- > > Hi. Thanks for the fix. > > It looks OK to me in general, but it seems like it was never accepted or even reviewed. > > See one comment inline. > > Best regards, Ilya Maximets. > >> tests/daemon.at | 12 ++++-------- >> 1 file changed, 4 insertions(+), 8 deletions(-) >> >> diff --git a/tests/daemon.at b/tests/daemon.at >> index bdc8910f9..ff1953633 100644 >> --- a/tests/daemon.at >> +++ b/tests/daemon.at >> @@ -212,17 +212,13 @@ abs_path="$(cd $(dirname `which ovsdb-server`); pwd -W; cd $OLDPWD)" >> AT_CHECK([sc create ovsdb-server binpath="$abs_path/ovsdb-server --no-db --log-file=`pwd`/ovsdb-server.log --pidfile=`pwd`/ovsdb-server.pid --unixctl=`pwd`/ovsdb-server.ctl --remote=punix:`pwd`/socket --service"], >> [0], [[[SC]] CreateService SUCCESS >> ]) >> - >> -AT_CHECK([sc start ovsdb-server], [0], [ignore], [ignore], [sc delete ovsdb-server]) >> -OVS_WAIT_UNTIL([test -s ovsdb-server.pid]) >> +on_exit 'sc delete ovsdb-server' >> +on_exit 'sc stop ovsdb-server' >> +AT_CHECK([sc start ovsdb-server], [0], [ignore]) >> OVS_WAIT_UNTIL([sc query ovsdb-server | grep STATE | grep RUNNING > /dev/null 2>&1]) >> -AT_CHECK([kill -0 `cat ovsdb-server.pid`], [0], [ignore]) >> AT_CHECK([ovs-appctl -t ovsdb-server ovsdb-server/list-dbs], [0], >> -[Open_vSwitch >> +[_Server >> ]) >> AT_CHECK([sc stop ovsdb-server], [0], [ignore]) >> OVS_WAIT_UNTIL([test ! -s ovsdb-server.pid]) >> -AT_CHECK([sc query ovsdb-server | grep STATE | grep STOPPED], [0], [ignore]) >> -AT_CHECK([sc delete ovsdb-server], [0], [[[SC]] DeleteService SUCCESS >> -]) > > I'd like to keep these explicit checks for 'STOPPED' state and deletion success. > I see that we have 'on_exit' now, but this test was designed to test graceful > service start/stop/delete and code executed during cleanup will not check for > any issues. So, I think, that we need to test graseful termination and have > 'on_exit' hooks just in case some part of the test failed early. > BTW, It might make sense to replace AT_CHECK with OVS_WAIT_UNTIL for checking > the 'STOPPED' state, the same way as it done for 'RUNNING'. > > What do you think? > > Best regards, Ilya Maximets. > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > >
diff --git a/tests/daemon.at b/tests/daemon.at index bdc8910f9..ff1953633 100644 --- a/tests/daemon.at +++ b/tests/daemon.at @@ -212,17 +212,13 @@ abs_path="$(cd $(dirname `which ovsdb-server`); pwd -W; cd $OLDPWD)" AT_CHECK([sc create ovsdb-server binpath="$abs_path/ovsdb-server --no-db --log-file=`pwd`/ovsdb-server.log --pidfile=`pwd`/ovsdb-server.pid --unixctl=`pwd`/ovsdb-server.ctl --remote=punix:`pwd`/socket --service"], [0], [[[SC]] CreateService SUCCESS ]) - -AT_CHECK([sc start ovsdb-server], [0], [ignore], [ignore], [sc delete ovsdb-server]) -OVS_WAIT_UNTIL([test -s ovsdb-server.pid]) +on_exit 'sc delete ovsdb-server' +on_exit 'sc stop ovsdb-server' +AT_CHECK([sc start ovsdb-server], [0], [ignore]) OVS_WAIT_UNTIL([sc query ovsdb-server | grep STATE | grep RUNNING > /dev/null 2>&1]) -AT_CHECK([kill -0 `cat ovsdb-server.pid`], [0], [ignore]) AT_CHECK([ovs-appctl -t ovsdb-server ovsdb-server/list-dbs], [0], -[Open_vSwitch +[_Server ]) AT_CHECK([sc stop ovsdb-server], [0], [ignore]) OVS_WAIT_UNTIL([test ! -s ovsdb-server.pid]) -AT_CHECK([sc query ovsdb-server | grep STATE | grep STOPPED], [0], [ignore]) -AT_CHECK([sc delete ovsdb-server], [0], [[[SC]] DeleteService SUCCESS -]) AT_CLEANUP
The database is now called "_Server" so look for that instead of "Open_vSwitch". Modify the test so it always stops and deletes the service. Signed-off-by: Alin Gabriel Serdean <aserdean@ovn.org> --- tests/daemon.at | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)