Francesco Romani has posted comments on this change. Change subject: vm: avoid to reply with half-baked statistics ......................................................................
Patch Set 5: (5 comments) http://gerrit.ovirt.org/#/c/25803/5//COMMIT_MSG Commit Message: Line 9: commit 8fedf8bde3c28edb07add23c3e9b72681cb48e49 introduced a tiny Line 10: window for races between libvirt notifications (vm._onQemuDeath) Line 11: and polling for stats from engine. Line 12: Line 13: This patch address tackles this problem by moving toward a more > s/address// Oops. Will fix. Line 14: 'transactional' behaviour for statistics gathering. Line 15: Either we reply to the engine's enquiry with a well formed, Line 16: complete statistic sample, or we don't reply at all, and instead Line 17: we raise an error code. Line 16: complete statistic sample, or we don't reply at all, and instead Line 17: we raise an error code. Line 18: Line 19: Change-Id: I65197459cd183af5e7a1634a5ffb01719364a070 Line 20: Bug-Url: https://bugzilla.redhat.com/1073478 > Please explain how this patch resolves this bug - I do not understand it at Will amend the commit message with proper and comprehensive explanation. http://gerrit.ovirt.org/#/c/25803/5/vdsm/API.py File vdsm/API.py: Line 361: """ Line 362: v = self._cif.vmContainer.get(self._UUID) Line 363: if not v: Line 364: return errCode['noVM'] Line 365: stats = v.getStats().copy() > why do we need the intermediate conversion of Exception to an empty dict? i Yes, I think it is better. Will fix. Line 366: if not stats: Line 367: return errCode['statsErr'] Line 368: stats['vmId'] = self._UUID Line 369: return {'status': doneCode, 'statsList': [stats]} http://gerrit.ovirt.org/#/c/25803/5/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 2819: if self._watchdogEvent: Line 2820: stats["watchdogEvent"] = self._watchdogEvent Line 2821: except Exception: Line 2822: self.log.error("Error fetching vm stats", exc_info=True) Line 2823: stats = {} > so we go with returning an empty dict? Is it because the older engines woul No, I'm not satisfied yet, and I'm not fully confident this is the proper solution. I added empty dict here just to make the code consistent with the commit message and with the advertised fix. That said, I'm not satisfied with the actual result for the very reasons you just described, and I definitely want to carefully investigate them. Most important, I haven't not yet inspected the code of the engine. Will do all of the above before to upload a next version of the patch. Line 2824: return stats Line 2825: Line 2826: def _getStatsInternal(self): Line 2827: # used by API.Vm.getStats Line 2901: stats['timeOffset'] = self.conf.get('timeOffset', '0') Line 2902: stats['clientIp'] = self.conf.get('clientIp', '') Line 2903: if 'pauseCode' in self.conf: Line 2904: stats['pauseCode'] = self.conf['pauseCode'] Line 2905: stats.update(self.guestAgent.getGuestInfo()) > much better, but please pre-verify that guestAgent is not None. Will do. Line 2906: memUsage = 0 Line 2907: realMemUsage = int(stats['memUsage']) Line 2908: if realMemUsage != 0: Line 2909: memUsage = (100 - float(realMemUsage) / -- To view, visit http://gerrit.ovirt.org/25803 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I65197459cd183af5e7a1634a5ffb01719364a070 Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Jarod.w <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Omer Frenkel <[email protected]> Gerrit-Reviewer: Roy Golan <[email protected]> Gerrit-Reviewer: Vinzenz Feenstra <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
