Piotr Kliczewski has posted comments on this change. Change subject: xmlrpc: detecting loop on centos7 ......................................................................
Patch Set 5: (4 comments) https://gerrit.ovirt.org/#/c/39343/5//COMMIT_MSG Commit Message: Line 20: event which finalizes processing. Line 21: Line 22: During investigation we found out that there is fd leak which was caused Line 23: by not closing plain socket wrapped by ssl socket. Calling shutdown and Line 24: close on ssl do not closes plain socket. > This is not related, and may be wrong change. Lets separate the changes. Done Line 25: Line 26: Line 27: Change-Id: I238d10b3bd8aaf8baac55ec81a7d406609e544e6 https://gerrit.ovirt.org/#/c/39343/5/lib/vdsm/sslutils.py File lib/vdsm/sslutils.py: Line 54: self.connection.shutdown(socket.SHUT_RDWR) Line 55: self.connection.close() Line 56: # if we do not close socket we get ssl protocol shutdown Line 57: # but fd is not released Line 58: self.connection.socket.close() > nice that you found the leak! Will split the changes into 2 patches. I spend some time narrowing the issue and found this innocent change stopped the leak. I noticed that there is different version of m2c on f20 and centos7 so it could be a regression there. Line 59: Line 60: def fileno(self): Line 61: return self.connection.fileno() Line 62: Line 54: self.connection.shutdown(socket.SHUT_RDWR) Line 55: self.connection.close() Line 56: # if we do not close socket we get ssl protocol shutdown Line 57: # but fd is not released Line 58: self.connection.socket.close() > I agree with Francesco, and I'm not sure that this is correct - the socket Ido found fd leak when he tested functional networkTests on centos7. We leak one fd per connection and this test connects 962 times so we quite easily reach default limit. According to my testing on centos7 when I call shutdown and close I am not able to send/receive but the fd is not released. When I added this line the fds are released properly. I could be wrong so it would be good for you to verify it on centos7. It is very easy to reproduce with latest master. Line 59: Line 60: def fileno(self): Line 61: return self.connection.fileno() Line 62: https://gerrit.ovirt.org/#/c/39343/5/vdsm/rpc/BindingXMLRPC.py File vdsm/rpc/BindingXMLRPC.py: Line 1197: def handle_dispatcher(self, dispatcher, socket_address): Line 1198: client_socket = dispatcher.socket Line 1199: dispatcher.del_channel() Line 1200: # skip further processing of handle_read Line 1201: dispatcher.connected = False > Looks nice - but why this is not part of del_channel()? Because del_channel is run for jsonrpc as well and we want to preserve the behavior for it. Line 1202: self.xml_binding.add_socket(client_socket, socket_address) -- To view, visit https://gerrit.ovirt.org/39343 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I238d10b3bd8aaf8baac55ec81a7d406609e544e6 Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Ido Barkan <ibar...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
