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 <[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
