Adam Litke has uploaded a new change for review.
Change subject: tests: Add initial FakeLVM tests and fix errors ......................................................................
tests: Add initial FakeLVM tests and fix errors
The FakeLVM class inside storagefakelib.py is becoming increasingly complex. In order to ensure that FakeLVM is providing correct LVM emulation we need to test its output against what a real LVM environment would present.
This first patch introduces the new test module and adds some tests for createVG and getVG. We test that the VG properties and attributes returned match a real LVM installation. After writing these tests I found and fixed several errors in the existing FakeLVM implementation.
Change-Id: I1650b98f25df1a22ba2a0920b093e78fc2fdee88 Signed-off-by: Adam Litke alitke@redhat.com --- M tests/Makefile.am M tests/storagefakelib.py A tests/storagefakelibTests.py 3 files changed, 161 insertions(+), 16 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/57/47857/1
diff --git a/tests/Makefile.am b/tests/Makefile.am index 1168862..e8b9d6e 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -102,6 +102,7 @@ stompAdapterTests.py \ stompAsyncClientTests.py \ stompTests.py \ + storagefakelibTests.py \ storageMailboxTests.py \ storageMonitorTests.py \ storageServerTests.py \ diff --git a/tests/storagefakelib.py b/tests/storagefakelib.py index 11954d5..a60aaf5 100644 --- a/tests/storagefakelib.py +++ b/tests/storagefakelib.py @@ -26,19 +26,29 @@
class FakeLVM(object): - _PV_SIZE = 10 << 30 # We pretend all PVs contribute 10G of space - + _PV_SIZE = 10 << 30 # We pretend all PVs are 10G in size + _PV_PE_SIZE = 128 << 20 # Found via inspection of real environment + _PV_MDA_COUNT = 2 # The number of PEs used for metadata areas + _PV_UNUSABLE = (_PV_PE_SIZE * # 2 PE for metadata + 1 PE to hold a header + (1 + _PV_MDA_COUNT)) def __init__(self, root): self.root = root os.mkdir(os.path.join(self.root, 'dev')) + self.pvmd = {} self.vgmd = {} self.lvmd = {}
def createVG(self, vgName, devices, initialTag, metadataSize, - extentsize=134217728, force=False): - devices = [_fqpvname(dev) for dev in devices] - size = self._PV_SIZE * len(devices) - extent_count = size / extentsize + extentsize=128, force=False): + # Convert params from MB to bytes to match other fields + metadataSize <<= 20 + extentsize <<= 20 + + for dev in devices: + self._create_pv(dev, vgName, self._PV_SIZE) + pv_name = tuple([_fqpvname(pdev) for pdev in _normalizeargs(devices)]) + extent_count = self._calc_vg_pe_count(vgName) + size = extent_count * self._PV_PE_SIZE
vg_attr = dict(permission='w', resizeable='z', @@ -49,17 +59,17 @@ vg_md = dict(uuid=fake_lvm_uuid(), name=vgName, attr=vg_attr, - size=size, - free=size, - extent_size=extentsize, - extent_count=extent_count, - free_count=extent_count, - tags=[initialTag], - vg_mda_size=metadataSize, - vg_mda_free=metadataSize, + size=str(size), + free=str(size), + extent_size=str(extentsize), + extent_count=str(extent_count), + free_count=str(extent_count), + tags=(initialTag,), + vg_mda_size=str(metadataSize), + vg_mda_free=None, lv_count='0', - pv_count=len(devices), - pv_name=tuple(devices), + pv_count=str(len(devices)), + pv_name=pv_name, writeable=True, partial='OK') self.vgmd[vgName] = vg_md @@ -123,7 +133,31 @@ with open(volpath, "w") as f: f.truncate(int(self.lvmd[vg_name][lv_name]['size']))
+ def _create_pv(self, pv_name, vg_name, size): + # pe_start is difficult to calculate correctly but since it's not + # currently needed by users of FakeLVM, set it to None. + pe_start = None + pe_count = (size - self._PV_UNUSABLE) / self._PV_PE_SIZE + pv_md = dict(uuid=fake_lvm_uuid(), + name='/dev/mapper/%s' % pv_name, + guid=pv_name, + size=str(pe_count * self._PV_PE_SIZE), + vg_name=vg_name, + vg_uuid=None, # This is calculated dynamically + pe_start=pe_start, + pe_count=str(pe_count), + pe_alloc_count='0', + mda_count=str(self._PV_MDA_COUNT), + dev_size=str(self._PV_SIZE)) + self.pvmd[pv_name] = pv_md + + def _calc_vg_pe_count(self, vg_name): + return sum(int(pv["pe_count"]) for pv in self.pvmd.values() + if pv["vg_name"] == vg_name) + + _fqpvname = real_lvm._fqpvname +_normalizeargs = real_lvm._normalizeargs
def fake_lvm_uuid(): diff --git a/tests/storagefakelibTests.py b/tests/storagefakelibTests.py new file mode 100644 index 0000000..8c73a54 --- /dev/null +++ b/tests/storagefakelibTests.py @@ -0,0 +1,110 @@ +# Copyright 2015 Red Hat, Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA +# +# Refer to the README and COPYING files for full details of the license +# + +from contextlib import contextmanager + +from testlib import VdsmTestCase, namedTemporaryDir +from testlib import permutations, expandPermutations +from storagefakelib import FakeLVM + +from storage import blockSD + + +@expandPermutations +class FakeLVMSimpleVGTests(VdsmTestCase): + VG_NAME = '1ffead52-7363-4968-a8c7-3bc34504d452' + DEVICES = ['360014054d75cb132d474c0eae9825766'] + + @contextmanager + def base_config(self): + """ + Create a simple volume group from a single 10G LUN. + + lvm.createVG('1ffead52-7363-4968-a8c7-3bc34504d452', + ['360014054d75cb132d474c0eae9825766'], + blockSD.STORAGE_UNREADY_DOMAIN_TAG, + blockSD.VG_METADATASIZE) + + print lvm.getVG('1ffead52-7363-4968-a8c7-3bc34504d452') + VG(uuid='15hlPF-V3eG-F9Cp-SGtu-4Mq0-28Do-HC806y', + name='1ffead52-7363-4968-a8c7-3bc34504d452', + attr=VG_ATTR(permission='w', resizeable='z', exported='-', + partial='-', allocation='n', clustered='-'), + size='10334765056', free='10334765056', extent_size='134217728', + extent_count='77', free_count='77', + tags=('RHAT_storage_domain_UNREADY',), vg_mda_size='134217728', + vg_mda_free='67107328', lv_count='0', pv_count='1', + pv_name=('/dev/mapper/360014054d75cb132d474c0eae9825766',), + writeable=True, partial='OK') + + print lvm.getPV('360014054d75cb132d474c0eae9825766') + PV(uuid='fIRjbD-usOA-tYgW-b2Uz-oUly-AJ49-bMMjYe', + name='/dev/mapper/360014054d75cb132d474c0eae9825766', + size='10334765056', vg_name='1ffead52-7363-4968-a8c7-3bc34504d452', + vg_uuid='15hlPF-V3eG-F9Cp-SGtu-4Mq0-28Do-HC806y', + pe_start='138412032', pe_count='77', pe_alloc_count='0', + mda_count='2', dev_size='10737418240', + guid='360014054d75cb132d474c0eae9825766') + """ + with namedTemporaryDir() as tmpdir: + lvm = FakeLVM(tmpdir) + lvm.createVG(self.VG_NAME, self.DEVICES, + blockSD.STORAGE_UNREADY_DOMAIN_TAG, + blockSD.VG_METADATASIZE) + yield lvm + + @permutations([ + ['name', VG_NAME], + ['size', '10334765056'], + ['free', '10334765056'], + ['extent_size', '134217728'], + ['extent_count', '77'], + ['free_count', '77'], + ['tags', (blockSD.STORAGE_UNREADY_DOMAIN_TAG,)], + ['vg_mda_size', '134217728'], + ['lv_count', '0'], + ['pv_count', '1'], + ['writeable', True], + ['partial', 'OK'], + ['pv_name', tuple(['/dev/mapper/%s' % d for d in DEVICES])], + ]) + def test_vg_properties(self, var, val): + with self.base_config() as lvm: + vg = lvm.getVG(self.VG_NAME) + self.assertEqual(val, getattr(vg, var)) + + @permutations([ + ['permission', 'w'], + ['resizeable', 'z'], + ['exported', '-'], + ['partial', '-'], + ['allocation', 'n'], + ['clustered', '-'], + ]) + def test_vg_attributes(self, var, val): + with self.base_config() as lvm: + vg = lvm.getVG(self.VG_NAME) + self.assertEqual(val, getattr(vg.attr, var)) + + def test_vg_mda_free(self): + # It is too complex to emulate vg_mda_free and at this point we do not + # rely on this value. For this reason, FakeLVM sets it to None. + with self.base_config() as lvm: + vg = lvm.getVG(self.VG_NAME) + self.assertEqual(None, vg.vg_mda_free)
automation@ovirt.org has posted comments on this change.
Change subject: tests: Add initial FakeLVM tests and fix errors ......................................................................
Patch Set 1:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
automation@ovirt.org has posted comments on this change.
Change subject: tests: Add initial FakeLVM tests and fix errors ......................................................................
Patch Set 2:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Nir Soffer has posted comments on this change.
Change subject: tests: Add initial FakeLVM tests and fix errors ......................................................................
Patch Set 2: Code-Review-1
(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 real constant instead of this. 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. 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 cleaner. 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? 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 from instead of creating this shortcut. 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? 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? 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 this helper, or create more generic helper that can be used like this:
with storagetestlib.FakeLVM() as lvm: use lvm ...
The temporary directory can be created in FakeLVM.__enter__ and removed in FakeLVM.__exit__.
If we have class that need common setup code, we can put the setup code in a method:
with storagetestlib.FakeLVM() as lvm: self.setup_lvm(lvm) specific test code 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 per value. 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 test any logic here that effect the permissions, right? 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.
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.
automation@ovirt.org has posted comments on this change.
Change subject: tests: Add initial FakeLVM tests and fix errors ......................................................................
Patch Set 3:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Nir Soffer has posted comments on this change.
Change subject: tests: Add initial FakeLVM tests and fix errors ......................................................................
Patch Set 2:
(4 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
But is the PE size always the same as the PE metadata size? If you used a
I think that extent size is the smallest unit you can manage in lvm, and metadata area will use whole extent. I think it is safe for now to assume this is true. 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
It's hard-coded in _createpv
ok :-) 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):
https://gerrit.ovirt.org/#/c/47857/2/tests/storagefakelibTests.py File tests/storagefakelibTests.py:
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
I fail to see why moving this to a separate class is any neater. I like ha
Having setup code for each fixture makes sense, but combining it with code to create temporary directory and fake lvm and returning fake lvm from the context manager is less convincing. 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))
I wrote it this way because in the sdm indirection tests you suggested that
I think we don't need permutations when we have one fixture. The is little chance that initialization of the fake lv will fail, as there is very little logic there, and using one permutation for each tested value means we are creating 13 temporary directories for checking contents of a dict.
You can test all values in one test, or maybe separate the few values that has some logic, like you did with the attributes.
Lets try to make these tests simplest as possible. Line 91: Line 92: @permutations([ Line 93: ['permission', 'w'], Line 94: ['resizeable', 'z'],
Nir Soffer has posted comments on this change.
Change subject: tests: Add initial FakeLVM tests and fix errors ......................................................................
Patch Set 3:
(1 comment)
https://gerrit.ovirt.org/#/c/47857/3/tests/storagefakelibTests.py File tests/storagefakelibTests.py:
Line 92: Line 93: def test_vg_attributes(self): Line 94: expected = real_lvm.VG_ATTR(permission='w', resizeable='z', Line 95: exported='-', partial='-', Line 96: allocation='n', clustered='-') This is much nicer - I can both the type of the object, and the value of the attributes.
Can we convert the previous test to use the same style? Line 97: with self.base_config() as lvm: Line 98: vg = lvm.getVG(self.VG_NAME) Line 99: self.assertEqual(expected, vg.attr) Line 100:
Adam Litke has posted comments on this change.
Change subject: tests: Add initial FakeLVM tests and fix errors ......................................................................
Patch Set 2:
(1 comment)
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
I think that extent size is the smallest unit you can manage in lvm, and me
Actually there is no constant for this one either. It's hard-coded in the same code (_createpv) as a default argument! Please let's stick with these clean constants that can be verified against the supplied lvm.py output in the tests. 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:
Adam Litke has posted comments on this change.
Change subject: tests: Add initial FakeLVM tests and fix errors ......................................................................
Patch Set 3:
(1 comment)
https://gerrit.ovirt.org/#/c/47857/3/tests/storagefakelibTests.py File tests/storagefakelibTests.py:
Line 92: Line 93: def test_vg_attributes(self): Line 94: expected = real_lvm.VG_ATTR(permission='w', resizeable='z', Line 95: exported='-', partial='-', Line 96: allocation='n', clustered='-')
This is much nicer - I can both the type of the object, and the value of th
Done Line 97: with self.base_config() as lvm: Line 98: vg = lvm.getVG(self.VG_NAME) Line 99: self.assertEqual(expected, vg.attr) Line 100:
automation@ovirt.org has posted comments on this change.
Change subject: tests: Add initial FakeLVM tests and fix errors ......................................................................
Patch Set 4:
* Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Nir Soffer has posted comments on this change.
Change subject: tests: Add initial FakeLVM tests and fix errors ......................................................................
Patch Set 4: Code-Review+2
Adam Litke has posted comments on this change.
Change subject: tests: Add initial FakeLVM tests and fix errors ......................................................................
Patch Set 4: Verified+1
automation@ovirt.org has posted comments on this change.
Change subject: tests: Add initial FakeLVM tests and fix errors ......................................................................
Patch Set 5:
* Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Nir Soffer has posted comments on this change.
Change subject: tests: Add initial FakeLVM tests and fix errors ......................................................................
Patch Set 5: Code-Review+2
Adam Litke has posted comments on this change.
Change subject: tests: Add initial FakeLVM tests and fix errors ......................................................................
Patch Set 5: Verified+1
Dan Kenigsberg has submitted this change and it was merged.
Change subject: tests: Add initial FakeLVM tests and fix errors ......................................................................
tests: Add initial FakeLVM tests and fix errors
The FakeLVM class inside storagefakelib.py is becoming increasingly complex. In order to ensure that FakeLVM is providing correct LVM emulation we need to test its output against what a real LVM environment would present.
This first patch introduces the new test module and adds some tests for createVG and getVG. We test that the VG properties and attributes returned match a real LVM installation. After writing these tests I found and fixed several errors in the existing FakeLVM implementation.
Change-Id: I1650b98f25df1a22ba2a0920b093e78fc2fdee88 Signed-off-by: Adam Litke alitke@redhat.com Reviewed-on: https://gerrit.ovirt.org/47857 Continuous-Integration: Jenkins CI Reviewed-by: Nir Soffer nsoffer@redhat.com --- M tests/Makefile.am M tests/storagefakelib.py A tests/storagefakelibTests.py 3 files changed, 158 insertions(+), 15 deletions(-)
Approvals: Adam Litke: Verified Nir Soffer: Looks good to me, approved Jenkins CI: Passed CI tests
automation@ovirt.org has posted comments on this change.
Change subject: tests: Add initial FakeLVM tests and fix errors ......................................................................
Patch Set 6:
* Update tracker: IGNORE, no Bug-Url found * Set MODIFIED::IGNORE, no Bug-Url found.
vdsm-patches@lists.fedorahosted.org