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