Hi Ilya, Thank you so much for the review!
You are right the PID in double quotes was not being propagated properly. I digged deeper to problem and the issue was actually a carriage return which is passed to the kill wrapper we use on Windows. This will mess up the search string we use here: https://github.com/openvswitch/ovs/blob/master/tests/ovs-macros.at#L114 I posted a patch to strip the line endings here: https://patchwork.ozlabs.org/project/openvswitch/patch/20200923103955.1694-1-aserd...@ovn.org/ Thanks, Alin. From: Ilya Maximets<mailto:i.maxim...@ovn.org> Sent: Thursday, September 10, 2020 2:55 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] tests: Queue for termination all OVSDB IDL pids On 7/24/19 4:32 PM, Alin Gabriel Serdean wrote: > When running OVSDB cluster tests on Windows not all the ovsdb processes are > terminated. > Queue up the pids of the started processes for termination when the test > stops. > > Signed-off-by: Alin Gabriel Serdean <aserd...@ovn.org> > --- Hi, Alin. Thanks for the fix! I'm looking through old patches in patchwork and this one seems to be never reviewed, but it targets a valid issue. See my review comments inline. > tests/ovsdb-idl.at | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at > index 7c937f742..fd9e973cd 100644 > --- a/tests/ovsdb-idl.at > +++ b/tests/ovsdb-idl.at > @@ -27,8 +27,8 @@ ovsdb_cluster_start_idltest () { > done > for i in `seq $n`; do > ovsdb-server -vraft -vconsole:warn --detach --no-chdir > --log-file=s$i.log --pidfile=s$i.pid --unixctl=s$i --remote=punix:s$i.ovsdb > ${2:+--remote=$2} s$i.db || return $? > + on_exit "kill `cat s$i.pid`" There is one issue with this solution. Previously expression in a single quotes was evaluated at the cleanup time, but changing it to double quotes makes the shell to process expression right away. It's possible that pidfile is not exist yet at this point or not filled with correct pid and resulted cleanup code will not kill the process in this case. 2 options here: 1. Wait for the pidfile. 2. Move previous "on_exit 'kill `cat s*.pid`'" before the loop. Option #2 is preferred since it will not add any extra time cost on waiting for the pidfile and actually covers all possible cases. It will work just fine because actual expansion of the * and actual 'cat' are executed during cleanup. Also, this patch needs a rebase. Would you mind implementing option #2 on top of current master and send v2? 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
