Royce Lv has posted comments on this change. Change subject: Using CrabRPC to communicate between vdsm and super vdsm ......................................................................
Patch Set 2: (6 inline comments) Haven't checked the corner cases and verified yet,Some comments on wrapper of crabRPC server,Do you use crabRPC instead of Rpyc because of socket? Whatever communication we use, I hope the caller interface stays unchange, which means vdsm just getProxy() without pass the pipe, and supervdsm just get_server() then serve_forever(). If you like, I hope we can base on http://gerrit.ovirt.org/#/q/status:open+project:vdsm+branch:master+topic:change-vdsm-startup,n,z to integrate crabRPC. , .................................................... Commit Message Line 7: Using CrabRPC to communicate between vdsm and super vdsm Line 8: Line 9: Starting up vdsm as usual, fork sub process and drop its privilege, set Line 10: the subprocess to manage vdsm requests, and the father as supervdsm. Line 11: Both communicate by a pipes. s/"by a pipes"/"through pipe" Line 12: Line 13: Change-Id: I4f1053e7d1264003fa265e44899d8b02f98bd68a .................................................... File tests/superVdsmTests.py Line 3: import supervdsmServer Line 4: import supervdsm Line 5: import imp Line 6: #from time import sleep Line 7: #from vdsm import utils unused comments here Line 8: from vdsm.constants import DISKIMAGE_GROUP, METADATA_GROUP,\ Line 9: QEMU_PROCESS_USER Line 10: Line 11: vdsmPackage = imp.load_source("vdsm", "../vdsm/vdsm") .................................................... File vdsm/supervdsm.py Line 49: return self Line 50: Line 51: def __call__(self, *args, **kwargs): Line 52: return self.callCrabRPCFunction(DEFAULT_CALL_TIMEOUT, Line 53: self._funcName, *args, **kwargs) I hope supervdsmServer create pipes as global variables or some other way, and here in proxy.__init__() we assign pipes as the underlying channel, by connect() we try to ping to make sure the pipe is OK. In this way we maintain the connection just like previous interface, and not expose them to upper layer. Line 54: Line 55: Line 56: def getProxy(): Line 57: global _g_singletonSupervdsmInstance .................................................... File vdsm/supervdsmServer.py Line 387: # Iterate all file funcs and expose only those who starts with exposed_ Line 388: for fName in dir(supervdsmServer): Line 389: try: Line 390: exposed, name = fName.split('_') Line 391: if exposed == 'exposed': I suggest to use decorator instead of renaming all exposed functions Line 392: func = getattr(supervdsmServer, fName) Line 393: s.registerFunction(func, name) Line 394: except (AttributeError, ValueError): Line 395: # means fName not a function or name doesn't include _ .................................................... File vdsm/vdsm Line 133: Line 134: vdsmPid = os.getpid() Line 135: vdsm_r, svdsm_w = os.pipe() Line 136: svdsm_r, vdsm_w = os.pipe() Line 137: supervdsm.setPipe(vdsm_r, vdsm_w) In my mind as a proxy, we don't expose the low level communication channel details to proxy user. RPC server and proxy manages its underlying channel, construct and destruct it, the proxy user only connect and call its functions. Line 138: son_pid = os.fork() Line 139: Line 140: if son_pid != 0: Line 141: startSuperVdsm(svdsm_r, svdsm_w, son_pid) Line 137: supervdsm.setPipe(vdsm_r, vdsm_w) Line 138: son_pid = os.fork() Line 139: Line 140: if son_pid != 0: Line 141: startSuperVdsm(svdsm_r, svdsm_w, son_pid) I'd prefet the python wrap of Process rather then fork and waitpid in C style, because expose too many details make functions difficult to debug and maintain. Line 142: Line 143: else: Line 144: dropPrivileges() Line 145: log = initVdsmLogger() -- To view, visit http://gerrit.ovirt.org/11910 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4f1053e7d1264003fa265e44899d8b02f98bd68a Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Royce Lv <[email protected]> Gerrit-Reviewer: Saggi Mizrahi <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
