Change in vdsm[master]: gluster: Allow gluster mount with additional servers

nsoffer at redhat.com nsoffer at redhat.com
Tue Jun 2 22:21:36 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: gluster: Allow gluster mount with additional servers
......................................................................


Patch Set 4:

(10 comments)

https://gerrit.ovirt.org/#/c/40665/4/tests/glusterStorageServerTests.py
File tests/glusterStorageServerTests.py:

Line 41:                    'volumeType': 'REPLICATE'}}
Line 42: 
Line 43:     @MonkeyPatch(storageServer, 'supervdsm', Supervdsm())
Line 44:     def testGetVolumeInfo(self):
Line 45:         def glusterVolumeInfo(volumeName=None, remoteServer=None):
It would be nice to assert that this volumeName and remoteServer have the correct value.
Line 46:             return self.volumeInfo
Line 47: 
Line 48:         storageServer.supervdsm.glusterVolumeInfo = glusterVolumeInfo
Line 49: 


Line 47: 
Line 48:         storageServer.supervdsm.glusterVolumeInfo = glusterVolumeInfo
Line 49: 
Line 50:         gluster = GlusterFSConnection("192.168.122.1:/music", "")
Line 51:         self.assertEquals(gluster._getGlusterMountOptions(),
Lets not check private functions. Instead, mock the mount command and check that connect() is sending the correct options.


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, not to vdsm group.


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 appended to self._options.

You want to check the options and add them to self._options only once.

This can be done by initializing in __init__, but we don't want to do stuff in __init__, this leads to bad things and objects were are hard to test.

So we can do this using lazy initialization:

    @property
    def options(self):
        if self._gluster_options is None:
            self._gluster_options = self._get_gluster_mount_options()
        return self._options + self._gluster_options

MountConnection should access self.options instead of self._options.

Since you are using a property, this is now a readonly value, and other code cannot change it.

Having readonly public property, annoying reviewers will not complain abut testing private methods ;-)
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 https://gerrit.ovirt.org/36237 about this.
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:

    server, volume = remotePath.split(":/", 1)

Note that I'm spliting once, in case volume name contains also ":/" - maybe this is impossible, but there is no reason to split more then once anyway.
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.

The correct way would be:

    return "backup-volfile-servers=" + ":".join(servers)

We also want to reject replica 2, but lets do this in another patch.
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 name? better use more descriptive name.
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 ":/" - since we could care less about the volume name in this code, why not split by ":"?
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:

    return [brick.split(":/")[0]
            for brick in volInfo[volume]['bricks']]

And we don't need a function for this, you can simply inline this in the caller:

    servers = [brick.split(":/")[0]
               for brick in volInfo[volume]['bricks']]
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 <ahino at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Sandro Bonazzola <sbonazzo at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list