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

danken at redhat.com danken at redhat.com
Mon Oct 15 21:07:50 UTC 2012


Dan Kenigsberg has posted comments on this change.

Change subject: super vdsm validation of recovery after crash
......................................................................


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

(11 inline comments)

....................................................
File tests/superVdsmTests.py
Line 7: 
Line 8:     def setUp(self):
Line 9:         self._proxy = supervdsm.getProxy()
Line 10: 
Line 11:         # changing default constants for test. remember to return the origin
you have monkeypatch.py for exactly this purpose. (but you do not HAVE to use it if you don't want to).
Line 12:         # value
Line 13:         self._originpidfile = self._proxy.pidfile
Line 14:         self._origintimestempfile = self._proxy.timestamp
Line 15:         self._originsock = self._proxy.address


Line 14:         self._origintimestempfile = self._proxy.timestamp
Line 15:         self._originsock = self._proxy.address
Line 16: 
Line 17:         # temporary values to run temporary svdsm
Line 18:         self._proxy.pidfile = '/tmp/svdsm.pid'
better use tempfile over hard-coded name. What if another user (say, Jenkins) runs the same test on the same machine?
Line 19:         self._proxy.timestamp = '/tmp/svdsm.time'
Line 20:         self._proxy.address = '/tmp/svdsm.sock'
Line 21: 
Line 22:     def tearDown(self):


Line 25:         self._proxy.timestamp = self._origintimestempfile
Line 26:         self._proxy.address = self._originsock
Line 27: 
Line 28:     def testIsSuperUp(self):
Line 29:         testValidation.checkSudo(['ls'])
why do you need the running user to be able to have "sudo ls"? I suppose that checkSudo(['python', "supervdsmServer.py"]) would be more relevant.
Line 30:         self._proxy.ping()  # this call initiate svdsm
Line 31:         self._proxy.ping()
Line 32: 
Line 33:         self.assertTrue(self._proxy.isRunning())


Line 27: 
Line 28:     def testIsSuperUp(self):
Line 29:         testValidation.checkSudo(['ls'])
Line 30:         self._proxy.ping()  # this call initiate svdsm
Line 31:         self._proxy.ping()
why isn't it enough to call ping() only once?
Line 32: 
Line 33:         self.assertTrue(self._proxy.isRunning())
Line 34: 
Line 35:     def testKillSuper(self):


Line 34: 
Line 35:     def testKillSuper(self):
Line 36:         testValidation.checkSudo(['ls'])
Line 37:         self._proxy.ping()
Line 38:         self._proxy.kill()
I suppose you'd want to assertFail(isRunning) here.
Line 39:         self._proxy.ping()  # Launching vdsm after kill


....................................................
File vdsm/clientIF.py
Line 465:                     if f == 'respawn.pid':
Line 466:                         continue
Line 467:                     if f == 'svdsm.pid':
Line 468:                         continue
Line 469:                     if f == 'svdsm.sock':
it's not your fault, but seems like this file is removed by this function, as it does not have a known "fileType". Is it fine by supervdsm?

yuck. this function begs for a rewrite using sets. (in another patch, please)
Line 470:                         continue
Line 471:                     if f == 'svdsm.time':
Line 472:                         continue
Line 473:                 else:


....................................................
File vdsm/supervdsm.py
Line 27: import uuid
Line 28: from time import sleep
Line 29: import storage.misc as misc
Line 30: from vdsm import constants, utils
Line 31: import getpass
hmm, nice to know about this module. But why do we need to trust the environment? why os.getuid() / os.getguid() are not enough?
Line 32: 
Line 33: _g_singletonSupervdsmInstance = None
Line 34: _g_singletonSupervdsmInstance_lock = threading.Lock()
Line 35: 


Line 86:     """
Line 87:     A wrapper around all the supervdsm init stuff
Line 88:     """
Line 89:     _log = logging.getLogger("SuperVdsmProxy")
Line 90:     proxyLock = threading.Lock()
Too bad that you continue to mangle functional changes with test-specific ones.

It is bad for readability, and going to make some hard time downstream. We do not have many reviewers, so please make their life easier.

now that Saggi has acked+2 your patch, I do not want to delay it any further. but please avoid this.
Line 91: 
Line 92:     def __init__(self):
Line 93:         self.pidfile = PIDFILE
Line 94:         self.timestamp = TIMESTAMP


Line 119:         try:
Line 120:             self._log.debug("Killing svdsm")
Line 121:             with open(self.pidfile, "r") as f:
Line 122:                 spid = int(f.read().strip())
Line 123:             self._svdsm.shutdown()  # very important before execute kill
why is it "very important"? how did we survive without this before? this comments does not help me much.
Line 124:             misc.execCmd([constants.EXT_KILL, "-9", str(spid)], sudo=True)
Line 125:             self._cleanOldFiles()
Line 126:         except Exception as e:
Line 127:             self._log.debug("Could not kill old super vdsm %s", e)


....................................................
File vdsm/supervdsmServer.py
Line 356:         servThread.setDaemon(True)
Line 357:         servThread.start()
Line 358: 
Line 359:         log.debug("Changing owner to svdsm files to %s", user)
Line 360:         chown(address, user, METADATA_GROUP)
there's a little bit of asymmetry here: why was it important to pass the user from the caller, but the group can be kept hard-coded?
Line 361:         chown(timestamp, user, METADATA_GROUP)
Line 362:         chown(pidfile, user, METADATA_GROUP)
Line 363: 
Line 364:         log.debug("Started serving super vdsm object")


Line 367:         log.error("Could not start Super Vdsm: %s", e, exc_info=True)
Line 368:         sys.exit(1)
Line 369:     finally:
Line 370:         try:
Line 371:             os.unlink(address)
on the sys.exit(errno.EPERM) path, this would explode, since "address" is not yet defined.
Line 372:         except OSError:
Line 373:             pass
Line 374: 
Line 375: if __name__ == '__main__':


--
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: 21
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