Re: [PATCH v3 2/4] python/machine: raise VMLaunchFailure exception from launch()

[John Snow]

On Thu, Jan 27, 2022 at 9:22 AM Hanna Reitz <hreitz@redhat.com> wrote:
>
> On 24.01.22 19:06, John Snow wrote:
> > This allows us to pack in some extra information about the failure,
> > which guarantees that if the caller did not *intentionally* cause a
> > failure (by capturing this Exception), some pretty good clues will be
> > printed at the bottom of the traceback information.
>
> OK, I presume in contrast to unconditionally logging this on debug
> level, which is less than ideal because on that level it’s most likely
> hidden, but that was exactly the point, because we don’t know whether
> the caller will catch the exception, so we mustn’t log it on a more
> urgent level.
>

Exactly. More urgent logging interferes with tests where we
intentionally give a bad configuration. device-crash-test is another
example.

> But by creating a new exception class, we get a reasonable log output
> exactly when the caller won’t catch it.
>

That's the intent. By stuffing this info into the Exception, we'll
always see it printed if the error went unhandled. It seemed like the
best way to make sure the error messages were more apparent more often
without requiring the use of debug mode -- so that errors in e.g.
GitLab CI would print good diagnostic info by default.

> > This will help make failures in the event of a non-negative return code
> > more obvious when they go unhandled; the current behavior is to print a
> > warning message only in the event of signal-based terminations (for
> > negative return codes).
>
> I assume you mean the one in _post_shutdown()...?
>

Yes.

> Confused me a bit, because for a while I interpreted this to mean “We
> don’t output anything in case of a positive return code”, but it means
> “We don’t print any details in that case, because the exception we
> re-raise in launch() doesn’t contain valuable information”.  Makes sense.
>

Sorry, I'll improve the commit message.

> > (Note: In Python, catching BaseException instead of Exception is like
> > installing a signal handler that will run as long as Python itself
> > doesn't crash.
>
> This really confused me, because I can’t really understand this at all.
>
> But I guess what I took from googling was that every exception object
> must be derived from BaseException eventually, and so we continue to
> catch all exceptions here, we just give them a name. (And then we create
> a VMLaunchFailure only for Exception exceptions, because the others
> don’t have much to do with launching the VM.)
>

Apologies for not being more clear. ("It made sense to me at the time
...") What I mean to say here is: there are several ways to catch all
exceptions.

"except:" will catch everything.
"except BaseException" catches everything, too. This is equivalent to the above.
"except Exception" won't catch anything that inherits directly from
BaseException, only Exception and children.

What I wanted to convey here is that:

(1) If the exception is a BaseException, it's probably something like
KeyboardInterrupt (SIGINT) or SystemExit, we don't want to wrap the
exception and instead we want to re-raise it as-is. We are functioning
more or less like a signal handler here, performing some cleanup and
then yielding back control.
(2) If the exception is merely an Exception, it's OK to wrap it with
the custom exception and re-raise.

Wrapping a BaseException would be a problem because it would
'downgrade' the severity of the exception (so to speak) and may cause
issues.
I'll try to improve the commit message.

> > KeyboardInterrupt and several other "strong" events in
> > Python are a BaseException. These events should generally not be
> > suppressed, but occasionally we want to perform some cleanup in response
> > to one.)
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   python/qemu/machine/machine.py            | 45 ++++++++++++++++++++---
> >   tests/qemu-iotests/tests/mirror-top-perms |  3 +-
> >   2 files changed, 40 insertions(+), 8 deletions(-)
>
> Reviewed-by: Hanna Reitz <hreitz@redhat.com>
>
> (Looked at `except` and `ConnectError` usage outside of
> mirror-top-perms, but couldn’t find anything else that looked like it
> caught VM launch exceptions.)
>

Thanks!