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