ipa-replica-conncheck: do not close listening ports until required #267
Conversation
| family = socket.AF_INET6 | ||
|
|
||
| host = '::' # all available interfaces | ||
| proto = 'tcp' if socket_type == socket.SOCK_STREAM else 'udp' |
There was a problem hiding this comment.
What about dict[proto]? It will catch error cases where protocol is neither tcp nor udp for free...
| class PortResponder(threading.Thread): | ||
|
|
||
| def __init__(self, port, port_type, socket_timeout=1): | ||
| def __init__(self, ports): |
There was a problem hiding this comment.
I would appreciate doc string saying what ports is expected to contain. Thanks!
| # Make sure IPv4 clients can connect to IPv6 socket | ||
| sock.setsockopt(socket.IPPROTO_IPV6, socket.IPV6_V6ONLY, 0) | ||
|
|
||
| if socket_type == socket.SOCK_STREAM: |
There was a problem hiding this comment.
The code around is using either if socket_type or if proto. It would be good to use the same condition everywhere so it is easier to read.
| @@ -548,6 +597,7 @@ def main(): | |||
| time.sleep(3600) | |||
| print_info("Connection check timeout: terminating listening program") | |||
|
|
|||
|
|
|||
| if __name__ == "__main__": | |||
| try: | |||
| sys.exit(main()) | |||
There was a problem hiding this comment.
LOL, please fix this as well.
tkrizek
force-pushed
the
t6487
branch
2 times, most recently
from
964243f
to
cc9d298
Compare
| else: | ||
| self._sockets.append(sock) | ||
|
|
||
| def _respond(self, sock): |
There was a problem hiding this comment.
Nitpick: Feel free to ignore this.
In general it would be better to split _respond into _respond_tcp and _respond_udp. The two ifs have basically nothing in common and it would remove one more nested if and could share logging (if logging is done in the calling function).
| def main(): | ||
| global RESPONDER |
There was a problem hiding this comment.
Nitpick: Feel free to ignore this.
Yuck. It would be better to split the huge main into smaller functions and pass RESPONDER between those. Anyway, this is mainly relict of the old code so I do not insist on getting rid of it.
| responder = PortResponder(port.port, port.port_type) | ||
| responder.start() | ||
| RESPONDERS.append(responder) | ||
| RESPONDER = PortResponder(required_ports) |
There was a problem hiding this comment.
The design should have Implementation section mentioning why it is done in this particular way (i.e. explain why we have one thread vs. other options).
| except KeyboardInterrupt: | ||
| print_info("\nCleaning up...") | ||
| sys.exit(1) | ||
| except RuntimeError as e: | ||
| sys.exit(e) |
There was a problem hiding this comment.
Sorry, my bad, except RuntimeError must stay there. The program is using RuntimeError to pass error messages to the user.
|
|
||
| sock.bind((host, port)) | ||
| if socket_type == socket.SOCK_STREAM: | ||
| sock.listen(1) |
There was a problem hiding this comment.
Please add rationale for using value 1 to a comment/commit message/design.
tkrizek
force-pushed
the
t6487
branch
2 times, most recently
from
93febb4
to
cbb58aa
Compare
|
needs rebase |
Previously, a separate thread would be created for each socket used for conncheck. It would also time out after one second, after which it would be closed and reopened again. This caused random failures of conncheck. Now all sockets are handled in a single thread and once the server starts to listen on a port, it does not close that connection until the script finishes. Only IPv6 socket is used for simplicity, since it can handle both IPv6 and IPv4 connections. This requires IPv6 kernel support, which is required by other parts of IPA anyway. https://fedorahosted.org/freeipa/ticket/6487
|
Fixed upstream |
Previously, a separate thread would be created for each socket used
for conncheck. It would also time out after one second, after which it
would be closed and reopened again. This caused random failures of
conncheck.
Now all sockets are handled in a single thread and once the server
starts to listen on a port, it does not close that connection until the
script finishes.
Only IPv6 socket is used for simplicity, since it can handle both IPv6
and IPv4 connections. This requires IPv6 kernel support, which is
required by other parts of IPA anyway.
https://fedorahosted.org/freeipa/ticket/6487