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 <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Vered Volansky <[email protected]> Gerrit-Reviewer: Yoav Kleinberger <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
