Francesco Romani has posted comments on this change. Change subject: Live Merge: support active layer commit ......................................................................
Patch Set 5: Code-Review-1 (5 comments) Since we are waiting on libvirt, let's try to make this code nicer. -1 for visibility. http://gerrit.ovirt.org/#/c/28598/5/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 5541: flags |= libvirt.VIR_DOMAIN_BLOCK_COMMIT_ACTIVE Line 5542: except AttributeError: Line 5543: self.log.error("Unable to merge the active layer. Libvirt is " Line 5544: "missing VIR_DOMAIN_BLOCK_COMMIT_ACTIVE.") Line 5545: return errCode['mergeErr'] I think we can factor out this code with the new one added into http://gerrit.ovirt.org/#/c/28998 (which btw looks nice and ready to go). I think that rebasing this patch on 28998 will lead to simpler and cleaner code. Line 5546: Line 5547: try: Line 5548: self.trackBlockJob(jobUUID, drive, baseVolUUID, topVolUUID, Line 5549: 'commit') Line 5666: if x['volumeID'] in volumes] Line 5667: device['volumeChain'] = drive.volumeChain = newChain Line 5668: Line 5669: def _onBlockJobActiveCommitEvent(self, diskName): Line 5670: flags = libvirt.VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT this temporary looks unnecessary Line 5671: try: Line 5672: self._dom.blockJobAbort(diskName, flags) Line 5673: except libvirt.libvirtError: Line 5674: self.log.error("Failed to perform pivot", exc_info=True) Line 5670: flags = libvirt.VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT Line 5671: try: Line 5672: self._dom.blockJobAbort(diskName, flags) Line 5673: except libvirt.libvirtError: Line 5674: self.log.error("Failed to perform pivot", exc_info=True) self._reportException may be handy here Line 5675: Line 5676: def _onBlockJobEvent(self, diskName, blockJobType, status): Line 5677: def isActiveCommit(jobType, status): Line 5678: if status != libvirt.VIR_DOMAIN_BLOCK_JOB_READY: Line 5681: if jobType == libvirt.VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT: Line 5682: return True Line 5683: except AttributeError: Line 5684: pass Line 5685: return False Same as per line 5524: can we simplify this helper function , maybe after rebasing this patch on top of http://gerrit.ovirt.org/#/c/28998 ? Line 5686: Line 5687: drive = self._findDriveByName(diskName) Line 5688: try: Line 5689: jobID = self.getBlockJob(drive)['jobID'] Line 5686: Line 5687: drive = self._findDriveByName(diskName) Line 5688: try: Line 5689: jobID = self.getBlockJob(drive)['jobID'] Line 5690: except LookupError: Just caught my eye - can this raise KeyError also? Line 5691: self.log.debug("Ignoring event for untracked block job on disk " Line 5692: "'%s'", diskName) Line 5693: return Line 5694: -- To view, visit http://gerrit.ovirt.org/28598 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idab76081c2d004bc4f9e5bc9cb72e86845640f6a Gerrit-PatchSet: 5 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
