Change in vdsm[master]: tests: new functional tests for vdsm storage
nsoffer at redhat.com
nsoffer at redhat.com
Mon Sep 8 15:11:54 UTC 2014
Nir Soffer has posted comments on this change.
Change subject: tests: new functional tests for vdsm storage
......................................................................
Patch Set 4:
(12 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.
Anyway, there is only one method here related to vdsm - _checkConnection
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?
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 an instance of this class here instead?
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.
service = Service(name)
service.start()
service.is_running()
...
We have such module in lib/vdsm/tool/, maybe we can reuse it instead of duplicating most of the code.
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 used sudo in utils.execCmd.
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.
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?
Line 52: logging.warning('removing /rhev/data-center without asking too many questions')
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.
Ideally, the single module containing vdsm helpers would be called: testlib.vdsm
And this module would contain two public classes:
- testlib.vdsm.Controller
- testlib.vdsm.Caller
Or both classes can merged, I don't see why they are separate, both use an instance of the vdscli.Client.
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 the start.
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 complicated then needed?
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 this class, a VdsmClient (that does not suck ;-) )
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.
I think that this should be good enough:
attribute = getattr(self._vdsm, name)
if callable(attribute):
return _CallWrapper(attribute)
return attribute
--
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