Alon Bar-Lev has posted comments on this change. Change subject: core: Introduce standalone fence_kdump listener ......................................................................
Patch Set 3: (31 comments) missing: spec changes, sysv, systemd http://gerrit.ovirt.org/#/c/27201/3//COMMIT_MSG Commit Message: Line 3: AuthorDate: 2014-04-29 12:55:41 +0200 Line 4: Commit: Martin Perina <[email protected]> Line 5: CommitDate: 2014-04-29 16:56:31 +0200 Line 6: Line 7: core: Introduce standalone fence_kdump listener this is not core. this is the fence_kdump... :) Line 8: Line 9: Introduces core parts of standalone fence_kdump listener. Line 10: Line 11: Change-Id: Ieec3bad47bbba860a52a9ff4e2eb7f61277f4e36 http://gerrit.ovirt.org/#/c/27201/3/Makefile File Makefile: Line 180: packaging/services/ovirt-websocket-proxy/config.py \ Line 181: packaging/services/ovirt-websocket-proxy/ovirt-websocket-proxy.conf \ Line 182: packaging/services/ovirt-websocket-proxy/ovirt-websocket-proxy.systemd \ Line 183: packaging/services/ovirt-websocket-proxy/ovirt-websocket-proxy.sysv \ Line 184: packaging/services/ovirt-fence-kdump-listener/config.py \ please sort block Line 185: packaging/setup/bin/ovirt-engine-setup.env \ Line 186: packaging/setup/ovirt_engine_setup/config.py \ Line 187: packaging/sys-etc/logrotate.d/ovirt-engine \ Line 188: packaging/sys-etc/logrotate.d/ovirt-engine-notifier \ http://gerrit.ovirt.org/#/c/27201/3/packaging/services/ovirt-fence-kdump-listener/config.py.in File packaging/services/ovirt-fence-kdump-listener/config.py.in: Line 1: # Copyright 2012 Red Hat please fix in all files, please add to all. Line 2: # Line 3: # Licensed under the Apache License, Version 2.0 (the "License"); Line 4: # you may not use this file except in compliance with the License. Line 5: # You may obtain a copy of the License at http://gerrit.ovirt.org/#/c/27201/3/packaging/services/ovirt-fence-kdump-listener/db.py File packaging/services/ovirt-fence-kdump-listener/db.py: Line 1: import psycopg2 please restrict use if sql to one class Line 2: Line 3: import logbase Line 4: Line 5: Line 1: import psycopg2 Line 2: two spaces, but you probably don't need logbase Line 3: import logbase Line 4: Line 5: Line 6: @logbase.Logable() Line 2: Line 3: import logbase Line 4: Line 5: Line 6: @logbase.Logable() please use the base.Base as base for all objects Line 7: class Manager(object): Line 8: def __init__( Line 9: self, Line 10: host, Line 15: secured, Line 16: secure_validation, Line 17: ): Line 18: self._db_dsn = ( Line 19: "host=%s port=%s dbname=%s user=%s password=%s sslmode=%s" % ( please see[1], and Statement as a while of how to create and use connection that will work on fedora and rhel, with password having special chars etc.. [1] packaging/setup/ovirt_engine_setup/database.py::Statement::execute Line 20: host, Line 21: port, Line 22: database, Line 23: username, Line 26: ) Line 27: ) Line 28: self._config = {} Line 29: Line 30: @staticmethod you do not really need staticmethod for private functions unless there is a goo reason to. Line 31: def _get_ssl_mode(secured, secure_validation): Line 32: sslmode = "disable" Line 33: if secured == "True": Line 34: if secure_validation == "True": Line 76: " AND version=%(version)s", Line 77: { Line 78: 'name': name, Line 79: 'version': version, Line 80: }, please use format similar to engine's database.py both for python and sql statements. this should be: cur.execute( """ select option_value from vdc_options where option_name=$(name)s and version=%(version)s """, { ... }, ) but again, I suggest you use the Statement wrapper so you enjoy the cursor to dictionary conversion. Line 81: ) Line 82: records = cur.fetchmany() Line 83: return records[0][0] if records and records[0] else None Line 84: Line 82: records = cur.fetchmany() Line 83: return records[0][0] if records and records[0] else None Line 84: Line 85: def init_config(self): Line 86: conn = self.open_connection() please do not repeat code... if you must to load many values... for x in ( { 'name': 'NextKdumpTimeout', 'type': int, }, ): ... please also try to use single quotes as our other python files within ovirt-engine. Line 87: self._config["NextKdumpTimeout"] = int( Line 88: Manager._load_config_value( Line 89: conn, Line 90: "NextKdumpTimeout", Line 128: if connection is not None: Line 129: connection.close() Line 130: except (psycopg2.Warning, psycopg2.Error) as e: Line 131: self._logger.error( Line 132: "Error closing connection: %s", please always use gettext for messages that are not for debug. Line 133: e, Line 130: except (psycopg2.Warning, psycopg2.Error) as e: Line 131: self._logger.error( Line 132: "Error closing connection: %s", Line 133: e, Line 134: ) please error exception string and debug the stack dump http://gerrit.ovirt.org/#/c/27201/3/packaging/services/ovirt-fence-kdump-listener/flowterminator.py File packaging/services/ovirt-fence-kdump-listener/flowterminator.py: Line 1: import datetime Line 2: import dateutil.tz unless good reason, import only parent package Line 3: import psycopg2 Line 4: import threading Line 5: import time Line 6: Line 8: import logbase Line 9: Line 10: Line 11: @logbase.Logable() Line 12: class KdumpFlowTerminator(threading.Thread): there is no need to use thread, please use async io. Line 13: """Creates record with status "finished" for finished kdump flows""" Line 14: Line 15: def __init__(self, queue, db_manager): Line 16: super(KdumpFlowTerminator, self).__init__() Line 51: def run(self): Line 52: conn = None Line 53: while True: Line 54: try: Line 55: conn = self._db_manager.open_connection() please use single connection for the entire application. Line 56: Line 57: finished = datetime.datetime.now(dateutil.tz.tzlocal()) Line 58: records = KdumpFlowTerminator._get_unfinished_flows(conn) Line 59: for record in records: Line 53: while True: Line 54: try: Line 55: conn = self._db_manager.open_connection() Line 56: Line 57: finished = datetime.datetime.now(dateutil.tz.tzlocal()) always use utc, never localtime, unless you present something to user. Line 58: records = KdumpFlowTerminator._get_unfinished_flows(conn) Line 59: for record in records: Line 60: if ( Line 61: record["received"] + self.kdump_finished_timeout Line 58: records = KdumpFlowTerminator._get_unfinished_flows(conn) Line 59: for record in records: Line 60: if ( Line 61: record["received"] + self.kdump_finished_timeout Line 62: < finished do not split boolean operator, use: if finished > ( x + y ): if must be split Line 63: ): Line 64: msg = db.Manager.params2message( Line 65: record["host"], Line 66: finished, Line 80: finally: Line 81: self._db_manager.close_connection(conn) Line 82: conn = None Line 83: Line 84: time.sleep(30) always have parameters for these, even invent vdc_option and set 30 as default, not that I can understand what 30 is... seems very long. http://gerrit.ovirt.org/#/c/27201/3/packaging/services/ovirt-fence-kdump-listener/heartbeat.py File packaging/services/ovirt-fence-kdump-listener/heartbeat.py: Line 7: import logbase Line 8: Line 9: Line 10: @logbase.Logable() Line 11: class Heartbeat(threading.Thread): please do not use threads but async io Line 12: """Periodically updates received to current time for record Line 13: with host 'fence-kdump-listener'""" Line 14: Line 15: def __init__(self, db_manager): Line 23: cur.execute( Line 24: "UPDATE fence_kdump_messages" Line 25: " SET received = %s" Line 26: " WHERE host = 'fence_kdump_listener'", Line 27: (timestamp, ), please always use dictionary Line 28: ) Line 29: connection.commit() Line 30: cur.close() Line 31: Line 32: def run(self): Line 33: conn = None Line 34: while True: Line 35: try: Line 36: conn = self._db_manager.open_connection() please use single connection for the entire application. Line 37: Line 38: Heartbeat._update_heartbeat( Line 39: conn, Line 40: datetime.datetime.now(dateutil.tz.tzlocal()), http://gerrit.ovirt.org/#/c/27201/3/packaging/services/ovirt-fence-kdump-listener/listener.py File packaging/services/ovirt-fence-kdump-listener/listener.py: Line 20: return number Line 21: Line 22: def handle(self): Line 23: data = self.request[0].strip() Line 24: received = datetime.datetime.now(dateutil.tz.tzlocal()) please never use local time Line 25: self._logger.debug( Line 26: ( Line 27: "Received message '%s' from host='%s' at '%s'." Line 28: ), Line 27: "Received message '%s' from host='%s' at '%s'." Line 28: ), Line 29: binascii.hexlify(data), Line 30: self.client_address[0], Line 31: received, you can skip this, as the logger will print timestamp anyway. Line 32: ) Line 33: Line 34: magic = self._parse_number(data) Line 35: version = self._parse_number(data, 4) Line 28: ), Line 29: binascii.hexlify(data), Line 30: self.client_address[0], Line 31: received, Line 32: ) please insert info messages for dump begin and dump end. Line 33: Line 34: magic = self._parse_number(data) Line 35: version = self._parse_number(data, 4) Line 36: Line 36: Line 37: if magic == self._FK_MSG_MAGIC_V1 and version == 1: Line 38: msg = db.Manager.params2message( Line 39: self.client_address[0], Line 40: received, never put in database local time Line 41: None, Line 42: ) Line 43: self._logger.debug( Line 44: "Message '%s' sent to queue.", Line 55: port, Line 56: queue, Line 57: ): Line 58: self._queue = queue Line 59: self._server = SocketServer.ThreadingUDPServer( there is no need to use threads Line 60: (host, port), Line 61: FenceKdumpRequestHandler, Line 62: ) Line 63: self._server.queue = self._queue http://gerrit.ovirt.org/#/c/27201/3/packaging/services/ovirt-fence-kdump-listener/logbase.py File packaging/services/ovirt-fence-kdump-listener/logbase.py: Line 1: import logging this file should be removed, please use the Base.base Line 2: Line 3: Line 4: _LOG_NAME = 'ovirt-fence-kdump-listener' Line 5: Line 10: setattr(target, "_logger", _logger) Line 11: return target Line 12: Line 13: Line 14: def setup_logger(filename, verbose=False): this already being taken care in[1] [1] packaging/pythonlib/ovirt_engine/service.py::setupLogger Line 15: """Configures logging to specified filename""" Line 16: logger = logging.getLogger(_LOG_NAME) Line 17: logger.setLevel(logging.DEBUG if verbose else logging.INFO) Line 18: ch = logging.FileHandler(filename) http://gerrit.ovirt.org/#/c/27201/3/packaging/services/ovirt-fence-kdump-listener/ovirt-fence-kdump-listener.py File packaging/services/ovirt-fence-kdump-listener/ovirt-fence-kdump-listener.py: Line 15: # limitations under the License. Line 16: Line 17: import gettext Line 18: import os Line 19: import Queue you can use plain list if you do not use threading Line 20: import sys Line 21: _ = lambda m: gettext.dgettext(message=m, domain="ovirt-engine") Line 22: Line 23: http://gerrit.ovirt.org/#/c/27201/3/packaging/services/ovirt-fence-kdump-listener/qconsumer.py File packaging/services/ovirt-fence-kdump-listener/qconsumer.py: Line 23: ) Line 24: self._db_conn = None Line 25: Line 26: def _create_message(self, conn, message): Line 27: cur = conn.cursor() please use our Statement and avoid using the psycoqpg2 all over Line 28: cur.execute( Line 29: "INSERT INTO fence_kdump_messages (host, received, status)" Line 30: " VALUES (%(host)s, %(received)s, %(status)s)", Line 31: message, Line 90: e, Line 91: ) Line 92: finally: Line 93: if self._queue.empty: Line 94: self._db_manager.close_connection(conn) not sure why this class is required, why queues are required... it should be so simple... you wait for package, you receive packet, you update the database. -- 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: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Peřina <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Barak Azulay <[email protected]> Gerrit-Reviewer: Eli Mesika <[email protected]> Gerrit-Reviewer: Martin Peřina <[email protected]> Gerrit-Reviewer: Oved Ourfali <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
