You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2019/09/13 07:53:42 UTC
[kudu-CR] [thirdparty] add SO REUSEPORT for chronyd NTP socket
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14228
Change subject: [thirdparty] add SO_REUSEPORT for chronyd NTP socket
......................................................................
[thirdparty] add SO_REUSEPORT for chronyd NTP socket
This patch adds SO_REUSEPORT option for NTP server socket opened
by chronyd. This is to allow for using the port reservation
technique from external minicluster when starting chronyd test
NTP servers.
Change-Id: Iee26fcf93976dd7affe77254751016bcbf398620
---
M thirdparty/download-thirdparty.sh
A thirdparty/patches/chrony-reuseport.patch
2 files changed, 36 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/14228/1
--
To view, visit http://gerrit.cloudera.org:8080/14228
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iee26fcf93976dd7affe77254751016bcbf398620
Gerrit-Change-Number: 14228
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [thirdparty] add SO REUSEPORT for chronyd NTP socket
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/14228 )
Change subject: [thirdparty] add SO_REUSEPORT for chronyd NTP socket
......................................................................
Patch Set 1: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/14228
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee26fcf93976dd7affe77254751016bcbf398620
Gerrit-Change-Number: 14228
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sat, 14 Sep 2019 00:24:07 +0000
Gerrit-HasComments: No
[kudu-CR] [thirdparty] add SO REUSEPORT for chronyd NTP socket
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14228 )
Change subject: [thirdparty] add SO_REUSEPORT for chronyd NTP socket
......................................................................
Patch Set 1:
> Could we get by with a patch to enable ephemeral port binding
> instead? That's less invasive than this, which seems unlikely to be
> accepted upstream.
Why do you think the ephemeral port binding change for chrony is less invasive than adding SO_REUSEPORT (i.e. this small patch)? Do you have some particular considerations w.r.t. SO_REUSEPORT change?
From what I see in chrony's source code, changing its code to properly support (and that's is needed if we talking about accepting the patch upstream by chrony) binding to ephemeral port would change many places there: interpretation of configuration directive 'port' in configuraiton file, multiple places in code that interpret port '0' as a special value meaning "don't open NTP port at all", etc. By my understanding, that would be much more invasive than this small patch.
What do you think?
--
To view, visit http://gerrit.cloudera.org:8080/14228
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee26fcf93976dd7affe77254751016bcbf398620
Gerrit-Change-Number: 14228
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sat, 14 Sep 2019 07:12:11 +0000
Gerrit-HasComments: No
[kudu-CR] [thirdparty] add SO REUSEPORT for chronyd NTP socket
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14228 )
Change subject: [thirdparty] add SO_REUSEPORT for chronyd NTP socket
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/14228
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee26fcf93976dd7affe77254751016bcbf398620
Gerrit-Change-Number: 14228
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 16 Sep 2019 23:29:21 +0000
Gerrit-HasComments: No
[kudu-CR] [thirdparty] add SO REUSEPORT for chronyd NTP socket
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14228 )
Change subject: [thirdparty] add SO_REUSEPORT for chronyd NTP socket
......................................................................
Patch Set 1:
> > > Could we get by with a patch to enable ephemeral port binding
> > > instead? That's less invasive than this, which seems unlikely
> to be
> > > accepted upstream.
> >
> > Why do you think the ephemeral port binding change for chrony is
> less invasive than adding SO_REUSEPORT (i.e. this small patch)? Do
> you have some particular considerations w.r.t. SO_REUSEPORT change?
> >
> > From what I see in chrony's source code, changing its code to
> properly support (and that's is needed if we talking about
> accepting the patch upstream by chrony) binding to ephemeral port
> would change many places there: interpretation of configuration
> directive 'port' in configuraiton file, multiple places in code
> that interpret port '0' as a special value meaning "don't open NTP
> port at all", etc. By my understanding, that would be much more
> invasive than this small patch.
> >
> > What do you think?
>
> I'll copy in what I wrote in Slack.
>
> When I wrote "invasive" I didn't mean that it'd require more or
> less code change to chronyd; I was thinking about it being more
> difficult to reason about chronyd and the other sockets bound on
> the system. As I understand it, the purpose of SO_REUSEPORT is for
> multi-threaded applications that wish to more evenly accept new
> connections (or new datagrams, for UDP sockets). It's not for "port
> reservation to enable testing" the way you're using it. If an
> application binds with SO_REUSEPORT, another application running
> with the same EUID can now accidentally bind to that address while
> it's in use (if SO_REUSEPORT is fed to the second bind call).
> Contrast this with ephemeral port usage, which is an age old
> sockets concept and has "fire and forget" semantics: you bind to
> port 0, you see what port you got, and at that point the semantics
> are just like non-ephemeral port usage.
>
> In general, I think we should seek to minimize the number of third
> party patches we carry, especially of the "upstream will never
> accept this" variety. My concern is that SO_REUSEPORT in chronyd
> won't be accepted because it's a little bit weird, and I think
> that's less of an issue for ephemeral port usage.
>
> All that said, if it's going to be much more work to support
> ephemeral ports, or if you think it's also nonsensical for reasons
> I'm not seeing, then we can move forward with this instead.
> > > Could we get by with a patch to enable ephemeral port binding
> > > instead? That's less invasive than this, which seems unlikely
> to be
> > > accepted upstream.
> >
> > Why do you think the ephemeral port binding change for chrony is
> less invasive than adding SO_REUSEPORT (i.e. this small patch)? Do
> you have some particular considerations w.r.t. SO_REUSEPORT change?
> >
> > From what I see in chrony's source code, changing its code to
> properly support (and that's is needed if we talking about
> accepting the patch upstream by chrony) binding to ephemeral port
> would change many places there: interpretation of configuration
> directive 'port' in configuraiton file, multiple places in code
> that interpret port '0' as a special value meaning "don't open NTP
> port at all", etc. By my understanding, that would be much more
> invasive than this small patch.
> >
> > What do you think?
>
> I'll copy in what I wrote in Slack.
>
> When I wrote "invasive" I didn't mean that it'd require more or
> less code change to chronyd; I was thinking about it being more
> difficult to reason about chronyd and the other sockets bound on
> the system. As I understand it, the purpose of SO_REUSEPORT is for
> multi-threaded applications that wish to more evenly accept new
> connections (or new datagrams, for UDP sockets). It's not for "port
> reservation to enable testing" the way you're using it. If an
> application binds with SO_REUSEPORT, another application running
> with the same EUID can now accidentally bind to that address while
> it's in use (if SO_REUSEPORT is fed to the second bind call).
> Contrast this with ephemeral port usage, which is an age old
> sockets concept and has "fire and forget" semantics: you bind to
> port 0, you see what port you got, and at that point the semantics
> are just like non-ephemeral port usage.
>
> In general, I think we should seek to minimize the number of third
> party patches we carry, especially of the "upstream will never
> accept this" variety. My concern is that SO_REUSEPORT in chronyd
> won't be accepted because it's a little bit weird, and I think
> that's less of an issue for ephemeral port usage.
>
> All that said, if it's going to be much more work to support
> ephemeral ports, or if you think it's also nonsensical for reasons
> I'm not seeing, then we can move forward with this instead.
Thank you for the clarification. Yep, I know what SO_REUSEPORT is usually targeted for, but from the number of changes in the chrony code it seemed the lesser evil compared with ephemeral port binding, especially given the fact that port number 0 now means 'don't open port at all' in chronyd's code.
BTW, using ephemeral port number for chronyd NTP port doesn't guarantee the acceptance of the patch neither, but I agree it looks less strange than SO_REUSEPORT :)
I think we can go with SO_REUSEPORT first, and in the background I'll try to poke around to see whether the maintainers of chronyd upstream would be willing to accept the ephemeral port binding change. If so, then we could invest more time into changing the approach, switching from SO_REUSEPORT to the ephemeral port binding.
--
To view, visit http://gerrit.cloudera.org:8080/14228
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee26fcf93976dd7affe77254751016bcbf398620
Gerrit-Change-Number: 14228
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 16 Sep 2019 23:27:19 +0000
Gerrit-HasComments: No
[kudu-CR] [thirdparty] add SO REUSEPORT for chronyd NTP socket
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: [thirdparty] add SO_REUSEPORT for chronyd NTP socket
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/14228
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Iee26fcf93976dd7affe77254751016bcbf398620
Gerrit-Change-Number: 14228
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [thirdparty] add SO REUSEPORT for chronyd NTP socket
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14228 )
Change subject: [thirdparty] add SO_REUSEPORT for chronyd NTP socket
......................................................................
Patch Set 1: Verified+1
Unrelated test failures in:
* MasterSentryTest.TestTableOwnership
* HmsSentryConfigurations/MasterStressTest.Test/2
--
To view, visit http://gerrit.cloudera.org:8080/14228
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee26fcf93976dd7affe77254751016bcbf398620
Gerrit-Change-Number: 14228
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 13 Sep 2019 15:27:11 +0000
Gerrit-HasComments: No
[kudu-CR] [thirdparty] add SO REUSEPORT for chronyd NTP socket
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14228 )
Change subject: [thirdparty] add SO_REUSEPORT for chronyd NTP socket
......................................................................
[thirdparty] add SO_REUSEPORT for chronyd NTP socket
This patch adds SO_REUSEPORT option for NTP server socket opened
by chronyd. This is to allow for using the port reservation
technique from external minicluster when starting chronyd test
NTP servers.
Change-Id: Iee26fcf93976dd7affe77254751016bcbf398620
Reviewed-on: http://gerrit.cloudera.org:8080/14228
Tested-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Greg Solovyev <gs...@cloudera.com>
Reviewed-by: Hao Hao <ha...@cloudera.com>
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M thirdparty/download-thirdparty.sh
A thirdparty/patches/chrony-reuseport.patch
2 files changed, 36 insertions(+), 2 deletions(-)
Approvals:
Alexey Serbin: Verified
Greg Solovyev: Looks good to me, but someone else must approve
Hao Hao: Looks good to me, but someone else must approve
Adar Dembo: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/14228
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Iee26fcf93976dd7affe77254751016bcbf398620
Gerrit-Change-Number: 14228
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [thirdparty] add SO REUSEPORT for chronyd NTP socket
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14228 )
Change subject: [thirdparty] add SO_REUSEPORT for chronyd NTP socket
......................................................................
Patch Set 1:
> > Could we get by with a patch to enable ephemeral port binding
> > instead? That's less invasive than this, which seems unlikely to be
> > accepted upstream.
>
> Why do you think the ephemeral port binding change for chrony is less invasive than adding SO_REUSEPORT (i.e. this small patch)? Do you have some particular considerations w.r.t. SO_REUSEPORT change?
>
> From what I see in chrony's source code, changing its code to properly support (and that's is needed if we talking about accepting the patch upstream by chrony) binding to ephemeral port would change many places there: interpretation of configuration directive 'port' in configuraiton file, multiple places in code that interpret port '0' as a special value meaning "don't open NTP port at all", etc. By my understanding, that would be much more invasive than this small patch.
>
> What do you think?
I'll copy in what I wrote in Slack.
When I wrote "invasive" I didn't mean that it'd require more or less code change to chronyd; I was thinking about it being more difficult to reason about chronyd and the other sockets bound on the system. As I understand it, the purpose of SO_REUSEPORT is for multi-threaded applications that wish to more evenly accept new connections (or new datagrams, for UDP sockets). It's not for "port reservation to enable testing" the way you're using it. If an application binds with SO_REUSEPORT, another application running with the same EUID can now accidentally bind to that address while it's in use (if SO_REUSEPORT is fed to the second bind call). Contrast this with ephemeral port usage, which is an age old sockets concept and has "fire and forget" semantics: you bind to port 0, you see what port you got, and at that point the semantics are just like non-ephemeral port usage.
In general, I think we should seek to minimize the number of third party patches we carry, especially of the "upstream will never accept this" variety. My concern is that SO_REUSEPORT in chronyd won't be accepted because it's a little bit weird, and I think that's less of an issue for ephemeral port usage.
All that said, if it's going to be much more work to support ephemeral ports, or if you think it's also nonsensical for reasons I'm not seeing, then we can move forward with this instead.
--
To view, visit http://gerrit.cloudera.org:8080/14228
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee26fcf93976dd7affe77254751016bcbf398620
Gerrit-Change-Number: 14228
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 16 Sep 2019 22:50:16 +0000
Gerrit-HasComments: No
[kudu-CR] [thirdparty] add SO REUSEPORT for chronyd NTP socket
Posted by "Greg Solovyev (Code Review)" <ge...@cloudera.org>.
Greg Solovyev has posted comments on this change. ( http://gerrit.cloudera.org:8080/14228 )
Change subject: [thirdparty] add SO_REUSEPORT for chronyd NTP socket
......................................................................
Patch Set 1: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/14228
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee26fcf93976dd7affe77254751016bcbf398620
Gerrit-Change-Number: 14228
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 13 Sep 2019 16:26:01 +0000
Gerrit-HasComments: No
[kudu-CR] [thirdparty] add SO REUSEPORT for chronyd NTP socket
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14228 )
Change subject: [thirdparty] add SO_REUSEPORT for chronyd NTP socket
......................................................................
Patch Set 1:
Could we get by with a patch to enable ephemeral port binding instead? That's less invasive than this, which seems unlikely to be accepted upstream.
--
To view, visit http://gerrit.cloudera.org:8080/14228
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee26fcf93976dd7affe77254751016bcbf398620
Gerrit-Change-Number: 14228
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sat, 14 Sep 2019 00:25:14 +0000
Gerrit-HasComments: No