Dan Kenigsberg has posted comments on this change. Change subject: Decoupling libvirtconnection.py from clintIf.py ......................................................................
Patch Set 1: I would prefer that you didn't submit this (7 inline comments) Thanks Mooli, I really want to have something in these lines merged. .................................................... Commit Message Line 14: causes vdsm to shutdown on libvirt connection error) Line 15: Line 16: These two come in the same patch because removing Line 17: the cli dependency (clientIF instance) from libvirtconnection Line 18: must handle errors in a new way - sendind SIGTERM to the current changing this could have been done in a first independent patch. (I would prefer that it had) Line 19: process which solves the bug. Line 20: Line 21: Change-Id: Ibc550ea40ebb3abcf37ebfa59cbf8df8e42daa96 Line 22: Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=958367 .................................................... File lib/vdsm/libvirtconnection.py Line 75: if callable(method) and name[0] != '_': Line 76: setattr(ret, name, wrapMethod(method)) Line 77: return ret Line 78: except libvirt.libvirtError as e: Line 79: if target is not None and hasattr(target, 'log'): why do we need this dynamic attribute test? Do we expect a "target" with no logger? Line 80: log = target.log Line 81: else: Line 82: log = logging.getLogger() Line 83: edom = e.get_error_domain() Line 92: log.error('connection to libvirt broken. ') Line 93: if killOnFailure: Line 94: log.error('taking calling process down. ecode:' Line 95: ' %d edom: %d', ecode, edom) Line 96: os.kill(os.getpid(), 15) signal.SIGTERM == 15. better use it. I believe you've mistakenly mangled the logging message. Line 97: else: Line 98: log.debug('Unknown libvirterror: ecode: %d edom: %d ' Line 99: 'level: %d message: %s', ecode, edom, Line 100: e.get_error_level(), e.get_error_message()) .................................................... File tests/vmTests.py Line 31: from monkeypatch import MonkeyPatch Line 32: from vmTestsData import CONF_TO_DOMXML Line 33: Line 34: Line 35: class connectionMock: UseCapitals for class names. Line 36: def domainEventRegisterAny(self, *arg): Line 37: pass Line 38: cm = connectionMock() Line 39: Line 34: Line 35: class connectionMock: Line 36: def domainEventRegisterAny(self, *arg): Line 37: pass Line 38: cm = connectionMock() This object has no state; better instantiate it only within the lambda expression that returns it. Line 39: Line 40: Line 41: class TestVm(TestCaseBase): Line 42: .................................................... File vdsm/vm.py Line 1673: libvirt.VIR_DOMAIN_EVENT_ID_IO_ERROR_REASON, Line 1674: libvirt.VIR_DOMAIN_EVENT_ID_GRAPHICS, Line 1675: libvirt.VIR_DOMAIN_EVENT_ID_BLOCK_JOB, Line 1676: libvirt.VIR_DOMAIN_EVENT_ID_WATCHDOG): Line 1677: self._connection.domainEventRegisterAny(None, I do not think we should call domainEventRegisterAny() multiple times, once whenever a new VM is started. Better do it once after the connection is first created (in libvirtconnection.py). Registering to all events is a pretty generic use case, that can be kept there. Line 1678: ev, Line 1679: _dispatchLibvirtEvent, Line 1680: (cif, ev)) Line 1681: Line 4563: hooks.before_vm_migrate_destination(srcDomXML, self.conf) Line 4564: return True Line 4565: Line 4566: Line 4567: def _dispatchLibvirtEvent(conn, dom, *args): I think it would be nicer if this was a method of the clientIF object. Then, it could be passed alone to the libvirtconnection.get() method. Thus, there would be no need to have libvirt's callback pass a reference to the "cif" object - the method would have its "self" object anyway. Line 4568: try: Line 4569: cif, eventid = args[-1] Line 4570: vmid = dom.UUIDString() Line 4571: v = cif.vmContainer.get(vmid) -- 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: 1 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: oVirt Jenkins CI Server _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
