Change in vdsm[master]: SuperVdsm validation of recovery after crash

danken at redhat.com danken at redhat.com
Wed Oct 17 11:10:05 UTC 2012


Dan Kenigsberg has posted comments on this change.

Change subject: SuperVdsm validation of recovery after crash
......................................................................


Patch Set 26: I would prefer that you didn't submit this

(3 inline comments)

....................................................
File vdsm/supervdsm.py
Line 87:     A wrapper around all the supervdsm init stuff
Line 88:     """
Line 89:     _log = logging.getLogger("SuperVdsmProxy")
Line 90:     proxyLock = threading.Lock()
Line 91:     firstLaunch = True
I do not understand why you need this flag - and certainly not why it is class-wide. it should have been an instance variable.
Line 92: 
Line 93:     def open(self, *args, **kwargs):
Line 94:         return self._manager.open(*args, **kwargs)
Line 95: 


Line 130:                 createdTime = f.read().strip()
Line 131:         except IOError as e:
Line 132:             # pid file and timestamp file must be exist after first launch,
Line 133:             # otherwise excpetion will be raised to svdsm caller
Line 134:             if e.errno == ENOENT and self.firstLaunch:
it is expected that the files would be missing also after kill(), not only in firstLaunch.
Line 135:                 return False
Line 136: 
Line 137:         try:
Line 138:             pTime = str(misc.getProcCtime(spid))


Line 136: 
Line 137:         try:
Line 138:             pTime = str(misc.getProcCtime(spid))
Line 139:         except OSError:
Line 140:             # Means pid is not exist, svdsm was killed
I think you are not specific enough. OSError can mean a lot of other thing. You should catch only ESRCH.
Line 141:             return False
Line 142: 
Line 143:         if pTime == createdTime:
Line 144:             return True


--
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: 26
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Barak Azulay <bazulay at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland at redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw at redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky at redhat.com>
Gerrit-Reviewer: Royce Lv <lvroyce at linux.vnet.ibm.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Shu Ming <shuming at linux.vnet.ibm.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan at redhat.com>


More information about the vdsm-patches mailing list