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-aserd...@ovn.org/
Alin. From: Ilya Maximets<mailto:i.maxim...@ovn.org> Sent: Thursday, September 10, 2020 3:07 PM To: Alin Gabriel Serdean<mailto:aserd...@ovn.org>; d...@openvswitch.org<mailto:d...@openvswitch.org> Cc: i.maxim...@ovn.org<mailto:i.maxim...@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 <aserd...@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 d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
