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