Alon Bar-Lev has posted comments on this change. Change subject: fklsnr: Introduce standalone fence_kdump listener ......................................................................
Patch Set 6: (29 comments) http://gerrit.ovirt.org/#/c/27201/6/ovirt-engine.spec.in File ovirt-engine.spec.in: Line 404: Requires: %{name} = %{version}-%{release} Line 405: Requires: %{name}-lib >= %{version}-%{release} Line 406: Requires: java Line 407: Requires: logrotate Line 408: Requires: python-dateutil as far as I can see this is not required. Line 409: Requires: python-psycopg2 Line 410: Line 411: %if %{ovirt_install_systemd} Line 412: Requires(post): systemd 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 50: self._connection = None Line 51: Line 52: def __enter__(self): Line 53: self._open_connection() Line 54: return self return instance of dao? danken would have commented that with should return a different object than self :) but if no dao then ok. Line 55: Line 56: def __exit__(self, exc_type, exc_value, traceback): Line 57: self._connection.close() Line 58: 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? 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 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. dao should expose only logical methods. 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? insert into fence_kdump_messages ( vds_id, received, address, status ) values ( %(vds_id)s, %(received)s, %(address)s, %(status)s ) 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 13: # limitations under the License. Line 14: Line 15: import binascii Line 16: import datetime Line 17: import dateutil.tz no need Line 18: import gettext Line 19: import socket Line 20: import struct Line 21: 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? 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 much lower if you know the max size of packet. Line 58: self._sessions = {} Line 59: self._populate_sessions() Line 60: Line 61: def __enter__(self): Line 66: def __exit__(self, exc_type, exc_value, traceback): Line 67: self._socket.close() Line 68: Line 69: def _now(self): Line 70: return datetime.datetime.now(dateutil.tz.tzutc()) datetime.datetime.utcnow() Line 71: Line 72: def _total_seconds(self, td): Line 73: return ( Line 74: td.microseconds + (td.seconds + td.days * 24 * 3600) * 10**6 Line 71: Line 72: def _total_seconds(self, td): Line 73: return ( Line 74: td.microseconds + (td.seconds + td.days * 24 * 3600) * 10**6 Line 75: ) / 10**6 please use the magic of: time.mktime(datetime.datetime.now().timetuple()) Line 76: Line 77: def _interval_finished(self, interval, last): Line 78: return ( Line 79: last is None 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 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 handle message at state initial, here just leave the session tracking and status. 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. now... that I see this... I think you can do: PREFIX = '01020333112'.decode('hex') return ( len(message) == self._MSG_V1_SIZE and message.encode[:len(self.PREFIX)] == self.PREFIX ) 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. Line 153: Line 154: if not entry['vds_id']: Line 155: self.logger.warning( Line 156: _( Line 163: Line 164: try: Line 165: self._dao.create_fence_kdump_message( Line 166: vds_id=entry['vds_id'], Line 167: received=received, I am unsure why you cannot use now() within database, why do you care of exact time within python, all we need is trigger keep alive Line 168: address=entry['address'], Line 169: status=entry['status'], Line 170: ) Line 171: 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 database. 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 within session. also I am unsure why you drop the port. Line 186: Line 187: elif entry['status'] == self.SESSION_STATE_DUMPING: Line 188: self.logger.debug( Line 189: 'dumping %s', Line 204: self.logger.debug('Exception', exc_info=True) Line 205: Line 206: def _house_keeping_sessions(self): Line 207: for session in self._sessions.values(): Line 208: received = self._now() please move out the now from loop Line 209: Line 210: if self._interval_finished( Line 211: interval=self._sessionExpirationTime, Line 212: last=session['received'] 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 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 access database as session will be kept expired. if database is down there is no reason to continue to listening to dumps. and if you think it is that critical... we need to modify the model. instead of using the dao directly, to have a list of dao commands. and at housekeeping try to process as many of these as we can. however, this also suggest that you load into memory the entire host list with all addresses, as you cannot assume that on session initiation you will have database connection, right? Line 225: session['received'] = received Line 226: self.logger.info( Line 227: _( Line 228: "Host '{vds_id}' with address '{address}' " Line 261: try: Line 262: new_heartbeat = self._now() Line 263: Line 264: self._dao.update_heartbeat( Line 265: heartbeat=new_heartbeat, please use database now() no need to involve python time Line 266: ) Line 267: Line 268: self._lastHeartbeat = new_heartbeat Line 269: self.logger.debug('heartbeat') 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? 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. Line 286: Line 287: http://gerrit.ovirt.org/#/c/27201/6/packaging/services/ovirt-fence-kdump-listener/ovirt-fence-kdump-listener.py File packaging/services/ovirt-fence-kdump-listener/ovirt-fence-kdump-listener.py: Line 75: self._config = configfile.ConfigFile( Line 76: ( Line 77: self._defaults, Line 78: config.ENGINE_VARS, Line 79: config.ENGINE_FKLSNR_VARS, you should not mix two configurations at one load. there are defaults for one and defaults for the other, there is file for one and file for the other. Line 80: ), Line 81: ) Line 82: Line 83: self._checkInstallation( Line 88: consoleLog = open(os.devnull, "w+") Line 89: return (consoleLog, consoleLog) Line 90: Line 91: def daemonContext(self): Line 92: db_manager = db.ConnectionManager( with .... Line 93: host=self._config.get('ENGINE_DB_HOST'), Line 94: port=self._config.get('ENGINE_DB_PORT'), Line 95: database=self._config.get('ENGINE_DB_DATABASE'), Line 96: username=self._config.get('ENGINE_DB_USER'), Line 95: database=self._config.get('ENGINE_DB_DATABASE'), Line 96: username=self._config.get('ENGINE_DB_USER'), Line 97: password=self._config.get('ENGINE_DB_PASSWORD'), Line 98: secured=self._config.getboolean('ENGINE_DB_SECURED'), Line 99: secure_validation=( we do: secure_validation=self._config.geboolean( '......' ), :) Line 100: self._config.getboolean('ENGINE_DB_SECURED_VALIDATION') Line 101: ), Line 102: ) Line 103: Line 98: secured=self._config.getboolean('ENGINE_DB_SECURED'), Line 99: secure_validation=( Line 100: self._config.getboolean('ENGINE_DB_SECURED_VALIDATION') Line 101: ), Line 102: ) ) as db_manager: Line 103: Line 104: with db_manager: Line 105: dao = db.EngineDao(connection_manager=db_manager) Line 106: Line 116: config_values['FenceKdumpListenerHeartbeatInterval'] Line 117: ), Line 118: session_expiration_time=int( Line 119: config_values['KdumpFinishedTimeout'] Line 120: ), if you load these explicitly in dao, why don't you convert presentation there? Line 121: ) as server: Line 122: server.run() Line 123: Line 124: -- 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
