Martin Peřina has posted comments on this change. Change subject: fklsnr: Introduce standalone fence_kdump listener ......................................................................
Patch Set 11: (14 comments) http://gerrit.ovirt.org/#/c/27201/11/packaging/services/ovirt-fence-kdump-listener/db.py File packaging/services/ovirt-fence-kdump-listener/db.py: Line 180: try: Line 181: with contextlib.closing( Line 182: self._connection.cursor() Line 183: ) as cursor: Line 184: cursor.callproc( > please make sure that at rhel's old driver this is supported. I verified it on Centos6 with python-psycopg2-2.0.14-2 Line 185: name, Line 186: args, Line 187: ) Line 188: Line 191: while True: Line 192: entry = cursor.fetchone() Line 193: if entry is None: Line 194: break Line 195: ret.append(dict(zip(cols, entry))) > move the above to function as you reuse it now. Done Line 196: except (psycopg2.Error, psycopg2.Warning) as e: Line 197: raise DbException( Line 198: message=( Line 199: "Error calling procedure '%s'" % name Line 236: def update_vds_kdump_status(self, vds_id, status, address): Line 237: return self._db_mgr.call_procedure( Line 238: name='UpsertKdumpStatus', Line 239: args=( Line 240: vds_id, # v_vds_id > the stored procedure can find the id its-self, you should provide the addre It's very hard to parse errors from db exception. That's why I prefer to find vds_id in separate procedure. Line 241: status, # v_status Line 242: json.dumps(address), # v_address Line 243: ), Line 244: ) http://gerrit.ovirt.org/#/c/27201/11/packaging/services/ovirt-fence-kdump-listener/listener.py File packaging/services/ovirt-fence-kdump-listener/listener.py: Line 28: class FenceKdumpListener(base.Base): Line 29: # Host kdump flow states Line 30: SESSION_STATE_INITIAL = 'started' Line 31: SESSION_STATE_DUMPING = 'dumping' Line 32: SESSION_STATE_CLOSED = 'finished' > please be consistent ... CLOSED->FINISHED or finished->closed Sorry, cut&paste your code (CLOSED), but I already had defined 'finished' as last status in engine. Line 33: Line 34: # buffer size to receive message Line 35: _BUF_SIZE = 0x20 Line 36: Line 126: _( Line 127: "Discarding invalid message '{msg}' from address " Line 128: "'{address}'." Line 129: ).format( Line 130: msg=binascii.hexlify(message), > message.encode("hex") ? Is there any difference I should know? I found this on internet when I searched how to achieve it ... Line 131: address=entry['address'][0], Line 132: ) Line 133: ) Line 134: Line 129: ).format( Line 130: msg=binascii.hexlify(message), Line 131: address=entry['address'][0], Line 132: ) Line 133: ) > you should have caught the exception in this function... not at the run() I need to stop processing if message is invalid. You didn't like that I called return after I logged an error in previous patch and you wanted me to throw an exception. So now I really don't understand :-( Line 134: Line 135: # message is valid, update timestamp Line 136: entry['updated'] = datetime.datetime.utcnow() Line 137: Line 211: if session['status'] == 'finished': Line 212: del self._sessions[session['address']] Line 213: Line 214: def _load_sessions(self): Line 215: self._lastDbSync = datetime.datetime.utcnow() > this should be done part of house keeping at successful FIRST database conn Well, it's needed only at startup now (if we drop the vdc_options updates, then on 1st db connection establish). Anyway, I set updated of each session to this time, to prevent unneeded db updates on next sync ... Line 216: for record in self._dao.get_unfinished_sessions(): Line 217: session = { Line 218: 'status': record['status'], Line 219: 'address': record['address'], Line 220: 'updated': self._lastDbSync, Line 221: } Line 222: self._sessions[session['address']] = session Line 223: Line 224: def _db_sync(self): > this is part of house keeping Well, I wanted session changes to be written into db asap, so in this case right after message accepted and validated. I didn't want to wait until next house keeping Line 225: if self._db_manager.validate_connection(): Line 226: try: Line 227: self._save_heartbeat() Line 228: self._save_sessions() Line 242: _( Line 243: "Database connection is not available, synchronization " Line 244: "will be postponed." Line 245: ) Line 246: ) > this message should appear once you lost connection and an info message sho I wanted sysadmin to notice the problem and fix it asap, but OK, I will log only when db connection down detected for 1st time Line 247: Line 248: def run(self): Line 249: # load session from db Line 250: self._load_sessions() Line 268: entry=entry, Line 269: message=data, Line 270: ) Line 271: except InvalidMessage as e: Line 272: self.logger.error(e) > this will create overflow within log if someone just sends invalid messages I wanted to alert sysadmin that someone is sending non fence_kdump traffic to this port ... Line 273: Line 274: # try to sync with db Line 275: self._db_sync() Line 276: http://gerrit.ovirt.org/#/c/27201/11/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 1: # 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= > do you use these? AFAIK no, but they are part of every service, so I left them there ... I didn't have time to find out what they are for :-( Line 6: ENGINE_USR="@ENGINE_USR@" Line 7: Line 8: # Line 9: # WARNING: 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@" > you do not use ENGINE_USR Sorry, it was left by mistake here, because it's used in ovirt-fence-kdump-listener.py in line 56 ... Line 7: Line 8: # Line 9: # WARNING: Line 10: # If some of the following options are changed, engine needs to be Line 20: # Defines the IP address(es) to send fence_kdump messages to from hosts Line 21: DESTINATION_ADDRESS= Line 22: Line 23: # Defines interval in seconds between messages sent by fence_kdump Line 24: MESSAGE_INTERVAL=5 > I do not understand this one This is the parameter for fence_kdump_send how often the message should be sent from kdumping host. Default is 10 sec, but it seems to me too long, so I want 5 sec to be default in engine. It will be sent to host along with LISTENER_PORT and DESTINATION_ADDRESS during hostdeploy to configure fence_kdump in host. Line 25: Line 26: # Defines the interval in seconds of listener's heartbeat updates Line 27: HEARTBEAT_INTERVAL=5 Line 28: http://gerrit.ovirt.org/#/c/27201/11/packaging/services/ovirt-fence-kdump-listener/ovirt-fence-kdump-listener.py File packaging/services/ovirt-fence-kdump-listener/ovirt-fence-kdump-listener.py: Line 56: self._config.get("ENGINE_USR"), Line 57: "services", Line 58: ), Line 59: directory=True, Line 60: ) > so you can drop ENGINE_USR from configuration and drop this one... as you d Done Line 61: Line 62: if pidfile is not None: Line 63: self.check( Line 64: name=pidfile, -- 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: 11 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
