Martin Peřina has posted comments on this change. Change subject: fklsnr: Introduce standalone fence_kdump listener ......................................................................
Patch Set 5: (14 comments) http://gerrit.ovirt.org/#/c/27201/5/packaging/services/ovirt-fence-kdump-listener/db.py File packaging/services/ovirt-fence-kdump-listener/db.py: Line 114: Line 115: def load_config_values(self): Line 116: config_values = {} Line 117: Line 118: with contextlib.closing(self.connect()) as connection: > please be consistent... if you have connect() then all functions should use Done Line 119: for name in ( Line 120: 'FenceKdumpListenerHost', Line 121: 'FenceKdumpListenerPort', Line 122: 'FenceKdumpListenerHeartbeatInterval', Line 149: config_values, Line 150: ) Line 151: return config_values Line 152: Line 153: def create_fence_kdump_message(self, connection, host, received, status): > better to have dao class that holds connection and has all these methods. OK, with above, just one question: If I use something like this: with DbManager.connect() as dao: ... with listener.FenceKdumpListener( dao=dao, ... and use only one connection for listener (without closing and reopening), how to achieve that connection will be reopened (for example after db restart) and listener will continue to serve once db connection is available again? Line 154: self.execute( Line 155: connection=connection, Line 156: statement=""" Line 157: INSERT INTO fence_kdump_messages (host, received, status) http://gerrit.ovirt.org/#/c/27201/5/packaging/services/ovirt-fence-kdump-listener/listener.py File packaging/services/ovirt-fence-kdump-listener/listener.py: Line 41: ) = ( Line 42: 'started', Line 43: 'dumping', Line 44: 'finished', Line 45: ) > if you want symbolic names... easier to read: Done Line 46: Line 47: # Magic number for fence_kdump message version 1 Line 48: _FK_MSG_MAGIC_V1 = 456141376 Line 49: Line 110: (data, address) = self._recvfrom() Line 111: if address is None: Line 112: self._house_keeping() Line 113: else: Line 114: if self._verify_message(data): > this should go into _handle_message - one place to manage the message Done Line 115: entry = self._sessions.get(address[0]) Line 116: if entry is None: Line 117: entry = { Line 118: 'status': self.SESSION_STATE_INITIAL, Line 121: } Line 122: self._handle_message(entry) Line 123: Line 124: else: Line 125: self.logger.warning( > same Done Line 126: _( Line 127: "Received invalid message '{msg}'" Line 128: " from host '{host}'." Line 129: ).format( Line 136: with contextlib.closing(self._db.connect()) as connection: Line 137: self._house_keeping_sessions(connection) Line 138: self._heartbeat(connection) Line 139: Line 140: def _parse_number(self, data, offset=0): > this can be inner function of _verify_message. Done Line 141: number = None Line 142: if data is not None: Line 143: number = struct.unpack_from("<I", data, offset)[0] Line 144: return number Line 140: def _parse_number(self, data, offset=0): Line 141: number = None Line 142: if data is not None: Line 143: number = struct.unpack_from("<I", data, offset)[0] Line 144: return number > why calling parse if data is None? Done Line 145: Line 146: def _verify_message(self, message): Line 147: valid = False Line 148: if message: Line 144: return number Line 145: Line 146: def _verify_message(self, message): Line 147: valid = False Line 148: if message: > why calling if message is nothing? Done Line 149: magic = self._parse_number(message) Line 150: version = self._parse_number(message, 4) Line 151: if magic == self._FK_MSG_MAGIC_V1 and version == 1: Line 152: valid = True Line 147: valid = False Line 148: if message: Line 149: magic = self._parse_number(message) Line 150: version = self._parse_number(message, 4) Line 151: if magic == self._FK_MSG_MAGIC_V1 and version == 1: > should be: Done Line 152: valid = True Line 153: return valid Line 154: Line 155: def _handle_message(self, entry): Line 226: except StandardError as e: Line 227: self.logger.error( Line 228: _( Line 229: "Error updating host {host} status to {status}:" Line 230: " {error}." > we usually put the spaces as trailing spaces. Done Line 231: ).format( Line 232: host=session['host'], Line 233: status=new_state, Line 234: error=e, Line 266: ) Line 267: self.logger.debug('Exception', exc_info=True) Line 268: Line 269: def _init_sessions(self): Line 270: sessions = {} > I would have set the _sessions for {} and rename this function to _populate Done Line 271: Line 272: for record in self._db.get_unfinished_flows(): Line 273: # if record is saved to db, in memory cache status Line 274: # is always 'dumping' Line 274: # is always 'dumping' Line 275: record['status'] = self.SESSION_STATE_DUMPING Line 276: sessions[record['host']] = record Line 277: Line 278: return sessions > no need to return sessions, you can modify object state as any member. Done Line 279: Line 280: http://gerrit.ovirt.org/#/c/27201/5/packaging/services/ovirt-fence-kdump-listener/ovirt-fence-kdump-listener.py File packaging/services/ovirt-fence-kdump-listener/ovirt-fence-kdump-listener.py: Line 88: return (consoleLog, consoleLog) Line 89: Line 90: def daemonContext(self): Line 91: db_manager = db.DbManager( Line 92: host=self._config.get("ENGINE_DB_HOST"), > please attempt to use single quotes all over. Done Line 93: port=self._config.get("ENGINE_DB_PORT"), Line 94: database=self._config.get("ENGINE_DB_DATABASE"), Line 95: username=self._config.get("ENGINE_DB_USER"), Line 96: password=self._config.get("ENGINE_DB_PASSWORD"), Line 93: port=self._config.get("ENGINE_DB_PORT"), Line 94: database=self._config.get("ENGINE_DB_DATABASE"), Line 95: username=self._config.get("ENGINE_DB_USER"), Line 96: password=self._config.get("ENGINE_DB_PASSWORD"), Line 97: secured=self._config.get("ENGINE_DB_SECURED") == 'True', > do you want to load the engine config as well? see the notifier service for I don't understand your comment. From engine service configuration I just need database connection setup ... Line 98: secure_validation=( Line 99: self._config.get("ENGINE_DB_SECURED_VALIDATION") == 'True' Line 100: ), Line 101: ) -- 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: 5 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
