[Engine-patches] Change in ovirt-engine[master]: fklsnr: Introduce standalone fence_kdump listener
mperina at redhat.com
mperina at redhat.com
Wed May 21 11:34:34 UTC 2014
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 <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
More information about the Engine-patches
mailing list