[ovs-dev] [PATCH] tests: Queue for termination all OVSDB IDL pids

[Ilya Maximets]

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 <aserdean at 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.