New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pybind: better error msg #14497

Merged
merged 2 commits into from May 2, 2017

Conversation

Projects
None yet
4 participants
@tchaikov
Contributor

tchaikov commented Apr 13, 2017

No description provided.

@tchaikov tchaikov requested a review from jdurgin Apr 13, 2017

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Apr 13, 2017

@jdurgin i found

    }
}

error calling ping_monitor
/home/jenkins-build/build/workspace/ceph-pull-requests/qa/workunits/cephtool/test.sh:1: test_mon_ping:  rm -fr /tmp/cephtool.DVB

in https://jenkins.ceph.com/job/ceph-pull-requests/21928/consoleFull#-15585949898b690f2e-7cab-492c-ad9c-1c25ca39eafa

but the error message is not helpful enough. hence this change.

@@ -238,9 +239,9 @@ cdef make_ex(ret, msg):
"""
ret = abs(ret)
if ret in errno_to_exception:
return errno_to_exception[ret](msg)
return errno_to_exception[ret](ret, msg)

This comment has been minimized.

@jdurgin

jdurgin Apr 13, 2017

Member

how about adding the error code to msg here instead of adding it as another arg, e.g. using msg + (": error code %d" % ret) like the Error() construction below does already?

Passing multiple args to Exception.init results in them being printed as a tuple:

>>> import rados
>>> print rados.Error(22, 'an error occurred')
(22, 'an error occurred')

which looks odd to an end user

This comment has been minimized.

@tchaikov

tchaikov Apr 13, 2017

Contributor

this is on purpose. i am trying to model after OSError in py 3.3+. and IMHO, (22, 'an error occurred') is easier to read for a developer, than an error occurred: error code 22. the reason i want to have a better error message, is for ease of debugging. ideally, we should catch an exception and handle it properly instead of printing it directly. so i think it's okay.

This comment has been minimized.

@jdurgin

jdurgin Apr 17, 2017

Member

I'm not convinced many callers will handle it specially - our own ceph cli just prints the exception for example. Callers won't always catch these, so I think we should be optimizing for the user, rather than the developer here.

We could make Error inherit from EnvironmentError instead, which will use your errno and strerror attributes:

>>> class Error(EnvironmentError):
...     pass
... 
>>> print(Error(22, 'bad input foo'))
[Errno 22] bad input foo

This comment has been minimized.

@tchaikov

tchaikov Apr 18, 2017

Contributor

great, will do. fwiw, EnvironmentError is an alias of OSError in py3.3+.

@@ -150,7 +148,10 @@ cdef extern from "cephfs/libcephfs.h" nogil:
class Error(Exception):
pass
def __init__(self, errno, strerror):
super().__init__(errno, strerror)

This comment has been minimized.

@jdurgin

jdurgin Apr 13, 2017

Member

I was surprised to see super with no arguments work - cython transforms it so it works with python2

This comment has been minimized.

@tchaikov

tchaikov Apr 13, 2017

Contributor

super() works for new style class. and i think Exception is. i tested a minimal reproducer using python2.7 (not cython) and it works.

pybind: use sys.version_info for checking py3
it's simpler and more consistent with other part of ceph.

Signed-off-by: Kefu Chai <kchai@redhat.com>
@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Apr 18, 2017

@jdurgin fixed and repushed.

@liewegas

This comment has been minimized.

Member

liewegas commented Apr 19, 2017

2017-04-19T17:32:22.230 INFO:tasks.workunit.client.0.smithi045.stderr:======================================================================
2017-04-19T17:32:22.230 INFO:tasks.workunit.client.0.smithi045.stderr:ERROR: test_rados.TestRadosStateError.test_configuring
2017-04-19T17:32:22.230 INFO:tasks.workunit.client.0.smithi045.stderr:----------------------------------------------------------------------
2017-04-19T17:32:22.230 INFO:tasks.workunit.client.0.smithi045.stderr:Traceback (most recent call last):
2017-04-19T17:32:22.230 INFO:tasks.workunit.client.0.smithi045.stderr:  File "/usr/lib/python2.7/dist-packages/nose/case.py", line 197, in runTest
2017-04-19T17:32:22.230 INFO:tasks.workunit.client.0.smithi045.stderr:    self.test(*self.arg)
2017-04-19T17:32:22.230 INFO:tasks.workunit.client.0.smithi045.stderr:  File "/home/ubuntu/cephtest/clone.client.0/src/test/pybind/test_rados.py", line 109, in test_configuring
2017-04-19T17:32:22.231 INFO:tasks.workunit.client.0.smithi045.stderr:    self._requires_connected(rados)
2017-04-19T17:32:22.231 INFO:tasks.workunit.client.0.smithi045.stderr:  File "/home/ubuntu/cephtest/clone.client.0/src/test/pybind/test_rados.py", line 91, in _requires_connected
2017-04-19T17:32:22.231 INFO:tasks.workunit.client.0.smithi045.stderr:    assert_raises(RadosStateError, rados.pool_exists, 'foo')
2017-04-19T17:32:22.231 INFO:tasks.workunit.client.0.smithi045.stderr:  File "/usr/lib/python2.7/unittest/case.py", line 475, in assertRaises
2017-04-19T17:32:22.231 INFO:tasks.workunit.client.0.smithi045.stderr:    callableObj(*args, **kwargs)
2017-04-19T17:32:22.231 INFO:tasks.workunit.client.0.smithi045.stderr:  File "rados.pyx", line 466, in rados.requires.wrapper.validate_func (/build/ceph-12.0.0-2827-g8f6cef9/obj-x86_64-linux-gnu/src/pybind/rados/pyrex/rados.c:4576)
2017-04-19T17:32:22.231 INFO:tasks.workunit.client.0.smithi045.stderr:  File "rados.pyx", line 873, in rados.Rados.pool_exists (/build/ceph-12.0.0-2827-g8f6cef9/obj-x86_64-linux-gnu/src/pybind/rados/pyrex/rados.c:9908)
2017-04-19T17:32:22.231 INFO:tasks.workunit.client.0.smithi045.stderr:  File "rados.pyx", line 622, in rados.Rados.require_state (/build/ceph-12.0.0-2827-g8f6cef9/obj-x86_64-linux-gnu/src/pybind/rados/pyrex/rados.c:7192)
2017-04-19T17:32:22.231 INFO:tasks.workunit.client.0.smithi045.stderr:  File "rados.pyx", line 300, in rados.Error.__init__ (/build/ceph-12.0.0-2827-g8f6cef9/obj-x86_64-linux-gnu/src/pybind/rados/pyrex/rados.c:2889)
2017-04-19T17:32:22.231 INFO:tasks.workunit.client.0.smithi045.stderr:TypeError: __init__() takes exactly 3 positional arguments (2 given)
2017-04-19T17:32:22.231 INFO:tasks.workunit.client.0.smithi045.stderr:
2017-04-19T17:32:22.232 INFO:tasks.workunit.client.0.smithi045.stderr:======================================================================
2017-04-19T17:32:22.232 INFO:tasks.workunit.client.0.smithi045.stderr:ERROR: test_rados.TestRadosStateError.test_connected
2017-04-19T17:32:22.232 INFO:tasks.workunit.client.0.smithi045.stderr:----------------------------------------------------------------------
2017-04-19T17:32:22.232 INFO:tasks.workunit.client.0.smithi045.stderr:Traceback (most recent call last):
2017-04-19T17:32:22.232 INFO:tasks.workunit.client.0.smithi045.stderr:  File "/usr/lib/python2.7/dist-packages/nose/case.py", line 197, in runTest
2017-04-19T17:32:22.232 INFO:tasks.workunit.client.0.smithi045.stderr:    self.test(*self.arg)
2017-04-19T17:32:22.232 INFO:tasks.workunit.client.0.smithi045.stderr:  File "/home/ubuntu/cephtest/clone.client.0/src/test/pybind/test_rados.py", line 115, in test_connected
2017-04-19T17:32:22.232 INFO:tasks.workunit.client.0.smithi045.stderr:    self._requires_configuring(rados)
2017-04-19T17:32:22.232 INFO:tasks.workunit.client.0.smithi045.stderr:  File "/home/ubuntu/cephtest/clone.client.0/src/test/pybind/test_rados.py", line 80, in _requires_configuring
2017-04-19T17:32:22.232 INFO:tasks.workunit.client.0.smithi045.stderr:    assert_raises(RadosStateError, rados.connect)
2017-04-19T17:32:22.233 INFO:tasks.workunit.client.0.smithi045.stderr:  File "/usr/lib/python2.7/unittest/case.py", line 475, in assertRaises
2017-04-19T17:32:22.233 INFO:tasks.workunit.client.0.smithi045.stderr:    callableObj(*args, **kwargs)
2017-04-19T17:32:22.233 INFO:tasks.workunit.client.0.smithi045.stderr:  File "rados.pyx", line 819, in rados.Rados.connect (/build/ceph-12.0.0-2827-g8f6cef9/obj-x86_64-linux-gnu/src/pybind/rados/pyrex/rados.c:9561)
2017-04-19T17:32:22.233 INFO:tasks.workunit.client.0.smithi045.stderr:  File "rados.pyx", line 622, in rados.Rados.require_state (/build/ceph-12.0.0-2827-g8f6cef9/obj-x86_64-linux-gnu/src/pybind/rados/pyrex/rados.c:7192)
2017-04-19T17:32:22.233 INFO:tasks.workunit.client.0.smithi045.stderr:  File "rados.pyx", line 300, in rados.Error.__init__ (/build/ceph-12.0.0-2827-g8f6cef9/obj-x86_64-linux-gnu/src/pybind/rados/pyrex/rados.c:2889)
2017-04-19T17:32:22.233 INFO:tasks.workunit.client.0.smithi045.stderr:TypeError: __init__() takes exactly 3 positional arguments (2 given)
2017-04-19T17:32:22.233 INFO:tasks.workunit.client.0.smithi045.stderr:
2017-04-19T17:32:22.233 INFO:tasks.workunit.client.0.smithi045.stderr:======================================================================
2017-04-19T17:32:22.233 INFO:tasks.workunit.client.0.smithi045.stderr:ERROR: test_rados.TestRadosStateError.test_shutdown
2017-04-19T17:32:22.233 INFO:tasks.workunit.client.0.smithi045.stderr:----------------------------------------------------------------------
2017-04-19T17:32:22.234 INFO:tasks.workunit.client.0.smithi045.stderr:Traceback (most recent call last):
2017-04-19T17:32:22.234 INFO:tasks.workunit.client.0.smithi045.stderr:  File "/usr/lib/python2.7/dist-packages/nose/case.py", line 197, in runTest
2017-04-19T17:32:22.234 INFO:tasks.workunit.client.0.smithi045.stderr:    self.test(*self.arg)
2017-04-19T17:32:22.234 INFO:tasks.workunit.client.0.smithi045.stderr:  File "/home/ubuntu/cephtest/clone.client.0/src/test/pybind/test_rados.py", line 122, in test_shutdown
2017-04-19T17:32:22.234 INFO:tasks.workunit.client.0.smithi045.stderr:    self._requires_configuring(rados)
2017-04-19T17:32:22.234 INFO:tasks.workunit.client.0.smithi045.stderr:  File "/home/ubuntu/cephtest/clone.client.0/src/test/pybind/test_rados.py", line 80, in _requires_configuring
2017-04-19T17:32:22.234 INFO:tasks.workunit.client.0.smithi045.stderr:    assert_raises(RadosStateError, rados.connect)
2017-04-19T17:32:22.234 INFO:tasks.workunit.client.0.smithi045.stderr:  File "/usr/lib/python2.7/unittest/case.py", line 475, in assertRaises
2017-04-19T17:32:22.234 INFO:tasks.workunit.client.0.smithi045.stderr:    callableObj(*args, **kwargs)
2017-04-19T17:32:22.235 INFO:tasks.workunit.client.0.smithi045.stderr:  File "rados.pyx", line 819, in rados.Rados.connect (/build/ceph-12.0.0-2827-g8f6cef9/obj-x86_64-linux-gnu/src/pybind/rados/pyrex/rados.c:9561)
2017-04-19T17:32:22.235 INFO:tasks.workunit.client.0.smithi045.stderr:  File "rados.pyx", line 622, in rados.Rados.require_state (/build/ceph-12.0.0-2827-g8f6cef9/obj-x86_64-linux-gnu/src/pybind/rados/pyrex/rados.c:7192)
2017-04-19T17:32:22.235 INFO:tasks.workunit.client.0.smithi045.stderr:  File "rados.pyx", line 300, in rados.Error.__init__ (/build/ceph-12.0.0-2827-g8f6cef9/obj-x86_64-linux-gnu/src/pybind/rados/pyrex/rados.c:2889)
2017-04-19T17:32:22.235 INFO:tasks.workunit.client.0.smithi045.stderr:TypeError: __init__() takes exactly 3 positional arguments (2 given)
2017-04-19T17:32:22.235 INFO:tasks.workunit.client.0.smithi045.stderr:
2017-04-19T17:32:22.235 INFO:tasks.workunit.client.0.smithi045.stderr:======================================================================
2017-04-19T17:32:22.235 INFO:tasks.workunit.client.0.smithi045.stderr:ERROR: test_rados.test_rados_init_error
2017-04-19T17:32:22.235 INFO:tasks.workunit.client.0.smithi045.stderr:----------------------------------------------------------------------
2017-04-19T17:32:22.235 INFO:tasks.workunit.client.0.smithi045.stderr:Traceback (most recent call last):
2017-04-19T17:32:22.235 INFO:tasks.workunit.client.0.smithi045.stderr:  File "/usr/lib/python2.7/dist-packages/nose/case.py", line 197, in runTest
2017-04-19T17:32:22.235 INFO:tasks.workunit.client.0.smithi045.stderr:    self.test(*self.arg)
2017-04-19T17:32:22.236 INFO:tasks.workunit.client.0.smithi045.stderr:  File "/home/ubuntu/cephtest/clone.client.0/src/test/pybind/test_rados.py", line 18, in test_rados_init_error
2017-04-19T17:32:22.236 INFO:tasks.workunit.client.0.smithi045.stderr:    name='client.admin')
2017-04-19T17:32:22.236 INFO:tasks.workunit.client.0.smithi045.stderr:  File "/usr/lib/python2.7/unittest/case.py", line 475, in assertRaises
2017-04-19T17:32:22.236 INFO:tasks.workunit.client.0.smithi045.stderr:    callableObj(*args, **kwargs)
2017-04-19T17:32:22.236 INFO:tasks.workunit.client.0.smithi045.stderr:  File "rados.pyx", line 566, in rados.Rados.__init__ (/build/ceph-12.0.0-2827-g8f6cef9/obj-x86_64-linux-gnu/src/pybind/rados/pyrex/rados.c:6183)
2017-04-19T17:32:22.236 INFO:tasks.workunit.client.0.smithi045.stderr:  File "rados.pyx", line 466, in rados.requires.wrapper.validate_func (/build/ceph-12.0.0-2827-g8f6cef9/obj-x86_64-linux-gnu/src/pybind/rados/pyrex/rados.c:4576)
2017-04-19T17:32:22.236 INFO:tasks.workunit.client.0.smithi045.stderr:  File "rados.pyx", line 579, in rados.Rados.__setup (/build/ceph-12.0.0-2827-g8f6cef9/obj-x86_64-linux-gnu/src/pybind/rados/pyrex/rados.c:6477)
2017-04-19T17:32:22.236 INFO:tasks.workunit.client.0.smithi045.stderr:  File "rados.pyx", line 300, in rados.Error.__init__ (/build/ceph-12.0.0-2827-g8f6cef9/obj-x86_64-linux-gnu/src/pybind/rados/pyrex/rados.c:2889)
2017-04-19T17:32:22.236 INFO:tasks.workunit.client.0.smithi045.stderr:TypeError: __init__() takes exactly 3 positional arguments (2 given)

/a/yuriw-2017-04-19_16:55:52-rados-wip-sage-testing2_2017_4_20_2-distro-basic-smithi/1045006

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Apr 20, 2017

retest this please.

pybind: store errno in Error exception also
also pass the errno and error string to Exception's __init__()
so we can print them all using str(err). otherwise str(err) will only
print out the errorstr, and the type information is lost. for example,
if ping_monitor() raises an exception, all we get is an error of:

  error calling ping_monitor

which is barely helpful. after this change:

  [Errno 22] error calling ping_monitor

is printed instead.

Signed-off-by: Kefu Chai <kchai@redhat.com>
@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Apr 20, 2017

retest this please

@yuriw yuriw merged commit 0fd1689 into ceph:master May 2, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@tchaikov tchaikov deleted the tchaikov:wip-pybind-better-error-msg branch May 2, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment