Adam Litke has posted comments on this change. Change subject: StorageDomainManifest: Move clusterLock into Manifest ......................................................................
Patch Set 12: (8 comments) 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?) Good point, but not related to this 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: Done 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: Done 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()? Because we removed it. This is the only consumer of that logic so I decided to call into the manifest. In general this module is badly behaved as it accesses many protected class members. 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 domainL Done 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. Done 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. ok 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 Done 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
