Martin Peřina has posted comments on this change. Change subject: fklsnr: Introduce standalone fence_kdump listener ......................................................................
Patch Set 6: (16 comments) http://gerrit.ovirt.org/#/c/27201/6/packaging/services/ovirt-fence-kdump-listener/db.py File packaging/services/ovirt-fence-kdump-listener/db.py: Line 102: pass Line 103: return valid Line 104: Line 105: def get_connection(self): Line 106: if not self._connection_valid(): > you probably need to close connection? I tested, that if db was restarted, the connection is already marked as close, but I can close it just to be sure Line 107: self._open_connection() Line 108: Line 109: return self._connection Line 110: Line 115: Line 116: def __init__( Line 117: self, Line 118: connection_manager): Line 119: super(EngineDao, self).__init__() > ): -> next line Done Line 120: self._conn_mgr = connection_manager Line 121: Line 122: def _execute( Line 123: self, Line 122: def _execute( Line 123: self, Line 124: statement, Line 125: args=None, Line 126: ): > I would have expected _execute be on the connection class. Done Line 127: ret = [] Line 128: self.logger.debug( Line 129: "Statement: '%s', Args: {%s}", Line 130: statement, Line 191: statement=""" Line 192: INSERT INTO fence_kdump_messages (vds_id, received, address, Line 193: status) Line 194: VALUES (%(vds_id)s, %(received)s, %(address)s, Line 195: %(status)s) > can you please use the same notation of coding for sql statements? Done Line 196: """, Line 197: args={ Line 198: 'vds_id': vds_id, Line 199: 'received': received, http://gerrit.ovirt.org/#/c/27201/6/packaging/services/ovirt-fence-kdump-listener/listener.py File packaging/services/ovirt-fence-kdump-listener/listener.py: Line 32: Line 33: # fence_kdump message version 1 Line 34: _MSG_V1_SIZE = 8 Line 35: _MSG_V1_VERSION = 1 Line 36: _MSG_V1_MAGIC = 456141376 > will hex make it easier to understand? Done Line 37: Line 38: def __init__( Line 39: self, Line 40: bind, Line 53: self._sessionExpirationTime Line 54: ) Line 55: self._lastHeartbeat = None Line 56: self._lastWakeup = None Line 57: self._bufsize = 0x10000 > can you please move this also as a constant while we at it... it can be muc Done Line 58: self._sessions = {} Line 59: self._populate_sessions() Line 60: Line 61: def __enter__(self): Line 76: Line 77: def _interval_finished(self, interval, last): Line 78: return ( Line 79: last is None Line 80: or self._total_seconds(self._now() - last) >= interval > please move the or to suffix of previous line Done Line 81: ) Line 82: Line 83: def _recvfrom(self): Line 84: ret = (None, None) Line 113: else: Line 114: entry = self._sessions.get(address[0]) Line 115: if entry is None: Line 116: entry = { Line 117: 'vds_id': self._dao.get_host_with_address(address[0]), > please do not access dao here, move all application specific into the handl Done Line 118: 'status': self.SESSION_STATE_INITIAL, Line 119: 'address': address, Line 120: 'received': None, Line 121: } Line 135: return ( Line 136: message Line 137: and len(message) >= self._MSG_V1_SIZE Line 138: and _parse_number(message) == self._MSG_V1_MAGIC Line 139: and _parse_number(message, 4) == self._MSG_V1_VERSION > please put boolean conditions at end of previous line. Done Line 140: ) Line 141: Line 142: def _handle_message(self, entry, message): Line 143: if not self._verify_message(message): Line 148: msg=binascii.hexlify(message), Line 149: address=entry['address'][0], Line 150: ) Line 151: ) Line 152: return > please do not return at middle of functions, throw exception. Done Line 153: Line 154: if not entry['vds_id']: Line 155: self.logger.warning( Line 156: _( Line 169: status=entry['status'], Line 170: ) Line 171: Line 172: # record successfully saved in db, update in memory cache Line 173: entry['received'] = received > I am unsure I understand why you cannot first update memory then sync into If I updated memory first and database error occurred during update, I would have to "rollback" also memory update. So I think it's easier to update memory after db update was successful. Line 174: if entry['status'] == self.SESSION_STATE_INITIAL: Line 175: self.logger.info( Line 176: _( Line 177: "Host '{vds_id}' with address '{address}' started " Line 181: address=entry['address'][0], Line 182: ) Line 183: ) Line 184: entry['status'] = self.SESSION_STATE_DUMPING Line 185: self._sessions[entry['address'][0]] = entry > why do you need to touch _Sessions in this function? entry is already withi This is entry with initial status, it's not contained in sessions, I will save it to sessions after successful db update. Line 186: Line 187: elif entry['status'] == self.SESSION_STATE_DUMPING: Line 188: self.logger.debug( Line 189: 'dumping %s', Line 208: received = self._now() Line 209: Line 210: if self._interval_finished( Line 211: interval=self._sessionExpirationTime, Line 212: last=session['received'] > too much indent Done Line 213: ): Line 214: new_state = self.SESSION_STATE_CLOSED Line 215: try: Line 216: self._dao.create_fence_kdump_message( Line 220: status=new_state, Line 221: ) Line 222: Line 223: # record successfully saved in db, update in memory cache Line 224: session['status'] = new_state > this will only create infinite loop and make more and more attempts to acce Expired sessions are not kept, they are removed using code on lines 249-254. Well, if I presume that db will be down only for neccessery time, once it became up, all sessions will be updated. So do you suggest to get rid of this code and shutdown service when db connection was closed due to db down? Line 225: session['received'] = received Line 226: self.logger.info( Line 227: _( Line 228: "Host '{vds_id}' with address '{address}' " Line 280: # if record is saved to db, in memory cache status Line 281: # is always 'dumping' Line 282: record['status'] = self.SESSION_STATE_DUMPING Line 283: # address is stored as string, convert back to tuple Line 284: record['address'] = tuple(record['address']) > return tuple from dao, as you already have conversion there? Sorry, I forgot to remove conversion here. Line 285: self._sessions[record['address'][0]] = record Line 286: Line 287: Line 281: # is always 'dumping' Line 282: record['status'] = self.SESSION_STATE_DUMPING Line 283: # address is stored as string, convert back to tuple Line 284: record['address'] = tuple(record['address']) Line 285: self._sessions[record['address'][0]] = record > not sure why this is needed. I don't understand. Do you mean why session needs to be loaded from db or something else? Line 286: Line 287: -- To view, visit http://gerrit.ovirt.org/27201 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ieec3bad47bbba860a52a9ff4e2eb7f61277f4e36 Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Peřina <mper...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Martin Peřina <mper...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Saggi Mizrahi <smizr...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
