Nir Soffer has posted comments on this change. Change subject: StorageDomainManifest: move BlockSD.getVSize ......................................................................
Patch Set 12: (20 comments) I looked only in the FakeLVM, since it is very complex now. I think we can make it simpler. https://gerrit.ovirt.org/#/c/41994/12/tests/manifest_tests.py File tests/manifest_tests.py: Line 32 Line 33 Line 34 Line 35 Line 36 Use tmpdir to make it clear what is this path. Line 60 Line 61 Line 62 Line 63 Line 64 Same, use tmpdir, or better term (e.g. root, basedir etc.) Or remove it - we don't use it. Line 38: manifest = self.make_manifest(path) Line 39: self.assertIsInstance(manifest.getReadDelay(), float) Line 40: Line 41: def test_getvsize(self): Line 42: with namedTemporaryDir() as path: path -> tmpdir Line 43: manifest = self.make_manifest(path) Line 44: imguuid, voluuid = make_volume(manifest.domaindir, VOLSIZE) Line 45: self.assertEqual(VOLSIZE, manifest.getVSize(imguuid, voluuid)) Line 46: Line 67: Line 68: with MonkeyPatchScope([(blockSD, 'lvm', lvm)]): Line 69: self.assertIsInstance(manifest.getReadDelay(), float) Line 70: Line 71: def test_getvsize_via_dev(self): It is more correct to call this test_getvsize_active_lv() Line 72: # Tests the fast path when the device file is present Line 73: with namedTemporaryDir() as path: Line 74: manifest = self.make_manifest(path) Line 75: vg = manifest.sdUUID Line 71: def test_getvsize_via_dev(self): Line 72: # Tests the fast path when the device file is present Line 73: with namedTemporaryDir() as path: Line 74: manifest = self.make_manifest(path) Line 75: vg = manifest.sdUUID Lets use vgname or to make it easy to detect a vg name and a vg object returned from lvm apis - we have the same distinction in the code. Line 76: lvm = FakeLVM(path) Line 77: voluuid = str(uuid.uuid4()) Line 78: volpath = lvm.lvPath(vg, voluuid) Line 79: os.makedirs(os.path.dirname(volpath)) Line 73: with namedTemporaryDir() as path: Line 74: manifest = self.make_manifest(path) Line 75: vg = manifest.sdUUID Line 76: lvm = FakeLVM(path) Line 77: voluuid = str(uuid.uuid4()) Maybe lvname instead of voluuid? Line 78: volpath = lvm.lvPath(vg, voluuid) Line 79: os.makedirs(os.path.dirname(volpath)) Line 80: with open(volpath, "w") as f: Line 81: f.truncate(VOLSIZE) Line 74: manifest = self.make_manifest(path) Line 75: vg = manifest.sdUUID Line 76: lvm = FakeLVM(path) Line 77: voluuid = str(uuid.uuid4()) Line 78: volpath = lvm.lvPath(vg, voluuid) Here a comment can help # Create fake file for calculating the file size via dev symlink Or better, a function like: lvm.create_fake_lv_symlink(vgname, lvname) It belongs to fake lvm makes because it knows the root direcotory and the dev directory, and we can reuse this in many tests. Line 79: os.makedirs(os.path.dirname(volpath)) Line 80: with open(volpath, "w") as f: Line 81: f.truncate(VOLSIZE) Line 82: with MonkeyPatchScope([(blockSD, 'lvm', lvm)]): Line 79: os.makedirs(os.path.dirname(volpath)) Line 80: with open(volpath, "w") as f: Line 81: f.truncate(VOLSIZE) Line 82: with MonkeyPatchScope([(blockSD, 'lvm', lvm)]): Line 83: self.assertEqual(VOLSIZE, manifest.getVSize('unused', voluuid)) I think it will be more clear to use: manifest.getVSize("<imguuid>", voluuid) Otherwise I look at the code an I don't feel I know what's going on, unless I check the original code to understand what is "unused". Line 84: Line 85: def test_getvsize_via_lvm(self): Line 86: # Tests the slow path in getVSize when the device file is not present Line 87: with namedTemporaryDir() as path: Line 81: f.truncate(VOLSIZE) Line 82: with MonkeyPatchScope([(blockSD, 'lvm', lvm)]): Line 83: self.assertEqual(VOLSIZE, manifest.getVSize('unused', voluuid)) Line 84: Line 85: def test_getvsize_via_lvm(self): More correct to call this test_getvsize_inactive_lv() Line 86: # Tests the slow path in getVSize when the device file is not present Line 87: with namedTemporaryDir() as path: Line 88: manifest = self.make_manifest(path) Line 89: vg = manifest.sdUUID Line 82: with MonkeyPatchScope([(blockSD, 'lvm', lvm)]): Line 83: self.assertEqual(VOLSIZE, manifest.getVSize('unused', voluuid)) Line 84: Line 85: def test_getvsize_via_lvm(self): Line 86: # Tests the slow path in getVSize when the device file is not present Well this is may be the fast path if the lv is cached. The previous path involves few syscalls, and this path is just few dictionary lookups and python function calls. Line 87: with namedTemporaryDir() as path: Line 88: manifest = self.make_manifest(path) Line 89: vg = manifest.sdUUID Line 90: lvm = FakeLVM(path) Line 102: manifest.bind(dict()) Line 103: return manifest Line 104: Line 105: def make_vg(self, lvm, vg): Line 106: devices = list((str(uuid.uuid4()),)) * 10 Why do we need one item tuple for each device? Line 107: lvm.createVG(vg, devices, blockSD.STORAGE_UNREADY_DOMAIN_TAG, Line 108: blockSD.VG_METADATASIZE) Line 109: Line 110: Line 104: Line 105: def make_vg(self, lvm, vg): Line 106: devices = list((str(uuid.uuid4()),)) * 10 Line 107: lvm.createVG(vg, devices, blockSD.STORAGE_UNREADY_DOMAIN_TAG, Line 108: blockSD.VG_METADATASIZE) This is confusing , not clear if you call the fake lvm or the real one. Why not move this logic into FakeLvm.createVG? Line 109: Line 110: Line 111: def make_metafile(metafile): Line 112: os.makedirs(os.path.dirname(metafile)) Line 113: with open(metafile, "w") as f: Line 114: f.truncate(MDSIZE) Line 115: Line 116: Line 117: def make_volume(path, size): Same, path should be tmpdir or more specific name - looks like domaindir in this code. repodir/domaindir/images/imagedir/volname Lets use the same names for these locations in both tests and code. Line 118: imguuid = str(uuid.uuid4()) Line 119: voluuid = str(uuid.uuid4()) Line 120: imgpath = os.path.join(path, "images", imguuid) Line 121: volpath = os.path.join(imgpath, voluuid) https://gerrit.ovirt.org/#/c/41994/12/tests/storagefakelib.py File tests/storagefakelib.py: Line 38: devices = [self._fqpvname(dev) for dev in devices] Line 39: size = self._PV_SIZE * len(devices) Line 40: extent_count = size / extentsize Line 41: if metadataSize is None: Line 42: metadataSize = extentsize The original code does not allow None for metadataSize. The value must be a number, and the value 0 is treat specially, meaning - do not create metadata area. See lvm.createpv() Line 43: Line 44: attr = dict(permission='w', resizeable='z', exported='-', partial='-', Line 45: allocation='n', clustered='-') Line 46: md = dict(uuid=fake_guid(), name=vgName, attr=attr, size=size, Line 40: extent_count = size / extentsize Line 41: if metadataSize is None: Line 42: metadataSize = extentsize Line 43: Line 44: attr = dict(permission='w', resizeable='z', exported='-', partial='-', attr -> vg_attr? Line 45: allocation='n', clustered='-') Line 46: md = dict(uuid=fake_guid(), name=vgName, attr=attr, size=size, Line 47: free=size, extent_size=extentsize, extent_count=extent_count, Line 48: free_count=extent_count, tags=[initialTag], Line 42: metadataSize = extentsize Line 43: Line 44: attr = dict(permission='w', resizeable='z', exported='-', partial='-', Line 45: allocation='n', clustered='-') Line 46: md = dict(uuid=fake_guid(), name=vgName, attr=attr, size=size, md -> vg_md? Line 47: free=size, extent_size=extentsize, extent_count=extent_count, Line 48: free_count=extent_count, tags=[initialTag], Line 49: vg_mda_size=metadataSize, vg_mda_free=metadataSize, Line 50: lv_count='0', pv_count=len(devices), pv_name=tuple(devices), Line 47: free=size, extent_size=extentsize, extent_count=extent_count, Line 48: free_count=extent_count, tags=[initialTag], Line 49: vg_mda_size=metadataSize, vg_mda_free=metadataSize, Line 50: lv_count='0', pv_count=len(devices), pv_name=tuple(devices), Line 51: writeable=True, partial='OK') This is little too complicated, and it is very hard to read. If you keep the dicts, please create it with one key per line: dict( key1=value1, key2=value2, ... ) Line 52: self.vgmd[vgName] = md Line 53: Line 54: def createLV(self, vgName, lvName, size, activate=True, contiguous=False, Line 55: initialTag=None): Line 61: writeable=True, opened=False, active=True) Line 62: Line 63: if vgName not in self.lvmd: Line 64: self.lvmd[vgName] = dict() Line 65: self.lvmd[vgName][lvName] = md I wonder if it is not easier to create a class for vg attributes and vg, and just return the instance. The real code is accepting a nametuple and will access the object as obj.attribute, so it should work fine with regular mutable object, and we don't have to make copies and conversions etc. For example: class VGAttr(object): def __init__(self, permission='w', resizeable='z', exported='-', partial='-', allocation='n', clustered='-') self.permission = permission ... class VG(object): def __init__(self, arg1, ...): self.arg1 = arg1 ... and now our getters become simply: def getVG(self, name): return self.vgs[name] If you are worried about not detecting attempts to write to returned object, we can wrap it in: class Frozen(object): def __init__(self, obj): self.obj = obj def __getattr__(self, name): return getattr(self.obj, name) def __setattr__(self, name, value): raise AttributeError # whatever namedtuple raise when you try to modify it. And then: def getVG(self, name): return Frozen(self.vgs[name]) To modify fake lvm data, we can use: lvm.vgs[name].value = "new value" Line 66: Line 67: def lvPath(self, vgName, lvName): Line 68: return os.path.join(self.root, "dev", vgName, lvName) Line 69: Line 82: @staticmethod Line 83: def _fqpvname(pv): Line 84: if pv and not pv.startswith(real_lvm.PV_PREFIX): Line 85: pv = os.path.join(real_lvm.PV_PREFIX, pv) Line 86: return pv Can we use the real function? For example: _fqpvname = real_lvm._fqpvname This is private, but we are duplicating the original code, which is safe. If you don't like to access private function, lets make it public :-) This function does not belong to lvm anyway - it should not care about the location of the devices, just accept and return device paths (e.g. /dev/mapper/<guid>). Line 87: Line 88: Line 89: def fake_guid(): Line 90: chars = string.ascii_letters + string.digits Line 90: chars = string.ascii_letters + string.digits Line 91: Line 92: def part(size): Line 93: return ''.join(random.choice(chars) for _ in range(size)) Line 94: return '-'.join(part(size) for size in [6, 4, 4, 4, 4, 6]) The guids I'm seeing look like this: 360060160f4a030008b5463fdf56fe411 or like this: 1IET_00070003 So I would just use: def make_guid(): return os.urandom(16).encode('hex') Which creates guids like this: 2f18b8c8e7e602c1dde394f82183e9b0 -- To view, visit https://gerrit.ovirt.org/41994 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib00eba218cfb4af201aebcdc5071f95164c31687 Gerrit-PatchSet: 12 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
