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

nsoffer at redhat.com nsoffer at redhat.com
Mon Jun 8 21:36:14 UTC 2015


Nir Soffer has posted comments on this change.

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


Patch Set 9:

(8 comments)

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

Line 5: 
Line 6: import supervdsm
Line 7: 
Line 8: 
Line 9: class Supervdsm(object):
Rename to FakeSupervdsm - make it more clear what is this strange empty class.
Line 10: 
Line 11:     def getProxy(self):
Line 12:         return self
Line 13: 


Line 13: 
Line 14: 
Line 15: class GlusterFSConnectionTests(VdsmTestCase):
Line 16: 
Line 17:     MOUNT_OPTIONS = "backup-volfile-servers=192.168.122.2:192.168.122.3"
To prevent confusion with the second test, rename this to "computed_options", and move it into the test.
Line 18: 
Line 19:     VOLUME_INFO = \
Line 20:         {'music': {'brickCount': '3',
Line 21:                    'bricks': ['192.168.122.1:/tmp/music',


Line 15: class GlusterFSConnectionTests(VdsmTestCase):
Line 16: 
Line 17:     MOUNT_OPTIONS = "backup-volfile-servers=192.168.122.2:192.168.122.3"
Line 18: 
Line 19:     VOLUME_INFO = \
No need for a class constant, since this is used by only one test. Just return this from the fake getVolumeInfo function in the test. Unless you plan to add more tests using same response, In which case, getVolumeInfo can be a function in the module.

    def getVolumeInfo(volname, remoteserver):
        return {
            ...
        }

    class Test(...):

        @MonkeyPatch(storageServer, 'supervdsm', Supervdsm())
        def test_foo(...):
            storageServer.supervdsm.glusterVolumeInfo = glusterVolumeInfo
            ...

       @MonkeyPatch(storageServer, 'supervdsm', Supervdsm())
       def test_bar(...):
            storageServer.supervdsm.glusterVolumeInfo = glusterVolumeInfo
            ...
Line 20:         {'music': {'brickCount': '3',
Line 21:                    'bricks': ['192.168.122.1:/tmp/music',
Line 22:                               '192.168.122.2:/tmp/music',
Line 23:                               '192.168.122.3:/tmp/music'],


Line 44:                    'volumeType': 'REPLICATE'}}
Line 45: 
Line 46:     @MonkeyPatch(storageServer, 'supervdsm', Supervdsm())
Line 47:     def testGetVolumeInfo(self):
Line 48:         def glusterVolumeInfo(volumeName=None, remoteServer=None):
Please assert that this function is called with the correct arguments. Would be nice to test this in a separate test.
Line 49:             return self.VOLUME_INFO
Line 50: 
Line 51:         storageServer.supervdsm.glusterVolumeInfo = glusterVolumeInfo
Line 52: 


Line 53:         gluster = GlusterFSConnection(spec="192.168.122.1:/music")
Line 54:         self.assertEquals(gluster.options, self.MOUNT_OPTIONS)
Line 55: 
Line 56:     def testProvidedGlusterVolumeInfo(self):
Line 57:         MOUNT_OPTIONS = "backup-volfile-servers=server1:server2"
This name is confusing - use "user_options" instead. No need to make this a constant in a context of a test method. If python had nice way to do this (e.g. const, final), why not, but having random CONSTANTS in the source make it harder to read.
Line 58:         gluster = GlusterFSConnection(spec="192.168.122.1:/music",
Line 59:                                       options=MOUNT_OPTIONS)


https://gerrit.ovirt.org/#/c/40665/9/vdsm/storage/storageServer.py
File vdsm/storage/storageServer.py:

Line 192:     @classmethod
Line 193:     def getLocalPathBase(cls):
Line 194:         return cls.localPathBase
Line 195: 
Line 196:     def __init__(self, spec, vfsType=None, options="", mountClass=mount.Mount):
This change is not needed for this patch, and this patch is not a refactoring. Please separate this to another patch, possible with the new FakeMount class and connect tests.
Line 197:         self._vfsType = vfsType
Line 198:         self._remotePath = spec
Line 199:         self._options = options
Line 200:         self._mount = mountClass(spec, self._getLocalPath())


Line 261:     @property
Line 262:     def options(self):
Line 263:         if self._gluster_options is None:
Line 264:             self._gluster_options = self._get_gluster_mount_options()
Line 265:         return self._options + self._gluster_options
Please put @properties after __init__. __init__ is the only place were you can understand what is the instance variables of this class. We want to have __init__ always first after class constants and variables.

Do not follow the bad examples in this file (MountConnection).
Line 266: 
Line 267:     def __init__(self, spec, vfsType=None, options="", mountClass=mount.Mount):
Line 268:         super(GlusterFSConnection, self).__init__(spec,
Line 269:                                                   vfsType,


Line 272:         self._gluster_options = None
Line 273: 
Line 274:     def _get_gluster_mount_options(self):
Line 275:         if "backup-volfile-servers" in self._options:
Line 276:             return ""
This means that we let the user override the optimal settings.

I don't think we want to support this. If we do, we still need to check gluster options, and warn if the settings the user chose are not optimal, or fail if they are invalid.

- No-optimal settings: include only 1 server in backup-volfile-servers
- Invalid settings: include non-existing brick in backup-volfile-servers.

The minimal change is to log the user preferred settings and warn that we are using user specified settings.

Please check how we behave when a user specify NFS or posix options that we also handle - I think we either fail the mount, or the mount will fail since you specified multiple options.
Line 277: 
Line 278:         volfileServer, volname = self._remotePath.split(":", 1)
Line 279:         volname = volname.strip('/')
Line 280:         volInfo = supervdsm.getProxy().glusterVolumeInfo(volname,


-- 
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: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino at redhat.com>
Gerrit-Reviewer: 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