Dan Kenigsberg has posted comments on this change.
Change subject: Decoupling libvirtconnection.py from clientIF.py
......................................................................
Patch Set 6: I would prefer that you didn't submit this
(2 inline comments)
Yaniv, we have plenty "except libvirt.libvirtError" in their relevant context.
The one in libvirtconnection is designed to catch ONLY the "connection broken"
exception, handle it, and re-raise any other exception for handling in their
own context.
I think that this is the correct design - libvirtconnection.py cannot tell what
is the right handling for each conceivable usage of a libvirt call. And in any
case, such a revolution should wait for another patch.
I'm giving a -1 only due to unsolicited text editing of an unrelated comment.
....................................................
File lib/vdsm/libvirtconnection.py
Line 92: if edom in EDOMAINS and ecode in ECODES:
Line 93: log.error('connection to libvirt broken.'
Line 94: ' ecode: %d edom: %d', ecode, edom)
Line 95: if killOnFailure:
Line 96: log.error('taking calling process down.')
We kill the process that calls the function. This is clear enough for me.
The fact that prepareForShutdown is unbounded is sad, but unrelated to this
patch - the patch does nothing to worsen it.
Line 97: os.kill(os.getpid(), signal.SIGTERM)
Line 98: else:
Line 99: log.debug('Unknown libvirterror: ecode: %d edom:
%d '
Line 100: 'level: %d message: %s', ecode, edom,
Line 141: ev,
Line 142:
target.dispatchLibvirtEvents,
Line 143: ev)
Line 144: # In case we're running into troubles with
Line 145: # keeping the connections alive we should place here:
Why do you care to edit this comment? It is unrelated to this patch - better to
let it be.
Line 146: # conn.setKeepAlive(interval=5, count=3)
Line 147: # However the values need to be
Line 148: # considered wisely to not affect
Line 149: # hosts which are hosting a lot of virtual machines
--
To view, visit http://gerrit.ovirt.org/16393
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibc550ea40ebb3abcf37ebfa59cbf8df8e42daa96
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: mooli tayer <[email protected]>
Gerrit-Reviewer: Barak Azulay <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Yaniv Bronhaim <[email protected]>
Gerrit-Reviewer: mooli tayer <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
