| Message ID | 20180306111712.3484-1-aserdean@ovn.org |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [ovs-dev] tests-windows: Fix SSL ovsdb test which is hanging | expand |
On Tue, Mar 06, 2018 at 01:17:12PM +0200, Alin Gabriel Serdean wrote: > The test: > `1827. ovsdb-server.at:490: testing SSL db: implementation ...` > is hanging on Windows because the returned in the case the client failed to > connect is "Unknown error" vs the normal "Protocol error". > > Update the test to accommodate for this. > > Signed-off-by: Alin Gabriel Serdean <aserdean@ovn.org> Thanks for finding and fixing the error. I think that there is an underlying problem here: the code is bad at error handling in general. It's supposed to kill the ovsdb-server if anything fails, but it didn't do it right: it killed $(cat pid) but the pidfile is actually in ovsdb-server.pid. This meant that the issue showed up as a hang instead of a test failure, which seems bad. Also, I'm kind of inclined to just disregard the particular error message. It might save time later. (I guess there's a risk that the connection fails happens for some other reason that should fail the test though.) Anyway, I sent a pair of patches that implement my suggestions: https://patchwork.ozlabs.org/patch/882796/ https://patchwork.ozlabs.org/patch/882798/ What are your thoughts? Thanks, Ben.
> -----Mesaj original----- > De la: ovs-dev-bounces@openvswitch.org <ovs-dev- > bounces@openvswitch.org> În numele Ben Pfaff > Trimis: Wednesday, March 7, 2018 11:20 PM > Către: Alin Gabriel Serdean <aserdean@ovn.org> > Cc: dev@openvswitch.org > Subiect: Re: [ovs-dev] [PATCH] tests-windows: Fix SSL ovsdb test which is > hanging > > On Tue, Mar 06, 2018 at 01:17:12PM +0200, Alin Gabriel Serdean wrote: > > The test: > > `1827. ovsdb-server.at:490: testing SSL db: implementation ...` is > > hanging on Windows because the returned in the case the client failed > > to connect is "Unknown error" vs the normal "Protocol error". > > > > Update the test to accommodate for this. > > > > Signed-off-by: Alin Gabriel Serdean <aserdean@ovn.org> > > Thanks for finding and fixing the error. > > I think that there is an underlying problem here: the code is bad at error > handling in general. It's supposed to kill the ovsdb-server if anything fails, > but it didn't do it right: it killed $(cat pid) but the pidfile is actually in ovsdb- > server.pid. This meant that the issue showed up as a hang instead of a test > failure, which seems bad. > > Also, I'm kind of inclined to just disregard the particular error message. It > might save time later. (I guess there's a risk that the connection fails > happens for some other reason that should fail the test > though.) > > Anyway, I sent a pair of patches that implement my suggestions: > https://patchwork.ozlabs.org/patch/882796/ > https://patchwork.ozlabs.org/patch/882798/ > > What are your thoughts? > > Thanks, > > Ben. [Alin Serdean] I'm dropping this change and acking yours. They are much cleaner and better 😊.
On Thu, Mar 08, 2018 at 01:45:51AM +0200, aserdean@ovn.org wrote: > > -----Mesaj original----- > > De la: ovs-dev-bounces@openvswitch.org <ovs-dev- > > bounces@openvswitch.org> În numele Ben Pfaff > > Trimis: Wednesday, March 7, 2018 11:20 PM > > Către: Alin Gabriel Serdean <aserdean@ovn.org> > > Cc: dev@openvswitch.org > > Subiect: Re: [ovs-dev] [PATCH] tests-windows: Fix SSL ovsdb test which is > > hanging > > > > On Tue, Mar 06, 2018 at 01:17:12PM +0200, Alin Gabriel Serdean wrote: > > > The test: > > > `1827. ovsdb-server.at:490: testing SSL db: implementation ...` is > > > hanging on Windows because the returned in the case the client failed > > > to connect is "Unknown error" vs the normal "Protocol error". > > > > > > Update the test to accommodate for this. > > > > > > Signed-off-by: Alin Gabriel Serdean <aserdean@ovn.org> > > > > Thanks for finding and fixing the error. > > > > I think that there is an underlying problem here: the code is bad at error > > handling in general. It's supposed to kill the ovsdb-server if anything fails, > > but it didn't do it right: it killed $(cat pid) but the pidfile is actually in ovsdb- > > server.pid. This meant that the issue showed up as a hang instead of a test > > failure, which seems bad. > > > > Also, I'm kind of inclined to just disregard the particular error message. It > > might save time later. (I guess there's a risk that the connection fails > > happens for some other reason that should fail the test > > though.) > > > > Anyway, I sent a pair of patches that implement my suggestions: > > https://patchwork.ozlabs.org/patch/882796/ > > https://patchwork.ozlabs.org/patch/882798/ > > > > What are your thoughts? > > > > Thanks, > > > > Ben. > [Alin Serdean] I'm dropping this change and acking yours. > They are much cleaner and better 😊. Thanks! I applied both of these fixes to master. Do you want me to backport them to branch-2.9 (or earlier)? I have not done that yet.
> -----Mesaj original----- > De la: ovs-dev-bounces@openvswitch.org <ovs-dev- > bounces@openvswitch.org> În numele Ben Pfaff > Trimis: Thursday, March 8, 2018 2:11 AM > Către: aserdean@ovn.org > Cc: dev@openvswitch.org > Subiect: Re: [ovs-dev] [PATCH] tests-windows: Fix SSL ovsdb test which is > hanging > > On Thu, Mar 08, 2018 at 01:45:51AM +0200, aserdean@ovn.org wrote: > > > -----Mesaj original----- > > > De la: ovs-dev-bounces@openvswitch.org <ovs-dev- > > > bounces@openvswitch.org> În numele Ben Pfaff > > > Trimis: Wednesday, March 7, 2018 11:20 PM > > > Către: Alin Gabriel Serdean <aserdean@ovn.org> > > > Cc: dev@openvswitch.org > > > Subiect: Re: [ovs-dev] [PATCH] tests-windows: Fix SSL ovsdb test > > > which is hanging > > > > > > On Tue, Mar 06, 2018 at 01:17:12PM +0200, Alin Gabriel Serdean wrote: > > > > The test: > > > > `1827. ovsdb-server.at:490: testing SSL db: implementation ...` is > > > > hanging on Windows because the returned in the case the client > > > > failed to connect is "Unknown error" vs the normal "Protocol error". > > > > > > > > Update the test to accommodate for this. > > > > > > > > Signed-off-by: Alin Gabriel Serdean <aserdean@ovn.org> > > > > > > Thanks for finding and fixing the error. > > > > > > I think that there is an underlying problem here: the code is bad at > > > error handling in general. It's supposed to kill the ovsdb-server > > > if anything fails, but it didn't do it right: it killed $(cat pid) > > > but the pidfile is actually in ovsdb- server.pid. This meant that > > > the issue showed up as a hang instead of a test failure, which seems bad. > > > > > > Also, I'm kind of inclined to just disregard the particular error > > > message. It might save time later. (I guess there's a risk that > > > the connection fails happens for some other reason that should fail > > > the test > > > though.) > > > > > > Anyway, I sent a pair of patches that implement my suggestions: > > > https://patchwork.ozlabs.org/patch/882796/ > > > https://patchwork.ozlabs.org/patch/882798/ > > > > > > What are your thoughts? > > > > > > Thanks, > > > > > > Ben. > > [Alin Serdean] I'm dropping this change and acking yours. > > They are much cleaner and better 😊. > > Thanks! I applied both of these fixes to master. > > Do you want me to backport them to branch-2.9 (or earlier)? I have not done > that yet. branch-2.9 also please.
On Thu, Mar 08, 2018 at 02:22:45AM +0200, aserdean@ovn.org wrote: > > -----Mesaj original----- > > De la: ovs-dev-bounces@openvswitch.org <ovs-dev- > > bounces@openvswitch.org> În numele Ben Pfaff > > Trimis: Thursday, March 8, 2018 2:11 AM > > Către: aserdean@ovn.org > > Cc: dev@openvswitch.org > > Subiect: Re: [ovs-dev] [PATCH] tests-windows: Fix SSL ovsdb test which is > > hanging > > > > On Thu, Mar 08, 2018 at 01:45:51AM +0200, aserdean@ovn.org wrote: > > > > -----Mesaj original----- > > > > De la: ovs-dev-bounces@openvswitch.org <ovs-dev- > > > > bounces@openvswitch.org> În numele Ben Pfaff > > > > Trimis: Wednesday, March 7, 2018 11:20 PM > > > > Către: Alin Gabriel Serdean <aserdean@ovn.org> > > > > Cc: dev@openvswitch.org > > > > Subiect: Re: [ovs-dev] [PATCH] tests-windows: Fix SSL ovsdb test > > > > which is hanging > > > > > > > > On Tue, Mar 06, 2018 at 01:17:12PM +0200, Alin Gabriel Serdean wrote: > > > > > The test: > > > > > `1827. ovsdb-server.at:490: testing SSL db: implementation ...` is > > > > > hanging on Windows because the returned in the case the client > > > > > failed to connect is "Unknown error" vs the normal "Protocol error". > > > > > > > > > > Update the test to accommodate for this. > > > > > > > > > > Signed-off-by: Alin Gabriel Serdean <aserdean@ovn.org> > > > > > > > > Thanks for finding and fixing the error. > > > > > > > > I think that there is an underlying problem here: the code is bad at > > > > error handling in general. It's supposed to kill the ovsdb-server > > > > if anything fails, but it didn't do it right: it killed $(cat pid) > > > > but the pidfile is actually in ovsdb- server.pid. This meant that > > > > the issue showed up as a hang instead of a test failure, which seems bad. > > > > > > > > Also, I'm kind of inclined to just disregard the particular error > > > > message. It might save time later. (I guess there's a risk that > > > > the connection fails happens for some other reason that should fail > > > > the test > > > > though.) > > > > > > > > Anyway, I sent a pair of patches that implement my suggestions: > > > > https://patchwork.ozlabs.org/patch/882796/ > > > > https://patchwork.ozlabs.org/patch/882798/ > > > > > > > > What are your thoughts? > > > > > > > > Thanks, > > > > > > > > Ben. > > > [Alin Serdean] I'm dropping this change and acking yours. > > > They are much cleaner and better 😊. > > > > Thanks! I applied both of these fixes to master. > > > > Do you want me to backport them to branch-2.9 (or earlier)? I have not done > > that yet. > > branch-2.9 also please. > Done.
> -----Mesaj original----- > De la: ovs-dev-bounces@openvswitch.org <ovs-dev- > bounces@openvswitch.org> În numele Ben Pfaff > Trimis: Thursday, March 8, 2018 3:07 AM > Către: aserdean@ovn.org > Cc: dev@openvswitch.org > Subiect: Re: [ovs-dev] [PATCH] tests-windows: Fix SSL ovsdb test which is > hanging > > On Thu, Mar 08, 2018 at 02:22:45AM +0200, aserdean@ovn.org wrote: > > > -----Mesaj original----- > > > De la: ovs-dev-bounces@openvswitch.org <ovs-dev- > > > bounces@openvswitch.org> În numele Ben Pfaff > > > Trimis: Thursday, March 8, 2018 2:11 AM > > > Către: aserdean@ovn.org > > > Cc: dev@openvswitch.org > > > Subiect: Re: [ovs-dev] [PATCH] tests-windows: Fix SSL ovsdb test > > > which is hanging > > > > > > On Thu, Mar 08, 2018 at 01:45:51AM +0200, aserdean@ovn.org wrote: > > > > > -----Mesaj original----- > > > > > De la: ovs-dev-bounces@openvswitch.org <ovs-dev- > > > > > bounces@openvswitch.org> În numele Ben Pfaff > > > > > Trimis: Wednesday, March 7, 2018 11:20 PM > > > > > Către: Alin Gabriel Serdean <aserdean@ovn.org> > > > > > Cc: dev@openvswitch.org > > > > > Subiect: Re: [ovs-dev] [PATCH] tests-windows: Fix SSL ovsdb test > > > > > which is hanging > > > > > > > > > > On Tue, Mar 06, 2018 at 01:17:12PM +0200, Alin Gabriel Serdean > wrote: > > > > > > The test: > > > > > > `1827. ovsdb-server.at:490: testing SSL db: implementation > > > > > > ...` is hanging on Windows because the returned in the case > > > > > > the client failed to connect is "Unknown error" vs the normal > "Protocol error". > > > > > > > > > > > > Update the test to accommodate for this. > > > > > > > > > > > > Signed-off-by: Alin Gabriel Serdean <aserdean@ovn.org> > > > > > > > > > > Thanks for finding and fixing the error. > > > > > > > > > > I think that there is an underlying problem here: the code is > > > > > bad at error handling in general. It's supposed to kill the > > > > > ovsdb-server if anything fails, but it didn't do it right: it > > > > > killed $(cat pid) but the pidfile is actually in ovsdb- > > > > > server.pid. This meant that the issue showed up as a hang instead of > a test failure, which seems bad. > > > > > > > > > > Also, I'm kind of inclined to just disregard the particular > > > > > error message. It might save time later. (I guess there's a > > > > > risk that the connection fails happens for some other reason > > > > > that should fail the test > > > > > though.) > > > > > > > > > > Anyway, I sent a pair of patches that implement my suggestions: > > > > > https://patchwork.ozlabs.org/patch/882796/ > > > > > https://patchwork.ozlabs.org/patch/882798/ > > > > > > > > > > What are your thoughts? > > > > > > > > > > Thanks, > > > > > > > > > > Ben. > > > > [Alin Serdean] I'm dropping this change and acking yours. > > > > They are much cleaner and better 😊. > > > > > > Thanks! I applied both of these fixes to master. > > > > > > Do you want me to backport them to branch-2.9 (or earlier)? I have > > > not done that yet. > > > > branch-2.9 also please. > > > > Done. Thank you!
diff --git a/tests/ovsdb-server.at b/tests/ovsdb-server.at index 968356781..7d94e1c71 100644 --- a/tests/ovsdb-server.at +++ b/tests/ovsdb-server.at @@ -569,11 +569,19 @@ AT_CHECK( [stderr], [test ! -e pid || kill `cat pid`]) cat stderr > output -AT_CHECK_UNQUOTED( - [grep "failed to connect" output], [0], - [ovsdb-client: failed to connect to "ssl:127.0.0.1:$SSL_PORT" (Protocol error) -], - [ignore], [test ! -e pid || kill `cat pid`]) +if test "$IS_WIN32" = "yes"; then + AT_CHECK_UNQUOTED( + [grep "failed to connect" output], [0], + [ovsdb-client: failed to connect to "ssl:127.0.0.1:$SSL_PORT" (Unknown error) +], + [ignore], [test ! -e pid || kill `cat pid`]) +else + AT_CHECK_UNQUOTED( + [grep "failed to connect" output], [0], + [ovsdb-client: failed to connect to "ssl:127.0.0.1:$SSL_PORT" (Protocol error) +], + [ignore], [test ! -e pid || kill `cat pid`]) +fi # Check that when ciphers are not compatible, that a negotiation # failure occurs. AT_CHECK( @@ -593,11 +601,19 @@ AT_CHECK( [stderr], [test ! -e pid || kill `cat pid`]) cat stderr > output -AT_CHECK_UNQUOTED( - [grep "failed to connect" output], [0], - [ovsdb-client: failed to connect to "ssl:127.0.0.1:$SSL_PORT" (Protocol error) -], - [ignore], [test ! -e pid || kill `cat pid`]) +if test "$IS_WIN32" = "yes"; then + AT_CHECK_UNQUOTED( + [grep "failed to connect" output], [0], + [ovsdb-client: failed to connect to "ssl:127.0.0.1:$SSL_PORT" (Unknown error) +], + [ignore], [test ! -e pid || kill `cat pid`]) +else + AT_CHECK_UNQUOTED( + [grep "failed to connect" output], [0], + [ovsdb-client: failed to connect to "ssl:127.0.0.1:$SSL_PORT" (Protocol error) +], + [ignore], [test ! -e pid || kill `cat pid`]) +fi # The error message for being unable to negotiate a shared ciphersuite # is 'sslv3 alert handshake failure'. This is not the clearest message. AT_CHECK_UNQUOTED(
The test: `1827. ovsdb-server.at:490: testing SSL db: implementation ...` is hanging on Windows because the returned in the case the client failed to connect is "Unknown error" vs the normal "Protocol error". Update the test to accommodate for this. Signed-off-by: Alin Gabriel Serdean <aserdean@ovn.org> --- tests/ovsdb-server.at | 36 ++++++++++++++++++++++++++---------- 1 file changed, 26 insertions(+), 10 deletions(-)