Adam Litke has posted comments on this change. Change subject: StorageDomainManifest: introduce class heirarchy ......................................................................
Patch Set 8: (17 comments) https://gerrit.ovirt.org/#/c/41993/8/tests/manifestTests.py File tests/manifestTests.py: Line 18: # Line 19: Line 20: import os Line 21: import shutil Line 22: import tempfile > shutil and tempfile not needed if we use testlib infrastructure. I'd prefer not to use the testlib versions because I don't want the contextmanager behavior. I create the dir in setUp() and clean it up in tearDown() Line 23: import uuid Line 24: Line 25: from testlib import VdsmTestCase as TestCaseBase Line 26: from monkeypatch import MonkeyPatchScope Line 21: import shutil Line 22: import tempfile Line 23: import uuid Line 24: Line 25: from testlib import VdsmTestCase as TestCaseBase > No need to rename VdsmTestCase Done Line 26: from monkeypatch import MonkeyPatchScope Line 27: Line 28: from storage import blockSD, fileSD, lvm Line 29: from storage.persistentDict import PersistentDict, DictValidator Line 27: Line 28: from storage import blockSD, fileSD, lvm Line 29: from storage.persistentDict import PersistentDict, DictValidator Line 30: Line 31: SDBLKSZ = 512 > Don't we have this in the actual code somewhere? I'm actually removing this as we shouldn't really need it at all. Line 32: Line 33: # /tmp may use tempfs filesystem, not suitable for some of the test assuming a Line 34: # filesystem with direct io support. Line 35: TEMPDIR = '/var/tmp' Line 31: SDBLKSZ = 512 Line 32: Line 33: # /tmp may use tempfs filesystem, not suitable for some of the test assuming a Line 34: # filesystem with direct io support. Line 35: TEMPDIR = '/var/tmp' > use testlib.TEMPDIR, or better testlib.namedTemporaryDir() Done Line 36: Line 37: Line 38: class FakeBlockSDMetadata(object): Line 39: def __init__(self): Line 45: def writelines(self, metadata): Line 46: self._md = {} Line 47: for line in metadata: Line 48: k, v = line.split('=') Line 49: self._md[k] = v > If all this does is writing and reading lines, maybe we can use a list: Done Line 50: Line 51: BlockSDMetadata = lambda: DictValidator( Line 52: PersistentDict(FakeBlockSDMetadata()), Line 53: blockSD.BLOCK_SD_MD_FIELDS) Line 56: class FileManifestTests(TestCaseBase): Line 57: VOLSIZE = 1024 Line 58: Line 59: def setUp(self): Line 60: self.mountpoint = tempfile.mkdtemp(dir=TEMPDIR) > This creates temporary directory. fixed. Line 61: sdUUID = str(uuid.uuid4()) Line 62: self.domainPath = os.path.join(self.mountpoint, sdUUID) Line 63: self.manifest = fileSD.FileStorageDomainManifest(self.domainPath) Line 64: Line 71: def tearDown(self): Line 72: shutil.rmtree(self.mountpoint) Line 73: Line 74: def testGetReadDelay(self): Line 75: self.assertIsInstance(self.manifest.getReadDelay(), float) > Lets add this helper that can be useful for other tests: Not sure I follow. I am doing more than simply creating a md file in order to set up a manifest. Particularly in later patches. This is why I'd prefer to use setUp and tearDown Line 76: Line 77: Line 78: class BlockManifestTests(TestCaseBase): Line 79: VOLSIZE = 1024 Line 84: self.manifest = blockSD.BlockStorageDomainManifest(sdUUID) Line 85: self.manifest.activate(BlockSDMetadata()) Line 86: Line 87: def tearDown(self): Line 88: shutil.rmtree(self.mountpoint) > Same issue as above, use namedTemporaryDir() Same comment as above :) Line 89: Line 90: def testGetReadDelay(self): Line 91: dummyFile = os.path.join(self.mountpoint, 'dummy') Line 92: open(dummyFile, "w").truncate(self.VOLSIZE * SDBLKSZ) Line 92: open(dummyFile, "w").truncate(self.VOLSIZE * SDBLKSZ) Line 93: Line 94: # We patch lvm so lvPath returns our dummy file. Line 95: with MonkeyPatchScope([(lvm, 'lvPath', lambda *unused: dummyFile)]): Line 96: self.assertIsInstance(self.manifest.getReadDelay(), float) > We probably need similar helper for the block tests. Please re-evaluate after seeing the next version of the patch. https://gerrit.ovirt.org/#/c/41993/8/tests/volumeTests.py File tests/volumeTests.py: Line 28 Line 29 Line 30 Line 31 Line 32 > This seems unused now, delete? Done 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() > These lines are identical to FileStorageDomainManifest, move up to StorageD The manifest is constructed with different args depending on its type. I could move the activate() to StorageDomain but since that operation is closely related to the construction of the manifest I prefer to keep these lines next to each other. 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 152: # and you wouldn't be able to access any domain Line 153: self.log.debug("Reading domain in path %s", domainPath) Line 154: self.mountpoint = os.path.dirname(domainPath) Line 155: self.remotePath = os.path.basename(self.mountpoint) Line 156: sdUUID = os.path.basename(domainPath) > We need this only on line 160 - why not move this down to the place that ne Done Line 157: self.metafile = os.path.join(domainPath, sd.DOMAIN_META_DATA, Line 158: sd.METADATA) Line 159: domaindir = os.path.join(self.mountpoint, sdUUID) Line 160: sd.StorageDomainManifest.__init__(self, sdUUID, domaindir) 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) > Isn't this actually load()? There are a few words we could use. Another I'd be okay with is 'bind'. The logical operation we are performing is binding the manifest to an actual piece of storage. I'll change it to bind and hopefully you like that name better. Line 168: Line 169: def getReadDelay(self): Line 170: stats = misc.readspeed(self.metafile, 4096) Line 171: return stats['seconds'] Line 175: manifestClass = FileStorageDomainManifest Line 176: Line 177: def __init__(self, domainPath): Line 178: manifest = self.manifestClass(domainPath) Line 179: manifest.activate() > Move up to superclass see comment for blockSD 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 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) > Lets change it to accept a manifest instance, or move the function into the This is a great idea but validateFileSystemFeatures is called in classmethod context so a manifest might not always be available. In those same places, I'd prefer to not need to build a stand-in manifest just to be able to run this function. In any case, it might be out of the scope of what this series is trying to accomplish. 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 > We need the activate()/load() interface here. What do you mean? We need to call it in replaceMetadata? If that's the case then I don't agree. activate/load/bind already calls replaceMetadata. If future callers want to reset all of the moving parts in the manifest they should call activate/load/bind. If they just want to swap out the metadata they can call this. I don't want to change behavior of the existing code. 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 > We can create and activate the manifest here. See my comment about the manifest ctor args being different depending on the type. For block we pass a sdUUID. For file we pass a domainPath. 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 <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Ala Hino <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Freddy Rolland <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Liron Aravot <[email protected]> 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
