| Message ID | 20190724143234.32368-1-aserdean@ovn.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] tests: Queue for termination all OVSDB IDL pids | expand |
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@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.
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@ovn.org> Sent: Thursday, September 10, 2020 2:55 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] 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@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.
On 9/23/20 12:42 PM, Alin Serdean wrote: > 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! I'll take a look. But anyway, patch below seems still valid (with my comments addressed) just because if one of ovsdb-servers will fail to start for any reason other servers will not be killed. So, If you could update the patch and send v2 it would be good. Best regards, Ilya Maximets. > > > > > > Thanks, > > Alin. > > > > *From: *Ilya Maximets <mailto:i.maximets@ovn.org> > *Sent: *Thursday, September 10, 2020 2:55 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] 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@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@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > >
> > I posted a patch to strip the line endings here: > > https://patchwork.ozlabs.org/project/openvswitch/patch/20200923103955.1694-1-aserdean@ovn.org/ Thanks! I'll take a look. But anyway, patch below seems still valid (with my comments addressed) just because if one of ovsdb-servers will fail to start for any reason other servers will not be killed. So, If you could update the patch and send v2 it would be good. Best regards, Ilya Maximets. [Alin] Of course! I sent an incremental here: https://patchwork.ozlabs.org/project/openvswitch/patch/20200923112247.738-1-aserdean@ovn.org/
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`" done - on_exit 'kill `cat s*.pid`' } # ovsdb_cluster_leader [REMOTES] [DATABASE]
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@ovn.org> --- tests/ovsdb-idl.at | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)