Alon Bar-Lev has posted comments on this change. Change subject: fklsnr: Introduce standalone fence_kdump listener ......................................................................
Patch Set 10: (21 comments) http://gerrit.ovirt.org/#/c/27201/10/packaging/services/ovirt-fence-kdump-listener/db.py File packaging/services/ovirt-fence-kdump-listener/db.py: Line 194: Line 195: class EngineDao(base.Base): Line 196: """DAO to access ovirt-engine database""" Line 197: # UUID of heartbeat record Line 198: _HEARTBEAT_ID = '11111111-1111-1111-1111-111111111111' this should not be a special case of uuid or we cannot have foreign key. please put it in vdc_options or something. Line 199: Line 200: def __init__( Line 201: self, Line 202: db_manager Line 219: 'version': version, Line 220: }, Line 221: ) Line 222: Line 223: def update_host_kdump_status(self, session): please do not move session structure dao, but scalar values. Line 224: # address needs to be converted from tuple to string Line 225: args = copy.copy(session) Line 226: args['address'] = json.dumps(args['address']) Line 227: Line 222: Line 223: def update_host_kdump_status(self, session): Line 224: # address needs to be converted from tuple to string Line 225: args = copy.copy(session) Line 226: args['address'] = json.dumps(args['address']) this ugly in place conversions are what happens when you share state between logic and dao, please avoid. Line 227: Line 228: rowcount = self._db_mgr.execute_update( Line 229: statement=""" Line 230: UPDATE vds_kdump_status Line 230: UPDATE vds_kdump_status Line 231: SET Line 232: status = %(status)s, Line 233: address = %(address)s, Line 234: updated = timezone('UTC', %(updated)s) please remove timestamp Line 235: WHERE vds_id = %(vds_id)s Line 236: """, Line 237: args=args, Line 238: ) Line 231: SET Line 232: status = %(status)s, Line 233: address = %(address)s, Line 234: updated = timezone('UTC', %(updated)s) Line 235: WHERE vds_id = %(vds_id)s we would like to avoid vds_id in memory cache to allow accepting session when database is down. so here you need: 1. query vds_id out of address. 2. update or insert. Line 236: """, Line 237: args=args, Line 238: ) Line 239: Line 258: Line 259: def update_heartbeat(self): Line 260: self._db_mgr.execute_update( Line 261: statement=""" Line 262: UPDATE vds_kdump_status please find different location to store heartbeat. Line 263: SET updated = now() Line 264: WHERE vds_id = %(heartbeat_id)s Line 265: """, Line 266: args={ Line 278: timezone('UTC', updated) as updated Line 279: FROM vds_kdump_status Line 280: WHERE Line 281: status <> 'finished' AND Line 282: vds_id <> %(heartbeat_id)s these ugly statements happens when you have exceptions for something that belongs elsewhere Line 283: """, Line 284: args={ Line 285: 'heartbeat_id': self._HEARTBEAT_ID Line 286: }, Line 335: if self._sessions_to_sync: Line 336: now = datetime.datetime.utcnow() Line 337: Line 338: # update db state for all updated sessions Line 339: for session in self._sessions_to_sync.values(): this loop should be at logic not at dao to avoid exposing the internal caches to dao. dao should be simple without logic. Line 340: if session['updated'] > self._last_sync: Line 341: if session.get('vds_id') is None: Line 342: # find vds_id for IP address Line 343: session['vds_id'] = self._dao.get_vds_id( Line 341: if session.get('vds_id') is None: Line 342: # find vds_id for IP address Line 343: session['vds_id'] = self._dao.get_vds_id( Line 344: session['address'][0], Line 345: ) so we skip this if database is down and keep trying to fetch vds_id if it is None? why is this better than having stored procedure to have ip, port and determine vds-id every time we try to update session? Line 346: Line 347: if session['vds_id'] is None: Line 348: self.logger.error( Line 349: _( Line 374: 'LISTENER_PORT': 'FenceKdumpDestinationPort', Line 375: 'DESTINATION_ADDRESS': 'FenceKdumpDestinationAddress', Line 376: 'MESSAGE_INTERVAL': 'FenceKdumpMessageInterval', Line 377: 'LISTENER_ALIVE_TIMEOUT': 'FenceKdumpListenerAliveTimeout', Line 378: 'KDUMP_STARTED_TIMEOUT': 'KdumpStartedTimeout', MESSAGE_INTERVAL and KDUMP_STARTED_TIMEOUT belongs to engine configuration as far as I understand. the only shared are listener port and destination address, the later is also engine only, but I gave up on that, and I do think that port should not be shared as well, as if it non standard the engine admin can set it as well. but for the other two, I do not think there is a question. Line 379: }: Line 380: self._dao.save_config_value( Line 381: option[1], Line 382: config.get(option[0]) Line 399: def synchronize(self): Line 400: if self._db_manager.validate_connection(): Line 401: try: Line 402: self._update_heartbeat() Line 403: self._sync_sessions() we need to sync sessions only if database was previously unavailable. so I would like to see explicit handling of database re-establish and the logic resulting. Line 404: except (psycopg2.Error, psycopg2.Warning) as e: Line 405: self.logger.error( Line 406: _( Line 407: "Error during synchronization with database, " Line 416: _( Line 417: "Database connection is not available, synchronization " Line 418: "will be postponed." Line 419: ) Line 420: ) please try shortest/trivial block first in conditionals. and I do not see any reason why not just try to run the commands and get exception per above handling... so you always attempt to access database and if fail you go into retry phase. BTW: the retry phase should have its own retry time... but we will discuss this once the logic is clean. Line 421: Line 422: http://gerrit.ovirt.org/#/c/27201/10/packaging/services/ovirt-fence-kdump-listener/listener.py File packaging/services/ovirt-fence-kdump-listener/listener.py: Line 127: msg=binascii.hexlify(message), Line 128: address=entry['address'][0], Line 129: ) Line 130: ) Line 131: return please do not return at middle of functions, I think multiple commented here, do not mixed procedural and exceptional within single function. throw an exception when there is an error. Line 132: Line 133: # message is valid, update timestamp Line 134: entry['updated'] = datetime.datetime.utcnow() Line 135: Line 134: entry['updated'] = datetime.datetime.utcnow() Line 135: Line 136: # if it's initial message, store it to sessions Line 137: if entry['status'] == self.SESSION_STATE_INITIAL: Line 138: self._sessions[entry['address']] = entry this should be in run to be consistent and always have a session establish at communications layer Line 139: self.logger.info( Line 140: _( Line 141: "Host '{address}' started kdump flow." Line 142: ).format( Line 177: self._db_sync.schedule_heartbeat() Line 178: Line 179: def _populate_sessions(self): Line 180: if self._db_sync.get_synced_sessions() is not None: Line 181: self._sessions = self._db_sync.get_synced_sessions() again... you exposing the internal structure of sessions to dao layer which is incorrect. Line 182: Line 183: def run(self): Line 184: while True: Line 185: (data, address) = self._recvfrom() Line 191: entry = { Line 192: 'status': self.SESSION_STATE_INITIAL, Line 193: 'address': address, Line 194: 'updated': None, Line 195: } here you should put in sessions. Line 196: self._handle_message( Line 197: entry=entry, Line 198: message=data, Line 199: ) Line 198: message=data, Line 199: ) Line 200: Line 201: # try to sync with db Line 202: self._db_sync.schedule_sessions_sync(self._sessions) here you expose the internal session structure to dao layer which is incorrect. Line 203: self._db_sync.synchronize() Line 204: self._populate_sessions() Line 205: Line 206: Line 200: Line 201: # try to sync with db Line 202: self._db_sync.schedule_sessions_sync(self._sessions) Line 203: self._db_sync.synchronize() Line 204: self._populate_sessions() please move the above to housekeeping. Line 205: Line 206: http://gerrit.ovirt.org/#/c/27201/10/packaging/services/ovirt-fence-kdump-listener/ovirt-fence-kdump-listener.py File packaging/services/ovirt-fence-kdump-listener/ovirt-fence-kdump-listener.py: Line 115: db_sync = db.DbSynchronizer( Line 116: db_manager=db_manager, Line 117: ) Line 118: Line 119: # db connection is necessary for startup this not true. Line 120: db_sync.update_vdc_options(self._config) Line 121: Line 122: db_sync.schedule_heartbeat() Line 123: db_sync.synchronize() Line 116: db_manager=db_manager, Line 117: ) Line 118: Line 119: # db connection is necessary for startup Line 120: db_sync.update_vdc_options(self._config) if database is down during startup this cannot be done, please move this into housekeeping. Line 121: Line 122: db_sync.schedule_heartbeat() Line 123: db_sync.synchronize() Line 124: db_sync.load_sessions() Line 120: db_sync.update_vdc_options(self._config) Line 121: Line 122: db_sync.schedule_heartbeat() Line 123: db_sync.synchronize() Line 124: db_sync.load_sessions() these should be done every time connection re-established. Line 125: Line 126: with listener.FenceKdumpListener( Line 127: bind=( Line 128: self._config.get('LISTENER_ADDRESS'), -- 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: 10 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
