Change in vdsm[master]: tests: new functional tests for vdsm storage

ykleinbe at redhat.com ykleinbe at redhat.com
Sun Sep 14 10:32:28 UTC 2014


Yoav Kleinberger has posted comments on this change.

Change subject: tests: new functional tests for vdsm storage
......................................................................


Patch Set 4:

(21 comments)

http://gerrit.ovirt.org/#/c/32496/4/tests/functional/testlib/controlvdsm.py
File tests/functional/testlib/controlvdsm.py:

Line 4: import time
Line 5: import vdsm.config
Line 6: import vdsm.vdscli
Line 7: 
Line 8: class ControlVDSM:
> This is not a good class name, this should be VdsmController.
I don't agree. Classes can be named after actions, I don't have to "nounify" the name.

this class is about controlling the vdsm service - that is its connection to vdsm. If we ever want to control other services, we can generalize in the future.
Line 9:     def cleanup(self):
Line 10:         self._stopService()
Line 11:         assert not self._serviceRunning()
Line 12:         self._brutallyCleanFiles()


Line 10:         self._stopService()
Line 11:         assert not self._serviceRunning()
Line 12:         self._brutallyCleanFiles()
Line 13:         self._restartService()
Line 14:         return self._checkConnection()
> Why do we care about connecting to vdsm on the end of cleanup?
if this doesn't work - there's no point in starting the test. It's part of asserting that the test environment works.

I agree it's not obviously needed, but in my experience when writing these tests... it's nice to have.
Line 15: 
Line 16:     def _checkConnection(self):
Line 17:         useSSL = vdsm.config.config.getboolean('vars', 'ssl')
Line 18:         vdsmClient = vdsm.vdscli.connect(useSSL=useSSL)


Line 14:         return self._checkConnection()
Line 15: 
Line 16:     def _checkConnection(self):
Line 17:         useSSL = vdsm.config.config.getboolean('vars', 'ssl')
Line 18:         vdsmClient = vdsm.vdscli.connect(useSSL=useSSL)
> This duplicates the code in vdsmcaller.VdsmCaller - maybe you like to use a
true, but I don't want the verification VDSMCaller does.

I just want to see that basic XMLRPC words.
Line 19:         RETRIES = 5
Line 20:         for _ in range(RETRIES):
Line 21:             try:
Line 22:                 vdsmClient.getStorageDomainsList()


Line 28: 
Line 29:         raise Exception('could not connect to VDSM')
Line 30: 
Line 31:     def _stopService(self):
Line 32:         self._run("sudo service vdsmd stop")
> This is not related to vdsm, should be move to Service class.
stopping vdsm is not related to vdsm?
besides, I don't want to use code from vdsm in the tests.
Line 33: 
Line 34:     def _serviceRunning(self):
Line 35:         returnCode = subprocess.call('sudo service vdsmd status', shell=True, stdout=open('/dev/null','w'), stderr=open('/dev/null','w'))
Line 36:         logging.info('vdsm running: %s' % (returnCode == 0))


Line 31:     def _stopService(self):
Line 32:         self._run("sudo service vdsmd stop")
Line 33: 
Line 34:     def _serviceRunning(self):
Line 35:         returnCode = subprocess.call('sudo service vdsmd status', shell=True, stdout=open('/dev/null','w'), stderr=open('/dev/null','w'))
> This can ask a password on the CI, blocking the job forever. Check how we u
will fix
Line 36:         logging.info('vdsm running: %s' % (returnCode == 0))
Line 37:         return returnCode == 0
Line 38: 
Line 39:     def _restartService(self):


Line 36:         logging.info('vdsm running: %s' % (returnCode == 0))
Line 37:         return returnCode == 0
Line 38: 
Line 39:     def _restartService(self):
Line 40:         self._run("sudo vdsm-tool configure --force")
> This is not related to starting the service, should be in a separate method
will fix
Line 41:         self._run("sudo service vdsmd start")
Line 42: 
Line 43:     def _run(self, command):
Line 44:         logging.info('running: %s' % command)


Line 47:             logging.warning('failure! command was: %s' % command)
Line 48:         else:
Line 49:             logging.info('finished.')
Line 50: 
Line 51:     def _brutallyCleanFiles(self):
> Why do we need to remove all files from /hev/data-center?
cleanup. make sure the next test starts with a clean slate.
Line 52:         logging.warning('removing /rhev/data-center without asking too many questions')


http://gerrit.ovirt.org/#/c/32496/4/tests/functional/testlib/storagebackends/base.py
File tests/functional/testlib/storagebackends/base.py:

Line 4: import time
Line 5: import uuid
Line 6: from functional.testlib import vdsmcaller
Line 7: 
Line 8: class Verify:
> This is old style class, bad practice. It is about 10 years that nobody sho
OK
Line 9:     def __init__(self, vdsm):
Line 10:         self.vdsm = vdsm
Line 11: 
Line 12:     def waitALittle(self, duration = 1):


Line 9:     def __init__(self, vdsm):
Line 10:         self.vdsm = vdsm
Line 11: 
Line 12:     def waitALittle(self, duration = 1):
Line 13:         time.sleep(duration)
> This is still a lie. For example:
I want to have a name that clarifies *why* I sleep. I don't want to clutter the test itself with sleeps. 

I changed the name of the function in the next patch set.
Line 14: 
Line 15:     def storagePoolCreated(self, poolID, masterDomainID):
Line 16:         self.waitALittle()
Line 17:         linkToDomain = os.path.join('/rhev/data-center', poolID, masterDomainID)


Line 52:     def _taskStatus(self, taskID):
Line 53:         result = self.vdsm().getTaskStatus(taskID)
Line 54:         return result['taskStatus']
Line 55: 
Line 56: class StorageBackend:
> Nothing that this class does is related to storage backend.
Obviously, changing the name to Backend was bad.
I renamed it in the next patchset.
Line 57:     def __init__(self):
Line 58:         self._vdsmCaller = vdsmcaller.VDSMCaller()
Line 59: 
Line 60:     def vdsm(self):


Line 57:     def __init__(self):
Line 58:         self._vdsmCaller = vdsmcaller.VDSMCaller()
Line 59: 
Line 60:     def vdsm(self):
Line 61:         return self._vdsmCaller
> Why the backend should know anything about vdsm?
see above.
Line 62: 
Line 63:     def newUUID(self):
Line 64:         return str(uuid.uuid4())
Line 65: 


Line 67:         return "%s_%04d" % (base, random.randint(1,10000))
Line 68: 
Line 69:     def connectStoragePool(self, poolID, masterDomainID):
Line 70:         SCSI_KEY_DEPRECATED = 0
Line 71:         self.vdsm().connectStoragePool(poolID, 1, SCSI_KEY_DEPRECATED, masterDomainID, 1)
> How is this related to the storage backend?
see above.
Line 72: 
Line 73:     def spmStart(self, poolID):
Line 74:         RECOVERY_MODE_DEPRECATED = 0
Line 75:         SCSI_FENCING_DEPRECATED = 0


Line 77: 
Line 78:     def activateStorageDomain(self, domainID, poolID):
Line 79:         self.vdsm().activateStorageDomain(domainID,poolID)
Line 80: 
Line 81:     def stringForXMLRPC(self, number):
> What?
the python xml-rpc implementation cannot handle very large integers. The solution is to pass them as strings. I will find a better name for this.


http://gerrit.ovirt.org/#/c/32496/4/tests/functional/testlib/storagebackends/iscsi.py
File tests/functional/testlib/storagebackends/iscsi.py:

Line 40:         result = subprocess.call('sudo lvs %s | grep %s' % (self._volumeGroup['uuid'], self._volumeID), shell = True)
Line 41:         assert result == 0, "did not find logical volume in volume group"
Line 42: 
Line 43: class ISCSI(base.StorageBackend):
Line 44:     _NULL_UUID = '00000000-0000-0000-0000-000000000000'
> Should be public constant in base.py
not used there, so not needed there.
Line 45:     def __init__(self):
Line 46:         base.StorageBackend.__init__(self)
Line 47:         self._iqn = 'iqn.1970-01.functional.test:%04d' % random.randint(1,10000)
Line 48:         self._volumeGroup = { 'uuid': self.newUUID(), 'vgs_uuid': None }


Line 63:         self._testDirectory = tempfile.mkdtemp()
Line 64:         self._storageFile = os.path.join(self._testDirectory, 'testfile')
Line 65:         self._fileioBackstore = self.randomName('backfile')
Line 66:         logging.info('using %s, %s' % (self._fileioBackstore, self._storageFile))
Line 67:         self._targetcli('/backstores/fileio create %s %s 10G' % (self._fileioBackstore, self._storageFile))
> We should separate the server driver from the backend, so it will be easier
later. let's get this working for now.
Line 68:         self._targetcli('/iscsi create %s' % self._iqn)
Line 69:         self._targetcli('/iscsi/%s/tpg1/luns create /backstores/fileio/%s' % (self._iqn, self._fileioBackstore))
Line 70:         self._targetcli('/iscsi/%s/tpg1 set attribute authentication=0 demo_mode_write_protect=0' % self._iqn)
Line 71:         self._targetcli('/iscsi/%s/tpg1 set attribute generate_node_acls=1 cache_dynamic_acls=1' % self._iqn)


Line 65:         self._fileioBackstore = self.randomName('backfile')
Line 66:         logging.info('using %s, %s' % (self._fileioBackstore, self._storageFile))
Line 67:         self._targetcli('/backstores/fileio create %s %s 10G' % (self._fileioBackstore, self._storageFile))
Line 68:         self._targetcli('/iscsi create %s' % self._iqn)
Line 69:         self._targetcli('/iscsi/%s/tpg1/luns create /backstores/fileio/%s' % (self._iqn, self._fileioBackstore))
> This  is unreadable, specially in gerrit.
Dont' agree with you about the lists vs. strings - guess it's a matter of personal taste.

Will refactor.
Line 70:         self._targetcli('/iscsi/%s/tpg1 set attribute authentication=0 demo_mode_write_protect=0' % self._iqn)
Line 71:         self._targetcli('/iscsi/%s/tpg1 set attribute generate_node_acls=1 cache_dynamic_acls=1' % self._iqn)
Line 72:         return self, Verify(self._iqn, self._volumeGroup, self.vdsm, self._volumeID)
Line 73: 


http://gerrit.ovirt.org/#/c/32496/4/tests/functional/testlib/vdsmcaller.py
File tests/functional/testlib/vdsmcaller.py:

Line 1: import vdsm.config
Line 2: import vdsm.vdscli
Line 3: 
> This module add little value, please merge it with the vdsmcontrol module.
actually, refactoring the verify-vdsm-result was your idea.

I think it's a good idea, and removed repetitive code I previously had.
Line 4: class _CallWrapper:
Line 5:     def __init__(self, function):
Line 6:         self._function = function
Line 7: 


Line 1: import vdsm.config
Line 2: import vdsm.vdscli
Line 3: 
Line 4: class _CallWrapper:
> Please put private names at the end of the module, and the public parts at 
OK
Line 5:     def __init__(self, function):
Line 6:         self._function = function
Line 7: 
Line 8:     def __call__(self, *args, **kwargs):


Line 11:         return result
Line 12: 
Line 13:     def _verifyVDSMSuccess(self, result):
Line 14:         if result[ 'status' ][ 'code' ] != 0:
Line 15:             raise Exception('expected OK result from VDSM, got "%s" instead' % str(result))
> This method adds no value, just inline it in __call__. Why make stuff more 
The name explains the meaning of the if statement, that's the value.
Line 16: 
Line 17: class VDSMCaller:
Line 18:     def __init__(self):
Line 19:         useSSL = vdsm.config.config.getboolean('vars', 'ssl')


Line 13:     def _verifyVDSMSuccess(self, result):
Line 14:         if result[ 'status' ][ 'code' ] != 0:
Line 15:             raise Exception('expected OK result from VDSM, got "%s" instead' % str(result))
Line 16: 
Line 17: class VDSMCaller:
> This is actually a VdsCliWrapper, or from the point of view of the user of 
You have a point. However, VDSMClient wrapping vdscli... I'd rather keep this name.
Line 18:     def __init__(self):
Line 19:         useSSL = vdsm.config.config.getboolean('vars', 'ssl')
Line 20:         self._vdsm = vdsm.vdscli.connect(useSSL=useSSL)
Line 21: 


Line 20:         self._vdsm = vdsm.vdscli.connect(useSSL=useSSL)
Line 21: 
Line 22:     def __getattr__(self, name):
Line 23:         function = getattr(self._vdsm, name)
Line 24:         return _CallWrapper(function)
> Wrapping should be done only for callables, not for all attribute.
YAGNI. when something fails, I'll fix.


-- 
To view, visit http://gerrit.ovirt.org/32496
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1703e7c1dc223ff707775865cd14c7dd62314caf
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yoav Kleinberger <ykleinbe at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Vered Volansky <vvolansk at redhat.com>
Gerrit-Reviewer: Yoav Kleinberger <ykleinbe at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list