Change in vdsm[master]: gluster: verbs for managing services

danken at redhat.com danken at redhat.com
Thu Jun 27 11:34:38 UTC 2013


Dan Kenigsberg has posted comments on this change.

Change subject: gluster: verbs for managing services
......................................................................


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

(10 inline comments)

I am sorry that it took me so much time to review this patch, but I do not think that I'm happy with the proposed API.

....................................................
File vdsm/gluster/Makefile.am
Line 27: 	cli.py \
Line 28: 	exception.py \
Line 29: 	hooks.py \
Line 30: 	hostname.py \
Line 31: 	services.py
next time, please add the $(NULL) trick.
Line 32: 
Line 33: dist_vdsmgluster_DATA = \
Line 34: 	vdsmapi-gluster-schema.json \


....................................................
File vdsm/gluster/services.py
Line 22: from vdsm.tool import service
Line 23: from . import makePublic
Line 24: 
Line 25: 
Line 26: ALLOWED_SERVICES = ["glusterd",
This should better be an immutable tuple. Actually, a frozenset() is even better, if the order does not matter.
Line 27:                     "memcached",
Line 28:                     "gluster-swift-proxy",
Line 29:                     "gluster-swift-container",
Line 30:                     "gluster-swift-object",


Line 56: def _formatStatus(serviceName, status, message=''):
Line 57:     return {'name': serviceName, 'status': status, 'message': message}
Line 58: 
Line 59: 
Line 60: def _serviceStatus(serviceName, failOnError=False):
arg failOnError unused - better drop it.
Line 61:     rc = service.service_status(serviceName)
Line 62: 
Line 63:     if rc == 0:
Line 64:         return _formatStatus(serviceName, StatusTypes.RUNNING)


Line 69:     rc1 = service.service_is_managed(serviceName)
Line 70:     if rc1 == 0:
Line 71:         return _formatStatus(serviceName, StatusTypes.STOPPED)
Line 72:     else:
Line 73:         return _formatStatus(serviceName, StatusTypes.NOT_AVAILABLE)
Are "not managed" and "not available" the same thing? If so, use one term.
Line 74: 
Line 75: 
Line 76: def _serviceAction(serviceName, action, failOnError=False):
Line 77:     if action == ServiceActions.STATUS:


Line 109:         else:
Line 110:             if not failOnError:
Line 111:                 resp = _formatStatus(serviceName, StatusTypes.NOT_SUPPORTED)
Line 112:             else:
Line 113:                 raise ge.GlusterServiceNotSupportedException(sName=serviceName)
Is there a difference between "allowed" and "supported"? If not, please use one term (I prefer "supported") both in the exception name and the variable ALLOWED_SERVICES.
Line 114: 
Line 115:         statusOutput.append(resp)
Line 116: 
Line 117:     return statusOutput


Line 117:     return statusOutput
Line 118: 
Line 119: 
Line 120: @makePublic
Line 121: def servicesManage(serviceNames, action, failOnError=False):
"manage" is a very vague term. I believe that "Act" would be clearer (but I am not sure).
Line 122:     """
Line 123:     {'services': [
Line 124:         {'name': SERVICE_NAME, 'status': STATUS, 'message': MESSAGE},..]}
Line 125:     """


Line 119: 
Line 120: @makePublic
Line 121: def servicesManage(serviceNames, action, failOnError=False):
Line 122:     """
Line 123:     {'services': [
please add the word "Return" before the example dict.
Line 124:         {'name': SERVICE_NAME, 'status': STATUS, 'message': MESSAGE},..]}
Line 125:     """
Line 126:     action = action.lower()
Line 127:     return _action(serviceNames, action, failOnError)


Line 122:     """
Line 123:     {'services': [
Line 124:         {'name': SERVICE_NAME, 'status': STATUS, 'message': MESSAGE},..]}
Line 125:     """
Line 126:     action = action.lower()
why would we want to be case-insensitive? It's not a human interface (and human interfaces too are better when they are case-sensitive).
Line 127:     return _action(serviceNames, action, failOnError)
Line 128: 
Line 129: 
Line 130: @makePublic


Line 127:     return _action(serviceNames, action, failOnError)
Line 128: 
Line 129: 
Line 130: @makePublic
Line 131: def servicesGet(serviceNames, failOnError=False):
Could you explain (in the commit message, maybe) why do we want failOnError as a tunable?

It confuses the semantics of the verb. failOnError=True means "If one service action fails but other succeed, return an error code with no explanation on which service action failed". This I do not see why this is useful.
Line 132:     """
Line 133:     {'services': [
Line 134:         {'name': SERVICE_NAME, 'status': STATUS, 'message': MESSAGE},..]}
Line 135:     """


....................................................
File vdsm/gluster/vdsmapi-gluster-schema.json
Line 260: # @RUNNING:    Service is Running
Line 261: #
Line 262: # @STOPPED:    Service is Stopped
Line 263: #
Line 264: # @NOT_AVAILABLE:    Service is not managed/available
does this mean that the relevant service is not installed on host?
Line 265: #
Line 266: # @NOT_SUPPORTED:    Service is not allowed to manage
Line 267: #
Line 268: # @ERROR:    Service action failed


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8a16bf566d17e186a66503391dfd04b2f2bb4bb4
Gerrit-PatchSet: 23
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Aravinda VK <avishwan at redhat.com>
Gerrit-Reviewer: Adam Litke <agl at us.ibm.com>
Gerrit-Reviewer: Aravinda VK <avishwan at redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin at redhat.com>
Gerrit-Reviewer: Zhou Zheng Sheng <zhshzhou at linux.vnet.ibm.com>
Gerrit-Reviewer: oVirt Jenkins CI Server


More information about the vdsm-patches mailing list