Change in vdsm[master]: StorageDomainManifest: Move clusterLock into Manifest

alitke at redhat.com alitke at redhat.com
Thu Sep 10 19:08:42 UTC 2015


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 <alitke at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list