Change in vdsm[master]: tests: new functional tests for vdsm storage

nsoffer at redhat.com nsoffer at redhat.com
Thu Sep 4 17:14:51 UTC 2014


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 <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