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

michal.skrivanek at redhat.com michal.skrivanek at redhat.com
Fri Mar 28 08:01:48 UTC 2014


Michal Skrivanek has posted comments on this change.

Change subject: vm: avoid to reply with half-baked statistics
......................................................................


Patch Set 3:

(3 comments)

not sure about the partial stats
otherwise looks good

http://gerrit.ovirt.org/#/c/25803/3/lib/vdsm/define.py
File lib/vdsm/define.py:

Line 139:         'code': 61,
Line 140:         'message': 'Failed to set Hosted Engine HA policy'}},
Line 141:     'statsErr': {'status': {
Line 142:         'code': 62,
Line 143:         'message': 'Failed to gather statistics'}},
please doublecheck and probably handle the new return code in VdcUpdateRuntimeInfo code.
it needs to work without engine changes(for backward compatibility), but it would be nice to have a specific log message in newer engines for this case
Line 144:     'recovery': {'status': {
Line 145:         'code': 99,
Line 146:         'message': 'Recovering from crash or Initializing'}},
Line 147: }


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:
hm, what about partial stats? if the exception occurs later in the stats gathering doesn't this contain something already? 
It also may make sense to return it together with the err code (but need to check the current/legacy engine behavior in this case)
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 getStats right above.
Would you mind to start refactoring this a little bit? The watchdogEvent above is also quite ugly. AFAICT there's no need for an extra getStatsInternal()

as a totally separate effort of course, nothing to do with this patch:)
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