Francesco Romani has posted comments on this change.
Change subject: dump the core of a VM
......................................................................
Patch Set 17:
(11 comments)
http://gerrit.ovirt.org/#/c/7329/17/client/vdsClient.py
File client/vdsClient.py:
Line 1760: try:
Line 1761: return {"true": True, "false": False}[v]
Line 1762: except KeyError:
Line 1763: raise ValueError("Option '%s' must be
'true' or 'false'" % k)
Line 1764:
I think we can use utils.tobool and replace most if not all of this helper.
Line 1765: vmId = args[0]
Line 1766: coreFile = args[1]
Line 1767: postAction = None
Line 1768: bypassCache = False
http://gerrit.ovirt.org/#/c/7329/17/vdsm/API.py
File vdsm/API.py:
Line 283: if postAction and postAction not in ("live",
"crash", "reset"):
Line 284: msg = "Invalid value '%s' for postAction" %
postAction
Line 285: return reportError(msg)
Line 286: if not isinstance(bypassCache, bool):
Line 287: return reportError("boolean value expected for
bypassCache")
Do we really need type checking here? I mean, why isn't enough to check for bool value
and we must ensure bool type?
Line 288: if not isinstance(memoryOnly, bool):
Line 289: return reportError("boolean value expected for
memoryOnly")
Line 290:
Line 291: v = self._cif.vmContainer.get(self._UUID)
Line 285: return reportError(msg)
Line 286: if not isinstance(bypassCache, bool):
Line 287: return reportError("boolean value expected for
bypassCache")
Line 288: if not isinstance(memoryOnly, bool):
Line 289: return reportError("boolean value expected for
memoryOnly")
ditto
Line 290:
Line 291: v = self._cif.vmContainer.get(self._UUID)
Line 292: if not v:
Line 293: return errCode['noVM']
http://gerrit.ovirt.org/#/c/7329/17/vdsm/vm.py
File vdsm/vm.py:
Line 433: )
super(CoreDumpThread, self).__init__()
would be nicer
Line 451: """
Line 452: if self.isAlive():
Line 453: return self.status
Line 454: else:
Line 455: status = self.status
Maybe a little helper which resets the status would led to clearer code here.
Line 456: self.status = {'code': CoreDumpThread.Status.NONE,
Line 457: 'message': 'No core dump operation'}
Line 458: return status
Line 459:
Line 2953: ']
I think this errCode is misleading here. errCode 61 (added by this change) may be slightly
better, but I think we need another explicit error code here.
Line 2992: self._acquireCpuLockWithTimeout()
Line 2993: try:
Line 2994: if self.isCoreDumpActive():
Line 2995: self.log.warning('Core dump is already in progress')
Line 2996: return errCode['exist']
if above applies, same here.
Line 2997: if self.isMigrating():
Line 2998: self.log.warning('VM is being migrated, '
Line 2999: 'cannot perform core dump')
Line 3000: return errCode['exist']
Line 2996: return errCode['exist']
Line 2997: if self.isMigrating():
Line 2998: self.log.warning('VM is being migrated, '
Line 2999: 'cannot perform core dump')
Line 3000: return errCode['exist']
and here.
Line 3001:
Line 3002: # Perform the core dump even if VM status is "Down"
Line 3003: self._coredumpThread = CoreDumpThread(self, to, postAction,
Line 3004: bypassCache, memoryOnly)
Line 3003: self._coredumpThread = CoreDumpThread(self, to, postAction,
Line 3004: bypassCache, memoryOnly)
Line 3005: self._coredumpThread.dumpStart()
Line 3006: return {'status': doneCode}
Line 3007: except Exception as e:
Any chance to narrow down this? We already have a lot of
except: or except Exception: in the codebase and I would *really* like to not increment
the number... :\
Line 3008: self.log.error("Failed to perform core dump. %s",
e.message)
Line 3009: status = self._coredumpThread.getStatus()
Line 3010: return {'status':
Line 3011: {'code':
errCode['coreDumpErr']['status']['code'],
http://gerrit.ovirt.org/#/c/7329/17/vdsm_api/vdsmapi-schema.json
File vdsm_api/vdsmapi-schema.json:
Line 5898: # @live: Do not interrupt the running VM
Line 5899: #
Line 5900: # @reset: Reboot the VM after gathering core dump
Line 5901: #
Line 5902: # Since: 4.14.2
4.15.0?
Line 5903: ##
Line 5904: {'enum': 'CoreDumpPostAction', 'data':
['crash', 'live', 'reset']}
Line 5905:
Line 5906: ##
Line 5917: # @bypassCache: #optional Avoid file system cache when saving
Line 5918: #
Line 5919: # @memoryOnly: #optional Dump VM's memory only
Line 5920: #
Line 5921: # Since: 4.14.2
same here
Line 5922: #
Line 5923: ##
Line 5924: {'command': {'class': 'VM', 'name':
'coreDump'},
Line 5925: 'data': {'to': 'str', 'postAction':
'CoreDumpPostAction',
--
To view, visit
http://gerrit.ovirt.org/7329
To unsubscribe, visit
http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If4aac9e747dc7aa64a6ff5ef256a7a4375aa2bb5
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: ShaoHe Feng <shaohef(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Better Saggi <bettersaggi(a)gmail.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfediuck(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Itamar Heim <iheim(a)redhat.com>
Gerrit-Reviewer: Jiří Moskovčák <jmoskovc(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: ShaoHe Feng <shaohef(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Shu Ming <shuming(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr(a)redhat.com>
Gerrit-Reviewer: Zhou Zheng Sheng <zhshzhou(a)linux.vnet.ibm.com>
Gerrit-HasComments: Yes