Alon Bar-Lev has posted comments on this change. Change subject: fklsnr: Introduce standalone fence_kdump listener ......................................................................
Patch Set 5: (27 comments) thank you so much! now I can understand the flow, implementation, termination and flow is much more simple. first pass of comments, maybe I did not understand certain things. the major issue I do not understand is why do we need to open a new database connection each time we use database and not reuse connection. http://gerrit.ovirt.org/#/c/27201/5/ovirt-engine.spec.in File ovirt-engine.spec.in: Line 295: Line 296: %description backend Line 297: The backend engine of %{ovirt_product_name_short} Line 298: Line 299: %package fence-kdump-listener any reason we want this to run on different host? if not, maybe better to merge this with tools as notifier. Line 300: Summary: %{ovirt_product_name_short} fence_kdump listener Line 301: Group: %{ovirt_product_group} Line 302: Requires: %{name}-lib >= %{version}-%{release} Line 303: Requires: python-dateutil Line 936: %ghost %config(noreplace) %{engine_pki}/serial.txt Line 937: Line 938: %files fence-kdump-listener Line 939: Line 940: %{engine_data}/services/ovirt-fence-kdump-listener please add / at suffix Line 941: %{engine_etc}/ovirt-fence-kdump-listener.conf.d/ Line 942: Line 943: %if %{ovirt_install_systemd} Line 944: %{_unitdir}/ovirt-fence-kdump-listener.service 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 pre-opened connection. 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. with DbManager.connect() as dao: use dao. drop the use of contextlib... connect will return: new FenceDao(connection) or similar. then you separate between the lowlevel connection open, execute and application specific dao, and remove the 'connection' requirement for each statement. 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 26: Line 27: # timedelta.total_seconds is included in Python >= 2.7 Line 28: _total_seconds = lambda td: ( Line 29: (td.microseconds + (td.seconds + td.days * 24 * 3600) * 10**6) / 10**6 Line 30: ) oh... sorry. please move this to class level as function. also, consider just use long(datetime.strftime("%s")) so you have seconds since 1970 instead of using timedelta Line 31: Line 32: _now = lambda: datetime.datetime.now(dateutil.tz.tzutc()) Line 33: Line 34: Line 28: _total_seconds = lambda td: ( Line 29: (td.microseconds + (td.seconds + td.days * 24 * 3600) * 10**6) / 10**6 Line 30: ) Line 31: Line 32: _now = lambda: datetime.datetime.now(dateutil.tz.tzutc()) please either put this in function or at utilities module... but avoid putting anything at global scope. Line 33: Line 34: Line 35: class FenceKdumpListener(base.Base): Line 36: # Host kdump flow states Line 41: ) = ( Line 42: 'started', Line 43: 'dumping', Line 44: 'finished', Line 45: ) if you want symbolic names... easier to read: SESSION_STATE_INITIAL = 'started' SESSION_STATE_DUMPING = 'dumping' SESSION_STATE_CLOSED = 'finished' 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 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 Line 126: _( Line 127: "Received invalid message '{msg}'" Line 128: " from host '{host}'." Line 129: ).format( Line 132: ) Line 133: ) Line 134: Line 135: def _house_keeping(self): Line 136: with contextlib.closing(self._db.connect()) as connection: not sure why you opening connections and not reuse single. Line 137: self._house_keeping_sessions(connection) Line 138: self._heartbeat(connection) Line 139: Line 140: def _parse_number(self, data, offset=0): 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. 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? 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? you should also verify message length before you start parsing. 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: valid = ( magic == self._FK_MSG_MAGIC_V1 and version == 1 ) strange that for one you have constant and for the other no :) Line 152: valid = True Line 153: return valid Line 154: Line 155: def _handle_message(self, entry): Line 155: def _handle_message(self, entry): Line 156: received = _now() Line 157: Line 158: try: Line 159: with contextlib.closing(self._db.connect()) as connection: why don't you keep connection open for the entire run? Line 160: self._db.create_fence_kdump_message( Line 161: connection=connection, Line 162: host=entry['host'], Line 163: received=received, Line 174: ) Line 175: ) Line 176: ) Line 177: entry['status'] = self.SESSION_STATE_DUMPING Line 178: self._sessions[entry['host']] = entry why do you need to modify _sessions? it is already set for you. Line 179: Line 180: elif entry['status'] == self.SESSION_STATE_DUMPING: Line 181: self.logger.debug( Line 182: 'dumping %s', Line 180: elif entry['status'] == self.SESSION_STATE_DUMPING: Line 181: self.logger.debug( Line 182: 'dumping %s', Line 183: entry, Line 184: ) update database? Line 185: except StandardError as e: Line 186: self.logger.error( Line 187: _( Line 188: "Error updating host {host} status to {status}:" 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. 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_in_progress() or something similar, to make it easier to understand that you actually access the dao. 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 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 275: record['status'] = self.SESSION_STATE_DUMPING please use the term address as in my example, per true (ip, port), unless this fence changing source port(?), then we should think of something. but even if we use only ip the term address is more correct. you can put on session whatever other information you would like, like the vdsm uuid. Line 276: sessions[record['host']] = record Line 277: Line 278: return sessions Line 279: 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. Line 279: Line 280: http://gerrit.ovirt.org/#/c/27201/5/packaging/services/ovirt-fence-kdump-listener/ovirt-fence-kdump-listener.conf.in File packaging/services/ovirt-fence-kdump-listener/ovirt-fence-kdump-listener.conf.in: Line 2: # This is a default configuration file for oVirt/RHEV-M fence_kdump listener Line 3: # Line 4: TRACE_ENABLE=False Line 5: TRACE_FILE= Line 6: ENGINE_USR="@ENGINE_USR@" at least two are missing: 1. listen port 2. listen address, default 0.0.0.0 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. 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 example. Line 98: secure_validation=( Line 99: self._config.get("ENGINE_DB_SECURED_VALIDATION") == 'True' Line 100: ), Line 101: ) 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', Line 98: secure_validation=( Line 99: self._config.get("ENGINE_DB_SECURED_VALIDATION") == 'True' please do not compare to 'True', use self._config.getboolean() Line 100: ), Line 101: ) Line 102: Line 103: config_values = db_manager.load_config_values() http://gerrit.ovirt.org/#/c/27201/5/packaging/services/ovirt-fence-kdump-listener/ovirt-fence-kdump-listener.systemd.in File packaging/services/ovirt-fence-kdump-listener/ovirt-fence-kdump-listener.systemd.in: Line 5: Type=notify Line 6: User=@ENGINE_USER@ Line 7: Group=@ENGINE_GROUP@ Line 8: LimitNOFILE=65535 Line 9: LimitNPROC=2048 you do not need the above two Line 10: ExecStart=@ENGINE_USR@/services/ovirt-fence-kdump-listener/ovirt-fence-kdump-listener.py --systemd=notify $EXTRA_ARGS start Line 11: EnvironmentFile=-/etc/sysconfig/ovirt-fence-kdump-listener Line 12: Line 13: [Install] http://gerrit.ovirt.org/#/c/27201/5/packaging/services/ovirt-fence-kdump-listener/ovirt-fence-kdump-listener.sysv.in File packaging/services/ovirt-fence-kdump-listener/ovirt-fence-kdump-listener.sysv.in: Line 33: exit 4 Line 34: fi Line 35: echo -n $"Starting $PROG: " Line 36: ulimit -n ${FILENO:-65535} Line 37: ulimit -n ${NPROC:-2048} you do not need the above two Line 38: touch "${PIDFILE}" Line 39: chown "${USER}" "${PIDFILE}" Line 40: daemon --user "${USER}" --pidfile="${PIDFILE}" \ Line 41: "${ENGINE_USR}/services/${NAME}/${NAME}.py" \ -- 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
