Francesco Romani has posted comments on this change. Change subject: Live Merge: Extend internal volumes during live merge ......................................................................
Patch Set 1: (5 comments) Some notes. Most of them are minor, but I'd like to have the direct QMP call tested somehow before to add my +1. http://gerrit.ovirt.org/#/c/28597/1/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 2287: Line 2288: def _getMergeWriteWatermarks(self): Line 2289: ret = {} Line 2290: cmd = {'execute': 'query-blockstats'} Line 2291: resp = self._internalQMPMonitorCommand(cmd) Well, I guess this is a necessary evil. I can live with this, but please provide some unit tests for this method. Line 2292: for device in resp['return']: Line 2293: name = device['device'] Line 2294: if not name.startswith('drive-'): Line 2295: continue Line 2347: continue Line 2348: info['volumeID'] = volumeID Line 2349: self.log.debug("Adding live merge extension candidate: volume=%s", Line 2350: volumeID) Line 2351: ret[drive.imageID] = info I think it is more readable as it follows: if volumeID in watermarks: self.log.debug("Adding live merge extension candidate: volume=%s", volumeID) ret[drive.imageID] = { 'alloc': watermarks[volumeID] 'physical':int(volSize['truesize']) 'capacity': drive.apparentsize, 'volumeID': volumeID} else: self.log.warning( "No watermark info available for %s", volumeID) No big deal however. Your call. Line 2352: return ret Line 2353: Line 2354: def _getExtendCandidates(self): Line 2355: ret = [] Line 2358: for drive in self._devices[DISK_DEVICES]: Line 2359: if not drive.blockDev or drive.format != 'cow': Line 2360: continue Line 2361: Line 2362: imageID = drive.imageID We can get rid of this temporary. Line 2363: capacity, alloc, physical = self._dom.blockInfo(drive.path, 0) Line 2364: ret.append((drive, drive.volumeID, capacity, alloc, physical)) Line 2365: Line 2366: try: Line 5217: with self._confLock: Line 5218: try: Line 5219: job = self.getBlockJob(drive) Line 5220: except LookupError: Line 5221: chain = self._driveGetActualVolumeChain(drive) Do we need this temporary? Line 5222: newJob = {'jobID': jobID, 'disk': driveSpec, Line 5223: 'baseVolume': base, 'topVolume': top, Line 5224: 'strategy': strategy, 'chain': chain} Line 5225: self.conf['_blockJobs'][jobID] = newJob Line 5446: self.log.warning("Ignoring unrecognized block job type: '%s'", Line 5447: blockJobType) Line 5448: return Line 5449: Line 5450: self.log.debug("Live merge completed for path '%s'", path) I think this deserves to be info() Line 5451: drive = self._lookupDeviceByPath(path) Line 5452: try: Line 5453: jobID = self.getBlockJob(drive)['jobID'] Line 5454: except LookupError: -- To view, visit http://gerrit.ovirt.org/28597 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1a5b11e7da185a699028b6127066cd01de010a0d Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
