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

Alin Serdean aserdean at cloudbasesolutions.com
Wed Sep 23 10:42:17 UTC 2020


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-aserdean@ovn.org/


Thanks,
Alin.

From: Ilya Maximets<mailto:i.maximets at ovn.org>
Sent: Thursday, September 10, 2020 2:55 PM
To: Alin Gabriel Serdean<mailto:aserdean at ovn.org>; dev at openvswitch.org<mailto:dev at openvswitch.org>
Cc: i.maximets at ovn.org<mailto:i.maximets at 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 <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.
_______________________________________________
dev mailing list
dev at openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev



More information about the dev mailing list