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

nsoffer at redhat.com nsoffer at redhat.com
Mon Sep 8 19:03:03 UTC 2014


Nir Soffer has posted comments on this change.

Change subject: tests: new functional tests for vdsm storage
......................................................................


Patch Set 4:

(9 comments)

Please address all my comments in patchset 2 (change or explain why you won't change). It is not productive to go and comment again about the same things.

http://gerrit.ovirt.org/#/c/32496/4/tests/functional/testlib/storagebackends/base.py
File tests/functional/testlib/storagebackends/base.py:

Line 4: import time
Line 5: import uuid
Line 6: from functional.testlib import vdsmcaller
Line 7: 
Line 8: class Verify:
This is old style class, bad practice. It is about 10 years that nobody should use them.

Old style classes do not fully support properties, which leads to hard to understand bugs.
Line 9:     def __init__(self, vdsm):
Line 10:         self.vdsm = vdsm
Line 11: 
Line 12:     def waitALittle(self, duration = 1):


Line 9:     def __init__(self, vdsm):
Line 10:         self.vdsm = vdsm
Line 11: 
Line 12:     def waitALittle(self, duration = 1):
Line 13:         time.sleep(duration)
This is still a lie. For example:

    verify.waitALittle(1000)

Also the default duration is very bad idea. What is this code doing?

    verify.waitALittle()

Does it wait 1 second? 10? 30?

You have to go the see the definition to understand what the code does. Why would you want to have such code?

This should be simply:

    def wait(duration):
        time.sleep(duration)

If for some reason you want to change the function used for waiting. But I don't see any reason to do it, so this should simply be inlined when you want to sleep:

    time.sleep(7)

Can you explain what is wrong with this?

And finally, how is waiting related to verifying? You promised that this class should be used like this:

    verify.thingHappend()

What does this verify?

    verify.wait(2)
Line 14: 
Line 15:     def storagePoolCreated(self, poolID, masterDomainID):
Line 16:         self.waitALittle()
Line 17:         linkToDomain = os.path.join('/rhev/data-center', poolID, masterDomainID)


Line 52:     def _taskStatus(self, taskID):
Line 53:         result = self.vdsm().getTaskStatus(taskID)
Line 54:         return result['taskStatus']
Line 55: 
Line 56: class StorageBackend:
Nothing that this class does is related to storage backend.
Line 57:     def __init__(self):
Line 58:         self._vdsmCaller = vdsmcaller.VDSMCaller()
Line 59: 
Line 60:     def vdsm(self):


Line 57:     def __init__(self):
Line 58:         self._vdsmCaller = vdsmcaller.VDSMCaller()
Line 59: 
Line 60:     def vdsm(self):
Line 61:         return self._vdsmCaller
Why the backend should know anything about vdsm?
Line 62: 
Line 63:     def newUUID(self):
Line 64:         return str(uuid.uuid4())
Line 65: 


Line 67:         return "%s_%04d" % (base, random.randint(1,10000))
Line 68: 
Line 69:     def connectStoragePool(self, poolID, masterDomainID):
Line 70:         SCSI_KEY_DEPRECATED = 0
Line 71:         self.vdsm().connectStoragePool(poolID, 1, SCSI_KEY_DEPRECATED, masterDomainID, 1)
How is this related to the storage backend?

Should move to VdsmCaller - this is a method that you don't want to use as is, because it is too ugly, using depracated arguments.

This should actually be fixed in vdscli, but can fix it in the meantime in our vdscli wrapper, but not certainly not here, in the storage backend.
Line 72: 
Line 73:     def spmStart(self, poolID):
Line 74:         RECOVERY_MODE_DEPRECATED = 0
Line 75:         SCSI_FENCING_DEPRECATED = 0


Line 72: 
Line 73:     def spmStart(self, poolID):
Line 74:         RECOVERY_MODE_DEPRECATED = 0
Line 75:         SCSI_FENCING_DEPRECATED = 0
Line 76:         self.vdsm().spmStart(poolID, -1, '-1', SCSI_FENCING_DEPRECATED, RECOVERY_MODE_DEPRECATED)
How is this related to the storage backend?
Line 77: 
Line 78:     def activateStorageDomain(self, domainID, poolID):
Line 79:         self.vdsm().activateStorageDomain(domainID,poolID)
Line 80: 


Line 75:         SCSI_FENCING_DEPRECATED = 0
Line 76:         self.vdsm().spmStart(poolID, -1, '-1', SCSI_FENCING_DEPRECATED, RECOVERY_MODE_DEPRECATED)
Line 77: 
Line 78:     def activateStorageDomain(self, domainID, poolID):
Line 79:         self.vdsm().activateStorageDomain(domainID,poolID)
How is this related to the storage backend?
Line 80: 
Line 81:     def stringForXMLRPC(self, number):


Line 77: 
Line 78:     def activateStorageDomain(self, domainID, poolID):
Line 79:         self.vdsm().activateStorageDomain(domainID,poolID)
Line 80: 
Line 81:     def stringForXMLRPC(self, number):
What?


http://gerrit.ovirt.org/#/c/32496/4/tests/functional/testlib/storagebackends/iscsi.py
File tests/functional/testlib/storagebackends/iscsi.py:

Line 40:         result = subprocess.call('sudo lvs %s | grep %s' % (self._volumeGroup['uuid'], self._volumeID), shell = True)
Line 41:         assert result == 0, "did not find logical volume in volume group"
Line 42: 
Line 43: class ISCSI(base.StorageBackend):
Line 44:     _NULL_UUID = '00000000-0000-0000-0000-000000000000'
Should be public constant in base.py
Line 45:     def __init__(self):
Line 46:         base.StorageBackend.__init__(self)
Line 47:         self._iqn = 'iqn.1970-01.functional.test:%04d' % random.randint(1,10000)
Line 48:         self._volumeGroup = { 'uuid': self.newUUID(), 'vgs_uuid': None }


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