[Engine-patches] Change in ovirt-engine[master]: fklsnr: Introduce standalone fence_kdump listener

[Alon Bar-Lev]

Alon Bar-Lev has posted comments on this change.

Change subject: fklsnr: Introduce standalone fence_kdump listener
......................................................................


Patch Set 11:

(6 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 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
> It's very hard to parse errors from db exception. That's why I prefer to fi
return # of lines modified or a value, not an error.
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 126:                 _(
Line 127:                     "Discarding invalid message '{msg}' from address "
Line 128:                     "'{address}'."
Line 129:                 ).format(
Line 130:                     msg=binascii.hexlify(message),
> Is there any difference I should know? I found this on internet when I sear
less dependencies makes me happy.
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:             )
> I need to stop processing if message is invalid. You didn't like that I cal
you should have:

 try:
     function body, throw an excpetion if error
 catch E:
     move entry to CLOSED state

all within the same function
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()
> Well, it's needed only at startup now (if we drop the vdc_options updates, 
we need this at fist database connection establish, so we can start without database
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):
> Well, I wanted session changes to be written into db asap, so in this case 
why? what's the rush?

the entire point was to relief load from database, even if we get 100000000 packets per second we persist once in 5 seconds or so.
Line 225:         if self._db_manager.validate_connection():
Line 226:             try:
Line 227:                 self._save_heartbeat()
Line 228:                 self._save_sessions()


Line 268:                         entry=entry,
Line 269:                         message=data,
Line 270:                     )
Line 271:                 except InvalidMessage as e:
Line 272:                     self.logger.error(e)
> I wanted to alert sysadmin that someone is sending non fence_kdump traffic 
yes, but you end up with filling his filesystem, causing server to halt.
Line 273: 
Line 274:             # try to sync with db
Line 275:             self._db_sync()
Line 276: 


-- 
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 <mperina at redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alonbl at redhat.com>
Gerrit-Reviewer: Barak Azulay <bazulay at redhat.com>
Gerrit-Reviewer: Eli Mesika <emesika at redhat.com>
Gerrit-Reviewer: Martin Peřina <mperina at redhat.com>
Gerrit-Reviewer: Oved Ourfali <oourfali at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes