Nir Soffer has posted comments on this change. Change subject: Live Merge: Return more info from _driveGetActualVolumeChain ......................................................................
Patch Set 4: (7 comments) I'm not convinced that this change is needed, and it make the code little uglier instead of little better. http://gerrit.ovirt.org/#/c/31364/4//COMMIT_MSG Commit Message: Line 9: In followup patches, we want to return more information about each path Line 10: component from _driveGetActualVolumeChain. We need the actual path as Line 11: given in the domain XML in order to properly update our metadata after Line 12: an active layer merge. We also want to use this function to return high Line 13: write watermark information for block volumes. This patch just converts Why do we want to do that? Currently we monitor every 2 seconds the write watermark, so we can use the cached value from the last monitor call - right? Line 14: the format returned by _driveGetActualVolumeChain from a simple list to Line 15: a list of named tuples. Line 16: Line 17: Change-Id: I01d142a68903048accc95eb04d9930c326965db0 Line 11: given in the domain XML in order to properly update our metadata after Line 12: an active layer merge. We also want to use this function to return high Line 13: write watermark information for block volumes. This patch just converts Line 14: the format returned by _driveGetActualVolumeChain from a simple list to Line 15: a list of named tuples. The problem with this change is that it make the code more ugly, just to get more info that we already get otherwise. Line 16: Line 17: Change-Id: I01d142a68903048accc95eb04d9930c326965db0 Line 18: Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1109920 http://gerrit.ovirt.org/#/c/31364/4/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 194: VolumeChainEntry = namedtuple('VolumeChainEntry', ['uuid']) Line 195: Line 196: Line 197: def _volumeChainToVolumeIds(chain): Line 198: return [x.uuid for x in chain] Please use meaningful variables names, like "vol" or "drive". Also can you put the private helpers at the end of the file, instead of the top? Or just inline this where it is used - this helper does not help a lot, and it pollute the namespace of this module. Line 199: Line 200: Line 201: class VmStatsThread(AdvancedStatsThread): Line 202: MBPS_TO_BPS = 10 ** 6 / 8 Line 5286: except LookupError: Line 5287: newJob = {'jobID': jobID, 'disk': driveSpec, Line 5288: 'baseVolume': base, 'topVolume': top, Line 5289: 'strategy': strategy, 'blockJobType': 'commit', Line 5290: 'chain': chain} Maybe accept a chain of named tuples, and extract here the uuids? Line 5291: self.conf['_blockJobs'][jobID] = newJob Line 5292: else: Line 5293: self.log.error("Cannot add block job %s. A block job with id " Line 5294: "%s already exists for image %s", jobID, Line 5396: return errCode['imageErr'] Line 5397: Line 5398: # Check that libvirt exposes full volume chain information Line 5399: ret = self._driveGetActualVolumeChain(drive) Line 5400: if ret is None: This would be nicer if we use the returned chain instead of building a list of uuids from it. Line 5401: self.log.error("merge: libvirt does not support volume chain " Line 5402: "monitoring. Unable to perform live merge.") Line 5403: return errCode['mergeErr'] Line 5404: chain = _volumeChainToVolumeIds(ret) Line 5538: return Line 5539: Line 5540: curVols = [x['volumeID'] for x in drive.volumeChain] Line 5541: ret = self._driveGetActualVolumeChain(drive) Line 5542: if ret is None: It would be nice if we always use the same name for the result of self._driveGetActualVolumeChain, and "chain" seems to be the best name for it. Line 5543: self.log.debug("Unable to determine volume chain. Skipping volume " Line 5544: "chain synchronization for drive %s", drive.name) Line 5545: return Line 5546: Line 5543: self.log.debug("Unable to determine volume chain. Skipping volume " Line 5544: "chain synchronization for drive %s", drive.name) Line 5545: return Line 5546: Line 5547: volumes = _volumeChainToVolumeIds(ret) This is not the best name for this, since we have both volume objects and volume ids, we probably should clean up this part of the code later. Line 5548: self.log.debug("vdsm chain: %s, libvirt chain: %s", curVols, volumes) Line 5549: Line 5550: # Ask the storage to sync metadata according to the new chain Line 5551: self.cif.irs.imageSyncVolumeChain(drive.domainID, drive.imageID, -- To view, visit http://gerrit.ovirt.org/31364 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I01d142a68903048accc95eb04d9930c326965db0 Gerrit-PatchSet: 4 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: Nir Soffer <[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
