Dan Kenigsberg has posted comments on this change.
Change subject: storage functional test with multiple storage domains and images
......................................................................
Patch Set 6: I would prefer that you didn't submit this
(6 inline comments)
I like the spirit!
But as you have noticed, we do not have a lot of reviewers. Let's try to help
those that we have, by keeping patches small and simple.
(this is not a thorough review yet)
....................................................
File tests/functional/xmlrpcTests.py
Line 31:
Line 32: from vdsm.config import config
Line 33: from vdsm.constants import VDSM_USER, VDSM_GROUP
Line 34: from storage.sd import BLANK_UUID
Line 35: from storage.sd import name2class
since this module is growing, maybe it's better to keep the "storage.sd"
qualifier?
Line 36: from storage.sd import name2type as sdname2type
Line 37: from storage.volume import name2type as volname2type
Line 38: from vdsm import vdscli
Line 39: from storage.misc import execCmd
Line 77: os.chmod(path, 0644)
Line 78: return path
Line 79:
Line 80:
Line 81: def rollbackManager(transaction):
I'd appreciate a separate patch, introducing this mechanism, adding some
docstrings to it, and maybe even a test for it.
Line 82: @contextmanager
Line 83: def wrapper(*args, **kwargs):
Line 84: rollback = []
Line 85: exception = None
Line 85: exception = None
Line 86: traceback = None
Line 87: try:
Line 88: yield transaction(rollback=rollback, *args, **kwargs)
Line 89: except Exception, e:
I think someone asked to use the new
except Exception as e:
syntax in new code.
Line 90: # keep the original exception and traceback info
Line 91: exception = e
Line 92: traceback = sys.exc_info()[2]
Line 93: finally:
Line 209: with _localfsStore(conf['conn']),
self._vdsmStorageLayout(conf):
Line 210: pass
Line 211:
Line 212: @rollbackManager
Line 213: def _vdsmStorageLayout(self, conf, rollback):
this function is too long. would you break apart? every place you have a
comment is a place you could call a properly-named function.
Line 214: connections = conf['conn']
Line 215: storageDomains = conf['sd']
Line 216: storagePools = conf['sp']
Line 217: images = conf['img']
Line 330:
Line 331: retry(assertTaskOK, expectedException=AssertionError,
timeout=20)
Line 332:
Line 333:
Line 334: storageLayouts = \
if this was written in YAML, it might have been a little more readable.
(just a thought, sorry for day-dreaming)
Line 335: {'localfs':
Line 336: {'conn': {'53acd629-47e6-42d8-ba99-cd0b12ff0e1e':
Line 337: {'type': 'localfs',
Line 338: 'params': {'path': '/tmp/teststorage0'}},
Line 334: storageLayouts = \
Line 335: {'localfs':
Line 336: {'conn': {'53acd629-47e6-42d8-ba99-cd0b12ff0e1e':
Line 337: {'type': 'localfs',
Line 338: 'params': {'path': '/tmp/teststorage0'}},
I know that functional tests cannot be run concurrently anyways, but I still
feel bad about hard-coding the paths. would you mind changing the semantics so
that every path here is relative to a temporary directory created anew by the
test?
Line 339: '87e618fe-587c-4704-a9f8-9fd9321fd907':
Line 340: {'type': 'localfs',
Line 341: 'params': {'path': '/tmp/teststorage1'}}},
Line 342: 'sd': {"def32ac7-1234-1234-8a8c-1c887333fe65":
--
To view, visit http://gerrit.ovirt.org/8182
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8287046046460f399f180d19e0717a91419297f8
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Zhou Zheng Sheng <[email protected]>
Gerrit-Reviewer: Adam Litke <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Federico Simoncelli <[email protected]>
Gerrit-Reviewer: Mark Wu <[email protected]>
Gerrit-Reviewer: Royce Lv <[email protected]>
Gerrit-Reviewer: Ryan Harper <[email protected]>
Gerrit-Reviewer: Saggi Mizrahi <[email protected]>
Gerrit-Reviewer: Zhou Zheng Sheng <[email protected]>
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
