Adam Litke has uploaded a new change for review.
Change subject: tests: FakeLVM test command failure ......................................................................
tests: FakeLVM test command failure
Change-Id: I954b750d744df0dbd9b7ac13138731d80db54bb8 Signed-off-by: Adam Litke alitke@redhat.com --- M tests/storagefakelib.py M tests/storagefakelibTests.py 2 files changed, 49 insertions(+), 8 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/64/47864/1
diff --git a/tests/storagefakelib.py b/tests/storagefakelib.py index 1548ab9..9e2536f 100644 --- a/tests/storagefakelib.py +++ b/tests/storagefakelib.py @@ -23,6 +23,7 @@ from copy import deepcopy
from storage import lvm as real_lvm +from storage import storage_exception as se
class FakeLVM(object): @@ -77,7 +78,10 @@ def createLV(self, vgName, lvName, size, activate=True, contiguous=False, initialTag=None): # Size is expected as a string in MB, convert to a string in bytes. - size = str(int(size) << 20) + try: + size = str(int(size) << 20) + except ValueError: + raise se.CannotCreateLogicalVolume(vgName, lvName)
# devices is hard to emulate properly (must have a PE allocator that # works the same as for LVM). Since we don't need this value, use None @@ -105,36 +109,57 @@ opened=False, active=bool(activate))
- vg_dict = self.lvmd.setdefault(vgName, {}) + try: + vg_dict = self.lvmd.setdefault(vgName, {}) + lv_count = int(self.vgmd[vgName]['lv_count']) + 1 + except KeyError: + raise se.CannotCreateLogicalVolume(vgName, lvName) + + if lvName in vg_dict: + raise se.CannotCreateLogicalVolume(vgName, lvName) + vg_dict[lvName] = lv_md - lv_count = int(self.vgmd[vgName]['lv_count']) + 1 self.vgmd[vgName]['lv_count'] = str(lv_count)
def activateLVs(self, vgName, lvNames): for lv in lvNames: - self.lvmd[vgName][lv]['active'] = True - self.lvmd[vgName][lv]['attr']['state'] = 'a' + try: + self.lvmd[vgName][lv]['active'] = True + self.lvmd[vgName][lv]['attr']['state'] = 'a' + except KeyError as e: + raise se.CannotActivateLogicalVolume(str(e))
def addtag(self, vg, lv, tag): - self.lvmd[vg][lv]['tags'] += (tag,) + try: + self.lvmd[vg][lv]['tags'] += (tag,) + except KeyError: + raise se.MissingTagOnLogicalVolume("%s/%s" % (vg, lv), tag)
def lvPath(self, vgName, lvName): return os.path.join(self.root, "dev", vgName, lvName)
def getPV(self, pvName): - md = deepcopy(self.pvmd[pvName]) + try: + md = deepcopy(self.pvmd[pvName]) + except KeyError: + raise se.InaccessiblePhysDev((pvName,)) vg_name = md['vg_name'] md['vg_uuid'] = self.vgmd[vg_name]['uuid'] return real_lvm.PV(**md)
def getVG(self, vgName): + if vgName not in self.vgmd: + raise se.VolumeGroupDoesNotExist(vgName) vg_md = deepcopy(self.vgmd[vgName]) vg_attr = real_lvm.VG_ATTR(**vg_md['attr']) vg_md['attr'] = vg_attr return real_lvm.VG(**vg_md)
def _getLV(self, vgName, lvName): - lv_md = deepcopy(self.lvmd[vgName][lvName]) + try: + lv_md = deepcopy(self.lvmd[vgName][lvName]) + except KeyError: + raise se.LogicalVolumeDoesNotExistError("%s/%s" % (vgName, lvName)) lv_attr = real_lvm.LV_ATTR(**lv_md['attr']) lv_md['attr'] = lv_attr return real_lvm.LV(**lv_md) diff --git a/tests/storagefakelibTests.py b/tests/storagefakelibTests.py index ca0d398..c3712dd 100644 --- a/tests/storagefakelibTests.py +++ b/tests/storagefakelibTests.py @@ -25,6 +25,7 @@ from storagefakelib import FakeLVM
from storage import blockSD, blockVolume +from storage import storage_exception as se
@expandPermutations @@ -281,6 +282,7 @@ self.assertEqual('a', lv.attr.state)
+@expandPermutations class FakeLVMGeneralTests(VdsmTestCase):
def test_lvpath(self): @@ -290,3 +292,17 @@ lv_name = 'bar' expected = os.path.join(tmpdir, 'dev', vg_name, lv_name) self.assertEqual(expected, lvm.lvPath(vg_name, lv_name)) + + @permutations([ + [se.VolumeGroupDoesNotExist, 'getVG', ['vg']], + [se.CannotActivateLogicalVolume, 'activateLVs', ['vg', ['lv']]], + [se.MissingTagOnLogicalVolume, 'addtag', ['vg', 'lv', 'tag']], + [se.CannotCreateLogicalVolume, 'createLV', ['vg', 'lv', '1024']], + [se.LogicalVolumeDoesNotExistError, 'getLV', ['vg', 'lv']], + [se.InaccessiblePhysDev, 'getPV', ['pv']], + ]) + def test_bad_args(self, exception, fn, args): + with namedTemporaryDir() as tmpdir: + lvm = FakeLVM(tmpdir) + lvm_fn = getattr(lvm, fn) + self.assertRaises(exception, lvm_fn, *args)
automation@ovirt.org has posted comments on this change.
Change subject: tests: FakeLVM test command failure ......................................................................
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: FakeLVM test command failure ......................................................................
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'])
automation@ovirt.org has posted comments on this change.
Change subject: tests: FakeLVM test command failure ......................................................................
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'])
automation@ovirt.org has posted comments on this change.
Change subject: tests: FakeLVM test command failure ......................................................................
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'])
automation@ovirt.org has posted comments on this change.
Change subject: tests: FakeLVM test command failure ......................................................................
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: FakeLVM test command failure ......................................................................
Patch Set 5: Code-Review-1
(8 comments)
https://gerrit.ovirt.org/#/c/47864/5/tests/storagefakelib.py File tests/storagefakelib.py:
Line 114: opened=False, Line 115: active=bool(activate)) Line 116: Line 117: try: Line 118: vg_dict = self.lvmd.setdefault(vgName, {}) This line cannot raise KeyError. Line 119: lv_count = int(self.vgmd[vgName]['lv_count']) + 1 Line 120: except KeyError: Line 121: raise se.CannotCreateLogicalVolume(vgName, lvName) Line 122:
Line 115: active=bool(activate)) Line 116: Line 117: try: Line 118: vg_dict = self.lvmd.setdefault(vgName, {}) Line 119: lv_count = int(self.vgmd[vgName]['lv_count']) + 1 We have both vgmd[vgname] and lvmd[vgname]? This is error prone.
lvm.py avoid this by using the tuple (vgname, lvname) as the key to the lvs dict. Line 120: except KeyError: Line 121: raise se.CannotCreateLogicalVolume(vgName, lvName) Line 122: Line 123: if lvName in vg_dict:
Line 128: Line 129: def activateLVs(self, vgName, lvNames): Line 130: for lv in lvNames: Line 131: try: Line 132: self.lvmd[vgName][lv]['active'] = True This can fail because of bug in fake lvm (missing 'active' key). Line 133: self.lvmd[vgName][lv]['attr']['state'] = 'a' Line 134: except KeyError as e: Line 135: raise se.CannotActivateLogicalVolume(str(e)) Line 136:
Line 129: def activateLVs(self, vgName, lvNames): Line 130: for lv in lvNames: Line 131: try: Line 132: self.lvmd[vgName][lv]['active'] = True Line 133: self.lvmd[vgName][lv]['attr']['state'] = 'a' If this line fails, this is a bug in fake lvm, not caller issue (missing 'attr' or 'state' keys) Line 134: except KeyError as e: Line 135: raise se.CannotActivateLogicalVolume(str(e)) Line 136: Line 137: def addtag(self, vg, lv, tag):
Line 131: try: Line 132: self.lvmd[vgName][lv]['active'] = True Line 133: self.lvmd[vgName][lv]['attr']['state'] = 'a' Line 134: except KeyError as e: Line 135: raise se.CannotActivateLogicalVolume(str(e)) Lets do this:
for lv in ...: try: lv = self.lvmd[vgName][lv] except KeyError: raise Cannot... lv['active'] = True ... Line 136: Line 137: def addtag(self, vg, lv, tag): Line 138: try: Line 139: self.lvmd[vg][lv]['tags'] += (tag,)
Line 135: raise se.CannotActivateLogicalVolume(str(e)) Line 136: Line 137: def addtag(self, vg, lv, tag): Line 138: try: Line 139: self.lvmd[vg][lv]['tags'] += (tag,) Same, missing tags is hidden Line 140: except KeyError: Line 141: raise se.MissingTagOnLogicalVolume("%s/%s" % (vg, lv), tag) Line 142: Line 143: def lvPath(self, vgName, lvName):
Line 144: return os.path.join(self.root, "dev", vgName, lvName) Line 145: Line 146: def getPV(self, pvName): Line 147: try: Line 148: md = deepcopy(self.pvmd[pvName]) Probably works, but better do:
try: pv = self.pvmd[pvName] except ... raise se.Inaccessible... md = deepcopy(pv) ... Line 149: except KeyError: Line 150: raise se.InaccessiblePhysDev((pvName,)) Line 151: return real_lvm.PV(**md) Line 152:
Line 159: return real_lvm.VG(**vg_md) Line 160: Line 161: def _getLV(self, vgName, lvName): Line 162: try: Line 163: lv_md = deepcopy(self.lvmd[vgName][lvName]) Probably works, but better keep deepcopy outside the try. Line 164: except KeyError: Line 165: raise se.LogicalVolumeDoesNotExistError("%s/%s" % (vgName, lvName)) Line 166: lv_attr = real_lvm.LV_ATTR(**lv_md['attr']) Line 167: lv_md['attr'] = lv_attr
Nir Soffer has posted comments on this change.
Change subject: tests: FakeLVM test command failure ......................................................................
Patch Set 5:
(1 comment)
https://gerrit.ovirt.org/#/c/47864/5/tests/storagefakelibTests.py File tests/storagefakelibTests.py:
Line 285: def test_bad_args(self, exception, fn, args): Line 286: with namedTemporaryDir() as tmpdir: Line 287: lvm = FakeLVM(tmpdir) Line 288: lvm_fn = getattr(lvm, fn) Line 289: self.assertRaises(exception, lvm_fn, *args) We don't need temporary directory, use:
lvm = FakeLVM(None)
automation@ovirt.org has posted comments on this change.
Change subject: tests: FakeLVM test command failure ......................................................................
Patch Set 6:
* 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'])
Adam Litke has posted comments on this change.
Change subject: tests: FakeLVM test command failure ......................................................................
Patch Set 5:
(8 comments)
https://gerrit.ovirt.org/#/c/47864/5/tests/storagefakelib.py File tests/storagefakelib.py:
Line 114: opened=False, Line 115: active=bool(activate)) Line 116: Line 117: try: Line 118: vg_dict = self.lvmd.setdefault(vgName, {})
This line cannot raise KeyError.
ok, moved above. Line 119: lv_count = int(self.vgmd[vgName]['lv_count']) + 1 Line 120: except KeyError: Line 121: raise se.CannotCreateLogicalVolume(vgName, lvName) Line 122:
Line 115: active=bool(activate)) Line 116: Line 117: try: Line 118: vg_dict = self.lvmd.setdefault(vgName, {}) Line 119: lv_count = int(self.vgmd[vgName]['lv_count']) + 1
We have both vgmd[vgname] and lvmd[vgname]? This is error prone.
hmm... Maybe for another patch. Line 120: except KeyError: Line 121: raise se.CannotCreateLogicalVolume(vgName, lvName) Line 122: Line 123: if lvName in vg_dict:
Line 128: Line 129: def activateLVs(self, vgName, lvNames): Line 130: for lv in lvNames: Line 131: try: Line 132: self.lvmd[vgName][lv]['active'] = True
This can fail because of bug in fake lvm (missing 'active' key).
fixed. Line 133: self.lvmd[vgName][lv]['attr']['state'] = 'a' Line 134: except KeyError as e: Line 135: raise se.CannotActivateLogicalVolume(str(e)) Line 136:
Line 129: def activateLVs(self, vgName, lvNames): Line 130: for lv in lvNames: Line 131: try: Line 132: self.lvmd[vgName][lv]['active'] = True Line 133: self.lvmd[vgName][lv]['attr']['state'] = 'a'
If this line fails, this is a bug in fake lvm, not caller issue (missing 'a
yep. Fixed. Line 134: except KeyError as e: Line 135: raise se.CannotActivateLogicalVolume(str(e)) Line 136: Line 137: def addtag(self, vg, lv, tag):
Line 131: try: Line 132: self.lvmd[vgName][lv]['active'] = True Line 133: self.lvmd[vgName][lv]['attr']['state'] = 'a' Line 134: except KeyError as e: Line 135: raise se.CannotActivateLogicalVolume(str(e))
Lets do this:
Done Line 136: Line 137: def addtag(self, vg, lv, tag): Line 138: try: Line 139: self.lvmd[vg][lv]['tags'] += (tag,)
Line 135: raise se.CannotActivateLogicalVolume(str(e)) Line 136: Line 137: def addtag(self, vg, lv, tag): Line 138: try: Line 139: self.lvmd[vg][lv]['tags'] += (tag,)
Same, missing tags is hidden
Done Line 140: except KeyError: Line 141: raise se.MissingTagOnLogicalVolume("%s/%s" % (vg, lv), tag) Line 142: Line 143: def lvPath(self, vgName, lvName):
Line 144: return os.path.join(self.root, "dev", vgName, lvName) Line 145: Line 146: def getPV(self, pvName): Line 147: try: Line 148: md = deepcopy(self.pvmd[pvName])
Probably works, but better do:
Done Line 149: except KeyError: Line 150: raise se.InaccessiblePhysDev((pvName,)) Line 151: return real_lvm.PV(**md) Line 152:
Line 159: return real_lvm.VG(**vg_md) Line 160: Line 161: def _getLV(self, vgName, lvName): Line 162: try: Line 163: lv_md = deepcopy(self.lvmd[vgName][lvName])
Probably works, but better keep deepcopy outside the try.
Done Line 164: except KeyError: Line 165: raise se.LogicalVolumeDoesNotExistError("%s/%s" % (vgName, lvName)) Line 166: lv_attr = real_lvm.LV_ATTR(**lv_md['attr']) Line 167: lv_md['attr'] = lv_attr
Adam Litke has posted comments on this change.
Change subject: tests: FakeLVM test command failure ......................................................................
Patch Set 6: Verified+1
Nir Soffer has posted comments on this change.
Change subject: tests: FakeLVM test command failure ......................................................................
Patch Set 6: Code-Review-1
(2 comments)
https://gerrit.ovirt.org/#/c/47864/6/tests/storagefakelib.py File tests/storagefakelib.py:
Line 34: (1 + _PV_MDA_COUNT)) Line 35: Line 36: def __init__(self, root): Line 37: self.root = root Line 38: if root is not None: Do you want to support root=""? I don't see a need for that, we assume that root is a tempoary directory created for the current test; it cannot be empty. Line 39: os.mkdir(os.path.join(self.root, 'dev')) Line 40: self.pvmd = {} Line 41: self.vgmd = {} Line 42: self.lvmd = {}
Line 173: Line 174: def getLV(self, vgName, lvName=None): Line 175: if lvName is None: Line 176: return [self._getLV(vgName, lv) Line 177: for (vg, lv) in self.lvmd.keys() if vg == vgName] Nice!
But the parenthesis are not needed, "for x, y in" is idiomatic Python. keys() is also not needed, iteration over dict return the keys. Line 178: else: Line 179: return self._getLV(vgName, lvName) Line 180: Line 181: def fake_lv_symlink_create(self, vg_name, lv_name):
automation@ovirt.org has posted comments on this change.
Change subject: tests: FakeLVM test command failure ......................................................................
Patch Set 7:
* 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'])
Adam Litke has posted comments on this change.
Change subject: tests: FakeLVM test command failure ......................................................................
Patch Set 7: Verified+1
Nir Soffer has posted comments on this change.
Change subject: tests: FakeLVM test command failure ......................................................................
Patch Set 7: Code-Review-1
(1 comment)
https://gerrit.ovirt.org/#/c/47864/7/tests/storagefakelib.py File tests/storagefakelib.py:
Line 118: opened=False, Line 119: active=bool(activate)) Line 120: Line 121: try: Line 122: lv_count = int(self.vgmd[vgName]['lv_count']) + 1 This still hides missing 'lv_count' key, will raise se.CannotCreateLogicalVolume, as if the vg does not exists.
Please separate modification and access. This line is doing too much. Line 123: except KeyError: Line 124: raise se.CannotCreateLogicalVolume(vgName, lvName) Line 125: Line 126: if (vgName, lvName) in self.lvmd:
automation@ovirt.org has posted comments on this change.
Change subject: tests: FakeLVM test command failure ......................................................................
Patch Set 8:
* 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: FakeLVM test command failure ......................................................................
Patch Set 8:
Jenkins complains about this: 00:04:33.872 ./tests/storagefakelibTests.py:290:1: W391 blank line at end of file
Nir Soffer has posted comments on this change.
Change subject: tests: FakeLVM test command failure ......................................................................
Patch Set 8: Code-Review-1
(1 comment)
https://gerrit.ovirt.org/#/c/47864/8/tests/storagefakelib.py File tests/storagefakelib.py:
Line 115: opened=False, Line 116: active=bool(activate)) Line 117: Line 118: try: Line 119: lv_count = int(self.vgmd[vgName]['lv_count']) + 1 Seems that you missed my comment about missing 'lv_count', would be hidden as missing vg.
Should separate getting the vg from modifying it. Line 120: except KeyError: Line 121: raise se.CannotCreateLogicalVolume(vgName, lvName) Line 122: Line 123: if (vgName, lvName) in self.lvmd:
vdsm-patches@lists.fedorahosted.org