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 <avish...@redhat.com>
Gerrit-Reviewer: Adam Litke <a...@us.ibm.com>
Gerrit-Reviewer: Aravinda VK <avish...@redhat.com>
Gerrit-Reviewer: Bala.FA <barum...@redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizr...@redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeya...@redhat.com>
Gerrit-Reviewer: Zhou Zheng Sheng <zhshz...@linux.vnet.ibm.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
