Piotr Kliczewski has posted comments on this change. Change subject: events: vm status notifications ......................................................................
Patch Set 1: (5 comments) https://gerrit.ovirt.org/#/c/38937/1/tests/jsonRpcTests.py File tests/jsonRpcTests.py: Line 59: return None Line 60: Line 61: def double_response(self): Line 62: n = self.cif.create_notification('vdsm.double_response') Line 63: n.emit(**{CALL_ID: True}) > related? I wanted to be sure that this structure works nicely. Not really related to the change. Line 64: return 'sent' Line 65: Line 66: def register_server_address(self, server_address): Line 67: self.server_address = server_address Line 192: pass Line 193: else: Line 194: def callback(client, event, params): Line 195: self.assertEquals(event, 'vdsm.double_response') Line 196: self.assertEquals(params, {CALL_ID: True}) > same As above Line 197: Line 198: client.registerEventCallback(callback) Line 199: res = self._callTimeout(client, "double_response", [], Line 200: CALL_ID, timeout=CALL_TIMEOUT) https://gerrit.ovirt.org/#/c/38937/1/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 867: with self._statusLock: Line 868: notification = self.cif.create_notification( Line 869: '|virt|VM_status|' + self.id Line 870: ) Line 871: notification.emit(**{self.id: value}) > this hides a lot IMHO. We need a simpler way to achieve the same result. As replied to other comment I will simplify it. Line 872: if self._lastStatus == vmstatus.DOWN: Line 873: self.log.warning( Line 874: 'trying to set state to %s when already Down', Line 875: value) Line 1624: self._lastStatus in vmstatus.PAUSED_STATES): Line 1625: notification = self.cif.create_notification( Line 1626: '|virt|VM_status|' + self.id Line 1627: ) Line 1628: notification.emit(**{self.id: 'Paused'}) > Need to check but I'm quite confident we can get rid of this hunk. Not a bi As I understood from our conversation we do not update vm state to 'Paused' but check for it when calling _get_lastStatus so if we want to send I think this is good place to do so. Line 1629: finally: Line 1630: if not guestCpuLocked: Line 1631: self._guestCpuLock.release() Line 1632: Line 5027: stats['monitorResponse'] = '-1' Line 5028: notification = self.cif.create_notification( Line 5029: '|virt|VM_status|' + self.id Line 5030: ) Line 5031: notification.emit(**{self.id: 'NotResponding'}) > We'll probably need the same treatment inside _timeoutExperienced. Done Line 5032: Line 5033: Line 5034: class LiveMergeCleanupThread(threading.Thread): Line 5035: def __init__(self, vm, jobId, drive, doPivot): -- To view, visit https://gerrit.ovirt.org/38937 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I746299f9f1e2f49831a05072f19af1d242796276 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Dima Kuznetsov <dkuzn...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <mskri...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: Yeela Kaplan <ykap...@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