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

[Nir Soffer]

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/format...
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 <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes