Nir Soffer has posted comments on this change. Change subject: tests: new functional tests for vdsm storage ......................................................................
Patch Set 2: (29 comments) Partial review, but this is enough for now. I will continue after these comments are addressed. Global comments: - testlib.testcontexts is hard to read and duplicate the work test - we are inside the testlib package. Should be "contexts", or better "backends", because it contains storage backend classes. - Use pep8_style for function names, not vdsmLegacyStyle. Using legacy style is ok only when modifying existing modules, but you add new code. http://gerrit.ovirt.org/#/c/32496/2//COMMIT_MSG Commit Message: Line 6: Line 7: tests: new functional tests for vdsm storage Line 8: Line 9: Ultimately, the purpose of this patch is to replace the existing Line 10: tests/functional/storageTests.py, henceforth "the old test". "old tests" Line 11: It needs to be replaced because: Line 12: Line 13: * The old test does not, in fact, verify VDSM behaviour. It only checks Line 14: for the return codes that VDSM returns to its caller. Line 9: Ultimately, the purpose of this patch is to replace the existing Line 10: tests/functional/storageTests.py, henceforth "the old test". Line 11: It needs to be replaced because: Line 12: Line 13: * The old test does not, in fact, verify VDSM behaviour. It only checks "old tests do not" Line 14: for the return codes that VDSM returns to its caller. Line 15: * The old test is hard to understand, debug and to extend Line 16: Line 17: This patch introduces a framework of "test contexts" that is extensible to various Line 15: * The old test is hard to understand, debug and to extend Line 16: Line 17: This patch introduces a framework of "test contexts" that is extensible to various Line 18: storage backends. Each test context, be it iscsi, nfs or some other Line 19: storage type, knows how to tell VDSM to create its particular type of The word context is too general, we are talking here about storage types, or storage backends. Line 20: storage domain, and also knows how to verify that observable actions Line 21: (e.g. the creation of a logical volume in an LVM volume group) have Line 22: actually been performed. Line 23: Line 20: storage domain, and also knows how to verify that observable actions Line 21: (e.g. the creation of a logical volume in an LVM volume group) have Line 22: actually been performed. Line 23: Line 24: Currently, only localfs and iscsi are supported. Other storage backends As you say: these are "backends", not "contexts" Line 25: will be added later. Line 26: Line 27: Change-Id: I1703e7c1dc223ff707775865cd14c7dd62314caf Line 28: Bug-Url: https://bugzilla.redhat.com/?????? Line 24: Currently, only localfs and iscsi are supported. Other storage backends Line 25: will be added later. Line 26: Line 27: Change-Id: I1703e7c1dc223ff707775865cd14c7dd62314caf Line 28: Bug-Url: https://bugzilla.redhat.com/?????? No need for bug url. http://gerrit.ovirt.org/#/c/32496/2/tests/functional/testlib/testcontexts/base.py File tests/functional/testlib/testcontexts/base.py: Line 3: import vdsm.config Line 4: import random Line 5: import uuid Line 6: import logging Line 7: import time sort imports Line 8: Line 9: class Verify: Line 10: def __init__(self, vdsm): Line 11: self._vdsm = vdsm Line 5: import uuid Line 6: import logging Line 7: import time Line 8: Line 9: class Verify: Verifier? Line 10: def __init__(self, vdsm): Line 11: self._vdsm = vdsm Line 12: Line 13: def assertPathExists(self, path, link = False): Line 9: class Verify: Line 10: def __init__(self, vdsm): Line 11: self._vdsm = vdsm Line 12: Line 13: def assertPathExists(self, path, link = False): Don't use space around operators in arguments (pep8 violation) Line 14: logging.info('verifying path: %s' % path) Line 15: if link: Line 16: assert os.path.lexists(path) Line 17: else: Line 14: logging.info('verifying path: %s' % path) Line 15: if link: Line 16: assert os.path.lexists(path) Line 17: else: Line 18: assert os.path.exists(path) This assert give very poor error message. Given this implementation, it is much better to use this assert in instead of calling this function. If you want to make this worthy, raise a nice exception like: raise AssertionError("path %r does not exists", path) And raise AssertionError("link %r does not exists", path) Line 19: Line 20: def assertPathDoesNotExist(self, path, link = False): Line 21: if link: Line 22: assert not os.path.lexists(path) Line 20: def assertPathDoesNotExist(self, path, link = False): Line 21: if link: Line 22: assert not os.path.lexists(path) Line 23: else: Line 24: assert not os.path.exists(path) Same as assertPathExists Line 25: Line 26: def waitForVDSMToFinishTask(self, duration = 1): Line 27: time.sleep(duration) Line 28: Line 22: assert not os.path.lexists(path) Line 23: else: Line 24: assert not os.path.exists(path) Line 25: Line 26: def waitForVDSMToFinishTask(self, duration = 1): This a lie - there is no point in having this method. Just use time.sleep() instead in the call site. Line 27: time.sleep(duration) Line 28: Line 29: def storagePoolCreated(self, poolID, masterDomainID): Line 30: self.waitForVDSMToFinishTask() Line 28: Line 29: def storagePoolCreated(self, poolID, masterDomainID): Line 30: self.waitForVDSMToFinishTask() Line 31: linkToDomain = os.path.join('/rhev/data-center', poolID, masterDomainID) Line 32: masterAliasLink = os.path.join('/rhev/data-center', poolID, 'mastersd') Why not "link_to_master_domain" - use consistent names. Line 33: logging.info('verifying domain %s in pool %s' % (masterDomainID, poolID)) Line 34: assert os.path.lexists(linkToDomain) Line 35: logging.info('verifying symlink to master domain') Line 36: assert os.path.lexists(masterAliasLink) Line 30: self.waitForVDSMToFinishTask() Line 31: linkToDomain = os.path.join('/rhev/data-center', poolID, masterDomainID) Line 32: masterAliasLink = os.path.join('/rhev/data-center', poolID, 'mastersd') Line 33: logging.info('verifying domain %s in pool %s' % (masterDomainID, poolID)) Line 34: assert os.path.lexists(linkToDomain) Why not use the assert you created above? It seems that you don't think that assert is needed :-) Line 35: logging.info('verifying symlink to master domain') Line 36: assert os.path.lexists(masterAliasLink) Line 37: Line 38: def waitFor(self, timeout, description, predicate, *args, **kwargs): Line 32: masterAliasLink = os.path.join('/rhev/data-center', poolID, 'mastersd') Line 33: logging.info('verifying domain %s in pool %s' % (masterDomainID, poolID)) Line 34: assert os.path.lexists(linkToDomain) Line 35: logging.info('verifying symlink to master domain') Line 36: assert os.path.lexists(masterAliasLink) I don't think we need to check the files here, we can test this in a unittest for the relevant class. Line 37: Line 38: def waitFor(self, timeout, description, predicate, *args, **kwargs): Line 39: logging.info('waiting for "%s"' % description) Line 40: start = time.time() Line 34: assert os.path.lexists(linkToDomain) Line 35: logging.info('verifying symlink to master domain') Line 36: assert os.path.lexists(masterAliasLink) Line 37: Line 38: def waitFor(self, timeout, description, predicate, *args, **kwargs): This should accept only integers, so lest make it clear by calling it "seconds", and raise if we are given a float. Line 39: logging.info('waiting for "%s"' % description) Line 40: start = time.time() Line 41: for _ in xrange(timeout): Line 42: if predicate(*args, **kwargs): Line 39: logging.info('waiting for "%s"' % description) Line 40: start = time.time() Line 41: for _ in xrange(timeout): Line 42: if predicate(*args, **kwargs): Line 43: logging.info('it took %s seconds' % (time.time() - start)) Use %0.3f - you are formatting a floating point number and we want to control the precision, not relay on the format that str(float) happen to use. Line 44: return Line 45: time.sleep(1) Line 46: Line 47: assert False, 'waited %s seconds for "%s" but it did not happen' % (timeout, description) Line 46: Line 47: assert False, 'waited %s seconds for "%s" but it did not happen' % (timeout, description) Line 48: Line 49: def spmStarted(self, poolID): Line 50: self.waitForVDSMToFinishTask() "wait to vdsm to finish task" is not english. "wait until vdsm finishes task" or something like that is would be better. But it would be much better if this was self.vdsm.wait_for_task() Line 51: masterDomainDirectory = '/rhev/data-center/%s/mastersd' % poolID Line 52: master = os.path.join(masterDomainDirectory, 'master') Line 53: tasks = os.path.join(master, 'tasks') Line 54: vms = os.path.join(master, 'vms') Line 47: assert False, 'waited %s seconds for "%s" but it did not happen' % (timeout, description) Line 48: Line 49: def spmStarted(self, poolID): Line 50: self.waitForVDSMToFinishTask() Line 51: masterDomainDirectory = '/rhev/data-center/%s/mastersd' % poolID Too long - we don't need so much context in a temporary. "mastersd" is enough. Line 52: master = os.path.join(masterDomainDirectory, 'master') Line 53: tasks = os.path.join(master, 'tasks') Line 54: vms = os.path.join(master, 'vms') Line 55: allExist = lambda: ( os.path.exists(master) and os.path.exists(tasks) and os.path.exists(vms) ) Line 51: masterDomainDirectory = '/rhev/data-center/%s/mastersd' % poolID Line 52: master = os.path.join(masterDomainDirectory, 'master') Line 53: tasks = os.path.join(master, 'tasks') Line 54: vms = os.path.join(master, 'vms') Line 55: allExist = lambda: ( os.path.exists(master) and os.path.exists(tasks) and os.path.exists(vms) ) This is a named function - there is no reason to use lambda, which exists to create unnamed functions. def allExists(): ... self.waitFor(60, "..", allExists) is more clear. Line 56: self.waitFor(60, 'SPM related subdirectories exist', allExist) Line 57: Line 58: def waitOnVDSMTask(self, taskID, timeout): Line 59: taskFinished = lambda: (self._taskStatus(taskID)['taskState'] == 'finished') Line 52: master = os.path.join(masterDomainDirectory, 'master') Line 53: tasks = os.path.join(master, 'tasks') Line 54: vms = os.path.join(master, 'vms') Line 55: allExist = lambda: ( os.path.exists(master) and os.path.exists(tasks) and os.path.exists(vms) ) Line 56: self.waitFor(60, 'SPM related subdirectories exist', allExist) I don't think we should test here what the spm does when it starts. We can test this in a unittest for the pool class. It is enough to get a success status from vdsm for the spmStart, or get spm status. Line 57: Line 58: def waitOnVDSMTask(self, taskID, timeout): Line 59: taskFinished = lambda: (self._taskStatus(taskID)['taskState'] == 'finished') Line 60: self.waitFor(timeout, 'vdsm task to be finished', taskFinished) Line 54: vms = os.path.join(master, 'vms') Line 55: allExist = lambda: ( os.path.exists(master) and os.path.exists(tasks) and os.path.exists(vms) ) Line 56: self.waitFor(60, 'SPM related subdirectories exist', allExist) Line 57: Line 58: def waitOnVDSMTask(self, taskID, timeout): Not clear why this is "waitOn" instead of "waitFor" - when it calls waitFor(). Line 59: taskFinished = lambda: (self._taskStatus(taskID)['taskState'] == 'finished') Line 60: self.waitFor(timeout, 'vdsm task to be finished', taskFinished) Line 61: taskStatus = self._taskStatus(taskID) Line 62: assert taskStatus['code'] == 0, taskStatus['message'] Line 55: allExist = lambda: ( os.path.exists(master) and os.path.exists(tasks) and os.path.exists(vms) ) Line 56: self.waitFor(60, 'SPM related subdirectories exist', allExist) Line 57: Line 58: def waitOnVDSMTask(self, taskID, timeout): Line 59: taskFinished = lambda: (self._taskStatus(taskID)['taskState'] == 'finished') Again, don't use lambda for creating named function. Line 60: self.waitFor(timeout, 'vdsm task to be finished', taskFinished) Line 61: taskStatus = self._taskStatus(taskID) Line 62: assert taskStatus['code'] == 0, taskStatus['message'] Line 63: Line 62: assert taskStatus['code'] == 0, taskStatus['message'] Line 63: Line 64: def _taskStatus(self, taskID): Line 65: result = self._vdsm.getTaskStatus(taskID) Line 66: assert result['status']['code'] == 0 This need better error message: raise AssertionError("Vdsm task failed with status: %d", ...) This also duplicate the logic of verifyVdsmSuccess() Line 67: taskStatus = result['taskStatus'] Line 68: return taskStatus Line 69: Line 70: class StorageBackend: Line 64: def _taskStatus(self, taskID): Line 65: result = self._vdsm.getTaskStatus(taskID) Line 66: assert result['status']['code'] == 0 Line 67: taskStatus = result['taskStatus'] Line 68: return taskStatus Use: return result['taskStatus'] Line 69: Line 70: class StorageBackend: Line 71: def __init__(self): Line 72: useSSL = vdsm.config.config.getboolean('vars', 'ssl') Line 71: def __init__(self): Line 72: useSSL = vdsm.config.config.getboolean('vars', 'ssl') Line 73: self._vdsm = vdsm.vdscli.connect(useSSL=useSSL) Line 74: Line 75: def newUUID(self): This is not specific to storage and It is also should not be an instance method. Add a function some utils module. Line 76: return str(uuid.uuid4()) Line 77: Line 78: def randomName(self, base): Line 79: return "%s_%04d" % (base, random.randint(1,10000)) Line 75: def newUUID(self): Line 76: return str(uuid.uuid4()) Line 77: Line 78: def randomName(self, base): Line 79: return "%s_%04d" % (base, random.randint(1,10000)) This is not random enough, use something like: os.urandom(4).encode('hex') Line 80: Line 81: def connectStoragePool(self, poolID, masterDomainID): Line 82: SCSI_KEY_DEPRECATED = 0 Line 83: result = self._vdsm.connectStoragePool(poolID, 1, SCSI_KEY_DEPRECATED, masterDomainID, 1) Line 85: Line 86: def spmStart(self, poolID): Line 87: RECOVERY_MODE_DEPRECATED = 0 Line 88: SCSI_FENCING_DEPRECATED = 0 Line 89: result = self._vdsm.spmStart(poolID, -1, '-1', SCSI_FENCING_DEPRECATED, RECOVERY_MODE_DEPRECATED) line too long - please use pep8 tool to make this pep8 compliant. Line 90: self.verifyVDSMSuccess(result) Line 91: Line 92: def activateStorageDomain(self, domainID, poolID): Line 93: result = self._vdsm.activateStorageDomain(domainID,poolID) Line 90: self.verifyVDSMSuccess(result) Line 91: Line 92: def activateStorageDomain(self, domainID, poolID): Line 93: result = self._vdsm.activateStorageDomain(domainID,poolID) Line 94: self.verifyVDSMSuccess(result) You can see that every vdsm api need this verify - it should move to vdsm object. Line 95: Line 96: def stringForXMLRPC(self, number): Line 97: return str(number) Line 98: Line 97: return str(number) Line 98: Line 99: def verifyVDSMSuccess(self, result): Line 100: if result[ 'status' ][ 'code' ] != 0: Line 101: raise Exception('expected OK result from VDSM, got "%s" instead' % str(result)) This should be part of the vdsm api. It should raise when vdsm failed, instead of wrapping it here. -- 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: 2 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
