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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Greg Solovyev <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 16 Sep 2019 22:50:16 +0000 Gerrit-HasComments: No
