Nir Soffer has posted comments on this change. Change subject: vm: Update LUN size when starting a vm ......................................................................
Patch Set 1: Code-Review-1 (4 comments) Some issue to fix in the implementation. http://gerrit.ovirt.org/#/c/24235/1/vdsm/clientIF.py File vdsm/clientIF.py: Line 259: if not res["visible"][drive["GUID"]]: Line 260: raise vm.VolumeError(drive) Line 261: Line 262: res = self.irs.appropriateDevice(drive["GUID"], vmId) Line 263: if res['status']['code']: This will raise KeyError now, since you are not returning standard status reply with zero (falsy) status code. Line 264: raise vm.VolumeError(drive) Line 265: Line 266: # Update size for LUN volume Line 267: drive["truesize"] = res['truesize'] http://gerrit.ovirt.org/#/c/24235/1/vdsm/storage/hsm.py File vdsm/storage/hsm.py: Line 3106: utils.retry(partial(fileUtils.validateQemuReadable, devPath), Line 3107: expectedException=OSError, Line 3108: timeout=QEMU_READABLE_TIMEOUT) Line 3109: Line 3110: # Get the size of the logical unit volume. This comment does not add any information or value. The code is explaining itself. The only comment needed is why we convert the size to a string. The reason is that xmlrpc cannot handle big integers, and our xmlrpc code is not smart enough to do the conversion when needed. Since this is an internal API, and this code is never called by the xmlrpc server, there is no need to convert the value to string here. But since we need to use the standard xmlrpc return value (see next comment), it may be better to keep it consistent with other methods and keep the str conversion. Line 3111: size = str(multipath.getDeviceSize(devicemapper.getDmId(guid))) Line 3112: Line 3113: return dict(truesize=size, apparentsize=size) Line 3114: Line 3109: Line 3110: # Get the size of the logical unit volume. Line 3111: size = str(multipath.getDeviceSize(devicemapper.getDmId(guid))) Line 3112: Line 3113: return dict(truesize=size, apparentsize=size) The code calling this expect a standard xmlrpc reply: {"status": {"code": some code, "message": error message...}} If this fails in line 3106 (utils.retry), the @public wrapper will generate an error response for you, so it seems that the best approach is to use the same format to return positive result. Check how other methods return data *and* status. Line 3114: Line 3115: @public Line 3116: def inappropriateDevices(self, thiefId): Line 3117: """ http://gerrit.ovirt.org/#/c/24235/1/vdsm/vm.py File vdsm/vm.py: Line 630: dStats = {'truesize': str(vmDrive.truesize), Line 631: 'apparentsize': str(vmDrive.apparentsize)} Line 632: if isVdsmImage(vmDrive): Line 633: dStats['imageID'] = vmDrive.imageID Line 634: else: Is this always correct? all devices are either vdsmimage or have a GUID? Looking at clientIF.prepareVolumePath, this seems to be false. Line 635: dStats['lunGUID'] = vmDrive.GUID Line 636: dStats['readRate'] = ((eInfo[dName][1] - sInfo[dName][1]) / Line 637: sampleInterval) Line 638: dStats['writeRate'] = ((eInfo[dName][3] - sInfo[dName][3]) / -- To view, visit http://gerrit.ovirt.org/24235 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia4988212f7f96078e774d2a4e7b5cd1681383cb0 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Daniel Erez <[email protected]> Gerrit-Reviewer: Ayal Baron <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Nir Soffer <[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
