Change in vdsm[master]: libvirtconnection: ping libvirt upon disconnection

Saggi Mizrahi has posted comments on this change.

Change subject: libvirtconnection: ping libvirt upon disconnection
......................................................................

Patch Set 3:

(6 comments)

Good job overall
Kudos for writing tests!!

....................................................
File lib/vdsm/libvirtconnection.py
Line 70:     Wrap methods of connection object so that they catch disconnection, and
Line 71:     take the current process down.
Line 72:     """
Line 73:     def wrapMethod(conn, f):
Line 74:         def wrapper(vdsmLibvirtPing=False, *args, **kwargs):
Took me a while to get what "vdsmLibvirtPing" means
maybe "pingLibvirt", it would align better with what the method does instead of who is using it. Also it would flip the default to true which is always a bit nicer
Line 75:             try:
Line 76:                 ret = f(*args, **kwargs)
Line 77:                 if isinstance(ret, libvirt.virDomain):
Line 78:                     for name in dir(ret):

Line 90:                           libvirt.VIR_ERR_NO_CONNECT,
Line 91:                           libvirt.VIR_ERR_INVALID_CONN)
Line 92:                 if edom in EDOMAINS and ecode in ECODES:
Line 93:                     if not vdsmLibvirtPing:
Line 94:                         conn.getLibVersion(vdsmLibvirtPing=True)
Since this can hang, it should be asynchronous.
Don't forget to lock so you only ping once (YOPO!)
Line 95:                     else:
Line 96:                         log.error('connection to libvirt broken.'
Line 97:                                   '  ecode: %d edom: %d', ecode, edom)
Line 98:                         if killOnFailure:

Line 92:                 if edom in EDOMAINS and ecode in ECODES:
Line 93:                     if not vdsmLibvirtPing:
Line 94:                         conn.getLibVersion(vdsmLibvirtPing=True)
Line 95:                     else:
Line 96:                         log.error('connection to libvirt broken.'
warning
Line 97:                                   '  ecode: %d edom: %d', ecode, edom)
Line 98:                         if killOnFailure:
Line 99:                             log.error('taking calling process down.')
Line 100:                             os.kill(os.getpid(), signal.SIGTERM)

Line 95:                     else:
Line 96:                         log.error('connection to libvirt broken.'
Line 97:                                   '  ecode: %d edom: %d', ecode, edom)
Line 98:                         if killOnFailure:
Line 99:                             log.error('taking calling process down.')
critical
Line 100:                             os.kill(os.getpid(), signal.SIGTERM)
Line 101:                         else:
Line 102:                             raise
Line 103:                 log.debug('Unknown libvirterror: ecode: %d edom: %d '

Line 104:                           'level: %d message: %s', ecode, edom,
Line 105:                           e.get_error_level(), e.get_error_message())
Line 106:                 raise
Line 107:         wrapper.__name__ = f.__name__
Line 108:         wrapper.__doc__ = f.__doc__
a bit unrelated but still. If you could do it in another patch while you are there.

http://docs.python.org/2/library/functools.html#functools.wraps
Line 109:         return wrapper
Line 110: 
Line 111:     def req(credentials, user_data):
Line 112:         passwd = file(constants.P_VDSM_LIBVIRT_PASSWD).readline().rstrip("\n")

....................................................
File tests/libvirtconnectionTests.py
Line 28: class TerminationException(Exception):
Line 29:     pass
Line 30: 
Line 31: 
Line 32: class LibvirtMock:
please use new style classes

short story:
inherit from object
long story:
http://docs.python.org/release/2.5.2/ref/node33.html
Line 33:     VIR_CRED_AUTHNAME = 1
Line 34:     VIR_CRED_PASSPHRASE = 2
Line 35:     VIR_FROM_RPC = 3
Line 36:     VIR_FROM_REMOTE = 4