Change in vdsm[master]: vm: avoid to reply with half-baked statistics

vfeenstr at redhat.com vfeenstr at redhat.com
Fri Mar 28 09:18:20 UTC 2014


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 <fromani at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofrenkel at redhat.com>
Gerrit-Reviewer: Roy Golan <rgolan at redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list