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 -- To view, visit https://gerrit.ovirt.org/47864 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I954b750d744df0dbd9b7ac13138731d80db54bb8 Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches