Nir Soffer has posted comments on this change. Change subject: StorageDomainManifest: Move clusterLock into Manifest ......................................................................
Patch Set 12: Code-Review-1 (8 comments) Generally ok, added few questions and minor cleanups. https://gerrit.ovirt.org/#/c/43549/12/tests/manifest_tests.py File tests/manifest_tests.py: Line 64: self.assertEquals(mdpath, manifest.getMDPath()) Line 65: Line 66: def test_getmetaparam(self): Line 67: with namedTemporaryDir() as tmpdir: Line 68: metadata = {sd.DMDK_VERSION: 3} Why not use FakeMetadata object for consistency? (for another patch?) Line 69: manifest = make_filesd_manifest(tmpdir, metadata) Line 70: metadata[sd.DMDK_SDUUID] = manifest.sdUUID Line 71: self.assertEquals(manifest.sdUUID, Line 72: manifest.getMetaParam(sd.DMDK_SDUUID)) https://gerrit.ovirt.org/#/c/43549/12/tests/storagetestlib.py File tests/storagetestlib.py: Line 38: def make_blocksd_manifest(tmpdir=None, metadata=None, sduuid=None): Line 39: if sduuid is None: Line 40: sduuid = str(uuid.uuid4()) Line 41: if metadata is None: Line 42: metadata = FakeMetadata([(sd.DMDK_VERSION, 3)]) You can use: FakeMetadata({sd.DMDK: 3}) Line 43: manifest = blockSD.BlockStorageDomainManifest(sduuid, metadata) Line 44: if tmpdir is not None: Line 45: manifest.domaindir = tmpdir Line 46: os.makedirs(os.path.join(manifest.domaindir, sduuid, sd.DOMAIN_IMAGES)) Line 77: sduuid = str(uuid.uuid4()) Line 78: domain_path = os.path.join(tmpdir, sduuid) Line 79: make_file(get_metafile_path(domain_path)) Line 80: if metadata is None: Line 81: metadata = FakeMetadata([(sd.DMDK_VERSION, 3)]) You can use: FakeMetadata({sd.DMDK: 3}) Line 82: manifest = fileSD.FileStorageDomainManifest(domain_path, metadata) Line 83: os.makedirs(os.path.join(manifest.domaindir, sduuid, sd.DOMAIN_IMAGES)) Line 84: return manifest Line 85: https://gerrit.ovirt.org/#/c/43549/12/vdsm/storage/imageRepository/formatConverter.py File vdsm/storage/imageRepository/formatConverter.py: Line 117: log.debug("Setting permissions for domain %s", domain.sdUUID) Line 118: domain.setMetadataPermissions() Line 119: Line 120: log.debug("Initializing the new cluster lock for domain %s", domain.sdUUID) Line 121: newClusterLock = domain._manifest._makeDomainLock(targetVersion) Why no not use domain._makeClusterLock()? Line 122: newClusterLock.initLock() Line 123: Line 124: log.debug("Acquiring the host id %s for domain %s", hostId, domain.sdUUID) Line 125: newClusterLock.acquireHostId(hostId, async=False) https://gerrit.ovirt.org/#/c/43549/12/vdsm/storage/sd.py File vdsm/storage/sd.py: Line 555 Line 556 Line 557 Line 558 Line 559 Lets move it to the manifest, so we don't have to make the manifest domainLock public. In the menifest, this should be called initDomainLock() Line 304: def __init__(self, sdUUID, domaindir, metadata): Line 305: self.sdUUID = sdUUID Line 306: self.domaindir = domaindir Line 307: self.replaceMetadata(metadata) Line 308: self.domainLock = self._makeDomainLock() Why public? old lock was private. Line 309: Line 310: @property Line 311: def oop(self): Line 312: return oop.getProcessPool(self.sdUUID) Line 428: DEFAULT_LEASE_PARAMS[DMDK_LOCK_RENEWAL_INTERVAL_SEC], Line 429: DEFAULT_LEASE_PARAMS[DMDK_LEASE_TIME_SEC], Line 430: DEFAULT_LEASE_PARAMS[DMDK_LEASE_RETRIES], Line 431: DEFAULT_LEASE_PARAMS[DMDK_IO_OP_TIMEOUT_SEC], Line 432: ) For another patch, move leaseParams after lock class selection. Line 433: Line 434: try: Line 435: clusterLockClass = self._domainLockTable[domVersion] Line 436: except KeyError: Line 431: DEFAULT_LEASE_PARAMS[DMDK_IO_OP_TIMEOUT_SEC], Line 432: ) Line 433: Line 434: try: Line 435: clusterLockClass = self._domainLockTable[domVersion] If you renamed _makeClusterLock to _makeDomainLock, you should also rename the class name to domainLockClass, or just lockClass. Line 436: except KeyError: Line 437: raise se.UnsupportedDomainVersion(domVersion) Line 438: Line 439: return clusterLockClass(self.sdUUID, self.getIdsFilePath(), -- To view, visit https://gerrit.ovirt.org/43549 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2c019a7fed6ba793d2b3a2459af221b1abbff2a3 Gerrit-PatchSet: 12 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
