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