Nir Soffer has posted comments on this change. Change subject: vdsm: update size of LUN volume in prepareVolume ......................................................................
Patch Set 3: (5 comments) Can be done with much less work from vm/clientIF. .................................................... Commit Message Line 3: AuthorDate: 2014-01-02 22:25:53 +0200 Line 4: Commit: Daniel Erez <[email protected]> Line 5: CommitDate: 2014-01-08 13:15:01 +0200 Line 6: Line 7: vdsm: update size of LUN volume in prepareVolume Everything here is "vdsm:" use more specific prefix. Line 8: Line 9: In order to handle resize of DirectLUN disks Line 10: (i.e. when a LUN volume is extended by user), Line 11: relevant data should be retrieved when invoking getVmStats. .................................................... File vdsm/clientIF.py Line 260: if res['status']['code']: Line 261: raise vm.VolumeError(drive) Line 262: Line 263: # Update size for LUN volume Line 264: volSize = self.irs.getDeviceSize(volPath) Why not use multipath here to check the device size directly? Line 265: drive["truesize"] = volSize['truesize'] Line 266: drive["apparentsize"] = volSize['apparentsize'] Line 267: Line 268: # UUID drive format .................................................... File vdsm/storage/hsm.py Line 2982: return sdCache.produce( Line 2983: sdUUID=sdUUID).produceVolume(imgUUID=imgUUID, Line 2984: volUUID=volUUID).refreshVolume() Line 2985: Line 2986: @public You added an undocumented public api. If you do want to add it, you will have to add it to API.py, vdsmapi-schema.json, xmlrpc binding and maybe also jsonrpc bindings. Line 2987: def getDeviceSize(self, volPath, options=None): Line 2988: """ Line 2989: Gets the size of a LUN device volume. Line 2990: Line 2983: sdUUID=sdUUID).produceVolume(imgUUID=imgUUID, Line 2984: volUUID=volUUID).refreshVolume() Line 2985: Line 2986: @public Line 2987: def getDeviceSize(self, volPath, options=None): I agree with Federico that you should do it from vm/clientIF - just import multipath there and use it. Line 2988: """ Line 2989: Gets the size of a LUN device volume. Line 2990: Line 2991: :param volPath: The LUN device volume path. Line 2998: size = str(multipath.getDeviceSize(devicemapper.getDmId(volPath))) Line 2999: except IOError: Line 3000: self.log.warn("Could not get size of LUN device in volPath %s", Line 3001: volPath, exc_info=True) Line 3002: raise First there is no point in logging a warning and then raising. If you handle this here, don't raise. If you cannot handle it, here, let the caller handle it. Second, when you may want to handle ENOENT error, because you don't want to fail the entire operation because a lun was missing, and add pointless exceptions to the logs. For example: try: get the size... except IOError as e: if e.errno != errno.ENOENT: raise # Cannot handle this self.log.warn("No such device: %s", path) set sizes to 0... In this context - not handling missing device is probably more correct, but if you use this check in clientIF, you probably want to handle this case. Line 3003: Line 3004: return dict(truesize=size, apparentsize=size) Line 3005: Line 3006: @public -- To view, visit http://gerrit.ovirt.org/22976 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I48b4343c6519fed366beec415c47226d4e3c8fef Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Daniel Erez <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Ayal Baron <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Eduardo <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[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
