Deepak C Shetty has posted comments on this change.
Change subject: Support for GLUSTERFS_DOMAIN ......................................................................
Patch Set 12: (7 inline comments)
@fsimonce, addressed your comments and posted some replies. Thanks for the comprehensive review. Pls have a look. Reg. the comment on the arch... is there a better way to go ?
.................................................... File vdsm/storage/glusterSD.py Line 18: return glusterVolume.GlusterVolume Line 19: Line 20: @staticmethod Line 21: def findDomainPath(sdUUID): Line 22: for tmpSdUUID, domainPath in fileSD.scanDomains(sd.GLUSTERSD_DIR + Done Line 23: "/*"): Line 24: if tmpSdUUID == sdUUID and mount.isMounted(os.path.join(domainPath, Line 25: "..")): Line 26: return domainPath
.................................................... File vdsm/storage/glusterVolume.py Line 1: from volume import VmVolumeInfo Line 2: import fileVolume Line 3: from sdc import sdCache Line 4: import string Done Line 5: import supervdsm as svdsm Line 6: Line 7: Line 8: class GlusterVolume(fileVolume.FileVolume):
Line 15: """ Line 16: Send info to represent Gluster volume as a network block device Line 17: """ Line 18: rpath = sdCache.produce(self.sdUUID).getRemotePath() Line 19: rpath_list = string.rsplit(rpath, ":") Done Line 20: volfileServer = rpath_list[0] Line 21: volname = rpath_list[1] Line 22: Line 23: # Volume transport to Libvirt transport mapping
Line 20: volfileServer = rpath_list[0] Line 21: volname = rpath_list[1] Line 22: Line 23: # Volume transport to Libvirt transport mapping Line 24: VOLUME_TRANS_MAP = {'TCP': 'tcp', 'RDMA': 'rdma'} Right now, only this function needs it, so i kept it inside the func. If more functions are added and they need it, then we can move it at the module or class level as needed. I think for now its fine. Hope you agree ? Line 25: Line 26: # Extract the volume's transport using gluster cli Line 27: svdsmProxy = svdsm.getProxy() Line 28: volInfo = svdsmProxy.glusterVolumeInfo(volname)
Line 31: # Use default port Line 32: volPort = "0" Line 33: Line 34: imgFilePath = self.getVolumePath() Line 35: imgFilePath_list = string.rsplit(imgFilePath, "/") Done Line 36: imgFileRelPath = string.join(imgFilePath_list[4:], "/") Line 37: Line 38: glusterPath = volname + '/' + imgFileRelPath Line 39:
Line 32: volPort = "0" Line 33: Line 34: imgFilePath = self.getVolumePath() Line 35: imgFilePath_list = string.rsplit(imgFilePath, "/") Line 36: imgFileRelPath = string.join(imgFilePath_list[4:], "/") Changed this too to avoid using string module. Line 37: Line 38: glusterPath = volname + '/' + imgFileRelPath Line 39: Line 40: return {'volType': VmVolumeInfo.TYPE_NETWORK, 'path': glusterPath,
.................................................... File vdsm/storage/volume.py Line 868: volume to the VM in a different way than the standard 'path' way. Line 869: """ Line 870: # By default, send path Line 871: return {'volType': VmVolumeInfo.TYPE_PATH, Line 872: 'path': self.getVolumePath()} I don't think so. It seems to be the case, if you look at it from volume.py's perspective. But if u look at it from glusterVolume.py perspective, path returned from getVolumePath and ['path'] returned from getVmVolumeInfo are different. The former path is used for the domain side of VDSM, while the latter is used to tell qemu how to access the gluster image using a gluster specific path ( not the domain path). Till now, VDSM libvirtvm didn't have support for network based device support, so path was same ( domain and qemu side). But with gluster, they are diff. In fact the ['path'] returned from prepareImage should be more aptly called volDomPath and ['path'] returned from getVmVolumeInfo is for VM purpose only.
In other words, if any derived class overrides getVmVolumeInfo and sends a diff ['path'] which is what glusterVolume.py does, then the volume.py's self.volumePath is not accessible, post return from prepareImage.
getVolumePath is used to represent the volume's path from a domain point of view. getVmVolumeInfo['path'] is used to represent the volume's path from a VM (libvirtvm) perspective.
Let me know if you still think its redundant ? Line 873: Line 874: def getMetaParam(self, key): Line 875: """ Line 876: Get a value of a specific key
-- To view, visit http://gerrit.ovirt.org/6856 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I9ac37da88625f20d148beaf53bb6371c15b33ad7 Gerrit-PatchSet: 12 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Deepak C Shetty deepakcs@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Bala.FA barumuga@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Deepak C Shetty deepakcs@linux.vnet.ibm.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Itamar Heim iheim@redhat.com Gerrit-Reviewer: Royce Lv lvroyce@linux.vnet.ibm.com Gerrit-Reviewer: Ryan Harper ryanh@us.ibm.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server