Change in vdsm[master]: add a jsonrpcSeverClient for jsonRPC Server functional test

Zhou Zheng Sheng has posted comments on this change.

Change subject: add a jsonrpcSeverClient for jsonRPC Server functional test
......................................................................

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

(6 inline comments)

-1 for not detecting the error field in the response message.

....................................................
File tests/functional/jsonrpcSeverClient.py
Line 1: #
Line 2: # Copyright 2012 Red Hat, Inc.
Do you take this file from Red Hat developers or just a typo?
Line 3: #
Line 4: # This program is free software; you can redistribute it and/or modify
Line 5: # it under the terms of the GNU General Public License as published by
Line 6: # the Free Software Foundation; either version 2 of the License, or

Line 43: 
Line 44: 
Line 45: class jsonRpcServerClient(object):
Line 46: 
Line 47:     #TODO: spport ssl
I notice you have SSL related code below in sendMessage. What else need to be done? If the SSL support is done, please remove this TODO. If it's not done, can you describe the what to do in detail?
Line 48:     def __init__(self, ip, port, certReq=False, tsPath=None):
Line 49:         self._ip = ip
Line 50:         self._port = port
Line 51:         self._tsPath = tsPath

Line 59:         msg = json.dumps(data)
Line 60:         msg = msg.encode('utf-8')
Line 61:         msize = self._Size.pack(len(msg))
Line 62:         resp = msize + msg
Line 63:         return resp
Does "resp" stand for response ? In buildMessage, I think "result" or "reqMsg" (stands for request message) is better.
Line 64: 
Line 65:     def _createRequest(self, method, reqId=None, params=()):
Line 66:         return {'jsonrpc': '2.0', "id": reqId, "method": method,
Line 67:                 "params": params}

Line 78:                 sock = ssl.wrap_socket(tcp_sock,
Line 79:                                        keyfile=KEYFILE,
Line 80:                                        certfile=CERTFILE,
Line 81:                                        ca_certs=CACERT,
Line 82:                                        cert_reqs=self._cert_reqs)
You use short connection with SSL. This is not efficient. SSL handshake is expensive at server side. If the JSON-RPC server supports persistent connection, I think we should utilize it. Modern server can deal with 10K concurrent connections easily if implemented well.

It's OK for this testing/demo client proxy to use short connection. You can improve it in future patch.
Line 83:             else:
Line 84:                 sock = tcp_sock
Line 85:             sock.settimeout(3)  # TBD timeout
Line 86:             try:

Line 108: 
Line 109:     def call(self, method, params=(), reqId=None):
Line 110:         if reqId is None:
Line 111:             rId = self._reqCounter
Line 112:             self._reqCounter += 1
The above code is not thread-safe, but it maybe OK because you just issue just one request in a connection, so you do not have to keep it unique, the id field can be any int value.

According to the JSON-RPC 2.0 spec [1], the id can be a string, so you can just use str(uuid.uuid4()) to make it thread-safe. However I have a look at JsonRpcServer._parseRequest implementation, the server forces the id to be an int, we should fix that.

[1] http://www.jsonrpc.org/specification
Line 113:         else:
Line 114:             rId = reqId
Line 115:         msg = self.buildMessage(self._createRequest(method, rId, params))
Line 116:         reply = self.sendMessage(msg)

Line 115:         msg = self.buildMessage(self._createRequest(method, rId, params))
Line 116:         reply = self.sendMessage(msg)
Line 117:         if rId != reply['id']:
Line 118:             raise ResIdResponseError("error reqId:%s response, expect %s" %
Line 119:                                      (reply['id'], rId))
If the error field in the received message is not null, the server has reported an exception or error to the client proxy. The client proxy should raise an exception to the caller in this case. I do not see any logic in this call() method to deal with this.
Line 120:         return reply['result']
Line 121: 
Line 122:     def _dynamicAttribute(self, schema):
Line 123:         self.dynamicAttr = {}

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib081e26203638114d7d632489432a50eeea45dd4
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: ShaoHe Feng 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Antoni Segura Puimedon 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Deepak C Shetty 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: ShaoHe Feng 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Vinzenz Feenstra 
Gerrit-Reviewer: Zhou Zheng Sheng 
Gerrit-Reviewer: oVirt Jenkins CI Server