Open Stack

On 07/11/2013 06:22 AM, Sean Dague wrote:
> On 07/11/2013 05:43 AM, Thomas Hervé wrote:
>>
>>
>> On Thu, Jul 11, 2013 at 1:49 AM, Monty Taylor <mordred at inaugust.com
>> <mailto:mordred at inaugust.com>> wrote:
>>
>>     I'd like top-post and hijack this thread for another exception
>> related
>>     thing:
>>
>>     a) Anyone writing code such as:
>>
>>     try:
>>        blah()
>>     except SomeException:
>>        raise SomeOtherExceptionLeavingOutStackContextFromSomeException
>>
>>     should be mocked ruthlessly.
>>
>>
>> i don't think mocking is a good way to convey your point. Losing
>> tracebacks is not great, but having an API raising random exceptions is
>> probably worse. Can you develop?
> 
> We have defined APIs. We have server projects that call other server
> projects through python-fooclients. We have python-fooclients that
> generate exceptions and stack traces on any non 20X return.
> 
> So we have a general exception translation issue. Because unless we want
> nova-api returns to be completely fluid based on what
> keystone/neutron/glance/cinder clients decided their exceptions are this
> week, you have to translate. And the stack trace from a 404 isn't
> something that we really care about from the client, we care that it
> happened, because that affects nova logic, but it's an expected exception.

It's an expected exception until it's not.

> And where would we even put the stack trace? take it to the user on
> their API call?

Oh gosh no! I'm CERTAINLY not suggesting that we passthrough translated
exceptions to our REST API consumers. I'm talking about actual
exceptions in server code which generate actual error logging because
they are actual errors but where you cannot find what the error is
because of masking.

> fill up the logs with stack traces for normal events
> (thus hiding real errors)? (we actually have some blueprints openned now
> to remove these because a *normal* passing tempest run generates 30+
> stack traces in nova logs, which aren't actually internal errors.)

Yes. This is actually much worse, and I fully support you in all of
these efforts!

> The bulk of these cases I've seen in Nova are exactly of that nature. We
> actually have a pretty standard pattern of 3 levels of exception
> translations for user API calls.
> 
> Underlying client exception (be it DB / python-fooclient) -> Nova
> internal exception -> Webobj exception to create our 40x / 50x returns.

I believe striping the underlying exception in the webobj translation is
perfectly reasonable. The alternative would be madness.

I honestly can't find a great example of this in our code this second (I
think concrete examples are great) but I know that I've hit it enough to
lodge an annoyance flag - so let me pull an example from the insides of
our friend d2to1:

    try:
        attrs = cfg_to_args(path)
    except:
        e = sys.exc_info()[1]
        raise DistutilsSetupError(
            'Error parsing %s: %s: %s' % (path, e.__class__.__name__,
                                          e.args[0]))

This is an attempt to give the user a meaningful error in the case that
they have a problem. So - first thing is, there's a bug, becuase
e.args[0] is the error code, so you get:

error in setup command: Error parsing setup.cfg: IOError: 2

Which is TOTALLY meaningless.

If you change the string line to:

+            'Error parsing %s: %s: %s' % (path, e.__class__.__name__, e))

You at least get:

error in setup command: Error parsing setup.cfg: IOError: No such file
or directory: 'README'

Which is much more helpful. However, that's a simple error, if you have
a more complex error, such as your pbr hook threw a traceback while
loading, it's a bit bonghits and to track it down you may have to expend
some effort. BUT - for the general case, you have a typo in your
setup.cfg file, IOError: No such file or directory: 'README' is probably
sufficient and you don't probably need the traceback.

That said - when I'm having problems using this code, I tend to just
edit that file, remove that exception wrapper altogether, and let the
exceptions fly- and can usually find the problem in about a second.
Maybe I should add a debug flag which skips the wrapping...