Change in vdsm[master]: tests: reduce noice after running detector test

nsoffer at redhat.com nsoffer at redhat.com
Thu Apr 9 14:54:26 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: tests: reduce noice after running detector test
......................................................................


Patch Set 6:

(3 comments)

Ok, the root cause looks like be a detector thread that is stuck on the socket after the test is finished.

The correct solution would be to stop the thread when the test ends.

The change closing server side detectors on tearDown is correct, but we do not need the dispatcher.

I think we should keep the socket in the detector in handle_dispatcher(), and close it in close()

When we close the socket, we can safely ignore socket.error.

Please do not add dependencies that we don't need, such as the dispatcher object. We don't use it in the application and we should not use it in the tests.

https://gerrit.ovirt.org/#/c/39031/6/lib/yajsonrpc/betterAsyncore.py
File lib/yajsonrpc/betterAsyncore.py:

Line 18: # while enabling compositing instead of inheritance.
Line 19: import asyncore
Line 20: import types
Line 21: from errno import EWOULDBLOCK
Line 22: from socket import error
> As part of clean up when interpreter is shutdown we get:
You are not fixing the root cause. We should not have thread stuck in recv during shutdown.

If we use your reasoning, we should never import modules, only names from modules, so during shutdown we will not get attribute errors.
Line 23: 
Line 24: from vdsm.infra.eventfd import EventFD
Line 25: 
Line 26: 


https://gerrit.ovirt.org/#/c/39031/6/tests/protocoldetectorTests.py
File tests/protocoldetectorTests.py:

Line 61:                 try:
Line 62:                     request += dispatcher.recv(1024)
Line 63:                 except error:
Line 64:                     # for some test we never receive data
Line 65:                     pass
> This one is only when we close socket but we still listen. It is expected s
We should not listen - the listener socket should be closed by protocol detector - you already have patch for closing it.
Line 66: 
Line 67:             response = self.response(request)
Line 68:             client_socket.setblocking(1)
Line 69:             client_socket.sendall(response)


Line 232:                     s = self.SSLCTX.wrapSocket(s)
Line 233:                 else:
Line 234:                     s = ssl.wrap_socket(s, KEY_FILE, CRT_FILE,
Line 235:                                         ca_certs=CRT_FILE,
Line 236:                                         server_side=False)
> I am confused by:
I don't see how my other comment is related.

The current code, using ssl.wrap_socket is correct, not sharing context with the server, and I don't see how it is related to the attribute errors on shutdown.
Line 237:             s.connect(self.acceptor_address)
Line 238:             yield s
Line 239:         finally:
Line 240:             s.close()


-- 
To view, visit https://gerrit.ovirt.org/39031
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I23302c7f742f31f0ab5e12265da3bb7f57a81d80
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Dima Kuznetsov <dkuznets at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list