Federico Simoncelli has posted comments on this change. Change subject: Live Merge: fix race between libvirt event and vm stats ......................................................................
Patch Set 2: Code-Review-1 (7 comments) Minor comments, mostly just questions. Marking -1 for visibility. http://gerrit.ovirt.org/#/c/29309/2/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 5430: job = self.getBlockJob(drive) Line 5431: except LookupError: Line 5432: newJob = {'jobID': jobID, 'disk': driveSpec, Line 5433: 'baseVolume': base, 'topVolume': top, Line 5434: 'strategy': strategy, 'blockJobType': 'commit', Is this change (and the BlockJobTypeMap one) unrelated? Line 5435: 'chain': self._driveGetActualVolumeChain(drive)} Line 5436: self.conf['_blockJobs'][jobID] = newJob Line 5437: else: Line 5438: self.log.error("A block job with id %s already exists for vm " Line 5452: self.saveState() Line 5453: return True Line 5454: Line 5455: def queryBlockJobs(self): Line 5456: jobs = {} "jobs" and "job" gets me confused, we could try to find a longer, more descriptive name for this dictionary. Line 5457: # We need to take the jobs lock here to ensure that we don't race with Line 5458: # another call to merge() where the job has been recorded but not yet Line 5459: # started. Line 5460: with self._jobsLock: Line 5457: # We need to take the jobs lock here to ensure that we don't race with Line 5458: # another call to merge() where the job has been recorded but not yet Line 5459: # started. Line 5460: with self._jobsLock: Line 5461: for job in self.conf['_blockJobs'].values()[:]: I think "[:]" is redundant, values() should already create a new list. Line 5462: jobID = job['jobID'] Line 5463: drive = self._findDriveByUUIDs(job['disk']) Line 5464: Line 5465: ret = None Line 5460: with self._jobsLock: Line 5461: for job in self.conf['_blockJobs'].values()[:]: Line 5462: jobID = job['jobID'] Line 5463: drive = self._findDriveByUUIDs(job['disk']) Line 5464: Could it make sense to prepare the entry here? entry = {'id': jobID, 'jobType': 'block', ... Line 5465: ret = None Line 5466: if 'done' not in job: Line 5467: ret = self._dom.blockJobInfo(drive.name, 0) Line 5468: if not ret: Line 5468: if not ret: Line 5469: self.log.debug("Block Job for vm:%s, img:%s has ended", Line 5470: self.conf['vmId'], Line 5471: job['disk']['imageID']) Line 5472: job['done'] = True If you prepared the entry at 5464, here you can set the bandwidth and all the rest, e.g.: if ret: entry['bandwidth'] = ret['bandwidth'] entry['cur'] = str(ret['cur']) entry['end'] = str(ret['end']) else: self.log.debug(...) job['done'] = True Line 5473: Line 5474: entry = {'id': jobID, 'jobType': 'block', Line 5475: 'blockJobType': job['blockJobType'], Line 5476: 'bandwidth': 0, 'cur': '0', 'end': '0', Line 5470: self.conf['vmId'], Line 5471: job['disk']['imageID']) Line 5472: job['done'] = True Line 5473: Line 5474: entry = {'id': jobID, 'jobType': 'block', Now you can remove all these, from line 5474 to 5483. Line 5475: 'blockJobType': job['blockJobType'], Line 5476: 'bandwidth': 0, 'cur': '0', 'end': '0', Line 5477: 'imgUUID': job['disk']['imageID']} Line 5478: Line 5557: flags) Line 5558: if ret != 0: Line 5559: raise RuntimeError("blockCommit failed rc:%i", ret) Line 5560: except (RuntimeError, libvirt.libvirtError): Line 5561: self.log.error("Live merge failed for '%s'", drive.path) self.log.exception? Line 5562: self.untrackBlockJob(jobUUID) Line 5563: return errCode['mergeErr'] Line 5564: Line 5565: # blockCommit will cause data to be written into the base volume. -- To view, visit http://gerrit.ovirt.org/29309 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic0314a39428419bb21bc579f07a92227d5a7acff Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke <[email protected]> Gerrit-Reviewer: Adam Litke <[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
