You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2017/11/03 19:41:58 UTC
[kudu-CR] WIP: test and fixes for tls socket EINTR issues
Hello Alexey Serbin,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/8462
to review the following change.
Change subject: WIP: test and fixes for tls socket EINTR issues
......................................................................
WIP: test and fixes for tls socket EINTR issues
Change-Id: Ibec9049186f79f1c43295e4735538ed7ba4e516e
---
M src/kudu/security/CMakeLists.txt
M src/kudu/security/tls_handshake.cc
A src/kudu/security/tls_socket-test.cc
M src/kudu/security/tls_socket.cc
M src/kudu/util/net/socket.cc
5 files changed, 215 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/8462/1
--
To view, visit http://gerrit.cloudera.org:8080/8462
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ibec9049186f79f1c43295e4735538ed7ba4e516e
Gerrit-Change-Number: 8462
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
[kudu-CR] WIP: test and fixes for tls socket EINTR issues
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has uploaded a new patch set (#4) to the change originally created by Todd Lipcon. ( http://gerrit.cloudera.org:8080/8462 )
Change subject: WIP: test and fixes for tls socket EINTR issues
......................................................................
WIP: test and fixes for tls socket EINTR issues
Change-Id: Ibec9049186f79f1c43295e4735538ed7ba4e516e
---
M src/kudu/security/CMakeLists.txt
M src/kudu/security/tls_handshake.cc
A src/kudu/security/tls_socket-test.cc
M src/kudu/security/tls_socket.cc
M src/kudu/util/net/socket.cc
5 files changed, 222 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/8462/4
--
To view, visit http://gerrit.cloudera.org:8080/8462
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibec9049186f79f1c43295e4735538ed7ba4e516e
Gerrit-Change-Number: 8462
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] WIP: test and fixes for tls socket EINTR issues
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has uploaded a new patch set (#3) to the change originally created by Todd Lipcon. ( http://gerrit.cloudera.org:8080/8462 )
Change subject: WIP: test and fixes for tls socket EINTR issues
......................................................................
WIP: test and fixes for tls socket EINTR issues
Change-Id: Ibec9049186f79f1c43295e4735538ed7ba4e516e
---
M src/kudu/security/CMakeLists.txt
M src/kudu/security/tls_handshake.cc
A src/kudu/security/tls_socket-test.cc
M src/kudu/security/tls_socket.cc
M src/kudu/util/net/socket.cc
5 files changed, 219 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/8462/3
--
To view, visit http://gerrit.cloudera.org:8080/8462
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibec9049186f79f1c43295e4735538ed7ba4e516e
Gerrit-Change-Number: 8462
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [security] test and fixes for TLS socket EINTR issues
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8462 )
Change subject: [security] test and fixes for TLS socket EINTR issues
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/8462
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibec9049186f79f1c43295e4735538ed7ba4e516e
Gerrit-Change-Number: 8462
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 06 Nov 2017 19:20:07 +0000
Gerrit-HasComments: No
[kudu-CR] WIP: test and fixes for tls socket EINTR issues
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8462 )
Change subject: WIP: test and fixes for tls socket EINTR issues
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/8462/1/src/kudu/security/tls_socket-test.cc
File src/kudu/security/tls_socket-test.cc:
http://gerrit.cloudera.org:8080/#/c/8462/1/src/kudu/security/tls_socket-test.cc@20
PS1, Line 20: #include <signal.h>
> warning: #includes are not sorted properly [llvm-include-order]
Done
http://gerrit.cloudera.org:8080/#/c/8462/1/src/kudu/security/tls_socket-test.cc@50
PS1, Line 50: using std::vector;
> warning: using decl 'vector' is unused [misc-unused-using-decls]
Done
http://gerrit.cloudera.org:8080/#/c/8462/1/src/kudu/security/tls_socket-test.cc@99
PS1, Line 99: received = string((char*)&buf[0], n);
> warning: C-style casts are discouraged; use reinterpret_cast [google-readab
Done
http://gerrit.cloudera.org:8080/#/c/8462/1/src/kudu/security/tls_socket-test.cc@106
PS1, Line 106: void handler(int sig) {}
> warning: parameter 'sig' is unused [misc-unused-parameters]
Done
--
To view, visit http://gerrit.cloudera.org:8080/8462
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibec9049186f79f1c43295e4735538ed7ba4e516e
Gerrit-Change-Number: 8462
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 03 Nov 2017 20:40:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP: test and fixes for tls socket EINTR issues
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8462 )
Change subject: WIP: test and fixes for tls socket EINTR issues
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8462/1/src/kudu/security/tls_socket-test.cc
File src/kudu/security/tls_socket-test.cc:
http://gerrit.cloudera.org:8080/#/c/8462/1/src/kudu/security/tls_socket-test.cc@181
PS1, Line 181: }
> Mind calling client_sock->Close()?
Oh, ignore that one.
I mean it would be easier to follow if the client explicitly closed the connection and set 'stop == true' and not calling the SleepFor() in the last iteration.
--
To view, visit http://gerrit.cloudera.org:8080/8462
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibec9049186f79f1c43295e4735538ed7ba4e516e
Gerrit-Change-Number: 8462
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 03 Nov 2017 22:20:49 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP: test and fixes for tls socket EINTR issues
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8462 )
Change subject: WIP: test and fixes for tls socket EINTR issues
......................................................................
Patch Set 1:
(10 comments)
Great! It seems I should have wrote such a test instead of relying on client-stress-test scenario.
http://gerrit.cloudera.org:8080/#/c/8462/1/src/kudu/security/tls_socket-test.cc
File src/kudu/security/tls_socket-test.cc:
http://gerrit.cloudera.org:8080/#/c/8462/1/src/kudu/security/tls_socket-test.cc@24
PS1, Line 24: #include <syscall.h>
This does not compile on OS X. Mind using pthread interface instead of Linux-specific ones?
http://gerrit.cloudera.org:8080/#/c/8462/1/src/kudu/security/tls_socket-test.cc@125
PS1, Line 125: int server_tid = 0
pthread_t ?
http://gerrit.cloudera.org:8080/#/c/8462/1/src/kudu/security/tls_socket-test.cc@128
PS1, Line 128: Thread::CurrentThreadId();
pthread_self() ?
http://gerrit.cloudera.org:8080/#/c/8462/1/src/kudu/security/tls_socket-test.cc@158
PS1, Line 158: syscall(SYS_tgkill, getpid(), server_tid, SIGUSR2);
I think it's better to use portable pthread_kill() here. Then we can run this test on OS X as well.
http://gerrit.cloudera.org:8080/#/c/8462/1/src/kudu/security/tls_socket-test.cc@179
PS1, Line 179: CHECK_OK
Any specific reason to use CHECK_OK instead of ASSERT_OK here?
http://gerrit.cloudera.org:8080/#/c/8462/1/src/kudu/security/tls_socket-test.cc@181
PS1, Line 181: }
Mind calling client_sock->Close()?
I spent some time trying to understand whether TlsSocket::Recv() can return SSL_ERROR_WANT_READ
http://gerrit.cloudera.org:8080/#/c/8462/1/src/kudu/security/tls_socket.cc
File src/kudu/security/tls_socket.cc:
http://gerrit.cloudera.org:8080/#/c/8462/1/src/kudu/security/tls_socket.cc@60
PS1, Line 60: if (error_code == SSL_ERROR_WANT_WRITE) {
: // Socket not ready to write yet.
: *nwritten = 0;
: return Status::OK();
: }
As I understand, the same story is with SSL_write() -- it's necessary to add handling EINTR here.
http://gerrit.cloudera.org:8080/#/c/8462/1/src/kudu/security/tls_socket.cc@107
PS1, Line 107: SSL_ERROR_WANT_READ
Mind adding SSL_ERROR_WANT_WRITE here as well? Our current client does not try to re-negotiate, but having SSL_ERROR_WANT_WRITE would be compliant with OpenSSL docs.
http://gerrit.cloudera.org:8080/#/c/8462/1/src/kudu/security/tls_socket.cc@108
PS1, Line 108: save_errno == EAGAIN
I don't think we need this. Is there any specific case where it's needed besides EINTR?
As I understand, from the upper level (Socket::BlockingRecv()) that would be a timeout if this method returned status with EAGAIN from here.
http://gerrit.cloudera.org:8080/#/c/8462/1/src/kudu/util/net/socket.cc
File src/kudu/util/net/socket.cc:
http://gerrit.cloudera.org:8080/#/c/8462/1/src/kudu/util/net/socket.cc@366
PS1, Line 366: accept4
There isn't accept4() on OS X. I think it's better to leave accept() here.
--
To view, visit http://gerrit.cloudera.org:8080/8462
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibec9049186f79f1c43295e4735538ed7ba4e516e
Gerrit-Change-Number: 8462
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 03 Nov 2017 22:10:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] [security] test and fixes for TLS socket EINTR issues
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8462 )
Change subject: [security] test and fixes for TLS socket EINTR issues
......................................................................
Patch Set 5: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/8462
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibec9049186f79f1c43295e4735538ed7ba4e516e
Gerrit-Change-Number: 8462
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 06 Nov 2017 17:14:58 +0000
Gerrit-HasComments: No
[kudu-CR] WIP: test and fixes for tls socket EINTR issues
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8462 )
Change subject: WIP: test and fixes for tls socket EINTR issues
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8462/1/src/kudu/security/tls_socket.cc
File src/kudu/security/tls_socket.cc:
http://gerrit.cloudera.org:8080/#/c/8462/1/src/kudu/security/tls_socket.cc@108
PS1, Line 108: save_errno == EAGAIN
> I don't think we need this. Is there any specific case where it's needed b
Or this is for the case when the socket is set in non-blocking mode?
--
To view, visit http://gerrit.cloudera.org:8080/8462
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibec9049186f79f1c43295e4735538ed7ba4e516e
Gerrit-Change-Number: 8462
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 03 Nov 2017 22:37:25 +0000
Gerrit-HasComments: Yes
[kudu-CR] [security] test and fixes for TLS socket EINTR issues
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8462 )
Change subject: [security] test and fixes for TLS socket EINTR issues
......................................................................
[security] test and fixes for TLS socket EINTR issues
SSL_{read,write}() can return SSL_ERROR_WANT_{READ,WRITE}
correspondingly when signal interrupts recv()/send() calls even if
SSL_MODE_AUTO_RETRY is set in the TLS context. To handle that
properly in Socket::Blocking{Recv,Write}() methods, return
NetworkError() with appropriate POSIX error code from
TlsSocket::{Recv,Write}().
As a by-product, this changelist fixes flakiness in TestUniqueClientIds
scenario of the ClientStressTest test and other flaky tests which failed
with errors like below:
Bad status: IO error: Could not connect to the cluster: \
Client connection negotiation failed: client connection to \
IP:port: Read zero bytes on a blocking Recv() call: \
Transferred 0 of 4 bytes
Prior to this fix, the test failure ratio observed with dist-test
for TSAN builds was about 6% in multiple 1K runs. After the fix,
no failures observed.
Change-Id: Ibec9049186f79f1c43295e4735538ed7ba4e516e
Reviewed-on: http://gerrit.cloudera.org:8080/8462
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert <da...@apache.org>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/security/CMakeLists.txt
M src/kudu/security/tls_handshake.cc
A src/kudu/security/tls_socket-test.cc
M src/kudu/security/tls_socket.cc
M src/kudu/util/net/socket.cc
5 files changed, 227 insertions(+), 7 deletions(-)
Approvals:
Kudu Jenkins: Verified
Dan Burkert: Looks good to me, but someone else must approve
Alexey Serbin: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/8462
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ibec9049186f79f1c43295e4735538ed7ba4e516e
Gerrit-Change-Number: 8462
Gerrit-PatchSet: 6
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [security] test and fixes for TLS socket EINTR issues
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has uploaded a new patch set (#5) to the change originally created by Todd Lipcon. ( http://gerrit.cloudera.org:8080/8462 )
Change subject: [security] test and fixes for TLS socket EINTR issues
......................................................................
[security] test and fixes for TLS socket EINTR issues
SSL_{read,write}() can return SSL_ERROR_WANT_{READ,WRITE}
correspondingly when signal interrupts recv()/send() calls even if
SSL_MODE_AUTO_RETRY is set in the TLS context. To handle that
properly in Socket::Blocking{Recv,Write}() methods, return
NetworkError() with appropriate POSIX error code from
TlsSocket::{Recv,Write}().
As a by-product, this changelist fixes flakiness in TestUniqueClientIds
scenario of the ClientStressTest test and other flaky tests which failed
with errors like below:
Bad status: IO error: Could not connect to the cluster: \
Client connection negotiation failed: client connection to \
IP:port: Read zero bytes on a blocking Recv() call: \
Transferred 0 of 4 bytes
Prior to this fix, the test failure ratio observed with dist-test
for TSAN builds was about 6% in multiple 1K runs. After the fix,
no failures observed.
Change-Id: Ibec9049186f79f1c43295e4735538ed7ba4e516e
---
M src/kudu/security/CMakeLists.txt
M src/kudu/security/tls_handshake.cc
A src/kudu/security/tls_socket-test.cc
M src/kudu/security/tls_socket.cc
M src/kudu/util/net/socket.cc
5 files changed, 227 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/8462/5
--
To view, visit http://gerrit.cloudera.org:8080/8462
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibec9049186f79f1c43295e4735538ed7ba4e516e
Gerrit-Change-Number: 8462
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>