Change in vdsm[master]: Using CrabRPC to communicate between vdsm and super vdsm

smizrahi at redhat.com smizrahi at redhat.com
Wed Mar 13 18:15:31 UTC 2013


Saggi Mizrahi has posted comments on this change.

Change subject: Using CrabRPC to communicate between vdsm and super vdsm
......................................................................


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

(11 inline comments)

....................................................
File vdsm/storage/remoteFileHandler.py
Line 59:     log = logging.getLogger("Storage.CrabRPCServer")
Line 60: 
Line 61:     def __init__(self, myRead, myWrite):
Line 62:         self.myRead = myRead
Line 63:         self.myWrite = myWrite
fdopen, opens a new fd so you can close myRead and myWrite at the end of this method.
Line 64:         self.rfile = os.fdopen(myRead, "r")
Line 65:         self.wfile = os.fdopen(myWrite, "wa")
Line 66:         self.registeredFunctions = {}
Line 67:         self.registeredModules = {}


Line 116:                 func = getattr(func, part)
Line 117: 
Line 118:         return func(*args, **kwargs)
Line 119: 
Line 120:     def close(self):
handle rfile and wfile and make sure you handle double close
Line 121:         if self.myRead is not None:
Line 122:             os.close(self.myRead)
Line 123:             self.myRead = None
Line 124: 


Line 199:             raise
Line 200:         except:
Line 201:             self.log.error("Problem with handler",
Line 202:                            exc_info=True)
Line 203:             raise
Why? Do you like error paths?
Line 204: 
Line 205:         res, err = pickle.loads(rawResponse)
Line 206:         if err is not None:
Line 207:             raise err


....................................................
File vdsm/supervdsmAPI.py
Line 65:     return wrapper
Line 66: 
Line 67: 
Line 68: @logDecorator
Line 69: def exportToSuperVdsm(func):
This all mechanism causes a double dependency even though only implied.

You need to get the oop specific code out of remoteFileHandler.py and make the executable of remoteFileHandler as a parameter and have a similar boilerplate for supervdsm.
Line 70:     @wraps(func)
Line 71:     def wrapper(*args, **kwargs):
Line 72:         return func(*args, **kwargs)
Line 73: 


....................................................
File vdsm/supervdsm.py
Line 30: _g_singletonSupervdsmInstance = None
Line 31: _g_singletonSupervdsmInstance_lock = threading.Lock()
Line 32: 
Line 33: vdsm_r = None
Line 34: vdsm_w = None
They don't need to be global
Line 35: 
Line 36: DEFAULT_CALL_TIMEOUT = 10
Line 37: 
Line 38: 


Line 35: 
Line 36: DEFAULT_CALL_TIMEOUT = 10
Line 37: 
Line 38: 
Line 39: def setPipe(r, w):
Doesn't need to be global
Line 40:     global vdsm_r
Line 41:     global vdsm_w
Line 42: 
Line 43:     if vdsm_r is not None or vdsm_w is not None:


Line 39: def setPipe(r, w):
Line 40:     global vdsm_r
Line 41:     global vdsm_w
Line 42: 
Line 43:     if vdsm_r is not None or vdsm_w is not None:
Can be removed once the methods isn't global anymore.
Line 44:         raise \
Line 45:             Exception('trying to set supervdsm pipe that already initialized')
Line 46:     vdsm_r = r
Line 47:     vdsm_w = w


Line 46:     vdsm_r = r
Line 47:     vdsm_w = w
Line 48: 
Line 49: 
Line 50: def closePipe():
Doesn't need to be global
Line 51:     global vdsm_r
Line 52:     global vdsm_w
Line 53:     os.close(vdsm_r)
Line 54:     os.close(vdsm_w)


Line 62:     t.setDaemon(True)
Line 63:     t.start()
Line 64: 
Line 65: 
Line 66: def _keepAliveThread(log):
Keep alive?
Line 67:     global vdsm_r
Line 68:     if vdsm_r is None:
Line 69:         panic('Cannot monitor supervdsm')
Line 70:     poller = select.poll()


Line 87:     def __getattr__(self, name):
Line 88:         self._funcName = name
Line 89:         return self
Line 90: 
Line 91:     def __call__(self, *args, **kwargs):
What?
Line 92:         try:
Line 93:             return self.callCrabRPCFunction(DEFAULT_CALL_TIMEOUT,
Line 94:                                             self._funcName, *args, **kwargs)
Line 95:         except Exception as e:


....................................................
File vdsm/supervdsmServer.py
Line 33: svdsm_r = None
Line 34: svdsm_w = None
Line 35: 
Line 36: 
Line 37: class SupervdsmManager(CrabRPCServer):
No inheritance, ever
Line 38: 
Line 39:     def __init__(self, svdsm_r, svdsm_w):
Line 40:         self.log = logging.getLogger("SuperVdsm")
Line 41:         supervdsmAPI.log = self.log


--
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: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Adam Litke <agl at us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Royce Lv <lvroyce at linux.vnet.ibm.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server


More information about the vdsm-patches mailing list