Change in vdsm[master]: Improve err msg when multipath cant access a pv
Vered Volansky has posted comments on this change. Change subject: Improve err msg when multipath cant access a pv .. Patch Set 2: (5 comments) Accepted Dan's suggestion, will submit soon. Commit Message Line 3: AuthorDate: 2013-11-05 17:33:36 +0200 Line 4: Commit: Vered Volansky vvola...@redhat.com Line 5: CommitDate: 2013-11-10 13:40:15 +0200 Line 6: Line 7: Improve err msg when multipath cant access a pv Because there's a limit to how long the error message can be, and I'm a character away from that limit. Line 8: Line 9: InvalidPhysDev gave a cryptic error message - Invalid physical device. Line 10: The message was amended to Multipath cannot access storage device along Line 11: with the device name. The error key was changed according to the new Line 3: AuthorDate: 2013-11-05 17:33:36 +0200 Line 4: Commit: Vered Volansky vvola...@redhat.com Line 5: CommitDate: 2013-11-10 13:40:15 +0200 Line 6: Line 7: Improve err msg when multipath cant access a pv Wasn't there before, I thought it might fail the script. If it won't, will add. Line 8: Line 9: InvalidPhysDev gave a cryptic error message - Invalid physical device. Line 10: The message was amended to Multipath cannot access storage device along Line 11: with the device name. The error key was changed according to the new Line 7: Improve err msg when multipath cant access a pv Line 8: Line 9: InvalidPhysDev gave a cryptic error message - Invalid physical device. Line 10: The message was amended to Multipath cannot access storage device along Line 11: with the device name. The error key was changed according to the new Done to both. Line 12: message content. The only thing needed here by the engine is the Line 13: error code, which was not changed in this patch. Line 14: Line 15: Change-Id: I648ee519873c51573e6e6306b79380f54bb25d2e File vdsm/storage/hsm.py Line 2037: in multipath.getMPDevNamesIter()) Line 2038: size = 0 Line 2039: devices = [] Line 2040: Line 2041: for dev in devlist: More important does not mean the other issue is NOT important. Current situation is exception-name almost equals already-agreed-upon-bad-message. Why do you want to fix the whole thing and then have an exception name that turns not really related to it's error message? What if were totally different? would you still want the older exception name? I think you should look at this extremity to decide, and if you still think the naming should remain the same please explain why. Line 2042: if dev in knowndevs: Line 2043: devices.append(dev) Line 2044: size += multipath.getDeviceSize(devicemapper.getDmId(dev)) Line 2045: else: File vdsm/storage/storage_exception.py Line 1456: class InaccessiblePhysDev(StorageException): Line 1457: def __init__(self, pvname): Line 1458: self.value = pvname=%s % (pvname) Line 1459: code = 606 Line 1460: message = Multipath cannot access storage device Ack. Note that when digging into the code in this exact flow the same devices are referred to as pvs. Maybe this should be changed as well (in a different patch, of course). Line 1461: Line 1462: Line 1463: class PartitionedPhysDev(StorageException): Line 1464: code = 607 -- To view, visit http://gerrit.ovirt.org/21089 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I648ee519873c51573e6e6306b79380f54bb25d2e Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Vered Volansky vvola...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Vered Volansky vvola...@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Improve err msg when multipath cant access a pv
Vered Volansky has posted comments on this change. Change subject: Improve err msg when multipath cant access a pv .. Patch Set 2: Verified+1 Addressed comments on previous patch. -- To view, visit http://gerrit.ovirt.org/21089 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I648ee519873c51573e6e6306b79380f54bb25d2e Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Vered Volansky vvola...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Vered Volansky vvola...@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Improve err msg when multipath cant access a pv
oVirt Jenkins CI Server has posted comments on this change. Change subject: Improve err msg when multipath cant access a pv .. Patch Set 2: Build Successful http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4509/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5309/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5387/ : SUCCESS -- To view, visit http://gerrit.ovirt.org/21089 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I648ee519873c51573e6e6306b79380f54bb25d2e Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Vered Volansky vvola...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Vered Volansky vvola...@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Improve err msg when multipath cant access a pv
Nir Soffer has posted comments on this change. Change subject: Improve err msg when multipath cant access a pv .. Patch Set 2: (3 comments) Commit Message Line 3: AuthorDate: 2013-11-05 17:33:36 +0200 Line 4: Commit: Vered Volansky vvola...@redhat.com Line 5: CommitDate: 2013-11-10 13:40:15 +0200 Line 6: Line 7: Improve err msg when multipath cant access a pv Not sure why you are trying to keep this so short - the topic: prefix and using real words (error instead of err) is more important then whatever limits that may be here. Line 8: Line 9: InvalidPhysDev gave a cryptic error message - Invalid physical device. Line 10: The message was amended to Multipath cannot access storage device along Line 11: with the device name. The error key was changed according to the new Line 7: Improve err msg when multipath cant access a pv Line 8: Line 9: InvalidPhysDev gave a cryptic error message - Invalid physical device. Line 10: The message was amended to Multipath cannot access storage device along Line 11: with the device name. The error key was changed according to the new You cannot use engine terms here - there is no such thing as error key in vdsm. And the change was a rename - please be specific. Line 12: message content. The only thing needed here by the engine is the Line 13: error code, which was not changed in this patch. Line 14: Line 15: Change-Id: I648ee519873c51573e6e6306b79380f54bb25d2e File vdsm/storage/storage_exception.py Line 1456: class InaccessiblePhysDev(StorageException): Line 1457: def __init__(self, pvname): Line 1458: self.value = pvname=%s % (pvname) Line 1459: code = 606 Line 1460: message = Multipath cannot access storage device Looking at it again, the term storage device is less correct then physical device. Line 1461: Line 1462: Line 1463: class PartitionedPhysDev(StorageException): Line 1464: code = 607 -- To view, visit http://gerrit.ovirt.org/21089 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I648ee519873c51573e6e6306b79380f54bb25d2e Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Vered Volansky vvola...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Vered Volansky vvola...@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Improve err msg when multipath cant access a pv
Nir Soffer has posted comments on this change. Change subject: Improve err msg when multipath cant access a pv .. Patch Set 2: Nir, I disagree. The name no longer (or was ever) fits the content and IMO has to change within this scope. I'm still not sure about it - invalid device is the error, engine ask to do something with foo but there is no such device. Seems valid to me. Multipath cannot access pyhsical device: foo is the error reason. Inaccessible device may be there is such device but I cannot access it. I'm sure that this rename is not needed to fix this bug. -- To view, visit http://gerrit.ovirt.org/21089 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I648ee519873c51573e6e6306b79380f54bb25d2e Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Vered Volansky vvola...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Vered Volansky vvola...@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Improve err msg when multipath cant access a pv
Dan Kenigsberg has posted comments on this change. Change subject: Improve err msg when multipath cant access a pv .. Patch Set 2: (2 comments) File vdsm/storage/hsm.py Line 2037: in multipath.getMPDevNamesIter()) Line 2038: size = 0 Line 2039: devices = [] Line 2040: Line 2041: for dev in devlist: I share Nir's opinion about the exception name and text changes. More important to give fuller information, and use simpler and quicker set operations. devlist = set(devlist) devices = devlist knowndevs unknown = devlist - knowndevs if unknown: raise whatever(unknown) Line 2042: if dev in knowndevs: Line 2043: devices.append(dev) Line 2044: size += multipath.getDeviceSize(devicemapper.getDmId(dev)) Line 2045: else: File vdsm/storage/storage_exception.py Line 1456: class InaccessiblePhysDev(StorageException): Line 1457: def __init__(self, pvname): Line 1458: self.value = pvname=%s % (pvname) Line 1459: code = 606 Line 1460: message = Multipath cannot access storage device ...and pvname may be wrong as well. We look for luns, to make pvs out of them. Line 1461: Line 1462: Line 1463: class PartitionedPhysDev(StorageException): Line 1464: code = 607 -- To view, visit http://gerrit.ovirt.org/21089 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I648ee519873c51573e6e6306b79380f54bb25d2e Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Vered Volansky vvola...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Vered Volansky vvola...@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Improve err msg when multipath cant access a pv
Nir Soffer has posted comments on this change. Change subject: Improve err msg when multipath cant access a pv .. Patch Set 2: (3 comments) I think the direction that Dan suggested, displaying list of all missing devices in the exception would give very useful error message. File vdsm/storage/blockSD.py Line 723: if len(mapping) + len(devlist) MAX_PVS: Line 724: raise se.StorageDomainIsMadeFromTooManyPVs() Line 725: Line 726: knowndevs = list(multipath.getMPDevNamesIter()) Line 727: devices = [] Use same logic as Dan suggested in the other call site and call with the list of unknown devices. Line 728: Line 729: for dev in devlist: Line 730: if dev in knowndevs: Line 731: devices.append(dev) File vdsm/storage/hsm.py Line 2037: in multipath.getMPDevNamesIter()) Line 2038: size = 0 Line 2039: devices = [] Line 2040: Line 2041: for dev in devlist: Nice - hopefully we will never have 100's of pvs missing? Line 2042: if dev in knowndevs: Line 2043: devices.append(dev) Line 2044: size += multipath.getDeviceSize(devicemapper.getDmId(dev)) Line 2045: else: File vdsm/storage/storage_exception.py Line 1456: class InaccessiblePhysDev(StorageException): Line 1457: def __init__(self, pvname): Line 1458: self.value = pvname=%s % (pvname) Line 1459: code = 606 Line 1460: message = Multipath cannot access storage device def __init__(self, devices): self.value = 'devices=%s' % (devices,) ? Line 1461: Line 1462: Line 1463: class PartitionedPhysDev(StorageException): Line 1464: code = 607 -- To view, visit http://gerrit.ovirt.org/21089 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I648ee519873c51573e6e6306b79380f54bb25d2e Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Vered Volansky vvola...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Vered Volansky vvola...@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Improve err msg when multipath cant access a pv
Allon Mureinik has posted comments on this change. Change subject: Improve err msg when multipath cant access a pv .. Patch Set 2: (2 comments) +1 on the approach Dan suggested Commit Message Line 3: AuthorDate: 2013-11-05 17:33:36 +0200 Line 4: Commit: Vered Volansky vvola...@redhat.com Line 5: CommitDate: 2013-11-10 13:40:15 +0200 Line 6: Line 7: Improve err msg when multipath cant access a pv sorry for noticing on the previous run - cant-can't Line 8: Line 9: InvalidPhysDev gave a cryptic error message - Invalid physical device. Line 10: The message was amended to Multipath cannot access storage device along Line 11: with the device name. The error key was changed according to the new Line 7: Improve err msg when multipath cant access a pv Line 8: Line 9: InvalidPhysDev gave a cryptic error message - Invalid physical device. Line 10: The message was amended to Multipath cannot access storage device along Line 11: with the device name. The error key was changed according to the new I partially agree - the sentence can be improved, but there is merit in explaining that it's OK to rename the exception as long as long as the error code is unchanged with breaking breaking backwards compatibility with older engines. I would, however, use the term oVirt Engine instead of just engine, for clarity's sake. Line 12: message content. The only thing needed here by the engine is the Line 13: error code, which was not changed in this patch. Line 14: Line 15: Change-Id: I648ee519873c51573e6e6306b79380f54bb25d2e -- To view, visit http://gerrit.ovirt.org/21089 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I648ee519873c51573e6e6306b79380f54bb25d2e Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Vered Volansky vvola...@redhat.com Gerrit-Reviewer: Allon Mureinik amure...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Vered Volansky vvola...@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
