Francesco Romani has posted comments on this change. Change subject: events: make sure to send only one event per status ......................................................................
Patch Set 2: (4 comments) -1 for visibility/discussion I'd prefer to notify(), and log, without the lock held, and should be easily done. Other than that, looks OK. Please see inline comments for suggestions. Some typos in the commit message, which I don't really care too much about. https://gerrit.ovirt.org/#/c/42579/2//COMMIT_MSG Commit Message: Line 6: Line 7: events: make sure to send only one event per status Line 8: Line 9: Initially we made an assumption that engine can handle more than one Line 10: event containing the same status. The assumtion is not true and there typo: assumption Line 11: are flows where engine logic assumes only one status change. Line 12: Line 13: One of the problematic flows is vm distroy for which having duplicate Line 14: means that the operation failed and recovery flow is triggered. Line 9: Initially we made an assumption that engine can handle more than one Line 10: event containing the same status. The assumtion is not true and there Line 11: are flows where engine logic assumes only one status change. Line 12: Line 13: One of the problematic flows is vm distroy for which having duplicate typo: destroy Line 14: means that the operation failed and recovery flow is triggered. Line 15: Line 16: Line 17: Change-Id: I044a8f409ccdac4210784e21167bab320ddfe808 https://gerrit.ovirt.org/#/c/42579/2/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 381: self._lastStatus = value Line 382: Line 383: def send_status_event(self): Line 384: vm_status = self._getVmStatus()['status'] Line 385: with self._eventLock: ok with the concept, but... Line 386: if vm_status != self._evaluatedStatus: Line 387: self._evaluatedStatus = vm_status Line 388: stats = { Line 389: 'status': vm_status, Line 399: if 'exitMessage' in self.conf: Line 400: stats['exitMessage'] = self.conf['exitMessage'] Line 401: if 'exitReason' in self.conf: Line 402: stats['exitReason'] = self.conf['exitReason'] Line 403: until here, we're copying data around, or doing easy computations (hash()). So far so good. I'm a bit worried by logging and _notify()ing with a lock held. I'm sure most of time will be OK, but still... It should be easy to move them both outside the lock, maybe with an helper variable for self.lastStatus. So, it will read something like: with. self._eventLock: # produce stats. If not stats needed, let's do stats = {} # ... current_status = self.lastStatus # to log the same thing we used to do logic if stats: self.log.debug('Last status %s and evaluated status %s', current_status, vm_status) self._notify('VM_status', stats) # method ends here this way should be a bit safer. Line 404: self.log.debug('Last status %s and evaluated status %s', Line 405: self.lastStatus, vm_status) Line 406: self._notify('VM_status', stats) Line 407: -- To view, visit https://gerrit.ovirt.org/42579 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I044a8f409ccdac4210784e21167bab320ddfe808 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI 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-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
