Dan Kenigsberg has posted comments on this change. Change subject: Live Merge: Ignore libvirt block job events ......................................................................
Patch Set 8: Code-Review-1 (5 comments) http://gerrit.ovirt.org/#/c/30046/8/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 1531: if (self.arch not in ['ppc64', 'x86_64']): Line 1532: raise RuntimeError('Unsupported architecture: %s' % self.arch) Line 1533: Line 1534: self._powerDownEvent = threading.Event() Line 1535: self.liveMergeCleanupThreads = {} I see no reason to define this as public Line 1536: Line 1537: def _get_lastStatus(self): Line 1538: PAUSED_STATES = (vmstatus.POWERING_DOWN, vmstatus.REBOOT_IN_PROGRESS, Line 1539: vmstatus.UP) Line 5285: 'bandwidth': 0, 'cur': '0', 'end': '0', Line 5286: 'imgUUID': storedJob['disk']['imageID']} Line 5287: Line 5288: liveInfo = None Line 5289: if 'done' not in storedJob: I find the term 'done' quite confusing. It's existence (and not its value) in the job entry only means that we have read blockJobInfo() from libvirt, and that 'bandwidth' is not the fake 0 value we start with. It does not mean that the job (or its cleanup) is 'done'. Line 5290: try: Line 5291: liveInfo = self._dom.blockJobInfo(drive.name, 0) Line 5292: except libvirt.libvirtError: Line 5293: self.log.exception("Error getting block job info") Line 5293: self.log.exception("Error getting block job info") Line 5294: jobsRet[jobID] = entry Line 5295: continue Line 5296: Line 5297: if liveInfo: elsewhere you have a much more complex test for the validity of blockJobsInfo (not isinstance(blkJobInfo, dict) or 'cur' not in blkJobInfo or 'end' not in blkJobInfo) why? Line 5298: entry['bandwidth'] = liveInfo['bandwidth'] Line 5299: entry['cur'] = str(liveInfo['cur']) Line 5300: entry['end'] = str(liveInfo['end']) Line 5301: else: Line 5307: # ended. Spawn an async cleanup. Line 5308: startCleanup(storedJob, drive) Line 5309: elif cleanThread.isAlive(): Line 5310: # Let previously started cleanup thread continue Line 5311: self.log.debug("Still waiting for block job %s to be " How often would this spam the logs? Line 5312: "cleaned up", jobID) Line 5313: elif not cleanThread.isSuccessful(): Line 5314: # At this point we know the thread is not alive and the Line 5315: # cleanup failed. Retry it with a new thread. Line 5536: self.vm = vm Line 5537: self.jobId = jobId Line 5538: self.drive = drive Line 5539: self.success = False Line 5540: Please add @utils.traceback() Line 5541: def run(self): Line 5542: self.vm.log.info("Starting live merge cleanup for job %s", Line 5543: self.jobId) Line 5544: ret = self.vm.handleBlockJobEvent(self.jobId, self.drive, 'finished') -- To view, visit http://gerrit.ovirt.org/30046 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibbcdca4c0c0e45e9323ecfef9ce2fce10d8451a5 Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Greg Padgett <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
