Adam Litke has posted comments on this change. Change subject: Live Merge: fix race between libvirt event and vm stats ......................................................................
Patch Set 2: -Verified (6 comments) 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? No, this is part of the patch because we need to create a placeholder job when libvirt isn't reporting the information but vdsm still knows it is either starting or being cleaned up. In that case, we won't have libvirt to tell us what kind of job it is so we need to start tracking it ourselves. Since we are tracking it ourselves, we will just always use our info and no longer need to pay attention to the libvirt job type when sampling. 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 desc ok, will change to jobsRet and jobInfo 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. ok. 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? sure. will rework it. 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 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. ok, sure. 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? ok 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
