Change in vdsm[master]: gluster: Add gluster volume geo-replication configuration fe...

barumuga at redhat.com barumuga at redhat.com
Wed Aug 21 10:40:52 UTC 2013


Bala.FA has posted comments on this change.

Change subject: gluster: Add gluster volume geo-replication configuration feature
......................................................................


Patch Set 1: Code-Review-1

(7 comments)

....................................................
Commit Message
Line 18:             'description': DESCRIPTION}... }
Line 19:  * glusterVolumeGeoRepConfigGet
Line 20:     - return value structure:
Line 21:        {NAME: VALUE,
Line 22: 	     writable: BOOL,
Please fix whitespace
Line 23:             'description': DESCRIPTION}
Line 24:  * glusterVolumeGeoRepConfigSet
Line 25:  * glusterVolumeGeoRepConfigDelete
Line 26: 


....................................................
File client/vdsClientGluster.py
Line 858:               'get volume geo-replication configuration'
Line 859:               )),
Line 860:          'glusterVolumeGeoRepConfigDelete': (
Line 861:              serv.do_glusterVolumeGeoRepConfigDelete,
Line 862:              ('masterVolName=<master_volume_name> slaveHost=<slave_host> '
Use volumeName, remoteHost, remoteVolume instead of masterVolName, slaveHost, slaveVolName everywhere
Line 863:               'slaveVolName=<slave_volume_name> keyName=<key> '
Line 864:               'value=<value>\n\t'
Line 865:               '<master_volume_name> is an existing volume name in '
Line 866:               'the master node\n\t'


....................................................
File vdsm/gluster/api.py
Line 319:                                                               slaveHost,
Line 320:                                                               slaveVolName,
Line 321:                                                               key,
Line 322:                                                               value)
Line 323:         return {'geoRepSet': status}
This is not matter of boolean?
Line 324: 
Line 325:     @exportAsVerb
Line 326:     def volumeGeoRepConfigGet(self, masterVolName, slaveHost, slaveVolName,
Line 327:                               key, options=None):


Line 327:                               key, options=None):
Line 328: 
Line 329:         fp = open("/tmp/file1.txt", "w")
Line 330:         fp.write("entering geo rep config get")
Line 331:         fp.close()
remove this debugging code
Line 332:         status = self.svdsmProxy.glusterVolumeGeoRepConfigGet(masterVolName,
Line 333:                                                               slaveHost,
Line 334:                                                               slaveVolName,
Line 335:                                                               key)


Line 342:         status = self.svdsmPorxy.glusterVolumeGeoRepConfigDelete(masterVolName,
Line 343:                                                                  slaveHost,
Line 344:                                                                  slaveVolName,
Line 345:                                                                  key)
Line 346:         return {'geoRepDelete': status}
return is not a boolean?
Line 347: 
Line 348: 
Line 349: def getGlusterMethods(gluster):
Line 350:     l = []


....................................................
File vdsm/gluster/cli.py
Line 159:     'working_dir': (
Line 160:         'workingDir',
Line 161:         'working directory',
Line 162:         GeoRepConf.READONLY)}
Line 163: 
I am not able to understand why you maintain config list.  What if this list grows/shrinks depending on glusterfs version?  What if some config turns into read-only or writable?

I think this should be outside of vdsm
Line 164: 
Line 165: def _execGluster(cmd):
Line 166:     return utils.execCmd(cmd)
Line 167: 


Line 1104:                   'editable': GEOREP_CONFIG[key][2]}}
Line 1105: 
Line 1106: 
Line 1107: @makePublic
Line 1108: def volumeGeoRepConfigDelete(masterVolName, slaveHost, slaveVolName, key):
is it deleting the conf or resetting the conf?
Line 1109:     command = _getGlusterVolCmd() + ["geo-replication", masterVolName,
Line 1110:                                      "%s::%s" % (slaveHost, slaveVolName),
Line 1111:                                      "config", "!%s" % key]
Line 1112:     rc, out, err = _execGluster(command)


-- 
To view, visit http://gerrit.ovirt.org/18303
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9c43f0950bbaa215cfe22ba18cc02e5c5851c347
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Timothy Asir <tjeyasin at redhat.com>
Gerrit-Reviewer: Aravinda VK <avishwan at redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron at redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: ndarshan <dnarayan at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list