Adam Litke has posted comments on this change. Change subject: dump the core of a VM ......................................................................
Patch Set 17: (9 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. Done 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 Done 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 Done 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 429: ERROR = "error" Line 430: Line 431: def __init__(self, vm, to='', postAction=None, bypassCache=False, Line 432: memoryOnly=False): Line 433: threading.Thread.__init__(self) > super(CoreDumpThread, self).__init__() Done Line 434: self.log = vm.log Line 435: self._vm = vm Line 436: self.dumpfile = to Line 437: self.flags = 0 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 her Done Line 456: self.status = {'code': CoreDumpThread.Status.NONE, Line 457: 'message': 'No core dump operation'} Line 458: return status Line 459: Line 2949: return errCode['exist'] Line 2950: if self.isCoreDumpActive(): Line 2951: self.log.warning('VM is performing a core dump, ' Line 2952: 'cannot perform migration.') Line 2953: return errCode['exist'] > I think this errCode is misleading here. errCode 61 (added by this change) Done Line 2954: if self.hasTransientDisks(): Line 2955: return errCode['transientErr'] Line 2956: # while we were blocking, another migrationSourceThread could have Line 2957: # taken self Down 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. Done 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. here errCode['migInProgress'] is more appropriate. 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 Well there shouldn't be any errors possible. The only one I see is if postAction is invalid, but API.py should filter those out for us. I'll delete the except block altogether. 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'], -- 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 <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]> Gerrit-Reviewer: Better Saggi <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Doron Fediuck <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Itamar Heim <[email protected]> Gerrit-Reviewer: Jiří Moskovčák <[email protected]> Gerrit-Reviewer: Mark Wu <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Saggi Mizrahi <[email protected]> Gerrit-Reviewer: ShaoHe Feng <[email protected]> Gerrit-Reviewer: Shu Ming <[email protected]> Gerrit-Reviewer: Vinzenz Feenstra <[email protected]> Gerrit-Reviewer: Zhou Zheng Sheng <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
