Nir Soffer has posted comments on this change.
Change subject: wip: jsonrpc: introduce new api
......................................................................
Patch Set 4:
(19 comments)
https://gerrit.ovirt.org/#/c/64502/4/lib/vdsm/jsonrpcapi.py
File lib/vdsm/jsonrpcapi.py:
Line 15: # along with this program; if not, write to the Free Software
Line 16: # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
Line 17: #
Line 18: # Refer to the README and COPYING files for full details of the license
Line 19: #
How about vdsm.client?
We are not going to switch the protocol again, this was painful enough last time. And
jsonrpc is not correct anyway, we are actually using stomp, not jsonrpc.
Line 20:
Line 21: from __future__ import absolute_import
Line 22:
Line 23: from uuid import uuid4
Line 19: #
Line 20:
Line 21: from __future__ import absolute_import
Line 22:
Line 23: from uuid import uuid4
Don't import functions, only modules. This make the dependencies easier to understand
and allow reloading of modules in runtime.
Line 24:
Line 25: from yajsonrpc import stompreactor
Line 26: from yajsonrpc import \
Line 27: JsonRpcRequest, \
Line 21: from __future__ import absolute_import
Line 22:
Line 23: from uuid import uuid4
Line 24:
Line 25: from yajsonrpc import stompreactor
This package depends on vdsm, we need to make the client separate from vdsm, it should be
a separate standalone package, maybe depending on the api package.
We can resolve this issue later, our most important users (hosted-engine, mom) run on the
same machine.
Line 26: from yajsonrpc import \
Line 27: JsonRpcRequest, \
Line 28: JsonRpcNoResponseError, \
Line 29: CALL_TIMEOUT
Line 25: from yajsonrpc import stompreactor
Line 26: from yajsonrpc import \
Line 27: JsonRpcRequest, \
Line 28: JsonRpcNoResponseError, \
Line 29: CALL_TIMEOUT
Better just to import yajsonrpc and use yajsonrpc.Xxx when needed.
Line 30:
Line 31: from api import vdsmapi
Line 32: from .config import config
Line 33:
Line 28: JsonRpcNoResponseError, \
Line 29: CALL_TIMEOUT
Line 30:
Line 31: from api import vdsmapi
Line 32: from .config import config
Vdsm config may not be avilable.
Line 33:
Line 34:
Line 35: class _Server(object):
Line 36:
Line 31: from api import vdsmapi
Line 32: from .config import config
Line 33:
Line 34:
Line 35: class _Server(object):
This is a client, not a server.
Line 36:
Line 37: def __init__(self, client):
Line 38: api_strict_mode = config.getboolean('devel',
'api_strict_mode')
Line 39: self._schema = vdsmapi.Schema([vdsmapi.find_schema()],
Line 34:
Line 35: class _Server(object):
Line 36:
Line 37: def __init__(self, client):
Line 38: api_strict_mode = config.getboolean('devel',
'api_strict_mode')
You cannot use vdsm configuration here, the library may be used on another machine
(another server, someone laptop), which does not have vdsm installed.
Line 39: self._schema = vdsmapi.Schema([vdsmapi.find_schema()],
Line 40: api_strict_mode)
Line 41: self._client = client
Line 42: self._default_timeout = CALL_TIMEOUT
Line 36:
Line 37: def __init__(self, client):
Line 38: api_strict_mode = config.getboolean('devel',
'api_strict_mode')
Line 39: self._schema = vdsmapi.Schema([vdsmapi.find_schema()],
Line 40: api_strict_mode)
Please avoid using the schema here, we don't want to pay for parsing the schema on
each client call.
Also, this is not needed to have use the client, maybe only when the client is run with
--help, or with --verify flag. This library must provide control to the user, not force
the shema on all users of the library.
Please send a very simple first version, we don't have lot of time for 4.1, and if we
waste time on the schema this will not be ready in time and we will have to carry xmlrpc
for another version.
Line 41: self._client = client
Line 42: self._default_timeout = CALL_TIMEOUT
Line 43: self._timeouts = {
Line 44: 'migrationCreate': config.getint(
Line 44: 'migrationCreate': config.getint(
Line 45: 'vars', 'migration_create_timeout'),
Line 46: }
Line 47:
Line 48: def set_default_timeout(self, timeout):
We don't need setters like this in python. just make default_timeout public
attribute.
If we need to do handle or control changes, we can convert it to a property.
Line 49: self._default_timeout = timeout
Line 50:
Line 51: def _prepare_args(self, className, methodName, args, kwargs):
Line 52: try:
Line 57: params = dict(zip(allargs, args))
Line 58: params.update(kwargs)
Line 59: return params
Line 60:
Line 61: def call(self, method, *args, **kwargs):
Can you explain why we need both *args and **kwargs? To send a request we need:
- The method name e..g "Host.getJobs"
- The arguments: always a dict
So we can use:
def call(self, method_names, args):
req = JsonRpcRequest(method_name, args, req_id=str(uuid.uuid4()))
Please do not complicate in the first version of the client.
We should *not* accept list of arguments and fill in the arguments name from the schema.
The caller *must* provide the exact names and values as the schema defines.
This is critical to allow users to pass only the required parameters, instead of providing
all parameters like we had to use with xmlrpc.
Line 62: class_name, method_name = method.split('.')
Line 63: params = self._prepare_args(class_name, method_name, args, kwargs)
Line 64:
Line 65: req = JsonRpcRequest(method, params, reqId=str(uuid4()))
Line 66:
Line 67: responses = self._client.call(
Line 68: req, timeout=self._timeouts.get(
Line 69: method_name,
Line 70: kwargs.pop('_transport_timeout',
self._default_timeout)))
Please don't do that, no hidden arguments in kwargs. If you want to support a timeout,
you must provide a timeout kwargs option, e.g.
def call(self, method_name, args, timeout=default_timeout):
If you cannot set the timeout statically, do:
def call(self, method_name, args, timeout=None):
if timeout is None:
timeout = self._default_timeout
When we call, we should have a timeout variable with the correct value, not add complex
logic into the call.
responses = self._client.call(req, timeout=timeout)
Can we get OSError, IOError or socket.error from this call?
If we can, we better catch them and wrap them with ClientError (see below):
try:
make the call...
except EnvironmentError as e:
raise ClientError(e)
Otherwise client code will have to handle these errors instead of expecting only our
public api.
Line 71: if responses:
Line 72: resp = responses[0]
Line 73: else:
Line 74: raise JsonRpcNoResponseError(method)
Line 70: kwargs.pop('_transport_timeout',
self._default_timeout)))
Line 71: if responses:
Line 72: resp = responses[0]
Line 73: else:
Line 74: raise JsonRpcNoResponseError(method)
Use the happy pass idiom:
if not responses:
raise ...
normal flow
This is true for any code in vdsm.
Also do not raise JsonRpc* errors, we want to raise only errors which are part of this
module api.
What is the meaning of getting no responses? timeout?
Line 75: # TODO: raise exception on error
Line 76: if resp.error:
Line 77: raise Exception(resp.error['code'],
resp.error['message'])
Line 78: return resp.result
Line 73: else:
Line 74: raise JsonRpcNoResponseError(method)
Line 75: # TODO: raise exception on error
Line 76: if resp.error:
Line 77: raise Exception(resp.error['code'],
resp.error['message'])
You never want to raise Exception, the user of this api cannot do anything with it.
We want to let the user detect client errors and vdsm errors.
We want these errors in this module:
class Error(Exception):
"""
Base class for vdsm.client errors
"""
msg = ""
def __str__(self):
return self.msg.format(self=self)
class ClientError(Error):
msg = "Error sending request: {self.reason}"
def __init__(self, reason):
self.reason = reason
class ServerError(Error):
msg = "Vdsm request failed (code={self.code}, message={self.message})"
def __init__(self, code, message):
self.code = code
self.message = message
And here we should raise ServerError.
Any other error raised from this module must be a bug in the module or the
underlying code it uses.
Line 78: return resp.result
Line 79:
Line 80: def migrationCreate(self, params, incomingLimit=None):
Line 81: args = [params]
Line 76: if resp.error:
Line 77: raise Exception(resp.error['code'],
resp.error['message'])
Line 78: return resp.result
Line 79:
Line 80: def migrationCreate(self, params, incomingLimit=None):
We don't need this.
Line 81: args = [params]
Line 82: if incomingLimit is not None:
Line 83: args.append(incomingLimit)
Line 84: return self.call('migrationCreate', params['vmId'],
*args)
Line 82: if incomingLimit is not None:
Line 83: args.append(incomingLimit)
Line 84: return self.call('migrationCreate', params['vmId'],
*args)
Line 85:
Line 86: def create(self, params):
Or this
Line 87: return self.call('create', params['vmId'], params)
Line 88:
Line 89: def close(self):
Line 90: self._client.close()
Line 88:
Line 89: def close(self):
Line 90: self._client.close()
Line 91:
Line 92: def __del__(self):
Never use __del__.
If you want to ensure that this object will be closed, provide a context manager
interface:
def __enter__(self):
return self
def __exit__(self, t, v, tb):
try:
self.close()
except Exception:
if t is None:
raise
log.exception("Error closing client")
With these methods implemented, you can use this class like this:
with client.connect() as cli:
# use cli here...
The user is responsible for closing the client or using the client as context manager, we
don't promise anything else.
Line 93: self._client.close()
Line 94:
Line 95:
Line 96: def connect(host=None, port=None, useSSL=None):
Line 92: def __del__(self):
Line 93: self._client.close()
Line 94:
Line 95:
Line 96: def connect(host=None, port=None, useSSL=None):
It is more common in stdlib to provide an open() method for such usage, and it also works
nicely with the close() method or a context manager.
Line 97: if host is None:
Line 98: host = 'localhost'
Line 99: if port is None:
Line 100: port = int(config.getint('addresses',
'management_port'))
Line 96: def connect(host=None, port=None, useSSL=None):
Line 97: if host is None:
Line 98: host = 'localhost'
Line 99: if port is None:
Line 100: port = int(config.getint('addresses',
'management_port'))
Do not assume you have vdsm config here.
Line 101:
Line 102: if useSSL is None:
Line 103: useSSL = config.getboolean('vars', 'ssl')
Line 104:
Line 99: if port is None:
Line 100: port = int(config.getint('addresses',
'management_port'))
Line 101:
Line 102: if useSSL is None:
Line 103: useSSL = config.getboolean('vars', 'ssl')
Same.
Force the user to specify everything, no defaults are needed in a library.
In the command line client using this library, we can provide defaults and try to handle
both running on a host, (with vdsm config, standard pki dir), or on another machine (user
must specify pki dir, and other options).
Line 104:
Line 105: client = stompreactor.SimpleClient(host, port, useSSL)
Line 106:
--
To view, visit
https://gerrit.ovirt.org/64502
To unsubscribe, visit
https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Idd45d7e88bf2246beaf30550b12201917f32c354
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Irit Goihman <igoihman(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes