You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Ashwani Raina (Code Review)" <ge...@cloudera.org> on 2025/05/12 16:17:55 UTC
[kudu-CR] [socket] Fix test failure caused due to unsupported socket option
Ashwani Raina has uploaded this change for review. ( http://gerrit.cloudera.org:8080/22882
Change subject: [socket] Fix test failure caused due to unsupported socket option
......................................................................
[socket] Fix test failure caused due to unsupported socket option
Socket API (SetSockOpt) doesn't support SO_REUSEPORT for a Unix
domain socket. Due to this, following tests fail:
- client-test.TestConnectViaUnixSocket
- dns_alias-itest.TestBasic
The patch skips setting of socket option (i.e. SO_REUSEPORT) on a
Unix domain sockets. The patch also removes macro check for
SO_REUSEPORT flag because this flag has been available since
kernel version 3.9 and was backported to 2.6 versions for some
Linux distros like RHEL/CentOS.
Change-Id: Ice00b1c4fd1df78fa84c3dd2a79c968a4a91cc21
---
M src/kudu/rpc/messenger.cc
M src/kudu/util/net/socket.cc
2 files changed, 7 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/82/22882/1
--
To view, visit http://gerrit.cloudera.org:8080/22882
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ice00b1c4fd1df78fa84c3dd2a79c968a4a91cc21
Gerrit-Change-Number: 22882
Gerrit-PatchSet: 1
Gerrit-Owner: Ashwani Raina <ar...@cloudera.com>
[kudu-CR] [socket] Fix test failure caused due to unsupported socket option
Posted by "Ashwani Raina (Code Review)" <ge...@cloudera.org>.
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/22882 )
Change subject: [socket] Fix test failure caused due to unsupported socket option
......................................................................
Patch Set 1:
> Patch Set 1:
>
> (1 comment)
The two tests are consistently failing on my local Ubuntu 20.04 machine for debug build (rebase 9e661cb34).
It appears that in my case, for some reason, the socket shutdown is failing consistently due to which subsequent operation of setting SO_REUSEPORT option fails on unix domain socket. If socket shutdown completes successfully, setting SO_REUSEPORT option may not fail. I will need to validate this theory though, by running some tests.
Considering above explanation holds, we need to address two issues:
1. Ensure proper socket cleanup is done successfully in order to avoid reuseport failure.
2. Ensure unix domain sockets are not unknowingly used for SO_REUSEPORT socket option and expected to work fine.
--
To view, visit http://gerrit.cloudera.org:8080/22882
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ice00b1c4fd1df78fa84c3dd2a79c968a4a91cc21
Gerrit-Change-Number: 22882
Gerrit-PatchSet: 1
Gerrit-Owner: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 13 May 2025 07:25:40 +0000
Gerrit-HasComments: No
[kudu-CR] [socket] Fix test failure caused due to unsupported socket option
Posted by "Ashwani Raina (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22882
to look at the new patch set (#6).
Change subject: [socket] Fix test failure caused due to unsupported socket option
......................................................................
[socket] Fix test failure caused due to unsupported socket option
setsockopt() system call doesn't support SO_REUSEPORT for a Unix domain
socket. Due to this, following tests fail on certain linux versions
(e.g. v6.15):
- client-test.TestConnectViaUnixSocket
- dns_alias-itest.TestBasic
Although there is no support available, a graceful error handling was
recently added as part of [1] commit. Linux kernel versions where [1]
is present, error out with appropriate message and test also fails
expectedly. However, for linux kernel versions (e.g. v5.15) where [1]
is not present, the tests may pass, but setting of SO_REUSEPORT socket
option may have no effect at all, potentially leading to confusion and
undefined behavior in tests.
The patch does the following to fix this:
1. Error out with appropriate error message if setting socket option
(i.e. SO_REUSEPORT) on a Unix domain socket.
2. Add a group flag validator to ensure either of two flags is set:
--rpc_listen_on_unix_domain_socket or --rpc_reuseport.
3. Explicitly initialise --rpc_reuseport flag in tests wherever
required as per test expectations.
[1] https://github.com/torvalds/linux/commit/5b0af621c3f6ef9261cf6067812f2fd9943acb4b
Change-Id: Ice00b1c4fd1df78fa84c3dd2a79c968a4a91cc21
---
M src/kudu/integration-tests/dns_alias-itest.cc
M src/kudu/master/mini_master.cc
M src/kudu/rpc/messenger.cc
M src/kudu/server/server_base.cc
4 files changed, 27 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/82/22882/6
--
To view, visit http://gerrit.cloudera.org:8080/22882
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ice00b1c4fd1df78fa84c3dd2a79c968a4a91cc21
Gerrit-Change-Number: 22882
Gerrit-PatchSet: 6
Gerrit-Owner: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [socket] Fix test failure caused due to unsupported socket option
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22882 )
Change subject: [socket] Fix test failure caused due to unsupported socket option
......................................................................
Patch Set 4:
Build Aborted
http://jenkins.kudu.apache.org/job/pre_commit/1402/ : ABORTED
--
To view, visit http://gerrit.cloudera.org:8080/22882
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ice00b1c4fd1df78fa84c3dd2a79c968a4a91cc21
Gerrit-Change-Number: 22882
Gerrit-PatchSet: 4
Gerrit-Owner: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 30 May 2025 07:58:24 +0000
Gerrit-HasComments: No
[kudu-CR] [socket] Fix test failure caused due to unsupported socket option
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/22882 )
Change subject: [socket] Fix test failure caused due to unsupported socket option
......................................................................
[socket] Fix test failure caused due to unsupported socket option
setsockopt() system call doesn't support SO_REUSEPORT for a Unix domain
socket. Due to this, following tests fail on certain linux versions
(e.g. v6.15):
- client-test.TestConnectViaUnixSocket
- dns_alias-itest.TestBasic
Although there is no support available, a graceful error handling was
recently added as part of [1] commit. Linux kernel versions where [1]
is present, error out with appropriate message and test also fails
expectedly. However, for linux kernel versions (e.g. v5.15) where [1]
is not present, the tests may pass, but setting of SO_REUSEPORT socket
option may have no effect at all, potentially leading to confusion and
undefined behavior in tests.
The patch does the following to fix this:
1. Error out with appropriate error message if setting socket option
(i.e. SO_REUSEPORT) on a Unix domain socket.
2. Add a group flag validator to ensure either of two flags is set:
--rpc_listen_on_unix_domain_socket or --rpc_reuseport.
3. Explicitly initialise --rpc_reuseport flag in tests wherever
required as per test expectations.
[1] https://github.com/torvalds/linux/commit/5b0af621c3f6ef9261cf6067812f2fd9943acb4b
Change-Id: Ice00b1c4fd1df78fa84c3dd2a79c968a4a91cc21
Reviewed-on: http://gerrit.cloudera.org:8080/22882
Reviewed-by: Alexey Serbin <al...@apache.org>
Tested-by: Alexey Serbin <al...@apache.org>
---
M src/kudu/integration-tests/dns_alias-itest.cc
M src/kudu/master/mini_master.cc
M src/kudu/rpc/messenger.cc
M src/kudu/server/server_base.cc
4 files changed, 27 insertions(+), 4 deletions(-)
Approvals:
Alexey Serbin: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/22882
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ice00b1c4fd1df78fa84c3dd2a79c968a4a91cc21
Gerrit-Change-Number: 22882
Gerrit-PatchSet: 7
Gerrit-Owner: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [socket] Fix test failure caused due to unsupported socket option
Posted by "Ashwani Raina (Code Review)" <ge...@cloudera.org>.
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/22882 )
Change subject: [socket] Fix test failure caused due to unsupported socket option
......................................................................
Patch Set 2:
(2 comments)
> Patch Set 1:
>
> (2 comments)
>
> > > Patch Set 1:
> > >
> > > > > Patch Set 1:
> > > > >
> > > > > > Patch Set 1:
> > > > > >
> > > > > > (1 comment)
> > > > >
> > > > > The two tests are consistently failing on my local Ubuntu
> > 20.04
> > > > machine for debug build (rebase 9e661cb34).
> > > > >
> > > > > It appears that in my case, for some reason, the socket
> > shutdown
> > > > is failing consistently due to which subsequent operation of
> > > > setting SO_REUSEPORT option fails on unix domain socket. If
> > socket
> > > > shutdown completes successfully, setting SO_REUSEPORT option
> > may
> > > > not fail. I will need to validate this theory though, by
> > running
> > > > some tests.
> > > > >
> > > > > Considering above explanation holds, we need to address two
> > > > issues:
> > > > > 1. Ensure proper socket cleanup is done successfully in
> > order to
> > > > avoid reuseport failure.
> > > > > 2. Ensure unix domain sockets are not unknowingly used for
> > > > SO_REUSEPORT socket option and expected to work fine.
> > > >
> > > >
> > > > I ran a sample routine, that sets socket option(SO_REUSEPORT)
> > on a
> > > > AF_UNIX type socket, on different linux kernel versions
> > (5.15.0 and
> > > > 6.15-rc7) and it became clear that kernel versions containing
> > > > commit [1] are gracefully erroring out on non-inet sockets.
> > The
> > > > change was introduced to fix a different issue however.
> > > >
> > > > [1]: https://github.com/torvalds/linux/commit/5b0af621c3f6ef9261cf6067812f2fd9943acb4b
> > > >
> > > > Socket cleanup issue (mentioned above) turns out to be
> > something
> > > > different and could be related to C++ version installed on my
> > > > ubuntu linux variant. I haven't figured that out yet.
> > > >
> > > > I still believe, we need to make sure we are not unknowingly
> > using
> > > > SO_REUSEPORT option while setting sockops on a AF_UNIX type
> > socket.
> > > > Current change should help in doing that.
> > >
> > > OK, but why to change socket.cc?
> > >
> > > Also, maybe update the description with your findings, since the
> > statement that SO_REUSEPORT for a Unix domain socket isn't
> > supported isn't true.
> >
> > SO_REUSEPORT macro has been available there since kernel 3.9 and it
> > would be safe to assume that it is available in all the linux
> > distros today, so there is no need to have this macro availability
> > check, hence the change in socket.cc.
>
> This doesn't explain why this change is needed to fix the tests that you reported as failing. As I understand, the change in socket.cc in PS1 is unrelated to changes needed to address the failing tests. What benefit does the change in socket.cc bring in? Did it fix or improve anything, even in a theoretical use case? I don't see much of that, but I might be missing something. However, I have one concern with the change in socket.cc: it might break compilation at some of the platforms listed as supported at https://kudu.apache.org/docs/installation.html.
>
> Did you verify that Kudu source code still compiles with this update on all the OSes listed there? Even if you did or you know there is nothing to break, I'd rather see the update on socket.cc published in a separate changelist since it's not related to fixing the failing tests. If you feel strongly about this and prefer lumping it with the rest of the changes in this patch, then I'm fine with keeping the update on socket.cc here as well, but otherwise I encourage you to post the changes in socket.cc in a separate patch.
>
> >
> > SO_REUSEPORT for a Unix domain socket is not supported is true
> > because there is no proper handling of such a case in kernel. Also,
> > there is no equivalent of sin_port in sockaddr_un structure. For
> > the kernel versions (with [1] commit), setsockopt fails with
> > appropriate error which is good because we will get to know the
> > error before bind operation. However, if there is a wrong
> > expectation of successfully setting reuseport option on unix domain
> > socket, we can certainly correct that expectation with this check.
> > The check will also handle the cases where [1] is not present in
> > the kernel, potentially leading to undefined behaviour.
>
> The statement the commit description for PS1 is literally this:
> > Socket API (SetSockOpt) doesn't support SO_REUSEPORT for a Unix domain socket.
>
> SetSockOpt() is Kudu's API, so maybe that was the source of my confusion. As one can see, SetSockOpt() simply forwards everything down to the setsockopt() syscall, but there is nothing specific about Unix domain sockets regarding particular socket options in SetSockOpt() itself. It seems you rather meant setsockopt() syscall. Indeed -- the port number isn't applicable for Unix socket family, so SO_REUSEPORT isn't relevant for AF_UNIX sockets. Please consider updating the commit description accordingly.
>
> Also, please add the kernel version information for the platform where you saw those two tests failing. Adding the relevant information regarding the change in setsockopt() behavior for SO_REUSEPORT in case of AF_UNIX sockets should be great as well.
>
> Thank you!
I have added more detail in the commit message. Hope that clarifies the doubts/questions.
http://gerrit.cloudera.org:8080/#/c/22882/1/src/kudu/rpc/messenger.cc
File src/kudu/rpc/messenger.cc:
http://gerrit.cloudera.org:8080/#/c/22882/1/src/kudu/rpc/messenger.cc@262
PS1, Line 262: Unix
> nit: UNIX
Replaced with Unix, as is everywhere else in the code.
http://gerrit.cloudera.org:8080/#/c/22882/1/src/kudu/rpc/messenger.cc@263
PS1, Line 263: (reuseport_ && accept_addr.is_unix())
> Instead of silently ignoring the 'reuseport_' option for UNIX sockets, I'd
Done
--
To view, visit http://gerrit.cloudera.org:8080/22882
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ice00b1c4fd1df78fa84c3dd2a79c968a4a91cc21
Gerrit-Change-Number: 22882
Gerrit-PatchSet: 2
Gerrit-Owner: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 26 May 2025 08:10:43 +0000
Gerrit-HasComments: Yes
[kudu-CR] [socket] Fix test failure caused due to unsupported socket option
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22882 )
Change subject: [socket] Fix test failure caused due to unsupported socket option
......................................................................
Patch Set 6: Code-Review+2
Thank you very much for addressing this!
--
To view, visit http://gerrit.cloudera.org:8080/22882
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ice00b1c4fd1df78fa84c3dd2a79c968a4a91cc21
Gerrit-Change-Number: 22882
Gerrit-PatchSet: 6
Gerrit-Owner: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 30 May 2025 14:12:00 +0000
Gerrit-HasComments: No
[kudu-CR] [socket] Fix test failure caused due to unsupported socket option
Posted by "Ashwani Raina (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22882
to look at the new patch set (#2).
Change subject: [socket] Fix test failure caused due to unsupported socket option
......................................................................
[socket] Fix test failure caused due to unsupported socket option
setsockopt() system call doesn't support SO_REUSEPORT for a Unix domain
socket. Due to this, following tests fail on certain linux versions:
- client-test.TestConnectViaUnixSocket
- dns_alias-itest.TestBasic
Although there is no support available, a graceful error handling was
recently added as part of [1] commit. Linux kernel versions where
[1] is present, error out with appropriate message and test also
fails expectedly. However, for linux kernel version where [1] is not
present, the tests may pass, but setting of SO_REUSEPORT socket option
may have no affect at all, potentially leading to confusion and undefined
behavior in tests.
The patch does the following to fix this:
1. Error out with appropriate error message if setting socket option
(i.e. SO_REUSEPORT) on a Unix domain socket.
2. Add a group flag validator to ensure either of two flags is set:
--rpc_listen_on_unix_domain_socket or --rpc_reuseport.
3. Explicitly initialise --rpc_reuseport flag in tests wherever
required as per test expectations.
[1] https://github.com/torvalds/linux/commit/5b0af621c3f6ef9261cf6067812f2fd9943acb4b
Change-Id: Ice00b1c4fd1df78fa84c3dd2a79c968a4a91cc21
---
M src/kudu/client/client-test.cc
M src/kudu/integration-tests/dns_alias-itest.cc
M src/kudu/master/master-test.cc
M src/kudu/master/mini_master-test.cc
M src/kudu/master/mini_master.cc
M src/kudu/master/sys_catalog-test.cc
M src/kudu/master/ts_state-test.cc
M src/kudu/mini-cluster/mini_cluster.cc
M src/kudu/rpc/messenger.cc
M src/kudu/server/server_base.cc
10 files changed, 65 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/82/22882/2
--
To view, visit http://gerrit.cloudera.org:8080/22882
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ice00b1c4fd1df78fa84c3dd2a79c968a4a91cc21
Gerrit-Change-Number: 22882
Gerrit-PatchSet: 2
Gerrit-Owner: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [socket] Fix test failure caused due to unsupported socket option
Posted by "Ashwani Raina (Code Review)" <ge...@cloudera.org>.
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/22882 )
Change subject: [socket] Fix test failure caused due to unsupported socket option
......................................................................
Patch Set 3:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/22882/3/src/kudu/master/mini_master.cc
File src/kudu/master/mini_master.cc:
http://gerrit.cloudera.org:8080/#/c/22882/3/src/kudu/master/mini_master.cc@61
PS3, Line 61: opts_.rpc_opts.rpc_reuseport = FLAGS_rpc_reuseport;
> Why to change this at all? Given the signature of this constructor, I'd be
The idea is to simply rely on the flag instead of hard-coded value.
Even if this is only for INET addresses, we can rely on flag instead of blindly setting the option to true.
http://gerrit.cloudera.org:8080/#/c/22882/3/src/kudu/mini-cluster/mini_cluster.cc
File src/kudu/mini-cluster/mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/22882/3/src/kudu/mini-cluster/mini_cluster.cc@72
PS3, Line 72: // SO_REUSEPORT socket option is not applicable to Unix domain sockets.
> By definition, MiniCluster::ReserveDaemonSocket() isn't supposed to be call
I left out the check in first PS with same understanding. But I was not sure whether ReserveDaemonSocket can ever be used for Unix domain sockets (GetBindIpForDaemon like method for Unix domain address and corresponding parsing method).
I guess we can skip this check for now and rely on inet_pton() inside ParseString() to do the check for us.
http://gerrit.cloudera.org:8080/#/c/22882/3/src/kudu/rpc/messenger.cc
File src/kudu/rpc/messenger.cc:
http://gerrit.cloudera.org:8080/#/c/22882/3/src/kudu/rpc/messenger.cc@263
PS3, Line 263: if (reuseport_ && accept_addr.is_unix()) {
: return Status::ConfigurationError("Port reuse is not applicable to Unix domain sockets.");
: } else if (reuseport_) {
: RETURN_NOT_OK(sock.SetReusePort(true));
: }
> nit: from the readability perspective, please consider re-structuring this
Done
http://gerrit.cloudera.org:8080/#/c/22882/3/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/22882/3/src/kudu/server/server_base.cc@516
PS3, Line 516: static
> nit: no need to add 'static' since this function is in the anonymous namesp
Done
http://gerrit.cloudera.org:8080/#/c/22882/3/src/kudu/server/server_base.cc@520
PS3, Line 520: Unix domain socket
> nit: 'a Unix domain socket' or 'Unix domain sockets'
Done
--
To view, visit http://gerrit.cloudera.org:8080/22882
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ice00b1c4fd1df78fa84c3dd2a79c968a4a91cc21
Gerrit-Change-Number: 22882
Gerrit-PatchSet: 3
Gerrit-Owner: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 28 May 2025 16:27:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] [socket] Fix test failure caused due to unsupported socket option
Posted by "Ashwani Raina (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22882
to look at the new patch set (#5).
Change subject: [socket] Fix test failure caused due to unsupported socket option
......................................................................
[socket] Fix test failure caused due to unsupported socket option
setsockopt() system call doesn't support SO_REUSEPORT for a Unix domain socket.
Due to this, following tests fail on certain linux versions (e.g. v6.15):
- client-test.TestConnectViaUnixSocket
- dns_alias-itest.TestBasic
Although there is no support available, a graceful error handling was
recently added as part of [1] commit. Linux kernel versions where [1]
is present, error out with appropriate message and test also fails
expectedly. However, for linux kernel versions (e.g. v5.15) where [1] is
not present, the tests may pass, but setting of SO_REUSEPORT socket option
may have no effect at all, potentially leading to confusion and undefined
behavior in tests.
The patch does the following to fix this:
1. Error out with appropriate error message if setting socket option
(i.e. SO_REUSEPORT) on a Unix domain socket.
2. Add a group flag validator to ensure either of two flags is set:
--rpc_listen_on_unix_domain_socket or --rpc_reuseport.
3. Explicitly initialise --rpc_reuseport flag in tests wherever
required as per test expectations.
[1] https://github.com/torvalds/linux/commit/5b0af621c3f6ef9261cf6067812f2fd9943acb4b
Change-Id: Ice00b1c4fd1df78fa84c3dd2a79c968a4a91cc21
---
M src/kudu/integration-tests/dns_alias-itest.cc
M src/kudu/master/mini_master.cc
M src/kudu/rpc/messenger.cc
M src/kudu/server/server_base.cc
4 files changed, 27 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/82/22882/5
--
To view, visit http://gerrit.cloudera.org:8080/22882
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ice00b1c4fd1df78fa84c3dd2a79c968a4a91cc21
Gerrit-Change-Number: 22882
Gerrit-PatchSet: 5
Gerrit-Owner: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [socket] Fix test failure caused due to unsupported socket option
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22882 )
Change subject: [socket] Fix test failure caused due to unsupported socket option
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/22882/3/src/kudu/master/mini_master.cc
File src/kudu/master/mini_master.cc:
http://gerrit.cloudera.org:8080/#/c/22882/3/src/kudu/master/mini_master.cc@61
PS3, Line 61: opts_.rpc_opts.rpc_reuseport = FLAGS_rpc_reuseport;
> There is one more reason that flag makes sense. This setting is applied per
Thank you for elaborating on this! Now I see that we didn't have a consistent approach w.r.t. using the --rpc_reuseport flag and RpcServerOptions in our test scaffolding. For some reason, I thought the situation was much better, but I was mistaken. And TestConnectViaUnixSocket is where the inconsistency becomes apparent, IIUC.
I agree it makes sense to resort to something similar you proposed for server_base.cc, but I'd rather vote doing so in the code that is specific to the test scaffolding itself.
What to do think if introducing an update like below into the MiniMaster's constructor?
--- a/src/kudu/master/mini_master.cc
+++ b/src/kudu/master/mini_master.cc
@@ -45,6 +45,7 @@ using std::vector;
using strings::Substitute;
DECLARE_bool(enable_minidumps);
+DECLARE_bool(rpc_listen_on_unix_domain_socket);
DECLARE_bool(rpc_server_allow_ephemeral_ports);
namespace kudu {
@@ -57,7 +58,10 @@ MiniMaster::MiniMaster(string fs_root, HostPort rpc_bind_addr, int num_data_dirs
FLAGS_enable_minidumps = false;
HostPort web_bind_addr(rpc_bind_addr_.host(), /*port=*/ 0);
opts_.rpc_opts.rpc_bind_addresses = rpc_bind_addr_.ToString();
- opts_.rpc_opts.rpc_reuseport = true;
+ // A MiniMaster enables SO_REUSEPORT on its RPC socket to simplify operation
+ // of multi-master mini-clusters, but the SO_REUSEPORT option isn't applicable
+ // for Unix domain sockets.
+ opts_.rpc_opts.rpc_reuseport = !FLAGS_rpc_listen_on_unix_domain_socket;
opts_.webserver_opts.bind_interface = web_bind_addr.host();
opts_.webserver_opts.port = web_bind_addr.port();
opts_.set_block_cache_metrics_policy(Cache::ExistingMetricsPolicy::kKeep);
--
To view, visit http://gerrit.cloudera.org:8080/22882
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ice00b1c4fd1df78fa84c3dd2a79c968a4a91cc21
Gerrit-Change-Number: 22882
Gerrit-PatchSet: 3
Gerrit-Owner: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 29 May 2025 17:56:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] [socket] Fix test failure caused due to unsupported socket option
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22882 )
Change subject: [socket] Fix test failure caused due to unsupported socket option
......................................................................
Patch Set 1: Verified-1
Build Failed
http://jenkins.kudu.apache.org/job/pre_commit/1363/ : FAILURE
--
To view, visit http://gerrit.cloudera.org:8080/22882
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ice00b1c4fd1df78fa84c3dd2a79c968a4a91cc21
Gerrit-Change-Number: 22882
Gerrit-PatchSet: 1
Gerrit-Owner: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 12 May 2025 17:14:40 +0000
Gerrit-HasComments: No
[kudu-CR] [socket] Fix test failure caused due to unsupported socket option
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22882 )
Change subject: [socket] Fix test failure caused due to unsupported socket option
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/22882/3/src/kudu/master/mini_master.cc
File src/kudu/master/mini_master.cc:
http://gerrit.cloudera.org:8080/#/c/22882/3/src/kudu/master/mini_master.cc@61
PS3, Line 61: opts_.rpc_opts.rpc_reuseport = FLAGS_rpc_reuseport;
> The idea is to simply rely on the flag instead of hard-coded value.
I don't think that explanation justifies for this change -- it simply doesn't make sense to me. MiniMaster was built to run a particular way, and this update isn't in line with that.
What is the particular need to source 'rpc_reuseport' option from FLAGS_rpc_reuseport in this particular case? Why is it necessary to change this?
If there is no particular need, please revert this change.
--
To view, visit http://gerrit.cloudera.org:8080/22882
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ice00b1c4fd1df78fa84c3dd2a79c968a4a91cc21
Gerrit-Change-Number: 22882
Gerrit-PatchSet: 3
Gerrit-Owner: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 28 May 2025 17:22:38 +0000
Gerrit-HasComments: Yes
[kudu-CR] [socket] Fix test failure caused due to unsupported socket option
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22882 )
Change subject: [socket] Fix test failure caused due to unsupported socket option
......................................................................
Patch Set 3:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/22882/3/src/kudu/master/mini_master.cc
File src/kudu/master/mini_master.cc:
http://gerrit.cloudera.org:8080/#/c/22882/3/src/kudu/master/mini_master.cc@61
PS3, Line 61: opts_.rpc_opts.rpc_reuseport = FLAGS_rpc_reuseport;
Why to change this at all? Given the signature of this constructor, I'd be surprised if 'rpc_bind_addr' were something else but INET address.
http://gerrit.cloudera.org:8080/#/c/22882/3/src/kudu/mini-cluster/mini_cluster.cc
File src/kudu/mini-cluster/mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/22882/3/src/kudu/mini-cluster/mini_cluster.cc@72
PS3, Line 72: // SO_REUSEPORT socket option is not applicable to Unix domain sockets.
By definition, MiniCluster::ReserveDaemonSocket() isn't supposed to be called for non-INET sockets. That's also clear from the call to GetBindIpForDaemonWithType() and from the signature of the method (it has the 'port' parameter). If that were done, it would be a programming error, and it would error out at least in sock_addr.ParseString() anyway.
With that, sock_addr.is_unix() cannot be 'true' at line 73, so there is no need to add any checks like this.
http://gerrit.cloudera.org:8080/#/c/22882/3/src/kudu/rpc/messenger.cc
File src/kudu/rpc/messenger.cc:
http://gerrit.cloudera.org:8080/#/c/22882/3/src/kudu/rpc/messenger.cc@263
PS3, Line 263: if (reuseport_ && accept_addr.is_unix()) {
: return Status::ConfigurationError("Port reuse is not applicable to Unix domain sockets.");
: } else if (reuseport_) {
: RETURN_NOT_OK(sock.SetReusePort(true));
: }
nit: from the readability perspective, please consider re-structuring this piece similar to the code below
if (reuseport_) {
if (PREDICT_FALSE(accept_addr.is_unix())) {
return Status::ConfigurationError(...);
}
RETURN_NOT_OK(sock.SetReusePort(true));
}
http://gerrit.cloudera.org:8080/#/c/22882/3/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/22882/3/src/kudu/server/server_base.cc@516
PS3, Line 516: static
nit: no need to add 'static' since this function is in the anonymous namespace already
http://gerrit.cloudera.org:8080/#/c/22882/3/src/kudu/server/server_base.cc@520
PS3, Line 520: Unix domain socket
nit: 'a Unix domain socket' or 'Unix domain sockets'
--
To view, visit http://gerrit.cloudera.org:8080/22882
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ice00b1c4fd1df78fa84c3dd2a79c968a4a91cc21
Gerrit-Change-Number: 22882
Gerrit-PatchSet: 3
Gerrit-Owner: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 27 May 2025 17:29:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] [socket] Fix test failure caused due to unsupported socket option
Posted by "Ashwani Raina (Code Review)" <ge...@cloudera.org>.
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/22882 )
Change subject: [socket] Fix test failure caused due to unsupported socket option
......................................................................
Patch Set 1:
> Patch Set 1:
>
> > > Patch Set 1:
> > >
> > > > Patch Set 1:
> > > >
> > > > (1 comment)
> > >
> > > The two tests are consistently failing on my local Ubuntu 20.04
> > machine for debug build (rebase 9e661cb34).
> > >
> > > It appears that in my case, for some reason, the socket shutdown
> > is failing consistently due to which subsequent operation of
> > setting SO_REUSEPORT option fails on unix domain socket. If socket
> > shutdown completes successfully, setting SO_REUSEPORT option may
> > not fail. I will need to validate this theory though, by running
> > some tests.
> > >
> > > Considering above explanation holds, we need to address two
> > issues:
> > > 1. Ensure proper socket cleanup is done successfully in order to
> > avoid reuseport failure.
> > > 2. Ensure unix domain sockets are not unknowingly used for
> > SO_REUSEPORT socket option and expected to work fine.
> >
> >
> > I ran a sample routine, that sets socket option(SO_REUSEPORT) on a
> > AF_UNIX type socket, on different linux kernel versions (5.15.0 and
> > 6.15-rc7) and it became clear that kernel versions containing
> > commit [1] are gracefully erroring out on non-inet sockets. The
> > change was introduced to fix a different issue however.
> >
> > [1]: https://github.com/torvalds/linux/commit/5b0af621c3f6ef9261cf6067812f2fd9943acb4b
> >
> > Socket cleanup issue (mentioned above) turns out to be something
> > different and could be related to C++ version installed on my
> > ubuntu linux variant. I haven't figured that out yet.
> >
> > I still believe, we need to make sure we are not unknowingly using
> > SO_REUSEPORT option while setting sockops on a AF_UNIX type socket.
> > Current change should help in doing that.
>
> OK, but why to change socket.cc?
>
> Also, maybe update the description with your findings, since the statement that SO_REUSEPORT for a Unix domain socket isn't supported isn't true.
SO_REUSEPORT macro has been available there since kernel 3.9 and it would be safe to assume that it is available in all the linux distros today, so there is no need to have this macro availability check, hence the change in socket.cc.
SO_REUSEPORT for a Unix domain socket is not supported is true because there is no proper handling of such a case in kernel. Also, there is no equivalent of sin_port in sockaddr_un structure. For the kernel versions (with [1] commit), setsockopt fails with appropriate error which is good because we will get to know the error before bind operation. However, if there is a wrong expectation of successfully setting reuseport option on unix domain socket, we can certainly correct that expectation with this check.
The check will also handle the cases where [1] is not present in the kernel, potentially leading to undefined behaviour.
--
To view, visit http://gerrit.cloudera.org:8080/22882
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ice00b1c4fd1df78fa84c3dd2a79c968a4a91cc21
Gerrit-Change-Number: 22882
Gerrit-PatchSet: 1
Gerrit-Owner: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 21 May 2025 09:14:21 +0000
Gerrit-HasComments: No
[kudu-CR] [socket] Fix test failure caused due to unsupported socket option
Posted by "Ashwani Raina (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22882
to look at the new patch set (#4).
Change subject: [socket] Fix test failure caused due to unsupported socket option
......................................................................
[socket] Fix test failure caused due to unsupported socket option
setsockopt() system call doesn't support SO_REUSEPORT for a Unix domain socket.
Due to this, following tests fail on certain linux versions (e.g. v6.15):
- client-test.TestConnectViaUnixSocket
- dns_alias-itest.TestBasic
Although there is no support available, a graceful error handling was
recently added as part of [1] commit. Linux kernel versions where [1]
is present, error out with appropriate message and test also fails
expectedly. However, for linux kernel versions (e.g. v5.15) where [1] is
not present, the tests may pass, but setting of SO_REUSEPORT socket option
may have no effect at all, potentially leading to confusion and undefined
behavior in tests.
The patch does the following to fix this:
1. Error out with appropriate error message if setting socket option
(i.e. SO_REUSEPORT) on a Unix domain socket.
2. Add a group flag validator to ensure either of two flags is set:
--rpc_listen_on_unix_domain_socket or --rpc_reuseport.
3. Explicitly initialise --rpc_reuseport flag in tests wherever
required as per test expectations.
[1] https://github.com/torvalds/linux/commit/5b0af621c3f6ef9261cf6067812f2fd9943acb4b
Change-Id: Ice00b1c4fd1df78fa84c3dd2a79c968a4a91cc21
---
M src/kudu/client/client-test.cc
M src/kudu/integration-tests/dns_alias-itest.cc
M src/kudu/master/master-test.cc
M src/kudu/master/mini_master-test.cc
M src/kudu/master/mini_master.cc
M src/kudu/master/sys_catalog-test.cc
M src/kudu/master/ts_state-test.cc
M src/kudu/rpc/messenger.cc
M src/kudu/server/server_base.cc
9 files changed, 43 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/82/22882/4
--
To view, visit http://gerrit.cloudera.org:8080/22882
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ice00b1c4fd1df78fa84c3dd2a79c968a4a91cc21
Gerrit-Change-Number: 22882
Gerrit-PatchSet: 4
Gerrit-Owner: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [socket] Fix test failure caused due to unsupported socket option
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22882 )
Change subject: [socket] Fix test failure caused due to unsupported socket option
......................................................................
Patch Set 1:
(2 comments)
> > Patch Set 1:
> >
> > > > Patch Set 1:
> > > >
> > > > > Patch Set 1:
> > > > >
> > > > > (1 comment)
> > > >
> > > > The two tests are consistently failing on my local Ubuntu
> 20.04
> > > machine for debug build (rebase 9e661cb34).
> > > >
> > > > It appears that in my case, for some reason, the socket
> shutdown
> > > is failing consistently due to which subsequent operation of
> > > setting SO_REUSEPORT option fails on unix domain socket. If
> socket
> > > shutdown completes successfully, setting SO_REUSEPORT option
> may
> > > not fail. I will need to validate this theory though, by
> running
> > > some tests.
> > > >
> > > > Considering above explanation holds, we need to address two
> > > issues:
> > > > 1. Ensure proper socket cleanup is done successfully in
> order to
> > > avoid reuseport failure.
> > > > 2. Ensure unix domain sockets are not unknowingly used for
> > > SO_REUSEPORT socket option and expected to work fine.
> > >
> > >
> > > I ran a sample routine, that sets socket option(SO_REUSEPORT)
> on a
> > > AF_UNIX type socket, on different linux kernel versions
> (5.15.0 and
> > > 6.15-rc7) and it became clear that kernel versions containing
> > > commit [1] are gracefully erroring out on non-inet sockets.
> The
> > > change was introduced to fix a different issue however.
> > >
> > > [1]: https://github.com/torvalds/linux/commit/5b0af621c3f6ef9261cf6067812f2fd9943acb4b
> > >
> > > Socket cleanup issue (mentioned above) turns out to be
> something
> > > different and could be related to C++ version installed on my
> > > ubuntu linux variant. I haven't figured that out yet.
> > >
> > > I still believe, we need to make sure we are not unknowingly
> using
> > > SO_REUSEPORT option while setting sockops on a AF_UNIX type
> socket.
> > > Current change should help in doing that.
> >
> > OK, but why to change socket.cc?
> >
> > Also, maybe update the description with your findings, since the
> statement that SO_REUSEPORT for a Unix domain socket isn't
> supported isn't true.
>
> SO_REUSEPORT macro has been available there since kernel 3.9 and it
> would be safe to assume that it is available in all the linux
> distros today, so there is no need to have this macro availability
> check, hence the change in socket.cc.
This doesn't explain why this change is needed to fix the tests that you reported as failing. As I understand, the change in socket.cc in PS1 is unrelated to changes needed to address the failing tests. What benefit does the change in socket.cc bring in? Did it fix or improve anything, even in a theoretical use case? I don't see much of that, but I might be missing something. However, I have one concern with the change in socket.cc: it might break compilation at some of the platforms listed as supported at https://kudu.apache.org/docs/installation.html.
Did you verify that Kudu source code still compiles with this update on all the OSes listed there? Even if you did or you know there is nothing to break, I'd rather see the update on socket.cc published in a separate changelist since it's not related to fixing the failing tests. If you feel strongly about this and prefer lumping it with the rest of the changes in this patch, then I'm fine with keeping the update on socket.cc here as well, but otherwise I encourage you to post the changes in socket.cc in a separate patch.
>
> SO_REUSEPORT for a Unix domain socket is not supported is true
> because there is no proper handling of such a case in kernel. Also,
> there is no equivalent of sin_port in sockaddr_un structure. For
> the kernel versions (with [1] commit), setsockopt fails with
> appropriate error which is good because we will get to know the
> error before bind operation. However, if there is a wrong
> expectation of successfully setting reuseport option on unix domain
> socket, we can certainly correct that expectation with this check.
> The check will also handle the cases where [1] is not present in
> the kernel, potentially leading to undefined behaviour.
The statement the commit description for PS1 is literally this:
> Socket API (SetSockOpt) doesn't support SO_REUSEPORT for a Unix domain socket.
SetSockOpt() is Kudu's API, so maybe that was the source of my confusion. As one can see, SetSockOpt() simply forwards everything down to the setsockopt() syscall, but there is nothing specific about Unix domain sockets regarding particular socket options in SetSockOpt() itself. It seems you rather meant setsockopt() syscall. Indeed -- the port number isn't applicable for Unix socket family, so SO_REUSEPORT isn't relevant for AF_UNIX sockets. Please consider updating the commit description accordingly.
Also, please add the kernel version information for the platform where you saw those two tests failing. Adding the relevant information regarding the change in setsockopt() behavior for SO_REUSEPORT in case of AF_UNIX sockets should be great as well.
Thank you!
http://gerrit.cloudera.org:8080/#/c/22882/1/src/kudu/rpc/messenger.cc
File src/kudu/rpc/messenger.cc:
http://gerrit.cloudera.org:8080/#/c/22882/1/src/kudu/rpc/messenger.cc@262
PS1, Line 262: unix
nit: UNIX
http://gerrit.cloudera.org:8080/#/c/22882/1/src/kudu/rpc/messenger.cc@263
PS1, Line 263: (reuseport_ && !accept_addr.is_unix())
Instead of silently ignoring the 'reuseport_' option for UNIX sockets, I'd rather think of reporting about the misconfiguration in this case, returning Status::ConfigurationError() when 'reuseport_' is set 'true' and the listening socket is of AF_UNIX family.
In addition, to catch such misconfiguration errors early, consider adding a group flag validator to check for the consistency of settings for --rpc_reuseport and --rpc_listen_on_unix_domain_socket flags.
--
To view, visit http://gerrit.cloudera.org:8080/22882
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ice00b1c4fd1df78fa84c3dd2a79c968a4a91cc21
Gerrit-Change-Number: 22882
Gerrit-PatchSet: 1
Gerrit-Owner: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 21 May 2025 22:30:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] [socket] Fix test failure caused due to unsupported socket option
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22882 )
Change subject: [socket] Fix test failure caused due to unsupported socket option
......................................................................
Patch Set 2:
Build Started http://jenkins.kudu.apache.org/job/pre_commit/1396/
--
To view, visit http://gerrit.cloudera.org:8080/22882
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ice00b1c4fd1df78fa84c3dd2a79c968a4a91cc21
Gerrit-Change-Number: 22882
Gerrit-PatchSet: 2
Gerrit-Owner: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 26 May 2025 08:05:56 +0000
Gerrit-HasComments: No
[kudu-CR] [socket] Fix test failure caused due to unsupported socket option
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22882 )
Change subject: [socket] Fix test failure caused due to unsupported socket option
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/22882/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/22882/1//COMMIT_MSG@9
PS1, Line 9: Socket API (SetSockOpt) doesn't support SO_REUSEPORT for a Unix
: domain socket. Due to this, following tests fail:
: - client-test.TestConnectViaUnixSocket
: - dns_alias-itest.TestBasic
Could you share more details on where did you see these failing?
I could not see these failing neither in pre-commit builds in gerrit, nor in my local macOS build, nor in local builds on Ubuntu 24.04 LTS.
--
To view, visit http://gerrit.cloudera.org:8080/22882
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ice00b1c4fd1df78fa84c3dd2a79c968a4a91cc21
Gerrit-Change-Number: 22882
Gerrit-PatchSet: 1
Gerrit-Owner: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 12 May 2025 18:08:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] [socket] Fix test failure caused due to unsupported socket option
Posted by "Ashwani Raina (Code Review)" <ge...@cloudera.org>.
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/22882 )
Change subject: [socket] Fix test failure caused due to unsupported socket option
......................................................................
Patch Set 1:
> Patch Set 1:
>
> > Patch Set 1:
> >
> > (1 comment)
>
> The two tests are consistently failing on my local Ubuntu 20.04 machine for debug build (rebase 9e661cb34).
>
> It appears that in my case, for some reason, the socket shutdown is failing consistently due to which subsequent operation of setting SO_REUSEPORT option fails on unix domain socket. If socket shutdown completes successfully, setting SO_REUSEPORT option may not fail. I will need to validate this theory though, by running some tests.
>
> Considering above explanation holds, we need to address two issues:
> 1. Ensure proper socket cleanup is done successfully in order to avoid reuseport failure.
> 2. Ensure unix domain sockets are not unknowingly used for SO_REUSEPORT socket option and expected to work fine.
I ran a sample routine, that sets socket option(SO_REUSEPORT) on a AF_UNIX type socket, on different linux kernel versions (5.15.0 and 6.15-rc7) and it became clear that kernel versions containing commit [1] are gracefully erroring out on non-inet sockets. The change was introduced to fix a different issue however.
[1]: https://github.com/torvalds/linux/commit/5b0af621c3f6ef9261cf6067812f2fd9943acb4b
Socket cleanup issue (mentioned above) turns out to be something different and could be related to C++ version installed on my ubuntu linux variant. I haven't figured that out yet.
I still believe, we need to make sure we are not unknowingly using SO_REUSEPORT option while setting sockops on a AF_UNIX type socket. Current change should help in doing that.
--
To view, visit http://gerrit.cloudera.org:8080/22882
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ice00b1c4fd1df78fa84c3dd2a79c968a4a91cc21
Gerrit-Change-Number: 22882
Gerrit-PatchSet: 1
Gerrit-Owner: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 20 May 2025 14:01:06 +0000
Gerrit-HasComments: No
[kudu-CR] [socket] Fix test failure caused due to unsupported socket option
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22882 )
Change subject: [socket] Fix test failure caused due to unsupported socket option
......................................................................
Patch Set 4:
Build Started http://jenkins.kudu.apache.org/job/pre_commit/1402/
--
To view, visit http://gerrit.cloudera.org:8080/22882
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ice00b1c4fd1df78fa84c3dd2a79c968a4a91cc21
Gerrit-Change-Number: 22882
Gerrit-PatchSet: 4
Gerrit-Owner: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 30 May 2025 07:39:02 +0000
Gerrit-HasComments: No
[kudu-CR] [socket] Fix test failure caused due to unsupported socket option
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22882 )
Change subject: [socket] Fix test failure caused due to unsupported socket option
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/22882/3/src/kudu/master/mini_master.cc
File src/kudu/master/mini_master.cc:
http://gerrit.cloudera.org:8080/#/c/22882/3/src/kudu/master/mini_master.cc@61
PS3, Line 61: opts_.rpc_opts.rpc_reuseport = FLAGS_rpc_reuseport;
> I don't think that explanation justifies for this change -- it simply doesn
If you are just looking for next steps in this context and want to introduce extra level of customization for this MiniMaster's option, please do so in its own changelist. There are multiple benefits for having logically separate updates in the source code repository.
In addition to that, if going this or similar route, I'd like to see a bit more holistic approach. There is MiniTabletServer and other components of the Kudu's test scaffolding that use ServerBaseOptions and derived data structures, where RpcServerOptions is an aggregated part. Throwing in such an update in this changelist only for MiniMaster looks a bit off-topic and not consistent enough.
I would highly appreciate if could you limit the amount of modifications in this patch, including the bare minimum to fulfill the summary of this patch (i.e. fixing test failure caused due to unsupported socket option):
* handle misconfigurations in address family and reuse port options for RPC sockets
* consequently, address the errors and failing tests in your environment once the former item is implemented
Thank you!
--
To view, visit http://gerrit.cloudera.org:8080/22882
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ice00b1c4fd1df78fa84c3dd2a79c968a4a91cc21
Gerrit-Change-Number: 22882
Gerrit-PatchSet: 3
Gerrit-Owner: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 28 May 2025 18:38:54 +0000
Gerrit-HasComments: Yes
[kudu-CR] [socket] Fix test failure caused due to unsupported socket option
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22882 )
Change subject: [socket] Fix test failure caused due to unsupported socket option
......................................................................
Patch Set 6: Verified+1
Carrying over Jenkin's +1 from PS5 (in PS6 the commit message was updated)
--
To view, visit http://gerrit.cloudera.org:8080/22882
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ice00b1c4fd1df78fa84c3dd2a79c968a4a91cc21
Gerrit-Change-Number: 22882
Gerrit-PatchSet: 6
Gerrit-Owner: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 30 May 2025 14:12:57 +0000
Gerrit-HasComments: No
[kudu-CR] [socket] Fix test failure caused due to unsupported socket option
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22882 )
Change subject: [socket] Fix test failure caused due to unsupported socket option
......................................................................
Patch Set 2: Verified-1
Build Failed
http://jenkins.kudu.apache.org/job/pre_commit/1396/ : FAILURE
--
To view, visit http://gerrit.cloudera.org:8080/22882
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ice00b1c4fd1df78fa84c3dd2a79c968a4a91cc21
Gerrit-Change-Number: 22882
Gerrit-PatchSet: 2
Gerrit-Owner: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 26 May 2025 10:06:12 +0000
Gerrit-HasComments: No
[kudu-CR] [socket] Fix test failure caused due to unsupported socket option
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22882 )
Change subject: [socket] Fix test failure caused due to unsupported socket option
......................................................................
Patch Set 1:
Build Started http://jenkins.kudu.apache.org/job/pre_commit/1363/
--
To view, visit http://gerrit.cloudera.org:8080/22882
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ice00b1c4fd1df78fa84c3dd2a79c968a4a91cc21
Gerrit-Change-Number: 22882
Gerrit-PatchSet: 1
Gerrit-Owner: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 12 May 2025 16:18:01 +0000
Gerrit-HasComments: No
[kudu-CR] [socket] Fix test failure caused due to unsupported socket option
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22882 )
Change subject: [socket] Fix test failure caused due to unsupported socket option
......................................................................
Patch Set 1:
> > Patch Set 1:
> >
> > > Patch Set 1:
> > >
> > > (1 comment)
> >
> > The two tests are consistently failing on my local Ubuntu 20.04
> machine for debug build (rebase 9e661cb34).
> >
> > It appears that in my case, for some reason, the socket shutdown
> is failing consistently due to which subsequent operation of
> setting SO_REUSEPORT option fails on unix domain socket. If socket
> shutdown completes successfully, setting SO_REUSEPORT option may
> not fail. I will need to validate this theory though, by running
> some tests.
> >
> > Considering above explanation holds, we need to address two
> issues:
> > 1. Ensure proper socket cleanup is done successfully in order to
> avoid reuseport failure.
> > 2. Ensure unix domain sockets are not unknowingly used for
> SO_REUSEPORT socket option and expected to work fine.
>
>
> I ran a sample routine, that sets socket option(SO_REUSEPORT) on a
> AF_UNIX type socket, on different linux kernel versions (5.15.0 and
> 6.15-rc7) and it became clear that kernel versions containing
> commit [1] are gracefully erroring out on non-inet sockets. The
> change was introduced to fix a different issue however.
>
> [1]: https://github.com/torvalds/linux/commit/5b0af621c3f6ef9261cf6067812f2fd9943acb4b
>
> Socket cleanup issue (mentioned above) turns out to be something
> different and could be related to C++ version installed on my
> ubuntu linux variant. I haven't figured that out yet.
>
> I still believe, we need to make sure we are not unknowingly using
> SO_REUSEPORT option while setting sockops on a AF_UNIX type socket.
> Current change should help in doing that.
OK, but why to change socket.cc?
Also, maybe update the description with your findings, since the statement that SO_REUSEPORT for a Unix domain socket isn't supported isn't true.
--
To view, visit http://gerrit.cloudera.org:8080/22882
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ice00b1c4fd1df78fa84c3dd2a79c968a4a91cc21
Gerrit-Change-Number: 22882
Gerrit-PatchSet: 1
Gerrit-Owner: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 20 May 2025 17:20:51 +0000
Gerrit-HasComments: No
[kudu-CR] [socket] Fix test failure caused due to unsupported socket option
Posted by "Ashwani Raina (Code Review)" <ge...@cloudera.org>.
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/22882 )
Change subject: [socket] Fix test failure caused due to unsupported socket option
......................................................................
Patch Set 3:
> Patch Set 1:
>
> (2 comments)
>
> > > Patch Set 1:
> > >
> > > > > Patch Set 1:
> > > > >
> > > > > > Patch Set 1:
> > > > > >
> > > > > > (1 comment)
> > > > >
> > > > > The two tests are consistently failing on my local Ubuntu
> > 20.04
> > > > machine for debug build (rebase 9e661cb34).
> > > > >
> > > > > It appears that in my case, for some reason, the socket
> > shutdown
> > > > is failing consistently due to which subsequent operation of
> > > > setting SO_REUSEPORT option fails on unix domain socket. If
> > socket
> > > > shutdown completes successfully, setting SO_REUSEPORT option
> > may
> > > > not fail. I will need to validate this theory though, by
> > running
> > > > some tests.
> > > > >
> > > > > Considering above explanation holds, we need to address two
> > > > issues:
> > > > > 1. Ensure proper socket cleanup is done successfully in
> > order to
> > > > avoid reuseport failure.
> > > > > 2. Ensure unix domain sockets are not unknowingly used for
> > > > SO_REUSEPORT socket option and expected to work fine.
> > > >
> > > >
> > > > I ran a sample routine, that sets socket option(SO_REUSEPORT)
> > on a
> > > > AF_UNIX type socket, on different linux kernel versions
> > (5.15.0 and
> > > > 6.15-rc7) and it became clear that kernel versions containing
> > > > commit [1] are gracefully erroring out on non-inet sockets.
> > The
> > > > change was introduced to fix a different issue however.
> > > >
> > > > [1]: https://github.com/torvalds/linux/commit/5b0af621c3f6ef9261cf6067812f2fd9943acb4b
> > > >
> > > > Socket cleanup issue (mentioned above) turns out to be
> > something
> > > > different and could be related to C++ version installed on my
> > > > ubuntu linux variant. I haven't figured that out yet.
> > > >
> > > > I still believe, we need to make sure we are not unknowingly
> > using
> > > > SO_REUSEPORT option while setting sockops on a AF_UNIX type
> > socket.
> > > > Current change should help in doing that.
> > >
> > > OK, but why to change socket.cc?
> > >
> > > Also, maybe update the description with your findings, since the
> > statement that SO_REUSEPORT for a Unix domain socket isn't
> > supported isn't true.
> >
> > SO_REUSEPORT macro has been available there since kernel 3.9 and it
> > would be safe to assume that it is available in all the linux
> > distros today, so there is no need to have this macro availability
> > check, hence the change in socket.cc.
>
> This doesn't explain why this change is needed to fix the tests that you reported as failing. As I understand, the change in socket.cc in PS1 is unrelated to changes needed to address the failing tests. What benefit does the change in socket.cc bring in? Did it fix or improve anything, even in a theoretical use case? I don't see much of that, but I might be missing something. However, I have one concern with the change in socket.cc: it might break compilation at some of the platforms listed as supported at https://kudu.apache.org/docs/installation.html.
>
> Did you verify that Kudu source code still compiles with this update on all the OSes listed there? Even if you did or you know there is nothing to break, I'd rather see the update on socket.cc published in a separate changelist since it's not related to fixing the failing tests. If you feel strongly about this and prefer lumping it with the rest of the changes in this patch, then I'm fine with keeping the update on socket.cc here as well, but otherwise I encourage you to post the changes in socket.cc in a separate patch.
>
> >
> > SO_REUSEPORT for a Unix domain socket is not supported is true
> > because there is no proper handling of such a case in kernel. Also,
> > there is no equivalent of sin_port in sockaddr_un structure. For
> > the kernel versions (with [1] commit), setsockopt fails with
> > appropriate error which is good because we will get to know the
> > error before bind operation. However, if there is a wrong
> > expectation of successfully setting reuseport option on unix domain
> > socket, we can certainly correct that expectation with this check.
> > The check will also handle the cases where [1] is not present in
> > the kernel, potentially leading to undefined behaviour.
>
> The statement the commit description for PS1 is literally this:
> > Socket API (SetSockOpt) doesn't support SO_REUSEPORT for a Unix domain socket.
>
> SetSockOpt() is Kudu's API, so maybe that was the source of my confusion. As one can see, SetSockOpt() simply forwards everything down to the setsockopt() syscall, but there is nothing specific about Unix domain sockets regarding particular socket options in SetSockOpt() itself. It seems you rather meant setsockopt() syscall. Indeed -- the port number isn't applicable for Unix socket family, so SO_REUSEPORT isn't relevant for AF_UNIX sockets. Please consider updating the commit description accordingly.
>
> Also, please add the kernel version information for the platform where you saw those two tests failing. Adding the relevant information regarding the change in setsockopt() behavior for SO_REUSEPORT in case of AF_UNIX sockets should be great as well.
>
> Thank you!
Forgot to mention before, changes in socket.cc are in separate changelist now that is out for review: https://gerrit.cloudera.org/c/22945/
--
To view, visit http://gerrit.cloudera.org:8080/22882
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ice00b1c4fd1df78fa84c3dd2a79c968a4a91cc21
Gerrit-Change-Number: 22882
Gerrit-PatchSet: 3
Gerrit-Owner: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 27 May 2025 12:41:19 +0000
Gerrit-HasComments: No
[kudu-CR] [socket] Fix test failure caused due to unsupported socket option
Posted by "Ashwani Raina (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22882
to look at the new patch set (#3).
Change subject: [socket] Fix test failure caused due to unsupported socket option
......................................................................
[socket] Fix test failure caused due to unsupported socket option
setsockopt() system call doesn't support SO_REUSEPORT for a Unix domain socket.
Due to this, following tests fail on certain linux versions (e.g. v6.15):
- client-test.TestConnectViaUnixSocket
- dns_alias-itest.TestBasic
Although there is no support available, a graceful error handling was
recently added as part of [1] commit. Linux kernel versions where [1]
is present, error out with appropriate message and test also fails
expectedly. However, for linux kernel versions (e.g. v5.15) where [1] is
not present, the tests may pass, but setting of SO_REUSEPORT socket option
may have no effect at all, potentially leading to confusion and undefined
behavior in tests.
The patch does the following to fix this:
1. Error out with appropriate error message if setting socket option
(i.e. SO_REUSEPORT) on a Unix domain socket.
2. Add a group flag validator to ensure either of two flags is set:
--rpc_listen_on_unix_domain_socket or --rpc_reuseport.
3. Explicitly initialise --rpc_reuseport flag in tests wherever
required as per test expectations.
[1] https://github.com/torvalds/linux/commit/5b0af621c3f6ef9261cf6067812f2fd9943acb4b
Change-Id: Ice00b1c4fd1df78fa84c3dd2a79c968a4a91cc21
---
M src/kudu/client/client-test.cc
M src/kudu/integration-tests/dns_alias-itest.cc
M src/kudu/master/master-test.cc
M src/kudu/master/mini_master-test.cc
M src/kudu/master/mini_master.cc
M src/kudu/master/sys_catalog-test.cc
M src/kudu/master/ts_state-test.cc
M src/kudu/mini-cluster/mini_cluster.cc
M src/kudu/rpc/messenger.cc
M src/kudu/server/server_base.cc
10 files changed, 65 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/82/22882/3
--
To view, visit http://gerrit.cloudera.org:8080/22882
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ice00b1c4fd1df78fa84c3dd2a79c968a4a91cc21
Gerrit-Change-Number: 22882
Gerrit-PatchSet: 3
Gerrit-Owner: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [socket] Fix test failure caused due to unsupported socket option
Posted by "Ashwani Raina (Code Review)" <ge...@cloudera.org>.
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/22882 )
Change subject: [socket] Fix test failure caused due to unsupported socket option
......................................................................
Patch Set 3:
(1 comment)
> Patch Set 3:
>
> (1 comment)
http://gerrit.cloudera.org:8080/#/c/22882/3/src/kudu/master/mini_master.cc
File src/kudu/master/mini_master.cc:
http://gerrit.cloudera.org:8080/#/c/22882/3/src/kudu/master/mini_master.cc@61
PS3, Line 61: opts_.rpc_opts.rpc_reuseport = FLAGS_rpc_reuseport;
> If you are just looking for next steps in this context and want to introduc
There is one more reason that flag makes sense. This setting is applied per messenger object which remains same for the rpc server object. So when a new master get initialized, the setting will part of its corresponding messenger.
Following check inside Messenger::AddAcceptorPool won't make sense because we will return config error from here and test will fail because it knowingly set the port reuse to true (because of "opts_.rpc_opts.rpc_reuseport = true" statement here) for a Unix domain socket:
if (reuseport_) {
if (PREDICT_FALSE(accept_addr.is_unix())) {
return Status::ConfigurationError(...);
}
RETURN_NOT_OK(sock.SetReusePort(true));
}
That raises questions on why MiniMaster object is being used for Unix domain socket. Maybe client test that work on Unix domain socket should not be using InternalMiniCluster because that uses MiniMaster internally and it has port reuse always set to true. Also, there is just one test (TestConnectViaUnixSocket which is also failing) inside client-test.cc that is making use of rpc server created from MiniMaster object and expects it to work for Unix domain sockets.
I am not sure but the commit message from the patch that introduced this test might provide some background on why the class was used:
++++
commit be68ce81beeb708edfc0545695e72282506f3845
Author: Todd Lipcon <to...@apache.org>
Date: Thu Apr 9 16:42:44 2020 -0700
client/tserver: add support for connecting over unix domain sockets
This adds new experiental flags -rpc_listen_on_unix_domain_socket and
-client_use_unix_domain_sockets. The former makes the RPC server bind to
a unix socket and advertise this to the kudu master as part of the TS
registration. The latter makes the client attempt to connect via a
domain socket when it sees such a socket path advertised.
Note that this makes one behavioral change even when those flags are not
enabled: we now consider any tablet server with a loopback IP to be
"local" (and thus a candidate for unix domain socket connection). This
mostly affects the MiniCluster where tablet servers register using
various IPs in the loopback range 127.0.0.0/8, and was necessary in
order to test unix socket connections from the client.
++++
Maybe, as a limited and targeted fix for test specific issue, don't set reuseport for messenger builder during server init so that while adding new acceptor pool, port reuse socket option is not set on Unix domain socket, thereby, proceeding with successful bind call. Something like this:
src/kudu/server/server_base.cc
- if (options_.rpc_opts.rpc_reuseport) {
--
+ if (!FLAGS_rpc_listen_on_unix_domain_socket && options_.rpc_opts.rpc_reuseport) {
+ builder.set_reuseport();
--
To view, visit http://gerrit.cloudera.org:8080/22882
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ice00b1c4fd1df78fa84c3dd2a79c968a4a91cc21
Gerrit-Change-Number: 22882
Gerrit-PatchSet: 3
Gerrit-Owner: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 29 May 2025 16:00:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] [socket] Fix test failure caused due to unsupported socket option
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22882 )
Change subject: [socket] Fix test failure caused due to unsupported socket option
......................................................................
Patch Set 5:
Build Started http://jenkins.kudu.apache.org/job/pre_commit/1403/
--
To view, visit http://gerrit.cloudera.org:8080/22882
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ice00b1c4fd1df78fa84c3dd2a79c968a4a91cc21
Gerrit-Change-Number: 22882
Gerrit-PatchSet: 5
Gerrit-Owner: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 30 May 2025 07:58:32 +0000
Gerrit-HasComments: No
[kudu-CR] [socket] Fix test failure caused due to unsupported socket option
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22882 )
Change subject: [socket] Fix test failure caused due to unsupported socket option
......................................................................
Patch Set 5: Verified+1
Build Successful
http://jenkins.kudu.apache.org/job/pre_commit/1403/ : SUCCESS
--
To view, visit http://gerrit.cloudera.org:8080/22882
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ice00b1c4fd1df78fa84c3dd2a79c968a4a91cc21
Gerrit-Change-Number: 22882
Gerrit-PatchSet: 5
Gerrit-Owner: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 30 May 2025 08:52:44 +0000
Gerrit-HasComments: No