Change in vdsm[master]: vdsm: Gluster UFO verbs

barumuga at redhat.com barumuga at redhat.com
Tue Jan 15 04:03:38 UTC 2013


Bala.FA has posted comments on this change.

Change subject: vdsm: Gluster UFO verbs
......................................................................


Patch Set 4: I would prefer that you didn't submit this

(9 inline comments)

Partial review

....................................................
Commit Message
Line 12: glusterGetServices: To get the status of all UFO services
Line 13: (glusterd, smb, memcached, swift)
Line 14: 
Line 15: glusterManageServices: To start/stop/restart UFO related
Line 16: services(glusterd, smb, memcached, swift)
These verbs are not UFO only and generic, I like to have these verbs as separate patch
Line 17: 
Line 18: glusterGetSwiftConfig: Get swift config items
Line 19: 
Line 20: glusterSetSwiftConfig: Set the config items


Line 17: 
Line 18: glusterGetSwiftConfig: Get swift config items
Line 19: 
Line 20: glusterSetSwiftConfig: Set the config items
Line 21: 
Please change verb names like glusterServicesGet for uniformity with existing verbs.  This is applicable to all related function/method names
Line 22: Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=850443
Line 23: 
Line 24: Change-Id: Ie966fb515275a0768f67cbbe2055a07002355327


Line 18: glusterGetSwiftConfig: Get swift config items
Line 19: 
Line 20: glusterSetSwiftConfig: Set the config items
Line 21: 
Line 22: Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=850443
Please remove this bug-url which is not for ovirt
Line 23: 
Line 24: Change-Id: Ie966fb515275a0768f67cbbe2055a07002355327


Line 19: 
Line 20: glusterSetSwiftConfig: Set the config items
Line 21: 
Line 22: Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=850443
Line 23: 
Add output structure of all getter verbs in commit message, docstring in functions/methods
Line 24: Change-Id: Ie966fb515275a0768f67cbbe2055a07002355327


....................................................
File vdsm/gluster/cli.py
Line 62: def _getGlusterServiceCmd(serviceName, action):
Line 63:     if serviceName == "swift":
Line 64:         return ["/usr/bin/swift-init", "main", action]
Line 65:     else:
Line 66:         return ["/sbin/service", serviceName, action]
1. use constants, or utils.CommandPath instead of hard-coded value for /sbin/service etc

2. its better to have RHEL/Fedora specifc code for sysvinit, upstart and systemd
Line 67: 
Line 68: 
Line 69: def exportToSuperVdsm(func):
Line 70:     @wraps(func)


Line 1010:     :param servicesDict dict with serviceName as key and action as value
Line 1011:     Ex: {"glusterd": "start", "swift": "stop"}
Line 1012:     :returns {"value": True}
Line 1013:     """
Line 1014:     for serviceName in serviceDict:
Not sure about performing bulk operations.  Manage one service and engine could call for other services repeatedly if needed
Line 1015:         action = serviceDict[serviceName]
Line 1016: 
Line 1017:         # Allowing to start any service can have side effects
Line 1018:         if serviceName not in SUPPORTED_GLUSTER_SERVICES:


Line 1035: 
Line 1036:     :param None
Line 1037:     :returns list of dict with PID and status details for each service
Line 1038:     Ex: {"value": [{"name": "glusterd", "status": "RUNNING", pid: "1027"},..]}
Line 1039:     """
above comment apply here too.  all=False can be an option to getServices()?
Line 1040:     serviceStatus = []
Line 1041: 
Line 1042:     for serviceName in SUPPORTED_GLUSTER_SERVICES:
Line 1043:         rc, out, err = _execGluster(_getGlusterServiceCmd(serviceName,


Line 1068:     try:
Line 1069:         config.readfp(open(config_file))
Line 1070:     except IOError as e:
Line 1071:         raise ge.GetGlusterSwiftConfigFailedException(e.errno,
Line 1072:                                                       [""],
No need to send empties, instead err=[e.strerror] is clean
Line 1073:                                                       [e.strerror])
Line 1074:     configValues = {}
Line 1075: 
Line 1076:     # Since config.items or config.get for any section will include default


....................................................
File vdsm/gluster/exception.py
Line 413: 
Line 414: 
Line 415: class GetGlusterSwiftConfigFailedException(GlusterException):
Line 416:     code = 4412
Line 417:     message = "Get Gluster Swift config failed"
I feel these exceptions are gluster generic, not gluster host command specific


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie966fb515275a0768f67cbbe2055a07002355327
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Aravinda VK <avishwan at redhat.com>
Gerrit-Reviewer: Aravinda VK <avishwan at redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga at redhat.com>
Gerrit-Reviewer: Shireesh Anjal <sanjal at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server


More information about the vdsm-patches mailing list