Change in vdsm[master]: StorageDomainManifest: introduce class heirarchy

nsoffer at redhat.com nsoffer at redhat.com
Mon Jun 15 21:11:58 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: StorageDomainManifest: introduce class heirarchy
......................................................................


Patch Set 8:

(5 comments)

https://gerrit.ovirt.org/#/c/41993/8/vdsm/storage/blockSD.py
File vdsm/storage/blockSD.py:

Line 417:     manifestClass = BlockStorageDomainManifest
Line 418: 
Line 419:     def __init__(self, sdUUID):
Line 420:         manifest = self.manifestClass(sdUUID)
Line 421:         manifest.activate()
> The manifest is constructed with different args depending on its type.  I c
Ok
Line 422:         sd.StorageDomain.__init__(self, manifest)
Line 423:         lvm.activateLVs(self.sdUUID, SPECIAL_LVS)
Line 424:         self.metavol = lvm.lvPath(self.sdUUID, sd.METADATA)
Line 425: 


https://gerrit.ovirt.org/#/c/41993/8/vdsm/storage/fileSD.py
File vdsm/storage/fileSD.py:

Line 163:         if metadata is None:
Line 164:             metadata = FileSDMetadata(self.metafile)
Line 165:         self.replaceMetadata(metadata)
Line 166:         if not self.oop.fileUtils.pathExists(self.metafile):
Line 167:             raise se.StorageDomainMetadataNotFound(self.sdUUID, self.metafile)
> There are a few words we could use.  Another I'd be okay with is 'bind'.  T
Sounds better.
Line 168: 
Line 169:     def getReadDelay(self):
Line 170:         stats = misc.readspeed(self.metafile, 4096)
Line 171:         return stats['seconds']


Line 180: 
Line 181:         # We perform validation here since filesystem features are relevant to
Line 182:         # construction of an actual Storage Domain.  Direct users of
Line 183:         # FileStorageDomainManifest should call this explicitly if required.
Line 184:         validateFileSystemFeatures(manifest.sdUUID, manifest.mountpoint)
> This is a great idea but validateFileSystemFeatures is called in classmetho
Ok
Line 185:         sd.StorageDomain.__init__(self, manifest)
Line 186:         self.imageGarbageCollector()
Line 187:         self._registerResourceNamespaces()
Line 188: 


https://gerrit.ovirt.org/#/c/41993/8/vdsm/storage/sd.py
File vdsm/storage/sd.py:

Line 303:     def oop(self):
Line 304:         return oop.getProcessPool(self.sdUUID)
Line 305: 
Line 306:     def replaceMetadata(self, md):
Line 307:         self._metadata = md
> What do you mean?  We need to call it in replaceMetadata?  If that's the ca
I mean that the super class should have either empty method or abstract method for stuff subclasses must/may implement. You want to look at this code and undestand what is the interface of a StorageDomainManifest without checking all subclasses.

Something like:

    def bind(self):
        """ Description """
        raise NotImplementedError
Line 308: 
Line 309: 
Line 310: class StorageDomain(object):
Line 311:     log = logging.getLogger("Storage.StorageDomain")


Line 320:         3: (clusterlock.SANLock, True),
Line 321:     }
Line 322: 
Line 323:     def __init__(self, manifest):
Line 324:         self._manifest = manifest
> See my comment about the manifest ctor args being different depending on th
Ok
Line 325:         self._lock = threading.Lock()
Line 326:         self.stat = None
Line 327:         self._clusterLock = self._makeClusterLock()
Line 328: 


-- 
To view, visit https://gerrit.ovirt.org/41993
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia275424a8d423722efb229a4d8545f035fe5fa51
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Ala Hino <ahino at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot <laravot at redhat.com>
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