Ala Hino has posted comments on this change. Change subject: gluster: Allow gluster mount with additional servers ......................................................................
Patch Set 4: (8 comments) https://gerrit.ovirt.org/#/c/40665/4/vdsm/storage/storageServer.py File vdsm/storage/storageServer.py: Line 31 Line 32 Line 33 Line 34 Line 35 > Add blank line before the supervdsm import. It belongs to the next group, n Done Line 251: DIR = "glusterSD" Line 252: Line 253: def connect(self): Line 254: glusterMountOptions = self._getGlusterMountOptions() Line 255: self._options += glusterMountOptions > This code means that each time you call connect, gluster mount options are Done Line 256: super(GlusterFSConnection, self).connect() Line 257: Line 258: def _getGlusterMountOptions(self): Line 259: remotePath = self._remotePath.split(":/") Line 255: self._options += glusterMountOptions Line 256: super(GlusterFSConnection, self).connect() Line 257: Line 258: def _getGlusterMountOptions(self): Line 259: remotePath = self._remotePath.split(":/") > Are you sure that all volumes are using ":/"? Plese see the discussion in h Done according to the discussion. Line 260: glusterServer = remotePath[0] Line 261: volume = remotePath[1] Line 262: volInfo = supervdsm.getProxy().glusterVolumeInfo(volume, Line 263: glusterServer) Line 257: Line 258: def _getGlusterMountOptions(self): Line 259: remotePath = self._remotePath.split(":/") Line 260: glusterServer = remotePath[0] Line 261: volume = remotePath[1] > The Python way to do this split is to use tuple unpacking: Done Line 262: volInfo = supervdsm.getProxy().glusterVolumeInfo(volume, Line 263: glusterServer) Line 264: servers = self._getGlusterServers(volume, volInfo) Line 265: servers.remove(glusterServer) Line 262: volInfo = supervdsm.getProxy().glusterVolumeInfo(volume, Line 263: glusterServer) Line 264: servers = self._getGlusterServers(volume, volInfo) Line 265: servers.remove(glusterServer) Line 266: return "backup-volfile-servers=%s:%s" % (servers[0], servers[1]) > This assume replica 3 - we want to support any replica 1, 3 and maybe other Done Line 267: Line 268: def _getGlusterServers(self, volume, volInfo): Line 269: servers = [] Line 270: bricks = volInfo[volume]['bricks'] Line 264: servers = self._getGlusterServers(volume, volInfo) Line 265: servers.remove(glusterServer) Line 266: return "backup-volfile-servers=%s:%s" % (servers[0], servers[1]) Line 267: Line 268: def _getGlusterServers(self, volume, volInfo): > The parameter volume is not very clear - is this a volume object or volume Done Line 269: servers = [] Line 270: bricks = volInfo[volume]['bricks'] Line 271: for brick in bricks: Line 272: server = brick.split(":/")[0] Line 268: def _getGlusterServers(self, volume, volInfo): Line 269: servers = [] Line 270: bricks = volInfo[volume]['bricks'] Line 271: for brick in bricks: Line 272: server = brick.split(":/")[0] > Check also here that volumes and servers are always separated by ":/" - sin Done Line 273: servers.append(server) Line 274: Line 275: return servers Line 276: Line 271: for brick in bricks: Line 272: server = brick.split(":/")[0] Line 273: servers.append(server) Line 274: Line 275: return servers > The pythonic way to do this is: Done Line 276: Line 277: Line 278: class NFSConnection(object): Line 279: DEFAULT_OPTIONS = ["soft", "nosharecache"] -- To view, visit https://gerrit.ovirt.org/40665 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2478a5edc1fc9d24eb96d64a32a98a2467ce2989 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Ala Hino <ah...@redhat.com> Gerrit-Reviewer: Ala Hino <ah...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Sandro Bonazzola <sbona...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
