Adam Litke has posted comments on this change.
Change subject: tests: Add initial FakeLVM tests and fix errors ......................................................................
Patch Set 2:
(10 comments)
https://gerrit.ovirt.org/#/c/47857/2/tests/storagefakelib.py File tests/storagefakelib.py:
Line 26: Line 27: Line 28: class FakeLVM(object): Line 29: _PV_SIZE = 10 << 30 # We pretend all PVs are 10G in size Line 30: _PV_PE_SIZE = 128 << 20 # Found via inspection of real environment
This is metadata size used when creating a pv ("128m"). So we can use the r
But is the PE size always the same as the PE metadata size? If you used a really large PE size your metadata might not consume the whole PE. Line 31: _PV_MDA_COUNT = 2 # The number of PEs used for metadata areas Line 32: _PV_UNUSABLE = (_PV_PE_SIZE * # 2 PE for metadata + 1 PE to hold a header Line 33: (1 + _PV_MDA_COUNT)) Line 34:
Line 27: Line 28: class FakeLVM(object): Line 29: _PV_SIZE = 10 << 30 # We pretend all PVs are 10G in size Line 30: _PV_PE_SIZE = 128 << 20 # Found via inspection of real environment Line 31: _PV_MDA_COUNT = 2 # The number of PEs used for metadata areas
Don't we have constant for this? We configure this value when crating a pv.
It's hard-coded in _createpv
cmd.extend(("--metadatasize", "%sm" % metadataSize, "--metadatacopies", "2", "--metadataignore", "y"))
Please don't make me fix every deficiency in the lvm implementation in order to test what's already there. Line 32: _PV_UNUSABLE = (_PV_PE_SIZE * # 2 PE for metadata + 1 PE to hold a header Line 33: (1 + _PV_MDA_COUNT)) Line 34: Line 35: def __init__(self, root):
Line 46: extentsize <<= 20 Line 47: Line 48: for dev in devices: Line 49: self._create_pv(dev, vgName, self._PV_SIZE) Line 50: pv_name = tuple([_fqpvname(pdev) for pdev in _normalizeargs(devices)])
We don't need the temporary list (tupele(for x in y)) works and little clea
ok Line 51: extent_count = self._calc_vg_pe_count(vgName) Line 52: size = extent_count * self._PV_PE_SIZE Line 53: Line 54: vg_attr = dict(permission='w',
Line 143: name='/dev/mapper/%s' % pv_name, Line 144: guid=pv_name, Line 145: size=str(pe_count * self._PV_PE_SIZE), Line 146: vg_name=vg_name, Line 147: vg_uuid=None, # This is calculated dynamically
Why not initialize this now?
Because the PV is created first (before the vg) so we don't have the vg uuid yet. Line 148: pe_start=pe_start, Line 149: pe_count=str(pe_count), Line 150: pe_alloc_count='0', Line 151: mda_count=str(self._PV_MDA_COUNT),
Line 157: if pv["vg_name"] == vg_name) Line 158: Line 159: Line 160: _fqpvname = real_lvm._fqpvname Line 161: _normalizeargs = real_lvm._normalizeargs
I would use real_lvm.name everywhere to make it clear where this name came
ok. Line 162: Line 163: Line 164: def fake_lvm_uuid(): Line 165: chars = string.ascii_letters + string.digits
https://gerrit.ovirt.org/#/c/47857/2/tests/storagefakelibTests.py File tests/storagefakelibTests.py:
Line 50: extent_count='77', free_count='77', Line 51: tags=('RHAT_storage_domain_UNREADY',), vg_mda_size='134217728', Line 52: vg_mda_free='67107328', lv_count='0', pv_count='1', Line 53: pv_name=('/dev/mapper/360014054d75cb132d474c0eae9825766',), Line 54: writeable=True, partial='OK')
Is this output from real lvm module?
yes. Line 55: Line 56: print lvm.getPV('360014054d75cb132d474c0eae9825766') Line 57: PV(uuid='fIRjbD-usOA-tYgW-b2Uz-oUly-AJ49-bMMjYe', Line 58: name='/dev/mapper/360014054d75cb132d474c0eae9825766',
Line 59: size='10334765056', vg_name='1ffead52-7363-4968-a8c7-3bc34504d452', Line 60: vg_uuid='15hlPF-V3eG-F9Cp-SGtu-4Mq0-28Do-HC806y', Line 61: pe_start='138412032', pe_count='77', pe_alloc_count='0', Line 62: mda_count='2', dev_size='10737418240', Line 63: guid='360014054d75cb132d474c0eae9825766')
Is this relevant?
It will be for future patches. It's here now because I want to be clear that the data came from the same session with the real lvm module. Line 64: """ Line 65: with namedTemporaryDir() as tmpdir: Line 66: lvm = FakeLVM(tmpdir) Line 67: lvm.createVG(self.VG_NAME, self.DEVICES,
Line 66: lvm = FakeLVM(tmpdir) Line 67: lvm.createVG(self.VG_NAME, self.DEVICES, Line 68: blockSD.STORAGE_UNREADY_DOMAIN_TAG, Line 69: blockSD.VG_METADATASIZE) Line 70: yield lvm
Not sure about this helper. I would let each test use this code without thi
I fail to see why moving this to a separate class is any neater. I like having the setup in a method as above because I can document the relevant lvm module output in the docstring. If I want to add different scenarios, I can add a new method with a new docstring and tests can use that other scenario instead. It seems like a clean and extensible approach to me. Line 71: Line 72: @permutations([ Line 73: ['name', VG_NAME], Line 74: ['size', '10334765056'],
Line 86: ]) Line 87: def test_vg_properties(self, var, val): Line 88: with self.base_config() as lvm: Line 89: vg = lvm.getVG(self.VG_NAME) Line 90: self.assertEqual(val, getattr(vg, var))
Not sure we need the permutations. I would simplify this with one assert pe
I wrote it this way because in the sdm indirection tests you suggested that each test invocation should only test one thing and that a failure of one property should not mask another failure. If we add these all as explicit asserts in series, then the first failed assert will fail the test and we will not be able to see which other properties are also incorrect. Line 91: Line 92: @permutations([ Line 93: ['permission', 'w'], Line 94: ['resizeable', 'z'],
Line 99: ]) Line 100: def test_vg_attributes(self, var, val): Line 101: with self.base_config() as lvm: Line 102: vg = lvm.getVG(self.VG_NAME) Line 103: self.assertEqual(val, getattr(vg.attr, var))
I think that one test checking all permissions is good enough, as we don't
Yeah, I can agree with that. Line 104: Line 105: def test_vg_mda_free(self): Line 106: # It is too complex to emulate vg_mda_free and at this point we do not Line 107: # rely on this value. For this reason, FakeLVM sets it to None.