Ala Hino has posted comments on this change. Change subject: gluster: Allow gluster mount with additional servers ......................................................................
Patch Set 9: (9 comments) https://gerrit.ovirt.org/#/c/40665/9/tests/glusterStorageServerTests.py File tests/glusterStorageServerTests.py: > Please mote the tests to storageServerTests.py. The rule is one test module Done Line 1: from monkeypatch import MonkeyPatch, MonkeyPatchScope Line 2: from testlib import VdsmTestCase Line 3: from storage.storageServer import GlusterFSConnection Line 4: from storage import storageServer 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 cla Done 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 Done 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 ret Done 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. Woul Done 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 Done 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 refactori Done 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 Done 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. Added warn message indicating that user specified mount options will be used. No need to log user setting as they appear in the mount cmd. 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 <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
