Vinzenz Feenstra has posted comments on this change. Change subject: vm: avoid to reply with half-baked statistics ......................................................................
Patch Set 3: (2 comments) http://gerrit.ovirt.org/#/c/25803/3/vdsm/API.py File vdsm/API.py: Line 361: v = self._cif.vmContainer.get(self._UUID) Line 362: if not v: Line 363: return errCode['noVM'] Line 364: stats = v.getStats().copy() Line 365: if not stats: > Yep, this was actually a typo of mine. Hmm I am not sure if this is a good idea to return partial data. That might have an unexpected impact on the engine because it might expect certain fields when data is there which aren't there then. This has to be verified that the engine does not make such assumptions. Even more problematic I see other vdsm API consumers we really should follow the API definition of optional and non-optional to make such a decision. Line 366: return errCode['statsErr'] Line 367: stats['vmId'] = self._UUID Line 368: return {'status': doneCode, 'statsList': [stats]} Line 369: http://gerrit.ovirt.org/#/c/25803/3/vdsm/vm.py File vdsm/vm.py: Line 2820: self.log.error("Error fetching vm stats", exc_info=True) Line 2821: return stats Line 2822: Line 2823: def _getStatsInternal(self): Line 2824: # used by API.Vm.getStats > the comment doesn't seem to be true actually, the only one calling this is Yeah the getStatsInternal call was introduced during the merge of libvirtvm.py and vm.py to avoid having to rewrite the logic of the code and by that introduce bugs. This was due to the fact that there is a return when the lastStatus is 'Down' and I did not want to introduce non-trivial changes in the merge. I'd be happy to see those two merged. Line 2825: Line 2826: def _getGuestStatus(): Line 2827: GUEST_WAIT_TIMEOUT = 60 Line 2828: now = time.time() -- 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: 3 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: 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
