Nir Soffer has posted comments on this change. Change subject: gluster: Set mount path based on gluster volume info ......................................................................
Patch Set 2: (3 comments) https://gerrit.ovirt.org/#/c/48308/2/tests/storageServerTests.py File tests/storageServerTests.py: Line 148: self.assertEquals(gluster.options, Line 149: "backup-volfile-servers=192.168.122.2:192.168.122.3") Line 150: Line 151: @MonkeyPatch(storageServer, 'supervdsm', FakeSupervdsm()) Line 152: def test_gluster_mount_options_when_gluster_provided_server_not_in_volinfo(self): This name his way too long - this break the test report visually. The word gluster is redundant, this is the GlusterFSConnectionTests. (Should be fixed also in other tests later). Use shorter name like "test_unknown_server" or "test_server_not_in_volinfo" If needed add a comment giving more context; test names are not short stories. Line 153: def glusterVolumeInfo(volname=None, volfileServer=None): Line 154: return {'music': {'brickCount': '3', Line 155: 'bricks': ['192.168.122.5:/tmp/music', Line 156: '192.168.122.2:/tmp/music', https://gerrit.ovirt.org/#/c/48308/2/vdsm/storage/storageServer.py File vdsm/storage/storageServer.py: Line 310 Line 311 Line 312 Line 313 Line 314 Keep the old code for now. Line 337: raise se.UnsupportedGlusterVolumeReplicaCountError(replicaCount) Line 338: Line 339: def _get_backup_servers_option(self): Line 340: servers = [brick.split(":")[0] for brick in self.volinfo['bricks']] Line 341: self._volfileserver = servers.pop(0) There is a major issue with this change. If a user has an old mount, it will use the user server:/volname. Using new semantics, we try to mount the same volume under possibly different mountpoint, mounting the same domain twice, and possibly causing data corruption or other severe trouble. So we cannot use bricks[0] as the server. We can do this: if self._volfileserver in servers: server.remove(self._volfileserver) else: self.log.warning("gluster server 'foo' not in bricks, using possibly duplicate backup server ['bar', 'baz', 'dup']") So we always connect the gluster server that the user configured, and we keep compatibility with older versions. If the user have two names for the one of the servers, she will have 3 backup servers - hopefully this works with gluster, if not, the user must fix the configuration. Sahina, what do you think? Line 342: if not servers: Line 343: return "" Line 344: Line 345: return "backup-volfile-servers=" + ":".join(servers) -- To view, visit https://gerrit.ovirt.org/48308 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id3386b37cd407c52e1b8f38d54c236bffc143e2f Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Ala Hino <ah...@redhat.com> Gerrit-Reviewer: Ala Hino <ah...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <nsof...@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
