Dan Kenigsberg has posted comments on this change. Change subject: vm: avoid to reply with half-baked statistics ......................................................................
Patch Set 5: Code-Review-1 (4 comments) Please add a unit test, documenting the expected result for getAllVmStats if one of two VMs has partial statistics? 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// 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 all. 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? it would be a little less ugly to catch Exception up here instead of in vm.py. 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 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. 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 <from...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jarod.w <work.iec23...@gmail.com> Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Roy Golan <rgo...@redhat.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@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