Gal Hammer has posted comments on this change.
Change subject: vdsm: Try to reconnect on vmchannel after errors
......................................................................
Patch Set 5: I would prefer that you didn't submit this
(5 inline comments)
....................................................
File vdsm/guestIF.py
Line 92: def _prepare_socket(self):
Line 93: supervdsm.getProxy().prepareVmChannel(self._socketName)
Line 94:
Line 95: @staticmethod
Line 96: def _setup_socket(self):
How about "_create" to match the "_connect" method?
Line 97: if hasattr(self, '_sock'):
Line 98: self._sock.close()
Line 99: self._sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
Line 100: self._sock.setblocking(0)
Line 323: # The connection is broken when recv returns no data
Line 324: # therefore we're going to set ourself to stopped
state
Line 325: if len(data) == 0:
Line 326: self._stopped = True
Line 327: break
I think you should return a value which tells the listener that the socket is
no longer in use and should be reconnected.
Line 328: self._handleData(data)
Line 329: except socket.error as err:
Line 330: if err.errno == errno.EWOULDBLOCK:
Line 331: pass
....................................................
File vdsm/vmChannels.py
Line 40: self._epoll = select.epoll()
Line 41: self._channels = {}
Line 42: self._unconnected = {}
Line 43: self._update_lock = threading.Lock()
Line 44: self._reconnect_cooldown = {}
Why not to use the _unconnected list and add a retries/timeout value?
Line 45: self._add_channels = {}
Line 46: self._del_channels = []
Line 47: self._timeout = None
Line 48:
Line 49: def _handle_event(self, fileno, event):
Line 50: """ Handle an epoll event occurred on a specific file
descriptor. """
Line 51: if (event & (select.EPOLLHUP | select.EPOLLERR)):
Line 52: self.log.error("Received EPOLLHUP on fileno %d", fileno)
Line 53: self._prepare_reconnect(fileno)
I am not familiar with the HUP/ERR errors and how they works, but since the
error is detected in the guestIF I don't think it is required here as well.
Line 54: elif (event & select.EPOLLIN):
Line 55: obj = self._channels[fileno]
Line 56: obj['reconnects'] = 0
Line 57: try:
Line 186: """ Set the timeout value (in seconds) for all channels. """
Line 187: self.log.info("Setting channels' timeout to %d seconds.",
seconds)
Line 188: self._timeout = seconds
Line 189:
Line 190: def register(self, connect_callback, setup_callback,
read_callback,
Stupid comment. I think the order should be setup (or create :-P), then
connect...
Line 191: timeout_callback, opaque):
Line 192: """ Register a new file descriptor to the listener. """
Line 193: fileno = setup_callback()
Line 194: self.log.debug("Add fileno %d to listener's channels.",
fileno)
--
To view, visit http://gerrit.ovirt.org/11977
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8b9b9418e39558d45c97e9545bc9ecc4935f004e
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vinzenz Feenstra <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Federico Simoncelli <[email protected]>
Gerrit-Reviewer: Gal Hammer <[email protected]>
Gerrit-Reviewer: Igor Lvovsky <[email protected]>
Gerrit-Reviewer: Mark Wu <[email protected]>
Gerrit-Reviewer: Michal Skrivanek <[email protected]>
Gerrit-Reviewer: Peter V. Saveliev <[email protected]>
Gerrit-Reviewer: Vinzenz Feenstra <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
