Change in vdsm[master]: json-rpc: Protocol detection

nsoffer at redhat.com nsoffer at redhat.com
Wed May 28 22:07:05 UTC 2014


Nir Soffer has posted comments on this change.

Change subject: json-rpc: Protocol detection
......................................................................


Patch Set 27:

(19 comments)

Since we have some more time [1], I did another round on clientIF. Nothing critical, mainly improving the way the acceptor and bindings are created, started and stopped.

[1] http://resources.ovirt.org/meetings/ovirt/2014/ovirt.2014-05-28-14.01.txt

http://gerrit.ovirt.org/#/c/26300/27/vdsm/clientIF.py
File vdsm/clientIF.py:

Line 83:         self.channelListener = Listener(self.log)
Line 84:         self._generationID = str(uuid.uuid4())
Line 85:         self.mom = None
Line 86:         self.xml_detector = None
Line 87:         self.stomp_detector = None
Lets remove the detectors, which are just wrappers for the bindings, given to the acceptor, and keep instead the binding objects, which should be started and stopped.

Instead, lets have a list of bindings, like the old code did:

    self._bindings = []

We will add binding to this list later, and it will make it easier to start and stop the bindings without checking if a binding is configured or not.
Line 88:         if _glusterEnabled:
Line 89:             self.gluster = gapi.GlusterApi(self, log)
Line 90:         else:
Line 91:             self.gluster = None


Line 111:                 self.irs.prepareForShutdown()
Line 112:             if self.mom:
Line 113:                 self.mom.stop()
Line 114:             raise
Line 115:         self._prepareProtocolDetector()
doing stuff here is not a good idea, since we don't have an exception handler to protect us in case on an error.
Line 116: 
Line 117:     @property
Line 118:     def ready(self):
Line 119:         return (self.irs is None or self.irs.ready) and not self._recovery


Line 150:                 else:
Line 151:                     cls._instance = clientIF(irs, log)
Line 152:         return cls._instance
Line 153: 
Line 154:     def _prepareProtocolDetector(self):
Nice how everything is simply configured here. But the name is misleading, since we do not prepare the detector here, we prepare the acceptor and the bindings.

We can find a better name, but I think that a better way would be to inline this into __init__.
Line 155:         host = config.get('addresses', 'management_ip')
Line 156:         port = config.getint('addresses', 'management_port')
Line 157:         default_bridge = config.get("vars", "default_bridge")
Line 158:         sslctx = self._createSSLContext()


Line 152:         return cls._instance
Line 153: 
Line 154:     def _prepareProtocolDetector(self):
Line 155:         host = config.get('addresses', 'management_ip')
Line 156:         port = config.getint('addresses', 'management_port')
The host, port and sslctx are used only for creating the acceptor - maybe extract a method for this?

   def _createAcceptor(self, host):
       host = config.get('addresses', 'management_ip')
       port = config.getint('addresses', 'management_port')
       sslctx = self._createSSLContext()
       return MultiProtocolAcceptor(host, port, sslctx)

Ok, I lied,  the xmlrpc binding also wants to host, but the method preparing it can get it from the config easily.
Line 157:         default_bridge = config.get("vars", "default_bridge")
Line 158:         sslctx = self._createSSLContext()
Line 159: 
Line 160:         self._acceptor = MultiProtocolAcceptor(host, port, sslctx)


Line 153: 
Line 154:     def _prepareProtocolDetector(self):
Line 155:         host = config.get('addresses', 'management_ip')
Line 156:         port = config.getint('addresses', 'management_port')
Line 157:         default_bridge = config.get("vars", "default_bridge")
Since this is used only by xmlrpc binding, why not move it into _prepareXMLRPCBindings?
Line 158:         sslctx = self._createSSLContext()
Line 159: 
Line 160:         self._acceptor = MultiProtocolAcceptor(host, port, sslctx)
Line 161:         self._prepareXMLRPCBinding(host, default_bridge)


Line 158:         sslctx = self._createSSLContext()
Line 159: 
Line 160:         self._acceptor = MultiProtocolAcceptor(host, port, sslctx)
Line 161:         self._prepareXMLRPCBinding(host, default_bridge)
Line 162:         self._prepareJSONRPCBinding()
So now we are left with 3 lines that can be inlined in __init__:

    self._acceptor = self._createAcceptor()
    self._prepareXMLRPCBinding()
    self._prepareJSONRPCBinding()
Line 163: 
Line 164:     def _createSSLContext(self):
Line 165:         sslctx = None
Line 166:         if config.getboolean('vars', 'ssl'):


Line 164:     def _createSSLContext(self):
Line 165:         sslctx = None
Line 166:         if config.getboolean('vars', 'ssl'):
Line 167:             truststore_path = config.get('vars', 'trust_store_path')
Line 168:             key_file = truststore_path + '/keys/vdsmkey.pem'
For creating paths, you should use os.path.join. This ensure that the path is correct without assuming that it ends or does not end with /.
Line 169:             cert_file = truststore_path + '/certs/vdsmcert.pem'
Line 170:             ca_cert = truststore_path + '/certs/cacert.pem'
Line 171:             sslctx = SSLContext(cert_file, key_file, ca_cert)
Line 172:         return sslctx


Line 178:                 from BindingXMLRPC import XmlDetector
Line 179:             except ImportError:
Line 180:                 self.log.error('Unable to load the xmlrpc server module. '
Line 181:                                'Please make sure it is installed.')
Line 182:             else:
Create default_bridge and host, here, just before your use them.
Line 183:                 xml_binding = BindingXMLRPC(self, self.log, host,
Line 184:                                             default_bridge)
Line 185:                 xml_binding.start()
Line 186:                 self.xml_detector = XmlDetector(xml_binding)


Line 179:             except ImportError:
Line 180:                 self.log.error('Unable to load the xmlrpc server module. '
Line 181:                                'Please make sure it is installed.')
Line 182:             else:
Line 183:                 xml_binding = BindingXMLRPC(self, self.log, host,
Lets add it to the bindings list:

    self._bindings.append(xml_binding)
Line 184:                                             default_bridge)
Line 185:                 xml_binding.start()
Line 186:                 self.xml_detector = XmlDetector(xml_binding)
Line 187:                 self._acceptor.add_detector(self.xml_detector)


Line 181:                                'Please make sure it is installed.')
Line 182:             else:
Line 183:                 xml_binding = BindingXMLRPC(self, self.log, host,
Line 184:                                             default_bridge)
Line 185:                 xml_binding.start()
We will start it later in start()
Line 186:                 self.xml_detector = XmlDetector(xml_binding)
Line 187:                 self._acceptor.add_detector(self.xml_detector)
Line 188: 
Line 189:     def _prepareJSONRPCBinding(self):


Line 182:             else:
Line 183:                 xml_binding = BindingXMLRPC(self, self.log, host,
Line 184:                                             default_bridge)
Line 185:                 xml_binding.start()
Line 186:                 self.xml_detector = XmlDetector(xml_binding)
This can be a temporary now, after we add it to the acceptor, it is not needed.
Line 187:                 self._acceptor.add_detector(self.xml_detector)
Line 188: 
Line 189:     def _prepareJSONRPCBinding(self):
Line 190:         if config.getboolean('vars', 'jsonrpc_enable'):


Line 196:                 self.log.warn('Unable to load the json rpc server module. '
Line 197:                               'Please make sure it is installed.')
Line 198:             else:
Line 199:                 bridge = Bridge.DynamicBridge()
Line 200:                 json_binding = BindingJsonRpc(bridge)
Lets add it to _bindings
Line 201:                 json_binding.start()
Line 202:                 self.stomp_detector = StompDetector(json_binding)
Line 203:                 self._acceptor.add_detector(self.stomp_detector)
Line 204: 


Line 197:                               'Please make sure it is installed.')
Line 198:             else:
Line 199:                 bridge = Bridge.DynamicBridge()
Line 200:                 json_binding = BindingJsonRpc(bridge)
Line 201:                 json_binding.start()
We will start it later
Line 202:                 self.stomp_detector = StompDetector(json_binding)
Line 203:                 self._acceptor.add_detector(self.stomp_detector)
Line 204: 
Line 205:     def _prepareMOM(self):


Line 198:             else:
Line 199:                 bridge = Bridge.DynamicBridge()
Line 200:                 json_binding = BindingJsonRpc(bridge)
Line 201:                 json_binding.start()
Line 202:                 self.stomp_detector = StompDetector(json_binding)
This can be a temporary
Line 203:                 self._acceptor.add_detector(self.stomp_detector)
Line 204: 
Line 205:     def _prepareMOM(self):
Line 206:         momconf = config.get("mom", "conf")


Line 234: 
Line 235:             if self.xml_detector:
Line 236:                 self.xml_detector.stop()
Line 237:             if self.stomp_detector:
Line 238:                 self.stomp_detector.stop()
Replace with:

    for binding in self._bindings:
        binding.stop()
Line 239:             self._acceptor.stop()
Line 240: 
Line 241:             self._enabled = False
Line 242:             self.channelListener.stop()


Line 235:             if self.xml_detector:
Line 236:                 self.xml_detector.stop()
Line 237:             if self.stomp_detector:
Line 238:                 self.stomp_detector.stop()
Line 239:             self._acceptor.stop()
But is better idea to stop the acceptor before the bindings, so no new connection is accepted while you stop the bindings.
Line 240: 
Line 241:             self._enabled = False
Line 242:             self.channelListener.stop()
Line 243:             self._hostStats.stop()


Line 249:                 return {'status': doneCode}
Line 250:         finally:
Line 251:             self._shutdownSemaphore.release()
Line 252: 
Line 253:     def start(self):
It will be cleaner to start the bindings here - there is no reason to start a binding before it can be used. Starting a binding and preparing it are different.

Similar to the old code:

    for binding in self._bindings:
        binding.start()
Line 254:         self.thread = threading.Thread(target=self._acceptor.serve_forever,
Line 255:                                        name='Detector thread')
Line 256:         self.thread.setDaemon(True)
Line 257:         self.thread.start()


Line 253:     def start(self):
Line 254:         self.thread = threading.Thread(target=self._acceptor.serve_forever,
Line 255:                                        name='Detector thread')
Line 256:         self.thread.setDaemon(True)
Line 257:         self.thread.start()
This is the second time I suggest to move this to Acceptor.start() - I don't think I got a reply in the first time.
Line 258: 
Line 259:     def _getUUIDSpecPath(self, uuid):
Line 260:         try:
Line 261:             return blkid.getDeviceByUuid(uuid)


http://gerrit.ovirt.org/#/c/26300/27/vdsm/protocolDetector.py
File vdsm/protocolDetector.py:

Line 101: 
Line 102:     def _cleanup(self):
Line 103:         self.log.debug("Cleaning Acceptor")
Line 104:         for handler in self._handlers:
Line 105:             handler.stop()
Stopping handlers is not needed because they are not something that you start or stop. What needs stopping is the binding/server used by the detector, and these can be stopp in clientIF.stop().
Line 106: 
Line 107:         for _, (_, client_socket) in self._pending_connections.items():
Line 108:             self._remove_connection(client_socket)
Line 109:             client_socket.close()


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id739a40e2b37dcc175137ec91cd5ec166ad24a75
Gerrit-PatchSet: 27
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Barak Azulay <bazulay at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list