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 <aser...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Greg Solovyev <gsolov...@cloudera.com> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 16 Sep 2019 23:27:19 +0000 Gerrit-HasComments: No
