Yaniv Bronhaim has posted comments on this change. Change subject: jsonrpc: Vdsm changes ......................................................................
Patch Set 17: Code-Review-1 (22 comments) http://gerrit.ovirt.org/#/c/19497/17//COMMIT_MSG Commit Message: Line 7: jsonrpc: Vdsm changes Line 8: Line 9: Here are engine changes: http://gerrit.ovirt.org/#/c/20926/ Line 10: Line 11: Changes are done on top of Saggi's reimplementation of async tcp server the "Signed-off-by: Saggi Mizrahi <smizr...@redhat.com>" is enough .. but as you like Line 12: using json. The changes include: Line 13: - Fixing ssl Line 14: - Gluster api support Line 15: - Fixing number of issues around Bridge.py and json binding Line 11: Changes are done on top of Saggi's reimplementation of async tcp server Line 12: using json. The changes include: Line 13: - Fixing ssl Line 14: - Gluster api support Line 15: - Fixing number of issues around Bridge.py and json binding elaborate? Line 16: - Update vdsm schema and code to be consistent Line 17: - getAllVmStats was only available for xml so it was moved to API Line 18: - vm snapshot method had not optional argument Line 19: - added implementation for full vm list Line 12: using json. The changes include: Line 13: - Fixing ssl Line 14: - Gluster api support Line 15: - Fixing number of issues around Bridge.py and json binding Line 16: - Update vdsm schema and code to be consistent this is part of http://gerrit.ovirt.org/#/c/23125/ now, move the comment there. Line 17: - getAllVmStats was only available for xml so it was moved to API Line 18: - vm snapshot method had not optional argument Line 19: - added implementation for full vm list Line 20: - added gerHardwareInfo method http://gerrit.ovirt.org/#/c/19497/17/lib/yajsonrpc/asyncoreReactor.py File lib/yajsonrpc/asyncoreReactor.py: Line 1: # Copyright (C) 2012 Saggi Mizrahi, Red Hat Inc. you can update to 2014, and what about putting an header on betterAsyncore.py as well? Line 2: # Line 3: # This program is free software; you can redistribute it and/or modify Line 4: # it under the terms of the GNU General Public License version 2 as Line 5: # published by the Free Software Foundation. Line 161: self._address = address Line 162: self._acceptHandler = acceptHandler Line 163: Line 164: def init(self, dispatcher): Line 165: address = self._address why do you need this temporary address? Line 166: address_family = socket.getaddrinfo(*address)[0][0] Line 167: dispatcher.create_socket(address_family, socket.SOCK_STREAM) Line 168: Line 169: dispatcher.set_reuse_addr() http://gerrit.ovirt.org/#/c/19497/17/lib/yajsonrpc/betterAsyncore.py File lib/yajsonrpc/betterAsyncore.py: Line 1: # Asyncore uses inheritance all around which makes it not flexible enought for s/enought/enough where is copyright header on that file? Line 2: # us to use. This does tries to reuse enought code from the original asyncore Line 3: # while enabling compositing instead of inheritance. Line 4: import asyncore Line 5: from M2Crypto import SSL, X509 Line 100: return SSLSocket(SSL.Connection(context, sock=sock), self) Line 101: Line 102: Line 103: # This is a copy of the standard library asyncore converted to support Line 104: # compositing. Also fixes races in original implementation. can you elaborate what races does it fix? and specify a link to the origin code Line 105: class AsyncChat(object): Line 106: # these are overridable defaults Line 107: Line 108: ac_in_buffer_size = 4096 http://gerrit.ovirt.org/#/c/19497/17/lib/yajsonrpc/protonReactor.py File lib/yajsonrpc/protonReactor.py: Line 26: CONNECTED = 1 Line 27: SERVER_AUTH = 2 Line 28: CLIENT_AUTH = 3 Line 29: Line 30: MBUFF_SIZE = 10 can you explain why 10 in a comment? Line 31: Line 32: Line 33: class ProtonError(RuntimeError): Line 34: pass Line 33: class ProtonError(RuntimeError): Line 34: pass Line 35: Line 36: Line 37: # Used for reactor coroutines I don't understand the usage from this comment.. either remove it or elaborate Line 38: class Return(object): Line 39: def __init__(self, value): Line 40: self.value = value Line 41: Line 103: self.log.debug("Opening active connection") Line 104: proton.pn_connection_open(self.connection) Line 105: Line 106: while True: Line 107: # TODO: Handle connection being closed mid authentication when do you plan to handle closed mid auth? Line 108: if proton.pn_sasl_state(sasl) in (proton.PN_SASL_PASS,): Line 109: proton.pn_connector_set_context(self.connector, CONNECTED) Line 110: break Line 111: Line 125: yield Return(1) Line 126: Line 127: def _pushIncomingMessage(self, msg): Line 128: try: Line 129: self._messageHandler((self, msg)) better to check if _messageHandler is None Line 130: except AttributeError: Line 131: # Inbox not set Line 132: self.log.warn("Message missed since inbox was not set for " Line 133: "this client") Line 134: Line 135: def _popPendingMessage(self): Line 136: msg = self._outbox.get_nowait() Line 137: self.log.debug("Pulling message from outbox") Line 138: return msg just return self._outbox.get_nowait() and is this debug print really needed? Line 139: Line 140: def setMessageHandler(self, msgHandler): Line 141: self._messageHandler = msgHandler Line 142: Line 145: self._reactor._activate(self.connector, Line 146: proton.PN_CONNECTOR_WRITABLE) Line 147: Line 148: def close(self): Line 149: #TODO nothing to handle on close? sounds risky no need to close the connector somehow? Line 150: pass Line 151: Line 152: Line 153: class ProtonReactor(object): http://gerrit.ovirt.org/#/c/19497/17/tests/Makefile.am File tests/Makefile.am: Line 133: Line 134: # crossImportsTests.py has to be run separate due to different tests which Line 135: # load modules first, making the env dirty. Unloading python modules Line 136: # is not supported, see http://bugs.python.org/issue9072 . Line 137: check-local: jsonrpc-tests.server.crt I'd prefer to add it as we run crossImportsTests.py and not in new label. but can't you execute this sh before the specific test and not in the makefile scope? I'm also not so sure.. Line 138: @echo '*** Running tests. To skip this step place NOSE_EXCLUDE=.* ***' Line 139: @echo '*** into your environment. Do not submit untested code! ***' Line 140: $(top_srcdir)/tests/run_tests_local.sh crossImportsTests.py http://gerrit.ovirt.org/#/c/19497/17/tests/apiTests.py File tests/apiTests.py: Line 31: from jsonRpcUtils import getFreePort Line 32: Line 33: Line 34: ip = '127.0.0.1' Line 35: port = 9824 declare globally also the id imo .. easier to modify Line 36: _fakeret = {} Line 37: Line 38: apiWhitelist = ('StorageDomain.Classes', 'StorageDomain.Types', Line 39: 'Volume.Formats', 'Volume.Types', 'Volume.Roles', Line 211: 'id': '2c8134fd-7dd4-4cfc-b7f8-6b7549399cb6', Line 212: 'method': 'Host.ping', Line 213: 'params': {}}) Line 214: reply = self.sendMessage(msg) Line 215: self.assertFalse('error' in reply) why did you change it? NotIn's output is much more informative Line 216: self.assertTrue(reply['result']) Line 217: Line 218: def testPingError(self): Line 219: self.clearAPI() http://gerrit.ovirt.org/#/c/19497/17/tests/jsonRpcUtils.py File tests/jsonRpcUtils.py: Line 39: Line 40: @contextmanager Line 41: def _tcpServerConstructor(): Line 42: port = getFreePort() Line 43: address = ("127.0.0.1", port) why not localhost? Line 44: reactorType = asyncoreReactor.AsyncoreReactor Line 45: yield reactorType, address Line 46: Line 47: http://gerrit.ovirt.org/#/c/19497/17/vdsm/API.py File vdsm/API.py: Line 1192: self.log.error("failed to retrieve hardware info", exc_info=True) Line 1193: return errCode['hwInfoErr'] Line 1194: Line 1195: def getAllVmStats(self): Line 1196: """ how is that related?? maybe it should be part of http://gerrit.ovirt.org/#/c/23125/ , or seperatly at all Line 1197: Get statistics of all running VMs. Line 1198: """ Line 1199: vms = self.getVMList() Line 1200: statsList = [] http://gerrit.ovirt.org/#/c/19497/17/vdsm/BindingXMLRPC.py File vdsm/BindingXMLRPC.py: Line 317: vm = API.VM(vmId) Line 318: return vm.getStats() Line 319: Line 320: def getAllVmStats(self): Line 321: api = API.Global() not related. please remove to different patch Line 322: return api.getAllVmStats() Line 323: Line 324: def vmMigrationCreate(self, params): Line 325: vm = API.VM(params['vmId']) http://gerrit.ovirt.org/#/c/19497/17/vdsm/clientIF.py File vdsm/clientIF.py: Line 163: ip = config.get('addresses', 'management_ip') Line 164: port = config.getint('addresses', 'json_port') Line 165: use_ssl = config.getboolean('vars', 'ssl') Line 166: truststore_path = None Line 167: if use_ssl: just if config.getboolean('vars', 'ssl'): you don't use this use_ssl anywhere Line 168: truststore_path = config.get('vars', 'trust_store_path') Line 169: conf = [('tcp', {"ip": ip, "port": port})] Line 170: self.bindings['json'] = BindingJsonRpc(DynamicBridge(), conf, Line 171: truststore_path) http://gerrit.ovirt.org/#/c/19497/17/vdsm/gluster/apiwrapper.py File vdsm/gluster/apiwrapper.py: Line 1: # Line 2: # Copyright 2012 Red Hat, Inc. 2014 and is it really part of the json refactoring ? I thing it can be handler and reviewed separately also by gluster guys.. and, im not sure it is a must for adding json-rpc .. no? 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 http://gerrit.ovirt.org/#/c/19497/17/vdsm_api/Bridge.py File vdsm_api/Bridge.py: Line 19: import logging Line 20: import types Line 21: import API Line 22: import yajsonrpc Line 23: try: those changes relate to - http://gerrit.ovirt.org/#/c/23125/ Line 24: import gluster.apiwrapper as gapi Line 25: _glusterEnabled = True Line 26: except ImportError: Line 27: _glusterEnabled = False -- To view, visit http://gerrit.ovirt.org/19497 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If828355b7efe28fe6a2e784069425fefd2f3f25c Gerrit-PatchSet: 17 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Saggi Mizrahi <smizr...@redhat.com> Gerrit-Reviewer: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com> Gerrit-Reviewer: Eduardo <ewars...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Saggi Mizrahi <smizr...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: mooli tayer <mta...@redhat.com> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
