Dan Kenigsberg has posted comments on this change.
Change subject: SuperVdsm validation of recovery after crash
......................................................................
Patch Set 24: I would prefer that you didn't submit this
(4 inline comments)
....................................................
File vdsm/supervdsm.py
Line 117: misc.execCmd([constants.EXT_KILL, "-9", str(pid)],
sudo=True)
Line 118: self._cleanOldFiles()
Line 119: except Exception, ex:
Line 120: self._log.debug("Could not kill old Super Vdsm %s", ex)
Line 121: self._cleanOldFiles()
nitpick: you could call self._cleanOldFiles() once, on finally, or after the
block.
Line 122:
Line 123: self._authkey = None
Line 124: self._manager = None
Line 125: self._svdsm = None
Line 124: self._manager = None
Line 125: self._svdsm = None
Line 126:
Line 127: def isRunning(self):
Line 128: try:
I did not notice this before, but why do you have here this catch-all try-block?
it is a bad practice in general - and I think that specifically here, too.
Assume that TIMESTAMP has become unreadable due to an evil admin. Why should we
continue to recreate a new superVdsm process? it would be better to crash and
burn and report a log of the problem.
Line 129: with open(PIDFILE, "r") as f:
Line 130: spid = int(f.read().strip())
Line 131: with open(TIMESTAMP, "r") as f:
Line 132: createdTime = f.read().strip()
Line 134: if pTime == createdTime:
Line 135: return True
Line 136:
Line 137: except Exception as e:
Line 138: self._log.debug("could not read if svdsm is up: %s", e)
if for some reason you need to keep this try-block, please add exc_info=True to
have more info in the logs.
Line 139:
Line 140: return False
Line 141:
Line 142: def _connect(self):
....................................................
File vdsm/supervdsmServer.py
Line 348: servThread = threading.Thread(target=server.serve_forever)
Line 349: servThread.setDaemon(True)
Line 350: servThread.start()
Line 351:
Line 352: chown(ADDRESS, METADATA_USER, METADATA_GROUP)
nitpick: sounds like a good place to use a for-loop.
Line 353: chown(TIMESTAMP, METADATA_USER, METADATA_GROUP)
Line 354: chown(PIDFILE, METADATA_USER, METADATA_GROUP)
Line 355: log.debug("Started serving super vdsm object")
Line 356: servThread.join()
--
To view, visit http://gerrit.ovirt.org/7901
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Idad4a622b82259b777851d1b0c1b37ec8da2b01e
Gerrit-PatchSet: 24
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <[email protected]>
Gerrit-Reviewer: Barak Azulay <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Douglas Schilling Landgraf <[email protected]>
Gerrit-Reviewer: Eduardo <[email protected]>
Gerrit-Reviewer: Igor Lvovsky <[email protected]>
Gerrit-Reviewer: Royce Lv <[email protected]>
Gerrit-Reviewer: Saggi Mizrahi <[email protected]>
Gerrit-Reviewer: Shu Ming <[email protected]>
Gerrit-Reviewer: Yaniv Bronhaim <[email protected]>
Gerrit-Reviewer: Yeela Kaplan <[email protected]>
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
